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

[Jiang Liu <liuj97@gmail.com>]

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.