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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

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

? Or am I missing something here?

> +
> +out:
> +	rcu_read_unlock();
> +	return thp_orders;
> +}

Cheers, Lorenzo