[PATCH 0/3] mm/gup: fixups for hugetlb gup rework series

[PATCH 0/3] mm/gup: fixups for hugetlb gup rework series

[peterx]

From: Peter Xu <peterx@redhat.com>

Hi,

This is a small patchset that will fix two known issues that got reported
today on the previous hugetlb unification series on slow gup [1].

The first issue was reported by Ryan Roberts [2] on a test failure over
gup_longterm.  Patch 1-2 should fix it.  Tested with 32MB hugepages on
arm64 VM.

The second issue was reported by Nathan Chancellor [3] on a build issue
over loongson's defconfig (loongson3_defconfig).  It can be easily
reproduced with my own build setup [4], while patch 3 fixes it, and should
hopefully fix similar archs where pud_pfn() is not defined even if
pud_leaf() can return true.

For the 2nd issue, it's debatable whether it's an arch issue, because
logically speaking pud_pfn() and pud_leaf() should either be both defined
or both not.  But since the current fix will be the simplest and still safe
(I at least checked loongarch doesn't support either pud dax or pud
hugetlb), I think we can consider having this to unbreak the build first,
assuming this could also happen to other archs.

One note is that the last 2 patches should be squashed into corresponding
patch, while the 1st patch should be kept standalone.

Thanks,

[1] https://lore.kernel.org/r/20240327152332.950956-1-peterx@redhat.com
[2] https://lore.kernel.org/r/adfdd89b-ee56-4758-836e-c66a0be7de25@arm.com
[3] https://lore.kernel.org/r/ZgyKLLVZ4vN56uZE@x1n
[4] https://gitlab.com/peterx/lkb-harness/-/blob/main/config.json?ref_type=heads#L32

Peter Xu (3):
  mm: Allow anon exclusive check over hugetlb tail pages
  fixup! mm/gup: handle huge pmd for follow_pmd_mask()
  fixup! mm/arch: provide pud_pfn() fallback

 include/linux/page-flags.h |  8 +++++++-
 include/linux/pgtable.h    |  6 +++++-
 mm/gup.c                   |  3 ---
 mm/internal.h              | 10 ----------
 4 files changed, 12 insertions(+), 15 deletions(-)

-- 
2.44.0

[PATCH 1/3] mm: Allow anon exclusive check over hugetlb tail pages

[peterx]

From: Peter Xu <peterx@redhat.com>

PageAnonExclusive() used to forbid tail pages for hugetlbfs, as that used
to be called mostly in hugetlb specific paths and the head page was
guaranteed.

As we move forward towards merging hugetlb paths into generic mm, we may
start to pass in tail hugetlb pages (when with cont-pte/cont-pmd huge
pages) for such check.  Allow it to properly fetch the head, in which case
the anon-exclusiveness of the head will always represents the tail page.

There's already a sign of it when we look at the fast-gup which already
contain the hugetlb processing altogether: we used to have a specific
commit 5805192c7b72 ("mm/gup: handle cont-PTE hugetlb pages correctly in
gup_must_unshare() via GUP-fast") covering that area.  Now with this more
generic change, that can also go away.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/linux/page-flags.h |  8 +++++++-
 mm/internal.h              | 10 ----------
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 888353c209c0..225357f48a79 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -1095,7 +1095,13 @@ PAGEFLAG(Isolated, isolated, PF_ANY);
 static __always_inline int PageAnonExclusive(const struct page *page)
 {
 	VM_BUG_ON_PGFLAGS(!PageAnon(page), page);
-	VM_BUG_ON_PGFLAGS(PageHuge(page) && !PageHead(page), page);
+	/*
+	 * Allow the anon-exclusive check to work on hugetlb tail pages.
+	 * Here hugetlb pages will always guarantee the anon-exclusiveness
+	 * of the head page represents the tail pages.
+	 */
+	if (PageHuge(page) && !PageHead(page))
+		page = compound_head(page);
 	return test_bit(PG_anon_exclusive, &PF_ANY(page, 1)->flags);
 }
 
diff --git a/mm/internal.h b/mm/internal.h
index 9512de7398d5..87f6e4fd56a5 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -1259,16 +1259,6 @@ static inline bool gup_must_unshare(struct vm_area_struct *vma,
 	if (IS_ENABLED(CONFIG_HAVE_FAST_GUP))
 		smp_rmb();
 
-	/*
-	 * During GUP-fast we might not get called on the head page for a
-	 * hugetlb page that is mapped using cont-PTE, because GUP-fast does
-	 * not work with the abstracted hugetlb PTEs that always point at the
-	 * head page. For hugetlb, PageAnonExclusive only applies on the head
-	 * page (as it cannot be partially COW-shared), so lookup the head page.
-	 */
-	if (unlikely(!PageHead(page) && PageHuge(page)))
-		page = compound_head(page);
-
 	/*
 	 * Note that PageKsm() pages cannot be exclusive, and consequently,
 	 * cannot get pinned.
-- 
2.44.0

[PATCH 2/3] fixup! mm/gup: handle huge pmd for follow_pmd_mask()

[peterx]

From: Peter Xu <peterx@redhat.com>

Allow follow_pmd_mask() to take hugetlb tail pages.  The old warnings do
not help now as hugetlb now allows it to happen, so drop them.

Reported-by: Ryan Roberts <ryan.roberts@arm.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 mm/gup.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 91d70057aea0..d60b63fcfc82 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -775,8 +775,6 @@ static struct page *follow_huge_pmd(struct vm_area_struct *vma,
 	assert_spin_locked(pmd_lockptr(mm, pmd));
 
 	page = pmd_page(pmdval);
-	VM_BUG_ON_PAGE(!PageHead(page) && !is_zone_device_page(page), page);
-
 	if ((flags & FOLL_WRITE) &&
 	    !can_follow_write_pmd(pmdval, page, vma, flags))
 		return NULL;
@@ -805,7 +803,6 @@ static struct page *follow_huge_pmd(struct vm_area_struct *vma,
 
 	page += (addr & ~HPAGE_PMD_MASK) >> PAGE_SHIFT;
 	ctx->page_mask = HPAGE_PMD_NR - 1;
-	VM_BUG_ON_PAGE(!PageCompound(page) && !is_zone_device_page(page), page);
 
 	return page;
 }
-- 
2.44.0

[PATCH 3/3] fixup! mm/arch: provide pud_pfn() fallback

[peterx]

From: Peter Xu <peterx@redhat.com>

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/linux/pgtable.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
index fa8f92f6e2d7..0f4b2faa1d71 100644
--- a/include/linux/pgtable.h
+++ b/include/linux/pgtable.h
@@ -1882,9 +1882,13 @@ typedef unsigned int pgtbl_mod_mask;
  * code.  Now it happens too for pud_pfn (and can happen for larger
  * mappings too in the future; we're not there yet).  Instead of defining
  * it for all archs (like pmd_pfn), provide a fallback.
+ *
+ * Note that returning 0 here means any arch that didn't define this can
+ * get severely wrong when it hits a real pud leaf.  It's arch's
+ * responsibility to properly define it when a huge pud is possible.
  */
 #ifndef pud_pfn
-#define pud_pfn(x) ({ BUILD_BUG(); 0; })
+#define pud_pfn(x) 0
 #endif
 
 /*
-- 
2.44.0

Re: [PATCH 1/3] mm: Allow anon exclusive check over hugetlb tail pages

[David Hildenbrand]

On 03.04.24 03:32, peterx@redhat.com wrote:
> From: Peter Xu <peterx@redhat.com>
> 
> PageAnonExclusive() used to forbid tail pages for hugetlbfs, as that used
> to be called mostly in hugetlb specific paths and the head page was
> guaranteed.
> 
> As we move forward towards merging hugetlb paths into generic mm, we may
> start to pass in tail hugetlb pages (when with cont-pte/cont-pmd huge
> pages) for such check.  Allow it to properly fetch the head, in which case
> the anon-exclusiveness of the head will always represents the tail page.
> 
> There's already a sign of it when we look at the fast-gup which already

"GUP-fast" ;)

> contain the hugetlb processing altogether: we used to have a specific
> commit 5805192c7b72 ("mm/gup: handle cont-PTE hugetlb pages correctly in
> gup_must_unshare() via GUP-fast") covering that area.  Now with this more
> generic change, that can also go away.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   include/linux/page-flags.h |  8 +++++++-
>   mm/internal.h              | 10 ----------
>   2 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 888353c209c0..225357f48a79 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -1095,7 +1095,13 @@ PAGEFLAG(Isolated, isolated, PF_ANY);
>   static __always_inline int PageAnonExclusive(const struct page *page)
>   {
>   	VM_BUG_ON_PGFLAGS(!PageAnon(page), page);
> -	VM_BUG_ON_PGFLAGS(PageHuge(page) && !PageHead(page), page);
> +	/*
> +	 * Allow the anon-exclusive check to work on hugetlb tail pages.
> +	 * Here hugetlb pages will always guarantee the anon-exclusiveness
> +	 * of the head page represents the tail pages.
> +	 */
> +	if (PageHuge(page) && !PageHead(page))
> +		page = compound_head(page);
>   	return test_bit(PG_anon_exclusive, &PF_ANY(page, 1)->flags);
>   }
>   
> diff --git a/mm/internal.h b/mm/internal.h
> index 9512de7398d5..87f6e4fd56a5 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -1259,16 +1259,6 @@ static inline bool gup_must_unshare(struct vm_area_struct *vma,
>   	if (IS_ENABLED(CONFIG_HAVE_FAST_GUP))
>   		smp_rmb();
>   
> -	/*
> -	 * During GUP-fast we might not get called on the head page for a
> -	 * hugetlb page that is mapped using cont-PTE, because GUP-fast does
> -	 * not work with the abstracted hugetlb PTEs that always point at the
> -	 * head page. For hugetlb, PageAnonExclusive only applies on the head
> -	 * page (as it cannot be partially COW-shared), so lookup the head page.
> -	 */
> -	if (unlikely(!PageHead(page) && PageHuge(page)))
> -		page = compound_head(page);
> -
>   	/*
>   	 * Note that PageKsm() pages cannot be exclusive, and consequently,
>   	 * cannot get pinned.


Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 0/3] mm/gup: fixups for hugetlb gup rework series

[Ryan Roberts]

On 03/04/2024 02:32, peterx@redhat.com wrote:
> From: Peter Xu <peterx@redhat.com>
> 
> Hi,
> 
> This is a small patchset that will fix two known issues that got reported
> today on the previous hugetlb unification series on slow gup [1].
> 
> The first issue was reported by Ryan Roberts [2] on a test failure over
> gup_longterm.  Patch 1-2 should fix it.  Tested with 32MB hugepages on
> arm64 VM.
> 
> The second issue was reported by Nathan Chancellor [3] on a build issue
> over loongson's defconfig (loongson3_defconfig).  It can be easily
> reproduced with my own build setup [4], while patch 3 fixes it, and should
> hopefully fix similar archs where pud_pfn() is not defined even if
> pud_leaf() can return true.
> 
> For the 2nd issue, it's debatable whether it's an arch issue, because
> logically speaking pud_pfn() and pud_leaf() should either be both defined
> or both not.  But since the current fix will be the simplest and still safe
> (I at least checked loongarch doesn't support either pud dax or pud
> hugetlb), I think we can consider having this to unbreak the build first,
> assuming this could also happen to other archs.
> 
> One note is that the last 2 patches should be squashed into corresponding
> patch, while the 1st patch should be kept standalone.
> 
> Thanks,
> 
> [1] https://lore.kernel.org/r/20240327152332.950956-1-peterx@redhat.com
> [2] https://lore.kernel.org/r/adfdd89b-ee56-4758-836e-c66a0be7de25@arm.com
> [3] https://lore.kernel.org/r/ZgyKLLVZ4vN56uZE@x1n
> [4] https://gitlab.com/peterx/lkb-harness/-/blob/main/config.json?ref_type=heads#L32
> 
> Peter Xu (3):
>   mm: Allow anon exclusive check over hugetlb tail pages
>   fixup! mm/gup: handle huge pmd for follow_pmd_mask()
>   fixup! mm/arch: provide pud_pfn() fallback
> 
>  include/linux/page-flags.h |  8 +++++++-
>  include/linux/pgtable.h    |  6 +++++-
>  mm/gup.c                   |  3 ---
>  mm/internal.h              | 10 ----------
>  4 files changed, 12 insertions(+), 15 deletions(-)
> 

With these applied, gup_longterm is now passing for me on arm64. So for the series:

Tested-by: Ryan Roberts <ryan.roberts@arm.com>

Re: [PATCH 1/3] mm: Allow anon exclusive check over hugetlb tail pages

[Matthew Wilcox]

On Tue, Apr 02, 2024 at 09:32:47PM -0400, peterx@redhat.com wrote:
> +++ b/include/linux/page-flags.h
> @@ -1095,7 +1095,13 @@ PAGEFLAG(Isolated, isolated, PF_ANY);
>  static __always_inline int PageAnonExclusive(const struct page *page)
>  {
>  	VM_BUG_ON_PGFLAGS(!PageAnon(page), page);
> -	VM_BUG_ON_PGFLAGS(PageHuge(page) && !PageHead(page), page);
> +	/*
> +	 * Allow the anon-exclusive check to work on hugetlb tail pages.
> +	 * Here hugetlb pages will always guarantee the anon-exclusiveness
> +	 * of the head page represents the tail pages.
> +	 */
> +	if (PageHuge(page) && !PageHead(page))
> +		page = compound_head(page);

I think this should be written as:

	/*
	 * HugeTLB stores this information on the head page; THP keeps
	 * it per page
	 */
	if (PageHuge(page))
		page = compound_head(page);

>  	return test_bit(PG_anon_exclusive, &PF_ANY(page, 1)->flags);
>  }

Re: [PATCH 1/3] mm: Allow anon exclusive check over hugetlb tail pages

[Peter Xu]

On Thu, Apr 04, 2024 at 01:05:57AM +0100, Matthew Wilcox wrote:
> On Tue, Apr 02, 2024 at 09:32:47PM -0400, peterx@redhat.com wrote:
> > +++ b/include/linux/page-flags.h
> > @@ -1095,7 +1095,13 @@ PAGEFLAG(Isolated, isolated, PF_ANY);
> >  static __always_inline int PageAnonExclusive(const struct page *page)
> >  {
> >  	VM_BUG_ON_PGFLAGS(!PageAnon(page), page);
> > -	VM_BUG_ON_PGFLAGS(PageHuge(page) && !PageHead(page), page);
> > +	/*
> > +	 * Allow the anon-exclusive check to work on hugetlb tail pages.
> > +	 * Here hugetlb pages will always guarantee the anon-exclusiveness
> > +	 * of the head page represents the tail pages.
> > +	 */
> > +	if (PageHuge(page) && !PageHead(page))
> > +		page = compound_head(page);
> 
> I think this should be written as:
> 
> 	/*
> 	 * HugeTLB stores this information on the head page; THP keeps
> 	 * it per page
> 	 */

This comment does look clean and cleaner indeed.  And yes, mention THP can
be helpful too.

> 	if (PageHuge(page))
> 		page = compound_head(page);

I would think PageHead() check would help us to avoid compound_head() on
heads, which should still be the majority cases iiuc (assuming page->flags
is already around in the cache anyway).  I've no strong opinion though, as
I can hardly tell a difference in reality.

> 
> >  	return test_bit(PG_anon_exclusive, &PF_ANY(page, 1)->flags);
> >  }
> 

Thanks,

-- 
Peter Xu

Re: [PATCH 1/3] mm: Allow anon exclusive check over hugetlb tail pages

[Matthew Wilcox]

On Thu, Apr 04, 2024 at 09:45:58AM -0400, Peter Xu wrote:
> On Thu, Apr 04, 2024 at 01:05:57AM +0100, Matthew Wilcox wrote:
> > 	if (PageHuge(page))
> > 		page = compound_head(page);
> 
> I would think PageHead() check would help us to avoid compound_head() on
> heads, which should still be the majority cases iiuc (assuming page->flags
> is already around in the cache anyway).  I've no strong opinion though, as
> I can hardly tell a difference in reality.

compound_head() includes a check for PageHead().  Adding the check just
makes things slower.

Re: [PATCH 1/3] mm: Allow anon exclusive check over hugetlb tail pages

[Peter Xu]

On Thu, Apr 04, 2024 at 03:06:25PM +0100, Matthew Wilcox wrote:
> On Thu, Apr 04, 2024 at 09:45:58AM -0400, Peter Xu wrote:
> > On Thu, Apr 04, 2024 at 01:05:57AM +0100, Matthew Wilcox wrote:
> > > 	if (PageHuge(page))
> > > 		page = compound_head(page);
> > 
> > I would think PageHead() check would help us to avoid compound_head() on
> > heads, which should still be the majority cases iiuc (assuming page->flags
> > is already around in the cache anyway).  I've no strong opinion though, as
> > I can hardly tell a difference in reality.
> 
> compound_head() includes a check for PageHead().  Adding the check just
> makes things slower.

They check different fields (compound_head, offset 0x8 for the former).
Again I'm ok with either way to go and I don't expect measurable
difference.. so if you prefer this I'm happy with it too.

Andrew, could you help update with Matthew's fixup?  The comment is
definitely better than what I wrote in all cases.

Thanks,

-- 
Peter Xu

Re: [PATCH 1/3] mm: Allow anon exclusive check over hugetlb tail pages

[Andrew Morton]

On Thu, 4 Apr 2024 10:21:21 -0400 Peter Xu <peterx@redhat.com> wrote:

> Andrew, could you help update with Matthew's fixup?  The comment is
> definitely better than what I wrote in all cases.


From: Andrew Morton <akpm@linux-foundation.org>
Subject: mm-allow-anon-exclusive-check-over-hugetlb-tail-pages-fix
Date: Thu Apr  4 01:27:47 PM PDT 2024

simplify PageAnonExclusive(), per Matthew

Link: https://lkml.kernel.org/r/Zg3u5Sh9EbbYPhaI@casper.infradead.org
Cc: David Hildenbrand <david@redhat.com>
Cc: Huacai Chen <chenhuacai@kernel.org>
Cc: Jason Gunthorpe <jgg@nvidia.com>
Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Peter Xu <peterx@redhat.com>
Cc: Ryan Roberts <ryan.roberts@arm.com>
Cc: WANG Xuerui <kernel@xen0n.name>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/page-flags.h |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

--- a/include/linux/page-flags.h~mm-allow-anon-exclusive-check-over-hugetlb-tail-pages-fix
+++ a/include/linux/page-flags.h
@@ -1096,11 +1096,10 @@ static __always_inline int PageAnonExclu
 {
 	VM_BUG_ON_PGFLAGS(!PageAnon(page), page);
 	/*
-	 * Allow the anon-exclusive check to work on hugetlb tail pages.
-	 * Here hugetlb pages will always guarantee the anon-exclusiveness
-	 * of the head page represents the tail pages.
+	 * HugeTLB stores this information on the head page; THP keeps it per
+	 * page
 	 */
-	if (PageHuge(page) && !PageHead(page))
+	if (PageHuge(page))
 		page = compound_head(page);
 	return test_bit(PG_anon_exclusive, &PF_ANY(page, 1)->flags);
 }
_

Re: [PATCH 1/3] mm: Allow anon exclusive check over hugetlb tail pages

[Peter Xu]

On Thu, Apr 04, 2024 at 01:31:37PM -0700, Andrew Morton wrote:
> On Thu, 4 Apr 2024 10:21:21 -0400 Peter Xu <peterx@redhat.com> wrote:
> 
> > Andrew, could you help update with Matthew's fixup?  The comment is
> > definitely better than what I wrote in all cases.
> 
> From: Andrew Morton <akpm@linux-foundation.org>
> Subject: mm-allow-anon-exclusive-check-over-hugetlb-tail-pages-fix
> Date: Thu Apr  4 01:27:47 PM PDT 2024
> 
> simplify PageAnonExclusive(), per Matthew

Thank you!

-- 
Peter Xu