Re: [PATCH] pci: limit default numa node to used devices

[Thomas Monjalon <thomas@monjalon.net>]

21/07/2017 18:47, Sergio Gonzalez Monroy:
> On 21/07/2017 16:37, Thomas Monjalon wrote:
> > 21/07/2017 18:03, Sergio Gonzalez Monroy:
> >> On 21/07/2017 15:53, Thomas Monjalon wrote:
> >>> The title and the text below should explain that you move
> >>> the warning log from scan to probe, thanks to a temporary
> >>> negative value.
> >> I thought that saying that I only check for devices managed by dpdk
> >> explains the purpose,
> >> and the patch itself shows the change from one file to another.
> > It is obvious when you look carefully at the code, yes.
> > I was just giving my help to better explain :)
> 
> Just giving my view of the commit message
> If you think it can be improve,by all meansfeel free to change it :)
> 
> >>> 21/07/2017 12:11, Sergio Gonzalez Monroy:
> >>>> Commit 8a04cb612589 ("pci: set default numa node for broken systems")
> >>>> added logic to default to NUMA node 0 when sysfs numa_node information
> >>>> was wrong or not available.
> >>>>
> >>>> Unfortunately there are many devices with wrong NUMA node information
> >>>> that DPDK does not care about but still show warnings for them.
> >>>>
> >>>> Instead, only check for invalid NUMA node information for devices
> >>>> managed by the DPDK.
> >>>>
> >>>> Signed-off-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
> >>> [...]
> >>>> -	if (eal_parse_sysfs_value(filename, &tmp) == 0 &&
> >>>> -		tmp < RTE_MAX_NUMA_NODES)
> >>>> +	if (eal_parse_sysfs_value(filename, &tmp) == 0)
> >>>>    		dev->device.numa_node = tmp;
> >>> Why are you removing the check of the value?
> >>> Are you going to accept invalid high values?
> >>> This check was introduced on purpose by this commit:
> >>> 	http://dpdk.org/commit/8a04cb6125
> >> tmp is unsigned long type, so -1 is going to be a large number.
> > Oh yes, I missed it was unsigned!
> >
> >> My understanding was that it was basically checking for -1 as numa_node.
> >>
> >> If we have valid numa_node greater than RTE_MAX_NUMA_NODES, defaulting
> >> to 0 is not a good idea, is it?
> >>
> >> What I try to achieve with the patch is:
> >> - if no numa_node avilable then parse is going to fail and we set -1.
> >> - if numa_node is present but wrong, my understanding was that it would
> >> be -1.
> > All your explanations make sense when you realize that it is unsigned.
> >
> > I have one more question,
> > Does it work to check for a negative value like this?
> > 	if (dev->device.numa_node < 0)
> 
> numa_node is signed int type in struct rte_device, so it should work.

OK, sorry I overlooked :)