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

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 1591 bytes --]

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.

We're only talking about the MMU notifier gratuitously taking the write
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.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]