Received-SPF: pass (google.com: domain of linux-kernel+bounces-147695-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 16 Apr 2024 15:34:55 -0700
In-Reply-To: <CABgObfZ4kqaXLaOAOj4aGB5GAe9GxOmJmOP+7kdke6OqA35HzA@mail.gmail.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240215160136.1256084-1-alejandro.j.jimenez@oracle.com>
 <Zh6-h0lBCpYBahw7@google.com> <CABgObfZ4kqaXLaOAOj4aGB5GAe9GxOmJmOP+7kdke6OqA35HzA@mail.gmail.com>
Message-ID: <Zh79D2BdtS0jKO6W@google.com>
Subject: Re: [RFC 0/3] Export APICv-related state via binary stats interface
From: Sean Christopherson <seanjc@google.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>, kvm@vger.kernel.org, 
	linux-kernel@vger.kernel.org, joao.m.martins@oracle.com, 
	boris.ostrovsky@oracle.com, mark.kanda@oracle.com, 
	suravee.suthikulpanit@amd.com, mlevitsk@redhat.com
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Apr 16, 2024, Paolo Bonzini wrote:
> On Tue, Apr 16, 2024 at 8:08=E2=80=AFPM Sean Christopherson <seanjc@googl=
e.com> wrote:
> >
> > On Thu, Feb 15, 2024, Alejandro Jimenez wrote:
> > > The goal of this RFC is to agree on a mechanism for querying the stat=
e (and
> > > related stats) of APICv/AVIC. I clearly have an AVIC bias when approa=
ching this
> > > topic since that is the side that I have mostly looked at, and has th=
e greater
> > > number of possible inhibits, but I believe the argument applies for b=
oth
> > > vendor's technologies.
> > >
> > > Currently, a user or monitoring app trying to determine if APICv is a=
ctually
> > > being used needs implementation-specific knowlegde in order to look f=
or specific
> > > types of #VMEXIT (i.e. AVIC_INCOMPLETE_IPI/AVIC_NOACCEL), checking GA=
Log events
> > > by watching /proc/interrupts for AMD-Vi*-GA, etc. There are existing =
tracepoints
> > > (e.g. kvm_apicv_accept_irq, kvm_avic_ga_log) that make this task easi=
er, but
> > > tracefs is not viable in some scenarios. Adding kvm debugfs entries h=
as similar
> > > downsides. Suravee has previously proposed a new IOCTL interface[0] t=
o expose
> > > this information, but there has not been any development in that dire=
ction.
> > > Sean has mentioned a preference for using BPF to extract info from th=
e current
> > > tracepoints, which would require reworking existing structs to access=
 some
> > > desired data, but as far as I know there isn't any work done on that =
approach
> > > yet.
> > >
> > > Recently Joao mentioned another alternative: the binary stats framewo=
rk that is
> > > already supported by kernel[1] and QEMU[2].
> >
> > The hiccup with stats are that they are ABI, e.g. we can't (easily) dit=
ch stats
> > once they're added, and KVM needs to maintain the exact behavior.
>=20
> Stats are not ABI---why would they be?

Because they exposed through an ioctl(), and userspace can and will use sta=
ts for
functional purposes?  Maybe I just had the wrong takeaway from an old threa=
d about
adding a big pile of stats[1], where unfortunately (for me) you weighed in =
on
whether or not tracepoints are ABI, but not stats.

And because I've have been operating under the assumption that stats are AB=
I, I've
been guiding people to using stats to make decisions in userspace.  E.g. KV=
M doesn't
currently expose is_guest_mode() in kvm_run, but it is a stat, so it's not =
hard
to imagine userspace using the stat to make decisions without needing to ca=
ll back
into KVM[2].

And based on the old discussion[1] I doubt I'm though only one that has thi=
s view.

That said, I'm definitely not opposed to stats _not_ being ABI, because tha=
t would
give us a ton of flexibility.  E.g. we have a non-trivial number of interna=
l stats
that are super useful _for us_, but are rather heavy and might not be desir=
able
for most environments.  If stats aren't considered ABI, then I'm pretty sur=
e we
could land some of the more generally useful ones upstream, but off-by-defa=
ult
and guarded by a Kconfig.  E.g. we have a pile of stats related to mmu_lock=
 that
are very helpful in identifying performance issues, but they aren't things =
I would
want enabled by default.

But if we do decide stats aren't ABI, then we need to document and dissemin=
ate
that information.

[1] https://lore.kernel.org/all/CALzav=3DcuzT=3Du6G0TCVZUfEgAKOCKTSCDE8x2v5=
qc-Gd_NL-pzg@mail.gmail.com
[2] https://lore.kernel.org/all/Zh6-e9hy7U6DD2QM@google.com

> They have an established meaning and it's not a good idea to change it, b=
ut
> it's not an absolute no-no(*); and removing them is not a problem at all.
>=20
> For example, if stats were ABI, there would be no need for the
> introspection mechanism, you could just use a struct like ethtool
> stats (which *are* ABO).
>=20
> Not everything makes a good stat but, if in doubt and it's cheap
> enough to collect it, go ahead and add it.

Marc raised the (IMO) valid concern that "if it's cheap, add it" will lead =
to
death by a thousand cuts.  E.g. add a few hundred vCPU stats and suddenly v=
CPUs
consumes an extra KiB or three of memory.

A few APIC stats obviously aren't going to move the needle much, I'm just p=
ointing
out that not everyone agrees that KVM should be hyper permissive when it co=
mes to
adding new stats.

> Cheap collection is the important point, because tracepoints in a hot pat=
h
> can be so expensive as to slow down the guest substantially, at least in
> microbenchmarks.
>=20
> In this case I'm not sure _all_ inhibits makes sense and I certainly
> wouldn't want a bitmask, but a generic APICv-enabled stat certainly
> makes sense, and perhaps another for a weirdly-configured local APIC.
>=20
> Paolo
>=20
> (*) you have to draw a line somewhere. New processor models may
> virtualize parts of the CPU in such a way that some stats become
> meaningless or just stay at zero. Should KVM not support those
> features because it is not possible anymore to introspect the guest
> through stat?