* [dpdk-dev] [PATCH v3] eal: Set numa node value for system which not support it.
@ 2017-05-09 6:45 Tonghao Zhang
0 siblings, 0 replies; 6+ messages in thread
From: Tonghao Zhang @ 2017-05-09 6:45 UTC (permalink / raw)
To: dev; +Cc: stephen, 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 <nic@opencloud.tech>
---
lib/librte_eal/linuxapp/eal/eal_pci.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 595622b..19ef8bd 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;
--
1.8.3.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH v3] eal: Set numa node value for system which not support it.
2017-05-10 14:20 ` nickcooper-zhangtonghao
@ 2017-06-01 1:22 ` nickcooper-zhangtonghao
0 siblings, 0 replies; 6+ messages in thread
From: nickcooper-zhangtonghao @ 2017-06-01 1:22 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, stephen
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH v3] eal: Set numa node value for system which not support it.
2017-05-10 12:45 ` Thomas Monjalon
@ 2017-05-10 14:20 ` nickcooper-zhangtonghao
2017-06-01 1:22 ` nickcooper-zhangtonghao
0 siblings, 1 reply; 6+ messages in thread
From: nickcooper-zhangtonghao @ 2017-05-10 14:20 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, stephen
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH v3] eal: Set numa node value for system which not support it.
2017-05-09 7:30 Tonghao Zhang
@ 2017-05-10 12:45 ` Thomas Monjalon
2017-05-10 14:20 ` nickcooper-zhangtonghao
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Monjalon @ 2017-05-10 12:45 UTC (permalink / raw)
To: Tonghao Zhang; +Cc: dev, stephen
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [dpdk-dev] [PATCH v3] eal: Set numa node value for system which not support it.
@ 2017-05-09 7:30 Tonghao Zhang
2017-05-10 12:45 ` Thomas Monjalon
0 siblings, 1 reply; 6+ messages in thread
From: Tonghao Zhang @ 2017-05-09 7:30 UTC (permalink / raw)
To: dev; +Cc: stephen, 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 <nic@opencloud.tech>
---
lib/librte_eal/linuxapp/eal/eal_pci.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
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;
--
1.8.3.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [dpdk-dev] [PATCH v3] eal: Set numa node value for system which not support it.
@ 2017-05-09 6:55 Tonghao Zhang
0 siblings, 0 replies; 6+ messages in thread
From: Tonghao Zhang @ 2017-05-09 6:55 UTC (permalink / raw)
To: dev; +Cc: stephen, 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 <nic@opencloud.tech>
---
lib/librte_eal/linuxapp/eal/eal_pci.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
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;
--
1.8.3.1
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-06-01 1:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-09 6:45 [dpdk-dev] [PATCH v3] eal: Set numa node value for system which not support it Tonghao Zhang
2017-05-09 6:55 Tonghao Zhang
2017-05-09 7:30 Tonghao Zhang
2017-05-10 12:45 ` Thomas Monjalon
2017-05-10 14:20 ` nickcooper-zhangtonghao
2017-06-01 1:22 ` nickcooper-zhangtonghao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).