Patchwork [v2,4/6] ACPI, PCI: add acpi_pci_roots protection

login
register
mail settings
Submitter Taku Izumi
Date Sept. 3, 2012, 8:06 a.m.
Message ID <20120903170602.375b00cc.izumi.taku@jp.fujitsu.com>
Download mbox | patch
Permalink /patch/181310/
State Superseded
Headers show

Comments

Taku Izumi - Sept. 3, 2012, 8:06 a.m.
Use mutex and RCU to protect global acpi_pci_roots list against
PCI host bridge hotplug operations.

RCU is used to avoid possible deadlock in function acpi_pci_find_root()
and acpi_get_pci_rootbridge_handle(). A possible call graph:
acpi_pci_register_driver()
	mutex_lock(&acpi_pci_root_lock)
		driver->add(root)
			......
				acpi_pci_find_root()

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
---
 drivers/acpi/pci_root.c |   50 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 17 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas - Sept. 12, 2012, 11:40 p.m.
On Mon, Sep 3, 2012 at 2:06 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
> Use mutex and RCU to protect global acpi_pci_roots list against
> PCI host bridge hotplug operations.
>
> RCU is used to avoid possible deadlock in function acpi_pci_find_root()
> and acpi_get_pci_rootbridge_handle(). A possible call graph:
> acpi_pci_register_driver()
>         mutex_lock(&acpi_pci_root_lock)
>                 driver->add(root)
>                         ......
>                                 acpi_pci_find_root()

Where does this path occur?  I didn't see in in the current tree
(where the only users of acpi_pci_register_driver() are for
acpi_pci_slot_driver and acpi_pci_hp_driver).  Maybe it's in Yinghai's
work, which adds more acpi_pci_register_driver() users.

RCU seems unnecessarily complicated for this list, but I haven't gone
through Yinghai's work yet, so I don't know what it requires.

In acpi_pci_root_start() and acpi_pci_root_remove(), we have the
struct acpi_pci_root, which has all sorts of information that would be
useful to the .add() and .remove() methods of sub-drivers.  It seems
sort of stupid that we only pass the acpi_handle to the sub-drivers,
forcing them to use hacks like acpi_pci_find_root() to look up the
information we just threw away.  Can we just fix the .add() and
.remove() interfaces to pass something more useful so we avoid the
need for this deadlock path?

> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
> ---
>  drivers/acpi/pci_root.c |   50 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 33 insertions(+), 17 deletions(-)
>
> Index: Bjorn-next-0903/drivers/acpi/pci_root.c
> ===================================================================
> --- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
> +++ Bjorn-next-0903/drivers/acpi/pci_root.c
> @@ -28,6 +28,7 @@
>  #include <linux/init.h>
>  #include <linux/types.h>
>  #include <linux/mutex.h>
> +#include <linux/rculist.h>
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pci.h>
> @@ -86,7 +87,7 @@ int acpi_pci_register_driver(struct acpi
>         mutex_lock(&acpi_pci_root_lock);
>         list_add_tail(&driver->node, &acpi_pci_drivers);
>         if (driver->add)
> -               list_for_each_entry(root, &acpi_pci_roots, node) {
> +               list_for_each_entry_rcu(root, &acpi_pci_roots, node) {
>                         driver->add(root->device->handle);
>                         n++;
>                 }
> @@ -103,7 +104,7 @@ void acpi_pci_unregister_driver(struct a
>         mutex_lock(&acpi_pci_root_lock);
>         list_del(&driver->node);
>         if (driver->remove)
> -               list_for_each_entry(root, &acpi_pci_roots, node)
> +               list_for_each_entry_rcu(root, &acpi_pci_roots, node)
>                         driver->remove(root->device->handle);
>         mutex_unlock(&acpi_pci_root_lock);
>  }
> @@ -112,14 +113,19 @@ EXPORT_SYMBOL(acpi_pci_unregister_driver
>  acpi_handle acpi_get_pci_rootbridge_handle(unsigned int seg, unsigned int bus)
>  {
>         struct acpi_pci_root *root;
> +       struct acpi_handle *handle = NULL;
>
> -       list_for_each_entry(root, &acpi_pci_roots, node)
> +       rcu_read_lock();
> +       list_for_each_entry_rcu(root, &acpi_pci_roots, node)
>                 if ((root->segment == (u16) seg) &&
> -                   (root->secondary.start == (u16) bus))
> -                       return root->device->handle;
> -       return NULL;
> -}
> +                   (root->secondary.start == (u16) bus)) {
> +                       handle = root->device->handle;
> +                       break;
> +               }
> +       rcu_read_unlock();
>
> +       return handle;
> +}
>  EXPORT_SYMBOL_GPL(acpi_get_pci_rootbridge_handle);
>
>  /**
> @@ -266,10 +272,14 @@ struct acpi_pci_root *acpi_pci_find_root
>  {
>         struct acpi_pci_root *root;
>
> -       list_for_each_entry(root, &acpi_pci_roots, node) {
> -               if (root->device->handle == handle)
> +       rcu_read_lock();
> +       list_for_each_entry_rcu(root, &acpi_pci_roots, node)
> +               if (root->device->handle == handle) {
> +                       rcu_read_unlock();
>                         return root;
> -       }
> +               }
> +       rcu_read_unlock();
> +
>         return NULL;
>  }
>  EXPORT_SYMBOL_GPL(acpi_pci_find_root);
> @@ -506,8 +516,9 @@ static int __devinit acpi_pci_root_add(s
>          * TBD: Need PCI interface for enumeration/configuration of roots.
>          */
>
> -       /* TBD: Locking */
> -       list_add_tail(&root->node, &acpi_pci_roots);
> +       mutex_lock(&acpi_pci_root_lock);
> +       list_add_tail_rcu(&root->node, &acpi_pci_roots);
> +       mutex_unlock(&acpi_pci_root_lock);
>
>         printk(KERN_INFO PREFIX "%s [%s] (domain %04x %pR)\n",
>                acpi_device_name(device), acpi_device_bid(device),
> @@ -526,7 +537,7 @@ static int __devinit acpi_pci_root_add(s
>                             "Bus %04x:%02x not present in PCI namespace\n",
>                             root->segment, (unsigned int)root->secondary.start);
>                 result = -ENODEV;
> -               goto end;
> +               goto out_del_root;
>         }
>
>         /*
> @@ -536,7 +547,7 @@ static int __devinit acpi_pci_root_add(s
>          */
>         result = acpi_pci_bind_root(device);
>         if (result)
> -               goto end;
> +               goto out_del_root;
>
>         /*
>          * PCI Routing Table
> @@ -614,9 +625,12 @@ static int __devinit acpi_pci_root_add(s
>
>         return 0;
>
> +out_del_root:
> +       mutex_lock(&acpi_pci_root_lock);
> +       list_del_rcu(&root->node);
> +       mutex_unlock(&acpi_pci_root_lock);
> +       synchronize_rcu();
>  end:
> -       if (!list_empty(&root->node))
> -               list_del(&root->node);
>         kfree(root);
>         return result;
>  }
> @@ -646,11 +660,13 @@ static int acpi_pci_root_remove(struct a
>         list_for_each_entry(driver, &acpi_pci_drivers, node)
>                 if (driver->remove)
>                         driver->remove(root->device->handle);
> -       mutex_unlock(&acpi_pci_root_lock);
>
>         device_set_run_wake(root->bus->bridge, false);
>         pci_acpi_remove_bus_pm_notifier(device);
>
> +       list_del_rcu(&root->node);
> +       mutex_unlock(&acpi_pci_root_lock);
> +       synchronize_rcu();
>         kfree(root);
>         return 0;
>  }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu - Sept. 13, 2012, 7:09 p.m.
On Wed, Sep 12, 2012 at 4:40 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Mon, Sep 3, 2012 at 2:06 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
>> Use mutex and RCU to protect global acpi_pci_roots list against
>> PCI host bridge hotplug operations.
>>
>> RCU is used to avoid possible deadlock in function acpi_pci_find_root()
>> and acpi_get_pci_rootbridge_handle(). A possible call graph:
>> acpi_pci_register_driver()
>>         mutex_lock(&acpi_pci_root_lock)
>>                 driver->add(root)
>>                         ......
>>                                 acpi_pci_find_root()
>
> Where does this path occur?  I didn't see in in the current tree
> (where the only users of acpi_pci_register_driver() are for
> acpi_pci_slot_driver and acpi_pci_hp_driver).  Maybe it's in Yinghai's
> work, which adds more acpi_pci_register_driver() users.
>
> RCU seems unnecessarily complicated for this list, but I haven't gone
> through Yinghai's work yet, so I don't know what it requires.
>
> In acpi_pci_root_start() and acpi_pci_root_remove(), we have the
> struct acpi_pci_root, which has all sorts of information that would be
> useful to the .add() and .remove() methods of sub-drivers.  It seems
> sort of stupid that we only pass the acpi_handle to the sub-drivers,
> forcing them to use hacks like acpi_pci_find_root() to look up the
> information we just threw away.  Can we just fix the .add() and
> .remove() interfaces to pass something more useful so we avoid the
> need for this deadlock path?

new added acpi_pci_driver for ioapic, and iommu does not call
acpi_pci_find_root().

after split out pci_root_hp.c from acpiphp_glue.c, there will be
acpi_root_configure_bridge in pci_root_hp.c. that one could be
converted to
passing device instead of handle.

Thanks

Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas - Sept. 13, 2012, 7:39 p.m.
On Thu, Sep 13, 2012 at 1:09 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Sep 12, 2012 at 4:40 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Mon, Sep 3, 2012 at 2:06 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
>>> Use mutex and RCU to protect global acpi_pci_roots list against
>>> PCI host bridge hotplug operations.
>>>
>>> RCU is used to avoid possible deadlock in function acpi_pci_find_root()
>>> and acpi_get_pci_rootbridge_handle(). A possible call graph:
>>> acpi_pci_register_driver()
>>>         mutex_lock(&acpi_pci_root_lock)
>>>                 driver->add(root)
>>>                         ......
>>>                                 acpi_pci_find_root()
>>
>> Where does this path occur?  I didn't see in in the current tree
>> (where the only users of acpi_pci_register_driver() are for
>> acpi_pci_slot_driver and acpi_pci_hp_driver).  Maybe it's in Yinghai's
>> work, which adds more acpi_pci_register_driver() users.
>>
>> RCU seems unnecessarily complicated for this list, but I haven't gone
>> through Yinghai's work yet, so I don't know what it requires.
>>
>> In acpi_pci_root_start() and acpi_pci_root_remove(), we have the
>> struct acpi_pci_root, which has all sorts of information that would be
>> useful to the .add() and .remove() methods of sub-drivers.  It seems
>> sort of stupid that we only pass the acpi_handle to the sub-drivers,
>> forcing them to use hacks like acpi_pci_find_root() to look up the
>> information we just threw away.  Can we just fix the .add() and
>> .remove() interfaces to pass something more useful so we avoid the
>> need for this deadlock path?
>
> new added acpi_pci_driver for ioapic, and iommu does not call
> acpi_pci_find_root().
>
> after split out pci_root_hp.c from acpiphp_glue.c, there will be
> acpi_root_configure_bridge in pci_root_hp.c. that one could be
> converted to
> passing device instead of handle.

If I understand you correctly, you're agreeing that if we change the
.add()/.remove() interfaces, there is no need for RCU here.  Correct
me if I'm wrong.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu - Sept. 13, 2012, 9:17 p.m.
On Thu, Sep 13, 2012 at 12:39 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> If I understand you correctly, you're agreeing that if we change the
> .add()/.remove() interfaces, there is no need for RCU here.  Correct
> me if I'm wrong.

yes.

just passing acpi_pci_root point or acpi_device pointer.

Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu - Sept. 13, 2012, 10:44 p.m.
On Thu, Sep 13, 2012 at 2:17 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Sep 13, 2012 at 12:39 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> If I understand you correctly, you're agreeing that if we change the
>> .add()/.remove() interfaces, there is no need for RCU here.  Correct
>> me if I'm wrong.
>
> yes.
>
> just passing acpi_pci_root point or acpi_device pointer.

related change.
1. update pci_root_hp split patch to *p2p* for some function, so will
make add_bridge to take acpi_pci_root point.
2. remove one find_acpi_pci_root calling.
3. update the interface...

now in acpiphp_glue.c::register_slot still have one calling left....
        pdev = pbus->self;
        if (pdev && pci_is_pcie(pdev)) {
                tmp = acpi_find_root_bridge_handle(pdev);
                if (tmp) {
                        struct acpi_pci_root *root = acpi_pci_find_root(tmp);

                        if (root && (root->osc_control_set &
                                        OSC_PCI_EXPRESS_NATIVE_HP_CONTROL)) {
                                dev_printk(KERN_DEBUG, &pdev->dev, "is
already used by pciehp\n");
                                return AE_OK;
                        }
                }
        }

maybe could put acpi_pci_root *root in into every acpiphp_bridge. ?

Thanks

Yinghai
Taku Izumi - Sept. 14, 2012, 4:35 a.m.
On Wed, 12 Sep 2012 17:40:45 -0600
Bjorn Helgaas <bhelgaas@google.com> wrote:

> On Mon, Sep 3, 2012 at 2:06 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
> > Use mutex and RCU to protect global acpi_pci_roots list against
> > PCI host bridge hotplug operations.
> >
> > RCU is used to avoid possible deadlock in function acpi_pci_find_root()
> > and acpi_get_pci_rootbridge_handle(). A possible call graph:
> > acpi_pci_register_driver()
> >         mutex_lock(&acpi_pci_root_lock)
> >                 driver->add(root)
> >                         ......
> >                                 acpi_pci_find_root()
> 
> Where does this path occur?  I didn't see in in the current tree
> (where the only users of acpi_pci_register_driver() are for
> acpi_pci_slot_driver and acpi_pci_hp_driver).  Maybe it's in Yinghai's
> work, which adds more acpi_pci_register_driver() users.

  First I protected acpi_pci_roots list by using mutex(acpi_pci_root_lock).
  In that case I faced deadlock at the following path:
  acpiphp_glue_init
     + acpi_pci_register_driver
       ...
         + add_bridge
           + acpi_pci_find_root

  So I used RCU instead.

> RCU seems unnecessarily complicated for this list, but I haven't gone
> through Yinghai's work yet, so I don't know what it requires.
> 
> In acpi_pci_root_start() and acpi_pci_root_remove(), we have the
> struct acpi_pci_root, which has all sorts of information that would be
> useful to the .add() and .remove() methods of sub-drivers.  It seems
> sort of stupid that we only pass the acpi_handle to the sub-drivers,
> forcing them to use hacks like acpi_pci_find_root() to look up the
> information we just threw away.  Can we just fix the .add() and
> .remove() interfaces to pass something more useful so we avoid the
> need for this deadlock path?

  Maybe yes. Do you prefer imprementation without RCU ?

  Best regards,
  Taku Izumi
> 
> > Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> > Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
> > ---
> >  drivers/acpi/pci_root.c |   50 +++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 33 insertions(+), 17 deletions(-)
> >
> > Index: Bjorn-next-0903/drivers/acpi/pci_root.c
> > ===================================================================
> > --- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
> > +++ Bjorn-next-0903/drivers/acpi/pci_root.c
> > @@ -28,6 +28,7 @@
> >  #include <linux/init.h>
> >  #include <linux/types.h>
> >  #include <linux/mutex.h>
> > +#include <linux/rculist.h>
> >  #include <linux/pm.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/pci.h>
> > @@ -86,7 +87,7 @@ int acpi_pci_register_driver(struct acpi
> >         mutex_lock(&acpi_pci_root_lock);
> >         list_add_tail(&driver->node, &acpi_pci_drivers);
> >         if (driver->add)
> > -               list_for_each_entry(root, &acpi_pci_roots, node) {
> > +               list_for_each_entry_rcu(root, &acpi_pci_roots, node) {
> >                         driver->add(root->device->handle);
> >                         n++;
> >                 }
> > @@ -103,7 +104,7 @@ void acpi_pci_unregister_driver(struct a
> >         mutex_lock(&acpi_pci_root_lock);
> >         list_del(&driver->node);
> >         if (driver->remove)
> > -               list_for_each_entry(root, &acpi_pci_roots, node)
> > +               list_for_each_entry_rcu(root, &acpi_pci_roots, node)
> >                         driver->remove(root->device->handle);
> >         mutex_unlock(&acpi_pci_root_lock);
> >  }
> > @@ -112,14 +113,19 @@ EXPORT_SYMBOL(acpi_pci_unregister_driver
> >  acpi_handle acpi_get_pci_rootbridge_handle(unsigned int seg, unsigned int bus)
> >  {
> >         struct acpi_pci_root *root;
> > +       struct acpi_handle *handle = NULL;
> >
> > -       list_for_each_entry(root, &acpi_pci_roots, node)
> > +       rcu_read_lock();
> > +       list_for_each_entry_rcu(root, &acpi_pci_roots, node)
> >                 if ((root->segment == (u16) seg) &&
> > -                   (root->secondary.start == (u16) bus))
> > -                       return root->device->handle;
> > -       return NULL;
> > -}
> > +                   (root->secondary.start == (u16) bus)) {
> > +                       handle = root->device->handle;
> > +                       break;
> > +               }
> > +       rcu_read_unlock();
> >
> > +       return handle;
> > +}
> >  EXPORT_SYMBOL_GPL(acpi_get_pci_rootbridge_handle);
> >
> >  /**
> > @@ -266,10 +272,14 @@ struct acpi_pci_root *acpi_pci_find_root
> >  {
> >         struct acpi_pci_root *root;
> >
> > -       list_for_each_entry(root, &acpi_pci_roots, node) {
> > -               if (root->device->handle == handle)
> > +       rcu_read_lock();
> > +       list_for_each_entry_rcu(root, &acpi_pci_roots, node)
> > +               if (root->device->handle == handle) {
> > +                       rcu_read_unlock();
> >                         return root;
> > -       }
> > +               }
> > +       rcu_read_unlock();
> > +
> >         return NULL;
> >  }
> >  EXPORT_SYMBOL_GPL(acpi_pci_find_root);
> > @@ -506,8 +516,9 @@ static int __devinit acpi_pci_root_add(s
> >          * TBD: Need PCI interface for enumeration/configuration of roots.
> >          */
> >
> > -       /* TBD: Locking */
> > -       list_add_tail(&root->node, &acpi_pci_roots);
> > +       mutex_lock(&acpi_pci_root_lock);
> > +       list_add_tail_rcu(&root->node, &acpi_pci_roots);
> > +       mutex_unlock(&acpi_pci_root_lock);
> >
> >         printk(KERN_INFO PREFIX "%s [%s] (domain %04x %pR)\n",
> >                acpi_device_name(device), acpi_device_bid(device),
> > @@ -526,7 +537,7 @@ static int __devinit acpi_pci_root_add(s
> >                             "Bus %04x:%02x not present in PCI namespace\n",
> >                             root->segment, (unsigned int)root->secondary.start);
> >                 result = -ENODEV;
> > -               goto end;
> > +               goto out_del_root;
> >         }
> >
> >         /*
> > @@ -536,7 +547,7 @@ static int __devinit acpi_pci_root_add(s
> >          */
> >         result = acpi_pci_bind_root(device);
> >         if (result)
> > -               goto end;
> > +               goto out_del_root;
> >
> >         /*
> >          * PCI Routing Table
> > @@ -614,9 +625,12 @@ static int __devinit acpi_pci_root_add(s
> >
> >         return 0;
> >
> > +out_del_root:
> > +       mutex_lock(&acpi_pci_root_lock);
> > +       list_del_rcu(&root->node);
> > +       mutex_unlock(&acpi_pci_root_lock);
> > +       synchronize_rcu();
> >  end:
> > -       if (!list_empty(&root->node))
> > -               list_del(&root->node);
> >         kfree(root);
> >         return result;
> >  }
> > @@ -646,11 +660,13 @@ static int acpi_pci_root_remove(struct a
> >         list_for_each_entry(driver, &acpi_pci_drivers, node)
> >                 if (driver->remove)
> >                         driver->remove(root->device->handle);
> > -       mutex_unlock(&acpi_pci_root_lock);
> >
> >         device_set_run_wake(root->bus->bridge, false);
> >         pci_acpi_remove_bus_pm_notifier(device);
> >
> > +       list_del_rcu(&root->node);
> > +       mutex_unlock(&acpi_pci_root_lock);
> > +       synchronize_rcu();
> >         kfree(root);
> >         return 0;
> >  }
> >
>
Bjorn Helgaas - Sept. 14, 2012, 2:43 p.m.
On Thu, Sep 13, 2012 at 10:35 PM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
> On Wed, 12 Sep 2012 17:40:45 -0600
> Bjorn Helgaas <bhelgaas@google.com> wrote:
>
>> On Mon, Sep 3, 2012 at 2:06 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
>> > Use mutex and RCU to protect global acpi_pci_roots list against
>> > PCI host bridge hotplug operations.
>> >
>> > RCU is used to avoid possible deadlock in function acpi_pci_find_root()
>> > and acpi_get_pci_rootbridge_handle(). A possible call graph:
>> > acpi_pci_register_driver()
>> >         mutex_lock(&acpi_pci_root_lock)
>> >                 driver->add(root)
>> >                         ......
>> >                                 acpi_pci_find_root()
>>
>> Where does this path occur?  I didn't see in in the current tree
>> (where the only users of acpi_pci_register_driver() are for
>> acpi_pci_slot_driver and acpi_pci_hp_driver).  Maybe it's in Yinghai's
>> work, which adds more acpi_pci_register_driver() users.
>
>   First I protected acpi_pci_roots list by using mutex(acpi_pci_root_lock).
>   In that case I faced deadlock at the following path:
>   acpiphp_glue_init
>      + acpi_pci_register_driver
>        ...
>          + add_bridge
>            + acpi_pci_find_root
>
>   So I used RCU instead.

Oh, right.  I missed the acpiphp_glue_init() path.  That's clearly a problem.

>> RCU seems unnecessarily complicated for this list, but I haven't gone
>> through Yinghai's work yet, so I don't know what it requires.
>>
>> In acpi_pci_root_start() and acpi_pci_root_remove(), we have the
>> struct acpi_pci_root, which has all sorts of information that would be
>> useful to the .add() and .remove() methods of sub-drivers.  It seems
>> sort of stupid that we only pass the acpi_handle to the sub-drivers,
>> forcing them to use hacks like acpi_pci_find_root() to look up the
>> information we just threw away.  Can we just fix the .add() and
>> .remove() interfaces to pass something more useful so we avoid the
>> need for this deadlock path?
>
>   Maybe yes. Do you prefer imprementation without RCU ?

Yes, if it's possible, I prefer to avoid RCU in this case.  RCU is
appropriate for performance paths, but it's much more difficult to
analyze than mutex locking.

Host bridge hotplug is definitely not a path where performance is an
issue, and I think reworking the .add()/.remove() interfaces will
allow us to use mutex locking.

I think it will also simplify the sub-drivers because having the
struct acpi_pci_root means they can get rid of acpi_pci_find_root(),
they don't have to re-evaluate _SEG and _BBN (in acpi_pci_slot_add()
-> walk_root_bridge()), they don't have to use pci_find_bus(), etc.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiang Liu - Sept. 15, 2012, 3:23 a.m.
On 09/14/2012 10:43 PM, Bjorn Helgaas wrote:
> On Thu, Sep 13, 2012 at 10:35 PM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
>> On Wed, 12 Sep 2012 17:40:45 -0600
>> Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>>> On Mon, Sep 3, 2012 at 2:06 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
>>>> Use mutex and RCU to protect global acpi_pci_roots list against
>>>> PCI host bridge hotplug operations.
>>>>
>>>> RCU is used to avoid possible deadlock in function acpi_pci_find_root()
>>>> and acpi_get_pci_rootbridge_handle(). A possible call graph:
>>>> acpi_pci_register_driver()
>>>>         mutex_lock(&acpi_pci_root_lock)
>>>>                 driver->add(root)
>>>>                         ......
>>>>                                 acpi_pci_find_root()
>>>
>>> Where does this path occur?  I didn't see in in the current tree
>>> (where the only users of acpi_pci_register_driver() are for
>>> acpi_pci_slot_driver and acpi_pci_hp_driver).  Maybe it's in Yinghai's
>>> work, which adds more acpi_pci_register_driver() users.
>>
>>   First I protected acpi_pci_roots list by using mutex(acpi_pci_root_lock).
>>   In that case I faced deadlock at the following path:
>>   acpiphp_glue_init
>>      + acpi_pci_register_driver
>>        ...
>>          + add_bridge
>>            + acpi_pci_find_root
>>
>>   So I used RCU instead.
> 
> Oh, right.  I missed the acpiphp_glue_init() path.  That's clearly a problem.
It's amazing. When I was writing the code, I just realized there's a possible
deadlock scenario and then wrote defensive code. Not it's proven to be true:)

>>> RCU seems unnecessarily complicated for this list, but I haven't gone
>>> through Yinghai's work yet, so I don't know what it requires.
>>>
>>> In acpi_pci_root_start() and acpi_pci_root_remove(), we have the
>>> struct acpi_pci_root, which has all sorts of information that would be
>>> useful to the .add() and .remove() methods of sub-drivers.  It seems
>>> sort of stupid that we only pass the acpi_handle to the sub-drivers,
>>> forcing them to use hacks like acpi_pci_find_root() to look up the
>>> information we just threw away.  Can we just fix the .add() and
>>> .remove() interfaces to pass something more useful so we avoid the
>>> need for this deadlock path?
>>
>>   Maybe yes. Do you prefer imprementation without RCU ?
> 
> Yes, if it's possible, I prefer to avoid RCU in this case.  RCU is
> appropriate for performance paths, but it's much more difficult to
> analyze than mutex locking.
> 
> Host bridge hotplug is definitely not a path where performance is an
> issue, and I think reworking the .add()/.remove() interfaces will
> allow us to use mutex locking.
> 
> I think it will also simplify the sub-drivers because having the
> struct acpi_pci_root means they can get rid of acpi_pci_find_root(),
> they don't have to re-evaluate _SEG and _BBN (in acpi_pci_slot_add()
> -> walk_root_bridge()), they don't have to use pci_find_bus(), etc.
Yes, it would be better to get rid of the RCU staff.


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

Index: Bjorn-next-0903/drivers/acpi/pci_root.c
===================================================================
--- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
+++ Bjorn-next-0903/drivers/acpi/pci_root.c
@@ -28,6 +28,7 @@ 
 #include <linux/init.h>
 #include <linux/types.h>
 #include <linux/mutex.h>
+#include <linux/rculist.h>
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <linux/pci.h>
@@ -86,7 +87,7 @@  int acpi_pci_register_driver(struct acpi
 	mutex_lock(&acpi_pci_root_lock);
 	list_add_tail(&driver->node, &acpi_pci_drivers);
 	if (driver->add)
-		list_for_each_entry(root, &acpi_pci_roots, node) {
+		list_for_each_entry_rcu(root, &acpi_pci_roots, node) {
 			driver->add(root->device->handle);
 			n++;
 		}
@@ -103,7 +104,7 @@  void acpi_pci_unregister_driver(struct a
 	mutex_lock(&acpi_pci_root_lock);
 	list_del(&driver->node);
 	if (driver->remove)
-		list_for_each_entry(root, &acpi_pci_roots, node)
+		list_for_each_entry_rcu(root, &acpi_pci_roots, node)
 			driver->remove(root->device->handle);
 	mutex_unlock(&acpi_pci_root_lock);
 }
@@ -112,14 +113,19 @@  EXPORT_SYMBOL(acpi_pci_unregister_driver
 acpi_handle acpi_get_pci_rootbridge_handle(unsigned int seg, unsigned int bus)
 {
 	struct acpi_pci_root *root;
+	struct acpi_handle *handle = NULL;
 	
-	list_for_each_entry(root, &acpi_pci_roots, node)
+	rcu_read_lock();
+	list_for_each_entry_rcu(root, &acpi_pci_roots, node)
 		if ((root->segment == (u16) seg) &&
-		    (root->secondary.start == (u16) bus))
-			return root->device->handle;
-	return NULL;		
-}
+		    (root->secondary.start == (u16) bus)) {
+			handle = root->device->handle;
+			break;
+		}
+	rcu_read_unlock();
 
+	return handle;
+}
 EXPORT_SYMBOL_GPL(acpi_get_pci_rootbridge_handle);
 
 /**
@@ -266,10 +272,14 @@  struct acpi_pci_root *acpi_pci_find_root
 {
 	struct acpi_pci_root *root;
 
-	list_for_each_entry(root, &acpi_pci_roots, node) {
-		if (root->device->handle == handle)
+	rcu_read_lock();
+	list_for_each_entry_rcu(root, &acpi_pci_roots, node)
+		if (root->device->handle == handle) {
+			rcu_read_unlock();
 			return root;
-	}
+		}
+	rcu_read_unlock();
+
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(acpi_pci_find_root);
@@ -506,8 +516,9 @@  static int __devinit acpi_pci_root_add(s
 	 * TBD: Need PCI interface for enumeration/configuration of roots.
 	 */
 
-	/* TBD: Locking */
-	list_add_tail(&root->node, &acpi_pci_roots);
+	mutex_lock(&acpi_pci_root_lock);
+	list_add_tail_rcu(&root->node, &acpi_pci_roots);
+	mutex_unlock(&acpi_pci_root_lock);
 
 	printk(KERN_INFO PREFIX "%s [%s] (domain %04x %pR)\n",
 	       acpi_device_name(device), acpi_device_bid(device),
@@ -526,7 +537,7 @@  static int __devinit acpi_pci_root_add(s
 			    "Bus %04x:%02x not present in PCI namespace\n",
 			    root->segment, (unsigned int)root->secondary.start);
 		result = -ENODEV;
-		goto end;
+		goto out_del_root;
 	}
 
 	/*
@@ -536,7 +547,7 @@  static int __devinit acpi_pci_root_add(s
 	 */
 	result = acpi_pci_bind_root(device);
 	if (result)
-		goto end;
+		goto out_del_root;
 
 	/*
 	 * PCI Routing Table
@@ -614,9 +625,12 @@  static int __devinit acpi_pci_root_add(s
 
 	return 0;
 
+out_del_root:
+	mutex_lock(&acpi_pci_root_lock);
+	list_del_rcu(&root->node);
+	mutex_unlock(&acpi_pci_root_lock);
+	synchronize_rcu();
 end:
-	if (!list_empty(&root->node))
-		list_del(&root->node);
 	kfree(root);
 	return result;
 }
@@ -646,11 +660,13 @@  static int acpi_pci_root_remove(struct a
 	list_for_each_entry(driver, &acpi_pci_drivers, node)
 		if (driver->remove)
 			driver->remove(root->device->handle);
-	mutex_unlock(&acpi_pci_root_lock);
 
 	device_set_run_wake(root->bus->bridge, false);
 	pci_acpi_remove_bus_pm_notifier(device);
 
+	list_del_rcu(&root->node);
+	mutex_unlock(&acpi_pci_root_lock);
+	synchronize_rcu();
 	kfree(root);
 	return 0;
 }