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@xxxxxxxxx> wrote:
>>
>>
>>
>> On 7/11/24 4:11 PM, Vlastimil Babka wrote:
>>> 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).
>>
>> 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@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)
>>>