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