Re: [PATCH v6 mm-new 01/10] mm: thp: add support for BPF based THP order selection

[Yafang Shao <laoar.shao@gmail.com>]

On Tue, Sep 2, 2025 at 3:50 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Tue, Sep 02, 2025 at 10:48:47AM +0800, Yafang Shao wrote:
> > > >
> > > > >
> > > > > >
> > > > > > However, when you switch the THP mode to "never", tasks that still
> > > > > > have MMF_VM_HUGEPAGE remain on the khugepaged scan list. This isn’t an
> > > > > > issue under the current global mode because khugepaged doesn’t run
> > > > > > when THP is set to "never".
> > > > > >
> > > > > > The problem arises when we move from a global mode to a per-task mode.
> > > > > > In that case, khugepaged may end up doing unnecessary work. For
> > > > > > example, if the THP mode is "always", but some tasks are not allowed
> > > > > > to allocate THP while still having MMF_VM_HUGEPAGE set, khugepaged
> > > > > > will continue scanning them unnecessarily.
> > > > >
> > > > > But this can change right?
> > > > >
> > > > > I really don't like the idea _at all_ of overriding this hook to do things
> > > > > other than what it says it does.
> > > > >
> > > > > It's 'set which order to use' except when it's this case then it's 'will we
> > > > > do any work'.
> > > > >
> > > > > This should be a separate callback or we should drop this and live with the
> > > > > possible additional work.
> > > >
> > > > Perhaps we could reuse the MMF_DISABLE_THP flag by introducing a new
> > > > BPF helper to set it when we want to disable THP for a specific task.
> > >
> > > Interesting, yeah perhaps that could work, as long as we're in a sensible
> > > context to be able to toggle this bit.
> >
> > Right, we can't set the mm->flags arbitrarily.
> > Perhaps we should add a generic BPF hook in dup_mmap().
> >
>
> Yeah perhaps that could be a way forward :)
>
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 7a057e0e8da9..1b60bdb08de1 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -1843,6 +1843,8 @@ __latent_entropy int dup_mmap(struct mm_struct
> > *mm, struct mm_struct *oldmm)
> >  loop_out:
> >         vma_iter_free(&vmi);
> >         if (!retval) {
> > +               /* Allow a BPF program to modify the new mm_struct in fork. */
> > +               bpf_hook_mm_fork(mm, oldmm);
> >                 mt_set_in_rcu(vmi.mas.tree);
> >                 ksm_fork(mm, oldmm);
> >                 khugepaged_fork(mm, oldmm);
> >
> > This provides a mechanism for BPF programs to configure the new
> > mm_struct on demand, acting as a modern, flexible replacement for
> > prctl() ;-)
>
> Hahaha that's obviously very appealing to me :)))
>
> >
> > >
> > > >
> > > > Separately from this patchset, I realized we can optimize khugepaged
> > > > handling for the MMF_DISABLE_THP case with the following changes:
> > > >
> > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > > > index 15203ea7d007..e9964edcee29 100644
> > > > --- a/mm/khugepaged.c
> > > > +++ b/mm/khugepaged.c
> > > > @@ -402,6 +402,11 @@ void __init khugepaged_destroy(void)
> > > >         kmem_cache_destroy(mm_slot_cache);
> > > >  }
> > > >
> > > > +static inline int hpage_collapse_test_disable(struct mm_struct *mm)
> > > > +{
> > > > +       return test_bit(MMF_DISABLE_THP, &mm->flags);
> > > > +}
> > > > +
> > > >  static inline int hpage_collapse_test_exit(struct mm_struct *mm)
> > > >  {
> > > >         return atomic_read(&mm->mm_users) == 0;
> > > > @@ -1448,6 +1453,11 @@ static void collect_mm_slot(struct
> > > > khugepaged_mm_slot *mm_slot)
> > > >                 /* khugepaged_mm_lock actually not necessary for the below */
> > > >                 mm_slot_free(mm_slot_cache, mm_slot);
> > > >                 mmdrop(mm);
> > > > +       } else if (hpage_collapse_test_disable(mm)) {
> > > > +               hash_del(&slot->hash);
> > > > +               list_del(&slot->mm_node);
> > > > +               mm_flags_clear(MMF_VM_HUGEPAGE, mm);
> > > > +               mm_slot_free(mm_slot_cache, mm_slot);
> > > >         }
> > > >  }
> > > >
> > > > Specifically, if MMF_DISABLE_THP is set, we should remove it from
> > > > mm_slot to prevent unnecessary khugepaged processing.
> > >
> > > Ohhh interesting, perhaps send as separate patch?
> >
> > sure, I will send it separately.
>
> Thanks!
>
> >
> > --
> > Regards
> > Yafang
>
> And overall - cheers for being an ABSOLUTE DELIGHT on review :) it's much
> appreciated. I shall buy you a beer (or whatever is your preferred
> beverage) at the next conference we are both at :)

Honestly, that's exactly what I wanted to say to you too! I learned so
much during your review process, and I owe you a beer (or your drink
of choice) as well!

-- 
Regards
Yafang