Message-ID: <511317b9-7155-40c7-91e0-faf134622b9d@lucifer.local>
Date: Tue, 12 Aug 2025 19:14:54 +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 05/11] mm/huge_memory: mark PMD mappings of the huge
 zero folio special

On Mon, Aug 11, 2025 at 01:26:25PM +0200, David Hildenbrand wrote:
> The huge zero folio is refcounted (+mapcounted -- is that a word?)
> differently than "normal" folios, similarly (but different) to the ordinary
> shared zeropage.
>
> For this reason, we special-case these pages in
> vm_normal_page*/vm_normal_folio*, and only allow selected callers to
> still use them (e.g., GUP can still take a reference on them).

Hm, interestingly in gup_fast_pmd_leaf() we explicitly check pmd_special(),
so surely setting the zero huge pmd special will change behaviour there?

But I guess this is actually _more_ correct as it's not really sensible to
grab the huge zero PMD page.

Then again, follow_huge_pmd() _will_, afaict.

I see the GUP fast change was introduced by commit ae3c99e650da ("mm/gup:
detect huge pfnmap entries in gup-fast") so was specifically intended for
pfnmap not the zero page.

>
> vm_normal_page_pmd() already filters out the huge zero folio, to
> indicate it a special (return NULL). However, so far we are not making
> use of pmd_special() on architectures that support it
> (CONFIG_ARCH_HAS_PTE_SPECIAL), like we would with the ordinary shared
> zeropage.
>
> Let's mark PMD mappings of the huge zero folio similarly as special, so we
> can avoid the manual check for the huge zero folio with
> CONFIG_ARCH_HAS_PTE_SPECIAL next, and only perform the check on
> !CONFIG_ARCH_HAS_PTE_SPECIAL.
>
> In copy_huge_pmd(), where we have a manual pmd_special() check to handle
> PFNMAP, we have to manually rule out the huge zero folio. That code
> needs a serious cleanup, but that's something for another day.
>
> While at it, update the doc regarding the shared zero folios.
>
> No functional change intended: vm_normal_page_pmd() still returns NULL
> when it encounters the huge zero folio.
>
> Reviewed-by: Oscar Salvador <osalvador@...e.de>
> Signed-off-by: David Hildenbrand <david@...hat.com>

I R-b this before, and Wei did also, did you drop because of changes?

Anyway, apart from query about GUP-fast above, this LGTM so:

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

> ---
>  mm/huge_memory.c |  8 ++++++--
>  mm/memory.c      | 15 ++++++++++-----
>  2 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index ec89e0607424e..58bac83e7fa31 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1309,6 +1309,7 @@ static void set_huge_zero_folio(pgtable_t pgtable, struct mm_struct *mm,
>  {
>  	pmd_t entry;
>  	entry = folio_mk_pmd(zero_folio, vma->vm_page_prot);
> +	entry = pmd_mkspecial(entry);
>  	pgtable_trans_huge_deposit(mm, pmd, pgtable);
>  	set_pmd_at(mm, haddr, pmd, entry);
>  	mm_inc_nr_ptes(mm);
> @@ -1418,7 +1419,9 @@ static vm_fault_t insert_pmd(struct vm_area_struct *vma, unsigned long addr,
>  	if (fop.is_folio) {
>  		entry = folio_mk_pmd(fop.folio, vma->vm_page_prot);
>
> -		if (!is_huge_zero_folio(fop.folio)) {
> +		if (is_huge_zero_folio(fop.folio)) {
> +			entry = pmd_mkspecial(entry);
> +		} else {
>  			folio_get(fop.folio);
>  			folio_add_file_rmap_pmd(fop.folio, &fop.folio->page, vma);
>  			add_mm_counter(mm, mm_counter_file(fop.folio), HPAGE_PMD_NR);
> @@ -1643,7 +1646,8 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>  	int ret = -ENOMEM;
>
>  	pmd = pmdp_get_lockless(src_pmd);
> -	if (unlikely(pmd_present(pmd) && pmd_special(pmd))) {
> +	if (unlikely(pmd_present(pmd) && pmd_special(pmd) &&
> +		     !is_huge_zero_pmd(pmd))) {

OK yeah this is new I see from cover letter + ranged-diff.

Yeah this is important actually wow, as otherwise the is_huge_zero_pmd()
branch will not be executed.

Good spot!

>  		dst_ptl = pmd_lock(dst_mm, dst_pmd);
>  		src_ptl = pmd_lockptr(src_mm, src_pmd);
>  		spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
> diff --git a/mm/memory.c b/mm/memory.c
> index 0ba4f6b718471..626caedce35e0 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -555,7 +555,14 @@ static void print_bad_pte(struct vm_area_struct *vma, unsigned long addr,
>   *
>   * "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
> - * case, NULL is returned here. "Normal" mappings do have a struct page.
> + * case, NULL is returned here. "Normal" mappings do have a struct page and
> + * are ordinarily refcounted.
> + *
> + * Page mappings of the shared zero folios are always considered "special", as
> + * they are not ordinarily refcounted: neither the refcount nor the mapcount
> + * of these folios is adjusted when mapping them into user page tables.
> + * Selected page table walkers (such as GUP) can still identify mappings of the
> + * shared zero folios and work with the underlying "struct page".

Thanks for this.

>   *
>   * 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
> @@ -585,9 +592,8 @@ static void print_bad_pte(struct vm_area_struct *vma, unsigned long addr,
>   *
>   * VM_MIXEDMAP mappings can likewise contain memory with or without "struct
>   * page" backing, however the difference is that _all_ pages with a struct
> - * page (that is, those where pfn_valid is true) are refcounted and considered
> - * normal pages by the VM. The only exception are zeropages, which are
> - * *never* refcounted.
> + * page (that is, those where pfn_valid is true, except the shared zero
> + * folios) are refcounted and considered normal pages by the VM.
>   *
>   * The disadvantage is that pages are refcounted (which can be slower and
>   * simply not an option for some PFNMAP users). The advantage is that we
> @@ -667,7 +673,6 @@ struct page *vm_normal_page_pmd(struct vm_area_struct *vma, unsigned long addr,
>  {
>  	unsigned long pfn = pmd_pfn(pmd);
>
> -	/* Currently it's only used for huge pfnmaps */
>  	if (unlikely(pmd_special(pmd)))
>  		return NULL;
>
> --
> 2.50.1
>

Powered by blists - more mailing lists