diff mbox

[dpdk-dev,v3] eal: Set numa node value for system which not support it.

Message ID 1494315002-41982-1-git-send-email-nic@opencloud.tech (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

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

Commit Message

nickcooper-zhangtonghao May 9, 2017, 7:30 a.m. UTC 
  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 <nic@opencloud.tech>
---
 lib/librte_eal/linuxapp/eal/eal_pci.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

Comments

Thomas Monjalon May 10, 2017, 12:45 p.m. UTC | #1 
09/05/2017 09:30, 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.

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?

> 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?

> Signed-off-by: Tonghao Zhang <nic@opencloud.tech>
[...]
> -	/* 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?

> -	} 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.
nickcooper-zhangtonghao May 10, 2017, 2:20 p.m. UTC | #2 
Thanks for your review.

> On May 10, 2017, at 8:45 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
> 
>> 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


>> 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?
> 
>> Signed-off-by: Tonghao Zhang <nic@opencloud.tech <mailto:nic@opencloud.tech>>
> [...]
>> -	/* 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?

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.

> 
>> -	} 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.

Yes
nickcooper-zhangtonghao June 1, 2017, 1:22 a.m. UTC | #3 
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

> On May 10, 2017, at 10:20 PM, nickcooper-zhangtonghao <nic@opencloud.tech> wrote:
> 
> Thanks for your review.
> 
>> On May 10, 2017, at 8:45 PM, Thomas Monjalon <thomas@monjalon.net <mailto:thomas@monjalon.net>> wrote:
>> 
>>> 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
> 
> 
>>> 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?
>> 
>>> Signed-off-by: Tonghao Zhang <nic@opencloud.tech <mailto:nic@opencloud.tech><mailto:nic@opencloud.tech <mailto:nic@opencloud.tech>>>
>> [...]
>>> -	/* 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?
> 
> 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.
> 
>> 
>>> -	} 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.
> 
> Yes
diff mbox

Patch

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 595622b..95a051f 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -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;