DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Johan Källström" <johan.kallstrom@ericsson.com>
To: David Marchand <david.marchand@redhat.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Cc: "anatoly.burakov@intel.com" <anatoly.burakov@intel.com>,
	"olivier.matz@6wind.com" <olivier.matz@6wind.com>,
	"stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] eal: fix ctrl thread affinity with --lcores
Date: Tue, 30 Jul 2019 11:38:25 +0000	[thread overview]
Message-ID: <HE1PR0701MB21534A0C449AA1E509016F8098DC0@HE1PR0701MB2153.eurprd07.prod.outlook.com> (raw)
In-Reply-To: <1564479354-11192-1-git-send-email-david.marchand@redhat.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
>  
>  /**

  parent reply	other threads:[~2019-07-30 12:03 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-30  9:35 David Marchand
2019-07-30  9:45 ` Jerin Jacob Kollanukkaran
2019-07-30  9:46   ` David Marchand
2019-07-30 11:38 ` Johan Källström [this message]
2019-07-30 13:47   ` David Marchand
2019-07-30 16:32     ` Johan Källström
2019-07-30 19:21       ` David Marchand
2019-07-31  8:12         ` Johan Källström
2019-07-30 15:05 ` [dpdk-dev] [PATCH v2] " David Marchand
2019-07-30 21:12   ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=HE1PR0701MB21534A0C449AA1E509016F8098DC0@HE1PR0701MB2153.eurprd07.prod.outlook.com \
    --to=johan.kallstrom@ericsson.com \
    --cc=anatoly.burakov@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=olivier.matz@6wind.com \
    --cc=stable@dpdk.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).