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

Shakeel Butt
2025-04-30T15:16:58Z

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.