diff mbox

[dpdk-dev,v4,04/17] eal: add support parsing socket_id from cpuset

Message ID 1422842559-13617-5-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
It returns the socket_id if all cpus in the cpuset belongs
to the same NUMA node, otherwise it will return SOCKET_ID_ANY.

Signed-off-by: Cunming Liang <cunming.liang@intel.com>
---
 lib/librte_eal/bsdapp/eal/eal_lcore.c   |  7 +++++
 lib/librte_eal/common/eal_thread.h      | 52 +++++++++++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/eal_lcore.c |  7 +++++
 3 files changed, 66 insertions(+)

Comments

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

On 02/02/2015 03:02 AM, Cunming Liang wrote:
> It returns the socket_id if all cpus in the cpuset belongs
> to the same NUMA node, otherwise it will return SOCKET_ID_ANY.
> 
> Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> ---
>  lib/librte_eal/bsdapp/eal/eal_lcore.c   |  7 +++++
>  lib/librte_eal/common/eal_thread.h      | 52 +++++++++++++++++++++++++++++++++
>  lib/librte_eal/linuxapp/eal/eal_lcore.c |  7 +++++
>  3 files changed, 66 insertions(+)
> 
> diff --git a/lib/librte_eal/bsdapp/eal/eal_lcore.c b/lib/librte_eal/bsdapp/eal/eal_lcore.c
> index 72f8ac2..162fb4f 100644
> --- a/lib/librte_eal/bsdapp/eal/eal_lcore.c
> +++ b/lib/librte_eal/bsdapp/eal/eal_lcore.c
> @@ -41,6 +41,7 @@
>  #include <rte_debug.h>
>  
>  #include "eal_private.h"
> +#include "eal_thread.h"
>  
>  /* No topology information available on FreeBSD including NUMA info */
>  #define cpu_core_id(X) 0
> @@ -112,3 +113,9 @@ rte_eal_cpu_init(void)
>  
>  	return 0;
>  }
> +
> +unsigned
> +eal_cpu_socket_id(__rte_unused unsigned cpu_id)
> +{
> +	return cpu_socket_id(cpu_id);
> +}
> diff --git a/lib/librte_eal/common/eal_thread.h b/lib/librte_eal/common/eal_thread.h
> index b53b84d..a25ee86 100644
> --- a/lib/librte_eal/common/eal_thread.h
> +++ b/lib/librte_eal/common/eal_thread.h
> @@ -34,6 +34,10 @@
>  #ifndef EAL_THREAD_H
>  #define EAL_THREAD_H
>  
> +#include <sched.h>
> +
> +#include <rte_debug.h>
> +
>  /**
>   * basic loop of thread, called for each thread by eal_init().
>   *
> @@ -50,4 +54,52 @@ __attribute__((noreturn)) void *eal_thread_loop(void *arg);
>   */
>  void eal_thread_init_master(unsigned lcore_id);
>  
> +/**
> + * Get the NUMA socket id from cpu id.
> + * This function is private to EAL.
> + *
> + * @param cpu_id
> + *   The logical process id.
> + * @return
> + *   socket_id or SOCKET_ID_ANY
> + */
> +unsigned eal_cpu_socket_id(unsigned cpu_id);

Wouldn't it be better to rename the existing function cpu_socket_id()
in eal_cpu_socket_id() and export it in eal_thread.h?

In case of bsd where cpu_socket_id() is implemented using a #define,
a new function should be created returning 0.


> +
> +/**
> + * Get the NUMA socket id from cpuset.
> + * This function is private to EAL.
> + *
> + * @param cpusetp
> + *   The point to a valid cpu set.
> + * @return
> + *   socket_id or SOCKET_ID_ANY
> + */
> +static inline int
> +eal_cpuset_socket_id(rte_cpuset_t *cpusetp)
> +{
> +	unsigned cpu = 0;
> +	int socket_id = SOCKET_ID_ANY;
> +	int sid;
> +
> +	if (cpusetp == NULL)
> +		return SOCKET_ID_ANY;

SOCKET_ID_ANY is not defined, maybe <rte_lcore.h> should be included
somewhere.

> +
> +	do {
> +		if (!CPU_ISSET(cpu, cpusetp))
> +			continue;
> +
> +		if (socket_id == SOCKET_ID_ANY)
> +			socket_id = eal_cpu_socket_id(cpu);
> +
> +		sid = eal_cpu_socket_id(cpu);
> +		if (socket_id != sid) {
> +			socket_id = SOCKET_ID_ANY;
> +			break;
> +		}
> +
> +	} while (++cpu < RTE_MAX_LCORE);
> +
> +	return socket_id;
> +}


I don't think this function should be inlined.

As this function is not used, it could be interesting for reviewers
to understand when

> +
>  #endif /* EAL_THREAD_H */
> diff --git a/lib/librte_eal/linuxapp/eal/eal_lcore.c b/lib/librte_eal/linuxapp/eal/eal_lcore.c
> index 29615f8..922af6d 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_lcore.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_lcore.c
> @@ -45,6 +45,7 @@
>  
>  #include "eal_private.h"
>  #include "eal_filesystem.h"
> +#include "eal_thread.h"
>  
>  #define SYS_CPU_DIR "/sys/devices/system/cpu/cpu%u"
>  #define CORE_ID_FILE "topology/core_id"
> @@ -197,3 +198,9 @@ rte_eal_cpu_init(void)
>  
>  	return 0;
>  }
> +
> +unsigned
> +eal_cpu_socket_id(unsigned cpu_id)
> +{
> +	return cpu_socket_id(cpu_id);
> +}
>
Cunming Liang Feb. 9, 2015, 12:26 p.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 04/17] eal: add support parsing socket_id
> from cpuset
> 
> Hi,
> 
> On 02/02/2015 03:02 AM, Cunming Liang wrote:
> > It returns the socket_id if all cpus in the cpuset belongs
> > to the same NUMA node, otherwise it will return SOCKET_ID_ANY.
> >
> > Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> > ---
> >  lib/librte_eal/bsdapp/eal/eal_lcore.c   |  7 +++++
> >  lib/librte_eal/common/eal_thread.h      | 52
> +++++++++++++++++++++++++++++++++
> >  lib/librte_eal/linuxapp/eal/eal_lcore.c |  7 +++++
> >  3 files changed, 66 insertions(+)
> >
> > diff --git a/lib/librte_eal/bsdapp/eal/eal_lcore.c
> b/lib/librte_eal/bsdapp/eal/eal_lcore.c
> > index 72f8ac2..162fb4f 100644
> > --- a/lib/librte_eal/bsdapp/eal/eal_lcore.c
> > +++ b/lib/librte_eal/bsdapp/eal/eal_lcore.c
> > @@ -41,6 +41,7 @@
> >  #include <rte_debug.h>
> >
> >  #include "eal_private.h"
> > +#include "eal_thread.h"
> >
> >  /* No topology information available on FreeBSD including NUMA info */
> >  #define cpu_core_id(X) 0
> > @@ -112,3 +113,9 @@ rte_eal_cpu_init(void)
> >
> >  	return 0;
> >  }
> > +
> > +unsigned
> > +eal_cpu_socket_id(__rte_unused unsigned cpu_id)
> > +{
> > +	return cpu_socket_id(cpu_id);
> > +}
> > diff --git a/lib/librte_eal/common/eal_thread.h
> b/lib/librte_eal/common/eal_thread.h
> > index b53b84d..a25ee86 100644
> > --- a/lib/librte_eal/common/eal_thread.h
> > +++ b/lib/librte_eal/common/eal_thread.h
> > @@ -34,6 +34,10 @@
> >  #ifndef EAL_THREAD_H
> >  #define EAL_THREAD_H
> >
> > +#include <sched.h>
> > +
> > +#include <rte_debug.h>
> > +
> >  /**
> >   * basic loop of thread, called for each thread by eal_init().
> >   *
> > @@ -50,4 +54,52 @@ __attribute__((noreturn)) void *eal_thread_loop(void
> *arg);
> >   */
> >  void eal_thread_init_master(unsigned lcore_id);
> >
> > +/**
> > + * Get the NUMA socket id from cpu id.
> > + * This function is private to EAL.
> > + *
> > + * @param cpu_id
> > + *   The logical process id.
> > + * @return
> > + *   socket_id or SOCKET_ID_ANY
> > + */
> > +unsigned eal_cpu_socket_id(unsigned cpu_id);
> 
> Wouldn't it be better to rename the existing function cpu_socket_id()
> in eal_cpu_socket_id() and export it in eal_thread.h?
> 
> In case of bsd where cpu_socket_id() is implemented using a #define,
> a new function should be created returning 0.
[LCM] In eal_lcore.c, the cpu_socket_id()/cpu_core_id() defined as static and only used in rte_eal_cpu_init().
I suppose the purpose of origin design is to make the sysfs parsing only visible in the file.
No matter remove the 'static' prefix of cpu_core_id() or add a new wrap eal_cpu_socket_id(), it results in a new extern EAL API.
So I prefer not change the visibility of the origin static function but have one as extern interface.
> 
> 
> > +
> > +/**
> > + * Get the NUMA socket id from cpuset.
> > + * This function is private to EAL.
> > + *
> > + * @param cpusetp
> > + *   The point to a valid cpu set.
> > + * @return
> > + *   socket_id or SOCKET_ID_ANY
> > + */
> > +static inline int
> > +eal_cpuset_socket_id(rte_cpuset_t *cpusetp)
> > +{
> > +	unsigned cpu = 0;
> > +	int socket_id = SOCKET_ID_ANY;
> > +	int sid;
> > +
> > +	if (cpusetp == NULL)
> > +		return SOCKET_ID_ANY;
> 
> SOCKET_ID_ANY is not defined, maybe <rte_lcore.h> should be included
> somewhere.
[LCM] Agree with you, eal_cpuset_socket_id() can move into eal_common_thread.c.
And add rte_memory.h for SOCKET_ID_ANY reference.
> 
> > +
> > +	do {
> > +		if (!CPU_ISSET(cpu, cpusetp))
> > +			continue;
> > +
> > +		if (socket_id == SOCKET_ID_ANY)
> > +			socket_id = eal_cpu_socket_id(cpu);
> > +
> > +		sid = eal_cpu_socket_id(cpu);
> > +		if (socket_id != sid) {
> > +			socket_id = SOCKET_ID_ANY;
> > +			break;
> > +		}
> > +
> > +	} while (++cpu < RTE_MAX_LCORE);
> > +
> > +	return socket_id;
> > +}
> 
> 
> I don't think this function should be inlined.
> 
> As this function is not used, it could be interesting for reviewers
> to understand when
[LCM] It's used in eal_thread_set_affinity() of eal_thread.c.
> 
> > +
> >  #endif /* EAL_THREAD_H */
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_lcore.c
> b/lib/librte_eal/linuxapp/eal/eal_lcore.c
> > index 29615f8..922af6d 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal_lcore.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_lcore.c
> > @@ -45,6 +45,7 @@
> >
> >  #include "eal_private.h"
> >  #include "eal_filesystem.h"
> > +#include "eal_thread.h"
> >
> >  #define SYS_CPU_DIR "/sys/devices/system/cpu/cpu%u"
> >  #define CORE_ID_FILE "topology/core_id"
> > @@ -197,3 +198,9 @@ rte_eal_cpu_init(void)
> >
> >  	return 0;
> >  }
> > +
> > +unsigned
> > +eal_cpu_socket_id(unsigned cpu_id)
> > +{
> > +	return cpu_socket_id(cpu_id);
> > +}
> >
Olivier Matz Feb. 9, 2015, 5:16 p.m. UTC | #3
Hi,

On 02/09/2015 01:26 PM, Liang, Cunming wrote:
>>> @@ -50,4 +54,52 @@ __attribute__((noreturn)) void *eal_thread_loop(void
>> *arg);
>>>   */
>>>  void eal_thread_init_master(unsigned lcore_id);
>>>
>>> +/**
>>> + * Get the NUMA socket id from cpu id.
>>> + * This function is private to EAL.
>>> + *
>>> + * @param cpu_id
>>> + *   The logical process id.
>>> + * @return
>>> + *   socket_id or SOCKET_ID_ANY
>>> + */
>>> +unsigned eal_cpu_socket_id(unsigned cpu_id);
>>
>> Wouldn't it be better to rename the existing function cpu_socket_id()
>> in eal_cpu_socket_id() and export it in eal_thread.h?
>>
>> In case of bsd where cpu_socket_id() is implemented using a #define,
>> a new function should be created returning 0.
> [LCM] In eal_lcore.c, the cpu_socket_id()/cpu_core_id() defined as static and only used in rte_eal_cpu_init().
> I suppose the purpose of origin design is to make the sysfs parsing only visible in the file.
> No matter remove the 'static' prefix of cpu_core_id() or add a new wrap eal_cpu_socket_id(), it results in a new extern EAL API.
> So I prefer not change the visibility of the origin static function but have one as extern interface.

Yes, but I don't see what is the advantage of using a wrapper.
If there is no advantage, I think the one with the less code is
better.



>>> +static inline int
>>> +eal_cpuset_socket_id(rte_cpuset_t *cpusetp)
>>> +{
>>> +	unsigned cpu = 0;
>>> +	int socket_id = SOCKET_ID_ANY;
>>> +	int sid;
>>> +
>>> +	if (cpusetp == NULL)
>>> +		return SOCKET_ID_ANY;
>>> +
>>> +	do {
>>> +		if (!CPU_ISSET(cpu, cpusetp))
>>> +			continue;
>>> +
>>> +		if (socket_id == SOCKET_ID_ANY)
>>> +			socket_id = eal_cpu_socket_id(cpu);
>>> +
>>> +		sid = eal_cpu_socket_id(cpu);
>>> +		if (socket_id != sid) {
>>> +			socket_id = SOCKET_ID_ANY;
>>> +			break;
>>> +		}
>>> +
>>> +	} while (++cpu < RTE_MAX_LCORE);
>>> +
>>> +	return socket_id;
>>> +}
>>
>>
>> I don't think this function should be inlined.
>>
>> As this function is not used, it could be interesting for reviewers
>> to understand when
> [LCM] It's used in eal_thread_set_affinity() of eal_thread.c.

As it's not visible in the patch, could you add an explanation in
the commit log?
diff mbox

Patch

diff --git a/lib/librte_eal/bsdapp/eal/eal_lcore.c b/lib/librte_eal/bsdapp/eal/eal_lcore.c
index 72f8ac2..162fb4f 100644
--- a/lib/librte_eal/bsdapp/eal/eal_lcore.c
+++ b/lib/librte_eal/bsdapp/eal/eal_lcore.c
@@ -41,6 +41,7 @@ 
 #include <rte_debug.h>
 
 #include "eal_private.h"
+#include "eal_thread.h"
 
 /* No topology information available on FreeBSD including NUMA info */
 #define cpu_core_id(X) 0
@@ -112,3 +113,9 @@  rte_eal_cpu_init(void)
 
 	return 0;
 }
+
+unsigned
+eal_cpu_socket_id(__rte_unused unsigned cpu_id)
+{
+	return cpu_socket_id(cpu_id);
+}
diff --git a/lib/librte_eal/common/eal_thread.h b/lib/librte_eal/common/eal_thread.h
index b53b84d..a25ee86 100644
--- a/lib/librte_eal/common/eal_thread.h
+++ b/lib/librte_eal/common/eal_thread.h
@@ -34,6 +34,10 @@ 
 #ifndef EAL_THREAD_H
 #define EAL_THREAD_H
 
+#include <sched.h>
+
+#include <rte_debug.h>
+
 /**
  * basic loop of thread, called for each thread by eal_init().
  *
@@ -50,4 +54,52 @@  __attribute__((noreturn)) void *eal_thread_loop(void *arg);
  */
 void eal_thread_init_master(unsigned lcore_id);
 
+/**
+ * Get the NUMA socket id from cpu id.
+ * This function is private to EAL.
+ *
+ * @param cpu_id
+ *   The logical process id.
+ * @return
+ *   socket_id or SOCKET_ID_ANY
+ */
+unsigned eal_cpu_socket_id(unsigned cpu_id);
+
+/**
+ * Get the NUMA socket id from cpuset.
+ * This function is private to EAL.
+ *
+ * @param cpusetp
+ *   The point to a valid cpu set.
+ * @return
+ *   socket_id or SOCKET_ID_ANY
+ */
+static inline int
+eal_cpuset_socket_id(rte_cpuset_t *cpusetp)
+{
+	unsigned cpu = 0;
+	int socket_id = SOCKET_ID_ANY;
+	int sid;
+
+	if (cpusetp == NULL)
+		return SOCKET_ID_ANY;
+
+	do {
+		if (!CPU_ISSET(cpu, cpusetp))
+			continue;
+
+		if (socket_id == SOCKET_ID_ANY)
+			socket_id = eal_cpu_socket_id(cpu);
+
+		sid = eal_cpu_socket_id(cpu);
+		if (socket_id != sid) {
+			socket_id = SOCKET_ID_ANY;
+			break;
+		}
+
+	} while (++cpu < RTE_MAX_LCORE);
+
+	return socket_id;
+}
+
 #endif /* EAL_THREAD_H */
diff --git a/lib/librte_eal/linuxapp/eal/eal_lcore.c b/lib/librte_eal/linuxapp/eal/eal_lcore.c
index 29615f8..922af6d 100644
--- a/lib/librte_eal/linuxapp/eal/eal_lcore.c
+++ b/lib/librte_eal/linuxapp/eal/eal_lcore.c
@@ -45,6 +45,7 @@ 
 
 #include "eal_private.h"
 #include "eal_filesystem.h"
+#include "eal_thread.h"
 
 #define SYS_CPU_DIR "/sys/devices/system/cpu/cpu%u"
 #define CORE_ID_FILE "topology/core_id"
@@ -197,3 +198,9 @@  rte_eal_cpu_init(void)
 
 	return 0;
 }
+
+unsigned
+eal_cpu_socket_id(unsigned cpu_id)
+{
+	return cpu_socket_id(cpu_id);
+}