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