+James and Yan
On Tue, Mar 18, 2025, Maxim Levitsky wrote:
> Hi!
>
> I recently came up with an interesting failure in the CI pipeline.
>
>
> [ 592.704446] WARNING: possible circular locking dependency detected
> [ 592.710625] 6.12.0-36.el10.x86_64+debug #1 Not tainted
> [ 592.715764] ------------------------------------------------------
> [ 592.721946] swapper/19/0 is trying to acquire lock:
> [ 592.726823] ff110001b0e64ec0 (&p->pi_lock)\{-.-.}-\{2:2}, at: try_to_wake_up+0xa7/0x15c0
> [ 592.734761]
> [ 592.734761] but task is already holding lock:
> [ 592.740596] ff1100079ec0c058 (&per_cpu(wakeup_vcpus_on_cpu_lock, cpu))\{-...}-\{2:2}, at: pi_wakeup_handler+0x60/0x130 [kvm_intel]
> [ 592.752185]
> [ 592.752185] which lock already depends on the new lock.
...
> As far as I see, there is no race, but lockdep doesn't understand this.
Yep :-(
This splat fires every time (literally) I run through my battery of tests on
systems with IPI virtualization, it's basically an old friend at this point.
> It thinks that:
>
> 1. pi_enable_wakeup_handler is called from schedule() which holds rq->lock, and it itself takes wakeup_vcpus_on_cpu_lock lock
>
> 2. pi_wakeup_handler takes wakeup_vcpus_on_cpu_lock and then calls try_to_wake_up which can eventually take rq->lock
> (at the start of the function there is a list of cases when it takes it)
>
> I don't know lockdep well yet, but maybe a lockdep annotation will help,
> if we can tell it that there are multiple 'wakeup_vcpus_on_cpu_lock' locks.
Yan posted a patch to fudge around the issue[*], I strongly objected (and still
object) to making a functional and confusing code change to fudge around a lockdep
false positive.
James has also looked at the issue, and wasn't able to find a way to cleanly tell
lockdep about the situation.
Looking at this (yet) again, what if we temporarily tell lockdep that
wakeup_vcpus_on_cpu_lock isn't held when calling kvm_vcpu_wake_up()? Gross, but
more surgical than disabling lockdep entirely on the lock. This appears to squash
the warning without any unwanted side effects.
diff --git a/arch/x86/kvm/vmx/posted_intr.c b/arch/x86/kvm/vmx/posted_intr.c
index ec08fa3caf43..5984ad6f6f21 100644
--- a/arch/x86/kvm/vmx/posted_intr.c
+++ b/arch/x86/kvm/vmx/posted_intr.c
@@ -224,9 +224,17 @@ void pi_wakeup_handler(void)
raw_spin_lock(spinlock);
list_for_each_entry(vmx, wakeup_list, pi_wakeup_list) {
-
+ /*
+ * Temporarily lie to lockdep to avoid false positives due to
+ * lockdep not understanding that deadlock is impossible. This
+ * is called only in IRQ context, and the problematic locks
+ * taken in the kvm_vcpu_wake_up() call chain are only acquired
+ * with IRQs disabled.
+ */
+ spin_release(&spinlock->dep_map, _RET_IP_);
if (pi_test_on(&vmx->pi_desc))
kvm_vcpu_wake_up(&vmx->vcpu);
+ spin_acquire(&spinlock->dep_map, 0, 0, _RET_IP_);
}
raw_spin_unlock(spinlock);
}
[*] https://lore.kernel.org/all/20230313111022.13793-1-yan.y.zhao@xxxxxxxxx
