Re: [PATCH] memcg: multi-memcg percpu charge cache

[Shakeel Butt <shakeel.butt@linux.dev>]

On Wed, Apr 30, 2025 at 12:05:48PM +0200, Vlastimil Babka wrote:
> On 4/25/25 22:18, Shakeel Butt wrote:
> > Hi Andrew,
> > 
> > Another fix for this patch. Basically simplification of refill_stock and
> > avoiding multiple cached entries of a memcg.
> > 
> > From 6f6f7736799ad8ca5fee48eca7b7038f6c9bb5b9 Mon Sep 17 00:00:00 2001
> > From: Shakeel Butt <shakeel.butt@linux.dev>
> > Date: Fri, 25 Apr 2025 13:10:43 -0700
> > Subject: [PATCH] memcg: multi-memcg percpu charge cache - fix 2
> > 
> > Simplify refill_stock by avoiding goto and doing the operations inline
> > and make sure the given memcg is not cached multiple times.
> > 
> > Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
> 
> It seems to me you could simplify further based on how cached/nr_pages
> arrays are filled from 0 to higher index and thus if you see a NULL it means
> all higher indices are also NULL. At least I don't think there's ever a
> drain_stock() that would "punch a NULL" in the middle? When it's done in
> refill_stock() for the random index, it's immediately reused.
> 
> Of course if that invariant was made official and relied upon, it would need
> to be documented and care taken not to break it.
> 
> But then I think:
> - refill_stock() could be further simplified
> - loops in consume_stop() and is_drain_needed() could stop on first NULL
> cached[i] encountered.
> 
> WDYT?
> 

Please see below.

> > ---
> >  mm/memcontrol.c | 27 +++++++++++++++------------
> >  1 file changed, 15 insertions(+), 12 deletions(-)
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 997e2da5d2ca..9dfdbb2fcccc 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -1907,7 +1907,8 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
> >  	struct mem_cgroup *cached;
> >  	uint8_t stock_pages;
> >  	unsigned long flags;
> > -	bool evict = true;
> > +	bool success = false;
> > +	int empty_slot = -1;
> >  	int i;
> >  
> >  	/*
> > @@ -1931,26 +1932,28 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
> >  
> >  	stock = this_cpu_ptr(&memcg_stock);
> >  	for (i = 0; i < NR_MEMCG_STOCK; ++i) {
> > -again:
> >  		cached = READ_ONCE(stock->cached[i]);
> > -		if (!cached) {
> > -			css_get(&memcg->css);
> > -			WRITE_ONCE(stock->cached[i], memcg);
> > -		}
> > -		if (!cached || memcg == READ_ONCE(stock->cached[i])) {
> > +		if (!cached && empty_slot == -1)
> > +			empty_slot = i;
> > +		if (memcg == READ_ONCE(stock->cached[i])) {
> >  			stock_pages = READ_ONCE(stock->nr_pages[i]) + nr_pages;
> >  			WRITE_ONCE(stock->nr_pages[i], stock_pages);
> >  			if (stock_pages > MEMCG_CHARGE_BATCH)
> >  				drain_stock(stock, i);

So, this drain_stock() above can punch a NULL hole in the array but I
think I do see your point. We can fill this hole by moving the last
non-NULL here. For now I plan to keep it as is as I have some followup
plans to make this specific drain_stock() conditional on the caller
(somewhat similar to commit 5387c90490f7f) and then I will re-check if
we can eliminate this NULL hole.

Thanks a lot for the reviews and suggestions.