diff mbox

[dpdk-dev,v4,01/17] eal: add cpuset into per EAL thread lcore_config

Message ID 1422842559-13617-2-git-send-email-cunming.liang@intel.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Cunming Liang Feb. 2, 2015, 2:02 a.m. UTC
The patch adds 'cpuset' into per-lcore configure 'lcore_config[]',
as the lcore no longer always 1:1 pinning with physical cpu.
The lcore now stands for a EAL thread rather than a logical cpu.

It doesn't change the default behavior of 1:1 mapping, but allows to
affinity the EAL thread to multiple cpus.

Signed-off-by: Cunming Liang <cunming.liang@intel.com>
---
 lib/librte_eal/bsdapp/eal/eal_lcore.c     | 7 +++++++
 lib/librte_eal/bsdapp/eal/eal_memory.c    | 2 ++
 lib/librte_eal/common/include/rte_lcore.h | 8 ++++++++
 lib/librte_eal/linuxapp/eal/Makefile      | 1 +
 lib/librte_eal/linuxapp/eal/eal_lcore.c   | 8 ++++++++
 5 files changed, 26 insertions(+)

Comments

Olivier Matz Feb. 8, 2015, 7:59 p.m. UTC | #1
Hi,

On 02/02/2015 03:02 AM, Cunming Liang wrote:
> The patch adds 'cpuset' into per-lcore configure 'lcore_config[]',
> as the lcore no longer always 1:1 pinning with physical cpu.
> The lcore now stands for a EAL thread rather than a logical cpu.
> 
> It doesn't change the default behavior of 1:1 mapping, but allows to
> affinity the EAL thread to multiple cpus.
> 
> [...]
> diff --git a/lib/librte_eal/bsdapp/eal/eal_memory.c b/lib/librte_eal/bsdapp/eal/eal_memory.c
> index 65ee87d..a34d500 100644
> --- a/lib/librte_eal/bsdapp/eal/eal_memory.c
> +++ b/lib/librte_eal/bsdapp/eal/eal_memory.c
> @@ -45,6 +45,8 @@
>  #include "eal_internal_cfg.h"
>  #include "eal_filesystem.h"
>  
> +/* avoid re-defined against with freebsd header */
> +#undef PAGE_SIZE
>  #define PAGE_SIZE (sysconf(_SC_PAGESIZE))

I don't see the link with the patch. Should this go somewhere else?


>  
>  /*
> diff --git a/lib/librte_eal/common/include/rte_lcore.h b/lib/librte_eal/common/include/rte_lcore.h
> index 49b2c03..4c7d6bb 100644
> --- a/lib/librte_eal/common/include/rte_lcore.h
> +++ b/lib/librte_eal/common/include/rte_lcore.h
> @@ -50,6 +50,13 @@ extern "C" {
>  
>  #define LCORE_ID_ANY -1    /**< Any lcore. */
>  
> +#if defined(__linux__)
> +	typedef	cpu_set_t rte_cpuset_t;
> +#elif defined(__FreeBSD__)
> +#include <pthread_np.h>
> +	typedef cpuset_t rte_cpuset_t;
> +#endif
> +

Should we also define RTE_CPU_SETSIZE?
For linux, should <sched.h> be included?

If I understand well, after the patch series, the user of
rte_thread_set_affinity() and rte_thread_get_affinity() are
supposed to use the macros from sched.h to access to this
cpuset parameter. So I'm wondering if it's not better to
use cpu_set_t from libc instead of redefining rte_cpuset_t.

To reword my question: what is the purpose of redefining
cpu_set_t in rte_cpuset_t if we still need to use all the
libc API to access to it?


Regards,
Olivier
Cunming Liang Feb. 9, 2015, 11:33 a.m. UTC | #2
> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Monday, February 09, 2015 4:00 AM
> To: Liang, Cunming; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 01/17] eal: add cpuset into per EAL thread
> lcore_config
> 
> Hi,
> 
> On 02/02/2015 03:02 AM, Cunming Liang wrote:
> > The patch adds 'cpuset' into per-lcore configure 'lcore_config[]',
> > as the lcore no longer always 1:1 pinning with physical cpu.
> > The lcore now stands for a EAL thread rather than a logical cpu.
> >
> > It doesn't change the default behavior of 1:1 mapping, but allows to
> > affinity the EAL thread to multiple cpus.
> >
> > [...]
> > diff --git a/lib/librte_eal/bsdapp/eal/eal_memory.c
> b/lib/librte_eal/bsdapp/eal/eal_memory.c
> > index 65ee87d..a34d500 100644
> > --- a/lib/librte_eal/bsdapp/eal/eal_memory.c
> > +++ b/lib/librte_eal/bsdapp/eal/eal_memory.c
> > @@ -45,6 +45,8 @@
> >  #include "eal_internal_cfg.h"
> >  #include "eal_filesystem.h"
> >
> > +/* avoid re-defined against with freebsd header */
> > +#undef PAGE_SIZE
> >  #define PAGE_SIZE (sysconf(_SC_PAGESIZE))
> 
> I don't see the link with the patch. Should this go somewhere else?
> 
> 
> >
> >  /*
> > diff --git a/lib/librte_eal/common/include/rte_lcore.h
> b/lib/librte_eal/common/include/rte_lcore.h
> > index 49b2c03..4c7d6bb 100644
> > --- a/lib/librte_eal/common/include/rte_lcore.h
> > +++ b/lib/librte_eal/common/include/rte_lcore.h
> > @@ -50,6 +50,13 @@ extern "C" {
> >
> >  #define LCORE_ID_ANY -1    /**< Any lcore. */
> >
> > +#if defined(__linux__)
> > +	typedef	cpu_set_t rte_cpuset_t;
> > +#elif defined(__FreeBSD__)
> > +#include <pthread_np.h>
> > +	typedef cpuset_t rte_cpuset_t;
> > +#endif
> > +
> 
> Should we also define RTE_CPU_SETSIZE?
> For linux, should <sched.h> be included?
[LCM] It uses the fix size cpuset, won't use CPU_ALLOC() to get the pointer of cpuset.
The RTE_CPU_SETSIZE always equal to sizeof(rte_cpuset_t).
> 
> If I understand well, after the patch series, the user of
> rte_thread_set_affinity() and rte_thread_get_affinity() are
> supposed to use the macros from sched.h to access to this
> cpuset parameter. So I'm wondering if it's not better to
> use cpu_set_t from libc instead of redefining rte_cpuset_t.
> 
> To reword my question: what is the purpose of redefining
> cpu_set_t in rte_cpuset_t if we still need to use all the
> libc API to access to it?
[LCM] In linux the type is *cpu_set_t*, but in freebsd it's *cpuset_t*.
The purpose of *rte_cpuset_t* is to make the consistent type definition in EAL, and to avoid lots of #ifdef for this diff.
In either linux or freebsd, it still can use the MACRO in libc to set the rte_cpuset_t.
> 
> 
> Regards,
> Olivier
Olivier Matz Feb. 9, 2015, 5:06 p.m. UTC | #3
Hi,

On 02/09/2015 12:33 PM, Liang, Cunming wrote:
>> On 02/02/2015 03:02 AM, Cunming Liang wrote:
>>> The patch adds 'cpuset' into per-lcore configure 'lcore_config[]',
>>> as the lcore no longer always 1:1 pinning with physical cpu.
>>> The lcore now stands for a EAL thread rather than a logical cpu.
>>>
>>> It doesn't change the default behavior of 1:1 mapping, but allows to
>>> affinity the EAL thread to multiple cpus.
>>>
>>> [...]
>>> diff --git a/lib/librte_eal/bsdapp/eal/eal_memory.c
>> b/lib/librte_eal/bsdapp/eal/eal_memory.c
>>> index 65ee87d..a34d500 100644
>>> --- a/lib/librte_eal/bsdapp/eal/eal_memory.c
>>> +++ b/lib/librte_eal/bsdapp/eal/eal_memory.c
>>> @@ -45,6 +45,8 @@
>>>  #include "eal_internal_cfg.h"
>>>  #include "eal_filesystem.h"
>>>
>>> +/* avoid re-defined against with freebsd header */
>>> +#undef PAGE_SIZE
>>>  #define PAGE_SIZE (sysconf(_SC_PAGESIZE))
>>
>> I don't see the link with the patch. Should this go somewhere else?

Maybe you missed this one.


>>> diff --git a/lib/librte_eal/common/include/rte_lcore.h
>> b/lib/librte_eal/common/include/rte_lcore.h
>>> index 49b2c03..4c7d6bb 100644
>>> --- a/lib/librte_eal/common/include/rte_lcore.h
>>> +++ b/lib/librte_eal/common/include/rte_lcore.h
>>> @@ -50,6 +50,13 @@ extern "C" {
>>>
>>>  #define LCORE_ID_ANY -1    /**< Any lcore. */
>>>
>>> +#if defined(__linux__)
>>> +	typedef	cpu_set_t rte_cpuset_t;
>>> +#elif defined(__FreeBSD__)
>>> +#include <pthread_np.h>
>>> +	typedef cpuset_t rte_cpuset_t;
>>> +#endif
>>> +
>>
>> Should we also define RTE_CPU_SETSIZE?
>> For linux, should <sched.h> be included?
> [LCM] It uses the fix size cpuset, won't use CPU_ALLOC() to get the pointer of cpuset.
> The RTE_CPU_SETSIZE always equal to sizeof(rte_cpuset_t).

The advantage of using CPU_ALLOC() is to avoid issues when the number
of core will be higher than 1024. I agree it's probably a bit early
to think about this, but it could happen soon :)


>> If I understand well, after the patch series, the user of
>> rte_thread_set_affinity() and rte_thread_get_affinity() are
>> supposed to use the macros from sched.h to access to this
>> cpuset parameter. So I'm wondering if it's not better to
>> use cpu_set_t from libc instead of redefining rte_cpuset_t.
>>
>> To reword my question: what is the purpose of redefining
>> cpu_set_t in rte_cpuset_t if we still need to use all the
>> libc API to access to it?
> [LCM] In linux the type is *cpu_set_t*, but in freebsd it's *cpuset_t*.
> The purpose of *rte_cpuset_t* is to make the consistent type definition in EAL, and to avoid lots of #ifdef for this diff.
> In either linux or freebsd, it still can use the MACRO in libc to set the rte_cpuset_t.

OK, it makes sense then. I did not notice the difference between linux
and bsd.
Konstantin Ananyev Feb. 9, 2015, 5:37 p.m. UTC | #4
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier MATZ
> Sent: Monday, February 09, 2015 5:07 PM
> To: Liang, Cunming; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 01/17] eal: add cpuset into per EAL thread lcore_config
> 
> Hi,
> 
> On 02/09/2015 12:33 PM, Liang, Cunming wrote:
> >> On 02/02/2015 03:02 AM, Cunming Liang wrote:
> >>> The patch adds 'cpuset' into per-lcore configure 'lcore_config[]',
> >>> as the lcore no longer always 1:1 pinning with physical cpu.
> >>> The lcore now stands for a EAL thread rather than a logical cpu.
> >>>
> >>> It doesn't change the default behavior of 1:1 mapping, but allows to
> >>> affinity the EAL thread to multiple cpus.
> >>>
> >>> [...]
> >>> diff --git a/lib/librte_eal/bsdapp/eal/eal_memory.c
> >> b/lib/librte_eal/bsdapp/eal/eal_memory.c
> >>> index 65ee87d..a34d500 100644
> >>> --- a/lib/librte_eal/bsdapp/eal/eal_memory.c
> >>> +++ b/lib/librte_eal/bsdapp/eal/eal_memory.c
> >>> @@ -45,6 +45,8 @@
> >>>  #include "eal_internal_cfg.h"
> >>>  #include "eal_filesystem.h"
> >>>
> >>> +/* avoid re-defined against with freebsd header */
> >>> +#undef PAGE_SIZE
> >>>  #define PAGE_SIZE (sysconf(_SC_PAGESIZE))
> >>
> >> I don't see the link with the patch. Should this go somewhere else?
> 
> Maybe you missed this one.
> 
> 
> >>> diff --git a/lib/librte_eal/common/include/rte_lcore.h
> >> b/lib/librte_eal/common/include/rte_lcore.h
> >>> index 49b2c03..4c7d6bb 100644
> >>> --- a/lib/librte_eal/common/include/rte_lcore.h
> >>> +++ b/lib/librte_eal/common/include/rte_lcore.h
> >>> @@ -50,6 +50,13 @@ extern "C" {
> >>>
> >>>  #define LCORE_ID_ANY -1    /**< Any lcore. */
> >>>
> >>> +#if defined(__linux__)
> >>> +	typedef	cpu_set_t rte_cpuset_t;
> >>> +#elif defined(__FreeBSD__)
> >>> +#include <pthread_np.h>
> >>> +	typedef cpuset_t rte_cpuset_t;
> >>> +#endif
> >>> +
> >>
> >> Should we also define RTE_CPU_SETSIZE?
> >> For linux, should <sched.h> be included?
> > [LCM] It uses the fix size cpuset, won't use CPU_ALLOC() to get the pointer of cpuset.
> > The RTE_CPU_SETSIZE always equal to sizeof(rte_cpuset_t).
> 
> The advantage of using CPU_ALLOC() is to avoid issues when the number
> of core will be higher than 1024. I agree it's probably a bit early
> to think about this, but it could happen soon :)

I personally don't think, we'll hit 1K cpu limit anytime soon...
From other side - fixed size cpuset allows to cleanup and simplify code quite a bit.
So, I'd suggest to stick with fixed size for now.
Konstantin

> 
> 
> >> If I understand well, after the patch series, the user of
> >> rte_thread_set_affinity() and rte_thread_get_affinity() are
> >> supposed to use the macros from sched.h to access to this
> >> cpuset parameter. So I'm wondering if it's not better to
> >> use cpu_set_t from libc instead of redefining rte_cpuset_t.
> >>
> >> To reword my question: what is the purpose of redefining
> >> cpu_set_t in rte_cpuset_t if we still need to use all the
> >> libc API to access to it?
> > [LCM] In linux the type is *cpu_set_t*, but in freebsd it's *cpuset_t*.
> > The purpose of *rte_cpuset_t* is to make the consistent type definition in EAL, and to avoid lots of #ifdef for this diff.
> > In either linux or freebsd, it still can use the MACRO in libc to set the rte_cpuset_t.
> 
> OK, it makes sense then. I did not notice the difference between linux
> and bsd.
Cunming Liang Feb. 10, 2015, 12:45 a.m. UTC | #5
> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, February 10, 2015 1:07 AM
> To: Liang, Cunming; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 01/17] eal: add cpuset into per EAL thread
> lcore_config
> 
> Hi,
> 
> On 02/09/2015 12:33 PM, Liang, Cunming wrote:
> >> On 02/02/2015 03:02 AM, Cunming Liang wrote:
> >>> The patch adds 'cpuset' into per-lcore configure 'lcore_config[]',
> >>> as the lcore no longer always 1:1 pinning with physical cpu.
> >>> The lcore now stands for a EAL thread rather than a logical cpu.
> >>>
> >>> It doesn't change the default behavior of 1:1 mapping, but allows to
> >>> affinity the EAL thread to multiple cpus.
> >>>
> >>> [...]
> >>> diff --git a/lib/librte_eal/bsdapp/eal/eal_memory.c
> >> b/lib/librte_eal/bsdapp/eal/eal_memory.c
> >>> index 65ee87d..a34d500 100644
> >>> --- a/lib/librte_eal/bsdapp/eal/eal_memory.c
> >>> +++ b/lib/librte_eal/bsdapp/eal/eal_memory.c
> >>> @@ -45,6 +45,8 @@
> >>>  #include "eal_internal_cfg.h"
> >>>  #include "eal_filesystem.h"
> >>>
> >>> +/* avoid re-defined against with freebsd header */
> >>> +#undef PAGE_SIZE
> >>>  #define PAGE_SIZE (sysconf(_SC_PAGESIZE))
> >>
> >> I don't see the link with the patch. Should this go somewhere else?
> 
> Maybe you missed this one.
[LCM] Yes, I missed this one. I agree to move to a separate one and remove undef but rename the PAGE_SIZE to EAL_PAGE_SIZE.
> 
> 
> >>> diff --git a/lib/librte_eal/common/include/rte_lcore.h
> >> b/lib/librte_eal/common/include/rte_lcore.h
> >>> index 49b2c03..4c7d6bb 100644
> >>> --- a/lib/librte_eal/common/include/rte_lcore.h
> >>> +++ b/lib/librte_eal/common/include/rte_lcore.h
> >>> @@ -50,6 +50,13 @@ extern "C" {
> >>>
> >>>  #define LCORE_ID_ANY -1    /**< Any lcore. */
> >>>
> >>> +#if defined(__linux__)
> >>> +	typedef	cpu_set_t rte_cpuset_t;
> >>> +#elif defined(__FreeBSD__)
> >>> +#include <pthread_np.h>
> >>> +	typedef cpuset_t rte_cpuset_t;
> >>> +#endif
> >>> +
> >>
> >> Should we also define RTE_CPU_SETSIZE?
> >> For linux, should <sched.h> be included?
> > [LCM] It uses the fix size cpuset, won't use CPU_ALLOC() to get the pointer of
> cpuset.
> > The RTE_CPU_SETSIZE always equal to sizeof(rte_cpuset_t).
> 
> The advantage of using CPU_ALLOC() is to avoid issues when the number
> of core will be higher than 1024. I agree it's probably a bit early
> to think about this, but it could happen soon :)
> 
> 
> >> If I understand well, after the patch series, the user of
> >> rte_thread_set_affinity() and rte_thread_get_affinity() are
> >> supposed to use the macros from sched.h to access to this
> >> cpuset parameter. So I'm wondering if it's not better to
> >> use cpu_set_t from libc instead of redefining rte_cpuset_t.
> >>
> >> To reword my question: what is the purpose of redefining
> >> cpu_set_t in rte_cpuset_t if we still need to use all the
> >> libc API to access to it?
> > [LCM] In linux the type is *cpu_set_t*, but in freebsd it's *cpuset_t*.
> > The purpose of *rte_cpuset_t* is to make the consistent type definition in EAL,
> and to avoid lots of #ifdef for this diff.
> > In either linux or freebsd, it still can use the MACRO in libc to set the
> rte_cpuset_t.
> 
> OK, it makes sense then. I did not notice the difference between linux
> and bsd.
diff mbox

Patch

diff --git a/lib/librte_eal/bsdapp/eal/eal_lcore.c b/lib/librte_eal/bsdapp/eal/eal_lcore.c
index 662f024..72f8ac2 100644
--- a/lib/librte_eal/bsdapp/eal/eal_lcore.c
+++ b/lib/librte_eal/bsdapp/eal/eal_lcore.c
@@ -76,11 +76,18 @@  rte_eal_cpu_init(void)
 	 * ones and enable them by default.
 	 */
 	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
+		/* init cpuset for per lcore config */
+		CPU_ZERO(&lcore_config[lcore_id].cpuset);
+
 		lcore_config[lcore_id].detected = (lcore_id < ncpus);
 		if (lcore_config[lcore_id].detected == 0) {
 			config->lcore_role[lcore_id] = ROLE_OFF;
 			continue;
 		}
+
+		/* By default, lcore 1:1 map to cpu id */
+		CPU_SET(lcore_id, &lcore_config[lcore_id].cpuset);
+
 		/* By default, each detected core is enabled */
 		config->lcore_role[lcore_id] = ROLE_RTE;
 		lcore_config[lcore_id].core_id = cpu_core_id(lcore_id);
diff --git a/lib/librte_eal/bsdapp/eal/eal_memory.c b/lib/librte_eal/bsdapp/eal/eal_memory.c
index 65ee87d..a34d500 100644
--- a/lib/librte_eal/bsdapp/eal/eal_memory.c
+++ b/lib/librte_eal/bsdapp/eal/eal_memory.c
@@ -45,6 +45,8 @@ 
 #include "eal_internal_cfg.h"
 #include "eal_filesystem.h"
 
+/* avoid re-defined against with freebsd header */
+#undef PAGE_SIZE
 #define PAGE_SIZE (sysconf(_SC_PAGESIZE))
 
 /*
diff --git a/lib/librte_eal/common/include/rte_lcore.h b/lib/librte_eal/common/include/rte_lcore.h
index 49b2c03..4c7d6bb 100644
--- a/lib/librte_eal/common/include/rte_lcore.h
+++ b/lib/librte_eal/common/include/rte_lcore.h
@@ -50,6 +50,13 @@  extern "C" {
 
 #define LCORE_ID_ANY -1    /**< Any lcore. */
 
+#if defined(__linux__)
+	typedef	cpu_set_t rte_cpuset_t;
+#elif defined(__FreeBSD__)
+#include <pthread_np.h>
+	typedef cpuset_t rte_cpuset_t;
+#endif
+
 /**
  * Structure storing internal configuration (per-lcore)
  */
@@ -65,6 +72,7 @@  struct lcore_config {
 	unsigned socket_id;        /**< physical socket id for this lcore */
 	unsigned core_id;          /**< core number on socket for this lcore */
 	int core_index;            /**< relative index, starting from 0 */
+	rte_cpuset_t cpuset;       /**< cpu set which the lcore affinity to */
 };
 
 /**
diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile
index 72ecf3a..0e9c447 100644
--- a/lib/librte_eal/linuxapp/eal/Makefile
+++ b/lib/librte_eal/linuxapp/eal/Makefile
@@ -87,6 +87,7 @@  SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_dev.c
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_options.c
 
 CFLAGS_eal.o := -D_GNU_SOURCE
+CFLAGS_eal_lcore.o := -D_GNU_SOURCE
 CFLAGS_eal_thread.o := -D_GNU_SOURCE
 CFLAGS_eal_log.o := -D_GNU_SOURCE
 CFLAGS_eal_common_log.o := -D_GNU_SOURCE
diff --git a/lib/librte_eal/linuxapp/eal/eal_lcore.c b/lib/librte_eal/linuxapp/eal/eal_lcore.c
index c67e0e6..29615f8 100644
--- a/lib/librte_eal/linuxapp/eal/eal_lcore.c
+++ b/lib/librte_eal/linuxapp/eal/eal_lcore.c
@@ -158,11 +158,19 @@  rte_eal_cpu_init(void)
 	 * ones and enable them by default.
 	 */
 	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
+		/* init cpuset for per lcore config */
+		CPU_ZERO(&lcore_config[lcore_id].cpuset);
+
+		/* in 1:1 mapping, record related cpu detected state */
 		lcore_config[lcore_id].detected = cpu_detected(lcore_id);
 		if (lcore_config[lcore_id].detected == 0) {
 			config->lcore_role[lcore_id] = ROLE_OFF;
 			continue;
 		}
+
+		/* By default, lcore 1:1 map to cpu id */
+		CPU_SET(lcore_id, &lcore_config[lcore_id].cpuset);
+
 		/* By default, each detected core is enabled */
 		config->lcore_role[lcore_id] = ROLE_RTE;
 		lcore_config[lcore_id].core_id = cpu_core_id(lcore_id);