| 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
| ||
|
Message-ID: <20251022021549.129413-1-sj@kernel.org>
Date: Tue, 21 Oct 2025 19:15:49 -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 18:28:02 -0700 Shakeel Butt <shakeel.butt@...ux.dev> wrote:
> 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@...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?
>
> There is a recent code where this is called (indirectly) from networking
> stack for MEMCG_SOCK_THROTTLED.
Thank you for enlightening me. Apparently the code is from
https://lore.kernel.org/20251016161035.86161-1-shakeel.butt@linux.dev.
>
> >
> > > +{
> > > + 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.
Again, thank you for clarifying.
Acked-by: SeongJae Park <sj@...nel.org>
Thanks,
SJ
[...]
Powered by blists - more mailing lists