Received-SPF: pass (google.com: domain of linux-kernel+bounces-56698-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 7 Feb 2024 07:10:28 -0800
In-Reply-To: <a817d64f3fe7b935a02e78df02dc0c6281e61af3.camel@infradead.org>
Precedence: bulk
Mime-Version: 1.0
References: <20240115125707.1183-1-paul@xen.org> <20240115125707.1183-19-paul@xen.org>
 <ZcMFb1epchA7Mbzo@google.com> <bbd59a2c0897d8ca642ea8c4787b829190e75a4d.camel@infradead.org>
 <ZcMLX5Omum3riZe8@google.com> <a817d64f3fe7b935a02e78df02dc0c6281e61af3.camel@infradead.org>
Message-ID: <ZcOdZKmmYz3kMgwp@google.com>
Subject: Re: [PATCH v12 18/20] KVM: pfncache: check the need for invalidation
 under read lock first
From: Sean Christopherson <seanjc@google.com>
To: David Woodhouse <dwmw2@infradead.org>
Cc: Paul Durrant <paul@xen.org>, Paolo Bonzini <pbonzini@redhat.com>, Jonathan Corbet <corbet@lwn.net>, 
	Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, 
	Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org, 
	"H. Peter Anvin" <hpa@zytor.com>, Shuah Khan <shuah@kernel.org>, kvm@vger.kernel.org, 
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, 
	linux-kselftest@vger.kernel.org
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Feb 06, 2024, David Woodhouse wrote:
> On Tue, 2024-02-06 at 20:47 -0800, Sean Christopherson wrote:
> >=20
> > I'm saying this:
> >=20
> > =C2=A0 When processing mmu_notifier invalidations for gpc caches, pre-c=
heck for
> > =C2=A0 overlap with the invalidation event while holding gpc->lock for =
read, and
> > =C2=A0 only take gpc->lock for write if the cache needs to be invalidat=
ed.=C2=A0 Doing
> > =C2=A0 a pre-check without taking gpc->lock for write avoids unnecessar=
ily
> > =C2=A0 contending the lock for unrelated invalidations, which is very b=
eneficial
> > =C2=A0 for caches that are heavily used (but rarely subjected to mmu_no=
tifier
> > =C2=A0 invalidations).
> >=20
> > is much friendlier to readers than this:
> >=20
> > =C2=A0 Taking a write lock on a pfncache will be disruptive if the cach=
e is
> > =C2=A0 heavily used (which only requires a read lock). Hence, in the MM=
U notifier
> > =C2=A0 callback, take read locks on caches to check for a match; only t=
aking a
> > =C2=A0 write lock to actually perform an invalidation (after a another =
check).
>=20
> That's a somewhat subjective observation. I actually find the latter to
> be far more succinct and obvious.
>=20
> 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 particula=
r GPC
may not be affected by the invalidation, it's entirely possible that a diff=
erent
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 loc=
k.  I
know I'm being somewhat pedantic, but the distinction does matter.  E.g. wi=
th
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 com=
es 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.