On Wed, Apr 27, 2022 at 12:49 PM Peter Xu <peterx@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
>
