Re: [RFC/RFT PATCH 3/6] arm64, numa: Move pcibus_to_node definition to generic numa code

Re: [RFC/RFT PATCH 3/6] arm64, numa: Move pcibus_to_node definition to generic numa code

[Jonathan Cameron]

On Fri, 14 Aug 2020 14:47:22 -0700
Atish Patra <atish.patra@wdc.com> wrote:

> pcibus_to_node is used only when numa is enabled and does not depend
> on ISA. Thus, it can be moved the generic numa implementation.
> 
> Signed-off-by: Atish Patra <atish.patra@wdc.com>

From a more general unification point of view, there seem to
be two ways architectures implement this.
Either

bus->sysdata.node

Or as here.
There are weird other options, but let us ignore those :)

That is going to take a bit of unwinding should we
want to take this unification further and perhaps we want to think
about doing this in pci generic code rather than here?

Perhaps this is one we are better keeping architecture specific for
now?

+CC Bjorn and Linux-pci


> ---
>  arch/arm64/kernel/pci.c  | 10 ----------
>  drivers/base/arch_numa.c | 11 +++++++++++
>  2 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 1006ed2d7c60..07c122946c11 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -54,16 +54,6 @@ int raw_pci_write(unsigned int domain, unsigned int bus,
>  	return b->ops->write(b, devfn, reg, len, val);
>  }
>  
> -#ifdef CONFIG_NUMA
> -
> -int pcibus_to_node(struct pci_bus *bus)
> -{
> -	return dev_to_node(&bus->dev);
> -}
> -EXPORT_SYMBOL(pcibus_to_node);
> -
> -#endif
> -
>  #ifdef CONFIG_ACPI
>  
>  struct acpi_pci_generic_root_info {
> diff --git a/drivers/base/arch_numa.c b/drivers/base/arch_numa.c
> index 83341c807240..4ab1b20a615d 100644
> --- a/drivers/base/arch_numa.c
> +++ b/drivers/base/arch_numa.c
> @@ -11,6 +11,7 @@
>  #include <linux/acpi.h>
>  #include <linux/memblock.h>
>  #include <linux/module.h>
> +#include <linux/pci.h>
>  #include <linux/of.h>
>  
>  #ifdef CONFIG_ARM64
> @@ -60,6 +61,16 @@ EXPORT_SYMBOL(cpumask_of_node);
>  
>  #endif
>  
> +#ifdef CONFIG_PCI
> +
> +int pcibus_to_node(struct pci_bus *bus)
> +{
> +	return dev_to_node(&bus->dev);
> +}
> +EXPORT_SYMBOL(pcibus_to_node);
> +
> +#endif
> +
>  static void numa_update_cpu(unsigned int cpu, bool remove)
>  {
>  	int nid = cpu_to_node(cpu);

Re: [RFC/RFT PATCH 3/6] arm64, numa: Move pcibus_to_node definition to generic numa code

[Bjorn Helgaas]

On Fri, Aug 28, 2020 at 10:48:30AM +0100, Jonathan Cameron wrote:
> On Fri, 14 Aug 2020 14:47:22 -0700
> Atish Patra <atish.patra@wdc.com> wrote:
> 
> > pcibus_to_node is used only when numa is enabled and does not depend
> > on ISA. Thus, it can be moved the generic numa implementation.
> > 
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> 
> From a more general unification point of view, there seem to
> be two ways architectures implement this.
> Either
> 
> bus->sysdata.node
> 
> Or as here.
> There are weird other options, but let us ignore those :)
> 
> That is going to take a bit of unwinding should we
> want to take this unification further and perhaps we want to think
> about doing this in pci generic code rather than here?
> 
> Perhaps this is one we are better keeping architecture specific for
> now?
> 
> +CC Bjorn and Linux-pci
> 
> 
> > ---
> >  arch/arm64/kernel/pci.c  | 10 ----------
> >  drivers/base/arch_numa.c | 11 +++++++++++
> >  2 files changed, 11 insertions(+), 10 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > index 1006ed2d7c60..07c122946c11 100644
> > --- a/arch/arm64/kernel/pci.c
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -54,16 +54,6 @@ int raw_pci_write(unsigned int domain, unsigned int bus,
> >  	return b->ops->write(b, devfn, reg, len, val);
> >  }
> >  
> > -#ifdef CONFIG_NUMA
> > -
> > -int pcibus_to_node(struct pci_bus *bus)
> > -{
> > -	return dev_to_node(&bus->dev);
> > -}
> > -EXPORT_SYMBOL(pcibus_to_node);
> > -
> > -#endif
> > -
> >  #ifdef CONFIG_ACPI
> >  
> >  struct acpi_pci_generic_root_info {
> > diff --git a/drivers/base/arch_numa.c b/drivers/base/arch_numa.c
> > index 83341c807240..4ab1b20a615d 100644
> > --- a/drivers/base/arch_numa.c
> > +++ b/drivers/base/arch_numa.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/acpi.h>
> >  #include <linux/memblock.h>
> >  #include <linux/module.h>
> > +#include <linux/pci.h>
> >  #include <linux/of.h>
> >  
> >  #ifdef CONFIG_ARM64
> > @@ -60,6 +61,16 @@ EXPORT_SYMBOL(cpumask_of_node);
> >  
> >  #endif
> >  
> > +#ifdef CONFIG_PCI
> > +
> > +int pcibus_to_node(struct pci_bus *bus)
> > +{
> > +	return dev_to_node(&bus->dev);
> > +}
> > +EXPORT_SYMBOL(pcibus_to_node);
> > +
> > +#endif

I certainly agree that this should not be arch-specific, but I'm not
really in favor of adding this PCI gunk in drivers/base.

I think we can do better (eventually) by getting rid of
pcibus_to_node() completely.  It's not used very much except by
cpumask_of_pcibus(), which itself is hardly used at all.

> >  static void numa_update_cpu(unsigned int cpu, bool remove)
> >  {
> >  	int nid = cpu_to_node(cpu);
> 
> 

Re: [RFC/RFT PATCH 3/6] arm64, numa: Move pcibus_to_node definition to generic numa code

[Atish Patra]

On Fri, Aug 28, 2020 at 9:15 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Aug 28, 2020 at 10:48:30AM +0100, Jonathan Cameron wrote:
> > On Fri, 14 Aug 2020 14:47:22 -0700
> > Atish Patra <atish.patra@wdc.com> wrote:
> >
> > > pcibus_to_node is used only when numa is enabled and does not depend
> > > on ISA. Thus, it can be moved the generic numa implementation.
> > >
> > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> >
> > From a more general unification point of view, there seem to
> > be two ways architectures implement this.
> > Either
> >
> > bus->sysdata.node
> >
> > Or as here.
> > There are weird other options, but let us ignore those :)
> >
> > That is going to take a bit of unwinding should we
> > want to take this unification further and perhaps we want to think
> > about doing this in pci generic code rather than here?
> >
> > Perhaps this is one we are better keeping architecture specific for
> > now?
> >
> > +CC Bjorn and Linux-pci
> >
> >
> > > ---
> > >  arch/arm64/kernel/pci.c  | 10 ----------
> > >  drivers/base/arch_numa.c | 11 +++++++++++
> > >  2 files changed, 11 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > > index 1006ed2d7c60..07c122946c11 100644
> > > --- a/arch/arm64/kernel/pci.c
> > > +++ b/arch/arm64/kernel/pci.c
> > > @@ -54,16 +54,6 @@ int raw_pci_write(unsigned int domain, unsigned int bus,
> > >     return b->ops->write(b, devfn, reg, len, val);
> > >  }
> > >
> > > -#ifdef CONFIG_NUMA
> > > -
> > > -int pcibus_to_node(struct pci_bus *bus)
> > > -{
> > > -   return dev_to_node(&bus->dev);
> > > -}
> > > -EXPORT_SYMBOL(pcibus_to_node);
> > > -
> > > -#endif
> > > -
> > >  #ifdef CONFIG_ACPI
> > >
> > >  struct acpi_pci_generic_root_info {
> > > diff --git a/drivers/base/arch_numa.c b/drivers/base/arch_numa.c
> > > index 83341c807240..4ab1b20a615d 100644
> > > --- a/drivers/base/arch_numa.c
> > > +++ b/drivers/base/arch_numa.c
> > > @@ -11,6 +11,7 @@
> > >  #include <linux/acpi.h>
> > >  #include <linux/memblock.h>
> > >  #include <linux/module.h>
> > > +#include <linux/pci.h>
> > >  #include <linux/of.h>
> > >
> > >  #ifdef CONFIG_ARM64
> > > @@ -60,6 +61,16 @@ EXPORT_SYMBOL(cpumask_of_node);
> > >
> > >  #endif
> > >
> > > +#ifdef CONFIG_PCI
> > > +
> > > +int pcibus_to_node(struct pci_bus *bus)
> > > +{
> > > +   return dev_to_node(&bus->dev);
> > > +}
> > > +EXPORT_SYMBOL(pcibus_to_node);
> > > +
> > > +#endif
>
> I certainly agree that this should not be arch-specific, but I'm not
> really in favor of adding this PCI gunk in drivers/base.
>
> I think we can do better (eventually) by getting rid of
> pcibus_to_node() completely.  It's not used very much except by
> cpumask_of_pcibus(), which itself is hardly used at all.
>
I am a bit confused here. A quick grep suggested that pcibus_to_node()
is also called from generic pci probe,
controller and few drivers(block, infiniband) as well. Maybe I am
missing something here ?

We can move the pcibus_to_node to arch specific code for now if that's
what is preferred.

> > >  static void numa_update_cpu(unsigned int cpu, bool remove)
> > >  {
> > >     int nid = cpu_to_node(cpu);
> >
> >
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv



--
Regards,
Atish

Re: [RFC/RFT PATCH 3/6] arm64, numa: Move pcibus_to_node definition to generic numa code

[Bjorn Helgaas]

On Fri, Aug 28, 2020 at 06:11:50PM -0700, Atish Patra wrote:
> On Fri, Aug 28, 2020 at 9:15 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, Aug 28, 2020 at 10:48:30AM +0100, Jonathan Cameron wrote:
> > > On Fri, 14 Aug 2020 14:47:22 -0700
> > > Atish Patra <atish.patra@wdc.com> wrote:
> > >
> > > > pcibus_to_node is used only when numa is enabled and does not depend
> > > > on ISA. Thus, it can be moved the generic numa implementation.
> > > >
> > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > >
> > > From a more general unification point of view, there seem to
> > > be two ways architectures implement this.
> > > Either
> > >
> > > bus->sysdata.node
> > >
> > > Or as here.
> > > There are weird other options, but let us ignore those :)
> > >
> > > That is going to take a bit of unwinding should we
> > > want to take this unification further and perhaps we want to think
> > > about doing this in pci generic code rather than here?
> > >
> > > Perhaps this is one we are better keeping architecture specific for
> > > now?
> > >
> > > +CC Bjorn and Linux-pci
> > >
> > >
> > > > ---
> > > >  arch/arm64/kernel/pci.c  | 10 ----------
> > > >  drivers/base/arch_numa.c | 11 +++++++++++
> > > >  2 files changed, 11 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > > > index 1006ed2d7c60..07c122946c11 100644
> > > > --- a/arch/arm64/kernel/pci.c
> > > > +++ b/arch/arm64/kernel/pci.c
> > > > @@ -54,16 +54,6 @@ int raw_pci_write(unsigned int domain, unsigned int bus,
> > > >     return b->ops->write(b, devfn, reg, len, val);
> > > >  }
> > > >
> > > > -#ifdef CONFIG_NUMA
> > > > -
> > > > -int pcibus_to_node(struct pci_bus *bus)
> > > > -{
> > > > -   return dev_to_node(&bus->dev);
> > > > -}
> > > > -EXPORT_SYMBOL(pcibus_to_node);
> > > > -
> > > > -#endif
> > > > -
> > > >  #ifdef CONFIG_ACPI
> > > >
> > > >  struct acpi_pci_generic_root_info {
> > > > diff --git a/drivers/base/arch_numa.c b/drivers/base/arch_numa.c
> > > > index 83341c807240..4ab1b20a615d 100644
> > > > --- a/drivers/base/arch_numa.c
> > > > +++ b/drivers/base/arch_numa.c
> > > > @@ -11,6 +11,7 @@
> > > >  #include <linux/acpi.h>
> > > >  #include <linux/memblock.h>
> > > >  #include <linux/module.h>
> > > > +#include <linux/pci.h>
> > > >  #include <linux/of.h>
> > > >
> > > >  #ifdef CONFIG_ARM64
> > > > @@ -60,6 +61,16 @@ EXPORT_SYMBOL(cpumask_of_node);
> > > >
> > > >  #endif
> > > >
> > > > +#ifdef CONFIG_PCI
> > > > +
> > > > +int pcibus_to_node(struct pci_bus *bus)
> > > > +{
> > > > +   return dev_to_node(&bus->dev);
> > > > +}
> > > > +EXPORT_SYMBOL(pcibus_to_node);
> > > > +
> > > > +#endif
> >
> > I certainly agree that this should not be arch-specific, but I'm not
> > really in favor of adding this PCI gunk in drivers/base.
> >
> > I think we can do better (eventually) by getting rid of
> > pcibus_to_node() completely.  It's not used very much except by
> > cpumask_of_pcibus(), which itself is hardly used at all.
> >
> I am a bit confused here. A quick grep suggested that pcibus_to_node()
> is also called from generic pci probe,
> controller and few drivers(block, infiniband) as well. Maybe I am
> missing something here ?

I didn't say it was *only* used by cpumask_of_pcibus().  13 of the 29
calls are from cpumask_of_pcibus().

As you point out, there are a few drivers that use it.  They typically
have a pci_dev, so they do the equivalent of pcibus_to_node(pdev->bus).
That seems silly; they should just do dev_to_node(&pdev->dev) instead.

I looked at this once, and it seems like there might have been a
wrinkle like the pdev->dev node not being set correctly or something.
If that's the case, I think it should be fixed.

> We can move the pcibus_to_node to arch specific code for now if that's
> what is preferred.

Now I'm the one who's confused :)  Most arches, including arm64,
already have arch-specific implementations of pcibus_to_node().  I
didn't look at the rest of the series to see if there's a reason you
need to move pcibus_to_node() from arch/arm64/kernel/pci.c to
drivers//base/arch_numa.c.  If you don't need to, I would just leave
it where it is.

> > > >  static void numa_update_cpu(unsigned int cpu, bool remove)
> > > >  {
> > > >     int nid = cpu_to_node(cpu);

Re: [RFC/RFT PATCH 3/6] arm64, numa: Move pcibus_to_node definition to generic numa code

[Atish Patra]

On Sat, Aug 29, 2020 at 7:54 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Aug 28, 2020 at 06:11:50PM -0700, Atish Patra wrote:
> > On Fri, Aug 28, 2020 at 9:15 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Fri, Aug 28, 2020 at 10:48:30AM +0100, Jonathan Cameron wrote:
> > > > On Fri, 14 Aug 2020 14:47:22 -0700
> > > > Atish Patra <atish.patra@wdc.com> wrote:
> > > >
> > > > > pcibus_to_node is used only when numa is enabled and does not depend
> > > > > on ISA. Thus, it can be moved the generic numa implementation.
> > > > >
> > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > >
> > > > From a more general unification point of view, there seem to
> > > > be two ways architectures implement this.
> > > > Either
> > > >
> > > > bus->sysdata.node
> > > >
> > > > Or as here.
> > > > There are weird other options, but let us ignore those :)
> > > >
> > > > That is going to take a bit of unwinding should we
> > > > want to take this unification further and perhaps we want to think
> > > > about doing this in pci generic code rather than here?
> > > >
> > > > Perhaps this is one we are better keeping architecture specific for
> > > > now?
> > > >
> > > > +CC Bjorn and Linux-pci
> > > >
> > > >
> > > > > ---
> > > > >  arch/arm64/kernel/pci.c  | 10 ----------
> > > > >  drivers/base/arch_numa.c | 11 +++++++++++
> > > > >  2 files changed, 11 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > > > > index 1006ed2d7c60..07c122946c11 100644
> > > > > --- a/arch/arm64/kernel/pci.c
> > > > > +++ b/arch/arm64/kernel/pci.c
> > > > > @@ -54,16 +54,6 @@ int raw_pci_write(unsigned int domain, unsigned int bus,
> > > > >     return b->ops->write(b, devfn, reg, len, val);
> > > > >  }
> > > > >
> > > > > -#ifdef CONFIG_NUMA
> > > > > -
> > > > > -int pcibus_to_node(struct pci_bus *bus)
> > > > > -{
> > > > > -   return dev_to_node(&bus->dev);
> > > > > -}
> > > > > -EXPORT_SYMBOL(pcibus_to_node);
> > > > > -
> > > > > -#endif
> > > > > -
> > > > >  #ifdef CONFIG_ACPI
> > > > >
> > > > >  struct acpi_pci_generic_root_info {
> > > > > diff --git a/drivers/base/arch_numa.c b/drivers/base/arch_numa.c
> > > > > index 83341c807240..4ab1b20a615d 100644
> > > > > --- a/drivers/base/arch_numa.c
> > > > > +++ b/drivers/base/arch_numa.c
> > > > > @@ -11,6 +11,7 @@
> > > > >  #include <linux/acpi.h>
> > > > >  #include <linux/memblock.h>
> > > > >  #include <linux/module.h>
> > > > > +#include <linux/pci.h>
> > > > >  #include <linux/of.h>
> > > > >
> > > > >  #ifdef CONFIG_ARM64
> > > > > @@ -60,6 +61,16 @@ EXPORT_SYMBOL(cpumask_of_node);
> > > > >
> > > > >  #endif
> > > > >
> > > > > +#ifdef CONFIG_PCI
> > > > > +
> > > > > +int pcibus_to_node(struct pci_bus *bus)
> > > > > +{
> > > > > +   return dev_to_node(&bus->dev);
> > > > > +}
> > > > > +EXPORT_SYMBOL(pcibus_to_node);
> > > > > +
> > > > > +#endif
> > >
> > > I certainly agree that this should not be arch-specific, but I'm not
> > > really in favor of adding this PCI gunk in drivers/base.
> > >
> > > I think we can do better (eventually) by getting rid of
> > > pcibus_to_node() completely.  It's not used very much except by
> > > cpumask_of_pcibus(), which itself is hardly used at all.
> > >
> > I am a bit confused here. A quick grep suggested that pcibus_to_node()
> > is also called from generic pci probe,
> > controller and few drivers(block, infiniband) as well. Maybe I am
> > missing something here ?
>
> I didn't say it was *only* used by cpumask_of_pcibus().  13 of the 29
> calls are from cpumask_of_pcibus().
>
Ahh okay. Sorry I misunderstood that.

> As you point out, there are a few drivers that use it.  They typically
> have a pci_dev, so they do the equivalent of pcibus_to_node(pdev->bus).
> That seems silly; they should just do dev_to_node(&pdev->dev) instead.
>

That covers the use case for ARM64. There are other arch implementations
as well which retrieve node information from sysdata which seems to be
type casted to different structures on different arch.
What should be done for those ?

> I looked at this once, and it seems like there might have been a
> wrinkle like the pdev->dev node not being set correctly or something.
> If that's the case, I think it should be fixed.
>
> > We can move the pcibus_to_node to arch specific code for now if that's
> > what is preferred.
>
> Now I'm the one who's confused :)  Most arches, including arm64,
> already have arch-specific implementations of pcibus_to_node().  I
> didn't look at the rest of the series to see if there's a reason you
> need to move pcibus_to_node() from arch/arm64/kernel/pci.c to
> drivers//base/arch_numa.c.  If you don't need to, I would just leave
> it where it is.
>

The reason I moved it from arch/arm64/kernel/pci.c to drivers//base/arch_numa.c.
so that we don't have to define it for RISC-V. But it's just a single
line function and
we can define it in RISC-V as well. I will try that in v2.

> > > > >  static void numa_update_cpu(unsigned int cpu, bool remove)
> > > > >  {
> > > > >     int nid = cpu_to_node(cpu);



-- 
Regards,
Atish