Re: [dpdk-dev] [PATCH 18.05 v4] eal: add function to return number of detected sockets

[gowrishankar muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>]

On Wednesday 07 February 2018 03:28 PM, Anatoly Burakov wrote:
> During lcore scan, find maximum socket ID and store it. This will
> break the ABI, so bump ABI version.
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>
> Notes:
>      v4:
>      - Remove backwards ABI compatibility, bump ABI instead
>      
>      v3:
>      - Added ABI compatibility
>      
>      v2:
>      - checkpatch changes
>      - check socket before deciding if the core is not to be used
>
>   lib/librte_eal/bsdapp/eal/Makefile        |  2 +-
>   lib/librte_eal/common/eal_common_lcore.c  | 37 +++++++++++++++++++++----------
>   lib/librte_eal/common/include/rte_eal.h   |  1 +
>   lib/librte_eal/common/include/rte_lcore.h |  8 +++++++
>   lib/librte_eal/linuxapp/eal/Makefile      |  2 +-
>   lib/librte_eal/rte_eal_version.map        |  9 +++++++-
>   6 files changed, 44 insertions(+), 15 deletions(-)
>
> diff --git a/lib/librte_eal/bsdapp/eal/Makefile b/lib/librte_eal/bsdapp/eal/Makefile
> index dd455e6..ed1d17b 100644
> --- a/lib/librte_eal/bsdapp/eal/Makefile
> +++ b/lib/librte_eal/bsdapp/eal/Makefile
> @@ -21,7 +21,7 @@ LDLIBS += -lgcc_s
>
>   EXPORT_MAP := ../../rte_eal_version.map
>
> -LIBABIVER := 6
> +LIBABIVER := 7
>
>   # specific to bsdapp exec-env
>   SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) := eal.c
> diff --git a/lib/librte_eal/common/eal_common_lcore.c b/lib/librte_eal/common/eal_common_lcore.c
> index 7724fa4..827ddeb 100644
> --- a/lib/librte_eal/common/eal_common_lcore.c
> +++ b/lib/librte_eal/common/eal_common_lcore.c
> @@ -28,6 +28,7 @@ rte_eal_cpu_init(void)
>   	struct rte_config *config = rte_eal_get_configuration();
>   	unsigned lcore_id;
>   	unsigned count = 0;
> +	unsigned int socket_id, max_socket_id = 0;
>
>   	/*
>   	 * Parse the maximum set of logical cores, detect the subset of running
> @@ -39,6 +40,19 @@ rte_eal_cpu_init(void)
>   		/* init cpuset for per lcore config */
>   		CPU_ZERO(&lcore_config[lcore_id].cpuset);
>
> +		/* find socket first */
> +		socket_id = eal_cpu_socket_id(lcore_id);
> +		if (socket_id >= RTE_MAX_NUMA_NODES) {
> +#ifdef RTE_EAL_ALLOW_INV_SOCKET_ID
> +			socket_id = 0;
> +#else
> +			RTE_LOG(ERR, EAL, "Socket ID (%u) is greater than RTE_MAX_NUMA_NODES (%d)\n",
> +					socket_id, RTE_MAX_NUMA_NODES);
> +			return -1;
> +#endif
> +		}
> +		max_socket_id = RTE_MAX(max_socket_id, socket_id);
> +
>   		/* in 1:1 mapping, record related cpu detected state */
>   		lcore_config[lcore_id].detected = eal_cpu_detected(lcore_id);
>   		if (lcore_config[lcore_id].detected == 0) {
> @@ -54,18 +68,7 @@ rte_eal_cpu_init(void)
>   		config->lcore_role[lcore_id] = ROLE_RTE;
>   		lcore_config[lcore_id].core_role = ROLE_RTE;
>   		lcore_config[lcore_id].core_id = eal_cpu_core_id(lcore_id);
> -		lcore_config[lcore_id].socket_id = eal_cpu_socket_id(lcore_id);
> -		if (lcore_config[lcore_id].socket_id >= RTE_MAX_NUMA_NODES) {
> -#ifdef RTE_EAL_ALLOW_INV_SOCKET_ID
> -			lcore_config[lcore_id].socket_id = 0;
> -#else
> -			RTE_LOG(ERR, EAL, "Socket ID (%u) is greater than "
> -				"RTE_MAX_NUMA_NODES (%d)\n",
> -				lcore_config[lcore_id].socket_id,
> -				RTE_MAX_NUMA_NODES);
> -			return -1;
> -#endif
> -		}
> +		lcore_config[lcore_id].socket_id = socket_id;
>   		RTE_LOG(DEBUG, EAL, "Detected lcore %u as "
>   				"core %u on socket %u\n",
>   				lcore_id, lcore_config[lcore_id].core_id,
> @@ -79,5 +82,15 @@ rte_eal_cpu_init(void)
>   		RTE_MAX_LCORE);
>   	RTE_LOG(INFO, EAL, "Detected %u lcore(s)\n", config->lcore_count);
>
> +	config->numa_node_count = max_socket_id + 1;

In some IBM servers, socket ID number does not seem to be in sequence. 
For an instance, 0 and 8 for a 2 node server.

In this case, numa_node_count would mislead users if wrongly understood 
by its variable name IMO (see below)
> +	RTE_LOG(INFO, EAL, "Detected %u NUMA nodes\n", config->numa_node_count);

For an instance, reading above message would tell 'EAL detected 8 nodes' 
in my server, but actually there are only two nodes.

Could its name better be 'numa_node_id_max' ?. Also, we store in actual 
count of numa nodes in _count variable.

Also, there could be a case when there is no local memory available to a 
numa node too.

Thanks,
Gowrishankar
> +
>   	return 0;
>   }
> +
> +unsigned int
> +rte_num_sockets(void)
> +{
> +	const struct rte_config *config = rte_eal_get_configuration();
> +	return config->numa_node_count;
> +}
> diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h
> index 08c6637..63fcc2e 100644
> --- a/lib/librte_eal/common/include/rte_eal.h
> +++ b/lib/librte_eal/common/include/rte_eal.h
> @@ -57,6 +57,7 @@ enum rte_proc_type_t {
>   struct rte_config {
>   	uint32_t master_lcore;       /**< Id of the master lcore */
>   	uint32_t lcore_count;        /**< Number of available logical cores. */
> +	uint32_t numa_node_count;    /**< Number of detected NUMA nodes. */
>   	uint32_t service_lcore_count;/**< Number of available service cores. */
>   	enum rte_lcore_role_t lcore_role[RTE_MAX_LCORE]; /**< State of cores. */
>
> diff --git a/lib/librte_eal/common/include/rte_lcore.h b/lib/librte_eal/common/include/rte_lcore.h
> index d84bcff..ddf4c64 100644
> --- a/lib/librte_eal/common/include/rte_lcore.h
> +++ b/lib/librte_eal/common/include/rte_lcore.h
> @@ -120,6 +120,14 @@ rte_lcore_index(int lcore_id)
>   unsigned rte_socket_id(void);
>
>   /**
> + * Return number of physical sockets on the system.
> + * @return
> + *   the number of physical sockets as recognized by EAL
> + *
> + */
> +unsigned int rte_num_sockets(void);
> +
> +/**
>    * Get the ID of the physical socket of the specified lcore
>    *
>    * @param lcore_id
> diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile
> index 7e5bbe8..b9c7727 100644
> --- a/lib/librte_eal/linuxapp/eal/Makefile
> +++ b/lib/librte_eal/linuxapp/eal/Makefile
> @@ -10,7 +10,7 @@ ARCH_DIR ?= $(RTE_ARCH)
>   EXPORT_MAP := ../../rte_eal_version.map
>   VPATH += $(RTE_SDK)/lib/librte_eal/common/arch/$(ARCH_DIR)
>
> -LIBABIVER := 6
> +LIBABIVER := 7
>
>   VPATH += $(RTE_SDK)/lib/librte_eal/common
>
> diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
> index 4146907..fc83e74 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -211,6 +211,13 @@ DPDK_18.02 {
>
>   }  DPDK_17.11;
>
> +DPDK_18.05 {
> +	global:
> +
> +	rte_num_sockets;
> +
> +} DPDK_18.02;
> +
>   EXPERIMENTAL {
>   	global:
>
> @@ -255,4 +262,4 @@ EXPERIMENTAL {
>   	rte_service_set_stats_enable;
>   	rte_service_start_with_defaults;
>
> -} DPDK_18.02;
> +} DPDK_18.05;