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@xxxxxxxxxx> 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@xxxxxxxxxx> 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.
>
> 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?
>
> >
> > >
> > > 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.
Thanks!
>
> --
> Regards
> Yafang
Cheers, Lorenzo
