Re: [PATCH] memcg: manually uninline __memcg_memory_event

[Shakeel Butt <shakeel.butt@linux.dev>]

On Tue, Oct 21, 2025 at 05:58:00PM -0700, SeongJae Park wrote:
> On Tue, 21 Oct 2025 16:44:25 -0700 Shakeel Butt <shakeel.butt@linux.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@linux.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?

There is a recent code where this is called (indirectly) from networking
stack for MEMCG_SOCK_THROTTLED.

> 
> > +{
> > +	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?

The networking code needs this export.

Thanks for taking a look.