On Tue, Jul 9, 2024 at 2:17 AM Vlastimil Babka <vbabka@xxxxxxx> wrote: > > On 7/9/24 10:19 AM, Alex Shi wrote: > > > > > > On 7/9/24 12:34 PM, Suren Baghdasaryan wrote: > >> On Tue, Jul 9, 2024 at 1:53 AM Alex Shi <seakeel@xxxxxxxxx> wrote: > >>> > >>> > >>> > >>> On 7/9/24 9:14 AM, Suren Baghdasaryan wrote: > >>>> On Mon, Jul 8, 2024 at 2:42 PM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote: > >>>>> > >>>>> > >>>>> The patch titled > >>>>> Subject: mm/memcg: alignment memcg_data define condition > >>>>> has been added to the -mm mm-unstable branch. Its filename is > >>>>> mm-memcg-alignment-memcg_data-define-condition.patch > >>>> > >>>> FYI, just posted my objection to this change at > >>>> https://lore.kernel.org/all/CAJuCfpEjB8wvodBDB__yR8pF5F3uGMPxue-tap68RYCO0O-K1Q@xxxxxxxxxxxxxx/ > >>>> It will break CONFIG_MEM_ALLOC_PROFILING=y && CONFIG_MEMCG=n case. > >>> > >>> 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. Yes, that would be a valid solution but with required changes in SLAB_MATCH() it would make the code more complex than what it is now. Not sure if it's worth it but I would not oppose it. > > Or maybe CONFIG_MEM_ALLOC_PROFILING could reuse the field in that !MEMCG > case instead of needing a page_ext? (or was that the plan already?) CONFIG_MEM_ALLOC_PROFILING already uses this field under `slab.obj_exts` name for slab allocations. page_ext is used only for page allocations. > > Vlastimil > > > As Willy's purpose, the page struct will be replaced by "unsigned long mem_desc;" > > https://lore.kernel.org/lkml/YvV1KTyzZ+Jrtj9x@xxxxxxxxxxxxxxxxxxxx/ > > With this destination, slab, folio, page struct better be used differently > > and not to relay on each other. > > > > In fact there is another solution work conventionally for logical match: > > > > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h > > index ef09c4eef6d3..8f9a01a24f18 100644 > > --- a/include/linux/mm_types.h > > +++ b/include/linux/mm_types.h > > @@ -180,7 +180,7 @@ struct page { > > /* Usage count. *DO NOT USE DIRECTLY*. See page_ref.h */ > > atomic_t _refcount; > > > > -#ifdef CONFIG_SLAB_OBJ_EXT > > +#if defined(CONFIG_SLAB_OBJ_EXT) || defined(CONIFG_MEMCG) > > unsigned long memcg_data; > > #endif > > > > @@ -343,7 +343,7 @@ struct folio { > > }; > > atomic_t _mapcount; > > atomic_t _refcount; > > -#ifdef CONFIG_SLAB_OBJ_EXT > > +#if defined(CONFIG_SLAB_OBJ_EXT) || defined(CONIFG_MEMCG) > > unsigned long memcg_data; > > #endif > > #if defined(WANT_PAGE_VIRTUAL) > > > > But compere above convention solution, the current way is simple and closer > > to our destination, and we could easy fall back even it fails, isn't it? > > > > Thanks > > Alex > > > >>> > >>> Thanks a lot! > >>> Alex > >>> > >>> diff --git a/mm/slab.h b/mm/slab.h > >>> index 3586e6183224..9e7ccaf868e2 100644 > >>> --- a/mm/slab.h > >>> +++ b/mm/slab.h > >>> @@ -97,7 +97,7 @@ struct slab { > >>> SLAB_MATCH(flags, __page_flags); > >>> SLAB_MATCH(compound_head, slab_cache); /* Ensure bit 0 is clear */ > >>> SLAB_MATCH(_refcount, __page_refcount); > >>> -#ifdef CONFIG_SLAB_OBJ_EXT > >>> +#if defined(CONFIG_SLAB_OBJ_EXT) && defined(CONFIG_MEMCG) > >>> SLAB_MATCH(memcg_data, obj_exts); > >>> #endif > >>> #undef SLAB_MATCH > >>>> > >>>>> > >>>>> This patch will shortly appear at > >>>>> https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/mm-memcg-alignment-memcg_data-define-condition.patch > >>>>> > >>>>> This patch will later appear in the mm-unstable branch at > >>>>> git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm > >>>>> > >>>>> Before you just go and hit "reply", please: > >>>>> a) Consider who else should be cc'ed > >>>>> b) Prefer to cc a suitable mailing list as well > >>>>> c) Ideally: find the original patch on the mailing list and do a > >>>>> reply-to-all to that, adding suitable additional cc's > >>>>> > >>>>> *** Remember to use Documentation/process/submit-checklist.rst when testing your code *** > >>>>> > >>>>> The -mm tree is included into linux-next via the mm-everything > >>>>> branch at git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm > >>>>> and is updated there every 2-3 working days > >>>>> > >>>>> ------------------------------------------------------ > >>>>> From: "Alex Shi (Tencent)" <alexs@xxxxxxxxxx> > >>>>> Subject: mm/memcg: alignment memcg_data define condition > >>>>> Date: Mon, 8 Jul 2024 14:32:36 +0800 > >>>>> > >>>>> commit 21c690a349ba ("mm: introduce slabobj_ext to support slab object > >>>>> extensions") changed the folio/page->memcg_data define condition from > >>>>> MEMCG to SLAB_OBJ_EXT. The code works well, since config SLAB_OBJ_EXT is > >>>>> fold into MEMCG in init/Kconfig. > >>>>> > >>>>> But many related functions that deal with memcg_data still defined under > >>>>> MEMCG instead of SLAB_OBJ_EXT, and FOLIO_MATCH with memcg_data are defined > >>>>> under MEMCG too. That looks weird and incorrect with memcg_data raw > >>>>> meaning. > >>>>> > >>>>> So let's put memcg_data under MEMCG config to alignment the definition > >>>>> with FOLIO_MATCH and its usage in functions. > >>>>> > >>>>> Link: https://lkml.kernel.org/r/20240708063236.1096395-1-alexs@xxxxxxxxxx > >>>>> Signed-off-by: Alex Shi (Tencent) <alexs@xxxxxxxxxx> > >>>>> Cc: Johannes Weiner <hannes@xxxxxxxxxxx> > >>>>> Cc: Kees Cook <keescook@xxxxxxxxxxxx> > >>>>> Cc: Pasha Tatashin <pasha.tatashin@xxxxxxxxxx> > >>>>> Cc: Suren Baghdasaryan <surenb@xxxxxxxxxx> > >>>>> Cc: Vlastimil Babka <vbabka@xxxxxxx> > >>>>> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> > >>>>> --- > >>>>> > >>>>> include/linux/mm_types.h | 4 ++-- > >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>>> > >>>>> --- a/include/linux/mm_types.h~mm-memcg-alignment-memcg_data-define-condition > >>>>> +++ a/include/linux/mm_types.h > >>>>> @@ -180,7 +180,7 @@ struct page { > >>>>> /* Usage count. *DO NOT USE DIRECTLY*. See page_ref.h */ > >>>>> atomic_t _refcount; > >>>>> > >>>>> -#ifdef CONFIG_SLAB_OBJ_EXT > >>>>> +#ifdef CONFIG_MEMCG > >>>>> unsigned long memcg_data; > >>>>> #endif > >>>>> > >>>>> @@ -343,7 +343,7 @@ struct folio { > >>>>> }; > >>>>> atomic_t _mapcount; > >>>>> atomic_t _refcount; > >>>>> -#ifdef CONFIG_SLAB_OBJ_EXT > >>>>> +#ifdef CONFIG_MEMCG > >>>>> unsigned long memcg_data; > >>>>> #endif > >>>>> #if defined(WANT_PAGE_VIRTUAL) > >>>>> _ > >>>>> > >>>>> Patches currently in -mm which might be from alexs@xxxxxxxxxx are > >>>>> > >>>>> mm-memcg-alignment-memcg_data-define-condition.patch > >>>>> >
