Skip to contentSkip to conversation

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

PATCH Superseded Tonghao Zhang 2 participants 4 messages May 9, 2017 – Jun 1, 2017

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Tonghao Zhang

author
#0 lore raw

PATCH v3 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 | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)
lib/librte_eal/linuxapp/eal/eal_pci.c +6 -10
index 595622b..95a051f 100644
@@ -310,19 +310,15 @@
 			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
+		dev->device.numa_node = 0;
 
 	rte_pci_device_name(addr, dev->name, sizeof(dev->name));
 	dev->device.name = dev->name;
Signature · 1 line
1.8.3.1
#1 lore raw

09/05/2017 09:30, Tonghao Zhang:

Quoted 3 lines

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.

Sorry I don't understand the range of affected platforms.
Is it only on Opteron? Opteron with RHEL6? Is it fixed in recent kernels? Which hypervisors? with which kernel?

Quoted 1 line

It is good to see more checking for valid values.

If values are wrong, what can we do?
Here you check that value is not too high.
What about other kind of wrong values?

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

[...]

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

Quoted 7 lines
lib/librte_eal/linuxapp/eal/eal_pci.c +1 -4
-	/* 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;

Why removing the access() check?

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

Quoted 12 lines
lib/librte_eal/linuxapp/eal/eal_pci.c +5 -6
-	} 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
+		dev->device.numa_node = 0;

It would deserve at least a warning log.

author
#2 lore raw

Thanks for your review.

Quoted 9 lines · Thomas Monjalon
Quoted 4 lines

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.

Sorry I don't understand the range of affected platforms.
Is it only on Opteron? Opteron with RHEL6? Is it fixed in recent kernels? Which hypervisors? with which kernel?

I get numa info from web: https://access.redhat.com/solutions/349913 <https://access.redhat.com/solutions/349913>
and VMs which OS is CentOS 7.0 and kernel is 3.10, are running on VMware fusion.

This VMs numa node is -1. For example:

$ cat /sys/devices/pci0000:00/0000:00:18.6/numa_node
-1
Quoted 8 lines
Quoted 1 line

It is good to see more checking for valid values.

If values are wrong, what can we do?
Here you check that value is not too high.
What about other kind of wrong values?

Quoted 1 line
Signed-off-by Tonghao Zhang <[email protected] <mailto:[email protected]>>

[...]

Quoted 9 lines

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

Quoted 7 lines
lib/librte_eal/linuxapp/eal/eal_pci.c +1 -1
-	/* get numa node */
+	/* get numa node, default to 0 if not present */

	snprintf(filename, sizeof(filename), "%s/numa_node",
		 dirname);
lib/librte_eal/linuxapp/eal/eal_pci.c +0 -3
-	if (access(filename, R_OK) != 0) {
-		/* if no NUMA support, set default to 0 */
-		dev->device.numa_node = 0;

Why removing the access() check?

I review the code of eal_parse_sysfs_value(). If the ‘filename’ cannot be accessed. the eal_parse_sysfs_value cannot open it, and returen -1. so using eal_parse_sysfs_value is simple.

Quoted 15 lines

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

Quoted 13 lines
lib/librte_eal/linuxapp/eal/eal_pci.c +3 -5
-	} 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;
lib/librte_eal/linuxapp/eal/eal_pci.c +2 -1
-	}
+	else
+		dev->device.numa_node = 0;

It would deserve at least a warning log.

Yes

author
#3 lore raw

Did you think this patch is necessary. I submitted v4.
v4:
http://dpdk.org/dev/patchwork/patch/24212/ <http://dpdk.org/dev/patchwork/patch/24212/>

Thanks.
Nick

Quoted 61 lines · nickcooper-zhangtonghao

Thanks for your review.

Quoted 9 lines · On May 10, 2017, at 8:45 PM, Thomas Monjalon <[email protected]…
Quoted 4 lines

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.

Sorry I don't understand the range of affected platforms.
Is it only on Opteron? Opteron with RHEL6? Is it fixed in recent kernels? Which hypervisors? with which kernel?

I get numa info from web: https://access.redhat.com/solutions/349913 <https://access.redhat.com/solutions/349913><https://access.redhat.com/solutions/349913 <https://access.redhat.com/solutions/349913>>
and VMs which OS is CentOS 7.0 and kernel is 3.10, are running on VMware fusion.

This VMs numa node is -1. For example:

$ cat /sys/devices/pci0000:00/0000:00:18.6/numa_node
-1
Quoted 8 lines
Quoted 1 line

It is good to see more checking for valid values.

If values are wrong, what can we do?
Here you check that value is not too high.
What about other kind of wrong values?

Quoted 1 line
Signed-off-by Tonghao Zhang <[email protected] <mailto:[email protected]><mailto:[email protected] <mailto:[email protected]>>>

[...]

Quoted 9 lines

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

Quoted 7 lines
lib/librte_eal/linuxapp/eal/eal_pci.c +1 -1
-	/* get numa node */
+	/* get numa node, default to 0 if not present */

	snprintf(filename, sizeof(filename), "%s/numa_node",
		 dirname);
lib/librte_eal/linuxapp/eal/eal_pci.c +0 -3
-	if (access(filename, R_OK) != 0) {
-		/* if no NUMA support, set default to 0 */
-		dev->device.numa_node = 0;

Why removing the access() check?

I review the code of eal_parse_sysfs_value(). If the ‘filename’ cannot be accessed. the eal_parse_sysfs_value cannot open it, and returen -1. so using eal_parse_sysfs_value is simple.

Quoted 15 lines

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

Quoted 13 lines
lib/librte_eal/linuxapp/eal/eal_pci.c +3 -5
-	} 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;
lib/librte_eal/linuxapp/eal/eal_pci.c +2 -1
-	}
+	else
+		dev->device.numa_node = 0;

It would deserve at least a warning log.

Yes