[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <mjmayud53r2ypus22ab75c7kfaq7izaidde2bju536e2ghifdi@lslljpj2hdtm>
Date: Wed, 30 Apr 2025 08:16:40 -0700
From: Shakeel Butt <shakeel.butt@...ux.dev>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
Johannes Weiner <hannes@...xchg.org>, Michal Hocko <mhocko@...nel.org>,
Roman Gushchin <roman.gushchin@...ux.dev>, Muchun Song <muchun.song@...ux.dev>,
Jakub Kicinski <kuba@...nel.org>, Eric Dumazet <edumazet@...gle.com>,
Soheil Hassas Yeganeh <soheil@...gle.com>, linux-mm@...ck.org, cgroups@...r.kernel.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
Meta kernel team <kernel-team@...a.com>, Hugh Dickins <hughd@...gle.com>
Subject: Re: [PATCH] memcg: multi-memcg percpu charge cache
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@...ux.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@...ux.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.
Powered by blists - more mailing lists