On Tue, Jul 9, 2024 at 7:24 PM Alex Shi <seakeel@xxxxxxxxx> wrote: > > > > 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. It would help you understand this if you think of slab.obj_exts and page.memcg_data as the same pointer with two different names. When one exists, the other should exist and vice versa, when one does not exist the other should not exist. In my patches they both depend on CONFIG_SLAB_OBJ_EXT. Now, since CONFIG_MEMCG requires page.memcg_data, it should make sure page.memcg_data exists and that's what it does by selecting CONFIG_SLAB_OBJ_EXT. The above description is how things work today. If you are changing the way Vlastimil suggested then this dependency can go away depending on how you implement it. Cheers, Suren. > > 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. >
