Re: [RFC PATCH v3 2/5] mm, thp: add bpf thp hook to determine thp allocator

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

On Thu, Jul 17, 2025 at 11:30 PM Usama Arif <usamaarif642@gmail.com> wrote:
>
>
>
> On 08/06/2025 08:35, Yafang Shao wrote:
> > A new hook bpf_thp_allocator() is added to determine if the THP is
> > allocated by khugepaged or by the current task.
>
> I would add in the summary why we need this. I am assuming I will find out
> when reviewing the next few patches, but would be good to know here.
>
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >  include/linux/huge_mm.h | 10 ++++++++++
> >  mm/khugepaged.c         |  2 ++
> >  2 files changed, 12 insertions(+)
> >
> > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> > index 2f190c90192d..db2eadd3f65b 100644
> > --- a/include/linux/huge_mm.h
> > +++ b/include/linux/huge_mm.h
> > @@ -190,6 +190,14 @@ static inline bool hugepage_global_always(void)
> >                       (1<<TRANSPARENT_HUGEPAGE_FLAG);
> >  }
> >
> > +#define THP_ALLOC_KHUGEPAGED (1 << 1)
> > +#define THP_ALLOC_CURRENT (1 << 2)
> > +static inline int bpf_thp_allocator(unsigned long vm_flags,
> > +                                  unsigned long tva_flags)
> > +{
> > +     return THP_ALLOC_KHUGEPAGED | THP_ALLOC_CURRENT;
>
> You dont use either vm_flags or tva_flags in this function?
> I am guessing you wanted to check if these bits are set here?
>
>
> But you dont seem to be setting these flags anywhere? I am guessing
> its in a future patch. If it is, I would move the setting of these bits
> here as its confusing to only see the check without knowing where its
>
> I feel this patch is broken and needs to be rewritten.

The `bpf_thp_allocator()` function serves as a placeholder that will
be implemented by a BPF program. The BPF implementation will use the
`@vm_flags` and `@tva_flags` parameters to determine whether a task
qualifies for THP allocation.

I see how this could be confusing.

> > +}
> > +
> >  static inline int highest_order(unsigned long orders)
> >  {
> >       return fls_long(orders) - 1;
> > @@ -290,6 +298,8 @@ unsigned long thp_vma_allowable_orders(struct vm_area_struct *vma,
> >       if ((tva_flags & TVA_ENFORCE_SYSFS) && vma_is_anonymous(vma)) {
> >               unsigned long mask = READ_ONCE(huge_anon_orders_always);
> >
> > +             if (!(bpf_thp_allocator(vm_flags, tva_flags) & THP_ALLOC_CURRENT))
> > +                     return 0;
>
> I am assuming that this is the point to check for allocation, but thp_vma_allowable_orders
> is not just used for allocation, its used for in other places as well, like hugepage_vma_revalidate
> and swap.

Agreed, some adjustments are necessary.

-- 
Regards
Yafang