| Message ID | 20120903170602.375b00cc.izumi.taku@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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
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
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
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
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
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; > > } > > >
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
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
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; }