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 [off-list ref] wrote:
...

quoted

quoted

quoted

quoted

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.

quoted

quoted

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

quoted

quoted

quoted

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

quoted

Cheers,
/fuad

quoted

[...]