diff mbox series

[1/2] mm: memcontrol: remove mem_cgroup_kmem_disabled

Message ID 20220830055949.12640-2-ryncsn@gmail.com (mailing list archive)
State New
Headers show
Series mm: memcontrol: cleanup and optimize for accounting params | expand

Commit Message

Kairui Song Aug. 30, 2022, 5:59 a.m. UTC 
From: Kairui Song <kasong@tencent.com>

There are currently two helpers for checking if cgroup kmem
accounting is enabled:

- mem_cgroup_kmem_disabled
- memcg_kmem_enabled

mem_cgroup_kmem_disabled is a simple helper that returns true if
cgroup.memory=nokmem is specified, otherwise returns false.

memcg_kmem_enabled is a bit different, it returns true if
cgroup.memory=nokmem is not specified and there is at least one
non-root cgroup ever created. And once there is any non-root memcg
created, it won't go back to return false again.

This may help improve performance for some corner use cases where
the user enables memory cgroup and kmem accounting globally but never
create any cgroup.

Considering that corner case is rare, especially nowadays cgroup is
widely used as a standard way to organize services. And the "once
enabled never disable" behavior is kind of strange. This commit simplifies
the behavior of memcg_kmem_enabled, making it simply the opposite of
mem_cgroup_kmem_disabled, always true if cgroup.memory=nokmem is
not specified. So mem_cgroup_kmem_disabled can be dropped.

This simplifies the code, and besides, memcg_kmem_enabled makes use
of static key so it has a lower overhead.

Signed-off-by: Kairui Song <kasong@tencent.com>
---
 include/linux/memcontrol.h |  8 +-------
 mm/memcontrol.c            | 17 +++++++----------
 mm/percpu.c                |  2 +-
 mm/slab_common.c           |  2 +-
 4 files changed, 10 insertions(+), 19 deletions(-)

Comments

Michal Hocko Aug. 30, 2022, 6:44 a.m. UTC | #1 
On Tue 30-08-22 13:59:48, Kairui Song wrote:
> From: Kairui Song <kasong@tencent.com>
> 
> There are currently two helpers for checking if cgroup kmem
> accounting is enabled:
> 
> - mem_cgroup_kmem_disabled
> - memcg_kmem_enabled

Yes, this is a bit confusing indeed!

> mem_cgroup_kmem_disabled is a simple helper that returns true if
> cgroup.memory=nokmem is specified, otherwise returns false.
> 
> memcg_kmem_enabled is a bit different, it returns true if
> cgroup.memory=nokmem is not specified and there is at least one
> non-root cgroup ever created. And once there is any non-root memcg
> created, it won't go back to return false again.
> 
> This may help improve performance for some corner use cases where
> the user enables memory cgroup and kmem accounting globally but never
> create any cgroup.
> 
> Considering that corner case is rare, especially nowadays cgroup is
> widely used as a standard way to organize services.

Is it really that rare? Most configurations would use a default setup, so
both MEMCG enabled and without nokmem on cmd line yet the memory
controller is not enabled in their setups.

> And the "once
> enabled never disable" behavior is kind of strange. This commit simplifies
> the behavior of memcg_kmem_enabled, making it simply the opposite of
> mem_cgroup_kmem_disabled, always true if cgroup.memory=nokmem is
> not specified. So mem_cgroup_kmem_disabled can be dropped.
> 
> This simplifies the code, and besides, memcg_kmem_enabled makes use
> of static key so it has a lower overhead.

I agree that this is slightly confusing and undocumented. The first step
would be finding out why we need both outside of the memcg proper.

E.g. it doesn't make much sense to me that count_objcg_event uses the
command line variant when it should be using the dynamic (and more
optimized no branch) variant.

On the other hand pcpu_alloc_chunk seems to be different because it can
be called before the controller is enabled but maybe we do not need to
waste memory before that? Similarly new_kmalloc_cache. I suspect these
are mostly to simplify the code and reduce special casing.
> 
> Signed-off-by: Kairui Song <kasong@tencent.com>
> ---
>  include/linux/memcontrol.h |  8 +-------
>  mm/memcontrol.c            | 17 +++++++----------
>  mm/percpu.c                |  2 +-
>  mm/slab_common.c           |  2 +-
>  4 files changed, 10 insertions(+), 19 deletions(-)

I do not think that saving 9 LOC and sacrifice optimization that might
be useful is a good justification.
Kairui Song Aug. 30, 2022, 7:06 a.m. UTC | #2 
Michal Hocko <mhocko@suse.com> 于2022年8月30日周二 14:45写道：
>
> On Tue 30-08-22 13:59:48, Kairui Song wrote:
> > From: Kairui Song <kasong@tencent.com>
> >
> > There are currently two helpers for checking if cgroup kmem
> > accounting is enabled:
> >
> > - mem_cgroup_kmem_disabled
> > - memcg_kmem_enabled
>
> Yes, this is a bit confusing indeed!
>
> > mem_cgroup_kmem_disabled is a simple helper that returns true if
> > cgroup.memory=nokmem is specified, otherwise returns false.
> >
> > memcg_kmem_enabled is a bit different, it returns true if
> > cgroup.memory=nokmem is not specified and there is at least one
> > non-root cgroup ever created. And once there is any non-root memcg
> > created, it won't go back to return false again.
> >
> > This may help improve performance for some corner use cases where
> > the user enables memory cgroup and kmem accounting globally but never
> > create any cgroup.
> >
> > Considering that corner case is rare, especially nowadays cgroup is
> > widely used as a standard way to organize services.
>
> Is it really that rare? Most configurations would use a default setup, so
> both MEMCG enabled and without nokmem on cmd line yet the memory
> controller is not enabled in their setups.

Actually I don't have too much confidence saying that as well... but
AFAIK, almost all distros will create a few sub cgroup on boot by the
init (eg. openrc, finit, systemd).
Maybe it's not that rare indeed.

>
> > And the "once
> > enabled never disable" behavior is kind of strange. This commit simplifies
> > the behavior of memcg_kmem_enabled, making it simply the opposite of
> > mem_cgroup_kmem_disabled, always true if cgroup.memory=nokmem is
> > not specified. So mem_cgroup_kmem_disabled can be dropped.
> >
> > This simplifies the code, and besides, memcg_kmem_enabled makes use
> > of static key so it has a lower overhead.
>
> I agree that this is slightly confusing and undocumented. The first step
> would be finding out why we need both outside of the memcg proper.
>
> E.g. it doesn't make much sense to me that count_objcg_event uses the
> command line variant when it should be using the dynamic (and more
> optimized no branch) variant.
>
> On the other hand pcpu_alloc_chunk seems to be different because it can
> be called before the controller is enabled but maybe we do not need to
> waste memory before that? Similarly new_kmalloc_cache. I suspect these
> are mostly to simplify the code and reduce special casing.

Yes, that's very insightful, let me tidy up the code and logic behind
and send a V2 later.
Michal Hocko Aug. 30, 2022, 7:12 a.m. UTC | #3 
On Tue 30-08-22 15:06:57, Kairui Song wrote:
> Michal Hocko <mhocko@suse.com> 于2022年8月30日周二 14:45写道：
> >
> > On Tue 30-08-22 13:59:48, Kairui Song wrote:
> > > From: Kairui Song <kasong@tencent.com>
> > >
> > > There are currently two helpers for checking if cgroup kmem
> > > accounting is enabled:
> > >
> > > - mem_cgroup_kmem_disabled
> > > - memcg_kmem_enabled
> >
> > Yes, this is a bit confusing indeed!
> >
> > > mem_cgroup_kmem_disabled is a simple helper that returns true if
> > > cgroup.memory=nokmem is specified, otherwise returns false.
> > >
> > > memcg_kmem_enabled is a bit different, it returns true if
> > > cgroup.memory=nokmem is not specified and there is at least one
> > > non-root cgroup ever created. And once there is any non-root memcg
> > > created, it won't go back to return false again.
> > >
> > > This may help improve performance for some corner use cases where
> > > the user enables memory cgroup and kmem accounting globally but never
> > > create any cgroup.
> > >
> > > Considering that corner case is rare, especially nowadays cgroup is
> > > widely used as a standard way to organize services.
> >
> > Is it really that rare? Most configurations would use a default setup, so
> > both MEMCG enabled and without nokmem on cmd line yet the memory
> > controller is not enabled in their setups.
> 
> Actually I don't have too much confidence saying that as well... but
> AFAIK, almost all distros will create a few sub cgroup on boot by the
> init (eg. openrc, finit, systemd).

Yeah, but do they enable the memory controller as well? Unless I am
missing something this will require at least one memcg enabled cgroup to
be created.
Kairui Song Aug. 30, 2022, 7:45 a.m. UTC | #4 
Michal Hocko <mhocko@suse.com> 于2022年8月30日周二 15:12写道：
>
> On Tue 30-08-22 15:06:57, Kairui Song wrote:
> > Michal Hocko <mhocko@suse.com> 于2022年8月30日周二 14:45写道：
> > >
> > > On Tue 30-08-22 13:59:48, Kairui Song wrote:
> > > > From: Kairui Song <kasong@tencent.com>
> > > >
> > > > There are currently two helpers for checking if cgroup kmem
> > > > accounting is enabled:
> > > >
> > > > - mem_cgroup_kmem_disabled
> > > > - memcg_kmem_enabled
> > >
> > > Yes, this is a bit confusing indeed!
> > >
> > > > mem_cgroup_kmem_disabled is a simple helper that returns true if
> > > > cgroup.memory=nokmem is specified, otherwise returns false.
> > > >
> > > > memcg_kmem_enabled is a bit different, it returns true if
> > > > cgroup.memory=nokmem is not specified and there is at least one
> > > > non-root cgroup ever created. And once there is any non-root memcg
> > > > created, it won't go back to return false again.
> > > >
> > > > This may help improve performance for some corner use cases where
> > > > the user enables memory cgroup and kmem accounting globally but never
> > > > create any cgroup.
> > > >
> > > > Considering that corner case is rare, especially nowadays cgroup is
> > > > widely used as a standard way to organize services.
> > >
> > > Is it really that rare? Most configurations would use a default setup, so
> > > both MEMCG enabled and without nokmem on cmd line yet the memory
> > > controller is not enabled in their setups.
> >
> > Actually I don't have too much confidence saying that as well... but
> > AFAIK, almost all distros will create a few sub cgroup on boot by the
> > init (eg. openrc, finit, systemd).
>
> Yeah, but do they enable the memory controller as well? Unless I am
> missing something this will require at least one memcg enabled cgroup to
> be created.

Systemd enable memory controller by default since ver 238 from 2018,
but I'm not sure about the others.
Now I think I was wrong about the assumption, will be sure to do more
homework next time.

And thanks for the review!
kernel test robot Aug. 30, 2022, 6:03 p.m. UTC | #5 
Hi Kairui,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Kairui-Song/mm-memcontrol-cleanup-and-optimize-for-accounting-params/20220830-140150
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
config: x86_64-randconfig-a004
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/45d0812323db1fbf1751cbd9d112f72f151ca3c6
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Kairui-Song/mm-memcontrol-cleanup-and-optimize-for-accounting-params/20220830-140150
        git checkout 45d0812323db1fbf1751cbd9d112f72f151ca3c6
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/static_key.h:1,
                    from arch/x86/include/asm/nospec-branch.h:6,
                    from arch/x86/include/asm/irqflags.h:9,
                    from include/linux/irqflags.h:16,
                    from include/linux/rcupdate.h:26,
                    from include/linux/rculist.h:11,
                    from include/linux/pid.h:5,
                    from include/linux/sched.h:14,
                    from include/linux/cgroup.h:12,
                    from include/linux/memcontrol.h:13,
                    from mm/memcontrol.c:29:
   mm/memcontrol.c: In function 'cgroup_memory':
>> mm/memcontrol.c:7182:39: error: 'memcg_kmem_enabled_key' undeclared (first use in this function); did you mean 'memcg_kmem_enabled'?
    7182 |                 static_branch_enable(&memcg_kmem_enabled_key);
         |                                       ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/jump_label.h:517:69: note: in definition of macro 'static_branch_enable'
     517 | #define static_branch_enable(x)                 static_key_enable(&(x)->key)
         |                                                                     ^
   mm/memcontrol.c:7182:39: note: each undeclared identifier is reported only once for each function it appears in
    7182 |                 static_branch_enable(&memcg_kmem_enabled_key);
         |                                       ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/jump_label.h:517:69: note: in definition of macro 'static_branch_enable'
     517 | #define static_branch_enable(x)                 static_key_enable(&(x)->key)
         |                                                                     ^


vim +7182 mm/memcontrol.c

  7167	
  7168	static int __init cgroup_memory(char *s)
  7169	{
  7170		char *token;
  7171	
  7172		while ((token = strsep(&s, ",")) != NULL) {
  7173			if (!*token)
  7174				continue;
  7175			if (!strcmp(token, "nosocket"))
  7176				cgroup_memory_nosocket = true;
  7177			if (!strcmp(token, "nokmem"))
  7178				cgroup_memory_nokmem = true;
  7179		}
  7180	
  7181		if (!cgroup_memory_nokmem)
> 7182			static_branch_enable(&memcg_kmem_enabled_key);
  7183	
  7184		return 1;
  7185	}
  7186	__setup("cgroup.memory=", cgroup_memory);
  7187
diff mbox series

Patch

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 6257867fbf953..9c08464ed6b46 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -1731,7 +1731,6 @@  static inline void set_shrinker_bit(struct mem_cgroup *memcg,
 #endif
 
 #ifdef CONFIG_MEMCG_KMEM
-bool mem_cgroup_kmem_disabled(void);
 int __memcg_kmem_charge_page(struct page *page, gfp_t gfp, int order);
 void __memcg_kmem_uncharge_page(struct page *page, int order);
 
@@ -1779,7 +1778,7 @@  static inline void count_objcg_event(struct obj_cgroup *objcg,
 {
 	struct mem_cgroup *memcg;
 
-	if (mem_cgroup_kmem_disabled())
+	if (!memcg_kmem_enabled())
 		return;
 
 	rcu_read_lock();
@@ -1825,11 +1824,6 @@  static inline struct mem_cgroup *mem_cgroup_or_root(struct mem_cgroup *memcg)
 	return memcg ? memcg : root_mem_cgroup;
 }
 #else
-static inline bool mem_cgroup_kmem_disabled(void)
-{
-	return true;
-}
-
 static inline int memcg_kmem_charge_page(struct page *page, gfp_t gfp,
 					 int order)
 {
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b69979c9ced5c..20e26ccd7dddc 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -86,7 +86,7 @@  EXPORT_PER_CPU_SYMBOL_GPL(int_active_memcg);
 static bool cgroup_memory_nosocket __ro_after_init;
 
 /* Kernel memory accounting disabled? */
-static bool cgroup_memory_nokmem __ro_after_init;
+static bool cgroup_memory_nokmem __initdata;
 
 /* Whether the swap controller is active */
 #ifdef CONFIG_MEMCG_SWAP
@@ -255,11 +255,6 @@  struct mem_cgroup *vmpressure_to_memcg(struct vmpressure *vmpr)
 #ifdef CONFIG_MEMCG_KMEM
 static DEFINE_SPINLOCK(objcg_lock);
 
-bool mem_cgroup_kmem_disabled(void)
-{
-	return cgroup_memory_nokmem;
-}
-
 static void obj_cgroup_uncharge_pages(struct obj_cgroup *objcg,
 				      unsigned int nr_pages);
 
@@ -3667,7 +3662,7 @@  static int memcg_online_kmem(struct mem_cgroup *memcg)
 {
 	struct obj_cgroup *objcg;
 
-	if (mem_cgroup_kmem_disabled())
+	if (!memcg_kmem_enabled())
 		return 0;
 
 	if (unlikely(mem_cgroup_is_root(memcg)))
@@ -3680,8 +3675,6 @@  static int memcg_online_kmem(struct mem_cgroup *memcg)
 	objcg->memcg = memcg;
 	rcu_assign_pointer(memcg->objcg, objcg);
 
-	static_branch_enable(&memcg_kmem_enabled_key);
-
 	memcg->kmemcg_id = memcg->id.id;
 
 	return 0;
@@ -3691,7 +3684,7 @@  static void memcg_offline_kmem(struct mem_cgroup *memcg)
 {
 	struct mem_cgroup *parent;
 
-	if (mem_cgroup_kmem_disabled())
+	if (!memcg_kmem_enabled())
 		return;
 
 	if (unlikely(mem_cgroup_is_root(memcg)))
@@ -7153,6 +7146,10 @@  static int __init cgroup_memory(char *s)
 		if (!strcmp(token, "nokmem"))
 			cgroup_memory_nokmem = true;
 	}
+
+	if (!cgroup_memory_nokmem)
+		static_branch_enable(&memcg_kmem_enabled_key);
+
 	return 1;
 }
 __setup("cgroup.memory=", cgroup_memory);
diff --git a/mm/percpu.c b/mm/percpu.c
index 27697b2429c2e..c62d6e98f7d20 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -1467,7 +1467,7 @@  static struct pcpu_chunk *pcpu_alloc_chunk(gfp_t gfp)
 		goto md_blocks_fail;
 
 #ifdef CONFIG_MEMCG_KMEM
-	if (!mem_cgroup_kmem_disabled()) {
+	if (memcg_kmem_enabled()) {
 		chunk->obj_cgroups =
 			pcpu_mem_zalloc(pcpu_chunk_map_bits(chunk) *
 					sizeof(struct obj_cgroup *), gfp);
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 17996649cfe3e..bbdc0fe3c5e34 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -829,7 +829,7 @@  new_kmalloc_cache(int idx, enum kmalloc_cache_type type, slab_flags_t flags)
 	if (type == KMALLOC_RECLAIM) {
 		flags |= SLAB_RECLAIM_ACCOUNT;
 	} else if (IS_ENABLED(CONFIG_MEMCG_KMEM) && (type == KMALLOC_CGROUP)) {
-		if (mem_cgroup_kmem_disabled()) {
+		if (!memcg_kmem_enabled()) {
 			kmalloc_caches[type][idx] = kmalloc_caches[KMALLOC_NORMAL][idx];
 			return;
 		}