Re: [dpdk-dev] [PATCH] eal: fix ctrl thread affinity with --lcores

["Johan Källström" <johan.kallstrom@ericsson.com>]

See inline comments about not using cpuset for "thread affinity" and possible online cpu failsafe to detect if the thread affinity mask is not a subset of online cpus.
This feature was present before your suggested change.

The CPU failsafe is nice to have as you could set the thread affinity to offline cpus.

Maybe also add the example I gave you to trigger the bug? https://bugs.dpdk.org/show_bug.cgi?id=322#c12
This also shows how to set the default_affinity mask and proves that the calculation will result in threads inside the cpuset on Linux.

/Johan

On tis, 2019-07-30 at 11:35 +0200, David Marchand wrote:
> When using -l/-c options, each lcore is mapped to a physical cpu in a
> 1:1 fashion.
> On the contrary, when using --lcores, each lcore has its own cpuset 

Use "thread affinity" instead of cpuset when we talk about setting the thread affinity.

I know that the term cpuset is used in the data structure, but it is not a cpuset as described by 'man cpuset' (on Linux). This comment can be seen as cosmetic, but I think that it could be good to have a clear definitions to minimize confusion.

> on
> which the associated EAL thread runs.
> 
> To handle those two situations, rely on the per lcore cpuset.
> 
> Introduced macros to manipulate cpusets in both Linux and FreeBSD.
> 
> Examples in a 4 cores FreeBSD vm:
> 
> $ ./build/app/testpmd --master-lcore 1 --lcores '0@(1,3),mailto:1@2' \
>  --no-huge --no-pci -m 512 -- -i --total-num-mbufs=2048
> 
>   PID    TID COMM                TDNAME              CPU CSID CPU
> MASK
> 31733 100155 testpmd             -                     2    1 2
> 31733 100286 testpmd             eal-intr-thread       0    1 0
> 31733 100287 testpmd             rte_mp_handle         0    1 0
> 31733 100288 testpmd             lcore-slave-0         3    1 1,3
> 
> $ cpuset -l 1,2,3 \
>  ./build/app/testpmd --master-lcore 1 --lcores '0@(1,3),mailto:1@2' \
>  --no-huge --no-pci -m 512 -- -i --total-num-mbufs=2048
> 
>   PID    TID COMM                TDNAME              CPU CSID CPU
> MASK
> 31757 100139 testpmd             -                     2    2 2
> 31757 100292 testpmd             eal-intr-thread       2    2 2
> 31757 100293 testpmd             rte_mp_handle         2    2 2
> 31757 100294 testpmd             lcore-slave-0         3    2 1,3
> 
> $ cpuset -l 1,2,3 \
>  ./build/app/testpmd --master-lcore 1 --lcores mailto:'0@1,mailto:1@2' \
>  --no-huge --no-pci -m 512 -- -i --total-num-mbufs=2048
> 
>   PID    TID COMM                TDNAME              CPU CSID CPU
> MASK
> 31776 100166 testpmd             -                     2    2 2
> 31776 100295 testpmd             eal-intr-thread       3    2 3
> 31776 100296 testpmd             rte_mp_handle         3    2 3
> 31776 100297 testpmd             lcore-slave-0         1    2 1
> 
> Bugzilla ID: 322
> Fixes: c3568ea37670 ("eal: restrict control threads to startup CPU
> affinity")
> Cc: mailto:stable@dpdk.org
> 
> Signed-off-by: David Marchand <mailto:david.marchand@redhat.com>
> ---
>  lib/librte_eal/common/eal_common_options.c | 16 +++++++++-------
>  lib/librte_eal/common/include/rte_lcore.h  | 28
> ++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_options.c
> b/lib/librte_eal/common/eal_common_options.c
> index 24e36cf..d828271 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -1455,11 +1455,11 @@ compute_ctrl_threads_cpuset(struct
> internal_config *internal_cfg)
>  	unsigned int lcore_id;
>  CSB B                                                              
>                                                                      
>                              

+       /* Get online cpus */
+       CPU_ZERO(&cset_online);
+       for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
+               if (eal_cpu_detected(lcore_id))
+                       CPU_SET(lcore_id, &cset_online);
+       }

>                                                                    
>                                                      
>  	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
> -		if (eal_cpu_detected(lcore_id) &&
> -				rte_lcore_has_role(lcore_id,
> ROLE_OFF)) {
> -			CPU_SET(lcore_id, cpuset);
> -		}
> +		if (rte_lcore_has_role(lcore_id, ROLE_OFF))
> +			continue;
> +		RTE_CPU_OR(cpuset, cpuset,
> &lcore_config[lcore_id].cpuset);
>  	}
> +	RTE_CPU_NOT(cpuset, cpuset);
>  
>  	if (pthread_getaffinity_np(pthread_self(),
> sizeof(rte_cpuset_t),
>  				&default_set))
> @@ -1467,9 +1467,11 @@ compute_ctrl_threads_cpuset(struct
> internal_config *internal_cfg)
>  
>  	RTE_CPU_AND(cpuset, cpuset, &default_set);
+ 	RTE_CPU_AND(cpuset, cpuset, &cset_online);
>  
> -	/* if no detected CPU is off, use master core */
> -	if (!CPU_COUNT(cpuset))
> -		CPU_SET(rte_get_master_lcore(), cpuset);
> +	/* if no detected CPU is off, use master lcore cpuset */
> +	if (!CPU_COUNT(cpuset)) {
> +		memcpy(cpuset,
> &lcore_config[rte_get_master_lcore()].cpuset,
> +			sizeof(*cpuset));
> +	}
>  }
>  
>  int
> diff --git a/lib/librte_eal/common/include/rte_lcore.h
> b/lib/librte_eal/common/include/rte_lcore.h
> index 411df30..9520d79 100644
> --- a/lib/librte_eal/common/include/rte_lcore.h
> +++ b/lib/librte_eal/common/include/rte_lcore.h
> @@ -25,6 +25,19 @@ extern "C" {
>  #if defined(__linux__)
>  typedef	cpu_set_t rte_cpuset_t;
>  #define RTE_CPU_AND(dst, src1, src2) CPU_AND(dst, src1, src2)
> +#define RTE_CPU_OR(dst, src1, src2) CPU_OR(dst, src1, src2)
> +#define RTE_CPU_FILL(set) do \
> +{ \
> +	unsigned int i; \
> +	for (i = 0; i < CPU_SETSIZE; i++) \
> +		CPU_SET(i, set); \
> +} while (0)
> +#define RTE_CPU_NOT(dst, src) do \
> +{ \
> +	cpu_set_t tmp; \
> +	RTE_CPU_FILL(&tmp); \
> +	CPU_XOR(dst, &tmp, src); \
> +} while (0)
>  #elif defined(__FreeBSD__)
>  #include <pthread_np.h>
>  typedef cpuset_t rte_cpuset_t;
> @@ -35,6 +48,21 @@ typedef cpuset_t rte_cpuset_t;
>  	CPU_AND(&tmp, src2); \
>  	CPU_COPY(&tmp, dst); \
>  } while (0)
> +#define RTE_CPU_OR(dst, src1, src2) do \
> +{ \
> +	cpuset_t tmp; \
> +	CPU_COPY(src1, &tmp); \
> +	CPU_OR(&tmp, src2); \
> +	CPU_COPY(&tmp, dst); \
> +} while (0)
> +#define RTE_CPU_FILL(set) CPU_FILL(set)
> +#define RTE_CPU_NOT(dst, src) do \
> +{ \
> +	cpuset_t tmp; \
> +	CPU_FILL(&tmp); \
> +	CPU_NAND(&tmp, src); \
> +	CPU_COPY(&tmp, dst); \
> +} while (0)
>  #endif
>  
>  /**