Re: [mm-unstable v7 03/18] mm/khugepaged: add struct collapse_control

[Andrew Morton <akpm@linux-foundation.org>]

On Mon, 11 Jul 2022 11:29:13 -0700 "Zach O'Keefe" <zokeefe@google.com> wrote:

> On Fri, Jul 8, 2022 at 2:01 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Wed,  6 Jul 2022 16:59:21 -0700 "Zach O'Keefe" <zokeefe@google.com> wrote:
> >
> > > Modularize hugepage collapse by introducing struct collapse_control.
> > > This structure serves to describe the properties of the requested
> > > collapse, as well as serve as a local scratch pad to use during the
> > > collapse itself.
> > >
> > > Start by moving global per-node khugepaged statistics into this
> > > new structure.  Note that this structure is still statically allocated
> > > since CONFIG_NODES_SHIFT might be arbitrary large, and stack-allocating
> > > a MAX_NUMNODES-sized array could cause -Wframe-large-than= errors.
> > >
> > > Signed-off-by: Zach O'Keefe <zokeefe@google.com>
> > > ---
> > >  mm/khugepaged.c | 87 ++++++++++++++++++++++++++++---------------------
> > >  1 file changed, 50 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > > index 196eaadbf415..f1ef02d9fe07 100644
> > > --- a/mm/khugepaged.c
> > > +++ b/mm/khugepaged.c
> > > @@ -85,6 +85,14 @@ static struct kmem_cache *mm_slot_cache __read_mostly;
> > >
> > >  #define MAX_PTE_MAPPED_THP 8
> > >
> > > +struct collapse_control {
> > > +     /* Num pages scanned per node */
> > > +     int node_load[MAX_NUMNODES];
> >
> > Does this actually need to be 32-bit?  Looking at the current code I'm
> > suspecting that khugepaged_node_load[] could be a ushort?
> >
> > [And unsigned int would be more appropriate, but we always do that :(]
> >
> 
> Hey Andrew,
> 
> Thanks for taking the time to review, and good catch - I don't think
> we need 32 bits.
> 
> Minimally, we just need to be able to hold the maximum value of
> HPAGE_PMD_NR = 1 << (PMD_SHIFT - PAGE_SHIFT).
> 
> I'm not sure what arch/config options (that also use THP) produce the
> minimum/maximum value here. I looked through most of the archs that
> define PMD_SHIFT, and couldn't find an example where we'd need > 16
> bits, with most cases still requiring > 8 bits. All the various
> configs do get complicated though.
> 
> Is it acceptable to use u16, with an #error if HPAGE_PMD_ORDER >= 16?

It might be ;)

It was just a thought - perhaps something which you or someone else
might choose to look at, but I don't think this work needs to be part
of the current series, unless the current series consumes egregious
amounts of memory.