On 7/9/24 10:54 PM, Suren Baghdasaryan wrote:
> On Tue, Jul 9, 2024 at 2:17 AM Vlastimil Babka <vbabka@xxxxxxx> wrote:
>>
>>>>> Hi Suren,
>>>>>
>>>>> Thanks a lot for point out the problem!
>>>>> Now I understand why we put page/folio.memcg_data under CONFIG_SLAB_OBJ_EXT.
>>>>> We need the memcg_data member only for SLAB_MATCH(memcg_data, obj_exts), even
>>>>> no CONFIG_MEMCG, isn't it?
>>>>>
>>>>> So for the config logical and SLAB_MATCH needs, could we keep this patch
>>>>> with a fix like the following? The match only checked with both configs
>>>>> enabled.
>>>>
>>>> No. The point of that check is to enforce that slab.obj_exts always
>>>> matches page.memcg_data / folio.memcg_data (enforcing the same layout
>>>> for slab, page and folio structs). Consider your original change when
>>>> CONFIG_MEM_ALLOC_PROFILING=y && CONFIG_MEMCG=n. slab.obj_exts exists
>>>> but page.memcg_data does not, so layouts of these structs do not
>>>> match. By adding the change you are suggesting below you simply avoid
>>>> the check that warns you about this mismatch. So, you are not fixing
>>>> the problem, just removing the safety checks that would correctly
>>>> complain about it.
>>>>
>>>
>>> Hi Suren,
>>>
>>> Thanks for reply!
>>> But what's the point of 2 members match each other in different struct?
>
> "struct slab" is a "struct page" in disguise :)
>
>>> Is there any problem w/o the match checking on CONFIG_MEM_ALLOC_PROFILING=y
>>> && CONFIG_MEMCG=n?
>>>
>>> No, the kernel works well under my test with MEM_ALLOC_PROFILING/MEMCG/KASAN
>>> random combined.
>>
>> I'm guessing it works only by luck thanks to _struct_page_alignment.
>
> Agree.
>
>> Once you keep obj_exts in struct slab, but remove memcg_data in struct
>> page/folio, it could (on 64bit) result in struct page/folio being 56 bytes,
>> and struct slab being 64 bytes, and then this will fail:
>>
>> static_assert(sizeof(struct slab) <= sizeof(struct page));
>>
>> _struct_page_alignment might fix it up silently, but only with
>> CONFIG_HAVE_ALIGNED_STRUCT_PAGE.
>>
>> We could maybe make it more obvious like this? for page/folio
>>
>> #ifdef CONFIG_MEMCG
>> unsigned long memcg_data;
>> #elif defined(CONFIG_SLAB_OBJ_EXT)
>> unsigned long _unused_slab_obj_ext;
>> #endif
>>
>> And adjusting SLAB_MATCH() assertions accordingly. As a result it will stop
>> exposing the unused memcg_data field in page/folio with !CONFIG_MEMCG &&
>> CONFIG_MEM_ALLOC_PROFILING.
Hi Suren & Vlastimil,
Thanks a lot for comments! I will follow your suggestion to update the patch.
But there are another issue related to the problem, in Suren's patch, make the
MEMCG select SLAB_OBJ_EXT? why memcg needs this? I checked the mm/memcontrol.c
seems should work with !SLAB_OBJ_EXT.
Thanks
Alex
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.
