[PATCH v3 1/2] mm/memcg: alignment memcg_data define condition

[PATCH v3 1/2] mm/memcg: alignment memcg_data define condition

[alexs]

From: "Alex Shi (Tencent)" <alexs@kernel.org>

commit 21c690a349ba ("mm: introduce slabobj_ext to support slab object
extensions") changed the folio/page->memcg_data define condition from
MEMCG to SLAB_OBJ_EXT. And selected SLAB_OBJ_EXT for MEMCG, just for
SLAB_MATCH(memcg_data, obj_exts), even no other relationship between them.

Above action make memcg_data exposed and include SLAB_OBJ_EXT for
!MEMCG. That's incorrect in logcial and pay on code size.

As Vlastimil Babka suggested, let's add _unused_slab_obj_ext for
SLAB_MATCH for slab.obj_exts while !MEMCG. That could resolve the match
issue, clean up the feature logical. And decouple the SLAB_OBJ_EXT from
MEMCG in next patch.

Signed-off-by: Alex Shi (Tencent) <alexs@kernel.org>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Yoann Congal <yoann.congal@smile.fr>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: Petr Mladek <pmladek@suse.com>
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
---
v1->v3: take Vlastimil's suggestion and move SLAB_OBJ_EXT/MEMCG decouple
to 2nd patch.
---
 include/linux/mm_types.h | 8 ++++++--
 mm/slab.h                | 4 ++++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index ef09c4eef6d3..4ac3abc673d3 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -180,8 +180,10 @@ struct page {
 	/* Usage count. *DO NOT USE DIRECTLY*. See page_ref.h */
 	atomic_t _refcount;
 
-#ifdef CONFIG_SLAB_OBJ_EXT
+#ifdef CONFIG_MEMCG
 	unsigned long memcg_data;
+#elif defined(CONFIG_SLAB_OBJ_EXT)
+	unsigned long _unused_slab_obj_ext;
 #endif
 
 	/*
@@ -343,8 +345,10 @@ struct folio {
 			};
 			atomic_t _mapcount;
 			atomic_t _refcount;
-#ifdef CONFIG_SLAB_OBJ_EXT
+#ifdef CONFIG_MEMCG
 			unsigned long memcg_data;
+#elif defined(CONFIG_SLAB_OBJ_EXT)
+			unsigned long _unused_slab_obj_ext;
 #endif
 #if defined(WANT_PAGE_VIRTUAL)
 			void *virtual;
diff --git a/mm/slab.h b/mm/slab.h
index 3586e6183224..8ffdd4f315f8 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -98,7 +98,11 @@ SLAB_MATCH(flags, __page_flags);
 SLAB_MATCH(compound_head, slab_cache);	/* Ensure bit 0 is clear */
 SLAB_MATCH(_refcount, __page_refcount);
 #ifdef CONFIG_SLAB_OBJ_EXT
+#ifdef CONFIG_MEMCG
 SLAB_MATCH(memcg_data, obj_exts);
+#else
+SLAB_MATCH(_unused_slab_obj_ext, obj_exts);
+#endif
 #endif
 #undef SLAB_MATCH
 static_assert(sizeof(struct slab) <= sizeof(struct page));
-- 
2.43.0

[REF PATCH v3 2/2] mm/slab: decouple the SLAB_OBJ_EXT from MEMCG

[alexs]

From: "Alex Shi (Tencent)" <alexs@kernel.org>

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@kernel.org>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Yoann Congal <yoann.congal@smile.fr>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: Petr Mladek <pmladek@suse.com>
---
 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)
-- 
2.43.0

Re: [REF PATCH v3 2/2] mm/slab: decouple the SLAB_OBJ_EXT from MEMCG

[Alex Shi]

On 7/10/24 1:43 PM, alexs@kernel.org wrote:
> From: "Alex Shi (Tencent)" <alexs@kernel.org>
> 
> 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 SLAB_OBJ_EXT.
> 
> 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.

remove SLAB_OBJ_EXT from MEMCG, it introduce a new path in 
__memcg_slab_post_alloc_hook()

        for (i = 0; i < size; i++) {
                slab = virt_to_slab(p[i]);

                if (!slab_obj_exts(slab) &&
>>>                 alloc_slab_obj_exts(slab, s, flags, false)) {
                        obj_cgroup_uncharge(objcg, obj_full_size(s));
                        continue;
                }

Here alloc_slab_obj_exts() return 0 for good, that lead to 
"slab_obj_exts(slab)[off].objcg = objcg;" failed on !SLAB_OBJ_EXT.

so change the return value could fix this. and its the only new path
for this function.
Another usage of alloc_slab_obj_exts() in prepare_slab_obj_exts_hook() doesn't
exist on !SLAB_OBJ_EXT.

Thanks
Alex
> 
> Signed-off-by: Alex Shi (Tencent) <alexs@kernel.org>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Yoann Congal <yoann.congal@smile.fr>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: Petr Mladek <pmladek@suse.com>
> ---
>  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)

Re: [REF PATCH v3 2/2] mm/slab: decouple the SLAB_OBJ_EXT from MEMCG

[Vlastimil Babka]

On 7/10/24 7:43 AM, alexs@kernel.org wrote:
> From: "Alex Shi (Tencent)" <alexs@kernel.org>
> 
> 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@kernel.org>

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@infradead.org>
> Cc: Yoann Congal <yoann.congal@smile.fr>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: Petr Mladek <pmladek@suse.com>
> ---
>  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)

Re: [PATCH v3 1/2] mm/memcg: alignment memcg_data define condition

[Vlastimil Babka]

On 7/10/24 7:43 AM, alexs@kernel.org wrote:
> From: "Alex Shi (Tencent)" <alexs@kernel.org>
> 
> commit 21c690a349ba ("mm: introduce slabobj_ext to support slab object
> extensions") changed the folio/page->memcg_data define condition from
> MEMCG to SLAB_OBJ_EXT. And selected SLAB_OBJ_EXT for MEMCG, just for
> SLAB_MATCH(memcg_data, obj_exts), even no other relationship between them.
> 
> Above action make memcg_data exposed and include SLAB_OBJ_EXT for
> !MEMCG. That's incorrect in logcial and pay on code size.
> 
> As Vlastimil Babka suggested, let's add _unused_slab_obj_ext for
> SLAB_MATCH for slab.obj_exts while !MEMCG. That could resolve the match
> issue, clean up the feature logical. And decouple the SLAB_OBJ_EXT from
> MEMCG in next patch.
> 
> Signed-off-by: Alex Shi (Tencent) <alexs@kernel.org>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Yoann Congal <yoann.congal@smile.fr>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: Petr Mladek <pmladek@suse.com>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> ---
> v1->v3: take Vlastimil's suggestion and move SLAB_OBJ_EXT/MEMCG decouple
> to 2nd patch.
> ---
>  include/linux/mm_types.h | 8 ++++++--
>  mm/slab.h                | 4 ++++
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index ef09c4eef6d3..4ac3abc673d3 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -180,8 +180,10 @@ struct page {
>  	/* Usage count. *DO NOT USE DIRECTLY*. See page_ref.h */
>  	atomic_t _refcount;
>  
> -#ifdef CONFIG_SLAB_OBJ_EXT
> +#ifdef CONFIG_MEMCG
>  	unsigned long memcg_data;
> +#elif defined(CONFIG_SLAB_OBJ_EXT)
> +	unsigned long _unused_slab_obj_ext;
>  #endif
>  
>  	/*
> @@ -343,8 +345,10 @@ struct folio {
>  			};
>  			atomic_t _mapcount;
>  			atomic_t _refcount;
> -#ifdef CONFIG_SLAB_OBJ_EXT
> +#ifdef CONFIG_MEMCG
>  			unsigned long memcg_data;
> +#elif defined(CONFIG_SLAB_OBJ_EXT)
> +			unsigned long _unused_slab_obj_ext;
>  #endif
>  #if defined(WANT_PAGE_VIRTUAL)
>  			void *virtual;
> diff --git a/mm/slab.h b/mm/slab.h
> index 3586e6183224..8ffdd4f315f8 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -98,7 +98,11 @@ SLAB_MATCH(flags, __page_flags);
>  SLAB_MATCH(compound_head, slab_cache);	/* Ensure bit 0 is clear */
>  SLAB_MATCH(_refcount, __page_refcount);
>  #ifdef CONFIG_SLAB_OBJ_EXT
> +#ifdef CONFIG_MEMCG
>  SLAB_MATCH(memcg_data, obj_exts);
> +#else
> +SLAB_MATCH(_unused_slab_obj_ext, obj_exts);
> +#endif
>  #endif

Why not also #ifdef / #elif like above, instead of this nesting?

>  #undef SLAB_MATCH
>  static_assert(sizeof(struct slab) <= sizeof(struct page));

Re: [REF PATCH v3 2/2] mm/slab: decouple the SLAB_OBJ_EXT from MEMCG

[Alex Shi]

On 7/11/24 4:11 PM, Vlastimil Babka wrote:
> On 7/10/24 7:43 AM, alexs@kernel.org wrote:
>> From: "Alex Shi (Tencent)" <alexs@kernel.org>
>>
>> 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@kernel.org>
> 
> 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).

Hi Vlastimil,

Thanks a lot for clarification! Yes, the patch isn't correct.

Just forgive my stupidity, why the memcg needs SLAB_OBJ_EXT?

And why we need to alloc_slab_obj_exts() at line 3019 with !slab_obj_exts?
3015         for (i = 0; i < size; i++) {
3016                 slab = virt_to_slab(p[i]);
3017 
3018                 if (!slab_obj_exts(slab) &&
3019                     alloc_slab_obj_exts(slab, s, flags, false)) {
3020                         obj_cgroup_uncharge(objcg, obj_full_size(s));
3021                         continue;
3022                 }  

Thanks!
Alex

> 
> 
>> Cc: Randy Dunlap <rdunlap@infradead.org>
>> Cc: Yoann Congal <yoann.congal@smile.fr>
>> Cc: Masahiro Yamada <masahiroy@kernel.org>
>> Cc: Petr Mladek <pmladek@suse.com>
>> ---
>>  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)
> 

Re: [PATCH v3 1/2] mm/memcg: alignment memcg_data define condition

[Alex Shi]

On 7/11/24 4:13 PM, Vlastimil Babka wrote:
> On 7/10/24 7:43 AM, alexs@kernel.org wrote:
>> From: "Alex Shi (Tencent)" <alexs@kernel.org>
>>
>> commit 21c690a349ba ("mm: introduce slabobj_ext to support slab object
>> extensions") changed the folio/page->memcg_data define condition from
>> MEMCG to SLAB_OBJ_EXT. And selected SLAB_OBJ_EXT for MEMCG, just for
>> SLAB_MATCH(memcg_data, obj_exts), even no other relationship between them.
>>
>> Above action make memcg_data exposed and include SLAB_OBJ_EXT for
>> !MEMCG. That's incorrect in logcial and pay on code size.
>>
>> As Vlastimil Babka suggested, let's add _unused_slab_obj_ext for
>> SLAB_MATCH for slab.obj_exts while !MEMCG. That could resolve the match
>> issue, clean up the feature logical. And decouple the SLAB_OBJ_EXT from
>> MEMCG in next patch.
>>
>> Signed-off-by: Alex Shi (Tencent) <alexs@kernel.org>
>> Cc: Randy Dunlap <rdunlap@infradead.org>
>> Cc: Yoann Congal <yoann.congal@smile.fr>
>> Cc: Masahiro Yamada <masahiroy@kernel.org>
>> Cc: Petr Mladek <pmladek@suse.com>
>> Cc: Suren Baghdasaryan <surenb@google.com>
>> Cc: Vlastimil Babka <vbabka@suse.cz>
>> ---
>> v1->v3: take Vlastimil's suggestion and move SLAB_OBJ_EXT/MEMCG decouple
>> to 2nd patch.
>> ---
>>  include/linux/mm_types.h | 8 ++++++--
>>  mm/slab.h                | 4 ++++
>>  2 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
>> index ef09c4eef6d3..4ac3abc673d3 100644
>> --- a/include/linux/mm_types.h
>> +++ b/include/linux/mm_types.h
>> @@ -180,8 +180,10 @@ struct page {
>>  	/* Usage count. *DO NOT USE DIRECTLY*. See page_ref.h */
>>  	atomic_t _refcount;
>>  
>> -#ifdef CONFIG_SLAB_OBJ_EXT
>> +#ifdef CONFIG_MEMCG
>>  	unsigned long memcg_data;
>> +#elif defined(CONFIG_SLAB_OBJ_EXT)
>> +	unsigned long _unused_slab_obj_ext;
>>  #endif
>>  
>>  	/*
>> @@ -343,8 +345,10 @@ struct folio {
>>  			};
>>  			atomic_t _mapcount;
>>  			atomic_t _refcount;
>> -#ifdef CONFIG_SLAB_OBJ_EXT
>> +#ifdef CONFIG_MEMCG
>>  			unsigned long memcg_data;
>> +#elif defined(CONFIG_SLAB_OBJ_EXT)
>> +			unsigned long _unused_slab_obj_ext;
>>  #endif
>>  #if defined(WANT_PAGE_VIRTUAL)
>>  			void *virtual;
>> diff --git a/mm/slab.h b/mm/slab.h
>> index 3586e6183224..8ffdd4f315f8 100644
>> --- a/mm/slab.h
>> +++ b/mm/slab.h
>> @@ -98,7 +98,11 @@ SLAB_MATCH(flags, __page_flags);
>>  SLAB_MATCH(compound_head, slab_cache);	/* Ensure bit 0 is clear */
>>  SLAB_MATCH(_refcount, __page_refcount);
>>  #ifdef CONFIG_SLAB_OBJ_EXT
>> +#ifdef CONFIG_MEMCG
>>  SLAB_MATCH(memcg_data, obj_exts);
>> +#else
>> +SLAB_MATCH(_unused_slab_obj_ext, obj_exts);
>> +#endif
>>  #endif
> 
> Why not also #ifdef / #elif like above, instead of this nesting?

Uh, it works too if MEMCG/SLAB_OBJ_EXT decoupled.
but right, it could be written with #ifdef/#elif.

Thanks
Alex
> 
>>  #undef SLAB_MATCH
>>  static_assert(sizeof(struct slab) <= sizeof(struct page));
> 

Re: [REF PATCH v3 2/2] mm/slab: decouple the SLAB_OBJ_EXT from MEMCG

[Suren Baghdasaryan]

On Thu, Jul 11, 2024 at 4:49 AM Alex Shi <seakeel@gmail.com> wrote:
>
>
>
> On 7/11/24 4:11 PM, Vlastimil Babka wrote:
> > On 7/10/24 7:43 AM, alexs@kernel.org wrote:
> >> From: "Alex Shi (Tencent)" <alexs@kernel.org>
> >>
> >> 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@kernel.org>
> >
> > 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).
>
> Hi Vlastimil,
>
> Thanks a lot for clarification! Yes, the patch isn't correct.
>
> Just forgive my stupidity, why the memcg needs SLAB_OBJ_EXT?

Because when CONFIG_MEMCG_KMEM=y, slabobj_ext contains obj_cgroup
(see: https://elixir.bootlin.com/linux/v6.10-rc7/source/include/linux/memcontrol.h#L1593)
and that's used for memcg accounting. Look into this call chain:

kfree
  slab_free
    memcg_slab_free_hook
      __memcg_slab_free_hook
        obj_cgroup_uncharge

>
> And why we need to alloc_slab_obj_exts() at line 3019 with !slab_obj_exts?
> 3015         for (i = 0; i < size; i++) {
> 3016                 slab = virt_to_slab(p[i]);
> 3017
> 3018                 if (!slab_obj_exts(slab) &&
> 3019                     alloc_slab_obj_exts(slab, s, flags, false)) {
> 3020                         obj_cgroup_uncharge(objcg, obj_full_size(s));
> 3021                         continue;
> 3022                 }
>
> Thanks!
> Alex
>
> >
> >
> >> Cc: Randy Dunlap <rdunlap@infradead.org>
> >> Cc: Yoann Congal <yoann.congal@smile.fr>
> >> Cc: Masahiro Yamada <masahiroy@kernel.org>
> >> Cc: Petr Mladek <pmladek@suse.com>
> >> ---
> >>  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)
> >

Re: [PATCH v3 1/2] mm/memcg: alignment memcg_data define condition

[Suren Baghdasaryan]

On Thu, Jul 11, 2024 at 4:51 AM Alex Shi <seakeel@gmail.com> wrote:
>
>
>
> On 7/11/24 4:13 PM, Vlastimil Babka wrote:
> > On 7/10/24 7:43 AM, alexs@kernel.org wrote:
> >> From: "Alex Shi (Tencent)" <alexs@kernel.org>
> >>
> >> commit 21c690a349ba ("mm: introduce slabobj_ext to support slab object
> >> extensions") changed the folio/page->memcg_data define condition from
> >> MEMCG to SLAB_OBJ_EXT. And selected SLAB_OBJ_EXT for MEMCG, just for
> >> SLAB_MATCH(memcg_data, obj_exts), even no other relationship between them.
> >>
> >> Above action make memcg_data exposed and include SLAB_OBJ_EXT for
> >> !MEMCG. That's incorrect in logcial and pay on code size.
> >>
> >> As Vlastimil Babka suggested, let's add _unused_slab_obj_ext for
> >> SLAB_MATCH for slab.obj_exts while !MEMCG. That could resolve the match
> >> issue, clean up the feature logical. And decouple the SLAB_OBJ_EXT from
> >> MEMCG in next patch.
> >>
> >> Signed-off-by: Alex Shi (Tencent) <alexs@kernel.org>
> >> Cc: Randy Dunlap <rdunlap@infradead.org>
> >> Cc: Yoann Congal <yoann.congal@smile.fr>
> >> Cc: Masahiro Yamada <masahiroy@kernel.org>
> >> Cc: Petr Mladek <pmladek@suse.com>
> >> Cc: Suren Baghdasaryan <surenb@google.com>
> >> Cc: Vlastimil Babka <vbabka@suse.cz>
> >> ---
> >> v1->v3: take Vlastimil's suggestion and move SLAB_OBJ_EXT/MEMCG decouple
> >> to 2nd patch.
> >> ---
> >>  include/linux/mm_types.h | 8 ++++++--
> >>  mm/slab.h                | 4 ++++
> >>  2 files changed, 10 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> >> index ef09c4eef6d3..4ac3abc673d3 100644
> >> --- a/include/linux/mm_types.h
> >> +++ b/include/linux/mm_types.h
> >> @@ -180,8 +180,10 @@ struct page {
> >>      /* Usage count. *DO NOT USE DIRECTLY*. See page_ref.h */
> >>      atomic_t _refcount;
> >>
> >> -#ifdef CONFIG_SLAB_OBJ_EXT
> >> +#ifdef CONFIG_MEMCG
> >>      unsigned long memcg_data;
> >> +#elif defined(CONFIG_SLAB_OBJ_EXT)
> >> +    unsigned long _unused_slab_obj_ext;
> >>  #endif
> >>
> >>      /*
> >> @@ -343,8 +345,10 @@ struct folio {
> >>                      };
> >>                      atomic_t _mapcount;
> >>                      atomic_t _refcount;
> >> -#ifdef CONFIG_SLAB_OBJ_EXT
> >> +#ifdef CONFIG_MEMCG
> >>                      unsigned long memcg_data;
> >> +#elif defined(CONFIG_SLAB_OBJ_EXT)
> >> +                    unsigned long _unused_slab_obj_ext;
> >>  #endif
> >>  #if defined(WANT_PAGE_VIRTUAL)
> >>                      void *virtual;
> >> diff --git a/mm/slab.h b/mm/slab.h
> >> index 3586e6183224..8ffdd4f315f8 100644
> >> --- a/mm/slab.h
> >> +++ b/mm/slab.h
> >> @@ -98,7 +98,11 @@ SLAB_MATCH(flags, __page_flags);
> >>  SLAB_MATCH(compound_head, slab_cache);      /* Ensure bit 0 is clear */
> >>  SLAB_MATCH(_refcount, __page_refcount);
> >>  #ifdef CONFIG_SLAB_OBJ_EXT
> >> +#ifdef CONFIG_MEMCG
> >>  SLAB_MATCH(memcg_data, obj_exts);
> >> +#else
> >> +SLAB_MATCH(_unused_slab_obj_ext, obj_exts);
> >> +#endif
> >>  #endif
> >
> > Why not also #ifdef / #elif like above, instead of this nesting?
>
> Uh, it works too if MEMCG/SLAB_OBJ_EXT decoupled.
> but right, it could be written with #ifdef/#elif.

Yes, please keep the same condition, otherwise it gets confusing.

>
> Thanks
> Alex
> >
> >>  #undef SLAB_MATCH
> >>  static_assert(sizeof(struct slab) <= sizeof(struct page));
> >

Re: [REF PATCH v3 2/2] mm/slab: decouple the SLAB_OBJ_EXT from MEMCG

[Alex Shi]

On 7/11/24 9:55 PM, Suren Baghdasaryan wrote:
> On Thu, Jul 11, 2024 at 4:49 AM Alex Shi <seakeel@gmail.com> wrote:
>>
>>
>>
>> On 7/11/24 4:11 PM, Vlastimil Babka wrote:
>>> On 7/10/24 7:43 AM, alexs@kernel.org wrote:
>>>> From: "Alex Shi (Tencent)" <alexs@kernel.org>
>>>>
>>>> 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@kernel.org>
>>>
>>> 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).
>>
>> Hi Vlastimil,
>>
>> Thanks a lot for clarification! Yes, the patch isn't correct.
>>
>> Just forgive my stupidity, why the memcg needs SLAB_OBJ_EXT?
> 
> Because when CONFIG_MEMCG_KMEM=y, slabobj_ext contains obj_cgroup
> (see: https://elixir.bootlin.com/linux/v6.10-rc7/source/include/linux/memcontrol.h#L1593)

Thanks for comments. 
Yes, if the obj_cg is sth we must have in MEMCG, then MEMCG should take OBJ_EXT.

> and that's used for memcg accounting. Look into this call chain:
> 
> kfree
>   slab_free
>     memcg_slab_free_hook
>       __memcg_slab_free_hook
>         obj_cgroup_uncharge> 
>>
>> And why we need to alloc_slab_obj_exts() at line 3019 with !slab_obj_exts?

I checked the history of slab for this part. It introduced
from commit 10befea91b61c ("mm: memcg/slab: use a single set of kmem_caches for all allocations")
But still don't know why !page_has_obj_cgroups followed by memcg_alloc_page_obj_cgroups. Anyone like
to give a hints?

                        page = virt_to_head_page(p[i]);
+
+                       if (!page_has_obj_cgroups(page) &&
+                           memcg_alloc_page_obj_cgroups(page, s, flags)) {
+                               obj_cgroup_uncharge(objcg, obj_full_size(s));
+                               continue;
+                       }

Thanks a lot
Alex


>> 3015         for (i = 0; i < size; i++) {
>> 3016                 slab = virt_to_slab(p[i]);
>> 3017
>> 3018                 if (!slab_obj_exts(slab) &&
>> 3019                     alloc_slab_obj_exts(slab, s, flags, false)) {
>> 3020                         obj_cgroup_uncharge(objcg, obj_full_size(s));
>> 3021                         continue;
>> 3022                 }
>>
>> Thanks!
>> Alex
>>
>>>
>>>
>>>> Cc: Randy Dunlap <rdunlap@infradead.org>
>>>> Cc: Yoann Congal <yoann.congal@smile.fr>
>>>> Cc: Masahiro Yamada <masahiroy@kernel.org>
>>>> Cc: Petr Mladek <pmladek@suse.com>
>>>> ---
>>>>  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)
>>>

Re: [REF PATCH v3 2/2] mm/slab: decouple the SLAB_OBJ_EXT from MEMCG

[Vlastimil Babka]

On 7/12/24 6:21 AM, Alex Shi wrote:
> 
> 
> On 7/11/24 9:55 PM, Suren Baghdasaryan wrote:
>> On Thu, Jul 11, 2024 at 4:49 AM Alex Shi <seakeel@gmail.com> wrote:
>>>
>>>
>>>
>>> On 7/11/24 4:11 PM, Vlastimil Babka wrote:
>>>> On 7/10/24 7:43 AM, alexs@kernel.org wrote:
>>>>> From: "Alex Shi (Tencent)" <alexs@kernel.org>
>>>>>
>>>>> 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@kernel.org>
>>>>
>>>> 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).
>>>
>>> Hi Vlastimil,
>>>
>>> Thanks a lot for clarification! Yes, the patch isn't correct.
>>>
>>> Just forgive my stupidity, why the memcg needs SLAB_OBJ_EXT?
>> 
>> Because when CONFIG_MEMCG_KMEM=y, slabobj_ext contains obj_cgroup
>> (see: https://elixir.bootlin.com/linux/v6.10-rc7/source/include/linux/memcontrol.h#L1593)
> 
> Thanks for comments. 
> Yes, if the obj_cg is sth we must have in MEMCG, then MEMCG should take OBJ_EXT.
> 
>> and that's used for memcg accounting. Look into this call chain:
>> 
>> kfree
>>   slab_free
>>     memcg_slab_free_hook
>>       __memcg_slab_free_hook
>>         obj_cgroup_uncharge> 
>>>
>>> And why we need to alloc_slab_obj_exts() at line 3019 with !slab_obj_exts?
> 
> I checked the history of slab for this part. It introduced
> from commit 10befea91b61c ("mm: memcg/slab: use a single set of kmem_caches for all allocations")
> But still don't know why !page_has_obj_cgroups followed by memcg_alloc_page_obj_cgroups. Anyone like
> to give a hints?
> 
>                         page = virt_to_head_page(p[i]);
> +
> +                       if (!page_has_obj_cgroups(page) &&
> +                           memcg_alloc_page_obj_cgroups(page, s, flags)) {
> +                               obj_cgroup_uncharge(objcg, obj_full_size(s));
> +                               continue;
> +                       }

I'm not sure I understand your question. The code is trying to charge the
allocation to a memcg and use the objext.memcg to associate that memcg to
the object so it can be properly uncharged when freeing.
When it's the first object in the particular slab page to be charged, the
objext may not be yet allocated, so it has has to be allocated at that point.

> Thanks a lot
> Alex
> 
> 
>>> 3015         for (i = 0; i < size; i++) {
>>> 3016                 slab = virt_to_slab(p[i]);
>>> 3017
>>> 3018                 if (!slab_obj_exts(slab) &&
>>> 3019                     alloc_slab_obj_exts(slab, s, flags, false)) {
>>> 3020                         obj_cgroup_uncharge(objcg, obj_full_size(s));
>>> 3021                         continue;
>>> 3022                 }
>>>
>>> Thanks!
>>> Alex
>>>
>>>>
>>>>
>>>>> Cc: Randy Dunlap <rdunlap@infradead.org>
>>>>> Cc: Yoann Congal <yoann.congal@smile.fr>
>>>>> Cc: Masahiro Yamada <masahiroy@kernel.org>
>>>>> Cc: Petr Mladek <pmladek@suse.com>
>>>>> ---
>>>>>  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)
>>>>

Re: [REF PATCH v3 2/2] mm/slab: decouple the SLAB_OBJ_EXT from MEMCG

[Alex Shi]

On 7/12/24 3:27 PM, Vlastimil Babka wrote:
>> I checked the history of slab for this part. It introduced
>> from commit 10befea91b61c ("mm: memcg/slab: use a single set of kmem_caches for all allocations")
>> But still don't know why !page_has_obj_cgroups followed by memcg_alloc_page_obj_cgroups. Anyone like
>> to give a hints?
>>
>>                         page = virt_to_head_page(p[i]);
>> +
>> +                       if (!page_has_obj_cgroups(page) &&
>> +                           memcg_alloc_page_obj_cgroups(page, s, flags)) {
>> +                               obj_cgroup_uncharge(objcg, obj_full_size(s));
>> +                               continue;
>> +                       }
> I'm not sure I understand your question. The code is trying to charge the
> allocation to a memcg and use the objext.memcg to associate that memcg to
> the object so it can be properly uncharged when freeing.
> When it's the first object in the particular slab page to be charged, the
> objext may not be yet allocated, so it has has to be allocated at that point.

I see. Thanks for explanation!