eal: Set numa node value for system which not support it.

PATCH Applied Tonghao Zhang 3 participants 7 messages May 11, 2017 – Jun 26, 2017

Share & tools

Tonghao Zhang

author

2017-05-11 01:56 UTC #0 lore raw

PATCH v4 1/1 eal: Set numa node value for system which not support it.

The NUMA node information for PCI devices provided through sysfs is invalid for AMD Opteron(TM) Processor 62xx and 63xx on Red Hat Enterprise Linux 6, and VMs on some hypervisors. It is good to see more checking for valid values.

Signed-off-by Tonghao Zhang <[email protected]>

---
 lib/librte_eal/linuxapp/eal/eal_pci.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

lib/librte_eal/linuxapp/eal/eal_pci.c +9 -9

index 595622b..c817b4c 100644
@@ -310,18 +310,18 @@
 			dev->max_vfs = (uint16_t)tmp;
 	}
 
-	/* get numa node */
+	/* get numa node, default to 0 if not present */
 	snprintf(filename, sizeof(filename), "%s/numa_node",
 		 dirname);
-	if (access(filename, R_OK) != 0) {
-		/* if no NUMA support, set default to 0 */
-		dev->device.numa_node = 0;
-	} else {
-		if (eal_parse_sysfs_value(filename, &tmp) < 0) {
-			free(dev);
-			return -1;
-		}
+
+	if (eal_parse_sysfs_value(filename, &tmp) == 0 &&
+		tmp < RTE_MAX_NUMA_NODES)
 		dev->device.numa_node = tmp;
+	else {
+		RTE_LOG(WARNING, EAL,
+			"numa_node is invalid or not present. "
+			"Set it 0 as default\n");
+		dev->device.numa_node = 0;
 	}
 
 	rte_pci_device_name(addr, dev->name, sizeof(dev->name));

Signature · 1 line

1.8.3.1

Sergio Gonzalez Monroy

Acked-by on v4 1/1

2017-06-22 15:15 UTC #1 lore raw

Just fyi, the summary line should be lowercase apart from acronyms (DPDK guidelines).

Quoted 7 lines · Tonghao Zhang

The NUMA node information for PCI devices provided through sysfs is invalid for AMD Opteron(TM) Processor 62xx and 63xx on Red Hat Enterprise Linux 6, and VMs on some hypervisors. It is good to see more checking for valid values.
Signed-off-by Tonghao Zhang <[email protected]>
---

IMHO the message could be slightly improved by adding some of the replies that you made to your v3. ie. Typical wrong numa node in VMs

$ cat /sys/devices/pci0000:00/0000:00:18.6/numa_node
-1

Quoted 25 lines

  lib/librte_eal/linuxapp/eal/eal_pci.c | 18 +++++++++---------
  1 file changed, 9 insertions(+), 9 deletions(-)

lib/librte_eal/linuxapp/eal/eal_pci.c +3 -9

index 595622b..c817b4c 100644
@@ -310,18 +310,18 @@
  			dev->max_vfs = (uint16_t)tmp;
  	}
 
-	/* get numa node */
+	/* get numa node, default to 0 if not present */
  	snprintf(filename, sizeof(filename), "%s/numa_node",
  		 dirname);
-	if (access(filename, R_OK) != 0) {
-		/* if no NUMA support, set default to 0 */
-		dev->device.numa_node = 0;
-	} else {
-		if (eal_parse_sysfs_value(filename, &tmp) < 0) {
-			free(dev);
-			return -1;
-		}
+
+	if (eal_parse_sysfs_value(filename, &tmp) == 0 &&

lib/librte_eal/linuxapp/eal/eal_pci.c line 327

Quoted 10 lines

lib/librte_eal/linuxapp/eal/eal_pci.c +6 -0

+		tmp < RTE_MAX_NUMA_NODES)
  		dev->device.numa_node = tmp;
+	else {
+		RTE_LOG(WARNING, EAL,
+			"numa_node is invalid or not present. "
+			"Set it 0 as default\n");
+		dev->device.numa_node = 0;
  	}
 
  	rte_pci_device_name(addr, dev->name, sizeof(dev->name));

The code changes look fine, so I leave it to Thomas regarding the commit message :)

Acked-by Sergio Gonzalez Monroy <[email protected]>

Thomas Monjalon

Applied

2017-06-23 13:02 UTC #2 lore raw

22/06/2017 17:15, Sergio Gonzalez Monroy:

Quoted 18 lines

Just fyi, the summary line should be lowercase apart from acronyms (DPDK guidelines).
Quoted 7 lines · Tonghao Zhang
The NUMA node information for PCI devices provided through sysfs is invalid for AMD Opteron(TM) Processor 62xx and 63xx on Red Hat Enterprise Linux 6, and VMs on some hypervisors. It is good to see more checking for valid values.
Signed-off-by Tonghao Zhang <[email protected]>
---
IMHO the message could be slightly improved by adding some of the replies that you made to your v3.
ie. Typical wrong numa node in VMs
$ cat /sys/devices/pci0000:00/0000:00:18.6/numa_node
-1

[...]

Quoted 4 lines

The code changes look fine, so I leave it to Thomas regarding the commit message :)
Acked-by Sergio Gonzalez Monroy <[email protected]>

Applied, thanks

Sergio Gonzalez Monroy

2017-06-26 09:15 UTC #3 lore raw

Quoted 16 lines · Thomas Monjalon

22/06/2017 17:15, Sergio Gonzalez Monroy:
Quoted 15 lines
Just fyi, the summary line should be lowercase apart from acronyms (DPDK guidelines).
Quoted 7 lines · Tonghao Zhang
The NUMA node information for PCI devices provided through sysfs is invalid for AMD Opteron(TM) Processor 62xx and 63xx on Red Hat Enterprise Linux 6, and VMs on some hypervisors. It is good to see more checking for valid values.
Signed-off-by Tonghao Zhang <[email protected]>
---
IMHO the message could be slightly improved by adding some of the replies that you made to your v3.
ie. Typical wrong numa node in VMs

Quoted 8 lines

Quoted 2 lines
$ cat /sys/devices/pci0000:00/0000:00:18.6/numa_node
-1
[...]
Quoted 4 lines
The code changes look fine, so I leave it to Thomas regarding the commit message :)
Acked-by Sergio Gonzalez Monroy <[email protected]>
Applied, thanks

It looks like some systems have quite a few devices that report -1 as numa_node value causing lots of warning messages being printed. Quick fixes that come to mind would be:
1) Change log level to DEBUG
2) Add static var to only print the message once.

I also think that the message itself should show at least the BDF to at least know which devices are reporting bad numa_node values.

Thoughts?

Sergio

Thomas Monjalon

2017-06-26 09:39 UTC #4 lore raw

26/06/2017 11:14, Sergio Gonzalez Monroy:

Quoted 23 lines · Thomas Monjalon

Quoted 22 lines
22/06/2017 17:15, Sergio Gonzalez Monroy:
Quoted 17 lines
Just fyi, the summary line should be lowercase apart from acronyms (DPDK guidelines).
Quoted 7 lines · Tonghao Zhang
The NUMA node information for PCI devices provided through sysfs is invalid for AMD Opteron(TM) Processor 62xx and 63xx on Red Hat Enterprise Linux 6, and VMs on some hypervisors. It is good to see more checking for valid values.
Signed-off-by Tonghao Zhang <[email protected]>
---
IMHO the message could be slightly improved by adding some of the replies that you made to your v3.
ie. Typical wrong numa node in VMs
$ cat /sys/devices/pci0000:00/0000:00:18.6/numa_node
-1
[...]
Quoted 3 lines
The code changes look fine, so I leave it to Thomas regarding the commit message :)

Quoted 7 lines

Quoted 2 lines
Quoted 1 line
Acked-by Sergio Gonzalez Monroy <[email protected]>
Applied, thanks
It looks like some systems have quite a few devices that report -1 as numa_node value causing lots of warning messages being printed. Quick fixes that come to mind would be:
1) Change log level to DEBUG

As it is important for performance, it should not be just for DEBUG.

Quoted 1 line

2) Add static var to only print the message once.

Yes good idea.

Quoted 2 lines

I also think that the message itself should show at least the BDF to at least know which devices are reporting bad numa_node values.

With the static variable, we will have only the first device BDF. Is it relevant?

Sergio Gonzalez Monroy

2017-06-26 12:50 UTC #5 lore raw

Quoted 32 lines · Thomas Monjalon

26/06/2017 11:14, Sergio Gonzalez Monroy:
Quoted 23 lines · Thomas Monjalon
Quoted 22 lines
22/06/2017 17:15, Sergio Gonzalez Monroy:
Quoted 17 lines
Just fyi, the summary line should be lowercase apart from acronyms (DPDK guidelines).
Quoted 7 lines · Tonghao Zhang
The NUMA node information for PCI devices provided through sysfs is invalid for AMD Opteron(TM) Processor 62xx and 63xx on Red Hat Enterprise Linux 6, and VMs on some hypervisors. It is good to see more checking for valid values.
Signed-off-by Tonghao Zhang <[email protected]>
---
IMHO the message could be slightly improved by adding some of the replies that you made to your v3.
ie. Typical wrong numa node in VMs
$ cat /sys/devices/pci0000:00/0000:00:18.6/numa_node
-1
[...]
Quoted 3 lines
The code changes look fine, so I leave it to Thomas regarding the commit message :)
Quoted 6 lines
Quoted 2 lines
Quoted 1 line
Acked-by Sergio Gonzalez Monroy <[email protected]>
Applied, thanks
It looks like some systems have quite a few devices that report -1 as numa_node value causing lots of warning messages being printed. Quick fixes that come to mind would be:
1) Change log level to DEBUG
As it is important for performance, it should not be just for DEBUG.

Quoted 8 lines

Quoted 1 line
2) Add static var to only print the message once.
Yes good idea.
Quoted 2 lines
I also think that the message itself should show at least the BDF to at least know which devices are reporting bad numa_node values.
With the static variable, we will have only the first device BDF. Is it relevant?

I think it is relevant if it affects a device used by DPDK, but we don't know that when doing full pci_scan.

At least on x86 platforms we usually see many PCI devices without numa_node:
ls /sys/bus/pci/devices | xargs -n 1 -I {} head -v "/sys/bus/pci/devices/{}/numa_node"

A single warning is not going to mean much if all platforms have PCI devices without proper numa_node, right?

A more cleaner solution might be to leave -1 if we failed to parse numa_node, then on rte_pci_probe_one_driver after checking if it is blacklisted check if socket_id is -1 and show warning message defaulting to 0?

I would be inclined to:
a) leave it as it is with DEBUG log level, also showing PCI BDF (very noisy in debug mode).
b) show the warning and default to 0 in rte_pci_probe_one_driver, showing only relevant devices.

Sergio

Thomas Monjalon

2017-06-26 14:36 UTC #6 lore raw

26/06/2017 14:50, Sergio Gonzalez Monroy:

Quoted 57 lines · Thomas Monjalon

Quoted 32 lines
26/06/2017 11:14, Sergio Gonzalez Monroy:
Quoted 23 lines · Thomas Monjalon
Quoted 22 lines
22/06/2017 17:15, Sergio Gonzalez Monroy:
Quoted 17 lines
Just fyi, the summary line should be lowercase apart from acronyms (DPDK guidelines).
Quoted 7 lines · Tonghao Zhang
The NUMA node information for PCI devices provided through sysfs is invalid for AMD Opteron(TM) Processor 62xx and 63xx on Red Hat Enterprise Linux 6, and VMs on some hypervisors. It is good to see more checking for valid values.
Signed-off-by Tonghao Zhang <[email protected]>
---
IMHO the message could be slightly improved by adding some of the replies that you made to your v3.
ie. Typical wrong numa node in VMs
$ cat /sys/devices/pci0000:00/0000:00:18.6/numa_node
-1
[...]
Quoted 3 lines
The code changes look fine, so I leave it to Thomas regarding the commit message :)
Quoted 6 lines
Quoted 2 lines
Quoted 1 line
Acked-by Sergio Gonzalez Monroy <[email protected]>
Applied, thanks
It looks like some systems have quite a few devices that report -1 as numa_node value causing lots of warning messages being printed. Quick fixes that come to mind would be:
1) Change log level to DEBUG
As it is important for performance, it should not be just for DEBUG.
Quoted 8 lines
Quoted 1 line
2) Add static var to only print the message once.
Yes good idea.
Quoted 2 lines
I also think that the message itself should show at least the BDF to at least know which devices are reporting bad numa_node values.
With the static variable, we will have only the first device BDF. Is it relevant?
I think it is relevant if it affects a device used by DPDK, but we don't know that when doing full pci_scan.
At least on x86 platforms we usually see many PCI devices without numa_node:
ls /sys/bus/pci/devices | xargs -n 1 -I {} head -v "/sys/bus/pci/devices/{}/numa_node"
A single warning is not going to mean much if all platforms have PCI devices without proper numa_node, right?
A more cleaner solution might be to leave -1 if we failed to parse numa_node, then on rte_pci_probe_one_driver after checking if it is blacklisted check if socket_id is -1 and show warning message defaulting to 0?

Quoted 5 lines

I would be inclined to:
a) leave it as it is with DEBUG log level, also showing PCI BDF (very noisy in debug mode).
b) show the warning and default to 0 in rte_pci_probe_one_driver, showing only relevant devices.

Looks a good proposal Sergio!

Thanks