Re: [PATCH v12 18/20] KVM: pfncache: check the need for invalidation under read lock first

[Sean Christopherson <seanjc@google.com>]

On Tue, Feb 06, 2024, David Woodhouse wrote:
> On Tue, 2024-02-06 at 20:47 -0800, Sean Christopherson wrote:
> > 
> > I'm saying this:
> > 
> >   When processing mmu_notifier invalidations for gpc caches, pre-check for
> >   overlap with the invalidation event while holding gpc->lock for read, and
> >   only take gpc->lock for write if the cache needs to be invalidated.  Doing
> >   a pre-check without taking gpc->lock for write avoids unnecessarily
> >   contending the lock for unrelated invalidations, which is very beneficial
> >   for caches that are heavily used (but rarely subjected to mmu_notifier
> >   invalidations).
> > 
> > is much friendlier to readers than this:
> > 
> >   Taking a write lock on a pfncache will be disruptive if the cache is
> >   heavily used (which only requires a read lock). Hence, in the MMU notifier
> >   callback, take read locks on caches to check for a match; only taking a
> >   write lock to actually perform an invalidation (after a another check).
> 
> That's a somewhat subjective observation. I actually find the latter to
> be far more succinct and obvious.
> 
> Actually... maybe I find yours harder because it isn't actually stating
> the situation as I understand it. You said "unrelated invalidation" in
> your first email, and "overlap with the invalidation event" in this
> one... neither of which makes sense to me because there is no *other*
> invalidation here.

I am referring to the "mmu_notifier invalidation event".  While a particular GPC
may not be affected by the invalidation, it's entirely possible that a different
GPC and/or some chunk of guest memory does need to be invalidated/zapped.

> We're only talking about the MMU notifier gratuitously taking the write

It's not "the MMU notifier" though, it's KVM that unnecessarily takes a lock.  I
know I'm being somewhat pedantic, but the distinction does matter.  E.g. with
guest_memfd, there will be invalidations that get routed through this code, but
that do not originate in the mmu_notifier.

And I think it's important to make it clear to readers that an mmu_notifier really
just is a notification from the primary MMU, albeit a notification that comes with
a rather strict contract.

> lock on a GPC that it *isn't* going to invalidate (the common case),
> and that disrupting users which are trying to take the read lock on
> that GPC.