Thanks for your review. > On May 10, 2017, at 8:45 PM, Thomas Monjalon <tho...@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 <n...@opencloud.tech >> <mailto:n...@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
