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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ae4b9ac8-d67d-471f-89b9-7eeaf58dd1b8@suse.cz>
Date: Wed, 30 Apr 2025 12:05:48 +0200
From: Vlastimil Babka <vbabka@...e.cz>
To: Shakeel Butt <shakeel.butt@...ux.dev>,
 Andrew Morton <akpm@...ux-foundation.org>
Cc: 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 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?

> ---
>  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);