linux-kernel - Re: [PATCH] memcg: manually uninline __memcg_memory

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: <20251022005801.120250-1-sj@kernel.org>
Date: Tue, 21 Oct 2025 17:58:00 -0700
From: SeongJae Park <sj@...nel.org>
To: Shakeel Butt <shakeel.butt@...ux.dev>
Cc: SeongJae Park <sj@...nel.org>,
	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>,
	linux-mm@...ck.org,
	cgroups@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Meta kernel team <kernel-team@...a.com>
Subject: Re: [PATCH] memcg: manually uninline __memcg_memory_event

On Tue, 21 Oct 2025 16:44:25 -0700 Shakeel Butt <shakeel.butt@...ux.dev> wrote:

> The function __memcg_memory_event has been unnecessarily marked inline
> even when it is not really performance critical. It is usually called
> to track extreme conditions. Over the time, it has evolved to include
> more functionality and inlining it is causing more harm.
> 
> Before the patch:
> $ size mm/memcontrol.o net/ipv4/tcp_input.o net/ipv4/tcp_output.o
>    text    data     bss     dec     hex filename
>   35645   10574    4192   50411    c4eb mm/memcontrol.o
>   54738    1658       0   56396    dc4c net/ipv4/tcp_input.o
>   34644    1065       0   35709    8b7d net/ipv4/tcp_output.o
> 
> After the patch:
> $ size mm/memcontrol.o net/ipv4/tcp_input.o net/ipv4/tcp_output.o
>    text    data     bss     dec     hex filename
>   35137   10446    4192   49775    c26f mm/memcontrol.o
>   54322    1562       0   55884    da4c net/ipv4/tcp_input.o
>   34492    1017       0   35509    8ab5 net/ipv4/tcp_output.o
> 
> Signed-off-by: Shakeel Butt <shakeel.butt@...ux.dev>
> ---
>  include/linux/memcontrol.h | 32 ++------------------------------
>  mm/memcontrol.c            | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+), 30 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index d37e7c93bb8c..8d2e250535a8 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -1002,36 +1002,8 @@ static inline void count_memcg_event_mm(struct mm_struct *mm,
>  	count_memcg_events_mm(mm, idx, 1);
>  }
>  
> -static inline void __memcg_memory_event(struct mem_cgroup *memcg,
> -					enum memcg_memory_event event,
> -					bool allow_spinning)
> -{
> -	bool swap_event = event == MEMCG_SWAP_HIGH || event == MEMCG_SWAP_MAX ||
> -			  event == MEMCG_SWAP_FAIL;
> -
> -	/* For now only MEMCG_MAX can happen with !allow_spinning context. */
> -	VM_WARN_ON_ONCE(!allow_spinning && event != MEMCG_MAX);
> -
> -	atomic_long_inc(&memcg->memory_events_local[event]);
> -	if (!swap_event && allow_spinning)
> -		cgroup_file_notify(&memcg->events_local_file);
> -
> -	do {
> -		atomic_long_inc(&memcg->memory_events[event]);
> -		if (allow_spinning) {
> -			if (swap_event)
> -				cgroup_file_notify(&memcg->swap_events_file);
> -			else
> -				cgroup_file_notify(&memcg->events_file);
> -		}
> -
> -		if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
> -			break;
> -		if (cgrp_dfl_root.flags & CGRP_ROOT_MEMORY_LOCAL_EVENTS)
> -			break;
> -	} while ((memcg = parent_mem_cgroup(memcg)) &&
> -		 !mem_cgroup_is_root(memcg));
> -}
> +void __memcg_memory_event(struct mem_cgroup *memcg,
> +			  enum memcg_memory_event event, bool allow_spinning);
>  
>  static inline void memcg_memory_event(struct mem_cgroup *memcg,
>  				      enum memcg_memory_event event)
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 1a95049d8b88..93f7c76f0ce9 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1626,6 +1626,37 @@ unsigned long mem_cgroup_size(struct mem_cgroup *memcg)
>  	return page_counter_read(&memcg->memory);
>  }
>  
> +void __memcg_memory_event(struct mem_cgroup *memcg,
> +			  enum memcg_memory_event event, bool allow_spinning)

Seems this function is called only from memcontrol.c.  Why not making it a
static function?

> +{
> +	bool swap_event = event == MEMCG_SWAP_HIGH || event == MEMCG_SWAP_MAX ||
> +			  event == MEMCG_SWAP_FAIL;
> +
> +	/* For now only MEMCG_MAX can happen with !allow_spinning context. */
> +	VM_WARN_ON_ONCE(!allow_spinning && event != MEMCG_MAX);
> +
> +	atomic_long_inc(&memcg->memory_events_local[event]);
> +	if (!swap_event && allow_spinning)
> +		cgroup_file_notify(&memcg->events_local_file);
> +
> +	do {
> +		atomic_long_inc(&memcg->memory_events[event]);
> +		if (allow_spinning) {
> +			if (swap_event)
> +				cgroup_file_notify(&memcg->swap_events_file);
> +			else
> +				cgroup_file_notify(&memcg->events_file);
> +		}
> +
> +		if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
> +			break;
> +		if (cgrp_dfl_root.flags & CGRP_ROOT_MEMORY_LOCAL_EVENTS)
> +			break;
> +	} while ((memcg = parent_mem_cgroup(memcg)) &&
> +		 !mem_cgroup_is_root(memcg));
> +}
> +EXPORT_SYMBOL(__memcg_memory_event);

Also, seems there is no reason to export this symbol?


Thanks,
SJ

[...]