* [PATCH] pci: Avoid unnecessary calls to work_on_cpu @ 2013-06-24 20:05 Alexander Duyck 2013-07-05 23:36 ` Bjorn Helgaas 0 siblings, 1 reply; 5+ messages in thread From: Alexander Duyck @ 2013-06-24 20:05 UTC (permalink / raw) To: bhelgaas; +Cc: linux-pci, linux-kernel This patch is meant to address the fact that we are making unnecessary calls to work_on_cpu. To resolve this I have added a check to see if the current node is the correct node for the device before we decide to assign the probe task to another CPU. The advantages to this approach is that we can avoid reentrant calls to work_on_cpu. In addition we should not make any calls to setup the work remotely in the case of a single node system that has NUMA enabled. Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com> --- This patch is based off of work I submitted in an earlier patch that I never heard back on. The change was originally submitted in: pci: Avoid reentrant calls to work_on_cpu I'm not sure what ever happened with that patch, however after reviewing it some myself I decided I could do without the change to the comments since they were unneeded. As such I am resubmitting this as a much simpler patch that only adds the line of code needed to avoid calling work_on_cpu for every call to probe on an NUMA node specific device. drivers/pci/pci-driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c index 79277fb..7d81713 100644 --- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c @@ -282,7 +282,7 @@ static int pci_call_probe(struct pci_driver *drv, struct pci_dev *dev, its local memory on the right node without any need to change it. */ node = dev_to_node(&dev->dev); - if (node >= 0) { + if ((node >= 0) && (node != numa_node_id())) { int cpu; get_online_cpus(); ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] pci: Avoid unnecessary calls to work_on_cpu 2013-06-24 20:05 [PATCH] pci: Avoid unnecessary calls to work_on_cpu Alexander Duyck @ 2013-07-05 23:36 ` Bjorn Helgaas 2013-07-06 0:29 ` Benjamin Herrenschmidt 2013-07-08 1:44 ` Rusty Russell 0 siblings, 2 replies; 5+ messages in thread From: Bjorn Helgaas @ 2013-07-05 23:36 UTC (permalink / raw) To: Alexander Duyck; +Cc: linux-pci, linux-kernel, Rusty Russell [+cc Rusty] On Mon, Jun 24, 2013 at 2:05 PM, Alexander Duyck <alexander.h.duyck@intel.com> wrote: > This patch is meant to address the fact that we are making unnecessary calls > to work_on_cpu. To resolve this I have added a check to see if the current > node is the correct node for the device before we decide to assign the probe > task to another CPU. > > The advantages to this approach is that we can avoid reentrant calls to > work_on_cpu. In addition we should not make any calls to setup the work > remotely in the case of a single node system that has NUMA enabled. The description above makes it sound like this is just a minor performance enhancement, but I think the real reason you want this is to resolve the lockdep warning mentioned at [1]. That thread is long and confusing, so I'd like to see a bugzilla that distills out the useful details, and a synopsis in this changelog. [1] https://lkml.kernel.org/r/1368498506-25857-7-git-send-email-yinghai@kernel.org > Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com> > --- > > This patch is based off of work I submitted in an earlier patch that I never > heard back on. The change was originally submitted in: > pci: Avoid reentrant calls to work_on_cpu > > I'm not sure what ever happened with that patch, however after reviewing it > some myself I decided I could do without the change to the comments since they > were unneeded. As such I am resubmitting this as a much simpler patch that > only adds the line of code needed to avoid calling work_on_cpu for every call > to probe on an NUMA node specific device. > > drivers/pci/pci-driver.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c > index 79277fb..7d81713 100644 > --- a/drivers/pci/pci-driver.c > +++ b/drivers/pci/pci-driver.c > @@ -282,7 +282,7 @@ static int pci_call_probe(struct pci_driver *drv, struct pci_dev *dev, > its local memory on the right node without any need to > change it. */ > node = dev_to_node(&dev->dev); > - if (node >= 0) { > + if ((node >= 0) && (node != numa_node_id())) { > int cpu; > > get_online_cpus(); I think it's theoretically unsafe to use numa_node_id() while preemption is enabled. It seems a little strange to me that this "run the driver probe method on the correct node" code is in PCI. I would think this behavior would be desirable for *all* bus types, not just PCI, so maybe it would make sense to do this up in device_attach() or somewhere similar. But Rusty added this (in 873392ca51), and he knows way more about this stuff than I do. Bjorn ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pci: Avoid unnecessary calls to work_on_cpu 2013-07-05 23:36 ` Bjorn Helgaas @ 2013-07-06 0:29 ` Benjamin Herrenschmidt 2013-07-08 1:44 ` Rusty Russell 1 sibling, 0 replies; 5+ messages in thread From: Benjamin Herrenschmidt @ 2013-07-06 0:29 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: Alexander Duyck, linux-pci, linux-kernel, Rusty Russell On Fri, 2013-07-05 at 17:36 -0600, Bjorn Helgaas wrote: > It seems a little strange to me that this "run the driver probe method > on the correct node" code is in PCI. I would think this behavior > would be desirable for *all* bus types, not just PCI, so maybe it > would make sense to do this up in device_attach() or somewhere > similar. > > But Rusty added this (in 873392ca51), and he knows way more about this > stuff than I do. I tend to agree... I can see this being useful on some of our non-PCI devices on power as well in fact. Cheers, Ben. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pci: Avoid unnecessary calls to work_on_cpu 2013-07-05 23:36 ` Bjorn Helgaas 2013-07-06 0:29 ` Benjamin Herrenschmidt @ 2013-07-08 1:44 ` Rusty Russell 2013-07-08 4:37 ` Andi Kleen 1 sibling, 1 reply; 5+ messages in thread From: Rusty Russell @ 2013-07-08 1:44 UTC (permalink / raw) To: Bjorn Helgaas, Alexander Duyck Cc: linux-pci, linux-kernel, Andi Kleen, Greg Kroah-Hartman Bjorn Helgaas <bhelgaas@google.com> writes: > [+cc Rusty] > > On Mon, Jun 24, 2013 at 2:05 PM, Alexander Duyck > <alexander.h.duyck@intel.com> wrote: >> This patch is meant to address the fact that we are making unnecessary calls >> to work_on_cpu. To resolve this I have added a check to see if the current >> node is the correct node for the device before we decide to assign the probe >> task to another CPU. >> >> The advantages to this approach is that we can avoid reentrant calls to >> work_on_cpu. In addition we should not make any calls to setup the work >> remotely in the case of a single node system that has NUMA enabled. > > The description above makes it sound like this is just a minor > performance enhancement, but I think the real reason you want this is > to resolve the lockdep warning mentioned at [1]. That thread is long > and confusing, so I'd like to see a bugzilla that distills out the > useful details, and a synopsis in this changelog. > > [1] https://lkml.kernel.org/r/1368498506-25857-7-git-send-email-yinghai@kernel.org > >> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com> >> --- >> >> This patch is based off of work I submitted in an earlier patch that I never >> heard back on. The change was originally submitted in: >> pci: Avoid reentrant calls to work_on_cpu >> >> I'm not sure what ever happened with that patch, however after reviewing it >> some myself I decided I could do without the change to the comments since they >> were unneeded. As such I am resubmitting this as a much simpler patch that >> only adds the line of code needed to avoid calling work_on_cpu for every call >> to probe on an NUMA node specific device. >> >> drivers/pci/pci-driver.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c >> index 79277fb..7d81713 100644 >> --- a/drivers/pci/pci-driver.c >> +++ b/drivers/pci/pci-driver.c >> @@ -282,7 +282,7 @@ static int pci_call_probe(struct pci_driver *drv, struct pci_dev *dev, >> its local memory on the right node without any need to >> change it. */ >> node = dev_to_node(&dev->dev); >> - if (node >= 0) { >> + if ((node >= 0) && (node != numa_node_id())) { >> int cpu; >> >> get_online_cpus(); > > I think it's theoretically unsafe to use numa_node_id() while > preemption is enabled. > > It seems a little strange to me that this "run the driver probe method > on the correct node" code is in PCI. I would think this behavior > would be desirable for *all* bus types, not just PCI, so maybe it > would make sense to do this up in device_attach() or somewhere > similar. > > But Rusty added this (in 873392ca51), and he knows way more about this > stuff than I do. Actually, I just stopped the code from playing cpumask games, which is what it used to do. You want Andi and/or Greg KH: commit d42c69972b853fd33a26c8c7405624be41a22136 Author: Andi Kleen <ak@suse.de> Date: Wed Jul 6 19:56:03 2005 +0200 [PATCH] PCI: Run PCI driver initialization on local node Run PCI driver initialization on local node Instead of adding messy kmalloc_node()s everywhere run the PCI driver probe on the node local to the device. This would not have helped for IDE, but should for other more clean drivers that do more initialization in probe(). It won't help for drivers that do most of the work on first open (like many network drivers) Signed-off-by: Andi Kleen <ak@suse.de> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de> Cheers, Rusty. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pci: Avoid unnecessary calls to work_on_cpu 2013-07-08 1:44 ` Rusty Russell @ 2013-07-08 4:37 ` Andi Kleen 0 siblings, 0 replies; 5+ messages in thread From: Andi Kleen @ 2013-07-08 4:37 UTC (permalink / raw) To: Rusty Russell Cc: Bjorn Helgaas, Alexander Duyck, linux-pci, linux-kernel, Andi Kleen, Greg Kroah-Hartman > > But Rusty added this (in 873392ca51), and he knows way more about this > > stuff than I do. > > Actually, I just stopped the code from playing cpumask games, which is > what it used to do. You're right the numa_node_id() check ptimization is not 100% safe on preempt kernels and should be probably removed. Also I agree it would probably make sense to move it up to the generic device layer (although I'm not sure that other bus types really care that much about NUMA locality) None of it seems to be a fatal problem though, so it boils down to "if someone cares enough to write a patch" -Andi ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-07-08 4:37 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-06-24 20:05 [PATCH] pci: Avoid unnecessary calls to work_on_cpu Alexander Duyck 2013-07-05 23:36 ` Bjorn Helgaas 2013-07-06 0:29 ` Benjamin Herrenschmidt 2013-07-08 1:44 ` Rusty Russell 2013-07-08 4:37 ` Andi Kleen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).