[dpdk-dev] [PATCH v1] app/test: fix pmd_perf issue in no NUMA case

[dpdk-dev] [PATCH v1] app/test: fix pmd_perf issue in no NUMA case

[Cunming Liang]

Reported-by: Jayakumar, Muthurajan <muthurajan.jayakumar@intel.com>
Signed-off-by: Cunming Liang <cunming.liang@intel.com>
---
 app/test/test_pmd_perf.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/app/test/test_pmd_perf.c b/app/test/test_pmd_perf.c
index 1fd6843..6f218f7 100644
--- a/app/test/test_pmd_perf.c
+++ b/app/test/test_pmd_perf.c
@@ -321,6 +321,19 @@ alloc_lcore(uint16_t socketid)
 	return (uint16_t)-1;
 }
 
+static int
+get_socket_id(uint8_t port_id)
+{
+	int socket_id;
+
+	socket_id = rte_eth_dev_socket_id(port_id);
+	if (socket_id < 0)
+		/* enforce using socket 0 when no NUMA support */
+		socket_id = 0;
+
+	return socket_id;
+}
+
 volatile uint64_t stop;
 uint64_t count;
 uint64_t drop;
@@ -727,7 +740,7 @@ test_pmd_perf(void)
 	num = 0;
 	for (portid = 0; portid < nb_ports; portid++) {
 		if (socketid == -1) {
-			socketid = rte_eth_dev_socket_id(portid);
+			socketid = get_socket_id(portid);
 			slave_id = alloc_lcore(socketid);
 			if (slave_id == (uint16_t)-1) {
 				printf("No avail lcore to run test\n");
@@ -737,7 +750,7 @@ test_pmd_perf(void)
 			       slave_id, socketid);
 		}
 
-		if (socketid != rte_eth_dev_socket_id(portid)) {
+		if (socketid != get_socket_id(portid)) {
 			printf("Skip port %d\n", portid);
 			continue;
 		}
@@ -818,7 +831,7 @@ test_pmd_perf(void)
 
 	/* port tear down */
 	for (portid = 0; portid < nb_ports; portid++) {
-		if (socketid != rte_eth_dev_socket_id(portid))
+		if (socketid != get_socket_id(portid))
 			continue;
 
 		rte_eth_dev_stop(portid);
-- 
1.8.1.4

Re: [dpdk-dev] [PATCH v1] app/test: fix pmd_perf issue in no NUMA case

[Jayakumar, Muthurajan]

Thank you Steve.
Acked.

Thanks 
M Jay
http://www.dpdk.org



-----Original Message-----
From: Liang, Cunming 
Sent: Sunday, June 07, 2015 11:33 PM
To: dev@dpdk.org
Cc: Jayakumar, Muthurajan; Liang, Cunming
Subject: [PATCH v1] app/test: fix pmd_perf issue in no NUMA case

Reported-by: Jayakumar, Muthurajan <muthurajan.jayakumar@intel.com>
Signed-off-by: Cunming Liang <cunming.liang@intel.com>
---
 app/test/test_pmd_perf.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/app/test/test_pmd_perf.c b/app/test/test_pmd_perf.c index 1fd6843..6f218f7 100644
--- a/app/test/test_pmd_perf.c
+++ b/app/test/test_pmd_perf.c
@@ -321,6 +321,19 @@ alloc_lcore(uint16_t socketid)
 	return (uint16_t)-1;
 }
 
+static int
+get_socket_id(uint8_t port_id)
+{
+	int socket_id;
+
+	socket_id = rte_eth_dev_socket_id(port_id);
+	if (socket_id < 0)
+		/* enforce using socket 0 when no NUMA support */
+		socket_id = 0;
+
+	return socket_id;
+}
+
 volatile uint64_t stop;
 uint64_t count;
 uint64_t drop;
@@ -727,7 +740,7 @@ test_pmd_perf(void)
 	num = 0;
 	for (portid = 0; portid < nb_ports; portid++) {
 		if (socketid == -1) {
-			socketid = rte_eth_dev_socket_id(portid);
+			socketid = get_socket_id(portid);
 			slave_id = alloc_lcore(socketid);
 			if (slave_id == (uint16_t)-1) {
 				printf("No avail lcore to run test\n"); @@ -737,7 +750,7 @@ test_pmd_perf(void)
 			       slave_id, socketid);
 		}
 
-		if (socketid != rte_eth_dev_socket_id(portid)) {
+		if (socketid != get_socket_id(portid)) {
 			printf("Skip port %d\n", portid);
 			continue;
 		}
@@ -818,7 +831,7 @@ test_pmd_perf(void)
 
 	/* port tear down */
 	for (portid = 0; portid < nb_ports; portid++) {
-		if (socketid != rte_eth_dev_socket_id(portid))
+		if (socketid != get_socket_id(portid))
 			continue;
 
 		rte_eth_dev_stop(portid);
--
1.8.1.4

Re: [dpdk-dev] [PATCH v1] app/test: fix pmd_perf issue in no NUMA case

[Thomas Monjalon]

2015-06-08 14:33, Cunming Liang:
> Reported-by: Jayakumar, Muthurajan <muthurajan.jayakumar@intel.com>
> Signed-off-by: Cunming Liang <cunming.liang@intel.com>

Please explain exactly what you try to fix.
Is it still needed since this patch?
	http://dpdk.org/browse/dpdk/commit/?id=94ef2964148a4540

> +	socket_id = rte_eth_dev_socket_id(port_id);
> +	if (socket_id < 0)
> +		/* enforce using socket 0 when no NUMA support */
> +		socket_id = 0;

Re: [dpdk-dev] [PATCH v1] app/test: fix pmd_perf issue in no NUMA case

[Liang, Cunming]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, June 23, 2015 5:02 AM
> To: Liang, Cunming
> Cc: dev@dpdk.org; Jayakumar, Muthurajan
> Subject: Re: [dpdk-dev] [PATCH v1] app/test: fix pmd_perf issue in no NUMA case
> 
> 2015-06-08 14:33, Cunming Liang:
> > Reported-by: Jayakumar, Muthurajan <muthurajan.jayakumar@intel.com>
> > Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> 
> Please explain exactly what you try to fix.
> Is it still needed since this patch?
> 	http://dpdk.org/browse/dpdk/commit/?id=94ef2964148a4540
> 
> > +	socket_id = rte_eth_dev_socket_id(port_id);
> > +	if (socket_id < 0)
> > +		/* enforce using socket 0 when no NUMA support */
> > +		socket_id = 0;

The referred patch 94ef29 fixes eal_cpu_socket_id() for the socket id of a logical core.
It's not for the case here which trying to get socket id of a port.
The original purpose of the patch is to allow the unit test works even without NUMA support.

Now I have a look on the function definition, when socket could not be determined, the default shall be zero instead of -1.
So I think the below change should be made in eal_pci.c

/*
 * Return the NUMA socket to which an Ethernet device is connected
 * @return
 *   The NUMA socket id to which the Ethernet device is connected or
 *   a default of zero if the socket could not be determined.
 *   -1 is returned is the port_id value is out of range.
 */
extern int rte_eth_dev_socket_id(uint8_t port_id);

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index d2adc66..8f9f136 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -317,8 +317,8 @@ pci_scan_one(const char *dirname, uint16_t domain, uint8_t bus,
        snprintf(filename, sizeof(filename), "%s/numa_node",
                 dirname);
        if (access(filename, R_OK) != 0) {
-               /* if no NUMA support just set node to -1 */
-               dev->numa_node = -1;
+               /* if no NUMA support just set node to 0 */
+               dev->numa_node = 0;
        } else {
                if (eal_parse_sysfs_value(filename, &tmp) < 0) {
                        free(dev);

/Steve

[dpdk-dev] [PATCH] eal/linux: fix negative value for undetermined numa_node

[Cunming Liang]

The patch sets zero as the default value of pci device numa_node
if the socket could not be determined.
It provides the same default value as FreeBSD which has no NUMA support,
and makes the return value of rte_eth_dev_socket_id() be consistent
with the API description.

Signed-off-by: Cunming Liang <cunming.liang@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_pci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 0e62f65..f573092 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -325,8 +325,8 @@ pci_scan_one(const char *dirname, uint16_t domain, uint8_t bus,
 	snprintf(filename, sizeof(filename), "%s/numa_node",
 		 dirname);
 	if (access(filename, R_OK) != 0) {
-		/* if no NUMA support just set node to -1 */
-		dev->numa_node = -1;
+		/* if no NUMA support, set default to 0 */
+		dev->numa_node = 0;
 	} else {
 		if (eal_parse_sysfs_value(filename, &tmp) < 0) {
 			free(dev);
-- 
2.4.3

Re: [dpdk-dev] [PATCH] eal/linux: fix negative value for undetermined numa_node

[Matthew Hall]

I asked about this many months ago and was informed that "-1" is a "standard 
error value" that I should expect from these APIs when NUMA is not present. 
Now we're saying I have to change my code again to handle a zero value?

Also not sure how to tell the difference between no NUMA, something running on 
socket zero, and something with multiple sockets. Seems like we need a bit of 
thought about how the NUMA APIs should behave overall.

Matthew.

On Fri, Jul 31, 2015 at 09:36:12AM +0800, Cunming Liang wrote:
> The patch sets zero as the default value of pci device numa_node
> if the socket could not be determined.
> It provides the same default value as FreeBSD which has no NUMA support,
> and makes the return value of rte_eth_dev_socket_id() be consistent
> with the API description.
> 
> Signed-off-by: Cunming Liang <cunming.liang@intel.com>

Re: [dpdk-dev] [PATCH] eal/linux: fix negative value for undetermined numa_node

[Liang, Cunming]

Hi,

On 8/1/2015 11:56 AM, Matthew Hall wrote:
> I asked about this many months ago and was informed that "-1" is a "standard
> error value" that I should expect from these APIs when NUMA is not present.
> Now we're saying I have to change my code again to handle a zero value?
>
> Also not sure how to tell the difference between no NUMA, something running on
> socket zero, and something with multiple sockets. Seems like we need a bit of
> thought about how the NUMA APIs should behave overall.
>
> Matthew.
>
> On Fri, Jul 31, 2015 at 09:36:12AM +0800, Cunming Liang wrote:
>> The patch sets zero as the default value of pci device numa_node
>> if the socket could not be determined.
>> It provides the same default value as FreeBSD which has no NUMA support,
>> and makes the return value of rte_eth_dev_socket_id() be consistent
>> with the API description.
>>
>> Signed-off-by: Cunming Liang <cunming.liang@intel.com>
>>
>>
/*
  * Return the NUMA socket to which an Ethernet device is connected
  *
  * @param port_id
  *   The port identifier of the Ethernet device
  * @return
  *   The NUMA socket id to which the Ethernet device is connected or
  *   a default of zero if the socket could not be determined.
  *   -1 is returned is the port_id value is out of range.
  */
extern int rte_eth_dev_socket_id(uint8_t port_id);

According to the API definition, if the socket could not be determined, 
a default of zero will take.
The '-1' is returned when the port_id value is out of range.

To your concern, "difference between no NUMA, something running on 
socket zero, and something with multiple sockets.".
The latter two belongs to the same situation, that is the numa_node 
stores the NUMA id.
So in fact the concern is about using '-1' or '0' when there's no NUMA 
detect.
If we won't plan to redefine the API return value, the fix patch is 
reasonable.

Btw, if it returns '-1' when no NUMA is detected, what will you do, do 
condition check '-1' and then use node 0 instead ?
In that way, you can't distinguish '-'1 is out of range port_id error or 
no NUMA detection error.
If it is, why not follow the API definition.

/Steve

Re: [dpdk-dev] [PATCH] eal/linux: fix negative value for undetermined numa_node

[Matthew Hall]

On Mon, Aug 03, 2015 at 09:46:54AM +0800, Liang, Cunming wrote:
> According to the API definition, if the socket could not be determined, a
> default of zero will take.
> The '-1' is returned when the port_id value is out of range.

Yes, but when I asked the exact same question and was told the documentation 
was wrong not the -1 return value.

> To your concern, "difference between no NUMA, something running on socket
> zero, and something with multiple sockets.".
> The latter two belongs to the same situation, that is the numa_node stores
> the NUMA id.
> So in fact the concern is about using '-1' or '0' when there's no NUMA
> detect.
> If we won't plan to redefine the API return value, the fix patch is
> reasonable.
> 
> Btw, if it returns '-1' when no NUMA is detected, what will you do, do
> condition check '-1' and then use node 0 instead ?
> In that way, you can't distinguish '-'1 is out of range port_id error or no
> NUMA detection error.

I asked that question also, and the answer I got was to use node 0 instead.

> If it is, why not follow the API definition.

Sure, if nobody objects like the last time I asked. But this will change the 
user behavior as I am looking for the -1 now.

> /Steve

Matthew.

Re: [dpdk-dev] [PATCH] eal/linux: fix negative value for undetermined numa_node

[Thomas Monjalon]

2015-08-02 22:04, Matthew Hall:
> On Mon, Aug 03, 2015 at 09:46:54AM +0800, Liang, Cunming wrote:
> > According to the API definition, if the socket could not be determined, a
> > default of zero will take.
> > The '-1' is returned when the port_id value is out of range.
> 
> Yes, but when I asked the exact same question and was told the documentation 
> was wrong not the -1 return value.

It was an error.
The correct API, as defined, is to return 0 when there is no NUMA.
This patch now reverts this one:
	http://dpdk.org/browse/dpdk/commit/?id=a4ff75496a18
Thanks Matthew for helping to clarify it.

> Sure, if nobody objects like the last time I asked. But this will change the 
> user behavior as I am looking for the -1 now.

You're right. But this fix follows the defined API, so it's rather an API fix
than an API break.

Applied, thanks