On Thu, Jul 31, 2025 at 08:20:18PM +0100, Usama Arif wrote:
[snip]
> >> 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.
> >
>
> 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]
> >> 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]
> >> @@ -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.
> But no strong opinion and can change the variable/macro args to tva_type if that
> is preferred.
>
> The diff over v2 after taking the review comments into account looks quite trivial:
>
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index b0ff54eee81c..bd4f9e6327e0 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -98,7 +98,7 @@ enum tva_type {
> TVA_SMAPS, /* Exposing "THPeligible:" in smaps. */
> TVA_PAGEFAULT, /* Serving a page fault. */
> TVA_KHUGEPAGED, /* Khugepaged collapse. */
> - TVA_FORCED_COLLAPSE, /* Forced collapse (i.e., MADV_COLLAPSE). */
> + TVA_FORCED_COLLAPSE, /* Forced collapse (e.g. MADV_COLLAPSE). */
> };
>
> #define thp_vma_allowable_order(vma, vm_flags, type, order) \
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 7a54b6f2a346..88cb6339e910 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -920,7 +920,7 @@ static int hugepage_vma_revalidate(struct mm_struct *mm, unsigned long address,
> struct collapse_control *cc)
> {
> struct vm_area_struct *vma;
> - enum tva_type tva_type = cc->is_khugepaged ? TVA_KHUGEPAGED :
> + enum tva_type type = cc->is_khugepaged ? TVA_KHUGEPAGED :
> TVA_FORCED_COLLAPSE;
>
> if (unlikely(hpage_collapse_test_exit_or_disable(mm)))
> @@ -932,7 +932,7 @@ static int hugepage_vma_revalidate(struct mm_struct *mm, unsigned long address,
>
> if (!thp_vma_suitable_order(vma, address, PMD_ORDER))
> return SCAN_ADDRESS_RANGE;
> - if (!thp_vma_allowable_order(vma, vma->vm_flags, tva_type, PMD_ORDER))
> + if (!thp_vma_allowable_order(vma, vma->vm_flags, type, PMD_ORDER))
> return SCAN_VMA_CHECK;
> /*
> * Anon VMA expected, the address may be unmapped then
> @@ -1532,8 +1532,7 @@ int collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr,
> * in the page cache with a single hugepage. If a mm were to fault-in
> * this memory (mapped by a suitably aligned VMA), we'd get the hugepage
> * and map it by a PMD, regardless of sysfs THP settings. As such, let's
> - * analogously elide sysfs THP settings here and pretend we are
> - * collapsing.
> + * analogously elide sysfs THP settings here and force collapse.
> */
> if (!thp_vma_allowable_order(vma, vma->vm_flags, TVA_FORCED_COLLAPSE, PMD_ORDER))
> return SCAN_VMA_CHECK;
Nice that's fine.
Cheers, Lorenzo
