Re: [PATCH v3 3/7] KVM: pfncache: Use RCU for readers instead of a rwlock

["Woodhouse, David" <dwmw@amazon.co.uk>]

[-- Attachment #1.1: Type: text/plain, Size: 3494 bytes --]

On Thu, 2026-08-06 at 11:11 -0700, Sean Christopherson wrote:
> On Thu, Aug 06, 2026, David Woodhouse wrote:
> > On Thu, 2026-08-06 at 09:53 -0700, Sean Christopherson wrote:
> > > On Wed, Aug 05, 2026, sashiko-bot@kernel.org wrote:
> > > > Replace the per-cache rwlock with RCU for the read side.
> > > 
> > > I don't hate the idea, but I am very against using RCU.  Unless it's "impossible",
> > > e.g. because synchronize_srcu() allocates memory and breaks OOM kill, I would
> > > strongly prefer to use SRCU, probably with a dedicated kvm->gpc_srcu, so that
> > > synchronization doesn't need to wait on all CPUs in the system.  The tail latencies
> > > for synchronize_rcu() are horrendous, especially for many-CPU systems.  If it
> > > were only mmu_notifiers that got hit, it miiiight be acceptable, but since this
> > > will affect vCPU tasks in the refresh() path as well, normal RCU is pretty much
> > > a non-starter.
> > 
> > Yeah, the refresh() path got pretty slow in my first attempt, before
> > optimising that *not* to have a grace period if the memslot generation
> > changed but the actual GPA→uHVA (and memslot) don't *change*.
> > 
> > > Even SRCU could be problematic: if synchronize_srcu_expedited() is forced to wait,
> > > the wait time can easily get to 20+ milliseconds, which again is a non-starter for
> > > things like steal-time updates and nVMX pages.
> > 
> > But we don't have to synchronize from the read side. The code path
> > which will do so most often is gfn_to_pfn_cache_invalidate_start(). And
> > the refresh() path which is already the fallback slow path which can sleep.
> 
> It's probably a slow path for all current users, but it definitely won't be a slow
> path for nested virtualization, because a refresh() will be required any time the
> GPA changes, i.e. any time the vCPU runs a different vmc{b,c}12.  That's why I
> think we should treat GPCs that are strictly bound to a vCPU differently.

"treat GPCs that are strictly bound to a vCPU differently"....

Of course, at this point we have to make the observation that with a
naïve and simple spinlock, we don't *need* any additional complexity to
treat single-CPU users differently; they just *naturally* go faster
because they always own that cache line anyway, while the rarer case of
concurrent access is naturally correct too.

The thing that's held me back is the shared_info and vcpu_info cases
which are used for incoming interrupts; I'd really need to do some
serious scalability testing to compare with the rwlock we have at the
moment.

But honestly, the Xen 2-level event channel mechanism is *already* a
cache contention minefield. Even if the GPC is lockless, two concurrent
host interrupts attempting to deliver event channels to the guest are
*also* going to clash on atomic accesses to the shinfo->evtchn_pending
array, as nobody makes any attempt to ensure that event channels for
different vCPUs are placed on different cache lines in that bitmap.

We might even find that concurrent event channel delivery is *better*
when it's serialised by a spinlock, than using rwlocks. Either way, I
think we can *certainly* live with just flipping it to be a (raw)
spinlock in the RT case.

I'll play. In the meantime, the rest of the fixes are in my branch¹
before the RCU stuff, so I'll post those probably on Monday.

¹ https://git.infradead.org/?p=users/dwmw2/linux.git;a=shortlog;h=refs/heads/xen-rcu-srcu

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

[-- Attachment #2.1: Type: text/plain, Size: 215 bytes --]




Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.



[-- Attachment #2.2: Type: text/html, Size: 228 bytes --]