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

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.

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.  Simply using a per-GPC SRCU would
be gross, as gfn_to_pfn_cache_invalidate_start() would become absurdly complex in
order to juggle gpc_lock with synchronize_srcu_expedited().

A somewhat crazy idea would be to have a per-VM gpc_srcu, *and* a per-GPC srcu.
Readers would take both, refresh() would sync gpc->srcu, and invalidation would
sync kvm->gpc_srcu.  That way, refresh() wouldn't need to wait on concurrent
readers of *other* GPCs.

Actually, a better idea: use kvm->gpc_srcu to synchronize invalidations and
refresh() for GPCs that aren't tightly coupled to a vCPU, but for GPCs that are
*only* accessed by a single loaded vCPU, protect readers and refresh() with
vcpu->mutex.  That way, single-vCPU GPCs wouldn't need to synchronize() on refresh(),
because by definition there can't be concurrent readers with refresh().

That would basically punt on optimizing most of the Xen GPCs, but that's probably
ok?  Because the hot path GPCs, e.g. runstate_cache{,2}, are generally associated
1:1 with a vCPU, i.e. can avoid synchronizing on SRCU.  The one GPC that I see
as being problematic is vcpu_info_cache, because it's accesses cross-vCPU and so
the owning vCPU would need to synchronize() on refresh().  But if you're ok with
potentially high tail latencies if the vcpu_info_cache page is migrated or
reclaimed, then I doubt anyone else will complain.

> > --- a/virt/kvm/pfncache.c
> > +++ b/virt/kvm/pfncache.c
> > @@ -26,35 +26,49 @@ void gfn_to_pfn_cache_invalidate_start(struct kvm *kvm, unsigned long start,
> >  				       unsigned long end)
> >  {
> >  	struct gfn_to_pfn_cache *gpc;
> > +	bool cleared = false;
> >  
> >  	spin_lock(&kvm->gpc_lock);
> >  	list_for_each_entry(gpc, &kvm->gpc_list, list) {
> > -		read_lock_irq(&gpc->lock);
> > -
> > -		/* Only a single page so no need to care about length */
> > -		if (gpc->valid && !is_error_noslot_pfn(gpc->pfn) &&
> > +		if (smp_load_acquire(&gpc->valid) &&
> >  		    gpc->uhva >= start && gpc->uhva < end) {
> > -			read_unlock_irq(&gpc->lock);
> > -
> > -			/*
> > -			 * There is a small window here where the cache could
> > -			 * be modified, and invalidation would no longer be
> > -			 * necessary. Hence check again whether invalidation
> > -			 * is still necessary once the write lock has been
> > -			 * acquired.
> > -			 */
> > -
> > -			write_lock_irq(&gpc->lock);
> > -			if (gpc->valid && !is_error_noslot_pfn(gpc->pfn) &&
> > -			    gpc->uhva >= start && gpc->uhva < end)
> > -				gpc->valid = false;
> > -			write_unlock_irq(&gpc->lock);
> > -			continue;
> > +			WRITE_ONCE(gpc->valid, false);
> > +			cleared = true;
> >  		}
> > -
> > -		read_unlock_irq(&gpc->lock);
> >  	}
> >  	spin_unlock(&kvm->gpc_lock);
> > +
> > +	/*
> > +	 * Readers may still be using the old mapping, having sampled
> > +	 * gpc->valid before it was cleared. Wait for them all to drain
> > +	 * before the caller proceeds to zap the page tables; like a TLB
> > +	 * shootdown, this guarantees no access via the stale mapping
> > +	 * once the invalidation completes.
> > +	 *
> > +	 * This wait is safe even on unblockable ranges (the OOM reaper):
> > +	 * the constraint there is not "no sleeping" but "no blocking on
> > +	 * anything which may itself depend on memory allocation to make
> > +	 * progress" (see the reasoning in commit 312364f3534c and its
> > +	 * discussion). An RCU grace period has no such dependency: GPC
> > +	 * readers never allocate, never take mmap_lock, and never sleep.
> > +	 */
> > +	if (cleared)
> > +		synchronize_rcu();
> >  }
> 
> [Severity: Critical]
> Could this conditional wait bypass the grace period and lead to a regression
> with physical pages being freed too early?
> 
> If a concurrent mutator like __kvm_gpc_refresh transitions gpc->valid to
> false and is currently blocking on its own synchronize_rcu wait, the MMU
> notifier will see valid as false, skip the wait since cleared is false, and
> return immediately.
> 
> This would allow the host memory subsystem to free the page while the
> original mutator is still waiting for active RCU readers to finish.

Ya, pretty sure, Sashiko is correct.  The mmu_notifier rules require that all
mappings are removed before .invalidate_range_start() completes.  Seems like the
easiest solution is to key off of gpc->active, not gpc->valid.

> [ ... ]
> > @@ -74,6 +88,22 @@ bool kvm_gpc_check(struct gfn_to_pfn_cache *gpc, unsigned long len)
> >  {
> >  	struct kvm_memslots *slots = kvm_memslots(gpc->kvm);
> >  
> > +	RCU_LOCKDEP_WARN(!rcu_read_lock_held(),
> > +			 "kvm_gpc_check() without RCU read lock");
> > +
> > +	/*
> > +	 * Check valid *first*. The acquire pairs with the release-publish
> > +	 * in hva_to_pfn_retry(), so every field read below — and any use
> > +	 * of gpc->khva by the caller — is guaranteed to be from the
> > +	 * published generation, not a stale value reordered from before
> > +	 * the publish. The fields are then stable for the remainder of
> > +	 * the RCU read-side critical section, because every mutator
> > +	 * clears valid and waits a full grace period before changing
> > +	 * anything.
> > +	 */
> > +	if (!smp_load_acquire(&gpc->valid))
> > +		return false;
> > +
> >  	if (!gpc->active)
> >  		return false;
> 
> [Severity: Medium]
> Should the read of gpc->generation use READ_ONCE?

+1, the {WRITE,READ}_ONCE() usage looks to be very inconsistent.