On 2024/12/2 11:25, fengnan chang wrote:
On Fri, Nov 29, 2024 at 4:00 PM Yi Liu <yi.l.liu@xxxxxxxxx> wrote:
On 2024/11/28 11:50, fengnan chang wrote:
2024年11月27日 14:56，fengnan chang <fengnanchang@xxxxxxxxx> 写道：
Dear PCI maintainers:
I'm having a deadlock issue, somewhat similar to a previous one https://lore.kernel.org/linux-pci/CS1PR8401MB0728FC6FDAB8A35C22BD90EC95F10@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/#t，; but my kernel (6.6.40) already included the fix f5eff55.
The previous bug was solved by the below commit.
commit f5eff5591b8f9c5effd25c92c758a127765f74c1
Author: Lukas Wunner <lukas@xxxxxxxxx>
Date: Tue Apr 11 08:21:02 2023 +0200
PCI: pciehp: Fix AB-BA deadlock between reset_lock and device_lock
Yes, my kernel version included this fix.
In 2013, commits
2e35afaefe64 ("PCI: pciehp: Add reset_slot() method")
608c388122c7 ("PCI: Add slot reset option to pci_dev_reset()")
amended PCIe hotplug to mask Presence Detect Changed events during a
Secondary Bus Reset. The reset thus no longer causes gratuitous slot
bringdown and bringup.
However the commits neglected to serialize reset with code paths reading
slot registers. For instance, a slot bringup due to an earlier hotplug
event may see the Presence Detect State bit cleared during a concurrent
Secondary Bus Reset.
In 2018, commit
5b3f7b7d062b ("PCI: pciehp: Avoid slot access during reset")
retrofitted the missing locking. It introduced a reset_lock which
serializes a Secondary Bus Reset with other parts of pciehp.
Unfortunately the locking turns out to be overzealous: reset_lock is
held for the entire enumeration and de-enumeration of hotplugged devices,
including driver binding and unbinding.
Driver binding and unbinding acquires device_lock while the reset_lock
of the ancestral hotplug port is held. A concurrent Secondary Bus Reset
acquires the ancestral reset_lock while already holding the device_lock.
The asymmetric locking order in the two code paths can lead to AB-BA
deadlocks.
Michael Haeuptle reports such deadlocks on simultaneous hot-removal and
vfio release (the latter implies a Secondary Bus Reset):
pciehp_ist() # down_read(reset_lock)
pciehp_handle_presence_or_link_change()
pciehp_disable_slot()
__pciehp_disable_slot()
remove_board()
pciehp_unconfigure_device()
pci_stop_and_remove_bus_device()
pci_stop_bus_device()
pci_stop_dev()
device_release_driver()
device_release_driver_internal()
__device_driver_lock() # device_lock()
SYS_munmap()
vfio_device_fops_release()
vfio_device_group_close()
vfio_device_close()
vfio_device_last_close()
Here is my test process, I’m running kernel with 6.6.40 and SPDK v22.05:
1. SPDK use vfio driver to takeover two nvme disks, running some io in nvme.
2. pull out two nvme disks
3. Try to kill -9 SPDK process.
Then deadlock issue happened. For now I can 100% reproduce this problem. I’m not an export in PCI, but I did a brief analysis:
irq 149 thread take pci_rescan_remove_lock mutex lock, and wait for SPDK to release vfio.
irq 148 thread take reset_lock of ctrl A, and wait for psi_rescan_remove_lock
SPDK process try to release vfio driver, but wait for reset_lock of ctrl A.
irq/149-pciehp stack, cat /proc/514/stack,
[<0>] pciehp_unconfigure_device+0x48/0x160 // wait for pci_rescan_remove_lock
[<0>] pciehp_disable_slot+0x6b/0x130 // hold reset_lock of ctrl A
[<0>] pciehp_handle_presence_or_link_change+0x7d/0x4d0
[<0>] pciehp_ist+0x236/0x260
[<0>] irq_thread_fn+0x1b/0x60
[<0>] irq_thread+0xed/0x190
[<0>] kthread+0xe4/0x110
[<0>] ret_from_fork+0x2d/0x50
[<0>] ret_from_fork_asm+0x11/0x20
irq/148-pciehp stack, cat /proc/513/stack
[<0>] vfio_unregister_group_dev+0x97/0xe0 [vfio] //wait for
My mistake, this is wait for SPDK to release vfio device. This problem can reproduce in 6.12.
Besides, My college give me an idea, we can make vfio_device_fops_release be async, so when we close fd, it
won’t block, and when we close another fd, it will release vfio device, this stack will not block too, then the deadlock disappears.
In the hotplug path, vfio needs to notify userspace to stop the usage of
this device and release reference on the vfio_device. When the last
refcount is released, the wait in the vfio_unregister_group_dev() will be
unblocked. It is the vfio_device_fops_release() either userspace exits or
userspace explicitly close the vfio device fd. Your below test patch moves
the majority of the vfio_device_fops_release() out of the existing path.
I don't see a reason why it can work so far.
Forgive my poor English. Let me explain in more detail.
irq 149 thread take pci_rescan_remove_lock mutex lock, take reset_lock
of ctrl B and release reset_lock of ctrl B. Then wait for SPDK to
release vfio refcount connected to ctrl B.
irq 148 thread take reset_lock of ctrl A, and wait for psi_rescan_remove_lock.
it's 148 which holds pci_rescan_remove_lock and wait for the last refcount
of vfio_device.
SPDK process exit, and try to release vfio refcount connected to ctrl
A, but need to wait for reset_lock of ctrl A. Since SPDK is blocked
here, it won’t release vfio refcount connected to ctrl B. So the
deadlock is happening.
but I got you now. So the SPDK process exit thread closes the vfio_device
A and B sequentially. The vfio_device A file release is blocked by the 149
while 149 is blocked by the pci_rescan_remove_lock which is held by 148.
148 is waiting for the vfio_device B's refcount which is done in
vfio_device B's file release. The vfio_device B file release is not done
because of the exit thread is dealing with vfio_device A. So this should be
related to timing. Did you see any difference w.r.t. the order of plugging
out the devices? Maybe you can try something different to double confirm
it. e.g. plugging out the devices as the order of how they are listed in
the QEMU cmdline.
After my patch. When SPDK exit it’s async to release vfio refcount
connected to ctrl A, it still will block, but SPDK can still can
release vfio refcount connected to ctrl B. Since irq 149 release
reset_lock of ctrl B, it won’t Block. After release vfio refcount
connected to ctrl B, irq 149 can finish it’s work, and will release
pci_rescan_remove_lock mutex lock, and then irq 148 can take
pci_rescan_remove_lock, then all is same as ctrl A.
As the locking issue has been solved in the above commit, seems there is
no deadlock with the reset_lock and device_lock. Can you confirm if the
scenario can be reproduced with one device? Also, even with two devices,
does killing the process matters or not?
This problem is not a deadlock with the reset_lock and device_lock.
The scenario can’t be reproduced with one
device, only two devices can reproduce this problem. Kill the process
may not be important, but
the interrupt thread is blocked too, and all lspci commands will
block, this affects some monitoring
programs.
yes, this issue is not related to no response from userspace.
--
Regards,
Yi Liu
