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