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