Re: [PATCH v3 02/12] mm/khugepaged: add struct collapse_control

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

On Wed, Apr 27, 2022 at 12:49 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Tue, Apr 26, 2022 at 07:44:02AM -0700, Zach O'Keefe 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.
>
> Reasonable, but IMHO worth mentioning that "we're introducing the context
> by first moving the per-node statistics into ...", or something like that.
>

Sounds good to me. Updated.

> >
> > Signed-off-by: Zach O'Keefe <zokeefe@google.com>
> > ---
> >  mm/khugepaged.c | 79 ++++++++++++++++++++++++++++---------------------
> >  1 file changed, 46 insertions(+), 33 deletions(-)
> >
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index 2933b13fc975..9d42fa330812 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -86,6 +86,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];
> > +
> > +     /* Last target selected in khugepaged_find_target_node() for this scan */
>
> Not really important, but.. iiuc this is not only for this scan but for all
> scans of the khugepaged thread.
>

Thanks for catching this. I've removed the "for this scan" bit to
avoid confusion.

> > +     int last_target_node;
> > +};
>
> [...]
>
> > @@ -829,28 +835,28 @@ static inline gfp_t alloc_hugepage_khugepaged_gfpmask(void)
> >  }
> >
> >  #ifdef CONFIG_NUMA
> > -static int khugepaged_find_target_node(void)
> > +static int khugepaged_find_target_node(struct collapse_control *cc)
> >  {
> > -     static int last_khugepaged_target_node = NUMA_NO_NODE;
> >       int nid, target_node = 0, max_value = 0;
> >
> >       /* find first node with max normal pages hit */
> >       for (nid = 0; nid < MAX_NUMNODES; nid++)
> > -             if (khugepaged_node_load[nid] > max_value) {
> > -                     max_value = khugepaged_node_load[nid];
> > +             if (cc->node_load[nid] > max_value) {
> > +                     max_value = cc->node_load[nid];
> >                       target_node = nid;
> >               }
> >
> >       /* do some balance if several nodes have the same hit record */
> > -     if (target_node <= last_khugepaged_target_node)
> > -             for (nid = last_khugepaged_target_node + 1; nid < MAX_NUMNODES;
> > -                             nid++)
> > -                     if (max_value == khugepaged_node_load[nid]) {
> > +     if (target_node <= cc->last_target_node)
> > +             for (nid = cc->last_target_node + 1; nid < MAX_NUMNODES;
> > +                  nid++) {
> > +                     if (max_value == cc->node_load[nid]) {
> >                               target_node = nid;
> >                               break;
> >                       }
> > +             }
>
> I'm not sure what's the coding style for this case, but IIUC we could
> either add both (to both outer "if" and "for") or none; adding one pair of
> brackets seems a bit odd.
>

Sorry about that - remnant of a change that was (partially) reverted.
I've removed the extra brackets to keep change minimal. Thank you.

> >
> > -     last_khugepaged_target_node = target_node;
> > +     cc->last_target_node = target_node;
> >       return target_node;
> >  }
>
> --
> Peter Xu
>