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

[nickcooper-zhangtonghao <nic@opencloud.tech>]

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