LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Greg KH <gregkh@linuxfoundation.org>
To: Yunsheng Lin <linyunsheng@huawei.com>
Cc: rafael@kernel.org, linux-kernel@vger.kernel.org,
	peterz@infradead.org, mingo@kernel.org, mhocko@kernel.org,
	linuxarm@huawei.com
Subject: Re: [PATCH RFC] driver core: ensure a device has valid node id in device_add()
Date: Thu, 5 Sep 2019 09:33:34 +0200
Message-ID: <20190905073334.GA29933@kroah.com> (raw)
In-Reply-To: <e5905af2-5a8d-7b00-d2a6-a961f3eee120@huawei.com>

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

thanks,

greg k-h

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-05  1:33 Yunsheng Lin
2019-09-05  5:57 ` Greg KH
2019-09-05  6:48   ` Yunsheng Lin
2019-09-05  7:33     ` Greg KH [this message]
2019-09-05  8:57       ` Yunsheng Lin
2019-09-05  9:02         ` Greg KH
2019-09-05  9:07           ` Yunsheng Lin
2019-09-06  6:41       ` Yunsheng Lin
2019-09-06  6:52         ` Greg KH
2019-09-06  8:21           ` Yunsheng Lin
2019-09-06 14:00             ` Greg KH
2019-09-07  2:10               ` Yunsheng Lin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190905073334.GA29933@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxarm@huawei.com \
    --cc=linyunsheng@huawei.com \
    --cc=mhocko@kernel.org \
    --cc=mingo@kernel.org \
    --cc=peterz@infradead.org \
    --cc=rafael@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git