Re: [PATCH RFC 02/10] KVM: Allow reading memslots while holding slots_arch_lock

[Marco Elver]

On Thu, 10 Sept 2026 at 19:52, Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> On Thu, Sep 10, 2026, Marco Elver wrote:
> > On Thu, 10 Sept 2026 at 18:30, Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > >
> > > On Thu, Sep 10, 2026, Marco Elver wrote:
> > > > kvm_swap_active_memslots() updates kvm->memslots[as_id] while holding both
> > > > kvm->slots_lock and kvm->slots_arch_lock. Holding either lock guarantees
> > > > that memslots cannot be concurrently modified.
> > >
> > > Sure, but that's irrelevant.  The goal of the srcu_dereference_check() is to
> > > ensure that readers see a stable view of the VM's overall memory, not simply that
> > > kvm->memslots can't be written.
> >
> > Functionally, this is irrelevant for readers. But under lockdep it
> > isn't for writers: srcu_dereference_check() (with lockdep) asserts
> > that the srcu reader-lock is held, or the condition 'c' holds, which
> > here is holding any of the writer locks.
>
> No, the rules for writing kvm->memslots is that *both* are held.

Right. But the condition 'c' is only the srcu-less fallback for reads,
which is "any".

> > > > Allow reading memslots in __kvm_memslots() when kvm->slots_arch_lock is
> > > > held.
> > >
> > > Why?
> >
> > Holding any of the writer locks guarantees no concurrent modification;
> > therefore, if any writer lock is held, it's not required that the srcu
> > reader-lock is held. There are few places where only either slots_lock
> > or slots_arch_lock is held, which is sufficient for reading.
>
> Yes, but with caveats.  And more importantly, pure readers really shouldn't be
> taking slots_arch_lock, because either it's overkill and will generate unnecessary
> lock content, or the alleged reader is doing more than just reading.

Pure readers never should, and not what this patch suggested. But
writers (or those about to become writers if they only hold one lock)
can read w/o the srcu reader-lock held.

> Holding just slots_arch_lock *could* be fine, depending on the usage, but those
> details matter, which is why I asked "why".  I want to know exactly why we should
> relax the locking rules.
>
> Ah, poking around the code, I suspect that the motivation is
> kvm_enable_external_write_tracking()?  Which grabs __kvm_memslots() but only
> holds slots_arch_lock, i.e. would get a lockdep splat if someone with KVMGT ran
> with lockdep enabled.

Right.

> That thing isn't a pure reader.  It only reads the actual kvm->memslots pointer,
> but it writes to each of the slots metadata.
>
> So, allowing __kvm_memslots() to be called with just slot_arch_lock is ok in
> this situation, and if my guess is right, necessary to fix a false positive.  But
> I'm on the fence as to whether or not we generally want to allow that, versus
> taking kvm->srcu in kvm_enable_external_write_tracking() even though strictly
> speaking it's unnecessary.

Clarification here would be good. In building up the locking
annotations, this was one inconsistency I found vs. what
srcu_dereference_check() said, so the conclusion was that the fallback
condition 'c' was outdated (or there was a bug).