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