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@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;
> +                       }

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