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

[Michal Hocko <mhocko@suse.com>]

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

-- 
Michal Hocko
SUSE Labs