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

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

On Mon, Sep 1, 2025 at 7:39 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Sun, Aug 31, 2025 at 11:11:34AM +0800, Yafang Shao wrote:
> > On Fri, Aug 29, 2025 at 6:42 PM Lorenzo Stoakes
> > <lorenzo.stoakes@oracle.com> wrote:
> > >
> > > On Fri, Aug 29, 2025 at 11:01:59AM +0800, Yafang Shao wrote:
> > > > On Thu, Aug 28, 2025 at 6:50 PM Lorenzo Stoakes
> > > > <lorenzo.stoakes@oracle.com> wrote:
> > > > >
> > > > > On Thu, Aug 28, 2025 at 01:54:39PM +0800, Yafang Shao wrote:
> > > > > > > Also will mm ever != vma->vm_mm?
> > > > > >
> > > > > > No it can't. It can be guaranteed by the caller.
> > > > >
> > > > > In this case we don't need to pass mm separately then right?
> > > >
> > > > Right, we need to pass either @mm or @vma. However, there are cases
> > > > where vma information is not available at certain call sites, such as
> > > > in khugepaged. In those cases, we need to pass @mm instead.
> > >
> > > Yeah... this is weird to me though, are you checking in _general_ what
> > > khugepaged should use, or otherwise surely it's per-VMA?
> > >
> > > Otherwise this bpf hook seems ill-suited for that, and we should have a
> > > separate one for khugepaged surely?
> > >
> > > I also hate that we're passing mm _just because of this one edge case_,
> > > otherwise always passing vma->vm_mm, it's a confusing interface.
> >
> > make sense.
> > I'll give some thought to how we can better handle this edge case.
>
> Thanks!
>
> > > >
> > > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > Also if we're returning a bitmask of orders which you seem to be (not sure I
> > > > > > > like that tbh - I feel like we shoudl simply provide one order but open for
> > > > > > > disucssion) - shouldn't it return an unsigned long?
> > > > > >
> > > > > > We are indifferent to whether a single order or a bitmask is returned,
> > > > > > as we only use order-0 and order-9. We have no use cases for
> > > > > > middle-order pages, though this feature might be useful for other
> > > > > > architectures or for some special use cases.
> > > > >
> > > > > Well surely we want to potentially specify a mTHP under certain circumstances
> > > > > no?
> > > >
> > > > Perhaps there are use cases, but I haven’t found any use cases for
> > > > this in our production environment. On the other hand, I can clearly
> > > > see a risk that it could lead to more costly high-order allocations.
> > >
> > > So why are we returning a bitmap then? Seems like we should just return a
> > > single order in this case... I think you say below that you are open to
> > > this?
> >
> > will return a single order in the next version.
>
> Thanks
>
> >
> > >
> > > >
> > > > >
> > > > > In any case I feel it's worth making any bitfield a system word size.
> > >
> > > Also :>)
> > >
> > > If we do move to returning a single order, should be unsigned int.
> >
> > sure
>
> Thanks!
>
> > > >
> > > > >
> > > > > And generally at this point I think we should just drop this bit of code
> > > > > honestly.
> > > >
> > > > MMF_VM_HUGEPAGE is set when the THP mode is "always" or "madvise". If
> > > > it’s set, any forked child processes will inherit this flag. It is
> > > > only cleared when the mm_struct is destroyed (please correct me if I’m
> > > > wrong).
> > >
> > > __mmput()
> > > -> khugepaged_exit()
> > > -> (if MMF_VM_HUGEPAGE set) __khugepaged_exit()
> > > -> Clear flag once mm fully done with (afaict), dropping associated mm refcount.
> > >
> > > ^--- this does seem to be accurate indeed.
> >
> > Thanks for the explanation.
>
> No problem, this was more 'Lorenzo's thought process' :P
>
> >
> > >
> > > >
> > > > 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().

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() ;-)

>
> >
> > 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.

-- 
Regards
Yafang