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

[Joshua Hahn]

On Mon, 7 Sep 2026 16:21:33 -0700 Shakeel Butt <shakeel.butt@xxxxxxxxx> wrote:

> 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.

Hi Shakeel, thanks for your review on the series!

Yes, I'm sorry about that. There were a few layers of protection that
I wanted to make against nested NMI, ordering, publishing for stock, 
etc. I think I didn't make it clear what was happening, so I'll
address that in the next version.

> > +	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.

Yes, that's exactly what it is. I think there was a lot of thinking
on my end on things to look out for that in the end it just kind of
became a jumbled mess, I'll definitely do a re-spin of what's happening
in the next version.

> > +
> > +	/* 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?

All I was trying to say here is that we don't need to drain an exact
CPU, all we need to do is to try and get any CPU. So if we migrate
and get a different CPU's ptr than we initially started with, it's no
big deal (that's what I tried to explain with saying that stock is
fungible). But I agree it's a little lacking in explanation here,
I'll rework it in the next version.

> > +	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.

I agree, sorry for that : -(

> 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.

That sounds like a good plan to me. I'll do exactly that and send a
new version.

Thank you for your time Shakeel, I hope you have a great rest of your
day! : -)
Joshua