Re: [PATCH v4 10/17] KVM: arm64: Add a shrinker for pKVM

On Tue, Aug 25, 2026 at 08:40:03AM +0100, Fuad Tabba wrote:
> On Tue, 25 Aug 2026 at 08:22, Vincent Donnefort <[email protected]> wrote:
> ...
> > > > > Nothing marks the pages a top-up just put in allocator->mc as spoken
> > > > > for, and hyp_allocator_reclaim() ends with an unbounded drain of it,
> > > > > so a shrink with target 1 hands back the lot. Land that between a
> > > > > top-up and the retry it was for, and the retry asks again, and
> > > > > pkvm_call_hyp_req() goes round.
> > > >
> > > > Sorry, I am not sure I follow here.
> > > >
> > > > IIRC, the shrinker will only reclaim half of what is available. So the pressure
> > > > should be proportional to what is available and limit races with topup!
> > >
> > > It's the ordering, not the amount.
> >
> > Do you think we should first try to reclaim from the mapped pages before
> > draining the allocator->mc?
> >
> > Mapped pages are more valuable hence why I have started with allocator->mc.
> >
> > But, it is true it might make sense. It is unlikely to have pages left unused
> > into that mc. If that mc has been topped-up that's because it is about to be
> > allocated from...
> 
> I think that would work, as long as the ULONG_MAX drain at the tail of
> hyp_allocator_reclaim() is bounded too. The memcache is LIFO, so the
> pages hyp_allocator_unmap() stages sit on top of the top-up ones, and
> draining just what the chunk loop reclaimed leaves the rest alone.
> 
> It would still fall through to the top-up pages once the chunks run
> out, which is where your last point comes in. hyp_allocator_map() only
> raises a request when it finds the mc empty, so pages sitting in there
> are ones a top-up just put there for a retry. Could the reclaim leave
> them alone altogether, and drop the mc.nr_pages term v4 added to
> hyp_allocator_reclaimable(), which is what advertises them to the
> shrinker?

There's nothing that prevents a users from topping-up the allocator mc... but to
never actually use the memory. That's why I think it is better to drain it.

> 
> > >
> > > topup and its retry are separate hypercalls, lock dropped between
> > > them, so a shrink on another CPU can slip in, right? .
> > > hyp_allocator_reclaim() drains allocator->mc, where the topup pages
> > > sit, before any chunk, so half still comes out of them first: a target
> > > of 1 fails the retry.
> >
> > I do not see where a target == 1 fails.
> 
> It's the retry that fails, not the reclaim. With target == 1,
> hyp_allocator_drain_memcache() pops one page and target is done, so
> the chunk loop never runs. But the top-up was sized to the exact
> shortfall, so the retry now runs the mc dry one page early, sets
> topup_needed = 1, returns -ENOMEM again, and pkvm_call_hyp_req() goes
> round for another top-up.
> 
> Nothing errors out, it just doesn't converge for as long as the
> shrinker keeps pace.
> 
> Keep in mind, I am not as familiar with this code as you are. So I
> might be completely off here :)

Ha yes, a concurrent topup with shrinking wouldn't cooperate well and I am not
sure how better we can do. But note the shrinker always "scans" half of the
reclaimable memory. So it is unlikely a user needs to top it up during
shrinking: if we reclaim hyp allocator memory that's because there's plenty
unused!

But I am happy to start with the memory mapped into the allocator and then if
necessary finish with the allocator->mc: if there are memory in allocator->mc
that's probably because a topup is pending.

-- 
Vincent

> 
> Cheers,
> /fuad
> 
> > >
> > > >
> > > > However now looking at it. I wonder if I don't want to ratelimit here the number
> > > > of pages reclaimed in one go to limit the time spent at EL2. Especially we do
> > > > all that with the allocator lock taken...
> > >
> > > Ratelimiting would cap the time under the lock, but it wouldn't stop a
> > > concurrent shrink from taking the topup pages, would it?
> >
> > Yes, my only intent is to avoid blocking at EL2 for too long.
> >
> > --
> > Vincent
> >
> > >
> > > Cheers,
> > > /fuad
> > >
> > >
> > > >
> >
> > [...]