[RFC/RFT,v2,3/3] PCI/ACPI: Add ACPI pci_bus_find_numa_node() implementation

On Tue, May 16, 2017 at 07:02:00PM +0100, Lorenzo Pieralisi wrote:
> On Tue, May 16, 2017 at 05:15:29PM +0200, Robert Richter wrote:
> > On 15.05.17 14:22:05, Lorenzo Pieralisi wrote:
> > > The introduction of pci_bus_find_numa_node(pci_bus) allows at PCI
> > > host bridge registration to detect the NUMA node for a given
> > > struct pci_bus.dev. Implement an ACPI method that, through
> > > the struct pci_bus.bridge ACPI companion, retrieve and return
> > > the NUMA node corresponding to a given struct pci_bus.dev.
> > > 
> > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > ---
> > >  drivers/pci/pci-acpi.c | 20 ++++++++++++++++++++
> > >  drivers/pci/pci.c      |  2 +-
> > >  include/linux/pci.h    |  6 ++++++
> > >  3 files changed, 27 insertions(+), 1 deletion(-)
> > > 
> > 
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index e9803c1..451342d 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -5406,7 +5406,7 @@ int pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent)
> > >  
> > >  int pci_bus_find_numa_node(struct pci_bus *bus)
> > >  {
> > > -	return NUMA_NO_NODE;
> > > +	return acpi_disabled ? NUMA_NO_NODE : acpi_pci_bus_find_numa_node(bus);
> > 
> > I looked into how this works with devicetree.
> > 
> > With ACPI it is set directly in pci_register_host_bridge() with
> > set_dev_node().
> > 
> > For the DT case the set_dev_node call sets it to NUMA_NO_NODE first.
> > Since in DT the bridge is a platform device which has the node id
> > assigned already (if there is one), the bus' node id is set later in
> > device_add() when deriving it from the parent device which is the
> > bridge. So this should work.
> 
> Which also means that the node propagation for bus->dev in patch 2 can
> be probably removed :), the problem with ACPI is setting the node
> for the host bridge which in DT is done by default at platform device
> creation, the rest is done by the core already there is not any need
> to propagate it again when child busses are created (they take their
> node from the parent).

More explicitly, I think the whole series should work also with the diff
below applied on top of it. Side note: for consistency, I do not think
that adding a DT counterpart to pci_bus_find_numa_node() would hurt.

Thanks !
Lorenzo

-- >8 --

On 17.05.17 14:46:54, Lorenzo Pieralisi wrote:

> More explicitly, I think the whole series should work also with the diff
> below applied on top of it. Side note: for consistency, I do not think
> that adding a DT counterpart to pci_bus_find_numa_node() would hurt.
> 
> Thanks !
> Lorenzo
> 
> -- >8 --
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 76c089f..cf0692c 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -862,7 +862,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>  	 */
>  	child->dev.class = &pcibus_class;
>  	dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
> -	set_dev_node(&child->dev, dev_to_node(&parent->dev));
> +

Hmm, in device_add() there is already:

	/* use parent numa_node */
	if (parent && (dev_to_node(dev) == NUMA_NO_NODE))
        	set_dev_node(dev, dev_to_node(parent));

So there are cases where the device has a different node than the
parent. I am not sure if we can assume for pci that it maps always.

And since device_add() is called later anyway, the above change might
not necessary at all. But at least we must assign the node id to the
bridge, which is the parent. Maybe just have in
acpi_pci_root_create():

	bridge = get_device(bus->bridge);
	adev = to_acpi_device_node(bridge->fwnode);
	set_dev_node(bridge, acpi_get_node(adev->handle));

-Robert


>  	/*
>  	 * Set up the primary, secondary and subordinate
>  	 * bus numbers.

On Wed, May 17, 2017 at 04:35:58PM +0200, Robert Richter wrote:
> On 17.05.17 14:46:54, Lorenzo Pieralisi wrote:
> 
> > More explicitly, I think the whole series should work also with the diff
> > below applied on top of it. Side note: for consistency, I do not think
> > that adding a DT counterpart to pci_bus_find_numa_node() would hurt.
> > 
> > Thanks !
> > Lorenzo
> > 
> > -- >8 --
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 76c089f..cf0692c 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -862,7 +862,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
> >  	 */
> >  	child->dev.class = &pcibus_class;
> >  	dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
> > -	set_dev_node(&child->dev, dev_to_node(&parent->dev));
> > +
> 
> Hmm, in device_add() there is already:
> 
> 	/* use parent numa_node */
> 	if (parent && (dev_to_node(dev) == NUMA_NO_NODE))
>         	set_dev_node(dev, dev_to_node(parent));
> 
> So there are cases where the device has a different node than the
> parent. I am not sure if we can assume for pci that it maps always.
> 
> And since device_add() is called later anyway, the above change might
> not necessary at all.

That's why I _removed_ the set_dev_node() in the diff above (that applies
to patch (2)), I do not think it is a) correct and b) necessary to
propagate the NUMA node from bus to a child bus given that device_add()
takes care of that already.

I should post a v3 (with the diff above applied) so that we are all on
the same page and we can test it.

> But at least we must assign the node id to the
> bridge, which is the parent. Maybe just have in
> acpi_pci_root_create():
> 
> 	bridge = get_device(bus->bridge);
> 	adev = to_acpi_device_node(bridge->fwnode);
> 	set_dev_node(bridge, acpi_get_node(adev->handle));

I do not think that's enough, I need to check again but I think that
also the bus->dev should have its NUMA node set for things to work (and
allow the NUMA node to propagate correctly through device_add())
otherwise pcibus_to_node() would fail for devices sitting on the root
bus, right ?

I will check again and post v3 shortly.

Thanks !
Lorenzo

> 
> -Robert
> 
> 
> >  	/*
> >  	 * Set up the primary, secondary and subordinate
> >  	 * bus numbers.

On 17.05.17 17:04:24, Lorenzo Pieralisi wrote:
> On Wed, May 17, 2017 at 04:35:58PM +0200, Robert Richter wrote:
> > On 17.05.17 14:46:54, Lorenzo Pieralisi wrote:
> > 
> > > More explicitly, I think the whole series should work also with the diff
> > > below applied on top of it. Side note: for consistency, I do not think
> > > that adding a DT counterpart to pci_bus_find_numa_node() would hurt.
> > > 
> > > Thanks !
> > > Lorenzo
> > > 
> > > -- >8 --
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index 76c089f..cf0692c 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -862,7 +862,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
> > >  	 */
> > >  	child->dev.class = &pcibus_class;
> > >  	dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
> > > -	set_dev_node(&child->dev, dev_to_node(&parent->dev));
> > > +
> > 
> > Hmm, in device_add() there is already:
> > 
> > 	/* use parent numa_node */
> > 	if (parent && (dev_to_node(dev) == NUMA_NO_NODE))
> >         	set_dev_node(dev, dev_to_node(parent));
> > 
> > So there are cases where the device has a different node than the
> > parent. I am not sure if we can assume for pci that it maps always.
> > 
> > And since device_add() is called later anyway, the above change might
> > not necessary at all.
> 
> That's why I _removed_ the set_dev_node() in the diff above (that applies
> to patch (2)), I do not think it is a) correct and b) necessary to
> propagate the NUMA node from bus to a child bus given that device_add()
> takes care of that already.

Ah, right, you removed it instead.

> 
> I should post a v3 (with the diff above applied) so that we are all on
> the same page and we can test it.
> 
> > But at least we must assign the node id to the
> > bridge, which is the parent. Maybe just have in
> > acpi_pci_root_create():
> > 
> > 	bridge = get_device(bus->bridge);
> > 	adev = to_acpi_device_node(bridge->fwnode);
> > 	set_dev_node(bridge, acpi_get_node(adev->handle));
> 
> I do not think that's enough, I need to check again but I think that
> also the bus->dev should have its NUMA node set for things to work (and
> allow the NUMA node to propagate correctly through device_add())
> otherwise pcibus_to_node() would fail for devices sitting on the root
> bus, right ?

Yeah, maybe another hop is in between and the node id for bus->dev is
used. Which need to be set then.

> 
> I will check again and post v3 shortly.

Thanks,

-Robert