Message-ID: <59c0c6b2-7112-4afd-8ea7-9e8bfaf86394@lucifer.local>
Date: Tue, 12 Aug 2025 20:06:34 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Hildenbrand <david@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        xen-devel@...ts.xenproject.org, linux-fsdevel@...r.kernel.org,
        nvdimm@...ts.linux.dev, linuxppc-dev@...ts.ozlabs.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Madhavan Srinivasan <maddy@...ux.ibm.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Nicholas Piggin <npiggin@...il.com>,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        Juergen Gross <jgross@...e.com>,
        Stefano Stabellini <sstabellini@...nel.org>,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Matthew Wilcox <willy@...radead.org>, Jan Kara <jack@...e.cz>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>,
        "Liam R. Howlett" <Liam.Howlett@...cle.com>,
        Vlastimil Babka <vbabka@...e.cz>, Mike Rapoport <rppt@...nel.org>,
        Suren Baghdasaryan <surenb@...gle.com>, Michal Hocko <mhocko@...e.com>,
        Zi Yan <ziy@...dia.com>, Baolin Wang <baolin.wang@...ux.alibaba.com>,
        Nico Pache <npache@...hat.com>, Ryan Roberts <ryan.roberts@....com>,
        Dev Jain <dev.jain@....com>, Barry Song <baohua@...nel.org>,
        Jann Horn <jannh@...gle.com>, Pedro Falcato <pfalcato@...e.de>,
        Hugh Dickins <hughd@...gle.com>, Oscar Salvador <osalvador@...e.de>,
        Lance Yang <lance.yang@...ux.dev>
Subject: Re: [PATCH v3 09/11] mm/memory: factor out common code from
 vm_normal_page_*()

On Mon, Aug 11, 2025 at 01:26:29PM +0200, David Hildenbrand wrote:
> Let's reduce the code duplication and factor out the non-pte/pmd related
> magic into __vm_normal_page().
>
> To keep it simpler, check the pfn against both zero folios, which
> shouldn't really make a difference.
>
> It's a good question if we can even hit the !CONFIG_ARCH_HAS_PTE_SPECIAL
> scenario in the PMD case in practice: but doesn't really matter, as
> it's now all unified in vm_normal_page_pfn().
>
> Add kerneldoc for all involved functions.
>
> Note that, as a side product, we now:
> * Support the find_special_page special thingy also for PMD
> * Don't check for is_huge_zero_pfn() anymore if we have
>   CONFIG_ARCH_HAS_PTE_SPECIAL and the PMD is not special. The
>   VM_WARN_ON_ONCE would catch any abuse
>
> No functional change intended.
>
> Reviewed-by: Oscar Salvador <osalvador@...e.de>
> Signed-off-by: David Hildenbrand <david@...hat.com>

Fantastic cleanup, thanks for refactoring with levels, this looks great!

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>

> ---
>  mm/memory.c | 186 ++++++++++++++++++++++++++++++----------------------
>  1 file changed, 109 insertions(+), 77 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index dc0107354d37b..78af3f243cee7 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -614,8 +614,14 @@ static void print_bad_page_map(struct vm_area_struct *vma,
>  #define print_bad_pte(vma, addr, pte, page) \
>  	print_bad_page_map(vma, addr, pte_val(pte), page, PGTABLE_LEVEL_PTE)
>
> -/*
> - * vm_normal_page -- This function gets the "struct page" associated with a pte.
> +/**
> + * __vm_normal_page() - Get the "struct page" associated with a page table entry.
> + * @vma: The VMA mapping the page table entry.
> + * @addr: The address where the page table entry is mapped.
> + * @pfn: The PFN stored in the page table entry.
> + * @special: Whether the page table entry is marked "special".
> + * @level: The page table level for error reporting purposes only.
> + * @entry: The page table entry value for error reporting purposes only.
>   *
>   * "Special" mappings do not wish to be associated with a "struct page" (either
>   * it doesn't exist, or it exists but they don't want to touch it). In this
> @@ -628,10 +634,10 @@ static void print_bad_page_map(struct vm_area_struct *vma,
>   * Selected page table walkers (such as GUP) can still identify mappings of the
>   * shared zero folios and work with the underlying "struct page".
>   *
> - * There are 2 broad cases. Firstly, an architecture may define a pte_special()
> - * pte bit, in which case this function is trivial. Secondly, an architecture
> - * may not have a spare pte bit, which requires a more complicated scheme,
> - * described below.
> + * There are 2 broad cases. Firstly, an architecture may define a "special"
> + * page table entry bit, such as pte_special(), in which case this function is
> + * trivial. Secondly, an architecture may not have a spare page table
> + * entry bit, which requires a more complicated scheme, described below.

OK cool, nice to have this here in one place!

>   *
>   * A raw VM_PFNMAP mapping (ie. one that is not COWed) is always considered a
>   * special mapping (even if there are underlying and valid "struct pages").
> @@ -664,63 +670,94 @@ static void print_bad_page_map(struct vm_area_struct *vma,
>   * don't have to follow the strict linearity rule of PFNMAP mappings in
>   * order to support COWable mappings.
>   *
> + * Return: Returns the "struct page" if this is a "normal" mapping. Returns
> + *	   NULL if this is a "special" mapping.
>   */
> -struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr,
> -			    pte_t pte)
> +static inline struct page *__vm_normal_page(struct vm_area_struct *vma,
> +		unsigned long addr, unsigned long pfn, bool special,
> +		unsigned long long entry, enum pgtable_level level)
>  {
> -	unsigned long pfn = pte_pfn(pte);
> -
>  	if (IS_ENABLED(CONFIG_ARCH_HAS_PTE_SPECIAL)) {
> -		if (likely(!pte_special(pte)))
> -			goto check_pfn;
> -		if (vma->vm_ops && vma->vm_ops->find_special_page)
> -			return vma->vm_ops->find_special_page(vma, addr);
> -		if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
> -			return NULL;
> -		if (is_zero_pfn(pfn))
> -			return NULL;
> -
> -		print_bad_pte(vma, addr, pte, NULL);
> -		return NULL;
> -	}
> -
> -	/* !CONFIG_ARCH_HAS_PTE_SPECIAL case follows: */
> -
> -	if (unlikely(vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP))) {
> -		if (vma->vm_flags & VM_MIXEDMAP) {
> -			if (!pfn_valid(pfn))
> +		if (unlikely(special)) {
> +			if (vma->vm_ops && vma->vm_ops->find_special_page)
> +				return vma->vm_ops->find_special_page(vma, addr);
> +			if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
>  				return NULL;
> -			if (is_zero_pfn(pfn))
> -				return NULL;
> -			goto out;
> -		} else {
> -			unsigned long off;
> -			off = (addr - vma->vm_start) >> PAGE_SHIFT;
> -			if (pfn == vma->vm_pgoff + off)
> -				return NULL;
> -			if (!is_cow_mapping(vma->vm_flags))
> +			if (is_zero_pfn(pfn) || is_huge_zero_pfn(pfn))

Yeah this works fine.

>  				return NULL;
> +
> +			print_bad_page_map(vma, addr, entry, NULL, level);

OK nice this is where the the print_bad_page_map() with level comes in handy.

> +			return NULL;
>  		}
> -	}
> +		/*
> +		 * With CONFIG_ARCH_HAS_PTE_SPECIAL, any special page table
> +		 * mappings (incl. shared zero folios) are marked accordingly.
> +		 */
> +	} else {
> +		if (unlikely(vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))) {
> +			if (vma->vm_flags & VM_MIXEDMAP) {
> +				/* If it has a "struct page", it's "normal". */
> +				if (!pfn_valid(pfn))
> +					return NULL;
> +			} else {
> +				unsigned long off = (addr - vma->vm_start) >> PAGE_SHIFT;
>
> -	if (is_zero_pfn(pfn))
> -		return NULL;
> +				/* Only CoW'ed anon folios are "normal". */
> +				if (pfn == vma->vm_pgoff + off)
> +					return NULL;
> +				if (!is_cow_mapping(vma->vm_flags))
> +					return NULL;
> +			}
> +		}
> +
> +		if (is_zero_pfn(pfn) || is_huge_zero_pfn(pfn))

Yeah this is fine too! This is all working out rather neatly! :)

> +			return NULL;
> +	}
>
> -check_pfn:
>  	if (unlikely(pfn > highest_memmap_pfn)) {
> -		print_bad_pte(vma, addr, pte, NULL);
> +		/* Corrupted page table entry. */
> +		print_bad_page_map(vma, addr, entry, NULL, level);
>  		return NULL;
>  	}
> -
>  	/*
>  	 * NOTE! We still have PageReserved() pages in the page tables.
> -	 * eg. VDSO mappings can cause them to exist.
> +	 * For example, VDSO mappings can cause them to exist.
>  	 */
> -out:
> -	VM_WARN_ON_ONCE(is_zero_pfn(pfn));
> +	VM_WARN_ON_ONCE(is_zero_pfn(pfn) || is_huge_zero_pfn(pfn));

And ACK on this as well.

>  	return pfn_to_page(pfn);
>  }
>
> +/**
> + * vm_normal_page() - Get the "struct page" associated with a PTE
> + * @vma: The VMA mapping the @pte.
> + * @addr: The address where the @pte is mapped.
> + * @pte: The PTE.
> + *
> + * Get the "struct page" associated with a PTE. See __vm_normal_page()
> + * for details on "normal" and "special" mappings.

Lovely.

> + *
> + * Return: Returns the "struct page" if this is a "normal" mapping. Returns
> + *	   NULL if this is a "special" mapping.
> + */
> +struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr,
> +			    pte_t pte)
> +{
> +	return __vm_normal_page(vma, addr, pte_pfn(pte), pte_special(pte),
> +				pte_val(pte), PGTABLE_LEVEL_PTE);

Nice and neat!

> +}
> +
> +/**
> + * vm_normal_folio() - Get the "struct folio" associated with a PTE
> + * @vma: The VMA mapping the @pte.
> + * @addr: The address where the @pte is mapped.
> + * @pte: The PTE.
> + *
> + * Get the "struct folio" associated with a PTE. See __vm_normal_page()
> + * for details on "normal" and "special" mappings.
> + *
> + * Return: Returns the "struct folio" if this is a "normal" mapping. Returns
> + *	   NULL if this is a "special" mapping.
> + */

Great, thanks for adding this! I especially like '*special*' :P

>  struct folio *vm_normal_folio(struct vm_area_struct *vma, unsigned long addr,
>  			    pte_t pte)
>  {
> @@ -732,42 +769,37 @@ struct folio *vm_normal_folio(struct vm_area_struct *vma, unsigned long addr,
>  }
>
>  #ifdef CONFIG_PGTABLE_HAS_HUGE_LEAVES
> +/**
> + * vm_normal_page_pmd() - Get the "struct page" associated with a PMD
> + * @vma: The VMA mapping the @pmd.
> + * @addr: The address where the @pmd is mapped.
> + * @pmd: The PMD.
> + *
> + * Get the "struct page" associated with a PTE. See __vm_normal_page()
> + * for details on "normal" and "special" mappings.
> + *
> + * Return: Returns the "struct page" if this is a "normal" mapping. Returns
> + *	   NULL if this is a "special" mapping.
> + */
>  struct page *vm_normal_page_pmd(struct vm_area_struct *vma, unsigned long addr,
>  				pmd_t pmd)
>  {
> -	unsigned long pfn = pmd_pfn(pmd);
> -
> -	if (unlikely(pmd_special(pmd)))
> -		return NULL;
> -
> -	if (unlikely(vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP))) {
> -		if (vma->vm_flags & VM_MIXEDMAP) {
> -			if (!pfn_valid(pfn))
> -				return NULL;
> -			goto out;
> -		} else {
> -			unsigned long off;
> -			off = (addr - vma->vm_start) >> PAGE_SHIFT;
> -			if (pfn == vma->vm_pgoff + off)
> -				return NULL;
> -			if (!is_cow_mapping(vma->vm_flags))
> -				return NULL;
> -		}
> -	}
> -
> -	if (is_huge_zero_pfn(pfn))
> -		return NULL;
> -	if (unlikely(pfn > highest_memmap_pfn))
> -		return NULL;
> -
> -	/*
> -	 * NOTE! We still have PageReserved() pages in the page tables.
> -	 * eg. VDSO mappings can cause them to exist.
> -	 */
> -out:
> -	return pfn_to_page(pfn);
> +	return __vm_normal_page(vma, addr, pmd_pfn(pmd), pmd_special(pmd),
> +				pmd_val(pmd), PGTABLE_LEVEL_PMD);

So much red... so much delight! :) this is great!

>  }
>
> +/**
> + * vm_normal_folio_pmd() - Get the "struct folio" associated with a PMD
> + * @vma: The VMA mapping the @pmd.
> + * @addr: The address where the @pmd is mapped.
> + * @pmd: The PMD.
> + *
> + * Get the "struct folio" associated with a PTE. See __vm_normal_page()
> + * for details on "normal" and "special" mappings.
> + *
> + * Return: Returns the "struct folio" if this is a "normal" mapping. Returns
> + *	   NULL if this is a "special" mapping.
> + */

This is great also!

>  struct folio *vm_normal_folio_pmd(struct vm_area_struct *vma,
>  				  unsigned long addr, pmd_t pmd)
>  {
> --
> 2.50.1
>

Powered by blists - more mailing lists