Re: [dpdk-dev] [PATCH v2 1/5] eal: Set numa node value for system which not support NUMA.

[Stephen Hemminger]

On Thu,  5 Jan 2017 04:01:45 -0800
nickcooper-zhangtonghao <n...@opencloud.tech> 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.
> +              * In the upstream linux kernel, the numa_node is an integer,
> +              * which data type is int, not unsigned long.
> +              */
> +             dev->device.numa_node = (int)tmp > 0 ? (int)tmp : 0;
>       }

It is good to see more checking for valid values.  I suspect that other systems
may have the same problem.  My preference would to have the code comment generic
and to have the precise details of about where this was observed in the commit
log. 

The following would do same thing but be simpler:

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c 
b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 43501342..9f09cd98 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -306,19 +306,12 @@ pci_scan_one(const char *dirname, uint16_t domain, 
uint8_t bus,
                        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;
-       }
 
        /* parse resources */
        snprintf(filename, sizeof(filename), "%s/resource", dirname);