Re: [PATCH v4 04/17] eal: add support parsing socket_id from cpuset

[Olivier MATZ <olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>]

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  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);
> +}
>