diff mbox series

[v2,13/14] mm: memcg: put cgroup v1-related members of task_struct under config option

Message ID 20240625005906.106920-14-roman.gushchin@linux.dev (mailing list archive)
State New
Headers show
Series mm: memcg: separate legacy cgroup v1 code and put under config option | expand

Commit Message

Roman Gushchin June 25, 2024, 12:59 a.m. UTC 
Guard cgroup v1-related members of task_struct under the CONFIG_MEMCG_V1
config option, so that users who adopted cgroup v2 don't have to waste
the memory for fields which are never accessed.

Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
---
 include/linux/memcontrol.h |  6 +++---
 init/Kconfig               |  9 +++++++++
 mm/Makefile                |  3 ++-
 mm/memcontrol-v1.h         | 21 ++++++++++++++++++++-
 mm/memcontrol.c            | 10 +++++++---
 5 files changed, 41 insertions(+), 8 deletions(-)

Comments

Michal Hocko June 25, 2024, 7:19 a.m. UTC | #1 
On Mon 24-06-24 17:59:05, Roman Gushchin wrote:
> Guard cgroup v1-related members of task_struct under the CONFIG_MEMCG_V1
> config option, so that users who adopted cgroup v2 don't have to waste
> the memory for fields which are never accessed.

This patch does more than that, right? It is essentially making the
whole v1 code conditional. Please change the wording accordingly.

I also think we should make it more clear when to enable the option. I
would propose the following for the config option help text:

Legacy cgroup v1 memory controller which has been deprecated by cgroup
v2 implementation. The v1 is there for legacy applications which haven't
migrated to the new cgroup v2 interface yet. If you do not have any such
application then you are completely fine leaving this option disabled.

Please note that feature set of the legacy memory controller is likely
going to shrink due to deprecation process. New deployments with v1
controller are highly discouraged.

> Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>

With that updated feel free to add
Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  include/linux/memcontrol.h |  6 +++---
>  init/Kconfig               |  9 +++++++++
>  mm/Makefile                |  3 ++-
>  mm/memcontrol-v1.h         | 21 ++++++++++++++++++++-
>  mm/memcontrol.c            | 10 +++++++---
>  5 files changed, 41 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index a70d64ed04f5..796cfa842346 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -1851,7 +1851,7 @@ static inline bool mem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg)
>  
>  /* Cgroup v1-related declarations */
>  
> -#ifdef CONFIG_MEMCG
> +#ifdef CONFIG_MEMCG_V1
>  unsigned long memcg1_soft_limit_reclaim(pg_data_t *pgdat, int order,
>  					gfp_t gfp_mask,
>  					unsigned long *total_scanned);
> @@ -1883,7 +1883,7 @@ static inline void mem_cgroup_unlock_pages(void)
>  	rcu_read_unlock();
>  }
>  
> -#else /* CONFIG_MEMCG */
> +#else /* CONFIG_MEMCG_V1 */
>  static inline
>  unsigned long memcg1_soft_limit_reclaim(pg_data_t *pgdat, int order,
>  					gfp_t gfp_mask,
> @@ -1922,6 +1922,6 @@ static inline bool mem_cgroup_oom_synchronize(bool wait)
>  	return false;
>  }
>  
> -#endif /* CONFIG_MEMCG */
> +#endif /* CONFIG_MEMCG_V1 */
>  
>  #endif /* _LINUX_MEMCONTROL_H */
> diff --git a/init/Kconfig b/init/Kconfig
> index febdea2afc3b..5191b6435b4e 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -969,6 +969,15 @@ config MEMCG
>  	help
>  	  Provides control over the memory footprint of tasks in a cgroup.
>  
> +config MEMCG_V1
> +	bool "Legacy memory controller"
> +	depends on MEMCG
> +	default n
> +	help
> +	  Legacy cgroup v1 memory controller.
> +
> +	  San N is unsure.
> +
>  config MEMCG_KMEM
>  	bool
>  	depends on MEMCG
> diff --git a/mm/Makefile b/mm/Makefile
> index 124d4dea2035..d2915f8c9dc0 100644
> --- a/mm/Makefile
> +++ b/mm/Makefile
> @@ -96,7 +96,8 @@ obj-$(CONFIG_NUMA) += memory-tiers.o
>  obj-$(CONFIG_DEVICE_MIGRATION) += migrate_device.o
>  obj-$(CONFIG_TRANSPARENT_HUGEPAGE) += huge_memory.o khugepaged.o
>  obj-$(CONFIG_PAGE_COUNTER) += page_counter.o
> -obj-$(CONFIG_MEMCG) += memcontrol.o memcontrol-v1.o vmpressure.o
> +obj-$(CONFIG_MEMCG_V1) += memcontrol-v1.o
> +obj-$(CONFIG_MEMCG) += memcontrol.o vmpressure.o
>  ifdef CONFIG_SWAP
>  obj-$(CONFIG_MEMCG) += swap_cgroup.o
>  endif
> diff --git a/mm/memcontrol-v1.h b/mm/memcontrol-v1.h
> index 89d420793048..64b053d7f131 100644
> --- a/mm/memcontrol-v1.h
> +++ b/mm/memcontrol-v1.h
> @@ -75,7 +75,7 @@ unsigned long memcg_page_state_local_output(struct mem_cgroup *memcg, int item);
>  int memory_stat_show(struct seq_file *m, void *v);
>  
>  /* Cgroup v1-specific declarations */
> -
> +#ifdef CONFIG_MEMCG_V1
>  void memcg1_remove_from_trees(struct mem_cgroup *memcg);
>  
>  static inline void memcg1_soft_limit_reset(struct mem_cgroup *memcg)
> @@ -110,4 +110,23 @@ void memcg1_stat_format(struct mem_cgroup *memcg, struct seq_buf *s);
>  extern struct cftype memsw_files[];
>  extern struct cftype mem_cgroup_legacy_files[];
>  
> +#else	/* CONFIG_MEMCG_V1 */
> +
> +static inline void memcg1_remove_from_trees(struct mem_cgroup *memcg) {}
> +static inline void memcg1_soft_limit_reset(struct mem_cgroup *memcg) {}
> +static inline bool memcg1_wait_acct_move(struct mem_cgroup *memcg) { return false; }
> +static inline void memcg1_css_offline(struct mem_cgroup *memcg) {}
> +
> +static inline bool memcg1_oom_prepare(struct mem_cgroup *memcg, bool *locked) { return true; }
> +static inline void memcg1_oom_finish(struct mem_cgroup *memcg, bool locked) {}
> +static inline void memcg1_oom_recover(struct mem_cgroup *memcg) {}
> +
> +static inline void memcg1_check_events(struct mem_cgroup *memcg, int nid) {}
> +
> +static inline void memcg1_stat_format(struct mem_cgroup *memcg, struct seq_buf *s) {}
> +
> +extern struct cftype memsw_files[];
> +extern struct cftype mem_cgroup_legacy_files[];
> +#endif	/* CONFIG_MEMCG_V1 */
> +
>  #endif	/* __MM_MEMCONTROL_V1_H */
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index c7341e811945..d2e1f8baeae8 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4471,18 +4471,20 @@ struct cgroup_subsys memory_cgrp_subsys = {
>  	.css_free = mem_cgroup_css_free,
>  	.css_reset = mem_cgroup_css_reset,
>  	.css_rstat_flush = mem_cgroup_css_rstat_flush,
> -	.can_attach = memcg1_can_attach,
>  #if defined(CONFIG_LRU_GEN) || defined(CONFIG_MEMCG_KMEM)
>  	.attach = mem_cgroup_attach,
>  #endif
> -	.cancel_attach = memcg1_cancel_attach,
> -	.post_attach = memcg1_move_task,
>  #ifdef CONFIG_MEMCG_KMEM
>  	.fork = mem_cgroup_fork,
>  	.exit = mem_cgroup_exit,
>  #endif
>  	.dfl_cftypes = memory_files,
> +#ifdef CONFIG_MEMCG_V1
> +	.can_attach = memcg1_can_attach,
> +	.cancel_attach = memcg1_cancel_attach,
> +	.post_attach = memcg1_move_task,
>  	.legacy_cftypes = mem_cgroup_legacy_files,
> +#endif
>  	.early_init = 0,
>  };
>  
> @@ -5653,7 +5655,9 @@ static int __init mem_cgroup_swap_init(void)
>  		return 0;
>  
>  	WARN_ON(cgroup_add_dfl_cftypes(&memory_cgrp_subsys, swap_files));
> +#ifdef CONFIG_MEMCG_V1
>  	WARN_ON(cgroup_add_legacy_cftypes(&memory_cgrp_subsys, memsw_files));
> +#endif
>  #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP)
>  	WARN_ON(cgroup_add_dfl_cftypes(&memory_cgrp_subsys, zswap_files));
>  #endif
> -- 
> 2.45.2
Roman Gushchin June 26, 2024, 6:06 p.m. UTC | #2 
On Tue, Jun 25, 2024 at 09:19:04AM +0200, Michal Hocko wrote:
> On Mon 24-06-24 17:59:05, Roman Gushchin wrote:
> > Guard cgroup v1-related members of task_struct under the CONFIG_MEMCG_V1
> > config option, so that users who adopted cgroup v2 don't have to waste
> > the memory for fields which are never accessed.
> 
> This patch does more than that, right? It is essentially making the
> whole v1 code conditional. Please change the wording accordingly.

More than that, it doesn't do this at all. This commit message was taken
from another patch in v1 of this series by a mistake.
> 
> I also think we should make it more clear when to enable the option. I
> would propose the following for the config option help text:
> 
> Legacy cgroup v1 memory controller which has been deprecated by cgroup
> v2 implementation. The v1 is there for legacy applications which haven't
> migrated to the new cgroup v2 interface yet. If you do not have any such
> application then you are completely fine leaving this option disabled.
> 
> Please note that feature set of the legacy memory controller is likely
> going to shrink due to deprecation process. New deployments with v1
> controller are highly discouraged.
> 
> > Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
> 
> With that updated feel free to add
> Acked-by: Michal Hocko <mhocko@suse.com>

An updated version with a correct commit subject and description and your config
option description is sent to Andrew, you're cc'ed.

Thank you for suggesting the config option description and reviewing the series,
appreciate it!

Thanks,
Roman
diff mbox series

Patch

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index a70d64ed04f5..796cfa842346 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -1851,7 +1851,7 @@  static inline bool mem_cgroup_zswap_writeback_enabled(struct mem_cgroup *memcg)
 
 /* Cgroup v1-related declarations */
 
-#ifdef CONFIG_MEMCG
+#ifdef CONFIG_MEMCG_V1
 unsigned long memcg1_soft_limit_reclaim(pg_data_t *pgdat, int order,
 					gfp_t gfp_mask,
 					unsigned long *total_scanned);
@@ -1883,7 +1883,7 @@  static inline void mem_cgroup_unlock_pages(void)
 	rcu_read_unlock();
 }
 
-#else /* CONFIG_MEMCG */
+#else /* CONFIG_MEMCG_V1 */
 static inline
 unsigned long memcg1_soft_limit_reclaim(pg_data_t *pgdat, int order,
 					gfp_t gfp_mask,
@@ -1922,6 +1922,6 @@  static inline bool mem_cgroup_oom_synchronize(bool wait)
 	return false;
 }
 
-#endif /* CONFIG_MEMCG */
+#endif /* CONFIG_MEMCG_V1 */
 
 #endif /* _LINUX_MEMCONTROL_H */
diff --git a/init/Kconfig b/init/Kconfig
index febdea2afc3b..5191b6435b4e 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -969,6 +969,15 @@  config MEMCG
 	help
 	  Provides control over the memory footprint of tasks in a cgroup.
 
+config MEMCG_V1
+	bool "Legacy memory controller"
+	depends on MEMCG
+	default n
+	help
+	  Legacy cgroup v1 memory controller.
+
+	  San N is unsure.
+
 config MEMCG_KMEM
 	bool
 	depends on MEMCG
diff --git a/mm/Makefile b/mm/Makefile
index 124d4dea2035..d2915f8c9dc0 100644
--- a/mm/Makefile
+++ b/mm/Makefile
@@ -96,7 +96,8 @@  obj-$(CONFIG_NUMA) += memory-tiers.o
 obj-$(CONFIG_DEVICE_MIGRATION) += migrate_device.o
 obj-$(CONFIG_TRANSPARENT_HUGEPAGE) += huge_memory.o khugepaged.o
 obj-$(CONFIG_PAGE_COUNTER) += page_counter.o
-obj-$(CONFIG_MEMCG) += memcontrol.o memcontrol-v1.o vmpressure.o
+obj-$(CONFIG_MEMCG_V1) += memcontrol-v1.o
+obj-$(CONFIG_MEMCG) += memcontrol.o vmpressure.o
 ifdef CONFIG_SWAP
 obj-$(CONFIG_MEMCG) += swap_cgroup.o
 endif
diff --git a/mm/memcontrol-v1.h b/mm/memcontrol-v1.h
index 89d420793048..64b053d7f131 100644
--- a/mm/memcontrol-v1.h
+++ b/mm/memcontrol-v1.h
@@ -75,7 +75,7 @@  unsigned long memcg_page_state_local_output(struct mem_cgroup *memcg, int item);
 int memory_stat_show(struct seq_file *m, void *v);
 
 /* Cgroup v1-specific declarations */
-
+#ifdef CONFIG_MEMCG_V1
 void memcg1_remove_from_trees(struct mem_cgroup *memcg);
 
 static inline void memcg1_soft_limit_reset(struct mem_cgroup *memcg)
@@ -110,4 +110,23 @@  void memcg1_stat_format(struct mem_cgroup *memcg, struct seq_buf *s);
 extern struct cftype memsw_files[];
 extern struct cftype mem_cgroup_legacy_files[];
 
+#else	/* CONFIG_MEMCG_V1 */
+
+static inline void memcg1_remove_from_trees(struct mem_cgroup *memcg) {}
+static inline void memcg1_soft_limit_reset(struct mem_cgroup *memcg) {}
+static inline bool memcg1_wait_acct_move(struct mem_cgroup *memcg) { return false; }
+static inline void memcg1_css_offline(struct mem_cgroup *memcg) {}
+
+static inline bool memcg1_oom_prepare(struct mem_cgroup *memcg, bool *locked) { return true; }
+static inline void memcg1_oom_finish(struct mem_cgroup *memcg, bool locked) {}
+static inline void memcg1_oom_recover(struct mem_cgroup *memcg) {}
+
+static inline void memcg1_check_events(struct mem_cgroup *memcg, int nid) {}
+
+static inline void memcg1_stat_format(struct mem_cgroup *memcg, struct seq_buf *s) {}
+
+extern struct cftype memsw_files[];
+extern struct cftype mem_cgroup_legacy_files[];
+#endif	/* CONFIG_MEMCG_V1 */
+
 #endif	/* __MM_MEMCONTROL_V1_H */
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index c7341e811945..d2e1f8baeae8 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4471,18 +4471,20 @@  struct cgroup_subsys memory_cgrp_subsys = {
 	.css_free = mem_cgroup_css_free,
 	.css_reset = mem_cgroup_css_reset,
 	.css_rstat_flush = mem_cgroup_css_rstat_flush,
-	.can_attach = memcg1_can_attach,
 #if defined(CONFIG_LRU_GEN) || defined(CONFIG_MEMCG_KMEM)
 	.attach = mem_cgroup_attach,
 #endif
-	.cancel_attach = memcg1_cancel_attach,
-	.post_attach = memcg1_move_task,
 #ifdef CONFIG_MEMCG_KMEM
 	.fork = mem_cgroup_fork,
 	.exit = mem_cgroup_exit,
 #endif
 	.dfl_cftypes = memory_files,
+#ifdef CONFIG_MEMCG_V1
+	.can_attach = memcg1_can_attach,
+	.cancel_attach = memcg1_cancel_attach,
+	.post_attach = memcg1_move_task,
 	.legacy_cftypes = mem_cgroup_legacy_files,
+#endif
 	.early_init = 0,
 };
 
@@ -5653,7 +5655,9 @@  static int __init mem_cgroup_swap_init(void)
 		return 0;
 
 	WARN_ON(cgroup_add_dfl_cftypes(&memory_cgrp_subsys, swap_files));
+#ifdef CONFIG_MEMCG_V1
 	WARN_ON(cgroup_add_legacy_cftypes(&memory_cgrp_subsys, memsw_files));
+#endif
 #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP)
 	WARN_ON(cgroup_add_dfl_cftypes(&memory_cgrp_subsys, zswap_files));
 #endif