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

On Thu, Aug 06, 2026 at 10:38:56PM +0200, David Woodhouse wrote:
> On Thu, 2026-08-06 at 19:59 +0200, Woodhouse, David wrote:
> > On Thu, 2026-08-06 at 09:53 -0700, Sean Christopherson wrote:
> > > On Wed, Aug 05, 2026, [email protected] 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. Although in my tree I've optimised that *not* to incur a grace
> > period when it's avoidable, as noted above.
> > 
> > I'm more concerned by the fact that srcu_gp_end() might have to
> > *allocate*, while the OOM reaper path waits for it. I'll see how we can
> > deal with that...
> 
> I think I need to invoke Paul et al for that one...
> 
> Paul, Boqun, please could I trouble you to look at the top commit in
> https://git.infradead.org/?p=users/dwmw2/linux.git;a=shortlog;h=refs/heads/xen-rcu-srcu
> 
> The "gfn_to_pfn_cache" (GPC) here is for all extents and purposes a
> TLB. It's caching the full two-stage translation through KVM memslots
> which map a guest physical address to a userspace virtual address, and
> then through the standard process page tables to a host physical
> address.
> 
> It is the second stage we are interested in here. Invalidation happens
> through an MMU notifier, which (after I obey Sean's request to switch
> to SRCU) calls synchronize_srcu_expedited().
> 
> My problem:
> 
>  1. The MMU notifier invalidation can be called from the OOM reaper
>     path. In this case it MUST NOT sleep to allocate memory, or block
>     on anything which does so. cf.
>     https://lore.kernel.org/all/[email protected]/
> 
>  2. srcu_gp_end() does so, through init_srcu_struct_nodes(), while
>     synchronize_srcu_expedited() effectively waits for it — precisely
>     the thing we must not do. (Strictly, the waiter's completion is
>     queued to the same workqueue on which process_srcu() could be
>     blocked in init_srcu_struct_nodes(…, GFP_KERNEL)).
> 
> On a system with >= 128 CPUs it's actually OK because that allocation
> path never happens; everything's allocated in advance ("rcu: srcu_init:
> Setting srcu_struct sizes to big"). But on smaller systems I don't see
> that we have a way to ensure that things are safe. (I don't count
> setting big_cpu_lim to zero!)
> 
> Could we change that init_srcu_struct_nodes(…, GFP_KERNEL) to a non-
> sleeping allocation? If the allocation fails, that isn't fatal, is it?
> Or some other way to trigger the transition to big under controlled
> circumstances in advance...? 

We could easily provide such an interface.  However, is there some
check that would allow us to determine when it is OK to use GFP_KERNEL?
If there was, we could replace that GFP_KERNEL with something like this:

	sleeping_alloc_ok() ? GFP_KERNEL : GPF_HEY_YOU_TELL_ME

My best guess for GPF_HEY_YOU_TELL_ME is GFP_NOWAIT.

That way, you don't need another SRCU API to transition to big, and
everything "just works".

What say you?

							Thanx, Paul