Message-ID: <CAJuCfpED6HQh8mT-8tvKzzcH2Xo=tP2Two+f=zk8sW-o_AJ8qw@mail.gmail.com>
Date: Wed, 16 Apr 2025 17:15:30 -0700
From: Suren Baghdasaryan <surenb@...gle.com>
To: Usama Arif <usamaarif642@...il.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org, hannes@...xchg.org, 
	shakeel.butt@...ux.dev, linux-kernel@...r.kernel.org, kernel-team@...a.com
Subject: Re: [PATCH] alloc_tag: introduce Kconfig option for default
 compressed profiling

On Wed, Apr 16, 2025 at 2:52 PM Usama Arif <usamaarif642@...il.com> wrote:
>
>
>
> On 16/04/2025 22:08, Suren Baghdasaryan wrote:
> > On Wed, Apr 16, 2025 at 11:06 AM Usama Arif <usamaarif642@...il.com> wrote:
> >>
> >> With this Kconfig option enabled, the kernel stores allocation tag references
> >> in the page flags by default.
> >>
> >> There are 2 reasons to introduce this:
> >> - As mentioned in [1], compressed tags dont have system memory overhead
> >> and much lower performance overhead. It would be preferrable to have this as
> >> the default option, and to be able to switch it at compile time. Another
> >> option is to just declare the static key as true by default?
> >> - As compressed option is the best one, it doesn't make sense to have to
> >> change both defconfig and command line options to enable memory
> >> allocation profiling. Changing commandline across a large number of services
> >> can result in signifcant work, which shouldn't be needed if the kernel
> >> defconfig needs to be changed anyways.
> >
> > The reason tag compression is not the default option is because it
> > works only if there are enough free bits in the page flags to store a
> > tag index. If you configure it to use page flags and your build does
> > not have enough free bits, the profiling will be disabled (see
> > alloc_tag_sec_init()). IOW there is no graceful fallback to use page
> > extensions. Therefore, the current default option is not the most
> > performant but the one which works on all builds. Instead of this just
> > set sysctl.vm.mem_profiling boot parameter in your config file.
>
> Hi Suren,

Hi Usama,

>
> Thanks for the review! The main reason is to not have to make a change in
> both defconfig and kernel command line while deploying it. We can ofcourse
> set the commandline as well, but just makes deployment more tedious, and
> adds an extra commandline parameter. In our case, we only want to deploy
> compressed tags, and if there aren't enough free bits, we would prefer to
> disable memory allocation profiling than to take the memory and performance
> hit.
>
> Would keeping the default value of this config disabled be an acceptable option?
> i.e. the below diff on top of this patch?

Well, in that case I fail to see why
CONFIG_MEM_ALLOC_PROFILING_COMPRESSED_ENABLED_BY_DEFAULT=y is better
than CONFIG_CMDLINE="sysctl.vm.mem_profiling=1,compressed" ? Either
way you need to change the config file, no?

>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 66d8995f3514..163ffcece47a 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1030,7 +1030,7 @@ config MEM_ALLOC_PROFILING_ENABLED_BY_DEFAULT
>
>  config MEM_ALLOC_PROFILING_COMPRESSED_ENABLED_BY_DEFAULT
>         bool "store page allocation tag references in the page flags by default"
> -       default y
> +       default n
>         depends on MEM_ALLOC_PROFILING
>
>  config MEM_ALLOC_PROFILING_DEBUG
>
>
> Thanks,
> Usama
> >
> > Your change effectively changes the default value of
> > mem_profiling_compressed and I don't see why you need to introduce a
> > new config option for that. But that really does not matter because
> > changing default to compressed tags is not the right choice IMO.
> >
> >>
> >> [1] https://lore.kernel.org/all/20241023170759.999909-7-surenb@google.com/T/#m0da08879435f7673eaa10871a6e9d1be4f605ac8
> >>
> >> Signed-off-by: Usama Arif <usamaarif642@...il.com>
> >> ---
> >>  include/linux/pgalloc_tag.h | 4 ++++
> >>  lib/Kconfig.debug           | 5 +++++
> >>  lib/alloc_tag.c             | 4 ++++
> >>  3 files changed, 13 insertions(+)
> >>
> >> diff --git a/include/linux/pgalloc_tag.h b/include/linux/pgalloc_tag.h
> >> index c74077977830..0226059bcf00 100644
> >> --- a/include/linux/pgalloc_tag.h
> >> +++ b/include/linux/pgalloc_tag.h
> >> @@ -16,7 +16,11 @@ extern unsigned long alloc_tag_ref_mask;
> >>  extern int alloc_tag_ref_offs;
> >>  extern struct alloc_tag_kernel_section kernel_tags;
> >>
> >> +#ifdef CONFIG_MEM_ALLOC_PROFILING_COMPRESSED_ENABLED_BY_DEFAULT
> >> +DECLARE_STATIC_KEY_TRUE(mem_profiling_compressed);
> >> +#else
> >>  DECLARE_STATIC_KEY_FALSE(mem_profiling_compressed);
> >> +#endif
> >>
> >>  typedef u16    pgalloc_tag_idx;
> >>
> >> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> >> index 9fe4d8dfe578..66d8995f3514 100644
> >> --- a/lib/Kconfig.debug
> >> +++ b/lib/Kconfig.debug
> >> @@ -1028,6 +1028,11 @@ config MEM_ALLOC_PROFILING_ENABLED_BY_DEFAULT
> >>         default y
> >>         depends on MEM_ALLOC_PROFILING
> >>
> >> +config MEM_ALLOC_PROFILING_COMPRESSED_ENABLED_BY_DEFAULT
> >> +       bool "store page allocation tag references in the page flags by default"
> >> +       default y
> >> +       depends on MEM_ALLOC_PROFILING
> >> +
> >>  config MEM_ALLOC_PROFILING_DEBUG
> >>         bool "Memory allocation profiler debugging"
> >>         default n
> >> diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c
> >> index 25ecc1334b67..30adad5630dd 100644
> >> --- a/lib/alloc_tag.c
> >> +++ b/lib/alloc_tag.c
> >> @@ -31,7 +31,11 @@ DEFINE_STATIC_KEY_MAYBE(CONFIG_MEM_ALLOC_PROFILING_ENABLED_BY_DEFAULT,
> >>                         mem_alloc_profiling_key);
> >>  EXPORT_SYMBOL(mem_alloc_profiling_key);
> >>
> >> +#ifdef CONFIG_MEM_ALLOC_PROFILING_COMPRESSED_ENABLED_BY_DEFAULT
> >> +DEFINE_STATIC_KEY_TRUE(mem_profiling_compressed);
> >> +#else
> >>  DEFINE_STATIC_KEY_FALSE(mem_profiling_compressed);
> >> +#endif
> >>
> >>  struct alloc_tag_kernel_section kernel_tags = { NULL, 0 };
> >>  unsigned long alloc_tag_ref_mask;
> >> --
> >> 2.47.1
> >>
>

Powered by blists - more mailing lists