On 7/10/24 7:43 AM, alexs@xxxxxxxxxx wrote:
> From: "Alex Shi (Tencent)" <alexs@xxxxxxxxxx>
>
> commit 21c690a349ba ("mm: introduce slabobj_ext to support slab object
> extensions") selected SLAB_OBJ_EXT on MEMCG just for SLAB_MATCH
> memcg_data, that included SLAB_OBJ_EXT for MEMCG. In fact, I didn't see
> the necessary to enable SLAB_OBJ_EXT for MEMCG.
>
> Let's decouple the SLAB_OBJ_EXT from MEMCG and move out
> alloc_slab_obj_exts() definition from SLAB_OBJ_EXT only. To alignment
> the alloc_slab_obj_exts() return 0 for good. change its return value to
> '-1' for always failed with !SLAB_OBJ_EXT. Now we could save unnecessary
> code from MEMCG but !SLAB_OBJ_EXT.
>
> Signed-off-by: Alex Shi (Tencent) <alexs@xxxxxxxxxx>
This seems just wrong to me. The memcg hooks for slab do use obj_ext. You
made alloc_slab_obj_exts() return -1 and that will just fail all memcg
charging (unless alloc profiling selects obj_ext). The kernel will appear to
work, but memcg charging for slab won't happen at all.
So no, it can't be decoupled for slab, only for pages/folios (patch 1).
> Cc: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
> Cc: Yoann Congal <yoann.congal@xxxxxxxx>
> Cc: Masahiro Yamada <masahiroy@xxxxxxxxxx>
> Cc: Petr Mladek <pmladek@xxxxxxxx>
> ---
> init/Kconfig | 1 -
> mm/slab.h | 6 +++---
> mm/slub.c | 6 +++---
> 3 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index 26bf8bb0a7ce..61e43ac9fe75 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -965,7 +965,6 @@ config MEMCG
> bool "Memory controller"
> select PAGE_COUNTER
> select EVENTFD
> - select SLAB_OBJ_EXT
> help
> Provides control over the memory footprint of tasks in a cgroup.
>
> diff --git a/mm/slab.h b/mm/slab.h
> index 8ffdd4f315f8..6c727ecc1068 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -559,9 +559,6 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
> return (struct slabobj_ext *)(obj_exts & ~OBJEXTS_FLAGS_MASK);
> }
>
> -int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
> - gfp_t gfp, bool new_slab);
> -
> #else /* CONFIG_SLAB_OBJ_EXT */
>
> static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
> @@ -571,6 +568,9 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
>
> #endif /* CONFIG_SLAB_OBJ_EXT */
>
> +int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
> + gfp_t gfp, bool new_slab);
> +
> static inline enum node_stat_item cache_vmstat_idx(struct kmem_cache *s)
> {
> return (s->flags & SLAB_RECLAIM_ACCOUNT) ?
> diff --git a/mm/slub.c b/mm/slub.c
> index cc11f3869cc6..f531c2d67238 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2075,10 +2075,10 @@ alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
>
> #else /* CONFIG_SLAB_OBJ_EXT */
>
> -static int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
> - gfp_t gfp, bool new_slab)
> +int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
> + gfp_t gfp, bool new_slab)
> {
> - return 0;
> + return -1;
> }
>
> static inline void free_slab_obj_exts(struct slab *slab)
