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

On Fri, 2026-08-07 at 14:55 -0700, Paul E. McKenney wrote:
> On Fri, Aug 07, 2026 at 12:02:57AM +0200, David Woodhouse wrote:
> > On 6 August 2026 23:52:06 CEST, "Paul E. McKenney" <[email protected]> wrote:
> > > 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?
> > 
> > Thanks for the prompt answer!
> > 
> > It's a nice idea, but I'm not sure what we'd trigger on. In an
> > earlier patch in this same series I already ripped *out* the
> > non_block_start() for this OOM reaper path, because that would make
> > it sad even about the non-allocating sleep for the (S)RCU wait. The
> > actual criterion is more subtle than that.
> 
> Yeah, you can be blocked waiting for memory "just because" from one
> call to synchronize_srcu() and then when you invoke synchronize_srcu()
> from the OOM reaper, you still block because your OOM-reaper-induced
> synchronize_srcu() can't start its grace period until the earlier
> synchronize_srcu()'s grace period completes.
> 
> And there probably are a *lot* of similarly fatal scenarios.  :-/
> 
> > And anyway, the allocation isn't even running from that thread;
> > it's on the workqueue.
> 
> Maybe we need a GFP_SHORT_WAIT that gives up if more than (say) ten
> jiffies elapse.  Then the needed NULL return shows up and everything
> goes forward.  Plus if someone starts their first SRCU grace period from
> a safe MM state, it does what is needed to get the memory, thus reducing
> contention on the srcu_struct-related synchronization mechanisms.
> Which is why I am uncomfortable with unconditional use of GFP_NOWAIT
> or similar.
> 
> What we have now is at best exceedingly non-ergonomic for the surprisingly
> large fraction of the kernel that can be invoked from OOM/reclaim.
> 
> > I think it has to be an API which lets us upgrade in advance. Or
> > just make it GFP_NOWAIT unconditionally? What's the failure mode if
> > the allocation fails? My unreliable AI friend told me it was
> > harmless enough... but I don't trust it much, which is why I
> > reached out to my meat friend...
> 
> So your setup is has the default srcutree.convert_to_big=3
> (SRCU_SIZING_AUTO) and more than 128 CPUs, correct?  Does this need to
> work with old-school CONFIG_PREEMPT_NONE and CONFIG_PREEMPT_VOLUNTARY,
> or is new-age CONFIG_PREEMPT_LAZY in place?

I'm trying to make this work for everyone. *My* setup doesn't have
PREEMPT_RT and I'm perfectly happy with the rwlocks, thank you very
much (well, I *was* until I saw how much better things scale when I
started experimenting with RCU...)

> Here are the options I can see at this point:
> 
> 1.	As you and your AI friend suggest, change the GFP_KERNEL
> 	to GFP_NOWAIT.	As noted above, I am not so happy about the
> 	possibility of eternal contention due to eternal failure.

The only failure mode here is that it defers the upgrade to big mode,
and a few more grace periods run slightly less efficiency after the
threshold for upgrading has been tripped?

> 2.	As above, change the GFP_KERNEL to GFP_NOWAIT.	But then, upon
> 	failure, also invoke something in MM that greatly increases the
> 	probability of success on the next attempt.  I have no idea
> 	that that "something" might be.

I was pondering 'set a global flag to indicate that OOM is running' and
only using GFP_NOWAIT in that circumstance. That was icky enough in the
first place but I don't think it's viable anyway — all SRCUs are
handled on the *same* workqueue, and that workqueue could already be
blocked on some *other* SRCU upgrading using GFP_KERNEL — which could
even have been the *reason* the OOM kicked in? 

> 3.	Use a GFP_SOMETHING that can return NULL, but dynamically
> 	figures out how far it can go, possibly bounded by a timeout.
> 	Then we replace SRCU's GFP_KERNEL with that GFP_SOMETHING.
> 
> 4.	Implement GFP_SOMETHING by hand by using GFP_NOWAIT, and on
> 	failure spawning a workqueue to do the allocation asynchronously
> 	in a clean environment, not blocking any grace period.
> 	This is complicated by the fact that it introduces concurrency
> 	into the allocation.

Is the concurrency so hard? The separate workqueue just needs to
allocate some memory, then WRITE_ONCE() the pointer to it. It can
actually be *consumed* by the next srcu_gp_end() run.

> 5.	Give you an init_srcu_struct_big() that is init_srcu_struct() if
> 	srcutree.convert_to_big is SRCU_SIZING_NONE or SRCU_SIZING_INIT,
> 	but immediately does the allocation otherwise.
> 
> 	Of course, this allocation can fail, and if it does, you are
> 	right back where you started.

I think this doesn't work for much the same reason that #2 doesn't
work, since everything on the same workqueue so it would need to make
sure *nothing* takes the upgrade path; not just the SRCU in question?

> 6.	As above, but if the allocation does fail, do a self-propagating
> 	call_srcu() that keeps running until the allocation within
> 	srcu_gp_end() succeeds.  Something that I am sure that the
> 	battery-powered-embedded guys will just love.	And so will you
> 	if that from-OOM/reclaim call gets there before the successful
> 	allocation.  ;-)
>
> 7.	Combine #6 and #1, so that if init_srcu_struct_big() was used,
> 	srcu_gp_end() uses GFP_NOWAIT, but otherwise sticks with the
> 	current GFP_KERNEL.  This at least restricts the blast radius
> 	of GFP_NOWAIT fallout.

As I was reading through, I was landing on "combine #4 and #1": use
GFP_NOWAIT unconditionally, but on failure trigger a workqueue to
allocate that memory in a clean environment.

> I don't know about you, but none of these are sparking joy at this end.
> Other than the mythical GFP_SOMETHING, of course, but for that there is
> the small issue of it currently being mythical.
> 
> If this must be fixed in SRCU, pragmatism says that I would hold my nose
> and take door #7.
> 
> But first, is this *really* the best that we can do?

I'm more than happy to entertain a discussion about whether we should
be using RCU for this, and what type of RCU.

Trying to summarize where we're coming from... I described the GPC
already. We are currently using a rwlock, which is kind of OK (except
for the known scalability issues with rwlocks since even the reads have
to contend for a cache line). But it doesn't work on PREEMPT_RT because
in that setup, rwlocks become *sleeping* locks. As noted above, that
isn't *my* use case and it hasn't kept me awake at night and I largely
blamed it on RT for changing the semantics of a known lock type... but
I really ought to fix it.

We use the GPC read side from hardirq context and on the schedule_out
path, so sleeping locks are Right Out. And RT has no 'raw_rwlock_t'.

Now, *most* of our GPCs are basically per-CPU, so a plain spinlock
would mostly suffice. But the Xen shared_info and vcpu_info regions are
*not* per-CPU, and this directly affects the scalability of interrupt
delivery:
https://lore.kernel.org/all/[email protected]/

I started experimenting with plain RCU, which is promising but Sean
pointed out that on a large hosting box with many pCPUs and lots of
small guests, each grace period is waiting for *all* of the host
CPUs... and directed me to SRCU:
https://lore.kernel.org/all/[email protected]/

This *currently* seems like the best approach, if we can just work out
the wart of the OOM reaper thing. But I'm absolutely open to input.