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

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

On Thu, Sep 11, 2025 at 10:51 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Wed, Sep 10, 2025 at 10:44:39AM +0800, Yafang Shao wrote:
> > diff --git a/mm/huge_memory_bpf.c b/mm/huge_memory_bpf.c
> > new file mode 100644
> > index 000000000000..525ee22ab598
> > --- /dev/null
> > +++ b/mm/huge_memory_bpf.c
>
> [snip]
>
> > +unsigned long bpf_hook_thp_get_orders(struct vm_area_struct *vma,
> > +                                   vm_flags_t vma_flags,
> > +                                   enum tva_type tva_type,
> > +                                   unsigned long orders)
> > +{
> > +     thp_order_fn_t *bpf_hook_thp_get_order;
> > +     unsigned long thp_orders = orders;
> > +     enum bpf_thp_vma_type vma_type;
> > +     int thp_order;
> > +
> > +     /* No BPF program is attached */
> > +     if (!test_bit(TRANSPARENT_HUGEPAGE_BPF_ATTACHED,
> > +                   &transparent_hugepage_flags))
> > +             return orders;
> > +
> > +     if (vma_flags & VM_HUGEPAGE)
> > +             vma_type = BPF_THP_VM_HUGEPAGE;
> > +     else if (vma_flags & VM_NOHUGEPAGE)
> > +             vma_type = BPF_THP_VM_NOHUGEPAGE;
> > +     else
> > +             vma_type = BPF_THP_VM_NONE;
> > +
> > +     rcu_read_lock();
> > +     bpf_hook_thp_get_order = rcu_dereference(bpf_thp.thp_get_order);
> > +     if (!bpf_hook_thp_get_order)
> > +             goto out;
> > +
> > +     thp_order = bpf_hook_thp_get_order(vma, vma_type, tva_type, orders);
> > +     if (thp_order < 0)
> > +             goto out;
> > +     /*
> > +      * The maximum requested order is determined by the callsite. E.g.:
> > +      * - PMD-mapped THP uses PMD_ORDER
> > +      * - mTHP uses (PMD_ORDER - 1)
> > +      *
> > +      * We must respect this upper bound to avoid undefined behavior. So the
> > +      * highest suggested order can't exceed the highest requested order.
> > +      */
> > +     if (thp_order <= highest_order(orders))
> > +             thp_orders = BIT(thp_order);
>
> OK so looking at Lance's reply re: setting 0 and what we're doing here in
> general - this seems a bit weird to me.
>
> Shouldn't orders be specifying a _mask_ as to which orders are _available_,
> rather than allowing a user to specify an arbitrary order?
>
> So if you're a position where the only possible order is PMD sized, now this
> would let you arbitrarily select an mTHP right? That does no seem correct.
>
> And as per Lance, if we cannot satisfy the requested order, we shouldn't fall
> back to available orders, we should take that as a signal that we cannot have
> THP at all.
>
> So shouldn't this just be:
>
>         thp_orders = orders & BIT(thp_order);

That's better.  I will change it.

-- 
Regards
Yafang