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

[Zach O'Keefe <zokeefe@google.com>]

On Jul 11 11:45, Andrew Morton wrote:
> 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.
> 

I think it makes sense. Reason we moved this struct to kmalloc was MAX_NUMNODES
can be pretty large - so might as well save a few bytes for a pretty small
change. Yang seems good with it, anyways :)