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

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.

And anyway, the allocation isn't even running from that thread; it's on the workqueue.

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...