On 31 Jul 2025, at 10:00, Lorenzo Stoakes wrote: > On Thu, Jul 31, 2025 at 01:27:19PM +0100, Usama Arif wrote: >> From: David Hildenbrand <david@xxxxxxxxxx> >> >> Describing the context through a type is much clearer, and good enough >> for our case. > > This is pretty bare bones. What context, what type? Under what > circumstances? > > This also is missing detail on the key difference here - that actually it > turns out we _don't_ need these to be flags, rather we can have _distinct_ > modes which are clearer. > > I'd say something like: > > when determining which THP orders are eligiible for a VMA mapping, > we have previously specified tva_flags, however it turns out it is > really not necessary to treat these as flags. > > Rather, we distinguish between distinct modes. > > The only case where we previously combined flags was with > TVA_ENFORCE_SYSFS, but we can avoid this by observing that this is > the default, except for MADV_COLLAPSE or an edge cases in > collapse_pte_mapped_thp() and hugepage_vma_revalidate(), and adding > a mode specifically for this case - TVA_FORCED_COLLAPSE. > > ... stuff about the different modes... > >> >> We have: >> * smaps handling for showing "THPeligible" >> * Pagefault handling >> * khugepaged handling >> * Forced collapse handling: primarily MADV_COLLAPSE, but one other odd case > > Can we actually state what this case is? I mean I guess a handwave in the > form of 'an edge case in collapse_pte_mapped_thp()' will do also. > > Hmm actually we do weird stuff with this so maybe just handwave. > > Like uprobes calls collapse_pte_mapped_thp()... :/ I'm not sure this 'If we > are here, we've succeeded in replacing all the native pages in the page > cache with a single hugepage.' comment is even correct. > > Anyway yeah, hand wave I guess... > >> >> Really, we want to ignore sysfs only when we are forcing a collapse >> through MADV_COLLAPSE, otherwise we want to enforce. > > I'd say 'ignoring this edge case, ...' > > I think the clearest thing might be to literally list the before/after > like: > > * TVA_SMAPS | TVA_ENFORCE_SYSFS -> TVA_SMAPS > * TVA_IN_PF | TVA_ENFORCE_SYSFS -> TVA_PAGEFAULT > * TVA_ENFORCE_SYSFS -> TVA_KHUGEPAGED > * 0 -> TVA_FORCED_COLLAPSE > >> >> With this change, we immediately know if we are in the forced collapse >> case, which will be valuable next. >> >> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx> >> Acked-by: Usama Arif <usamaarif642@xxxxxxxxx> >> Signed-off-by: Usama Arif <usamaarif642@xxxxxxxxx> > > Overall this is a great cleanup, some various nits however. > >> --- >> fs/proc/task_mmu.c | 4 ++-- >> include/linux/huge_mm.h | 30 ++++++++++++++++++------------ >> mm/huge_memory.c | 8 ++++---- >> mm/khugepaged.c | 18 +++++++++--------- >> mm/memory.c | 14 ++++++-------- >> 5 files changed, 39 insertions(+), 35 deletions(-) >> >> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c >> index 3d6d8a9f13fc..d440df7b3d59 100644 >> --- a/fs/proc/task_mmu.c >> +++ b/fs/proc/task_mmu.c >> @@ -1293,8 +1293,8 @@ static int show_smap(struct seq_file *m, void *v) >> __show_smap(m, &mss, false); >> >> seq_printf(m, "THPeligible: %8u\n", >> - !!thp_vma_allowable_orders(vma, vma->vm_flags, >> - TVA_SMAPS | TVA_ENFORCE_SYSFS, THP_ORDERS_ALL)); >> + !!thp_vma_allowable_orders(vma, vma->vm_flags, TVA_SMAPS, >> + THP_ORDERS_ALL)); > > This !! is so gross, wonder if we could have a bool wrapper. But not a big > deal. > > I also sort of _hate_ the smaps flag anyway, invoking this 'allowable > orders' thing just for smaps reporting with maybe some minor delta is just > odd. > > Something like `bool vma_has_thp_allowed_orders(struct vm_area_struct > *vma);` would be nicer. > > Anyway thoughts for another time... :) Or just bool thp_eligible = thp_vma_allowable_orders(...); seq_printf(m, "THPeligible: %8u\n", thp_eligible); Best Regards, Yan, Zi
