On Thu, Jul 31, 2025 at 08:20:18PM +0100, Usama Arif wrote:
[snip]

quoted

quoted

Signed-off-by: David Hildenbrand <redacted>
Acked-by: Usama Arif <redacted>
Signed-off-by: Usama Arif <redacted>

Overall this is a great cleanup, some various nits however.

Thanks for the feedback Lorenzo!

No problem :) I'm glad by the way we've found a solution that serves the
needs you and other's specified while not encountering the issues I raised
concerns with, the approach of extending this interface I think is a good
compromise.

I have modified the commit message to be:

    mm/huge_memory: convert "tva_flags" to "enum tva_type"

    When determining which THP orders are eligible 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.

    We have:
    * smaps handling for showing "THPeligible"
    * Pagefault handling
    * khugepaged handling
    * Forced collapse handling: primarily MADV_COLLAPSE, but also for
      an edge case in collapse_pte_mapped_thp()

    Ignoring the collapse_pte_mapped_thp edgecase, we only want to

I'd say 'disregarding the edge cases,' here as there's also
hugepage_vma_revalidate() above and being really really nitty we say
'ignore' a 2nd time below which reads less well.

    ignore sysfs only when we are forcing a collapse through

I'd say 'ignore sysfs settings' just to be clear.

    MADV_COLLAPSE, otherwise we want to enforce it, hence this patch
    does the following flag to enum conversions:

    * 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.

Other than nits above this looks really good, thanks!

[snip]

quoted

quoted

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 2b4ea5a2ce7d..85252b468f80 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c

[snip]

quoted

quoted

@@ -921,7 +920,8 @@ static int hugepage_vma_revalidate(struct mm_struct *mm, unsigned long address,
 				   struct collapse_control *cc)
 {
 	struct vm_area_struct *vma;
-	unsigned long tva_flags = cc->is_khugepaged ? TVA_ENFORCE_SYSFS : 0;
+	enum tva_type tva_type = cc->is_khugepaged ? TVA_KHUGEPAGED :
+				 TVA_FORCED_COLLAPSE;

This is great, this is so much clearer.

A nit though, I mean I come back to my 'type' vs 'tva_type' nit above, this
is inconsistent, so we should choose one approach and stick with it.

I dont exactly like the name "tva" (It has nothing to do with the fact it took
me more time than I would like to figure out that it meant THP VMA allowable :)),

Hey, dude, at least you worked it out, I had to ask :P

so what I will do is use "type" everywhere if that is ok?

Sure that's fine, it's not a big deal and I'd rather we just make it
consistent.

Nice that's fine.

Cheers, Lorenzo