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

[Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>]

Just fyi, the summary line should be lowercase apart from acronyms (DPDK 
guidelines).

On 11/05/2017 02:56, Tonghao Zhang 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.
> It is good to see more checking for valid values.
>
> Signed-off-by: Tonghao Zhang <nic@opencloud.tech>
> ---

IMHO the message could be slightly improved by adding some of the 
replies that you made to your v3.
ie. Typical wrong numa node in VMs

$ cat /sys/devices/pci0000:00/0000:00:18.6/numa_node
-1

>   lib/librte_eal/linuxapp/eal/eal_pci.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
> index 595622b..c817b4c 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> @@ -310,18 +310,18 @@
>   			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 {
> +		RTE_LOG(WARNING, EAL,
> +			"numa_node is invalid or not present. "
> +			"Set it 0 as default\n");
> +		dev->device.numa_node = 0;
>   	}
>   
>   	rte_pci_device_name(addr, dev->name, sizeof(dev->name));

The code changes look fine, so I leave it to Thomas regarding the commit 
message :)

Acked-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>