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

[Greg KH]

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@xxxxxxxxxx>
> >> ---
> >>  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?  You should also say something like "default node of 0 now
selected" or something like that, right?

thanks,

greg k-h