On Tue, 2023-01-10 at 19:17 +0000, David Woodhouse wrote:
> On Tue, 2023-01-10 at 15:10 +0100, Paolo Bonzini wrote:
> > On 1/10/23 13:55, David Woodhouse wrote:
> > > > However, I
> > > > completely forgot the sev_lock_vcpus_for_migration case, which is the
> > > > exception that... well, disproves the rule.
> > > >
> > > But because it's an exception and rarely happens in practice, lockdep
> > > didn't notice and keep me honest sooner? Can we take them in that order
> > > just for fun at startup, to make sure lockdep knows?
> >
> > Sure, why not. Out of curiosity, is this kind of "priming" a thing
> > elsewhere in the kernel
>
> I did this:
>
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -461,6 +461,11 @@ void *kvm_mmu_memory_cache_alloc(struct kvm_mmu_memory_cache *mc)
> static void kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)
> {
> mutex_init(&vcpu->mutex);
> +
> + /* Ensure that lockdep knows vcpu->mutex is taken *inside* kvm->lock */
> + mutex_lock(&vcpu->mutex);
> + mutex_unlock(&vcpu->mutex);
> +
> vcpu->cpu = -1;
> vcpu->kvm = kvm;
> vcpu->vcpu_id = id;
>
>
> What I got when I ran xen_shinfo_test was... not what I expected:
>
>
> [13890.148203] ======================================================
> [13890.148205] WARNING: possible circular locking dependency detected
> [13890.148207] 6.1.0-rc4+ #1024 Tainted: G I E
> [13890.148209] ------------------------------------------------------
> [13890.148210] xen_shinfo_test/13326 is trying to acquire lock:
> [13890.148212] ffff888107d493b0 (&gpc->lock){....}-{2:2}, at: kvm_xen_update_runstate_guest+0xf2/0x4e0 [kvm]
> [13890.148285]
> but task is already holding lock:
> [13890.148287] ffff88887f671718 (&rq->__lock){-.-.}-{2:2}, at: __schedule+0x84/0x7c0
> [13890.148295]
> which lock already depends on the new lock.
>
...
> [13890.148997] Chain exists of:
> &gpc->lock --> &p->pi_lock --> &rq->__lock
I believe that's because of the priority inheritance check which
happens when there's *contention* for gpc->lock.
But in the majority of cases, anything taking a write lock on that (and
thus causing contention) is going to be invalidating the cache *anyway*
and causing us to abort. Or revalidating it, I suppose. But either way
we're racing with seeing an invalid state.
In which case we much as well just make it a trylock. Doing so *only*
for the scheduling-out atomic code path looks a bit like this... is
there a prettier way?
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 07e61cc9881e..c444948ab1ac 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -272,7 +272,12 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
* Attempt to obtain the GPC lock on *both* (if there are two)
* gfn_to_pfn caches that cover the region.
*/
- read_lock_irqsave(&gpc1->lock, flags);
+ local_irq_save(flags);
+ if (!read_trylock(&gpc1->lock)) {
+ if (atomic)
+ return;
+ read_lock(&gpc1->lock);
+ }
while (!kvm_gpc_check(gpc1, user_len1)) {
read_unlock_irqrestore(&gpc1->lock, flags);
@@ -283,7 +288,7 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
if (kvm_gpc_refresh(gpc1, user_len1))
return;
- read_lock_irqsave(&gpc1->lock, flags);
+ goto retry;
}
if (likely(!user_len2)) {
@@ -309,7 +314,13 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
* gpc1 lock to make lockdep shut up about it.
*/
lock_set_subclass(&gpc1->lock.dep_map, 1, _THIS_IP_);
- read_lock(&gpc2->lock);
+ if (!read_trylock(&gpc2->lock)) {
+ if (atomic) {
+ read_unlock_irqrestore(&gpc1->lock, flags);
+ return;
+ }
+ read_lock(&gpc2->lock);
+ }
if (!kvm_gpc_check(gpc2, user_len2)) {
read_unlock(&gpc2->lock);
--
2.35.3
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
