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

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

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.

>
> >
> > >
> > > >
> > > > >
> > > > > Are we hacking this for the sake of overloading what this does?
> > > >
> > > > The @vma is actually unneeded. I will remove it.
> > >
> > > Ah OK.
> > >
> > > I am still a little concerned about passing around a value reference to the VMA
> > > flags though, esp as this type can + will change in future (not sure what that
> > > means for BPF).
> > >
> > > We may go to e.g. a 128 bit bitmap there etc.
> >
> > As mentioned in another thread, we only need to determine whether the
> > flag is VM_HUGEPAGE or VM_NOHUGEPAGE, so it can be simplified.
>
> OK cool thanks. Maybe missed.
>
> >
> > >
> > >
> > > >
> > > > >
> > > > > 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.

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

>
> > >
> > > >
> > > > >
> > > > > > +#else
> > > > > > +static inline int
> > > > > > +get_suggested_order(struct mm_struct *mm, struct vm_area_struct *vma__nullable,
> > > > > > +                 u64 vma_flags, enum tva_type tva_flags, int orders)
> > > > > > +{
> > > > > > +     return orders;
> > > > > > +}
> > > > > > +#endif
> > > > > > +
> > > > > >  static inline int highest_order(unsigned long orders)
> > > > > >  {
> > > > > >       return fls_long(orders) - 1;
> > > > > > diff --git a/include/linux/khugepaged.h b/include/linux/khugepaged.h
> > > > > > index eb1946a70cff..d81c1228a21f 100644
> > > > > > --- a/include/linux/khugepaged.h
> > > > > > +++ b/include/linux/khugepaged.h
> > > > > > @@ -4,6 +4,8 @@
> > > > > >
> > > > > >  #include <linux/mm.h>
> > > > > >
> > > > > > +#include <linux/huge_mm.h>
> > > > > > +
> > > > >
> > > > > Hm this is iffy too, There's probably a reason we didn't include this before,
> > > > > the headers can be so so fragile. Let's be cautious...
> > > >
> > > > I will check.
> > >
> > > Thanks!
> > >
> > > >
> > > > >
> > > > > >  extern unsigned int khugepaged_max_ptes_none __read_mostly;
> > > > > >  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> > > > > >  extern struct attribute_group khugepaged_attr_group;
> > > > > > @@ -22,7 +24,15 @@ extern int collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr,
> > > > > >
> > > > > >  static inline void khugepaged_fork(struct mm_struct *mm, struct mm_struct *oldmm)
> > > > > >  {
> > > > > > -     if (mm_flags_test(MMF_VM_HUGEPAGE, oldmm))
> > > > > > +     /*
> > > > > > +      * THP allocation policy can be dynamically modified via BPF. Even if a
> > > > > > +      * task was allowed to allocate THPs, BPF can decide whether its forked
> > > > > > +      * child can allocate THPs.
> > > > > > +      *
> > > > > > +      * The MMF_VM_HUGEPAGE flag will be cleared by khugepaged.
> > > > > > +      */
> > > > > > +     if (mm_flags_test(MMF_VM_HUGEPAGE, oldmm) &&
> > > > > > +             get_suggested_order(mm, NULL, 0, -1, BIT(PMD_ORDER)))
> > > > >
> > > > > Hmmm so there seems to be some kind of additional functionality you're providing
> > > > > here kinda quietly, which is to allow the exact same interface to determine
> > > > > whether we kick off khugepaged or not.
> > > > >
> > > > > Don't love that, I think we should be hugely specific about that.
> > > > >
> > > > > This bpf interface should literally be 'ok we're deciding what order we
> > > > > want'. It feels like a bit of a gross overloading?
> > > >
> > > > This makes sense. I have no objection to reverting to returning a single order.
> > >
> > > OK but key point here is - we're now determining if a forked child can _not_
> > > allocate THPs using this function.
> > >
> > > To me this should be a separate function rather than some _weird_ usage of this
> > > same function.
> >
> > Perhaps a separate function is better.
>
> 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.

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

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.

>
> >
> > To avoid this, we should prevent setting this flag for child processes
> > if they are not allowed to allocate THP in the first place. This way,
> > khugepaged won’t waste cycles scanning them. While an alternative
> > approach would be to set the flag at fork and later clear it for
> > khugepaged, it’s clearly more efficient to avoid setting it from the
> > start.
>
> We also obviously should have a comment with all this context here.

Understood. I'll give some thought to a better way of handling this.

-- 
Regards
Yafang