On Thu, Sep 13, 2012 at 10:35 PM, Taku Izumi <izumi.taku@xxxxxxxxxxxxxx> wrote: > On Wed, 12 Sep 2012 17:40:45 -0600 > Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote: > >> On Mon, Sep 3, 2012 at 2:06 AM, Taku Izumi <izumi.taku@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
