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

Vlastimil Babka
2025-04-30T10:05:52Z

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?

> ---
> 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);
> - evict = false;
> + success = true;
> break;
> }
> }
>
> - if (evict) {
> - i = get_random_u32_below(NR_MEMCG_STOCK);
> - drain_stock(stock, i);
> - goto again;
> + if (!success) {
> + i = empty_slot;
> + if (i == -1) {
> + i = get_random_u32_below(NR_MEMCG_STOCK);
> + drain_stock(stock, i);
> + }
> + css_get(&memcg->css);
> + WRITE_ONCE(stock->cached[i], memcg);
> + WRITE_ONCE(stock->nr_pages[i], stock_pages);
> }
>
> local_unlock_irqrestore(&memcg_stock.stock_lock, flags);