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

On Sat, Aug 08, 2026 at 11:09:41AM +0100, David Woodhouse wrote:
> On Sat, 2026-08-08 at 08:09 +0100, David Woodhouse wrote:
> > 
> > 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.

By the way, good point on all the SRCU instances sharing a common
set of workqueues.  More ways to deadlock!  But I don't see having
per-srcu_struct sets of dedicated kthreads.  ;-)

> In fact, why even do that as a fallback? This allocation is *literally*
> one size fits all, isn't it? The size depends on rcu_num_nodes which is
> a boot-time constant.
> 
> Just allocate it in *advance* from a safe context, and have
> init_srcu_struct_nodes() consume the preallocated one. And if a second
> init_srcu_struct_nodes() runs before it can be replenished, *then* fall
> back to a direct GFP_NOWAIT allocation.
> 
> We'd probably want to take a look at the early boot path, but
> GFP_KERNEL is effectively GFP_NOWAIT at that point anyway —
> gfp_allowed_mask strips __GFP_RECLAIM until well after srcu_init() has
> run.
> 
> Something like this, perhaps...

This does look like a reasonable approach, though the thought of having
something like this surrounding many memory allocations throughout the
kernel is not a happy one.

Nevertheless, given the current state, it is not at all bad.

A few questions and comments below...

							Thanx, Paul

> From: David Woodhouse <[email protected]>
> Date: Sat, 8 Aug 2026 11:01:52 +0100
> Subject: [PATCH] srcu: Keep a spare node array so srcu_gp_end() need not allocate
> 
> 
> The one-time transition of an srcu_struct from SRCU_SIZE_SMALL to
> SRCU_SIZE_BIG allocates the srcu_node combining tree with GFP_KERNEL
> from srcu_gp_end(). That runs on the same workqueue which processes
> grace periods for every srcu_struct in the system — including grace
> periods awaited from OOM/reclaim contexts such as the OOM reaper
> calling synchronize_srcu() via an mmu_notifier. If the allocation
> blocks in direct reclaim, it can be waiting on the very OOM reaper
> whose grace period is queued behind it: a deadlock.
> 
> The allocation is literally one size fits all: it depends only on
> rcu_num_nodes, which is fixed once rcu_init_geometry() has run. So
> keep a single preallocated spare array, primed in srcu_init() when
> lazy (contention-triggered) sizing is in effect, and have
> init_srcu_struct_nodes() consume it with xchg(). The consumer kicks a
> replenish worker on system_wq (a clean context where GFP_KERNEL is
> safe); if a second transition races ahead of the replenish, the
> GFP_NOWAIT fallback keeps today's benign fail-and-retry behaviour,
> now with no possibility of blocking the grace-period workqueue in
> reclaim.
> 
> The init-time paths (init_srcu_struct_fields() and boot) also flow
> through srcu_alloc_nodes() and may consume the spare; that is
> harmless, as the replenish worker refills it.

True, but shouldn't we take as much pressure off of the spare as we can
so that it will be there for us when we really need it.

> Not-yet-tested-by: David Woodhouse <[email protected]>
> Signed-off-by: David Woodhouse <[email protected]>
> Assisted-by: Claude:claude-mythos-5
> ---
>  kernel/rcu/srcutree.c | 64 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 61 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 7c2f7cc131f7..914a0574dce2 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -123,6 +123,51 @@ static inline bool srcu_invl_snp_seq(unsigned long s)
>  	return s == SRCU_SNP_INIT_SEQ;
>  }
>  
> +/*
> + * A standing spare srcu_node array. The size of the allocation depends
> + * only on rcu_num_nodes, which is fixed once rcu_init_geometry() has run,
> + * so one preallocated array fits every srcu_struct in the system.
> + *
> + * This exists because srcu_gp_end() may need to allocate the array when
> + * a size transition is triggered by contention, and srcu_gp_end() runs
> + * on the same workqueue for every srcu_struct — including grace periods
> + * awaited from OOM/reclaim contexts (e.g. the OOM reaper via an
> + * mmu_notifier). Blocking there in GFP_KERNEL reclaim can deadlock: the
> + * reclaim may be waiting on the very OOM reaper whose grace period is
> + * queued behind this allocation. Consuming a preallocated array instead
> + * keeps the grace-period path allocation-free; the spare is replenished
> + * from a clean context on system_wq, and a direct GFP_NOWAIT attempt
> + * remains as the fallback if a second transition wins the race for the
> + * spare before it can be replenished.
> + */
> +static struct srcu_node *srcu_spare_nodes;
> +
> +static void srcu_spare_replenish_wq(struct work_struct *work)
> +{
> +	struct srcu_node *spare;
> +
> +	spare = kzalloc_objs(*spare, rcu_num_nodes, GFP_KERNEL);
> +	if (!spare)
> +		return;
> +	if (cmpxchg(&srcu_spare_nodes, NULL, spare))

Mightn't !try_cmpxchg() be a better fit here?  You are using the returned
pointer as a boolean anyway.  (One could also argue for xchg(), but why
unnecessarily write to that poor cache line?)

Of course, we get here only if two different SRCU workqueue handlers
both see a NULL srcu_spare_nodes, and one of the is delayed so that its
schedule_work() call follows the other's completion of its workqueue
handler.  So low probability, right?  (Otherwise, I would suggest a
check of srcu_spare_nodes before doign the initial kzalloc_objs().)

> +		kfree(spare);	/* Someone else refilled it first. */
> +}
> +static DECLARE_WORK(srcu_spare_replenish_work, srcu_spare_replenish_wq);
> +
> +static struct srcu_node *srcu_alloc_nodes(gfp_t gfp_flags)
> +{
> +	struct srcu_node *node;
> +

Why not do a "GFP_NOWAIT | __GFP_NOWARN" attempt before raiding
srcu_spare_nodes?  Wouldn't that increase the probability that there
would be an srcu_node array available when someone really needed it?

Yes, I do see that schedule_work(), but that can involve significant
delays if the system is under any sort of pressure, right?

> +	node = xchg(&srcu_spare_nodes, NULL);
> +	if (node) {
> +		schedule_work(&srcu_spare_replenish_work);
> +		return node;
> +	}
> +
> +	/* Spare already taken and not yet replenished. */
> +	return kzalloc_objs(*node, rcu_num_nodes, gfp_flags);

And the across-SRCU shared-workqueue deadlock that you pointed out is
avoided because the only way that gfp_flags is set to GFP_KERNEL is when
the caller is supplying its own task, correct?

> +}
> +
>  /*
>   * Allocated and initialize SRCU combining tree.  Returns @true if
>   * allocation succeeded and @false otherwise.
> @@ -139,8 +184,7 @@ static bool init_srcu_struct_nodes(struct srcu_struct *ssp, gfp_t gfp_flags)
>  
>  	/* Initialize geometry if it has not already been initialized. */
>  	rcu_init_geometry();
> -	ssp->srcu_sup->node = kzalloc_objs(*ssp->srcu_sup->node, rcu_num_nodes,
> -					   gfp_flags);
> +	ssp->srcu_sup->node = srcu_alloc_nodes(gfp_flags);
>  	if (!ssp->srcu_sup->node)
>  		return false;
>  
> @@ -1004,7 +1048,7 @@ static void srcu_gp_end(struct srcu_struct *ssp)
>  	/* Transition to big if needed. */
>  	if (ss_state != SRCU_SIZE_SMALL && ss_state != SRCU_SIZE_BIG) {
>  		if (ss_state == SRCU_SIZE_ALLOC)
> -			init_srcu_struct_nodes(ssp, GFP_KERNEL);
> +			init_srcu_struct_nodes(ssp, GFP_NOWAIT | __GFP_NOWARN);
>  		else
>  			smp_store_release(&sup->srcu_size_state, ss_state + 1);
>  	}
> @@ -2111,6 +2155,20 @@ void __init srcu_init(void)
>  		}
>  	}
>  
> +	/*
> +	 * Prime the spare node array if lazy (contention-triggered) size
> +	 * transitions are possible, so that srcu_gp_end() never needs to
> +	 * allocate. Early-boot GFP_KERNEL is implicitly non-blocking
> +	 * (gfp_allowed_mask strips __GFP_RECLAIM until much later), and
> +	 * failure here is harmless: the GFP_NOWAIT fallback and replenish
> +	 * worker remain.
> +	 */
> +	if (SRCU_SIZING_IS_CONTEND() || SRCU_SIZING_IS_TORTURE()) {
> +		rcu_init_geometry();

And this is early boot, which means that there is only one CPU with
interrupts off, thus rcu_init_geometry() is idempotent, so OK.

> +		srcu_spare_nodes = kzalloc_objs(*srcu_spare_nodes,
> +						rcu_num_nodes, GFP_KERNEL);
> +	}
> +
>  	/*
>  	 * Once that is set, call_srcu() can follow the normal path and
>  	 * queue delayed work. This must follow RCU workqueues creation
> -- 
> 2.43.0
>