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

[Alex Shi <seakeel@gmail.com>]

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)
>