linux-kernel - Re: [PATCH v2 2/5] mm/huge_memory: convert "tva_flags" to "enum tva_type" for thp_vma_allowable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <de360a8e-6bfd-4033-b388-1ffae039dadd@lucifer.local>
Date: Fri, 1 Aug 2025 11:12:40 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Usama Arif <usamaarif642@...il.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, david@...hat.com,
        linux-mm@...ck.org, linux-fsdevel@...r.kernel.org, corbet@....net,
        rppt@...nel.org, surenb@...gle.com, mhocko@...e.com,
        hannes@...xchg.org, baohua@...nel.org, shakeel.butt@...ux.dev,
        riel@...riel.com, ziy@...dia.com, laoar.shao@...il.com,
        dev.jain@....com, baolin.wang@...ux.alibaba.com, npache@...hat.com,
        Liam.Howlett@...cle.com, ryan.roberts@....com, vbabka@...e.cz,
        jannh@...gle.com, Arnd Bergmann <arnd@...db.de>, sj@...nel.org,
        linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
        kernel-team@...a.com
Subject: Re: [PATCH v2 2/5] mm/huge_memory: convert "tva_flags" to "enum
 tva_type" for thp_vma_allowable_order*()

On Thu, Jul 31, 2025 at 08:20:18PM +0100, Usama Arif wrote:
[snip]
> >> Signed-off-by: David Hildenbrand <david@...hat.com>
> >> Acked-by: Usama Arif <usamaarif642@...il.com>
> >> Signed-off-by: Usama Arif <usamaarif642@...il.com>
> >
> > 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