[PATCH v2] app/testpmd: support non contiguous socket ids

[PATCH v2] app/testpmd: support non contiguous socket ids

[Shahaf Shuler]

The test assumes the socket ids are contiguous. This
is not necessarily the case on all servers and may cause
mempool creation to fail.

Fixing it by detecting the list of valid socket ids and
use it for the mempool creation.

Fixes: 7acf894d07d1 ("app/testpmd: detect numa socket count")

CC: stable@dpdk.org
Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
---
on v2:
 * fix minor typo on commit message : be->by.
---
 app/test-pmd/parameters.c | 38 ++++++++++++++++++++++++++++----------
 app/test-pmd/testpmd.c    | 38 +++++++++++++++++++++++++++++---------
 app/test-pmd/testpmd.h    |  4 +++-
 3 files changed, 60 insertions(+), 20 deletions(-)

diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 787e1434c..4822a8a8a 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -410,9 +410,14 @@ parse_portnuma_config(const char *q_arg)
 			return -1;
 		}
 		socket_id = (uint8_t)int_fld[FLD_SOCKET];
-		if(socket_id >= max_socket) {
-			printf("Invalid socket id, range is [0, %d]\n",
-				 max_socket - 1);
+		if (new_socket_id(socket_id)) {
+			unsigned int i = 0;
+
+			printf("Invalid socket id, options are: ");
+			for (i = 0; i < num_sockets; i++) {
+				printf("%u%s", socket_ids[i],
+				(i == num_sockets - 1) ? "\n" : ",");
+			}
 			return -1;
 		}
 		port_numa[port_id] = socket_id;
@@ -470,9 +475,14 @@ parse_ringnuma_config(const char *q_arg)
 			return -1;
 		}
 		socket_id = (uint8_t)int_fld[FLD_SOCKET];
-		if (socket_id >= max_socket) {
-			printf("Invalid socket id, range is [0, %d]\n",
-				max_socket - 1);
+		if (new_socket_id(socket_id)) {
+			unsigned int i = 0;
+
+			printf("Invalid socket id, options are: ");
+			for (i = 0; i < num_sockets; i++) {
+				printf("%u%s", socket_ids[i],
+				(i == num_sockets - 1) ? "\n" : ",");
+			}
 			return -1;
 		}
 		ring_flag = (uint8_t)int_fld[FLD_FLAG];
@@ -700,12 +710,20 @@ launch_args_parse(int argc, char** argv)
 					   "invalid ring-numa configuration\n");
 			if (!strcmp(lgopts[opt_idx].name, "socket-num")) {
 				n = atoi(optarg);
-				if((uint8_t)n < max_socket)
+				if (!new_socket_id((uint8_t)n)) {
 					socket_num = (uint8_t)n;
-				else
+				} else {
+					unsigned int i = 0;
+
+					printf("socket id options are: ");
+					for (i = 0; i < num_sockets; i++) {
+						printf("%u%s", socket_ids[i],
+						(i == num_sockets - 1) ?
+						"\n" : ",");
+					}
 					rte_exit(EXIT_FAILURE,
-						"The socket number should be < %d\n",
-						max_socket);
+						"Invalid socket id");
+				}
 			}
 			if (!strcmp(lgopts[opt_idx].name, "mbuf-size")) {
 				n = atoi(optarg);
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index dfe64427d..a556a8aff 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -354,7 +354,8 @@ struct queue_stats_mappings *rx_queue_stats_mappings = rx_queue_stats_mappings_a
 uint16_t nb_tx_queue_stats_mappings = 0;
 uint16_t nb_rx_queue_stats_mappings = 0;
 
-unsigned max_socket = 0;
+unsigned int num_sockets = 0;
+unsigned int socket_ids[RTE_MAX_NUMA_NODES];
 
 #ifdef RTE_LIBRTE_BITRATE
 /* Bitrate statistics */
@@ -377,6 +378,22 @@ static void eth_event_callback(uint8_t port_id,
 static int all_ports_started(void);
 
 /*
+ * Helper function to check if socket is allready discovered.
+ * If yes, return positive value. If not, return zero.
+ */
+int
+new_socket_id(unsigned int socket_id)
+{
+	unsigned int i;
+
+	for (i = 0; i < num_sockets; i++) {
+		if (socket_ids[i] == socket_id)
+			return 0;
+	}
+	return 1;
+}
+
+/*
  * Setup default configuration.
  */
 static void
@@ -388,12 +405,14 @@ set_default_fwd_lcores_config(void)
 
 	nb_lc = 0;
 	for (i = 0; i < RTE_MAX_LCORE; i++) {
-		sock_num = rte_lcore_to_socket_id(i) + 1;
-		if (sock_num > max_socket) {
-			if (sock_num > RTE_MAX_NUMA_NODES)
-				rte_exit(EXIT_FAILURE, "Total sockets greater than %u\n", RTE_MAX_NUMA_NODES);
-			max_socket = sock_num;
+		sock_num = rte_lcore_to_socket_id(i);
+		if (sock_num > RTE_MAX_NUMA_NODES) {
+			rte_exit(EXIT_FAILURE,
+				 "Total sockets greater than %u\n",
+				 RTE_MAX_NUMA_NODES);
 		}
+		if (new_socket_id(sock_num))
+			socket_ids[num_sockets++] = sock_num;
 		if (!rte_lcore_is_enabled(i))
 			continue;
 		if (i == rte_get_master_lcore())
@@ -506,7 +525,7 @@ check_socket_id(const unsigned int socket_id)
 {
 	static int warning_once = 0;
 
-	if (socket_id >= max_socket) {
+	if (new_socket_id(socket_id)) {
 		if (!warning_once && numa_support)
 			printf("Warning: NUMA should be configured manually by"
 			       " using --port-numa-config and"
@@ -591,8 +610,9 @@ init_config(void)
 	if (numa_support) {
 		uint8_t i;
 
-		for (i = 0; i < max_socket; i++)
-			mbuf_pool_create(mbuf_data_size, nb_mbuf_per_pool, i);
+		for (i = 0; i < num_sockets; i++)
+			mbuf_pool_create(mbuf_data_size, nb_mbuf_per_pool,
+					 socket_ids[i]);
 	} else {
 		if (socket_num == UMA_NO_CONFIG)
 			mbuf_pool_create(mbuf_data_size, nb_mbuf_per_pool, 0);
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 8b3d903ef..8186b6bfd 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -341,7 +341,8 @@ extern lcoreid_t nb_lcores; /**< Number of logical cores probed at init time. */
 extern lcoreid_t nb_cfg_lcores; /**< Number of configured logical cores. */
 extern lcoreid_t nb_fwd_lcores; /**< Number of forwarding logical cores. */
 extern unsigned int fwd_lcores_cpuids[RTE_MAX_LCORE];
-extern unsigned max_socket;
+extern unsigned int num_sockets;
+extern unsigned int socket_ids[RTE_MAX_NUMA_NODES];
 
 /*
  * Configuration of Ethernet ports:
@@ -636,6 +637,7 @@ enum print_warning {
 	DISABLED_WARN
 };
 int port_id_is_invalid(portid_t port_id, enum print_warning warning);
+int new_socket_id(unsigned int socket_id);
 
 /*
  * Work-around of a compilation error with ICC on invocations of the
-- 
2.12.0

Re: [PATCH v2] app/testpmd: support non contiguous socket ids

[Wu, Jingjing]

> -----Original Message-----
> From: Shahaf Shuler [mailto:shahafs@mellanox.com]
> Sent: Wednesday, May 3, 2017 9:44 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; thomas@monjalon.net; stable@dpdk.org
> Subject: [PATCH v2] app/testpmd: support non contiguous socket ids
> 
> The test assumes the socket ids are contiguous. This is not necessarily the case
> on all servers and may cause mempool creation to fail.
> 
> Fixing it by detecting the list of valid socket ids and use it for the mempool
> creation.
> 
> Fixes: 7acf894d07d1 ("app/testpmd: detect numa socket count")
> 
> CC: stable@dpdk.org
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> ---
> on v2:
>  * fix minor typo on commit message : be->by.
> ---
>  app/test-pmd/parameters.c | 38 ++++++++++++++++++++++++++++----------
>  app/test-pmd/testpmd.c    | 38 +++++++++++++++++++++++++++++---------
>  app/test-pmd/testpmd.h    |  4 +++-
>  3 files changed, 60 insertions(+), 20 deletions(-)
> 
> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c index
> 787e1434c..4822a8a8a 100644
> --- a/app/test-pmd/parameters.c
> +++ b/app/test-pmd/parameters.c
> @@ -410,9 +410,14 @@ parse_portnuma_config(const char *q_arg)
>  			return -1;
>  		}
>  		socket_id = (uint8_t)int_fld[FLD_SOCKET];
> -		if(socket_id >= max_socket) {
> -			printf("Invalid socket id, range is [0, %d]\n",
> -				 max_socket - 1);
> +		if (new_socket_id(socket_id)) {
> +			unsigned int i = 0;
> +
> +			printf("Invalid socket id, options are: ");
> +			for (i = 0; i < num_sockets; i++) {
> +				printf("%u%s", socket_ids[i],
> +				(i == num_sockets - 1) ? "\n" : ",");
> +			}
>  			return -1;
>  		}
>  		port_numa[port_id] = socket_id;
> @@ -470,9 +475,14 @@ parse_ringnuma_config(const char *q_arg)
>  			return -1;
>  		}
>  		socket_id = (uint8_t)int_fld[FLD_SOCKET];
> -		if (socket_id >= max_socket) {
> -			printf("Invalid socket id, range is [0, %d]\n",
> -				max_socket - 1);
> +		if (new_socket_id(socket_id)) {
> +			unsigned int i = 0;
> +
> +			printf("Invalid socket id, options are: ");
> +			for (i = 0; i < num_sockets; i++) {
> +				printf("%u%s", socket_ids[i],
> +				(i == num_sockets - 1) ? "\n" : ",");
> +			}
>  			return -1;
>  		}
>  		ring_flag = (uint8_t)int_fld[FLD_FLAG]; @@ -700,12 +710,20
> @@ launch_args_parse(int argc, char** argv)
>  					   "invalid ring-numa configuration\n");
>  			if (!strcmp(lgopts[opt_idx].name, "socket-num")) {
>  				n = atoi(optarg);
> -				if((uint8_t)n < max_socket)
> +				if (!new_socket_id((uint8_t)n)) {
>  					socket_num = (uint8_t)n;
> -				else
> +				} else {
> +					unsigned int i = 0;
> +
> +					printf("socket id options are: ");
> +					for (i = 0; i < num_sockets; i++) {
> +						printf("%u%s", socket_ids[i],
> +						(i == num_sockets - 1) ?
> +						"\n" : ",");
> +					}
>  					rte_exit(EXIT_FAILURE,
> -						"The socket number should be
> < %d\n",
> -						max_socket);
> +						"Invalid socket id");
> +				}
>  			}
>  			if (!strcmp(lgopts[opt_idx].name, "mbuf-size")) {
>  				n = atoi(optarg);
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> dfe64427d..a556a8aff 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -354,7 +354,8 @@ struct queue_stats_mappings
> *rx_queue_stats_mappings = rx_queue_stats_mappings_a  uint16_t
> nb_tx_queue_stats_mappings = 0;  uint16_t nb_rx_queue_stats_mappings = 0;
> 
> -unsigned max_socket = 0;
> +unsigned int num_sockets = 0;
> +unsigned int socket_ids[RTE_MAX_NUMA_NODES];
> 
>  #ifdef RTE_LIBRTE_BITRATE
>  /* Bitrate statistics */
> @@ -377,6 +378,22 @@ static void eth_event_callback(uint8_t port_id,  static
> int all_ports_started(void);
> 
>  /*
> + * Helper function to check if socket is allready discovered.
> + * If yes, return positive value. If not, return zero.
> + */
> +int
> +new_socket_id(unsigned int socket_id)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < num_sockets; i++) {
> +		if (socket_ids[i] == socket_id)
> +			return 0;
> +	}
> +	return 1;
> +}
> +
> +/*
>   * Setup default configuration.
>   */
>  static void
> @@ -388,12 +405,14 @@ set_default_fwd_lcores_config(void)
> 
>  	nb_lc = 0;
>  	for (i = 0; i < RTE_MAX_LCORE; i++) {
> -		sock_num = rte_lcore_to_socket_id(i) + 1;
> -		if (sock_num > max_socket) {
> -			if (sock_num > RTE_MAX_NUMA_NODES)
> -				rte_exit(EXIT_FAILURE, "Total sockets greater
> than %u\n", RTE_MAX_NUMA_NODES);
> -			max_socket = sock_num;
> +		sock_num = rte_lcore_to_socket_id(i);
+1 is missed?

Re: [PATCH v2] app/testpmd: support non contiguous socket ids

[Shahaf Shuler]

Saturday, May 6, 2017 4:41 AM, Wu, Jingjing:
> >
> > The test assumes the socket ids are contiguous. This is not
> > necessarily the case on all servers and may cause mempool creation to fail.
> >
> > Fixing it by detecting the list of valid socket ids and use it for the
> > mempool creation.
> >
> > Fixes: 7acf894d07d1 ("app/testpmd: detect numa socket count")
> >
> > CC: stable@dpdk.org
> > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > ---
> > on v2:
> >  * fix minor typo on commit message : be->by.
> > ---
> >  app/test-pmd/parameters.c | 38 ++++++++++++++++++++++++++++-----
> -----
> >  app/test-pmd/testpmd.c    | 38 +++++++++++++++++++++++++++++------
> ---
> >  app/test-pmd/testpmd.h    |  4 +++-
> >  3 files changed, 60 insertions(+), 20 deletions(-)
> >

[..]

> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > dfe64427d..a556a8aff 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c

[..]

> > +/*
> >   * Setup default configuration.
> >   */
> >  static void
> > @@ -388,12 +405,14 @@ set_default_fwd_lcores_config(void)
> >
> >  	nb_lc = 0;
> >  	for (i = 0; i < RTE_MAX_LCORE; i++) {
> > -		sock_num = rte_lcore_to_socket_id(i) + 1;
> > -		if (sock_num > max_socket) {
> > -			if (sock_num > RTE_MAX_NUMA_NODES)
> > -				rte_exit(EXIT_FAILURE, "Total sockets
> greater
> > than %u\n", RTE_MAX_NUMA_NODES);
> > -			max_socket = sock_num;
> > +		sock_num = rte_lcore_to_socket_id(i);
> +1 is missed?

I don't think so.
On previous implementation this logic was meant to find the max_socket. 
max_socket was the first invalid socket_id and was used, for example :

if (socket_id >= max_socket) {

or

for (i = 0; i < max_socket; i++)

now, on above logic, we list the valid socket id. Therefore rte_lcore_to_socket_id return a valid id and not need to add 1.

Re: [PATCH v2] app/testpmd: support non contiguous socket ids

[Thomas Monjalon]

03/05/2017 15:44, Shahaf Shuler:
> +			printf("Invalid socket id, options are: ");
> +			for (i = 0; i < num_sockets; i++) {
> +				printf("%u%s", socket_ids[i],
> +				(i == num_sockets - 1) ? "\n" : ",");
[...]
> +			printf("Invalid socket id, options are: ");
> +			for (i = 0; i < num_sockets; i++) {
> +				printf("%u%s", socket_ids[i],
> +				(i == num_sockets - 1) ? "\n" : ",");
[...]
> +					printf("socket id options are: ");
> +					for (i = 0; i < num_sockets; i++) {
> +						printf("%u%s", socket_ids[i],
> +						(i == num_sockets - 1) ?
> +						"\n" : ",");

It is three times the same copy pasted code.
Please make a function.

[PATCH v3] app/testpmd: support non contiguous socket ids

[Shahaf Shuler]

The test assumes the socket ids are contiguous. This
is not necessarily the case on all servers and may cause
mempool creation to fail.

Fixing it by detecting the list of valid socket ids and
use it for the mempool creation.

Fixes: 7acf894d07d1 ("app/testpmd: detect numa socket count")

CC: stable@dpdk.org
Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
---
on v3:
 * create a function to print invalid socket id error.

on v2:
 * fix minor typo on commit message : be->by.
---
 app/test-pmd/parameters.c | 31 +++++++++++++++++++++----------
 app/test-pmd/testpmd.c    | 38 +++++++++++++++++++++++++++++---------
 app/test-pmd/testpmd.h    |  4 +++-
 3 files changed, 53 insertions(+), 20 deletions(-)

diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 787e1434c..0781e19c3 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -365,6 +365,18 @@ parse_queue_stats_mapping_config(const char *q_arg, int is_rx)
 	return 0;
 }
 
+static void
+print_invalid_socket_id_error(void)
+{
+	unsigned int i = 0;
+
+	printf("Invalid socket id, options are: ");
+	for (i = 0; i < num_sockets; i++) {
+		printf("%u%s", socket_ids[i],
+		      (i == num_sockets - 1) ? "\n" : ",");
+	}
+}
+
 static int
 parse_portnuma_config(const char *q_arg)
 {
@@ -410,9 +422,8 @@ parse_portnuma_config(const char *q_arg)
 			return -1;
 		}
 		socket_id = (uint8_t)int_fld[FLD_SOCKET];
-		if(socket_id >= max_socket) {
-			printf("Invalid socket id, range is [0, %d]\n",
-				 max_socket - 1);
+		if (new_socket_id(socket_id)) {
+			print_invalid_socket_id_error();
 			return -1;
 		}
 		port_numa[port_id] = socket_id;
@@ -470,9 +481,8 @@ parse_ringnuma_config(const char *q_arg)
 			return -1;
 		}
 		socket_id = (uint8_t)int_fld[FLD_SOCKET];
-		if (socket_id >= max_socket) {
-			printf("Invalid socket id, range is [0, %d]\n",
-				max_socket - 1);
+		if (new_socket_id(socket_id)) {
+			print_invalid_socket_id_error();
 			return -1;
 		}
 		ring_flag = (uint8_t)int_fld[FLD_FLAG];
@@ -700,12 +710,13 @@ launch_args_parse(int argc, char** argv)
 					   "invalid ring-numa configuration\n");
 			if (!strcmp(lgopts[opt_idx].name, "socket-num")) {
 				n = atoi(optarg);
-				if((uint8_t)n < max_socket)
+				if (!new_socket_id((uint8_t)n)) {
 					socket_num = (uint8_t)n;
-				else
+				} else {
+					print_invalid_socket_id_error();
 					rte_exit(EXIT_FAILURE,
-						"The socket number should be < %d\n",
-						max_socket);
+						"Invalid socket id");
+				}
 			}
 			if (!strcmp(lgopts[opt_idx].name, "mbuf-size")) {
 				n = atoi(optarg);
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index dfe64427d..a556a8aff 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -354,7 +354,8 @@ struct queue_stats_mappings *rx_queue_stats_mappings = rx_queue_stats_mappings_a
 uint16_t nb_tx_queue_stats_mappings = 0;
 uint16_t nb_rx_queue_stats_mappings = 0;
 
-unsigned max_socket = 0;
+unsigned int num_sockets = 0;
+unsigned int socket_ids[RTE_MAX_NUMA_NODES];
 
 #ifdef RTE_LIBRTE_BITRATE
 /* Bitrate statistics */
@@ -377,6 +378,22 @@ static void eth_event_callback(uint8_t port_id,
 static int all_ports_started(void);
 
 /*
+ * Helper function to check if socket is allready discovered.
+ * If yes, return positive value. If not, return zero.
+ */
+int
+new_socket_id(unsigned int socket_id)
+{
+	unsigned int i;
+
+	for (i = 0; i < num_sockets; i++) {
+		if (socket_ids[i] == socket_id)
+			return 0;
+	}
+	return 1;
+}
+
+/*
  * Setup default configuration.
  */
 static void
@@ -388,12 +405,14 @@ set_default_fwd_lcores_config(void)
 
 	nb_lc = 0;
 	for (i = 0; i < RTE_MAX_LCORE; i++) {
-		sock_num = rte_lcore_to_socket_id(i) + 1;
-		if (sock_num > max_socket) {
-			if (sock_num > RTE_MAX_NUMA_NODES)
-				rte_exit(EXIT_FAILURE, "Total sockets greater than %u\n", RTE_MAX_NUMA_NODES);
-			max_socket = sock_num;
+		sock_num = rte_lcore_to_socket_id(i);
+		if (sock_num > RTE_MAX_NUMA_NODES) {
+			rte_exit(EXIT_FAILURE,
+				 "Total sockets greater than %u\n",
+				 RTE_MAX_NUMA_NODES);
 		}
+		if (new_socket_id(sock_num))
+			socket_ids[num_sockets++] = sock_num;
 		if (!rte_lcore_is_enabled(i))
 			continue;
 		if (i == rte_get_master_lcore())
@@ -506,7 +525,7 @@ check_socket_id(const unsigned int socket_id)
 {
 	static int warning_once = 0;
 
-	if (socket_id >= max_socket) {
+	if (new_socket_id(socket_id)) {
 		if (!warning_once && numa_support)
 			printf("Warning: NUMA should be configured manually by"
 			       " using --port-numa-config and"
@@ -591,8 +610,9 @@ init_config(void)
 	if (numa_support) {
 		uint8_t i;
 
-		for (i = 0; i < max_socket; i++)
-			mbuf_pool_create(mbuf_data_size, nb_mbuf_per_pool, i);
+		for (i = 0; i < num_sockets; i++)
+			mbuf_pool_create(mbuf_data_size, nb_mbuf_per_pool,
+					 socket_ids[i]);
 	} else {
 		if (socket_num == UMA_NO_CONFIG)
 			mbuf_pool_create(mbuf_data_size, nb_mbuf_per_pool, 0);
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 8b3d903ef..8186b6bfd 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -341,7 +341,8 @@ extern lcoreid_t nb_lcores; /**< Number of logical cores probed at init time. */
 extern lcoreid_t nb_cfg_lcores; /**< Number of configured logical cores. */
 extern lcoreid_t nb_fwd_lcores; /**< Number of forwarding logical cores. */
 extern unsigned int fwd_lcores_cpuids[RTE_MAX_LCORE];
-extern unsigned max_socket;
+extern unsigned int num_sockets;
+extern unsigned int socket_ids[RTE_MAX_NUMA_NODES];
 
 /*
  * Configuration of Ethernet ports:
@@ -636,6 +637,7 @@ enum print_warning {
 	DISABLED_WARN
 };
 int port_id_is_invalid(portid_t port_id, enum print_warning warning);
+int new_socket_id(unsigned int socket_id);
 
 /*
  * Work-around of a compilation error with ICC on invocations of the
-- 
2.12.0

Re: [PATCH v2] app/testpmd: support non contiguous socket ids

[Wu, Jingjing]

> -----Original Message-----
> From: Shahaf Shuler [mailto:shahafs@mellanox.com]
> Sent: Sunday, May 7, 2017 2:06 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>;
> stable@dpdk.org
> Subject: RE: [PATCH v2] app/testpmd: support non contiguous socket ids
> 
> Saturday, May 6, 2017 4:41 AM, Wu, Jingjing:
> > >
> > > The test assumes the socket ids are contiguous. This is not
> > > necessarily the case on all servers and may cause mempool creation to fail.
> > >
> > > Fixing it by detecting the list of valid socket ids and use it for
> > > the mempool creation.
> > >
> > > Fixes: 7acf894d07d1 ("app/testpmd: detect numa socket count")
> > >
> > > CC: stable@dpdk.org
> > > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > > ---
> > > on v2:
> > >  * fix minor typo on commit message : be->by.
> > > ---
> > >  app/test-pmd/parameters.c | 38 ++++++++++++++++++++++++++++-----
> > -----
> > >  app/test-pmd/testpmd.c    | 38 +++++++++++++++++++++++++++++------
> > ---
> > >  app/test-pmd/testpmd.h    |  4 +++-
> > >  3 files changed, 60 insertions(+), 20 deletions(-)
> > >
> 
> [..]
> 
> > > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > > dfe64427d..a556a8aff 100644
> > > --- a/app/test-pmd/testpmd.c
> > > +++ b/app/test-pmd/testpmd.c
> 
> [..]
> 
> > > +/*
> > >   * Setup default configuration.
> > >   */
> > >  static void
> > > @@ -388,12 +405,14 @@ set_default_fwd_lcores_config(void)
> > >
> > >  	nb_lc = 0;
> > >  	for (i = 0; i < RTE_MAX_LCORE; i++) {
> > > -		sock_num = rte_lcore_to_socket_id(i) + 1;
> > > -		if (sock_num > max_socket) {
> > > -			if (sock_num > RTE_MAX_NUMA_NODES)
> > > -				rte_exit(EXIT_FAILURE, "Total sockets
> > greater
> > > than %u\n", RTE_MAX_NUMA_NODES);
> > > -			max_socket = sock_num;
> > > +		sock_num = rte_lcore_to_socket_id(i);
> > +1 is missed?
> 
> I don't think so.
> On previous implementation this logic was meant to find the max_socket.
> max_socket was the first invalid socket_id and was used, for example :
> 
> if (socket_id >= max_socket) {
> 
> or
> 
> for (i = 0; i < max_socket; i++)
> 
> now, on above logic, we list the valid socket id. Therefore
> rte_lcore_to_socket_id return a valid id and not need to add 1.
> 
> 
OK, but at list the following check "if (sock_num > RTE_MAX_NUMA_NODES)"
Should be "if (sock_num +1 > RTE_MAX_NUMA_NODES)", right?

One more thing, if this patch is fixing a bug, I think "fix" should be added in title.

Thanks
Jingjing

Re: [PATCH v2] app/testpmd: support non contiguous socket ids

[Shahaf Shuler]

--Shahaf


> -----Original Message-----
> From: Wu, Jingjing [mailto:jingjing.wu@intel.com]
> Sent: Monday, May 8, 2017 3:54 AM
> To: Shahaf Shuler <shahafs@mellanox.com>
> Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>;
> stable@dpdk.org
> Subject: RE: [PATCH v2] app/testpmd: support non contiguous socket ids
> 
> 
> 
> > -----Original Message-----
> > From: Shahaf Shuler [mailto:shahafs@mellanox.com]
> > Sent: Sunday, May 7, 2017 2:06 PM
> > To: Wu, Jingjing <jingjing.wu@intel.com>
> > Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>;
> > stable@dpdk.org
> > Subject: RE: [PATCH v2] app/testpmd: support non contiguous socket ids
> >
> > Saturday, May 6, 2017 4:41 AM, Wu, Jingjing:
> > > >
> > > > The test assumes the socket ids are contiguous. This is not
> > > > necessarily the case on all servers and may cause mempool creation to
> fail.
> > > >
> > > > Fixing it by detecting the list of valid socket ids and use it for
> > > > the mempool creation.
> > > >
> > > > Fixes: 7acf894d07d1 ("app/testpmd: detect numa socket count")
> > > >
> > > > CC: stable@dpdk.org
> > > > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > > > ---
> > > > on v2:
> > > >  * fix minor typo on commit message : be->by.
> > > > ---
> > > >  app/test-pmd/parameters.c | 38 ++++++++++++++++++++++++++++-
> ----
> > > -----
> > > >  app/test-pmd/testpmd.c    | 38 +++++++++++++++++++++++++++++-
> -----
> > > ---
> > > >  app/test-pmd/testpmd.h    |  4 +++-
> > > >  3 files changed, 60 insertions(+), 20 deletions(-)
> > > >
> >
> > [..]
> >
> > > > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > > > dfe64427d..a556a8aff 100644
> > > > --- a/app/test-pmd/testpmd.c
> > > > +++ b/app/test-pmd/testpmd.c
> >
> > [..]
> >
> > > > +/*
> > > >   * Setup default configuration.
> > > >   */
> > > >  static void
> > > > @@ -388,12 +405,14 @@ set_default_fwd_lcores_config(void)
> > > >
> > > >  	nb_lc = 0;
> > > >  	for (i = 0; i < RTE_MAX_LCORE; i++) {
> > > > -		sock_num = rte_lcore_to_socket_id(i) + 1;
> > > > -		if (sock_num > max_socket) {
> > > > -			if (sock_num > RTE_MAX_NUMA_NODES)
> > > > -				rte_exit(EXIT_FAILURE, "Total sockets
> > > greater
> > > > than %u\n", RTE_MAX_NUMA_NODES);
> > > > -			max_socket = sock_num;
> > > > +		sock_num = rte_lcore_to_socket_id(i);
> > > +1 is missed?
> >
> > I don't think so.
> > On previous implementation this logic was meant to find the max_socket.
> > max_socket was the first invalid socket_id and was used, for example :
> >
> > if (socket_id >= max_socket) {
> >
> > or
> >
> > for (i = 0; i < max_socket; i++)
> >
> > now, on above logic, we list the valid socket id. Therefore
> > rte_lcore_to_socket_id return a valid id and not need to add 1.
> >
> >
> OK, but at list the following check "if (sock_num >
> RTE_MAX_NUMA_NODES)"
> Should be "if (sock_num +1 > RTE_MAX_NUMA_NODES)", right?
> 

Right, i prefer the following though, what do you think?

       for (i = 0; i < RTE_MAX_LCORE; i++) {
                sock_num = rte_lcore_to_socket_id(i);
                if (new_socket_id(sock_num)) {
                        if (num_sockets >= RTE_MAX_NUMA_NODES) {
                                rte_exit(EXIT_FAILURE,
                                         "Total sockets greater than %u\n",
                                         RTE_MAX_NUMA_NODES);
                        }
                        socket_ids[num_sockets++] = sock_num;
                }

> One more thing, if this patch is fixing a bug, I think "fix" should be added in
> title.

OK, I guess I can change the commit title and message.

BTW note that there is a V3 with Thomas requested changes.
If we agree on above I can submit a v4 with the last updates.


> 
> Thanks
> Jingjing

Re: [PATCH v2] app/testpmd: support non contiguous socket ids

[Wu, Jingjing]

> 
> Right, i prefer the following though, what do you think?
> 
>        for (i = 0; i < RTE_MAX_LCORE; i++) {
>                 sock_num = rte_lcore_to_socket_id(i);
>                 if (new_socket_id(sock_num)) {
>                         if (num_sockets >= RTE_MAX_NUMA_NODES) {
>                                 rte_exit(EXIT_FAILURE,
>                                          "Total sockets greater than %u\n",
>                                          RTE_MAX_NUMA_NODES);
>                         }
>                         socket_ids[num_sockets++] = sock_num;
>                 }
> 
That looks good :)

> > One more thing, if this patch is fixing a bug, I think "fix" should be
> > added in title.
> 
> OK, I guess I can change the commit title and message.
> 
> BTW note that there is a V3 with Thomas requested changes.
> If we agree on above I can submit a v4 with the last updates.
> 
>
OK.
 
Thanks
Jingjing

[PATCH v4] app/testpmd: fix mempool creation by socket id

[Shahaf Shuler]

The test assumes the socket ids are contiguous. This
is not necessarily the case on all servers and may cause
mempool creation to fail.

Fixing it by detecting the list of valid socket ids and
use it for the mempool creation.

Fixes: 7acf894d07d1 ("app/testpmd: detect numa socket count")

CC: stable@dpdk.org
Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
---
on v4:
 * change commit title.
 * fix out of range socket id evaluation.

on v3:
 * create a function to print invalid socket id error.

on v2:
 * fix minor typo on commit message : be->by.
---
 app/test-pmd/parameters.c | 31 +++++++++++++++++++++----------
 app/test-pmd/testpmd.c    | 39 ++++++++++++++++++++++++++++++---------
 app/test-pmd/testpmd.h    |  4 +++-
 3 files changed, 54 insertions(+), 20 deletions(-)

diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 23f2fa313..a480d8f9b 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -369,6 +369,18 @@ parse_queue_stats_mapping_config(const char *q_arg, int is_rx)
 	return 0;
 }
 
+static void
+print_invalid_socket_id_error(void)
+{
+	unsigned int i = 0;
+
+	printf("Invalid socket id, options are: ");
+	for (i = 0; i < num_sockets; i++) {
+		printf("%u%s", socket_ids[i],
+		      (i == num_sockets - 1) ? "\n" : ",");
+	}
+}
+
 static int
 parse_portnuma_config(const char *q_arg)
 {
@@ -414,9 +426,8 @@ parse_portnuma_config(const char *q_arg)
 			return -1;
 		}
 		socket_id = (uint8_t)int_fld[FLD_SOCKET];
-		if(socket_id >= max_socket) {
-			printf("Invalid socket id, range is [0, %d]\n",
-				 max_socket - 1);
+		if (new_socket_id(socket_id)) {
+			print_invalid_socket_id_error();
 			return -1;
 		}
 		port_numa[port_id] = socket_id;
@@ -474,9 +485,8 @@ parse_ringnuma_config(const char *q_arg)
 			return -1;
 		}
 		socket_id = (uint8_t)int_fld[FLD_SOCKET];
-		if (socket_id >= max_socket) {
-			printf("Invalid socket id, range is [0, %d]\n",
-				max_socket - 1);
+		if (new_socket_id(socket_id)) {
+			print_invalid_socket_id_error();
 			return -1;
 		}
 		ring_flag = (uint8_t)int_fld[FLD_FLAG];
@@ -732,12 +742,13 @@ launch_args_parse(int argc, char** argv)
 					   "invalid ring-numa configuration\n");
 			if (!strcmp(lgopts[opt_idx].name, "socket-num")) {
 				n = atoi(optarg);
-				if((uint8_t)n < max_socket)
+				if (!new_socket_id((uint8_t)n)) {
 					socket_num = (uint8_t)n;
-				else
+				} else {
+					print_invalid_socket_id_error();
 					rte_exit(EXIT_FAILURE,
-						"The socket number should be < %d\n",
-						max_socket);
+						"Invalid socket id");
+				}
 			}
 			if (!strcmp(lgopts[opt_idx].name, "mbuf-size")) {
 				n = atoi(optarg);
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index cec1cb904..d1041afa5 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -365,7 +365,8 @@ struct queue_stats_mappings *rx_queue_stats_mappings = rx_queue_stats_mappings_a
 uint16_t nb_tx_queue_stats_mappings = 0;
 uint16_t nb_rx_queue_stats_mappings = 0;
 
-unsigned max_socket = 0;
+unsigned int num_sockets = 0;
+unsigned int socket_ids[RTE_MAX_NUMA_NODES];
 
 #ifdef RTE_LIBRTE_BITRATE
 /* Bitrate statistics */
@@ -388,6 +389,22 @@ static void eth_event_callback(uint8_t port_id,
 static int all_ports_started(void);
 
 /*
+ * Helper function to check if socket is allready discovered.
+ * If yes, return positive value. If not, return zero.
+ */
+int
+new_socket_id(unsigned int socket_id)
+{
+	unsigned int i;
+
+	for (i = 0; i < num_sockets; i++) {
+		if (socket_ids[i] == socket_id)
+			return 0;
+	}
+	return 1;
+}
+
+/*
  * Setup default configuration.
  */
 static void
@@ -399,11 +416,14 @@ set_default_fwd_lcores_config(void)
 
 	nb_lc = 0;
 	for (i = 0; i < RTE_MAX_LCORE; i++) {
-		sock_num = rte_lcore_to_socket_id(i) + 1;
-		if (sock_num > max_socket) {
-			if (sock_num > RTE_MAX_NUMA_NODES)
-				rte_exit(EXIT_FAILURE, "Total sockets greater than %u\n", RTE_MAX_NUMA_NODES);
-			max_socket = sock_num;
+		sock_num = rte_lcore_to_socket_id(i);
+		if (new_socket_id(sock_num)) {
+			if (num_sockets >= RTE_MAX_NUMA_NODES) {
+				rte_exit(EXIT_FAILURE,
+					 "Total sockets greater than %u\n",
+					 RTE_MAX_NUMA_NODES);
+			}
+			socket_ids[num_sockets++] = sock_num;
 		}
 		if (!rte_lcore_is_enabled(i))
 			continue;
@@ -517,7 +537,7 @@ check_socket_id(const unsigned int socket_id)
 {
 	static int warning_once = 0;
 
-	if (socket_id >= max_socket) {
+	if (new_socket_id(socket_id)) {
 		if (!warning_once && numa_support)
 			printf("Warning: NUMA should be configured manually by"
 			       " using --port-numa-config and"
@@ -609,8 +629,9 @@ init_config(void)
 	if (numa_support) {
 		uint8_t i;
 
-		for (i = 0; i < max_socket; i++)
-			mbuf_pool_create(mbuf_data_size, nb_mbuf_per_pool, i);
+		for (i = 0; i < num_sockets; i++)
+			mbuf_pool_create(mbuf_data_size, nb_mbuf_per_pool,
+					 socket_ids[i]);
 	} else {
 		if (socket_num == UMA_NO_CONFIG)
 			mbuf_pool_create(mbuf_data_size, nb_mbuf_per_pool, 0);
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 3c6a59a54..e6c43ba03 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -343,7 +343,8 @@ extern lcoreid_t nb_lcores; /**< Number of logical cores probed at init time. */
 extern lcoreid_t nb_cfg_lcores; /**< Number of configured logical cores. */
 extern lcoreid_t nb_fwd_lcores; /**< Number of forwarding logical cores. */
 extern unsigned int fwd_lcores_cpuids[RTE_MAX_LCORE];
-extern unsigned max_socket;
+extern unsigned int num_sockets;
+extern unsigned int socket_ids[RTE_MAX_NUMA_NODES];
 
 /*
  * Configuration of Ethernet ports:
@@ -638,6 +639,7 @@ enum print_warning {
 	DISABLED_WARN
 };
 int port_id_is_invalid(portid_t port_id, enum print_warning warning);
+int new_socket_id(unsigned int socket_id);
 
 /*
  * Work-around of a compilation error with ICC on invocations of the
-- 
2.12.0

Re: [PATCH v4] app/testpmd: fix mempool creation by socket id

[Wu, Jingjing]

> -----Original Message-----
> From: Shahaf Shuler [mailto:shahafs@mellanox.com]
> Sent: Tuesday, May 9, 2017 3:29 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; thomas@monjalon.net; stable@dpdk.org
> Subject: [PATCH v4] app/testpmd: fix mempool creation by socket id
> 
> The test assumes the socket ids are contiguous. This is not necessarily the case
> on all servers and may cause mempool creation to fail.
> 
> Fixing it by detecting the list of valid socket ids and use it for the mempool
> creation.
> 
> Fixes: 7acf894d07d1 ("app/testpmd: detect numa socket count")
> 
> CC: stable@dpdk.org
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
Acked-by: Jingjing Wu <jingjing.wu@intel.com>

Re: [PATCH v4] app/testpmd: fix mempool creation by socket id

[Thomas Monjalon]

09/05/2017 10:54, Wu, Jingjing:
> From: Shahaf Shuler [mailto:shahafs@mellanox.com]
> > 
> > The test assumes the socket ids are contiguous. This is not necessarily the case
> > on all servers and may cause mempool creation to fail.
> > 
> > Fixing it by detecting the list of valid socket ids and use it for the mempool
> > creation.
> > 
> > Fixes: 7acf894d07d1 ("app/testpmd: detect numa socket count")
> > 
> > CC: stable@dpdk.org
> > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> Acked-by: Jingjing Wu <jingjing.wu@intel.com>

Applied, thanks