[dpdk-dev,v3] eal: add function to return number of detected sockets

Message ID 750e30c6dcc7a22a87df9c56fb824042b1db984f.1517848624.git.anatoly.burakov@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Burakov, Anatoly Feb. 5, 2018, 4:37 p.m. UTC
  During lcore scan, find maximum socket ID and store it.

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---

Notes:
    v3:
    - Added ABI backwards compatibility
    
    v2:
    - checkpatch changes
    - check socket before deciding if the core is not to be used

 lib/librte_eal/common/eal_common_lcore.c  | 37 +++++++++++++++++++++----------
 lib/librte_eal/common/include/rte_eal.h   | 25 +++++++++++++++++++++
 lib/librte_eal/common/include/rte_lcore.h |  8 +++++++
 lib/librte_eal/linuxapp/eal/eal.c         | 27 +++++++++++++++++++++-
 lib/librte_eal/rte_eal_version.map        |  9 +++++++-
 5 files changed, 92 insertions(+), 14 deletions(-)
  

Comments

Burakov, Anatoly Feb. 5, 2018, 5:39 p.m. UTC | #1
On 05-Feb-18 4:37 PM, Anatoly Burakov wrote:
> During lcore scan, find maximum socket ID and store it.
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> 
> Notes:
>      v3:
>      - Added ABI backwards compatibility
>      
>      v2:
>      - checkpatch changes
>      - check socket before deciding if the core is not to be used
> 
>   lib/librte_eal/common/eal_common_lcore.c  | 37 +++++++++++++++++++++----------
>   lib/librte_eal/common/include/rte_eal.h   | 25 +++++++++++++++++++++
>   lib/librte_eal/common/include/rte_lcore.h |  8 +++++++
>   lib/librte_eal/linuxapp/eal/eal.c         | 27 +++++++++++++++++++++-
>   lib/librte_eal/rte_eal_version.map        |  9 +++++++-
>   5 files changed, 92 insertions(+), 14 deletions(-)
> 

This patch does not break ABI, but does it in a very ugly way. Is it 
worth it?
  
Thomas Monjalon Feb. 5, 2018, 10:45 p.m. UTC | #2
05/02/2018 18:39, Burakov, Anatoly:
> On 05-Feb-18 4:37 PM, Anatoly Burakov wrote:
> > During lcore scan, find maximum socket ID and store it.
> > 
> > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > ---
> > 
> > Notes:
> >      v3:
> >      - Added ABI backwards compatibility
> >      
> >      v2:
> >      - checkpatch changes
> >      - check socket before deciding if the core is not to be used
> > 
> >   lib/librte_eal/common/eal_common_lcore.c  | 37 +++++++++++++++++++++----------
> >   lib/librte_eal/common/include/rte_eal.h   | 25 +++++++++++++++++++++
> >   lib/librte_eal/common/include/rte_lcore.h |  8 +++++++
> >   lib/librte_eal/linuxapp/eal/eal.c         | 27 +++++++++++++++++++++-
> >   lib/librte_eal/rte_eal_version.map        |  9 +++++++-
> >   5 files changed, 92 insertions(+), 14 deletions(-)
> > 
> 
> This patch does not break ABI, but does it in a very ugly way. Is it 
> worth it?

I think we agreed to not get this patch in 18.02.
Did you change your mind?
  
Burakov, Anatoly Feb. 6, 2018, 9:28 a.m. UTC | #3
On 05-Feb-18 10:45 PM, Thomas Monjalon wrote:
> 05/02/2018 18:39, Burakov, Anatoly:
>> On 05-Feb-18 4:37 PM, Anatoly Burakov wrote:
>>> During lcore scan, find maximum socket ID and store it.
>>>
>>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>>> ---
>>>
>>> Notes:
>>>       v3:
>>>       - Added ABI backwards compatibility
>>>       
>>>       v2:
>>>       - checkpatch changes
>>>       - check socket before deciding if the core is not to be used
>>>
>>>    lib/librte_eal/common/eal_common_lcore.c  | 37 +++++++++++++++++++++----------
>>>    lib/librte_eal/common/include/rte_eal.h   | 25 +++++++++++++++++++++
>>>    lib/librte_eal/common/include/rte_lcore.h |  8 +++++++
>>>    lib/librte_eal/linuxapp/eal/eal.c         | 27 +++++++++++++++++++++-
>>>    lib/librte_eal/rte_eal_version.map        |  9 +++++++-
>>>    5 files changed, 92 insertions(+), 14 deletions(-)
>>>
>>
>> This patch does not break ABI, but does it in a very ugly way. Is it
>> worth it?
> 
> I think we agreed to not get this patch in 18.02.
> Did you change your mind?
> 

Sorry, how do i mark this patch as for 18.05? Is it a patch header?
  
Thomas Monjalon Feb. 6, 2018, 9:47 a.m. UTC | #4
06/02/2018 10:28, Burakov, Anatoly:
> On 05-Feb-18 10:45 PM, Thomas Monjalon wrote:
> > 05/02/2018 18:39, Burakov, Anatoly:
> >> On 05-Feb-18 4:37 PM, Anatoly Burakov wrote:
> >>> During lcore scan, find maximum socket ID and store it.
> >>>
> >>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> >>> ---
> >>>
> >>> Notes:
> >>>       v3:
> >>>       - Added ABI backwards compatibility
> >>>       
> >>>       v2:
> >>>       - checkpatch changes
> >>>       - check socket before deciding if the core is not to be used
> >>>
> >>>    lib/librte_eal/common/eal_common_lcore.c  | 37 +++++++++++++++++++++----------
> >>>    lib/librte_eal/common/include/rte_eal.h   | 25 +++++++++++++++++++++
> >>>    lib/librte_eal/common/include/rte_lcore.h |  8 +++++++
> >>>    lib/librte_eal/linuxapp/eal/eal.c         | 27 +++++++++++++++++++++-
> >>>    lib/librte_eal/rte_eal_version.map        |  9 +++++++-
> >>>    5 files changed, 92 insertions(+), 14 deletions(-)
> >>>
> >>
> >> This patch does not break ABI, but does it in a very ugly way. Is it
> >> worth it?
> > 
> > I think we agreed to not get this patch in 18.02.
> > Did you change your mind?
> > 
> 
> Sorry, how do i mark this patch as for 18.05? Is it a patch header?

So your answer is "yes, it is for 18.05" :)

Next time, you could add 18.05 near "PATCH v3", or say it in annotations.
  
Burakov, Anatoly Feb. 7, 2018, 9:58 a.m. UTC | #5
This patch is for 18.05 and implements changes referenced
in the deprecation notice[1]. (not yet merged as of this
writing)

This patchset breaks the EAL ABI and bumps its version. This
is arguably OK as memory changes will change a lot in EAL and
thus likely break ABI anyway. However, two other alternative
implementations are possible:

1) local static variable recording number of detected
   sockets. This is arguably less clean approach, but will not
   break the ABI and will have relatively little impact on the
   codebase.

2) keeping ABI compatibility, as shown in v3 of this patch [2].

My preference would be to keep this one.

[1] http://dpdk.org/dev/patchwork/patch/33853/
[2] http://dpdk.org/dev/patchwork/patch/34994/

Anatoly Burakov (1):
  eal: add function to return number of detected sockets

 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(-)
  

Patch

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;
+	RTE_LOG(INFO, EAL, "Detected %u NUMA nodes\n", config->numa_node_count);
+
 	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..bbf54e2 100644
--- a/lib/librte_eal/common/include/rte_eal.h
+++ b/lib/librte_eal/common/include/rte_eal.h
@@ -57,6 +57,29 @@  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. */
+
+	/** Primary or secondary configuration */
+	enum rte_proc_type_t process_type;
+
+	/** PA or VA mapping mode */
+	enum rte_iova_mode iova_mode;
+
+	/**
+	 * Pointer to memory configuration, which may be shared across multiple
+	 * DPDK instances
+	 */
+	struct rte_mem_config *mem_config;
+} __attribute__((__packed__));
+
+/**
+ * The global RTE configuration structure - 18.02 ABI version.
+ */
+struct rte_config_v1802 {
+	uint32_t master_lcore;       /**< Id of the master lcore */
+	uint32_t lcore_count;        /**< Number of available logical cores. */
 	uint32_t service_lcore_count;/**< Number of available service cores. */
 	enum rte_lcore_role_t lcore_role[RTE_MAX_LCORE]; /**< State of cores. */
 
@@ -80,6 +103,8 @@  struct rte_config {
  *   A pointer to the global configuration structure.
  */
 struct rte_config *rte_eal_get_configuration(void);
+struct rte_config_v1802 *rte_eal_get_configuration_v1802(void);
+struct rte_config *rte_eal_get_configuration_v1805(void);
 
 /**
  * Get a lcore's role.
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/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 451fdaf..757f404 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -67,6 +67,9 @@  static rte_usage_hook_t	rte_application_usage_hook = NULL;
 /* early configuration structure, when memory config is not mmapped */
 static struct rte_mem_config early_mem_config;
 
+/* compatibility structure to return to old ABI calls */
+static struct rte_config_v1802 v1802_config;
+
 /* define fd variable here, because file needs to be kept open for the
  * duration of the program, as we hold a write lock on it in the primary proc */
 static int mem_cfg_fd = -1;
@@ -103,11 +106,33 @@  rte_eal_mbuf_default_mempool_ops(void)
 }
 
 /* Return a pointer to the configuration structure */
+struct rte_config_v1802 *
+rte_eal_get_configuration_v1802(void)
+{
+	/* copy everything to old config so that it's up to date */
+	v1802_config.iova_mode = rte_config.iova_mode;
+	v1802_config.lcore_count = rte_config.lcore_count;
+	memcpy(v1802_config.lcore_role, rte_config.lcore_role,
+			sizeof(rte_config.lcore_role));
+	v1802_config.master_lcore = rte_config.master_lcore;
+	v1802_config.mem_config = rte_config.mem_config;
+	v1802_config.process_type = rte_config.process_type;
+	v1802_config.service_lcore_count = rte_config.service_lcore_count;
+
+	return &v1802_config;
+}
+VERSION_SYMBOL(rte_eal_get_configuration, _v1802, 18.02);
+
+/* Return a pointer to the configuration structure */
 struct rte_config *
-rte_eal_get_configuration(void)
+rte_eal_get_configuration_v1805(void)
 {
 	return &rte_config;
 }
+VERSION_SYMBOL(rte_eal_get_configuration, _v1805, 18.05);
+BIND_DEFAULT_SYMBOL(rte_eal_get_configuration, _v1805, 18.05);
+MAP_STATIC_SYMBOL(struct rte_config *rte_eal_get_configuration(void),
+		rte_eal_get_configuration_v1805);
 
 enum rte_iova_mode
 rte_eal_iova_mode(void)
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;