Re: [PATCH] mm: huge_memory: don't start_stop_khugepaged for non-PMD THP

[Barry Song]

On Fri, Jul 26, 2024 at 10:45 PM Ryan Roberts <ryan.roberts@xxxxxxx> wrote:
>
> On 26/07/2024 11:04, Barry Song wrote:
> > On Fri, Jul 26, 2024 at 9:48 PM David Hildenbrand <david@xxxxxxxxxx> wrote:
> >>
> >> On 26.07.24 11:43, Ryan Roberts wrote:
> >>> On 26/07/2024 09:28, Barry Song wrote:
> >>>> From: Barry Song <v-songbaohua@xxxxxxxx>
> >>>>
> >>>> khugepaged will be automatically started when PMD-sized THP is enabled
> >>>> (either of the per-size anon control or the top-level control are set
> >>>> to "always" or "madvise"), and it'll be automatically shutdown when
> >>>> PMD-sized THP is disabled (when both the per-size anon control and the
> >>>> top-level control are "never").
> >>>>
> >>>> It seems unnecessary to call start_stop_khugepaged() for non-PMD THP,
> >>>> as it would only waste CPU time.
> >>>>
> >>>> Cc: Lance Yang <ioworker0@xxxxxxxxx>
> >>>> Cc: Ryan Roberts <ryan.roberts@xxxxxxx>
> >>>> Cc: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
> >>>> Cc: David Hildenbrand <david@xxxxxxxxxx>
> >>>> Cc: Yang Shi <shy828301@xxxxxxxxx>
> >>>> Cc: Zi Yan <ziy@xxxxxxxxxx>
> >>>> Signed-off-by: Barry Song <v-songbaohua@xxxxxxxx>
> >>>> ---
> >>>>   mm/huge_memory.c | 2 +-
> >>>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> >>>> index 41460847988c..bd365e35acf7 100644
> >>>> --- a/mm/huge_memory.c
> >>>> +++ b/mm/huge_memory.c
> >>>> @@ -514,7 +514,7 @@ static ssize_t thpsize_enabled_store(struct kobject *kobj,
> >>>>      } else
> >>>>              ret = -EINVAL;
> >>>>
> >>>> -    if (ret > 0) {
> >>>> +    if (ret > 0 && order == HPAGE_PMD_ORDER) {
> >>>>              int err;
> >>>>
> >>>>              err = start_stop_khugepaged();
> >>>
> >>> Personally I see this as a bit of a layering violation; its
> >>> start_stop_khugepaged() that should decide the policy for when to start and stop
> >>> the daemon. thpsize_enabled_store() should just be calling
> >>> start_stop_khugepaged() to notify that something potentially pertinent to the a
> >>> policy decision has changed.
> >>
> >
> > My impression is that it slightly deviates from the huge page documentation in
> > Documentation/admin-guide/mm/transhuge.rst.
> >
> > khugepaged will be automatically started when PMD-sized THP is enabled
> > (either of the per-size anon control or the top-level control are set
> > to "always" or "madvise"), and it'll be automatically shutdown when
> > PMD-sized THP is disabled (when both the per-size anon control and the
> > top-level control are "never").
>
> But start_stop_khugepaged() doesn't unconditionally start khugepaged, it takes
> action based on hugepage_pmd_enabled() which only returns true if there are any
> pmd sized THP enabled (currently looking at anon and file, but should also look
> at shmem in future; that's a known bug that's been there forever). So I don't
> think it is inconsistent with the documentation?

My point is that, as a code reader, if non-PMD sizes are never
involved with khugepaged,
we can proceed without needing to check the lower-layer code.
otherwise it is a bit
confusing, especially since start_stop_khugepaged() unconditionally calls
set_recommended_min_free_kbytes() for all sizes, regardless of whether
they are PMD.

However, I agree with your point that if non-PMD sizes are eventually managed by
khugepaged, then this approach is flawed. This is the downside of this change.

>
> >
> > non-PMD size is not involved in khugepaged, but I agree the policy might change
> > in the future.
> >
> >> Agreed, skimming the subject I was under the impression that we would be
> >> fixing something here.
> >
> > working on another swapin_enabled and reviewing the enabled source code.
> > I don't need this startstop for all sizes in that case, so I made a
> > quick adjustment
> > to this part as well. If neither of you likes it, that's fine with me :-)
> >
> >>
> >> --
> >> Cheers,
> >>
> >> David / dhildenb
> >>

Thanks
Barry