Re: [PATCH v1] ACPI / scan: Acquire device_hotplug_lock in acpi_scan_init()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 25.07.19 15:57, Michal Hocko wrote:
> On Thu 25-07-19 15:05:02, David Hildenbrand wrote:
>> On 25.07.19 14:56, Michal Hocko wrote:
>>> On Wed 24-07-19 16:30:17, David Hildenbrand wrote:
>>>> We end up calling __add_memory() without the device hotplug lock held.
>>>> (I used a local patch to assert in __add_memory() that the
>>>>  device_hotplug_lock is held - I might upstream that as well soon)
>>>>
>>>> [   26.771684]        create_memory_block_devices+0xa4/0x140
>>>> [   26.772952]        add_memory_resource+0xde/0x200
>>>> [   26.773987]        __add_memory+0x6e/0xa0
>>>> [   26.775161]        acpi_memory_device_add+0x149/0x2b0
>>>> [   26.776263]        acpi_bus_attach+0xf1/0x1f0
>>>> [   26.777247]        acpi_bus_attach+0x66/0x1f0
>>>> [   26.778268]        acpi_bus_attach+0x66/0x1f0
>>>> [   26.779073]        acpi_bus_attach+0x66/0x1f0
>>>> [   26.780143]        acpi_bus_scan+0x3e/0x90
>>>> [   26.780844]        acpi_scan_init+0x109/0x257
>>>> [   26.781638]        acpi_init+0x2ab/0x30d
>>>> [   26.782248]        do_one_initcall+0x58/0x2cf
>>>> [   26.783181]        kernel_init_freeable+0x1bd/0x247
>>>> [   26.784345]        kernel_init+0x5/0xf1
>>>> [   26.785314]        ret_from_fork+0x3a/0x50
>>>>
>>>> So perform the locking just like in acpi_device_hotplug().
>>>
>>> While playing with the device_hotplug_lock, can we actually document
>>> what it is protecting please? I have a bad feeling that we are adding
>>> this lock just because some other code path does rather than with a good
>>> idea why it is needed. This patch just confirms that. What exactly does
>>> the lock protect from here in an early boot stage.
>>
>> We have plenty of documentation already
>>
>> mm/memory_hotplug.c
>>
>> git grep -C5 device_hotplug mm/memory_hotplug.c
>>
>> Also see
>>
>> Documentation/core-api/memory-hotplug.rst
> 
> OK, fair enough. I was more pointing to a documentation right there
> where the lock is declared because that is the place where people
> usually check for documentation. The core-api documentation looks quite
> nice. And based on that doc it seems that this patch is actually not
> needed because neither the online/offline or cpu hotplug should be
> possible that early unless I am missing something.

I really prefer to stick to locking rules as outlined on the
interfaces if it doesn't hurt. Why it is not needed is not clear.

> 
>> Regarding the early stage: primarily lockdep as I mentioned.
> 
> Could you add a lockdep splat that would be fixed by this patch to the
> changelog for reference?
> 

I have one where I enforce what's documented (but that's of course not
upstream and therefore not "real" yet)

commit 263da346cd3cf526de3f5138827fbc3520f2f8e0
Author: David Hildenbrand <david@xxxxxxxxxx>
Date:   Fri Jun 21 12:05:39 2019 +0200

    mm/memory_hotplug: Assert that the device_hotplug_lock is held
    
    We currently need the device_hotplug_lock(), as documented. Let's assert
    that the lock is held when adding/removing/onlining/offlining memory.
    
    Updated documentation to make this clearer.
    
    Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>


That patch in return was the result of debugging a lockdep warning
we can trigger right now (and I think it's a false positive
prevented by the device_hotplug_lock - I think it is the tie breaker).
Anyhow, this patch here didn't change it.


1. Start a guest with a DIMM attached
2. Online a memory block of that DIMM
3. Unplug the DIMM

:/# [   22.616108] Offlined Pages 32768
[   22.631567] 
[   22.632337] ======================================================
[   22.635104] WARNING: possible circular locking dependency detected
[   22.637475] 5.3.0-rc1-next-20190723+ #111 Not tainted
[   22.639314] ------------------------------------------------------
[   22.641276] kworker/u4:0/8 is trying to acquire lock:
[   22.642578] (____ptrval____) (kn->count#39){++++}, at: kernfs_remove_by_name_ns+0x3c/0x80
[   22.645004] 
[   22.645004] but task is already holding lock:
[   22.646495] (____ptrval____) (mem_sysfs_mutex){+.+.}, at: remove_memory_block_devices+0x65/0xd0
[   22.648649] 
[   22.648649] which lock already depends on the new lock.
[   22.648649] 
[   22.650488] 
[   22.650488] the existing dependency chain (in reverse order) is:
[   22.651987] 
[   22.651987] -> #4 (mem_sysfs_mutex){+.+.}:
[   22.653254]        __mutex_lock+0x8d/0x930
[   22.654079]        create_memory_block_devices+0xa4/0x140
[   22.655292]        add_memory_resource+0xd6/0x200
[   22.656252]        __add_memory+0x58/0x90
[   22.657096]        acpi_memory_device_add+0x149/0x2b0
[   22.658126]        acpi_bus_attach+0xf1/0x1f0
[   22.658899]        acpi_bus_attach+0x66/0x1f0
[   22.659698]        acpi_bus_attach+0x66/0x1f0
[   22.660482]        acpi_bus_attach+0x66/0x1f0
[   22.661265]        acpi_bus_scan+0x3e/0x90
[   22.662098]        acpi_scan_init+0x104/0x24d
[   22.662920]        acpi_init+0x2ab/0x30d
[   22.663733]        do_one_initcall+0x58/0x2cf
[   22.664727]        kernel_init_freeable+0x1b8/0x242
[   22.665780]        kernel_init+0x5/0xf1
[   22.666494]        ret_from_fork+0x3a/0x50
[   22.667271] 
[   22.667271] -> #3 (mem_hotplug_lock.rw_sem){++++}:
[   22.668378]        get_online_mems+0x39/0xc0
[   22.669327]        kmem_cache_create_usercopy+0x29/0x280
[   22.670369]        kmem_cache_create+0xd/0x10
[   22.671412]        ptlock_cache_init+0x1b/0x23
[   22.672206]        start_kernel+0x225/0x501
[   22.672979]        secondary_startup_64+0xa4/0xb0
[   22.673887] 
[   22.673887] -> #2 (cpu_hotplug_lock.rw_sem){++++}:
[   22.675091]        cpus_read_lock+0x39/0xc0
[   22.675962]        __offline_pages+0x3e/0x7c0
[   22.676997]        memory_subsys_offline+0x3a/0x60
[   22.678073]        device_offline+0x82/0xb0
[   22.679039]        acpi_bus_offline+0xdb/0x150
[   22.679912]        acpi_device_hotplug+0x1b4/0x3a0
[   22.680939]        acpi_hotplug_work_fn+0x15/0x20
[   22.682025]        process_one_work+0x26c/0x5a0
[   22.683019]        worker_thread+0x48/0x3e0
[   22.683942]        kthread+0x103/0x140
[   22.684855]        ret_from_fork+0x3a/0x50
[   22.685841] 
[   22.685841] -> #1 (&device->physical_node_lock){+.+.}:
[   22.687246]        __mutex_lock+0x8d/0x930
[   22.688179]        acpi_get_first_physical_node+0x18/0x60
[   22.689699]        acpi_companion_match+0x3b/0x60
[   22.690989]        acpi_device_uevent_modalias+0x9/0x20
[   22.692626]        platform_uevent+0xd/0x40
[   22.693832]        dev_uevent+0x86/0x1c0
[   22.695133]        uevent_show+0x93/0x100
[   22.695988]        dev_attr_show+0x14/0x40
[   22.697342]        sysfs_kf_seq_show+0xb2/0xf0
[   22.698845]        seq_read+0xd0/0x3f0
[   22.700066]        vfs_read+0xc0/0x170
[   22.701168]        ksys_read+0x63/0xe0
[   22.702392]        do_syscall_64+0x4b/0x1b0
[   22.703979]        entry_SYSCALL_64_after_hwframe+0x49/0xbe
[   22.705708] 
[   22.705708] -> #0 (kn->count#39){++++}:
[   22.707658]        __lock_acquire+0xe2f/0x1a20
[   22.708877]        lock_acquire+0x95/0x190
[   22.710299]        __kernfs_remove+0x253/0x2f0
[   22.711936]        kernfs_remove_by_name_ns+0x3c/0x80
[   22.713392]        device_del+0x148/0x360
[   22.714685]        device_unregister+0x9/0x20
[   22.716414]        remove_memory_block_devices+0x90/0xd0
[   22.718135]        try_remove_memory+0xc6/0x130
[   22.719669]        __remove_memory+0x5/0xc
[   22.721178]        acpi_memory_device_remove+0x72/0xf0
[   22.723178]        acpi_bus_trim+0x50/0x90
[   22.724537]        acpi_device_hotplug+0x222/0x3a0
[   22.726257]        acpi_hotplug_work_fn+0x15/0x20
[   22.728044]        process_one_work+0x26c/0x5a0
[   22.729825]        worker_thread+0x48/0x3e0
[   22.731128]        kthread+0x103/0x140
[   22.732137]        ret_from_fork+0x3a/0x50
[   22.733368] 
[   22.733368] other info that might help us debug this:
[   22.733368] 
[   22.736178] Chain exists of:
[   22.736178]   kn->count#39 --> mem_hotplug_lock.rw_sem --> mem_sysfs_mutex
[   22.736178] 
[   22.739723]  Possible unsafe locking scenario:
[   22.739723] 
[   22.741143]        CPU0                    CPU1
[   22.741788]        ----                    ----
[   22.742653]   lock(mem_sysfs_mutex);
[   22.743990]                                lock(mem_hotplug_lock.rw_sem);
[   22.746069]                                lock(mem_sysfs_mutex);
[   22.747207]   lock(kn->count#39);
[   22.748132] 
[   22.748132]  *** DEADLOCK ***
[   22.748132] 
[   22.749182] 7 locks held by kworker/u4:0/8:
[   22.750684]  #0: (____ptrval____) ((wq_completion)kacpi_hotplug){+.+.}, at: process_one_work+0x1e9/0x5a0
[   22.753966]  #1: (____ptrval____) ((work_completion)(&hpw->work)){+.+.}, at: process_one_work+0x1e9/0x5a0
[   22.756429]  #2: (____ptrval____) (device_hotplug_lock){+.+.}, at: acpi_device_hotplug+0x2d/0x3a0
[   22.758292]  #3: (____ptrval____) (acpi_scan_lock){+.+.}, at: acpi_device_hotplug+0x3b/0x3a0
[   22.759836]  #4: (____ptrval____) (cpu_hotplug_lock.rw_sem){++++}, at: try_remove_memory+0x3b/0x130
[   22.761463]  #5: (____ptrval____) (mem_hotplug_lock.rw_sem){++++}, at: percpu_down_write+0x1b/0xf2
[   22.763812]  #6: (____ptrval____) (mem_sysfs_mutex){+.+.}, at: remove_memory_block_devices+0x65/0xd0


-- 

Thanks,

David / dhildenb



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux