Re: [PATCH v5 4/7] mm/page_counter: use stock in page_counter_try_charge

[Shakeel Butt]

On Mon, Aug 31, 2026 at 09:37:48AM -0700, Joshua Hahn wrote:
> Transparently make page_counter_try_charge attempt to service the charge
> from its stock. We preserve the same semantics as the existing stock
> management in try_charge_memcg:
> 
> 1. Limit-check against the stock. If there is enough, then skip the
>    hierarchy walk and charge to the stock.
> 2. Greedily attempt to fulfill the charge request and refill the stock
>    simultaneously to the hierarchy.
> 3. If this fails, retry the stock and charge without trying to refill
>    the stock, i.e. with the number of pages requested.
> 4. If the greedy attempt succeeds, return excess pages to the stock.
> 
> page_counter_refill_stock() falls back to a hierarchical uncharge when
> there is no stock, in NMI contexts, on lock contention, or for a refill
> larger than the batch.
> 
> The greedy charge is also skipped in NMI where both stock helpers bail
> out since the batch charge would be undone again.
> 
> No functional change intended, since no page_counter enables stock yet
> and counter->batch is left at 0.
> 
> Suggested-by: Johannes Weiner <hannes@xxxxxxxxxxx>
> Signed-off-by: Joshua Hahn <joshua.hahnjy@xxxxxxxxx>
> ---
>  include/linux/page_counter.h |   2 +
>  mm/page_counter.c            | 135 +++++++++++++++++++++++++++++++----
>  2 files changed, 125 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/page_counter.h b/include/linux/page_counter.h
> index c1fe331f34e7e..428ca8e7b2da5 100644
> --- a/include/linux/page_counter.h
> +++ b/include/linux/page_counter.h
> @@ -82,6 +82,8 @@ static inline unsigned long page_counter_read(struct page_counter *counter)
>  
>  void page_counter_cancel(struct page_counter *counter, unsigned long nr_pages);
>  void page_counter_charge(struct page_counter *counter, unsigned long nr_pages);
> +unsigned long page_counter_refill_stock(struct page_counter *counter,
> +					unsigned long overage);
>  bool page_counter_try_charge(struct page_counter *counter,
>  			     unsigned long nr_pages, struct page_counter **fail,
>  			     unsigned long *nr_charged);
> diff --git a/mm/page_counter.c b/mm/page_counter.c
> index 3f61eba695518..a76949abf04e7 100644
> --- a/mm/page_counter.c
> +++ b/mm/page_counter.c
> @@ -113,25 +113,126 @@ void page_counter_charge(struct page_counter *counter, unsigned long nr_pages)
>  	}
>  }
>  
> +static bool page_counter_consume_stock(struct page_counter *counter,
> +				       unsigned long nr_pages)
> +{
> +	struct page_counter_stock __percpu *stock = READ_ONCE(counter->stock);
> +	struct page_counter_stock *pcp_stock;
> +	unsigned long flags;
> +	bool charged = false;
> +
> +	if (!stock || nr_pages > counter->batch)
> +		return false;
> +
> +	/* raw_spin_trylock isn't enough to protect against nested NMI in UP */

I don't understand what this comment is trying to say. The nested NMI is
confusing.

> +	if (in_nmi())
> +		return false;

You are completely disabling stocks for memcg charges in nmi context. Why? I
assume that is what the comment above trying to explain but it is failing.

IIUC you want to use spin_lock instead of local_trylock because you want to
support draining from remote cpus and spin_lock on UP are simply disable irq and
does not protect from NMI. Maybe you need spin_trylock similar to local_trylock.
Not saying you to implement that but please explain stuff clearly.

> +
> +	/* It's OK to migrate here, since stock is fungible within a counter. */
> +	pcp_stock = raw_cpu_ptr(stock);

migrate between cpus? Why? What are you gaining by allowing that?

> +
> +	if (!raw_spin_trylock_irqsave(&pcp_stock->lock, flags))

Why do you need to disable irqs?

Anyways, you are changing the fast path of the charge drastically. Previously
there was no atomic ops and not irq toggling but this patch is adding atomic op
and irq toggle (not sure about why irq toggle is needed) on the fast path.

I understand that remote draining is the only reason you need to use spin locks
here otherwise you will need to allocate work_struct in page_counter_stock. You
are making these design decisions very silently and implicitly.

How about we decouple the decision of remote drain / spin lock from moving stock
inside page counter? First move the stock to page counter without any spin lock
or remote drain and later in the series you convert to spin lock plus remote
draining with performance numbers.