Re: [PATCH v4 10/27] mm/sparse: allow for alternate vmemmap section init at boot

[Johannes Weiner <hannes@cmpxchg.org>]

On Thu, Feb 27, 2025 at 10:03:04AM -0800, Frank van der Linden wrote:
> On Thu, Feb 27, 2025 at 9:56 AM Frank van der Linden <fvdl@google.com> wrote:
> >
> > On Thu, Feb 27, 2025 at 9:32 AM Frank van der Linden <fvdl@google.com> wrote:
> > >
> > > On Thu, Feb 27, 2025 at 9:20 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
> > > >
> > > > On Thu, Feb 27, 2025 at 08:47:18AM -0800, Frank van der Linden wrote:
> > > > > On Wed, Feb 26, 2025 at 10:09 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
> > > > > >
> > > > > > On Tue, Feb 18, 2025 at 06:16:38PM +0000, Frank van der Linden wrote:
> > > > > > > @@ -489,6 +489,14 @@ config SPARSEMEM_VMEMMAP
> > > > > > >         SPARSEMEM_VMEMMAP uses a virtually mapped memmap to optimise
> > > > > > >         pfn_to_page and page_to_pfn operations.  This is the most
> > > > > > >         efficient option when sufficient kernel resources are available.
> > > > > > > +
> > > > > > > +config ARCH_WANT_SPARSEMEM_VMEMMAP_PREINIT
> > > > > > > +     bool
> > > > > > > +
> > > > > > > +config SPARSEMEM_VMEMMAP_PREINIT
> > > > > > > +     bool "Early init of sparse memory virtual memmap"
> > > > > > > +     depends on SPARSEMEM_VMEMMAP && ARCH_WANT_SPARSEMEM_VMEMMAP_PREINIT
> > > > > > > +     default y
> > > > > >
> > > > > > oldconfig just prompted me on this, but it's not clear to me what it
> > > > > > does. Not even after skimming the changelog of the patch to be honest.
> > > > > >
> > > > > > Can you please add a help text that explains the user-visible effects
> > > > > > of the toggle, as well as guidance as to who might care to change it?
> > > > >
> > > > > Hi Johannes,
> > > > >
> > > > > Thanks for your comment. How's this:
> > > >
> > > > Thanks for the quick reply!
> > > >
> > > > > Enables subsystems to pre-initialize memmap in their own way,
> > > > > allowing for memory savings during boot. The HugeTLB code uses
> > > > > this to initialize memmap for bootmem allocated gigantic hugepages
> > > > > in a way that is done by HUGETLB_PAGE_OPTIMIZE_VMEMMAP. This
> > > > > means saving this memory right away, instead of allocating it
> > > > > first and then freeing it later. Not allocating these pages
> > > > > at all during boot allows for specifying a bigger number of
> > > > > hugepages on the kernel commandline on larger systems.
> > > >
> > > > That makes sense.
> > > >
> > > > But if it's infra code for a hugetlb feature, it should either be
> > > > something that HUGETLB_PAGE_OPTIMIZE_VMEMMAP pulls in automatically,
> > > > or at least be a hugetlb-specific option that pulls it in.
> > > >
> > > > Keep in mind that not everybody enables HUGETLBFS. In fact, hugetlb is
> > > > default N. It's moot to ask users whether they want to enable infra
> > > > code for a feature they aren't using, and default to Y no less. You're
> > > > regressing innocent bystanders doing this.
> > >
> > > The main reason that I added a separate config was:
> > >
> > > 1) I could see other subsystems use this.
> > > 2) The number of section flags is limited, so I wanted to put the one
> > > I added inside an option instead of always using it.

Yeah, an *internal* config symbol make sense, so that the sparse flag
and the code generation are gated on whether there is an actual user.

I'm just proposing to make it invisible and let
HUGETLB_PAGE_OPTIMIZE_VMEMMAP (and future users) select/depend on it.

> > > If especially 2) is not a concern or can be solved differently, I'll
> > > be happy to remove the option. I don't particularly like having it,
> > > but I didn't see a better way.
> > >
> > > Let me think of a way to clean this up a little, and suggestions are
> > > welcome, of course.
> > >
> > > - Frank
> >
> > I'll just do:
> >
> > diff --git a/fs/Kconfig b/fs/Kconfig
> > index 64d420e3c475..fb9831927a08 100644
> > --- a/fs/Kconfig
> > +++ b/fs/Kconfig
> > @@ -286,6 +286,7 @@ config HUGETLB_PAGE_OPTIMIZE_VMEMMAP
> >         def_bool HUGETLB_PAGE
> >         depends on ARCH_WANT_OPTIMIZE_HUGETLB_VMEMMAP
> >         depends on SPARSEMEM_VMEMMAP
> > +       select SPARSEMEM_VMEMMAP_PREINIT
> >
> >  config HUGETLB_PMD_PAGE_TABLE_SHARING
> >         def_bool HUGETLB_PAGE
> > diff --git a/mm/Kconfig b/mm/Kconfig
> > index f984dd928ce7..44b52f8e5296 100644
> > --- a/mm/Kconfig
> > +++ b/mm/Kconfig
> > @@ -496,7 +496,6 @@ config ARCH_WANT_SPARSEMEM_VMEMMAP_PREINIT
> >  config SPARSEMEM_VMEMMAP_PREINIT
> >         bool "Early init of sparse memory virtual memmap"
> >         depends on SPARSEMEM_VMEMMAP && ARCH_WANT_SPARSEMEM_VMEMMAP_PREINIT
> > -       default y
> >
> > Does that seem ok? I'll send an mm-unstable follow-up patch.
> >
> 
> Wait, that's actually not correct. Anyway, I'll stop spamming - I'll
> do it along these lines but properly, and will send a follow-up patch.

If you remove the prompt after "bool" it becomes an internal symbol
that you can then pull in as needed.

I agree that unconditionally consuming the sparse flag would be
unfortunate, but consuming it when HUGETLB_PAGE_OPTIMIZE_VMEMMAP is
enabled is fine, right? Seems like a specialized enough config.

Thanks!