Re: [PATCH RFC] driver core: ensure a device has valid node id in device_add()

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Sep 06, 2019 at 02:41:36PM +0800, Yunsheng Lin wrote:
> On 2019/9/5 15:33, Greg KH wrote:
> > On Thu, Sep 05, 2019 at 02:48:24PM +0800, Yunsheng Lin wrote:
> >> On 2019/9/5 13:57, Greg KH wrote:
> >>> On Thu, Sep 05, 2019 at 09:33:50AM +0800, Yunsheng Lin wrote:
> >>>> Currently a device does not belong to any of the numa nodes
> >>>> (dev->numa_node is NUMA_NO_NODE) when the FW does not provide
> >>>> the node id and the device has not no parent device.
> >>>>
> >>>> According to discussion in [1]:
> >>>> Even if a device's numa node is not set by fw, the device
> >>>> really does belong to a node.
> >>>>
> >>>> This patch sets the device node to node 0 in device_add() if
> >>>> the fw has not specified the node id and it either has no
> >>>> parent device, or the parent device also does not have a valid
> >>>> node id.
> >>>>
> >>>> There may be explicit handling out there relying on NUMA_NO_NODE,
> >>>> like in nvme_probe().
> >>>>
> >>>> [1] https://lkml.org/lkml/2019/9/2/466
> >>>>
> >>>> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> >>>> ---
> >>>>  drivers/base/core.c  | 17 ++++++++++++++---
> >>>>  include/linux/numa.h |  2 ++
> >>>>  2 files changed, 16 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/base/core.c b/drivers/base/core.c
> >>>> index 1669d41..466b8ff 100644
> >>>> --- a/drivers/base/core.c
> >>>> +++ b/drivers/base/core.c
> >>>> @@ -2107,9 +2107,20 @@ int device_add(struct device *dev)
> >>>>  	if (kobj)
> >>>>  		dev->kobj.parent = kobj;
> >>>>  
> >>>> -	/* use parent numa_node */
> >>>> -	if (parent && (dev_to_node(dev) == NUMA_NO_NODE))
> >>>> -		set_dev_node(dev, dev_to_node(parent));
> >>>> +	/* use parent numa_node or default node 0 */
> >>>> +	if (!numa_node_valid(dev_to_node(dev))) {
> >>>> +		int nid = parent ? dev_to_node(parent) : NUMA_NO_NODE;
> >>>
> >>> Can you expand this to be a "real" if statement please?
> >>
> >> Sure. May I ask why "? :" is not appropriate here?
> > 
> > Because it is a pain to read, just spell it out and make it obvious what
> > is happening.  You write code for developers first, and the compiler
> > second, and in this case, either way is identical to the compiler.
> > 
> >>>> +
> >>>> +		if (numa_node_valid(nid)) {
> >>>> +			set_dev_node(dev, nid);
> >>>> +		} else {
> >>>> +			if (nr_node_ids > 1U)
> >>>> +				pr_err("device: '%s': has invalid NUMA node(%d)\n",
> >>>> +				       dev_name(dev), dev_to_node(dev));
> >>>
> >>> dev_err() will show you the exact device properly, instead of having to
> >>> rely on dev_name().
> >>>
> >>> And what is a user to do if this message happens?  How do they fix this?
> >>> If they can not, what good is this error message?
> >>
> >> If user know about their system's topology well enough and node 0
> >> is not the nearest node to the device, maybe user can readjust that by
> >> writing the nearest node to /sys/class/pci_bus/XXXX/device/numa_node,
> >> if not, then maybe user need to contact the vendor for info or updates.
> >>
> >> Maybe print error message as below:
> >>
> >> dev_err(dev, FW_BUG "has invalid NUMA node(%d). Readjust it by writing to sysfs numa_node or contact your vendor for updates.\n",
> >> 	dev_to_node(dev));
> > 
> > FW_BUG?
> > 
> > Anyway, if you make this change, how many machines start reporting this
> > error? 
> 
> Any machines with more than one numa node will start reporting this error.
> 
> 1) many virtual deivces maybe do not set the node id before calling
>    device_register(), such as vfio, tun, etc.
> 
> 2) struct cpu has a dev, but does not set the dev' node according to
>    cpu_to_node().
> 
> 3) Many platform Device also do not have a node id provided by FW.

Then this patch is not ok, as you are flooding the kernel log saying the
system is "broken" when this is just what it always has been like.  How
is anyone going to "fix" things?

You can adjust the default node to 0 as isn't that what always has
happened, but you can not claim it is a "error" that this is happening
because it is not an error, it's just the default operation.

thanks,

greg k-h