Thanks for the review, David!
On Thu, May 12, 2022 at 1:02 PM David Rientjes <rientjes@xxxxxxxxxx> wrote:
>
> On Wed, 4 May 2022, Zach O'Keefe wrote:
>
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index c94bc43dff3e..6095fcb3f07c 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -92,6 +92,10 @@ struct collapse_control {
> >
> > /* Last target selected in khugepaged_find_target_node() */
> > int last_target_node;
> > +
> > + struct page *hpage;
> > + int (*alloc_charge_hpage)(struct mm_struct *mm,
> > + struct collapse_control *cc);
> > };
> >
> > /**
>
> Embedding this function pointer into collapse_contol seems like it would
> need some pretty strong rationale. Not to say that it should be a
> non-starter, but I think the changelog needs to clearly indicate why this
> is better/cleaner than embedding the needed info for a single allocation
> and charge function to use. If the callbacks would truly be so different
> that unifying them would be more complex, I think this makes sense.
Mostly, this boils down to khugepaged having different a allocation
pattern for NUMA/UMA ; the former scans the pages first to determine
the right node, the latter preallocates before scanning. khugepaged
has the luxury on UMA systems of just holding onto a hugepage
indefinitely for the next collapse target.
For MADV_COLLAPSE, we never preallocate, and so its pattern doesn't
depend on NUMA or UMA configs. Trying to avoid "if (khugepaged) ...
else" casing, defining this as a context-defined operation seemed
appropriate.
Collapsing both alloc and charging together was mostly a code
cleanliness decision resulting from not wanting to embed a ->gfp()
hook (gfp flags are used both by allocation and memcg charging).
Alternatively, a .gfp member could exist - it would just need to be
refreshed periodically in the khugepaged codepath.
That all said - let me take another crack at seeing if I can make this
work without the need for a function pointer here.
