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

[Bjorn Helgaas <bhelgaas@google.com>]

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.