Re: [PATCH 3/3] KVM: x86: Decouple APICv activation state from apicv_inhibit_reasons

[Sean Christopherson <seanjc@google.com>]

On Mon, Feb 03, 2025, Paolo Bonzini wrote:
> On 2/3/25 19:45, Sean Christopherson wrote:
> > Unless there's a very, very good reason to support a use case that generates
> > ExtInts during boot, but _only_ during boot, and otherwise doesn't have any APICv
> > ihibits, I'm leaning towards making SVM's IRQ window inhibit sticky, i.e. never
> > clear it.
> 
> BIOS tends to use PIT, so that may be too much.  With respect to Naveen's report
> of contention on apicv_update_lock, I would go with the sticky-bit idea but apply
> it to APICV_INHIBIT_REASON_PIT_REINJ.

That won't work, at least not with yet more changes, because KVM creates the
in-kernel PIT with reinjection enabled by default.  The stick-bit idea is that
if a bit is set and can never be cleared, then there's no need to track new
updates.  Since userspace needs to explicitly disable reinjection, the inhibit
can't be sticky.

I assume We could fudge around that easily enough by deferring the inhibit until
a vCPU is created (or run?), but piggybacking PIT_REINJ won't help the userspace
I/O APIC case.

> I don't love adding another inhibit reason but, together, these two should
> remove the contention on apicv_update_lock.  Another idea could be to move
> IRQWIN to per-vCPU reason but Maxim tells me that it's not so easy.

Oh, yeah, that reminds me of the other reason I would vote for a sticky flag:
if inhibition really is toggling rapidly, performance is going to be quite bad
because inhibiting APICv requires (a) zapping APIC SPTEs and (b) serializing
writers if multiple vCPUs trigger the 0=>1 transition.

And there's some amount of serialization even if there's only a single writer,
as KVM kicks all vCPUs to toggle APICv (and again to flush TLBs, if necessary).

Hmm, something doesn't add up.  Naveen's changelog says:

  KVM additionally inhibits AVIC for requesting a IRQ window every time it has
  to inject external interrupts resulting in a barrage of inhibits being set and
  cleared. This shows significant performance degradation compared to AVIC being
  disabled, due to high contention on apicv_update_lock.

But if this is a "real world" use case where the only source of ExtInt is the
PIT, and kernels typically only wire up the PIT to the BSP, why is there
contention on apicv_update_lock?  APICv isn't actually being toggled, so readers
blocking writers to handle KVM_REQ_APICV_UPDATE shouldn't be a problem.

Naveen, do you know why there's a contention on apicv_update_lock?  Are multiple
vCPUs actually trying to inject ExtInt?