Fix some issues when looking up hugetlb page

On some architectures (like ARM64), it can support CONT-PTE/PMD size hugetlb, which means it can support not only PMD/PUD size hugetlb (2M and 1G), but also CONT-PTE/PMD size(64K and 32M) if a 4K page size specified.

So when looking up a CONT-PTE size hugetlb page by follow_page(), it will use pte_offset_map_lock() to get the pte entry lock for the CONT-PTE size hugetlb in follow_page_pte(). However this pte entry lock is incorrect for the CONT-PTE size hugetlb, since we should use huge_pte_lock() to get the correct lock, which is mm->page_table_lock.

That means the pte entry of the CONT-PTE size hugetlb under current pte lock is unstable in follow_page_pte(), we can continue to migrate or poison the pte entry of the CONT-PTE size hugetlb, which can cause some potential race issues, and following pte_xxx() validation is also unstable in follow_page_pte(), even though they are under the 'pte lock'.

Moreover we should use huge_ptep_get() to get the pte entry value of the CONT-PTE size hugetlb, which already takes into account the subpages' dirty or young bits in case we missed the dirty or young state of the CONT-PTE size hugetlb.

To fix above issues, introducing a new helper follow_huge_pte() to look up a CONT-PTE size hugetlb page, which uses huge_pte_lock() to get the correct pte entry lock to make the pte entry stable, as well as supporting non-present pte handling.

Signed-off-by Baolin Wang <[email protected]>

---
 include/linux/hugetlb.h |  8 ++++++++
 mm/gup.c                | 11 ++++++++++
 mm/hugetlb.c            | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+)

include/linux/hugetlb.h +8 -0

index 3ec981a..d491138 100644
@@ -207,6 +207,8 @@ struct page *follow_huge_addr(struct mm_struct *mm, unsigned long address,
 struct page *follow_huge_pd(struct vm_area_struct *vma,
 			    unsigned long address, hugepd_t hpd,
 			    int flags, int pdshift);
+struct page *follow_huge_pte(struct vm_area_struct *vma, unsigned long address,
+			     pmd_t *pmd, int flags);
 struct page *follow_huge_pmd(struct mm_struct *mm, unsigned long address,
 				pmd_t *pmd, int flags);
 struct page *follow_huge_pud(struct mm_struct *mm, unsigned long address,

@@ -312,6 +314,12 @@ static inline struct page *follow_huge_pd(struct vm_area_struct *vma,
 	return NULL;
 }
 
+static inline struct page *follow_huge_pte(struct vm_area_struct *vma,
+				unsigned long address, pmd_t *pmd, int flags)
+{
+	return NULL;
+}
+
 static inline struct page *follow_huge_pmd(struct mm_struct *mm,
 				unsigned long address, pmd_t *pmd, int flags)
 {

mm/gup.c +11 -0

index 3b656b7..87a94f5 100644
@@ -534,6 +534,17 @@ static struct page *follow_page_pte(struct vm_area_struct *vma,
 	if (unlikely(pmd_bad(*pmd)))
 		return no_page_table(vma, flags);
 
+	/*
+	 * Considering PTE level hugetlb, like continuous-PTE hugetlb on
+	 * ARM64 architecture.
+	 */
+	if (is_vm_hugetlb_page(vma)) {
+		page = follow_huge_pte(vma, address, pmd, flags);
+		if (page)
+			return page;
+		return no_page_table(vma, flags);
+	}
+
 	ptep = pte_offset_map_lock(mm, pmd, address, &ptl);
 	pte = *ptep;
 	if (!pte_present(pte)) {

mm/hugetlb.c +46 -0

index 6c00ba1..cf742d1 100644
@@ -6981,6 +6981,59 @@ struct page * __weak
 	return NULL;
 }
 
+/* Support looking up a CONT-PTE size hugetlb page. */
+struct page * __weak
+follow_huge_pte(struct vm_area_struct *vma, unsigned long address,
+		pmd_t *pmd, int flags)
+{
+	struct mm_struct *mm = vma->vm_mm;
+	struct hstate *hstate = hstate_vma(vma);
+	unsigned long size = huge_page_size(hstate);
+	struct page *page = NULL;
+	spinlock_t *ptl;
+	pte_t *ptep, pte;
+
+	/*
+	 * FOLL_PIN is not supported for follow_page(). Ordinary GUP goes via
+	 * follow_hugetlb_page().
+	 */
+	if (WARN_ON_ONCE(flags & FOLL_PIN))
+		return NULL;
+
+	ptep = huge_pte_offset(mm, address, size);
+	if (!ptep)
+		return NULL;
+
+retry:
+	ptl = huge_pte_lock(hstate, mm, ptep);
+	pte = huge_ptep_get(ptep);
+	if (pte_present(pte)) {
+		page = pte_page(pte);
+		if (WARN_ON_ONCE(!try_grab_page(page, flags))) {
+			page = NULL;
+			goto out;
+		}
+	} else {
+		if (!(flags & FOLL_MIGRATION)) {
+			page = NULL;
+			goto out;
+		}
+
+		if (is_hugetlb_entry_migration(pte)) {
+			spin_unlock(ptl);
+			__migration_entry_wait_huge(ptep, ptl);
+			goto retry;
+		}
+		/*
+		 * hwpoisoned entry is treated as no_page_table in
+		 * follow_page_mask().

Quoted 136 lines · Baolin Wang

On some architectures (like ARM64), it can support CONT-PTE/PMD size hugetlb, which means it can support not only PMD/PUD size hugetlb (2M and 1G), but also CONT-PTE/PMD size(64K and 32M) if a 4K page size specified.

So when looking up a CONT-PTE size hugetlb page by follow_page(), it will use pte_offset_map_lock() to get the pte entry lock for the CONT-PTE size hugetlb in follow_page_pte(). However this pte entry lock is incorrect for the CONT-PTE size hugetlb, since we should use huge_pte_lock() to get the correct lock, which is mm->page_table_lock.

That means the pte entry of the CONT-PTE size hugetlb under current pte lock is unstable in follow_page_pte(), we can continue to migrate or poison the pte entry of the CONT-PTE size hugetlb, which can cause some potential race issues, and following pte_xxx() validation is also unstable in follow_page_pte(), even though they are under the 'pte lock'.

Moreover we should use huge_ptep_get() to get the pte entry value of the CONT-PTE size hugetlb, which already takes into account the subpages' dirty or young bits in case we missed the dirty or young state of the CONT-PTE size hugetlb.

To fix above issues, introducing a new helper follow_huge_pte() to look up a CONT-PTE size hugetlb page, which uses huge_pte_lock() to get the correct pte entry lock to make the pte entry stable, as well as supporting non-present pte handling.

Signed-off-by Baolin Wang <[email protected]>

---
  include/linux/hugetlb.h |  8 ++++++++
  mm/gup.c                | 11 ++++++++++
  mm/hugetlb.c            | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 72 insertions(+)

include/linux/hugetlb.h +8 -0

index 3ec981a..d491138 100644
@@ -207,6 +207,8 @@ struct page *follow_huge_addr(struct mm_struct *mm, unsigned long address,
  struct page *follow_huge_pd(struct vm_area_struct *vma,
  			    unsigned long address, hugepd_t hpd,
  			    int flags, int pdshift);
+struct page *follow_huge_pte(struct vm_area_struct *vma, unsigned long address,
+			     pmd_t *pmd, int flags);
  struct page *follow_huge_pmd(struct mm_struct *mm, unsigned long address,
  				pmd_t *pmd, int flags);
  struct page *follow_huge_pud(struct mm_struct *mm, unsigned long address,

@@ -312,6 +314,12 @@ static inline struct page *follow_huge_pd(struct vm_area_struct *vma,
  	return NULL;
  }
 
+static inline struct page *follow_huge_pte(struct vm_area_struct *vma,
+				unsigned long address, pmd_t *pmd, int flags)
+{
+	return NULL;
+}
+
  static inline struct page *follow_huge_pmd(struct mm_struct *mm,
  				unsigned long address, pmd_t *pmd, int flags)
  {

mm/gup.c +11 -0

index 3b656b7..87a94f5 100644
@@ -534,6 +534,17 @@ static struct page *follow_page_pte(struct vm_area_struct *vma,
  	if (unlikely(pmd_bad(*pmd)))
  		return no_page_table(vma, flags);
 
+	/*
+	 * Considering PTE level hugetlb, like continuous-PTE hugetlb on
+	 * ARM64 architecture.
+	 */
+	if (is_vm_hugetlb_page(vma)) {
+		page = follow_huge_pte(vma, address, pmd, flags);
+		if (page)
+			return page;
+		return no_page_table(vma, flags);
+	}
+
  	ptep = pte_offset_map_lock(mm, pmd, address, &ptl);
  	pte = *ptep;
  	if (!pte_present(pte)) {

mm/hugetlb.c +46 -0

index 6c00ba1..cf742d1 100644
@@ -6981,6 +6981,59 @@ struct page * __weak
  	return NULL;
  }
 
+/* Support looking up a CONT-PTE size hugetlb page. */
+struct page * __weak
+follow_huge_pte(struct vm_area_struct *vma, unsigned long address,
+		pmd_t *pmd, int flags)
+{
+	struct mm_struct *mm = vma->vm_mm;
+	struct hstate *hstate = hstate_vma(vma);
+	unsigned long size = huge_page_size(hstate);
+	struct page *page = NULL;
+	spinlock_t *ptl;
+	pte_t *ptep, pte;
+
+	/*
+	 * FOLL_PIN is not supported for follow_page(). Ordinary GUP goes via
+	 * follow_hugetlb_page().
+	 */
+	if (WARN_ON_ONCE(flags & FOLL_PIN))
+		return NULL;
+
+	ptep = huge_pte_offset(mm, address, size);
+	if (!ptep)
+		return NULL;
+
+retry:
+	ptl = huge_pte_lock(hstate, mm, ptep);
+	pte = huge_ptep_get(ptep);
+	if (pte_present(pte)) {
+		page = pte_page(pte);
+		if (WARN_ON_ONCE(!try_grab_page(page, flags))) {
+			page = NULL;
+			goto out;
+		}
+	} else {
+		if (!(flags & FOLL_MIGRATION)) {
+			page = NULL;
+			goto out;
+		}
+
+		if (is_hugetlb_entry_migration(pte)) {
+			spin_unlock(ptl);
+			__migration_entry_wait_huge(ptep, ptl);
+			goto retry;
+		}
+		/*
+		 * hwpoisoned entry is treated as no_page_table in
+		 * follow_page_mask().

mm/hugetlb.c struct page * __weak · line 7039

Quoted 10 lines

mm/hugetlb.c +7 -0

+		 */
+	}
+out:
+	spin_unlock(ptl);
+	return page;
+}
+
  struct page * __weak
  follow_huge_pmd(struct mm_struct *mm, unsigned long address,
  		pmd_t *pmd, int flags)

Can someone explain why:
* follow_page() goes via follow_page_mask() for hugetlb
* __get_user_pages() goes via follow_hugetlb_page() and never via follow_page_mask() for hugetlb?

IOW, why can't we make follow_page_mask() just not handle hugetlb and route everything via follow_hugetlb_page() -- we primarily only have to teach it to not trigger faults.

What's the reason that this hugetlb code has to be overly complicated?

Quoted 154 lines · David Hildenbrand

Quoted 136 lines · Baolin Wang

On some architectures (like ARM64), it can support CONT-PTE/PMD size hugetlb, which means it can support not only PMD/PUD size hugetlb (2M and 1G), but also CONT-PTE/PMD size(64K and 32M) if a 4K page size specified.

So when looking up a CONT-PTE size hugetlb page by follow_page(), it will use pte_offset_map_lock() to get the pte entry lock for the CONT-PTE size hugetlb in follow_page_pte(). However this pte entry lock is incorrect for the CONT-PTE size hugetlb, since we should use huge_pte_lock() to get the correct lock, which is mm->page_table_lock.

That means the pte entry of the CONT-PTE size hugetlb under current pte lock is unstable in follow_page_pte(), we can continue to migrate or poison the pte entry of the CONT-PTE size hugetlb, which can cause some potential race issues, and following pte_xxx() validation is also unstable in follow_page_pte(), even though they are under the 'pte lock'.

Moreover we should use huge_ptep_get() to get the pte entry value of the CONT-PTE size hugetlb, which already takes into account the subpages' dirty or young bits in case we missed the dirty or young state of the CONT-PTE size hugetlb.

To fix above issues, introducing a new helper follow_huge_pte() to look up a CONT-PTE size hugetlb page, which uses huge_pte_lock() to get the correct pte entry lock to make the pte entry stable, as well as supporting non-present pte handling.

Signed-off-by Baolin Wang <[email protected]>

---
  include/linux/hugetlb.h |  8 ++++++++
  mm/gup.c                | 11 ++++++++++
  mm/hugetlb.c            | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 72 insertions(+)

include/linux/hugetlb.h +8 -0

index 3ec981a..d491138 100644
@@ -207,6 +207,8 @@ struct page *follow_huge_addr(struct mm_struct *mm, unsigned long address,
  struct page *follow_huge_pd(struct vm_area_struct *vma,
  			    unsigned long address, hugepd_t hpd,
  			    int flags, int pdshift);
+struct page *follow_huge_pte(struct vm_area_struct *vma, unsigned long address,
+			     pmd_t *pmd, int flags);
  struct page *follow_huge_pmd(struct mm_struct *mm, unsigned long address,
  				pmd_t *pmd, int flags);
  struct page *follow_huge_pud(struct mm_struct *mm, unsigned long address,

@@ -312,6 +314,12 @@ static inline struct page *follow_huge_pd(struct vm_area_struct *vma,
  	return NULL;
  }
 
+static inline struct page *follow_huge_pte(struct vm_area_struct *vma,
+				unsigned long address, pmd_t *pmd, int flags)
+{
+	return NULL;
+}
+
  static inline struct page *follow_huge_pmd(struct mm_struct *mm,
  				unsigned long address, pmd_t *pmd, int flags)
  {

mm/gup.c +11 -0

index 3b656b7..87a94f5 100644
@@ -534,6 +534,17 @@ static struct page *follow_page_pte(struct vm_area_struct *vma,
  	if (unlikely(pmd_bad(*pmd)))
  		return no_page_table(vma, flags);
 
+	/*
+	 * Considering PTE level hugetlb, like continuous-PTE hugetlb on
+	 * ARM64 architecture.
+	 */
+	if (is_vm_hugetlb_page(vma)) {
+		page = follow_huge_pte(vma, address, pmd, flags);
+		if (page)
+			return page;
+		return no_page_table(vma, flags);
+	}
+
  	ptep = pte_offset_map_lock(mm, pmd, address, &ptl);
  	pte = *ptep;
  	if (!pte_present(pte)) {

mm/hugetlb.c +46 -0

index 6c00ba1..cf742d1 100644
@@ -6981,6 +6981,59 @@ struct page * __weak
  	return NULL;
  }
 
+/* Support looking up a CONT-PTE size hugetlb page. */
+struct page * __weak
+follow_huge_pte(struct vm_area_struct *vma, unsigned long address,
+		pmd_t *pmd, int flags)
+{
+	struct mm_struct *mm = vma->vm_mm;
+	struct hstate *hstate = hstate_vma(vma);
+	unsigned long size = huge_page_size(hstate);
+	struct page *page = NULL;
+	spinlock_t *ptl;
+	pte_t *ptep, pte;
+
+	/*
+	 * FOLL_PIN is not supported for follow_page(). Ordinary GUP goes via
+	 * follow_hugetlb_page().
+	 */
+	if (WARN_ON_ONCE(flags & FOLL_PIN))
+		return NULL;
+
+	ptep = huge_pte_offset(mm, address, size);
+	if (!ptep)
+		return NULL;
+
+retry:
+	ptl = huge_pte_lock(hstate, mm, ptep);
+	pte = huge_ptep_get(ptep);
+	if (pte_present(pte)) {
+		page = pte_page(pte);
+		if (WARN_ON_ONCE(!try_grab_page(page, flags))) {
+			page = NULL;
+			goto out;
+		}
+	} else {
+		if (!(flags & FOLL_MIGRATION)) {
+			page = NULL;
+			goto out;
+		}
+
+		if (is_hugetlb_entry_migration(pte)) {
+			spin_unlock(ptl);
+			__migration_entry_wait_huge(ptep, ptl);
+			goto retry;
+		}
+		/*
+		 * hwpoisoned entry is treated as no_page_table in
+		 * follow_page_mask().

mm/hugetlb.c struct page * __weak · line 7039

Quoted 10 lines

mm/hugetlb.c +7 -0

+		 */
+	}
+out:
+	spin_unlock(ptl);
+	return page;
+}
+
  struct page * __weak
  follow_huge_pmd(struct mm_struct *mm, unsigned long address,
  		pmd_t *pmd, int flags)

Can someone explain why:
* follow_page() goes via follow_page_mask() for hugetlb
* __get_user_pages() goes via follow_hugetlb_page() and never via follow_page_mask() for hugetlb?

Quoted 3 lines

IOW, why can't we make follow_page_mask() just not handle hugetlb and route everything via follow_hugetlb_page() -- we primarily only have to teach it to not trigger faults.

IMHO, these follow_huge_xxx() functions are arch-specified at first and were moved into the common hugetlb.c by commit 9e5fc74c3025 ("mm:
hugetlb: Copy general hugetlb code from x86 to mm"), and now there are still some arch-specified follow_huge_xxx() definition, for example:
ia64: follow_huge_addr
powerpc: follow_huge_pd
s390: follow_huge_pud

What I mean is that follow_hugetlb_page() is a common and not-arch-specified function, is it suitable to change it to be arch-specified?
And thinking more, can we rename follow_hugetlb_page() as hugetlb_page_faultin() and simplify it to only handle the page faults of hugetlb like the faultin_page() for normal page? That means we can make sure only follow_page_mask() can handle hugetlb.

Quoted 157 lines · Baolin Wang

Quoted 154 lines · David Hildenbrand

Quoted 136 lines · Baolin Wang

On some architectures (like ARM64), it can support CONT-PTE/PMD size hugetlb, which means it can support not only PMD/PUD size hugetlb (2M and 1G), but also CONT-PTE/PMD size(64K and 32M) if a 4K page size specified.

So when looking up a CONT-PTE size hugetlb page by follow_page(), it will use pte_offset_map_lock() to get the pte entry lock for the CONT-PTE size hugetlb in follow_page_pte(). However this pte entry lock is incorrect for the CONT-PTE size hugetlb, since we should use huge_pte_lock() to get the correct lock, which is mm->page_table_lock.

That means the pte entry of the CONT-PTE size hugetlb under current pte lock is unstable in follow_page_pte(), we can continue to migrate or poison the pte entry of the CONT-PTE size hugetlb, which can cause some potential race issues, and following pte_xxx() validation is also unstable in follow_page_pte(), even though they are under the 'pte lock'.

Moreover we should use huge_ptep_get() to get the pte entry value of the CONT-PTE size hugetlb, which already takes into account the subpages' dirty or young bits in case we missed the dirty or young state of the CONT-PTE size hugetlb.

To fix above issues, introducing a new helper follow_huge_pte() to look up a CONT-PTE size hugetlb page, which uses huge_pte_lock() to get the correct pte entry lock to make the pte entry stable, as well as supporting non-present pte handling.

Signed-off-by Baolin Wang <[email protected]>

---
  include/linux/hugetlb.h |  8 ++++++++
  mm/gup.c                | 11 ++++++++++
  mm/hugetlb.c            | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 72 insertions(+)

include/linux/hugetlb.h +8 -0

index 3ec981a..d491138 100644
@@ -207,6 +207,8 @@ struct page *follow_huge_addr(struct mm_struct *mm, unsigned long address,
  struct page *follow_huge_pd(struct vm_area_struct *vma,
  			    unsigned long address, hugepd_t hpd,
  			    int flags, int pdshift);
+struct page *follow_huge_pte(struct vm_area_struct *vma, unsigned long address,
+			     pmd_t *pmd, int flags);
  struct page *follow_huge_pmd(struct mm_struct *mm, unsigned long address,
  				pmd_t *pmd, int flags);
  struct page *follow_huge_pud(struct mm_struct *mm, unsigned long address,

@@ -312,6 +314,12 @@ static inline struct page *follow_huge_pd(struct vm_area_struct *vma,
  	return NULL;
  }
 
+static inline struct page *follow_huge_pte(struct vm_area_struct *vma,
+				unsigned long address, pmd_t *pmd, int flags)
+{
+	return NULL;
+}
+
  static inline struct page *follow_huge_pmd(struct mm_struct *mm,
  				unsigned long address, pmd_t *pmd, int flags)
  {

mm/gup.c +11 -0

index 3b656b7..87a94f5 100644
@@ -534,6 +534,17 @@ static struct page *follow_page_pte(struct vm_area_struct *vma,
  	if (unlikely(pmd_bad(*pmd)))
  		return no_page_table(vma, flags);
 
+	/*
+	 * Considering PTE level hugetlb, like continuous-PTE hugetlb on
+	 * ARM64 architecture.
+	 */
+	if (is_vm_hugetlb_page(vma)) {
+		page = follow_huge_pte(vma, address, pmd, flags);
+		if (page)
+			return page;
+		return no_page_table(vma, flags);
+	}
+
  	ptep = pte_offset_map_lock(mm, pmd, address, &ptl);
  	pte = *ptep;
  	if (!pte_present(pte)) {

mm/hugetlb.c +46 -0

index 6c00ba1..cf742d1 100644
@@ -6981,6 +6981,59 @@ struct page * __weak
  	return NULL;
  }
 
+/* Support looking up a CONT-PTE size hugetlb page. */
+struct page * __weak
+follow_huge_pte(struct vm_area_struct *vma, unsigned long address,
+		pmd_t *pmd, int flags)
+{
+	struct mm_struct *mm = vma->vm_mm;
+	struct hstate *hstate = hstate_vma(vma);
+	unsigned long size = huge_page_size(hstate);
+	struct page *page = NULL;
+	spinlock_t *ptl;
+	pte_t *ptep, pte;
+
+	/*
+	 * FOLL_PIN is not supported for follow_page(). Ordinary GUP goes via
+	 * follow_hugetlb_page().
+	 */
+	if (WARN_ON_ONCE(flags & FOLL_PIN))
+		return NULL;
+
+	ptep = huge_pte_offset(mm, address, size);
+	if (!ptep)
+		return NULL;
+
+retry:
+	ptl = huge_pte_lock(hstate, mm, ptep);
+	pte = huge_ptep_get(ptep);
+	if (pte_present(pte)) {
+		page = pte_page(pte);
+		if (WARN_ON_ONCE(!try_grab_page(page, flags))) {
+			page = NULL;
+			goto out;
+		}
+	} else {
+		if (!(flags & FOLL_MIGRATION)) {
+			page = NULL;
+			goto out;
+		}
+
+		if (is_hugetlb_entry_migration(pte)) {
+			spin_unlock(ptl);
+			__migration_entry_wait_huge(ptep, ptl);
+			goto retry;
+		}
+		/*
+		 * hwpoisoned entry is treated as no_page_table in
+		 * follow_page_mask().

mm/hugetlb.c struct page * __weak · line 7039

Quoted 10 lines

mm/hugetlb.c +7 -0

+		 */
+	}
+out:
+	spin_unlock(ptl);
+	return page;
+}
+
  struct page * __weak
  follow_huge_pmd(struct mm_struct *mm, unsigned long address,
  		pmd_t *pmd, int flags)

Can someone explain why:
* follow_page() goes via follow_page_mask() for hugetlb
* __get_user_pages() goes via follow_hugetlb_page() and never via follow_page_mask() for hugetlb?

Quoted 3 lines

Quoted 3 lines

IOW, why can't we make follow_page_mask() just not handle hugetlb and route everything via follow_hugetlb_page() -- we primarily only have to teach it to not trigger faults.

Quoted 9 lines

IMHO, these follow_huge_xxx() functions are arch-specified at first and were moved into the common hugetlb.c by commit 9e5fc74c3025 ("mm:
hugetlb: Copy general hugetlb code from x86 to mm"), and now there are still some arch-specified follow_huge_xxx() definition, for example:
ia64: follow_huge_addr
powerpc: follow_huge_pd
s390: follow_huge_pud

Quoted 8 lines

What I mean is that follow_hugetlb_page() is a common and not-arch-specified function, is it suitable to change it to be arch-specified?
And thinking more, can we rename follow_hugetlb_page() as hugetlb_page_faultin() and simplify it to only handle the page faults of hugetlb like the faultin_page() for normal page? That means we can make sure only follow_page_mask() can handle hugetlb.

If follow_hugetlb_page() can be arch-independent, why do we need the other arch-dependent functions?

It all looks a bit weird to have two functions that walk page tables and are hugetlb aware.

Either this screams for a cleanup or I am missing something fundamental.

Signature · 3 lines

Thanks,

David / dhildenb

Quoted 158 lines · David Hildenbrand

Quoted 157 lines · Baolin Wang

Quoted 154 lines · David Hildenbrand

Quoted 136 lines · Baolin Wang

On some architectures (like ARM64), it can support CONT-PTE/PMD size hugetlb, which means it can support not only PMD/PUD size hugetlb (2M and 1G), but also CONT-PTE/PMD size(64K and 32M) if a 4K page size specified.

So when looking up a CONT-PTE size hugetlb page by follow_page(), it will use pte_offset_map_lock() to get the pte entry lock for the CONT-PTE size hugetlb in follow_page_pte(). However this pte entry lock is incorrect for the CONT-PTE size hugetlb, since we should use huge_pte_lock() to get the correct lock, which is mm->page_table_lock.

That means the pte entry of the CONT-PTE size hugetlb under current pte lock is unstable in follow_page_pte(), we can continue to migrate or poison the pte entry of the CONT-PTE size hugetlb, which can cause some potential race issues, and following pte_xxx() validation is also unstable in follow_page_pte(), even though they are under the 'pte lock'.

Moreover we should use huge_ptep_get() to get the pte entry value of the CONT-PTE size hugetlb, which already takes into account the subpages' dirty or young bits in case we missed the dirty or young state of the CONT-PTE size hugetlb.

To fix above issues, introducing a new helper follow_huge_pte() to look up a CONT-PTE size hugetlb page, which uses huge_pte_lock() to get the correct pte entry lock to make the pte entry stable, as well as supporting non-present pte handling.

Signed-off-by Baolin Wang <[email protected]>

---
   include/linux/hugetlb.h |  8 ++++++++
   mm/gup.c                | 11 ++++++++++
   mm/hugetlb.c            | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
   3 files changed, 72 insertions(+)

include/linux/hugetlb.h +8 -0

index 3ec981a..d491138 100644
@@ -207,6 +207,8 @@ struct page *follow_huge_addr(struct mm_struct *mm, unsigned long address,
   struct page *follow_huge_pd(struct vm_area_struct *vma,
   			    unsigned long address, hugepd_t hpd,
   			    int flags, int pdshift);
+struct page *follow_huge_pte(struct vm_area_struct *vma, unsigned long address,
+			     pmd_t *pmd, int flags);
   struct page *follow_huge_pmd(struct mm_struct *mm, unsigned long address,
   				pmd_t *pmd, int flags);
   struct page *follow_huge_pud(struct mm_struct *mm, unsigned long address,

@@ -312,6 +314,12 @@ static inline struct page *follow_huge_pd(struct vm_area_struct *vma,
   	return NULL;
   }
 
+static inline struct page *follow_huge_pte(struct vm_area_struct *vma,
+				unsigned long address, pmd_t *pmd, int flags)
+{
+	return NULL;
+}
+
   static inline struct page *follow_huge_pmd(struct mm_struct *mm,
   				unsigned long address, pmd_t *pmd, int flags)
   {

mm/gup.c +11 -0

index 3b656b7..87a94f5 100644
@@ -534,6 +534,17 @@ static struct page *follow_page_pte(struct vm_area_struct *vma,
   	if (unlikely(pmd_bad(*pmd)))
   		return no_page_table(vma, flags);
 
+	/*
+	 * Considering PTE level hugetlb, like continuous-PTE hugetlb on
+	 * ARM64 architecture.
+	 */
+	if (is_vm_hugetlb_page(vma)) {
+		page = follow_huge_pte(vma, address, pmd, flags);
+		if (page)
+			return page;
+		return no_page_table(vma, flags);
+	}
+
   	ptep = pte_offset_map_lock(mm, pmd, address, &ptl);
   	pte = *ptep;
   	if (!pte_present(pte)) {

mm/hugetlb.c +46 -0

index 6c00ba1..cf742d1 100644
@@ -6981,6 +6981,59 @@ struct page * __weak
   	return NULL;
   }
 
+/* Support looking up a CONT-PTE size hugetlb page. */
+struct page * __weak
+follow_huge_pte(struct vm_area_struct *vma, unsigned long address,
+		pmd_t *pmd, int flags)
+{
+	struct mm_struct *mm = vma->vm_mm;
+	struct hstate *hstate = hstate_vma(vma);
+	unsigned long size = huge_page_size(hstate);
+	struct page *page = NULL;
+	spinlock_t *ptl;
+	pte_t *ptep, pte;
+
+	/*
+	 * FOLL_PIN is not supported for follow_page(). Ordinary GUP goes via
+	 * follow_hugetlb_page().
+	 */
+	if (WARN_ON_ONCE(flags & FOLL_PIN))
+		return NULL;
+
+	ptep = huge_pte_offset(mm, address, size);
+	if (!ptep)
+		return NULL;
+
+retry:
+	ptl = huge_pte_lock(hstate, mm, ptep);
+	pte = huge_ptep_get(ptep);
+	if (pte_present(pte)) {
+		page = pte_page(pte);
+		if (WARN_ON_ONCE(!try_grab_page(page, flags))) {
+			page = NULL;
+			goto out;
+		}
+	} else {
+		if (!(flags & FOLL_MIGRATION)) {
+			page = NULL;
+			goto out;
+		}
+
+		if (is_hugetlb_entry_migration(pte)) {
+			spin_unlock(ptl);
+			__migration_entry_wait_huge(ptep, ptl);
+			goto retry;
+		}
+		/*
+		 * hwpoisoned entry is treated as no_page_table in
+		 * follow_page_mask().

mm/hugetlb.c struct page * __weak · line 7039

Quoted 10 lines

mm/hugetlb.c +7 -0

+		 */
+	}
+out:
+	spin_unlock(ptl);
+	return page;
+}
+
   struct page * __weak
   follow_huge_pmd(struct mm_struct *mm, unsigned long address,
   		pmd_t *pmd, int flags)

Can someone explain why:
* follow_page() goes via follow_page_mask() for hugetlb
* __get_user_pages() goes via follow_hugetlb_page() and never via follow_page_mask() for hugetlb?

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

IOW, why can't we make follow_page_mask() just not handle hugetlb and route everything via follow_hugetlb_page() -- we primarily only have to teach it to not trigger faults.

I have no idea how we got into this situation, and do agree that it makes little sense for both follow_page_mask and follow_hugetlb_page to do page table walking differently for hugetlb pages.

I think I have noted elsewhere that all those follow_huge_p*d rotines will look the same. It seems they were just added as needed when the follow_page_mask page table walking code was fleshed out. This also needs a cleanup. If we eliminate hugetlb handling from follow_page_mask, perhaps we can get rid of all these?

Quoted 9 lines

Quoted 9 lines

IMHO, these follow_huge_xxx() functions are arch-specified at first and were moved into the common hugetlb.c by commit 9e5fc74c3025 ("mm:
hugetlb: Copy general hugetlb code from x86 to mm"), and now there are still some arch-specified follow_huge_xxx() definition, for example:
ia64: follow_huge_addr
powerpc: follow_huge_pd
s390: follow_huge_pud

Quoted 8 lines

Quoted 8 lines

What I mean is that follow_hugetlb_page() is a common and not-arch-specified function, is it suitable to change it to be arch-specified?
And thinking more, can we rename follow_hugetlb_page() as hugetlb_page_faultin() and simplify it to only handle the page faults of hugetlb like the faultin_page() for normal page? That means we can make sure only follow_page_mask() can handle hugetlb.

Something like that might work, but you still have two page table walkers for hugetlb. I like David's idea (if I understand it correctly) of

using follow_hugetlb_page for both cases. As noted, it will need to be taught how to not trigger faults in the follow_page_mask case.

Quoted 18 lines

Quoted 9 lines

Quoted 9 lines

IMHO, these follow_huge_xxx() functions are arch-specified at first and were moved into the common hugetlb.c by commit 9e5fc74c3025 ("mm:
hugetlb: Copy general hugetlb code from x86 to mm"), and now there are still some arch-specified follow_huge_xxx() definition, for example:
ia64: follow_huge_addr
powerpc: follow_huge_pd
s390: follow_huge_pud

Quoted 8 lines

Quoted 8 lines

What I mean is that follow_hugetlb_page() is a common and not-arch-specified function, is it suitable to change it to be arch-specified?
And thinking more, can we rename follow_hugetlb_page() as hugetlb_page_faultin() and simplify it to only handle the page faults of hugetlb like the faultin_page() for normal page? That means we can make sure only follow_page_mask() can handle hugetlb.

Quoted 2 lines

Something like that might work, but you still have two page table walkers for hugetlb. I like David's idea (if I understand it correctly) of

What I mean is we may change the hugetlb handling like normal page:
1) use follow_page_mask() to look up a hugetlb firstly.
2) if can not get the hugetlb, then try to page fault by hugetlb_page_faultin().
3) if page fault successed, then retry to find hugetlb by follow_page_mask().

Just a rough thought, and I need more investigation for my idea and David's idea.

Quoted 2 lines

using follow_hugetlb_page for both cases. As noted, it will need to be taught how to not trigger faults in the follow_page_mask case.

Anyway, I also agree we need some cleanup, and firstly I think we should cleanup these arch-specified follow_huge_xxx() on some architectures which are similar with the common ones. I will look into these.

However, considering cleanup may need more investigation and refactoring, now I prefer to make these bug-fix patches of this patchset into mainline firstly, which are suitable to backport to old version to fix potential race issues. Mike and David, how do you think? Could you help to review these patches? Thanks.

Quoted 21 lines

Quoted 18 lines

Quoted 9 lines

Quoted 9 lines

IMHO, these follow_huge_xxx() functions are arch-specified at first and were moved into the common hugetlb.c by commit 9e5fc74c3025 ("mm:
hugetlb: Copy general hugetlb code from x86 to mm"), and now there are still some arch-specified follow_huge_xxx() definition, for example:
ia64: follow_huge_addr
powerpc: follow_huge_pd
s390: follow_huge_pud

Quoted 8 lines

Quoted 8 lines

What I mean is that follow_hugetlb_page() is a common and not-arch-specified function, is it suitable to change it to be arch-specified?
And thinking more, can we rename follow_hugetlb_page() as hugetlb_page_faultin() and simplify it to only handle the page faults of hugetlb like the faultin_page() for normal page? That means we can make sure only follow_page_mask() can handle hugetlb.

Quoted 2 lines

Something like that might work, but you still have two page table walkers for hugetlb. I like David's idea (if I understand it correctly) of

Quoted 6 lines

What I mean is we may change the hugetlb handling like normal page:
1) use follow_page_mask() to look up a hugetlb firstly.
2) if can not get the hugetlb, then try to page fault by hugetlb_page_faultin().
3) if page fault successed, then retry to find hugetlb by follow_page_mask().

That implies putting more hugetlbfs special code into generic GUP, turning it even more complicated. But of course, it depends on how the end result looks like. My gut feeling was that hugetlb is better handled in follow_hugetlb_page() separately (just like we do with a lot of other page table walkers).

Quoted 7 lines

Just a rough thought, and I need more investigation for my idea and David's idea.

Quoted 2 lines

using follow_hugetlb_page for both cases. As noted, it will need to be taught how to not trigger faults in the follow_page_mask case.

Quoted 3 lines

Anyway, I also agree we need some cleanup, and firstly I think we should cleanup these arch-specified follow_huge_xxx() on some architectures which are similar with the common ones. I will look into these.

There was a recent discussion on that, e.g.:

https://lkml.kernel.org/r/20220818135717.609eef8a@thinkpad

Quoted 7 lines

However, considering cleanup may need more investigation and refactoring, now I prefer to make these bug-fix patches of this patchset into mainline firstly, which are suitable to backport to old version to fix potential race issues. Mike and David, how do you think? Could you help to review these patches? Thanks.

Patch #1 certainly add more special code just to handle another hugetlb corner case (CONT pages), and maybe just making it all use follow_hugetlb_page() would be even cleaner and less error prone.

I agree that locking is shaky, but I'm not sure if we really want to backport this to stable trees:

https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

"It must fix a real bug that bothers people (not a, “This could be a problem...” type thing)."

Do we actually have any instance of this being a real (and not a theoretical) problem? If not, I'd rather clean it all up right away.

Quoted 28 lines · David Hildenbrand

Quoted 21 lines

Quoted 18 lines

Quoted 9 lines

Quoted 9 lines

IMHO, these follow_huge_xxx() functions are arch-specified at first and were moved into the common hugetlb.c by commit 9e5fc74c3025 ("mm:
hugetlb: Copy general hugetlb code from x86 to mm"), and now there are still some arch-specified follow_huge_xxx() definition, for example:
ia64: follow_huge_addr
powerpc: follow_huge_pd
s390: follow_huge_pud

Quoted 8 lines

Quoted 8 lines

What I mean is that follow_hugetlb_page() is a common and not-arch-specified function, is it suitable to change it to be arch-specified?
And thinking more, can we rename follow_hugetlb_page() as hugetlb_page_faultin() and simplify it to only handle the page faults of hugetlb like the faultin_page() for normal page? That means we can make sure only follow_page_mask() can handle hugetlb.

Quoted 2 lines

Something like that might work, but you still have two page table walkers for hugetlb. I like David's idea (if I understand it correctly) of

Quoted 6 lines

What I mean is we may change the hugetlb handling like normal page:
1) use follow_page_mask() to look up a hugetlb firstly.
2) if can not get the hugetlb, then try to page fault by hugetlb_page_faultin().
3) if page fault successed, then retry to find hugetlb by follow_page_mask().

Quoted 5 lines

That implies putting more hugetlbfs special code into generic GUP, turning it even more complicated. But of course, it depends on how the end result looks like. My gut feeling was that hugetlb is better handled in follow_hugetlb_page() separately (just like we do with a lot of other page table walkers).

OK, fair enough.

Quoted 7 lines

Quoted 7 lines

Just a rough thought, and I need more investigation for my idea and David's idea.

Quoted 2 lines

using follow_hugetlb_page for both cases. As noted, it will need to be taught how to not trigger faults in the follow_page_mask case.

Quoted 7 lines

Quoted 3 lines

Anyway, I also agree we need some cleanup, and firstly I think we should cleanup these arch-specified follow_huge_xxx() on some architectures which are similar with the common ones. I will look into these.

There was a recent discussion on that, e.g.:

https://lkml.kernel.org/r/20220818135717.609eef8a@thinkpad

Thanks.

Quoted 15 lines

Quoted 7 lines

However, considering cleanup may need more investigation and refactoring, now I prefer to make these bug-fix patches of this patchset into mainline firstly, which are suitable to backport to old version to fix potential race issues. Mike and David, how do you think? Could you help to review these patches? Thanks.

Patch #1 certainly add more special code just to handle another hugetlb corner case (CONT pages), and maybe just making it all use follow_hugetlb_page() would be even cleaner and less error prone.

I agree that locking is shaky, but I'm not sure if we really want to backport this to stable trees:

Quoted 8 lines

https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

"It must fix a real bug that bothers people (not a, “This could be a problem...” type thing)."

Do we actually have any instance of this being a real (and not a theoretical) problem? If not, I'd rather clean it all up right away.

I think this is a real problem (not theoretical), and easy to write some code to show the issue. For example, suppose thread A is trying to look up a CONT-PTE size hugetlb page under the lock, however antoher thread B can migrate the CONT-PTE hugetlb page at the same time, which will cause thread A to get an incorrect page, if thread A want to do something for this incorrect page, error occurs.

Actually we also want to backport these fixes to the distro with old kernel versions to make the hugetlb more stable. Otherwise we must hit these issues sooner or later if the customers use CONT-PTE/PMD hugetlb.

Anyway, if you and Mike still think these issues are not important enough to be fixed in the old versions, I can do the cleanup firstly.

Quoted 87 lines · Baolin Wang

Quoted 28 lines · David Hildenbrand

Quoted 21 lines

Quoted 18 lines

Quoted 9 lines

Quoted 9 lines

IMHO, these follow_huge_xxx() functions are arch-specified at first and were moved into the common hugetlb.c by commit 9e5fc74c3025 ("mm:
hugetlb: Copy general hugetlb code from x86 to mm"), and now there are still some arch-specified follow_huge_xxx() definition, for example:
ia64: follow_huge_addr
powerpc: follow_huge_pd
s390: follow_huge_pud

Quoted 8 lines

Quoted 8 lines

What I mean is that follow_hugetlb_page() is a common and not-arch-specified function, is it suitable to change it to be arch-specified?
And thinking more, can we rename follow_hugetlb_page() as hugetlb_page_faultin() and simplify it to only handle the page faults of hugetlb like the faultin_page() for normal page? That means we can make sure only follow_page_mask() can handle hugetlb.

Quoted 2 lines

Something like that might work, but you still have two page table walkers for hugetlb. I like David's idea (if I understand it correctly) of

Quoted 6 lines

What I mean is we may change the hugetlb handling like normal page:
1) use follow_page_mask() to look up a hugetlb firstly.
2) if can not get the hugetlb, then try to page fault by hugetlb_page_faultin().
3) if page fault successed, then retry to find hugetlb by follow_page_mask().

Quoted 5 lines

That implies putting more hugetlbfs special code into generic GUP, turning it even more complicated. But of course, it depends on how the end result looks like. My gut feeling was that hugetlb is better handled in follow_hugetlb_page() separately (just like we do with a lot of other page table walkers).

OK, fair enough.

Quoted 7 lines

Quoted 7 lines

Just a rough thought, and I need more investigation for my idea and David's idea.

Quoted 2 lines

using follow_hugetlb_page for both cases. As noted, it will need to be taught how to not trigger faults in the follow_page_mask case.

Quoted 7 lines

Quoted 3 lines

Anyway, I also agree we need some cleanup, and firstly I think we should cleanup these arch-specified follow_huge_xxx() on some architectures which are similar with the common ones. I will look into these.

There was a recent discussion on that, e.g.:

https://lkml.kernel.org/r/20220818135717.609eef8a@thinkpad

Thanks.

Quoted 15 lines

Quoted 7 lines

However, considering cleanup may need more investigation and refactoring, now I prefer to make these bug-fix patches of this patchset into mainline firstly, which are suitable to backport to old version to fix potential race issues. Mike and David, how do you think? Could you help to review these patches? Thanks.

Patch #1 certainly add more special code just to handle another hugetlb corner case (CONT pages), and maybe just making it all use follow_hugetlb_page() would be even cleaner and less error prone.

I agree that locking is shaky, but I'm not sure if we really want to backport this to stable trees:

Quoted 8 lines

https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

"It must fix a real bug that bothers people (not a, “This could be a problem...” type thing)."

Do we actually have any instance of this being a real (and not a theoretical) problem? If not, I'd rather clean it all up right away.

I think this is a real problem (not theoretical), and easy to write some code to show the issue. For example, suppose thread A is trying to look up a CONT-PTE size hugetlb page under the lock, however antoher thread B can migrate the CONT-PTE hugetlb page at the same time, which will cause thread A to get an incorrect page, if thread A want to do something for this incorrect page, error occurs.

Quoted 7 lines

Actually we also want to backport these fixes to the distro with old kernel versions to make the hugetlb more stable. Otherwise we must hit these issues sooner or later if the customers use CONT-PTE/PMD hugetlb.

Anyway, if you and Mike still think these issues are not important enough to be fixed in the old versions, I can do the cleanup firstly.

[asking myself which follow_page() users actually care about hugetlb, and why we need this handling in follow_page at all]

Which follow_page() user do we care about here? Primarily mm/migrate.c only I assume?

Quoted 99 lines · David Hildenbrand

Quoted 87 lines · Baolin Wang

Quoted 28 lines · David Hildenbrand

Quoted 21 lines

Quoted 18 lines

Quoted 9 lines

Quoted 9 lines

IMHO, these follow_huge_xxx() functions are arch-specified at first and were moved into the common hugetlb.c by commit 9e5fc74c3025 ("mm:
hugetlb: Copy general hugetlb code from x86 to mm"), and now there are still some arch-specified follow_huge_xxx() definition, for example:
ia64: follow_huge_addr
powerpc: follow_huge_pd
s390: follow_huge_pud

Quoted 8 lines

Quoted 8 lines

What I mean is that follow_hugetlb_page() is a common and not-arch-specified function, is it suitable to change it to be arch-specified?
And thinking more, can we rename follow_hugetlb_page() as hugetlb_page_faultin() and simplify it to only handle the page faults of hugetlb like the faultin_page() for normal page? That means we can make sure only follow_page_mask() can handle hugetlb.

Quoted 2 lines

Something like that might work, but you still have two page table walkers for hugetlb. I like David's idea (if I understand it correctly) of

Quoted 6 lines

What I mean is we may change the hugetlb handling like normal page:
1) use follow_page_mask() to look up a hugetlb firstly.
2) if can not get the hugetlb, then try to page fault by hugetlb_page_faultin().
3) if page fault successed, then retry to find hugetlb by follow_page_mask().

Quoted 5 lines

That implies putting more hugetlbfs special code into generic GUP, turning it even more complicated. But of course, it depends on how the end result looks like. My gut feeling was that hugetlb is better handled in follow_hugetlb_page() separately (just like we do with a lot of other page table walkers).

OK, fair enough.

Quoted 7 lines

Quoted 7 lines

Just a rough thought, and I need more investigation for my idea and David's idea.

Quoted 2 lines

using follow_hugetlb_page for both cases. As noted, it will need to be taught how to not trigger faults in the follow_page_mask case.

Quoted 7 lines

Quoted 3 lines

Anyway, I also agree we need some cleanup, and firstly I think we should cleanup these arch-specified follow_huge_xxx() on some architectures which are similar with the common ones. I will look into these.

There was a recent discussion on that, e.g.:

https://lkml.kernel.org/r/20220818135717.609eef8a@thinkpad

Thanks.

Quoted 15 lines

Quoted 7 lines

However, considering cleanup may need more investigation and refactoring, now I prefer to make these bug-fix patches of this patchset into mainline firstly, which are suitable to backport to old version to fix potential race issues. Mike and David, how do you think? Could you help to review these patches? Thanks.

Patch #1 certainly add more special code just to handle another hugetlb corner case (CONT pages), and maybe just making it all use follow_hugetlb_page() would be even cleaner and less error prone.

I agree that locking is shaky, but I'm not sure if we really want to backport this to stable trees:

Quoted 8 lines

https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

"It must fix a real bug that bothers people (not a, “This could be a problem...” type thing)."

Do we actually have any instance of this being a real (and not a theoretical) problem? If not, I'd rather clean it all up right away.

I think this is a real problem (not theoretical), and easy to write some code to show the issue. For example, suppose thread A is trying to look up a CONT-PTE size hugetlb page under the lock, however antoher thread B can migrate the CONT-PTE hugetlb page at the same time, which will cause thread A to get an incorrect page, if thread A want to do something for this incorrect page, error occurs.

Quoted 7 lines

Actually we also want to backport these fixes to the distro with old kernel versions to make the hugetlb more stable. Otherwise we must hit these issues sooner or later if the customers use CONT-PTE/PMD hugetlb.

Anyway, if you and Mike still think these issues are not important enough to be fixed in the old versions, I can do the cleanup firstly.

[asking myself which follow_page() users actually care about hugetlb, and why we need this handling in follow_page at all]

Quoted 2 lines

Which follow_page() user do we care about here? Primarily mm/migrate.c only I assume?

Right, mainly affects the move_pages() syscall I think. Yes, I can not know all of the users of the move_pages() syscall now or in the future in our data center, but like I said the move_pages() syscall + hugetlb can be a real potential stability issue.

Quoted 102 lines · Baolin Wang

Quoted 99 lines · David Hildenbrand

Quoted 87 lines · Baolin Wang

Quoted 28 lines · David Hildenbrand

Quoted 21 lines

Quoted 18 lines

Quoted 9 lines

Quoted 9 lines

IMHO, these follow_huge_xxx() functions are arch-specified at first and were moved into the common hugetlb.c by commit 9e5fc74c3025 ("mm:
hugetlb: Copy general hugetlb code from x86 to mm"), and now there are still some arch-specified follow_huge_xxx() definition, for example:
ia64: follow_huge_addr
powerpc: follow_huge_pd
s390: follow_huge_pud

Quoted 8 lines

Quoted 8 lines

What I mean is that follow_hugetlb_page() is a common and not-arch-specified function, is it suitable to change it to be arch-specified?
And thinking more, can we rename follow_hugetlb_page() as hugetlb_page_faultin() and simplify it to only handle the page faults of hugetlb like the faultin_page() for normal page? That means we can make sure only follow_page_mask() can handle hugetlb.

Quoted 2 lines

Something like that might work, but you still have two page table walkers for hugetlb. I like David's idea (if I understand it correctly) of

Quoted 6 lines

What I mean is we may change the hugetlb handling like normal page:
1) use follow_page_mask() to look up a hugetlb firstly.
2) if can not get the hugetlb, then try to page fault by hugetlb_page_faultin().
3) if page fault successed, then retry to find hugetlb by follow_page_mask().

Quoted 5 lines

That implies putting more hugetlbfs special code into generic GUP, turning it even more complicated. But of course, it depends on how the end result looks like. My gut feeling was that hugetlb is better handled in follow_hugetlb_page() separately (just like we do with a lot of other page table walkers).

OK, fair enough.

Quoted 7 lines

Quoted 7 lines

Just a rough thought, and I need more investigation for my idea and David's idea.

Quoted 2 lines

using follow_hugetlb_page for both cases. As noted, it will need to be taught how to not trigger faults in the follow_page_mask case.

Quoted 7 lines

Quoted 3 lines

Anyway, I also agree we need some cleanup, and firstly I think we should cleanup these arch-specified follow_huge_xxx() on some architectures which are similar with the common ones. I will look into these.

There was a recent discussion on that, e.g.:

https://lkml.kernel.org/r/20220818135717.609eef8a@thinkpad

Thanks.

Quoted 15 lines

Quoted 7 lines

However, considering cleanup may need more investigation and refactoring, now I prefer to make these bug-fix patches of this patchset into mainline firstly, which are suitable to backport to old version to fix potential race issues. Mike and David, how do you think? Could you help to review these patches? Thanks.

Patch #1 certainly add more special code just to handle another hugetlb corner case (CONT pages), and maybe just making it all use follow_hugetlb_page() would be even cleaner and less error prone.

I agree that locking is shaky, but I'm not sure if we really want to backport this to stable trees:

Quoted 8 lines

https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

"It must fix a real bug that bothers people (not a, “This could be a problem...” type thing)."

Do we actually have any instance of this being a real (and not a theoretical) problem? If not, I'd rather clean it all up right away.

I think this is a real problem (not theoretical), and easy to write some code to show the issue. For example, suppose thread A is trying to look up a CONT-PTE size hugetlb page under the lock, however antoher thread B can migrate the CONT-PTE hugetlb page at the same time, which will cause thread A to get an incorrect page, if thread A want to do something for this incorrect page, error occurs.

Quoted 7 lines

Actually we also want to backport these fixes to the distro with old kernel versions to make the hugetlb more stable. Otherwise we must hit these issues sooner or later if the customers use CONT-PTE/PMD hugetlb.

Anyway, if you and Mike still think these issues are not important enough to be fixed in the old versions, I can do the cleanup firstly.

[asking myself which follow_page() users actually care about hugetlb, and why we need this handling in follow_page at all]

Quoted 8 lines

Quoted 2 lines

Which follow_page() user do we care about here? Primarily mm/migrate.c only I assume?

Right, mainly affects the move_pages() syscall I think. Yes, I can not know all of the users of the move_pages() syscall now or in the future in our data center, but like I said the move_pages() syscall + hugetlb can be a real potential stability issue.

I wonder if we can get rid of follow_page() completely, there are not too many users. Or alternatively simply make it use general GUP infrastructure more clearly. We'd need something like FOLL_NOFAULT that also covers "absolutely no faults".

Quoted 112 lines · David Hildenbrand

Quoted 102 lines · Baolin Wang

Quoted 99 lines · David Hildenbrand

Quoted 87 lines · Baolin Wang

Quoted 28 lines · David Hildenbrand

Quoted 21 lines

Quoted 18 lines

Quoted 9 lines

Quoted 9 lines

IMHO, these follow_huge_xxx() functions are arch-specified at first and were moved into the common hugetlb.c by commit 9e5fc74c3025 ("mm:
hugetlb: Copy general hugetlb code from x86 to mm"), and now there are still some arch-specified follow_huge_xxx() definition, for example:
ia64: follow_huge_addr
powerpc: follow_huge_pd
s390: follow_huge_pud

Quoted 8 lines

Quoted 8 lines

What I mean is that follow_hugetlb_page() is a common and not-arch-specified function, is it suitable to change it to be arch-specified?
And thinking more, can we rename follow_hugetlb_page() as hugetlb_page_faultin() and simplify it to only handle the page faults of hugetlb like the faultin_page() for normal page? That means we can make sure only follow_page_mask() can handle hugetlb.

Quoted 2 lines

Something like that might work, but you still have two page table walkers for hugetlb. I like David's idea (if I understand it correctly) of

Quoted 6 lines

What I mean is we may change the hugetlb handling like normal page:
1) use follow_page_mask() to look up a hugetlb firstly.
2) if can not get the hugetlb, then try to page fault by hugetlb_page_faultin().
3) if page fault successed, then retry to find hugetlb by follow_page_mask().

Quoted 5 lines

That implies putting more hugetlbfs special code into generic GUP, turning it even more complicated. But of course, it depends on how the end result looks like. My gut feeling was that hugetlb is better handled in follow_hugetlb_page() separately (just like we do with a lot of other page table walkers).

OK, fair enough.

Quoted 7 lines

Quoted 7 lines

Just a rough thought, and I need more investigation for my idea and David's idea.

Quoted 2 lines

using follow_hugetlb_page for both cases. As noted, it will need to be taught how to not trigger faults in the follow_page_mask case.

Quoted 7 lines

Quoted 3 lines

Anyway, I also agree we need some cleanup, and firstly I think we should cleanup these arch-specified follow_huge_xxx() on some architectures which are similar with the common ones. I will look into these.

There was a recent discussion on that, e.g.:

https://lkml.kernel.org/r/20220818135717.609eef8a@thinkpad

Thanks.

Quoted 15 lines

Quoted 7 lines

However, considering cleanup may need more investigation and refactoring, now I prefer to make these bug-fix patches of this patchset into mainline firstly, which are suitable to backport to old version to fix potential race issues. Mike and David, how do you think? Could you help to review these patches? Thanks.

Patch #1 certainly add more special code just to handle another hugetlb corner case (CONT pages), and maybe just making it all use follow_hugetlb_page() would be even cleaner and less error prone.

I agree that locking is shaky, but I'm not sure if we really want to backport this to stable trees:

Quoted 8 lines

https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

"It must fix a real bug that bothers people (not a, “This could be a problem...” type thing)."

Do we actually have any instance of this being a real (and not a theoretical) problem? If not, I'd rather clean it all up right away.

I think this is a real problem (not theoretical), and easy to write some code to show the issue. For example, suppose thread A is trying to look up a CONT-PTE size hugetlb page under the lock, however antoher thread B can migrate the CONT-PTE hugetlb page at the same time, which will cause thread A to get an incorrect page, if thread A want to do something for this incorrect page, error occurs.

Quoted 7 lines

Actually we also want to backport these fixes to the distro with old kernel versions to make the hugetlb more stable. Otherwise we must hit these issues sooner or later if the customers use CONT-PTE/PMD hugetlb.

Anyway, if you and Mike still think these issues are not important enough to be fixed in the old versions, I can do the cleanup firstly.

[asking myself which follow_page() users actually care about hugetlb, and why we need this handling in follow_page at all]

Quoted 8 lines

Quoted 2 lines

Which follow_page() user do we care about here? Primarily mm/migrate.c only I assume?

Right, mainly affects the move_pages() syscall I think. Yes, I can not know all of the users of the move_pages() syscall now or in the future in our data center, but like I said the move_pages() syscall + hugetlb can be a real potential stability issue.

Quoted 4 lines

I wonder if we can get rid of follow_page() completely, there are not too many users. Or alternatively simply make it use general GUP infrastructure more clearly. We'd need something like FOLL_NOFAULT that also covers "absolutely no faults".

I am not sure I get your point. So you want change to use __get_user_pages() (or silimar wrappers) to look up a normal page or hugetlb instead of follow_page()? and adding a new FOLL_NOFAULT flag to __get_user_pages().

If I understand correctly, we still need more work to move those arch-specified follow_huge_xxx() into follow_hugetlb_page() firstly like we disscussed before? Which seems not backportable too.

I am not againt your idea, and I also agree that we should do some cleanup. But the point is if we need backport patches to fix this issue, which affects move_pages() syscall, if the answer is yes, I think my current fixes are suitable to backport.

Quoted 120 lines · Baolin Wang

Quoted 116 lines · David Hildenbrand

Quoted 102 lines · Baolin Wang

Quoted 99 lines · David Hildenbrand

Quoted 87 lines · Baolin Wang

Quoted 28 lines · David Hildenbrand

Quoted 21 lines

Quoted 18 lines

Quoted 9 lines

Quoted 9 lines

IMHO, these follow_huge_xxx() functions are arch-specified at first and were moved into the common hugetlb.c by commit 9e5fc74c3025 ("mm:
hugetlb: Copy general hugetlb code from x86 to mm"), and now there are still some arch-specified follow_huge_xxx() definition, for example:
ia64: follow_huge_addr
powerpc: follow_huge_pd
s390: follow_huge_pud

Quoted 8 lines

Quoted 8 lines

What I mean is that follow_hugetlb_page() is a common and not-arch-specified function, is it suitable to change it to be arch-specified?
And thinking more, can we rename follow_hugetlb_page() as hugetlb_page_faultin() and simplify it to only handle the page faults of hugetlb like the faultin_page() for normal page? That means we can make sure only follow_page_mask() can handle hugetlb.

Quoted 2 lines

Something like that might work, but you still have two page table walkers for hugetlb. I like David's idea (if I understand it correctly) of

Quoted 6 lines

What I mean is we may change the hugetlb handling like normal page:
1) use follow_page_mask() to look up a hugetlb firstly.
2) if can not get the hugetlb, then try to page fault by hugetlb_page_faultin().
3) if page fault successed, then retry to find hugetlb by follow_page_mask().

Quoted 5 lines

That implies putting more hugetlbfs special code into generic GUP, turning it even more complicated. But of course, it depends on how the end result looks like. My gut feeling was that hugetlb is better handled in follow_hugetlb_page() separately (just like we do with a lot of other page table walkers).

OK, fair enough.

Quoted 7 lines

Quoted 7 lines

Just a rough thought, and I need more investigation for my idea and David's idea.

Quoted 2 lines

using follow_hugetlb_page for both cases. As noted, it will need to be taught how to not trigger faults in the follow_page_mask case.

Quoted 7 lines

Quoted 3 lines

Anyway, I also agree we need some cleanup, and firstly I think we should cleanup these arch-specified follow_huge_xxx() on some architectures which are similar with the common ones. I will look into these.

There was a recent discussion on that, e.g.:

https://lkml.kernel.org/r/20220818135717.609eef8a@thinkpad

Thanks.

Quoted 15 lines

Quoted 7 lines

However, considering cleanup may need more investigation and refactoring, now I prefer to make these bug-fix patches of this patchset into mainline firstly, which are suitable to backport to old version to fix potential race issues. Mike and David, how do you think? Could you help to review these patches? Thanks.

Patch #1 certainly add more special code just to handle another hugetlb corner case (CONT pages), and maybe just making it all use follow_hugetlb_page() would be even cleaner and less error prone.

I agree that locking is shaky, but I'm not sure if we really want to backport this to stable trees:

Quoted 8 lines

https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

"It must fix a real bug that bothers people (not a, “This could be a problem...” type thing)."

Do we actually have any instance of this being a real (and not a theoretical) problem? If not, I'd rather clean it all up right away.

I think this is a real problem (not theoretical), and easy to write some code to show the issue. For example, suppose thread A is trying to look up a CONT-PTE size hugetlb page under the lock, however antoher thread B can migrate the CONT-PTE hugetlb page at the same time, which will cause thread A to get an incorrect page, if thread A want to do something for this incorrect page, error occurs.

Quoted 7 lines

Actually we also want to backport these fixes to the distro with old kernel versions to make the hugetlb more stable. Otherwise we must hit these issues sooner or later if the customers use CONT-PTE/PMD hugetlb.

Anyway, if you and Mike still think these issues are not important enough to be fixed in the old versions, I can do the cleanup firstly.

[asking myself which follow_page() users actually care about hugetlb, and why we need this handling in follow_page at all]

Quoted 8 lines

Quoted 2 lines

Which follow_page() user do we care about here? Primarily mm/migrate.c only I assume?

Right, mainly affects the move_pages() syscall I think. Yes, I can not know all of the users of the move_pages() syscall now or in the future in our data center, but like I said the move_pages() syscall + hugetlb can be a real potential stability issue.

I wonder if we can get rid of follow_page() completely, there are not too many users. Or alternatively simply make it use general GUP infrastructure more clearly. We'd need something like FOLL_NOFAULT that also covers "absolutely no faults".

Quoted 4 lines

I am not sure I get your point. So you want change to use __get_user_pages() (or silimar wrappers) to look up a normal page or hugetlb instead of follow_page()? and adding a new FOLL_NOFAULT flag to __get_user_pages().

Essentially just getting rid of follow_page() completely or making it a wrapper of __get_user_pages().

Quoted 5 lines

If I understand correctly, we still need more work to move those arch-specified follow_huge_xxx() into follow_hugetlb_page() firstly like we disscussed before? Which seems not backportable too.

I'm not sure we need all that magic in these arch specific helpers after all. I haven't looked into the details, but I really wonder why they handle something that follow_hugetlb_page() cannot easily handle. It all smells like legacy cruft.

Quoted 6 lines

I am not againt your idea, and I also agree that we should do some cleanup. But the point is if we need backport patches to fix this issue, which affects move_pages() syscall, if the answer is yes, I think my current fixes are suitable to backport.

I really don't like adding more make-legacy-cruft-happy code unless there is *real* need for it. (you could always just fix old kernels you care about with your patches here -- do they have to be in mainline?

don't think so)

But of course, it's up to Mike to decide, just my 2 cents :)

Quoted 28 lines · David Hildenbrand

Quoted 21 lines

Quoted 18 lines

Quoted 9 lines

Quoted 9 lines

IMHO, these follow_huge_xxx() functions are arch-specified at first and were moved into the common hugetlb.c by commit 9e5fc74c3025 ("mm:
hugetlb: Copy general hugetlb code from x86 to mm"), and now there are still some arch-specified follow_huge_xxx() definition, for example:
ia64: follow_huge_addr
powerpc: follow_huge_pd
s390: follow_huge_pud

Quoted 8 lines

Quoted 8 lines

What I mean is that follow_hugetlb_page() is a common and not-arch-specified function, is it suitable to change it to be arch-specified?
And thinking more, can we rename follow_hugetlb_page() as hugetlb_page_faultin() and simplify it to only handle the page faults of hugetlb like the faultin_page() for normal page? That means we can make sure only follow_page_mask() can handle hugetlb.

Quoted 2 lines

Something like that might work, but you still have two page table walkers for hugetlb. I like David's idea (if I understand it correctly) of

Quoted 6 lines

What I mean is we may change the hugetlb handling like normal page:
1) use follow_page_mask() to look up a hugetlb firstly.
2) if can not get the hugetlb, then try to page fault by hugetlb_page_faultin().
3) if page fault successed, then retry to find hugetlb by follow_page_mask().

Quoted 5 lines

That implies putting more hugetlbfs special code into generic GUP, turning it even more complicated. But of course, it depends on how the end result looks like. My gut feeling was that hugetlb is better handled in follow_hugetlb_page() separately (just like we do with a lot of other page table walkers).

OK, fair enough.

Quoted 7 lines

Quoted 7 lines

Just a rough thought, and I need more investigation for my idea and David's idea.

Quoted 2 lines

using follow_hugetlb_page for both cases. As noted, it will need to be taught how to not trigger faults in the follow_page_mask case.

Quoted 7 lines

Quoted 3 lines

Anyway, I also agree we need some cleanup, and firstly I think we should cleanup these arch-specified follow_huge_xxx() on some architectures which are similar with the common ones. I will look into these.

There was a recent discussion on that, e.g.:

https://lkml.kernel.org/r/20220818135717.609eef8a@thinkpad

Thanks.

Quoted 15 lines

Quoted 7 lines

However, considering cleanup may need more investigation and refactoring, now I prefer to make these bug-fix patches of this patchset into mainline firstly, which are suitable to backport to old version to fix potential race issues. Mike and David, how do you think? Could you help to review these patches? Thanks.

Patch #1 certainly add more special code just to handle another hugetlb corner case (CONT pages), and maybe just making it all use follow_hugetlb_page() would be even cleaner and less error prone.

I agree that locking is shaky, but I'm not sure if we really want to backport this to stable trees:

Quoted 8 lines

https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

"It must fix a real bug that bothers people (not a, “This could be a problem...” type thing)."

Do we actually have any instance of this being a real (and not a theoretical) problem? If not, I'd rather clean it all up right away.

I think this is a real problem (not theoretical), and easy to write some code to show the issue. For example, suppose thread A is trying to look up a CONT-PTE size hugetlb page under the lock, however antoher thread B can migrate the CONT-PTE hugetlb page at the same time, which will cause thread A to get an incorrect page, if thread A want to do something for this incorrect page, error occurs.

Quoted 80 lines · Baolin Wang

Quoted 28 lines · David Hildenbrand

Quoted 21 lines

Quoted 18 lines

Quoted 9 lines

Quoted 9 lines

IMHO, these follow_huge_xxx() functions are arch-specified at first and were moved into the common hugetlb.c by commit 9e5fc74c3025 ("mm:
hugetlb: Copy general hugetlb code from x86 to mm"), and now there are still some arch-specified follow_huge_xxx() definition, for example:
ia64: follow_huge_addr
powerpc: follow_huge_pd
s390: follow_huge_pud

Quoted 8 lines

Quoted 8 lines

What I mean is that follow_hugetlb_page() is a common and not-arch-specified function, is it suitable to change it to be arch-specified?
And thinking more, can we rename follow_hugetlb_page() as hugetlb_page_faultin() and simplify it to only handle the page faults of hugetlb like the faultin_page() for normal page? That means we can make sure only follow_page_mask() can handle hugetlb.

Quoted 2 lines

Something like that might work, but you still have two page table walkers for hugetlb. I like David's idea (if I understand it correctly) of

Quoted 6 lines

What I mean is we may change the hugetlb handling like normal page:
1) use follow_page_mask() to look up a hugetlb firstly.
2) if can not get the hugetlb, then try to page fault by hugetlb_page_faultin().
3) if page fault successed, then retry to find hugetlb by follow_page_mask().

Quoted 5 lines

That implies putting more hugetlbfs special code into generic GUP, turning it even more complicated. But of course, it depends on how the end result looks like. My gut feeling was that hugetlb is better handled in follow_hugetlb_page() separately (just like we do with a lot of other page table walkers).

OK, fair enough.

Quoted 7 lines

Quoted 7 lines

Just a rough thought, and I need more investigation for my idea and David's idea.

Quoted 2 lines

using follow_hugetlb_page for both cases. As noted, it will need to be taught how to not trigger faults in the follow_page_mask case.

Quoted 7 lines

Quoted 3 lines

Anyway, I also agree we need some cleanup, and firstly I think we should cleanup these arch-specified follow_huge_xxx() on some architectures which are similar with the common ones. I will look into these.

There was a recent discussion on that, e.g.:

https://lkml.kernel.org/r/20220818135717.609eef8a@thinkpad

Thanks.

Quoted 23 lines

Quoted 7 lines

However, considering cleanup may need more investigation and refactoring, now I prefer to make these bug-fix patches of this patchset into mainline firstly, which are suitable to backport to old version to fix potential race issues. Mike and David, how do you think? Could you help to review these patches? Thanks.

Patch #1 certainly add more special code just to handle another hugetlb corner case (CONT pages), and maybe just making it all use follow_hugetlb_page() would be even cleaner and less error prone.

I agree that locking is shaky, but I'm not sure if we really want to backport this to stable trees:

https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

"It must fix a real bug that bothers people (not a, “This could be a problem...” type thing)."

Do we actually have any instance of this being a real (and not a theoretical) problem? If not, I'd rather clean it all up right away.

Quoted 6 lines

I think this is a real problem (not theoretical), and easy to write some code to show the issue. For example, suppose thread A is trying to look up a CONT-PTE size hugetlb page under the lock, however antoher thread B can migrate the CONT-PTE hugetlb page at the same time, which will cause thread A to get an incorrect page, if thread A want to do something for this incorrect page, error occurs.

Is the primary concern the locking? If so, I am not sure we have an issue.

As mentioned in your commit message, current code will use pte_offset_map_lock(). pte_offset_map_lock uses pte_lockptr, and pte_lockptr will either be the mm wide lock or pmd_page lock. To me, it seems that

either would provide correct synchronization for CONT-PTE entries. Am I missing something or misreading the code?

I started looking at code cleanup suggested by David. Here is a quick patch (not tested and likely containing errors) to see if this is a step in the right direction.

I like it because we get rid of/combine all those follow_huge_p*d routines.

From 35d117a707c1567ddf350554298697d40eace0d7 Mon Sep 17 00:00:00 2001
From: Mike Kravetz <[email protected]>
Date: Wed, 24 Aug 2022 15:59:15 -0700
Subject: [PATCH] hugetlb: call hugetlb_follow_page_mask for hugetlb pages in
follow_page_mask

At the beginning of follow_page_mask, there currently is a call to follow_huge_addr which 'may' handle hugetlb pages. ia64 is the only architecture which (incorrectly) provides a follow_huge_addr routine that does not return error. Instead, at each level of the page table a check is made for a hugetlb entry. If a hugetlb entry is found, a call to a routine associated with that page table level such as follow_huge_pmd is made.

All the follow_huge_p*d routines are basically the same. In addition huge page size can be derived from the vma, so we know where in the page table a huge page would reside. So, replace follow_huge_addr with a new architecture independent routine which will provide the same functionality as the follow_huge_p*d routines. We can then eliminate the p*d_huge checks in follow_page_mask page table walking as well as the follow_huge_p*d routines themselves.

follow_page_mask still has is_hugepd hugetlb checks during page table walking. This is due to these checks and follow_huge_pd being architecture specific. These can be eliminated if hugetlb_follow_page_mask can be overwritten by architectures (powerpc) that need to do follow_huge_pd processing.

Signed-off-by Mike Kravetz <[email protected]>

---
  arch/ia64/mm/hugetlbpage.c |  15 ----
  arch/s390/mm/hugetlbpage.c |  10 ---
  include/linux/hugetlb.h    |  41 +++-------
  mm/gup.c                   |  27 +------
  mm/hugetlb.c               | 159 ++++++++++++-------------------------
  5 files changed, 62 insertions(+), 190 deletions(-)

arch/ia64/mm/hugetlbpage.c +0 -15

index f993cb36c062..380d2f3966c9 100644
@@ -91,21 +91,6 @@ int prepare_hugepage_range(struct file *file,
  	return 0;
  }
 
-struct page *follow_huge_addr(struct mm_struct *mm, unsigned long addr, int write)
-{
-	struct page *page;
-	pte_t *ptep;
-
-	if (REGION_NUMBER(addr) != RGN_HPAGE)
-		return ERR_PTR(-EINVAL);
-
-	ptep = huge_pte_offset(mm, addr, HPAGE_SIZE);
-	if (!ptep || pte_none(*ptep))
-		return NULL;
-	page = pte_page(*ptep);
-	page += ((addr & ~HPAGE_MASK) >> PAGE_SHIFT);
-	return page;
-}
  int pmd_huge(pmd_t pmd)
  {
  	return 0;

arch/s390/mm/hugetlbpage.c +0 -10

index 10e51ef9c79a..c299a18273ff 100644
@@ -237,16 +237,6 @@ int pud_huge(pud_t pud)
  	return pud_large(pud);
  }
 
-struct page *
-follow_huge_pud(struct mm_struct *mm, unsigned long address,
-		pud_t *pud, int flags)
-{
-	if (flags & FOLL_GET)
-		return NULL;
-
-	return pud_page(*pud) + ((address & ~PUD_MASK) >> PAGE_SHIFT);
-}
-
  bool __init arch_hugetlb_valid_size(unsigned long size)
  {
  	if (MACHINE_HAS_EDAT1 && size == PMD_SIZE)

include/linux/hugetlb.h +9 -32

index 3ec981a0d8b3..0c19d200c851 100644
@@ -142,6 +142,8 @@ int move_hugetlb_page_tables(struct vm_area_struct *vma,
  			     unsigned long len);
  int copy_hugetlb_page_range(struct mm_struct *, struct mm_struct *,
  			    struct vm_area_struct *, struct vm_area_struct *);
+struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma,
+                              unsigned long address, unsigned int flags);
  long follow_hugetlb_page(struct mm_struct *, struct vm_area_struct *,
  			 struct page **, struct vm_area_struct **,
  			 unsigned long *, unsigned long *, long, unsigned int,

@@ -202,17 +204,9 @@ int huge_pmd_unshare(struct mm_struct *mm, struct vm_area_struct *vma,
  				unsigned long addr, pte_t *ptep);
  void adjust_range_if_pmd_sharing_possible(struct vm_area_struct *vma,
  				unsigned long *start, unsigned long *end);
-struct page *follow_huge_addr(struct mm_struct *mm, unsigned long address,
-			      int write);
  struct page *follow_huge_pd(struct vm_area_struct *vma,
  			    unsigned long address, hugepd_t hpd,
  			    int flags, int pdshift);
-struct page *follow_huge_pmd(struct mm_struct *mm, unsigned long address,
-				pmd_t *pmd, int flags);
-struct page *follow_huge_pud(struct mm_struct *mm, unsigned long address,
-				pud_t *pud, int flags);
-struct page *follow_huge_pgd(struct mm_struct *mm, unsigned long address,
-			     pgd_t *pgd, int flags);
 
  int pmd_huge(pmd_t pmd);
  int pud_huge(pud_t pud);

@@ -257,6 +251,13 @@ static inline void adjust_range_if_pmd_sharing_possible(
  {
  }
 
+static inline struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma,
+                              unsigned long address, unsigned int flags)
+{
+	/* should never happen, but do not want to BUG */
+	return ERR_PTR(-EINVAL);
+}
+
  static inline long follow_hugetlb_page(struct mm_struct *mm,
  			struct vm_area_struct *vma, struct page **pages,
  			struct vm_area_struct **vmas, unsigned long *position,

@@ -267,12 +268,6 @@ static inline long follow_hugetlb_page(struct mm_struct *mm,
  	return 0;
  }
 
-static inline struct page *follow_huge_addr(struct mm_struct *mm,
-					unsigned long address, int write)
-{
-	return ERR_PTR(-EINVAL);
-}
-
  static inline int copy_hugetlb_page_range(struct mm_struct *dst,
  					  struct mm_struct *src,
  					  struct vm_area_struct *dst_vma,

@@ -312,24 +307,6 @@ static inline struct page *follow_huge_pd(struct vm_area_struct *vma,
  	return NULL;
  }
 
-static inline struct page *follow_huge_pmd(struct mm_struct *mm,
-				unsigned long address, pmd_t *pmd, int flags)
-{
-	return NULL;
-}
-
-static inline struct page *follow_huge_pud(struct mm_struct *mm,
-				unsigned long address, pud_t *pud, int flags)
-{
-	return NULL;
-}
-
-static inline struct page *follow_huge_pgd(struct mm_struct *mm,
-				unsigned long address, pgd_t *pgd, int flags)
-{
-	return NULL;
-}
-
  static inline int prepare_hugepage_range(struct file *file,
  				unsigned long addr, unsigned long len)
  {

mm/gup.c +3 -24

index 3b656b7e8a3c..a93c04437faa 100644
@@ -661,12 +661,6 @@ static struct page *follow_pmd_mask(struct vm_area_struct *vma,
  	pmdval = READ_ONCE(*pmd);
  	if (pmd_none(pmdval))
  		return no_page_table(vma, flags);
-	if (pmd_huge(pmdval) && is_vm_hugetlb_page(vma)) {
-		page = follow_huge_pmd(mm, address, pmd, flags);
-		if (page)
-			return page;
-		return no_page_table(vma, flags);
-	}
  	if (is_hugepd(__hugepd(pmd_val(pmdval)))) {
  		page = follow_huge_pd(vma, address,
  				      __hugepd(pmd_val(pmdval)), flags,

@@ -764,12 +758,6 @@ static struct page *follow_pud_mask(struct vm_area_struct *vma,
  	pud = pud_offset(p4dp, address);
  	if (pud_none(*pud))
  		return no_page_table(vma, flags);
-	if (pud_huge(*pud) && is_vm_hugetlb_page(vma)) {
-		page = follow_huge_pud(mm, address, pud, flags);
-		if (page)
-			return page;
-		return no_page_table(vma, flags);
-	}
  	if (is_hugepd(__hugepd(pud_val(*pud)))) {
  		page = follow_huge_pd(vma, address,
  				      __hugepd(pud_val(*pud)), flags,

@@ -851,24 +839,15 @@ static struct page *follow_page_mask(struct vm_area_struct *vma,
 
  	ctx->page_mask = 0;
 
-	/* make this handle hugepd */
-	page = follow_huge_addr(mm, address, flags & FOLL_WRITE);
-	if (!IS_ERR(page)) {
-		WARN_ON_ONCE(flags & (FOLL_GET | FOLL_PIN));
-		return page;
-	}
+	/* hugetlb is special */
+	if (is_vm_hugetlb_page(vma))
+		return hugetlb_follow_page_mask(vma, address, flags);
 
  	pgd = pgd_offset(mm, address);
 
  	if (pgd_none(*pgd) || unlikely(pgd_bad(*pgd)))
  		return no_page_table(vma, flags);
 
-	if (pgd_huge(*pgd)) {
-		page = follow_huge_pgd(mm, address, pgd, flags);
-		if (page)
-			return page;
-		return no_page_table(vma, flags);
-	}
  	if (is_hugepd(__hugepd(pgd_val(*pgd)))) {
  		page = follow_huge_pd(vma, address,
  				      __hugepd(pgd_val(*pgd)), flags,

mm/hugetlb.c +16 -0

index 6c00ba1dde32..947401df8190 100644
@@ -6168,6 +6168,56 @@ static inline bool __follow_hugetlb_must_fault(unsigned int flags, pte_t *pte,
  	return false;
  }
 
+struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma,
+				unsigned long address, unsigned int flags)
+{
+	struct hstate *h = hstate_vma(vma);
+	struct mm_struct *mm = vma->vm_mm;
+	unsigned long haddr = address & huge_page_mask(h);
+	struct page *page = NULL;
+	spinlock_t *ptl;
+	pte_t *pte, entry;
+
+	/*
+	 * FOLL_PIN is not supported for follow_page(). Ordinary GUP goes via
+	 * follow_hugetlb_page().
+	 */
+	if (WARN_ON_ONCE(flags & FOLL_PIN))
+		return NULL;

mm/hugetlb.c +15 -109

+		if (is_hugetlb_entry_migration(entry)) {
+			spin_unlock(ptl);
+			__migration_entry_wait_huge(pte, ptl);
+			goto retry;
+		}
+		/*
+		 * hwpoisoned entry is treated as no_page_table in
+		 * follow_page_mask().
+		 */
+	}
+out:
+	spin_unlock(ptl);
+	return page;
+}
+
  long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
  			 struct page **pages, struct vm_area_struct **vmas,
  			 unsigned long *position, unsigned long *nr_pages,

@@ -6966,13 +7016,6 @@ __weak unsigned long hugetlb_mask_last_page(struct hstate *h)
   * These functions are overwritable if your architecture needs its own
   * behavior.
   */
-struct page * __weak
-follow_huge_addr(struct mm_struct *mm, unsigned long address,
-			      int write)
-{
-	return ERR_PTR(-EINVAL);
-}
-
  struct page * __weak
  follow_huge_pd(struct vm_area_struct *vma,
  	       unsigned long address, hugepd_t hpd, int flags, int pdshift)

@@ -6981,108 +7024,6 @@ follow_huge_pd(struct vm_area_struct *vma,
  	return NULL;
  }
 
-struct page * __weak
-follow_huge_pmd(struct mm_struct *mm, unsigned long address,
-		pmd_t *pmd, int flags)
-{
-	struct page *page = NULL;
-	spinlock_t *ptl;
-	pte_t pte;
-
-	/*
-	 * FOLL_PIN is not supported for follow_page(). Ordinary GUP goes via
-	 * follow_hugetlb_page().
-	 */
-	if (WARN_ON_ONCE(flags & FOLL_PIN))
-		return NULL;
-
-retry:
-	ptl = pmd_lockptr(mm, pmd);
-	spin_lock(ptl);
-	/*
-	 * make sure that the address range covered by this pmd is not
-	 * unmapped from other threads.
-	 */
-	if (!pmd_huge(*pmd))
-		goto out;
-	pte = huge_ptep_get((pte_t *)pmd);
-	if (pte_present(pte)) {
-		page = pmd_page(*pmd) + ((address & ~PMD_MASK) >> PAGE_SHIFT);
-		/*
-		 * try_grab_page() should always succeed here, because: a) we
-		 * hold the pmd (ptl) lock, and b) we've just checked that the
-		 * huge pmd (head) page is present in the page tables. The ptl
-		 * prevents the head page and tail pages from being rearranged
-		 * in any way. So this page must be available at this point,
-		 * unless the page refcount overflowed:
-		 */
-		if (WARN_ON_ONCE(!try_grab_page(page, flags))) {
-			page = NULL;
-			goto out;
-		}
-	} else {
-		if (is_hugetlb_entry_migration(pte)) {
-			spin_unlock(ptl);
-			__migration_entry_wait_huge((pte_t *)pmd, ptl);
-			goto retry;
-		}
-		/*
-		 * hwpoisoned entry is treated as no_page_table in
-		 * follow_page_mask().
-		 */
-	}
-out:
-	spin_unlock(ptl);
-	return page;
-}
-
-struct page * __weak
-follow_huge_pud(struct mm_struct *mm, unsigned long address,
-		pud_t *pud, int flags)
-{
-	struct page *page = NULL;
-	spinlock_t *ptl;
-	pte_t pte;
-
-	if (WARN_ON_ONCE(flags & FOLL_PIN))
-		return NULL;
-
-retry:
-	ptl = huge_pte_lock(hstate_sizelog(PUD_SHIFT), mm, (pte_t *)pud);
-	if (!pud_huge(*pud))
-		goto out;
-	pte = huge_ptep_get((pte_t *)pud);
-	if (pte_present(pte)) {
-		page = pud_page(*pud) + ((address & ~PUD_MASK) >> PAGE_SHIFT);
-		if (WARN_ON_ONCE(!try_grab_page(page, flags))) {
-			page = NULL;
-			goto out;
-		}
-	} else {
-		if (is_hugetlb_entry_migration(pte)) {
-			spin_unlock(ptl);
-			__migration_entry_wait(mm, (pte_t *)pud, ptl);
-			goto retry;
-		}
-		/*
-		 * hwpoisoned entry is treated as no_page_table in
-		 * follow_page_mask().
-		 */
-	}
-out:
-	spin_unlock(ptl);
-	return page;
-}
-
-struct page * __weak
-follow_huge_pgd(struct mm_struct *mm, unsigned long address, pgd_t *pgd, int flags)
-{
-	if (flags & (FOLL_GET | FOLL_PIN))
-		return NULL;
-
-	return pte_page(*(pte_t *)pgd) + ((address & ~PGDIR_MASK) >> PAGE_SHIFT);
-}
-
  int isolate_hugetlb(struct page *page, struct list_head *list)
  {
  	int ret = 0;

mm/hugetlb.c static inline bool __follow_hugetlb_must_fault(unsigned int flags, pte_t *pte, · line 6205

Quoted 10 lines

mm/hugetlb.c +9 -0

+		/*
+		 * try_grab_page() should always succeed here, because we hold
+		 * the ptl lock and have verified pte_present().
+		 */
+		if (WARN_ON_ONCE(!try_grab_page(page, flags))) {
+			page = NULL;
+			goto out;
+		}
+	} else {

Should add FOLL_MIGRATION validation before waiting a migration entry.

Quoted 12 lines

mm/hugetlb.c static inline bool __follow_hugetlb_must_fault(unsigned int flags, pte_t *pte, · line 6205

Quoted 10 lines

mm/hugetlb.c +9 -0

+		/*
+		 * try_grab_page() should always succeed here, because we hold
+		 * the ptl lock and have verified pte_present().
+		 */
+		if (WARN_ON_ONCE(!try_grab_page(page, flags))) {
+			page = NULL;
+			goto out;
+		}
+	} else {

Should add FOLL_MIGRATION validation before waiting a migration entry.

We really only need FOLL_MIGRATION for KSM. As hugetlb pages cannot be KSM pages, we don't need this.

Actually, I do have patches in the works that rip out FOLL_MIGRATION completely by adjusting KSM code.

So let's try to not add dead code (although it would make sense for feature completeness as is -- but then, FOLL_MIGRATION really needs to go).

Quoted 81 lines · Mike Kravetz

Quoted 80 lines · Baolin Wang

Quoted 28 lines · David Hildenbrand

Quoted 21 lines

Quoted 18 lines

Quoted 9 lines

Quoted 9 lines

IMHO, these follow_huge_xxx() functions are arch-specified at first and were moved into the common hugetlb.c by commit 9e5fc74c3025 ("mm:
hugetlb: Copy general hugetlb code from x86 to mm"), and now there are still some arch-specified follow_huge_xxx() definition, for example:
ia64: follow_huge_addr
powerpc: follow_huge_pd
s390: follow_huge_pud

Quoted 8 lines

Quoted 8 lines

What I mean is that follow_hugetlb_page() is a common and not-arch-specified function, is it suitable to change it to be arch-specified?
And thinking more, can we rename follow_hugetlb_page() as hugetlb_page_faultin() and simplify it to only handle the page faults of hugetlb like the faultin_page() for normal page? That means we can make sure only follow_page_mask() can handle hugetlb.

Quoted 2 lines

Something like that might work, but you still have two page table walkers for hugetlb. I like David's idea (if I understand it correctly) of

Quoted 6 lines

What I mean is we may change the hugetlb handling like normal page:
1) use follow_page_mask() to look up a hugetlb firstly.
2) if can not get the hugetlb, then try to page fault by hugetlb_page_faultin().
3) if page fault successed, then retry to find hugetlb by follow_page_mask().

Quoted 5 lines

That implies putting more hugetlbfs special code into generic GUP, turning it even more complicated. But of course, it depends on how the end result looks like. My gut feeling was that hugetlb is better handled in follow_hugetlb_page() separately (just like we do with a lot of other page table walkers).

OK, fair enough.

Quoted 7 lines

Quoted 7 lines

Just a rough thought, and I need more investigation for my idea and David's idea.

Quoted 2 lines

using follow_hugetlb_page for both cases. As noted, it will need to be taught how to not trigger faults in the follow_page_mask case.

Quoted 7 lines

Quoted 3 lines

Anyway, I also agree we need some cleanup, and firstly I think we should cleanup these arch-specified follow_huge_xxx() on some architectures which are similar with the common ones. I will look into these.

There was a recent discussion on that, e.g.:

https://lkml.kernel.org/r/20220818135717.609eef8a@thinkpad

Thanks.

Quoted 23 lines

Quoted 7 lines

However, considering cleanup may need more investigation and refactoring, now I prefer to make these bug-fix patches of this patchset into mainline firstly, which are suitable to backport to old version to fix potential race issues. Mike and David, how do you think? Could you help to review these patches? Thanks.

Patch #1 certainly add more special code just to handle another hugetlb corner case (CONT pages), and maybe just making it all use follow_hugetlb_page() would be even cleaner and less error prone.

I agree that locking is shaky, but I'm not sure if we really want to backport this to stable trees:

https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

"It must fix a real bug that bothers people (not a, “This could be a problem...” type thing)."

Do we actually have any instance of this being a real (and not a theoretical) problem? If not, I'd rather clean it all up right away.

Quoted 8 lines

Quoted 6 lines

I think this is a real problem (not theoretical), and easy to write some code to show the issue. For example, suppose thread A is trying to look up a CONT-PTE size hugetlb page under the lock, however antoher thread B can migrate the CONT-PTE hugetlb page at the same time, which will cause thread A to get an incorrect page, if thread A want to do something for this incorrect page, error occurs.

Is the primary concern the locking? If so, I am not sure we have an issue.

Yes.

Quoted 3 lines

As mentioned in your commit message, current code will use pte_offset_map_lock(). pte_offset_map_lock uses pte_lockptr, and pte_lockptr will either be the mm wide lock or pmd_page lock. To me, it seems that

The ALLOC_SPLIT_PTLOCKS can be always true on my machine, that means the pte_lockptr() will always use the PTE page lock, however huge_pte_lock() will use the mm wide lock.

Quoted 8 lines

either would provide correct synchronization for CONT-PTE entries. Am I missing something or misreading the code?

I started looking at code cleanup suggested by David. Here is a quick patch (not tested and likely containing errors) to see if this is a step in the right direction.

Quoted 2 lines

I like it because we get rid of/combine all those follow_huge_p*d routines.

Great, this looks straight forward to me (some nits as below). David, how do you think?

Quoted 21 lines

mm/hugetlb.c +21 -0

+struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma,
+				unsigned long address, unsigned int flags)
+{
+	struct hstate *h = hstate_vma(vma);
+	struct mm_struct *mm = vma->vm_mm;
+	unsigned long haddr = address & huge_page_mask(h);
+	struct page *page = NULL;
+	spinlock_t *ptl;
+	pte_t *pte, entry;
+
+	/*
+	 * FOLL_PIN is not supported for follow_page(). Ordinary GUP goes via
+	 * follow_hugetlb_page().
+	 */
+	if (WARN_ON_ONCE(flags & FOLL_PIN))
+		return NULL;
+
+	pte = huge_pte_offset(mm, haddr, huge_page_size(h));
+	if (!pte)
+		return NULL;
+

mm/hugetlb.c static inline bool __follow_hugetlb_must_fault(unsigned int flags, pte_t *pte, · line 6196

Quoted 5 lines

mm/hugetlb.c +5 -0

+retry:
+	ptl = huge_pte_lock(h, mm, pte);
+	entry = huge_ptep_get(pte);
+	if (pte_present(entry)) {
+		page = pte_page(entry);

Should follow previous logic?
page = pte_page(entry) + ((address & ~huge_page_mask(h)) >> PAGE_SHIFT);

Quoted 9 lines

mm/hugetlb.c +9 -0

+		/*
+		 * try_grab_page() should always succeed here, because we hold
+		 * the ptl lock and have verified pte_present().
+		 */
+		if (WARN_ON_ONCE(!try_grab_page(page, flags))) {
+			page = NULL;
+			goto out;
+		}
+	} else {

Is the primary concern the locking? If so, I am not sure we have an issue. As mentioned in your commit message, current code will use pte_offset_map_lock(). pte_offset_map_lock uses pte_lockptr, and pte_lockptr will either be the mm wide lock or pmd_page lock. To me, it seems that either would provide correct synchronization for CONT-PTE entries. Am I missing something or misreading the code?

I started looking at code cleanup suggested by David. Here is a quick patch (not tested and likely containing errors) to see if this is a step in the right direction.

I like it because we get rid of/combine all those follow_huge_p*d routines.

From 35d117a707c1567ddf350554298697d40eace0d7 Mon Sep 17 00:00:00 2001
From: Mike Kravetz <[email protected]>
Date: Wed, 24 Aug 2022 15:59:15 -0700
Subject: [PATCH] hugetlb: call hugetlb_follow_page_mask for hugetlb pages in
follow_page_mask

At the beginning of follow_page_mask, there currently is a call to follow_huge_addr which 'may' handle hugetlb pages. ia64 is the only architecture which (incorrectly) provides a follow_huge_addr routine that does not return error. Instead, at each level of the page table a check is made for a hugetlb entry. If a hugetlb entry is found, a call to a routine associated with that page table level such as follow_huge_pmd is made.

All the follow_huge_p*d routines are basically the same. In addition huge page size can be derived from the vma, so we know where in the page table a huge page would reside. So, replace follow_huge_addr with a new architecture independent routine which will provide the same functionality as the follow_huge_p*d routines. We can then eliminate the p*d_huge checks in follow_page_mask page table walking as well as the follow_huge_p*d routines themselves.>
follow_page_mask still has is_hugepd hugetlb checks during page table walking. This is due to these checks and follow_huge_pd being architecture specific. These can be eliminated if hugetlb_follow_page_mask can be overwritten by architectures (powerpc) that need to do follow_huge_pd processing.

mm/hugetlb.c +38 -0

+struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma,
+				unsigned long address, unsigned int flags)
+{
+	struct hstate *h = hstate_vma(vma);
+	struct mm_struct *mm = vma->vm_mm;
+	unsigned long haddr = address & huge_page_mask(h);
+	struct page *page = NULL;
+	spinlock_t *ptl;
+	pte_t *pte, entry;
+
+	/*
+	 * FOLL_PIN is not supported for follow_page(). Ordinary GUP goes via
+	 * follow_hugetlb_page().
+	 */
+	if (WARN_ON_ONCE(flags & FOLL_PIN))
+		return NULL;
+
+	pte = huge_pte_offset(mm, haddr, huge_page_size(h));
+	if (!pte)
+		return NULL;
+
+retry:
+	ptl = huge_pte_lock(h, mm, pte);
+	entry = huge_ptep_get(pte);
+	if (pte_present(entry)) {
+		page = pte_page(entry);
+		/*
+		 * try_grab_page() should always succeed here, because we hold
+		 * the ptl lock and have verified pte_present().
+		 */
+		if (WARN_ON_ONCE(!try_grab_page(page, flags))) {
+			page = NULL;
+			goto out;
+		}
+	} else {
+		if (is_hugetlb_entry_migration(entry)) {
+			spin_unlock(ptl);
+			__migration_entry_wait_huge(pte, ptl);

Quoted 7 lines

Is the primary concern the locking? If so, I am not sure we have an issue. As mentioned in your commit message, current code will use pte_offset_map_lock(). pte_offset_map_lock uses pte_lockptr, and pte_lockptr will either be the mm wide lock or pmd_page lock. To me, it seems that either would provide correct synchronization for CONT-PTE entries. Am I missing something or misreading the code?

Quoted 7 lines

I started looking at code cleanup suggested by David. Here is a quick patch (not tested and likely containing errors) to see if this is a step in the right direction.

I like it because we get rid of/combine all those follow_huge_p*d routines.

Yes, see comments below.

Quoted 14 lines

From 35d117a707c1567ddf350554298697d40eace0d7 Mon Sep 17 00:00:00 2001
From: Mike Kravetz <[email protected]>
Date: Wed, 24 Aug 2022 15:59:15 -0700
Subject: [PATCH] hugetlb: call hugetlb_follow_page_mask for hugetlb pages in
follow_page_mask

At the beginning of follow_page_mask, there currently is a call to follow_huge_addr which 'may' handle hugetlb pages. ia64 is the only architecture which (incorrectly) provides a follow_huge_addr routine that does not return error. Instead, at each level of the page table a check is made for a hugetlb entry. If a hugetlb entry is found, a call to a routine associated with that page table level such as follow_huge_pmd is made.

Quoted 12 lines

All the follow_huge_p*d routines are basically the same. In addition huge page size can be derived from the vma, so we know where in the page table a huge page would reside. So, replace follow_huge_addr with a new architecture independent routine which will provide the same functionality as the follow_huge_p*d routines. We can then eliminate the p*d_huge checks in follow_page_mask page table walking as well as the follow_huge_p*d routines themselves.>
follow_page_mask still has is_hugepd hugetlb checks during page table walking. This is due to these checks and follow_huge_pd being architecture specific. These can be eliminated if hugetlb_follow_page_mask can be overwritten by architectures (powerpc) that need to do follow_huge_pd processing.

But won't the

Review

Quoted 3 lines

(quoted diff) +3 -0

+	/* hugetlb is special */
+	if (is_vm_hugetlb_page(vma))
+		return hugetlb_follow_page_mask(vma, address, flags);

code route everything via hugetlb_follow_page_mask() and all these (beloved) hugepd checks would essentially be unreachable?

At least my understanding is that hugepd only applies to hugetlb.

Can't we move the hugepd handling code into hugetlb_follow_page_mask() as well?

I mean, doesn't follow_hugetlb_page() also have to handle that hugepd stuff already ... ?

Quoted 39 lines

(quoted diff) +38 -0

+struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma,
+				unsigned long address, unsigned int flags)
+{
+	struct hstate *h = hstate_vma(vma);
+	struct mm_struct *mm = vma->vm_mm;
+	unsigned long haddr = address & huge_page_mask(h);
+	struct page *page = NULL;
+	spinlock_t *ptl;
+	pte_t *pte, entry;
+
+	/*
+	 * FOLL_PIN is not supported for follow_page(). Ordinary GUP goes via
+	 * follow_hugetlb_page().
+	 */
+	if (WARN_ON_ONCE(flags & FOLL_PIN))
+		return NULL;
+
+	pte = huge_pte_offset(mm, haddr, huge_page_size(h));
+	if (!pte)
+		return NULL;
+
+retry:
+	ptl = huge_pte_lock(h, mm, pte);
+	entry = huge_ptep_get(pte);
+	if (pte_present(entry)) {
+		page = pte_page(entry);
+		/*
+		 * try_grab_page() should always succeed here, because we hold
+		 * the ptl lock and have verified pte_present().
+		 */
+		if (WARN_ON_ONCE(!try_grab_page(page, flags))) {
+			page = NULL;
+			goto out;
+		}
+	} else {
+		if (is_hugetlb_entry_migration(entry)) {
+			spin_unlock(ptl);
+			__migration_entry_wait_huge(pte, ptl);

Review

Quoted 10 lines

(quoted diff) +10 -0

+			goto retry;
+		}
+		/*
+		 * hwpoisoned entry is treated as no_page_table in
+		 * follow_page_mask().
+		 */
+	}
+out:
+	spin_unlock(ptl);
+	return page;

This is neat and clean enough to not reuse follow_hugetlb_page(). I wonder if we want to add some comment to the function how this differs to follow_hugetlb_page().

... or do we maybe want to rename follow_hugetlb_page() to someting like __hugetlb_get_user_pages() to make it clearer in which context it will get called?

I guess it might be feasible in the future to eliminate follow_hugetlb_page() and centralizing the faulting code. For now, this certainly improves the situation.

Quoted 7 lines

Is the primary concern the locking? If so, I am not sure we have an issue. As mentioned in your commit message, current code will use pte_offset_map_lock(). pte_offset_map_lock uses pte_lockptr, and pte_lockptr will either be the mm wide lock or pmd_page lock. To me, it seems that either would provide correct synchronization for CONT-PTE entries. Am I missing something or misreading the code?

Quoted 7 lines

I started looking at code cleanup suggested by David. Here is a quick patch (not tested and likely containing errors) to see if this is a step in the right direction.

I like it because we get rid of/combine all those follow_huge_p*d routines.

Yes, see comments below.

Quoted 14 lines

From 35d117a707c1567ddf350554298697d40eace0d7 Mon Sep 17 00:00:00 2001
From: Mike Kravetz <[email protected]>
Date: Wed, 24 Aug 2022 15:59:15 -0700
Subject: [PATCH] hugetlb: call hugetlb_follow_page_mask for hugetlb pages in
follow_page_mask

At the beginning of follow_page_mask, there currently is a call to follow_huge_addr which 'may' handle hugetlb pages. ia64 is the only architecture which (incorrectly) provides a follow_huge_addr routine that does not return error. Instead, at each level of the page table a check is made for a hugetlb entry. If a hugetlb entry is found, a call to a routine associated with that page table level such as follow_huge_pmd is made.

Quoted 12 lines

All the follow_huge_p*d routines are basically the same. In addition huge page size can be derived from the vma, so we know where in the page table a huge page would reside. So, replace follow_huge_addr with a new architecture independent routine which will provide the same functionality as the follow_huge_p*d routines. We can then eliminate the p*d_huge checks in follow_page_mask page table walking as well as the follow_huge_p*d routines themselves.>
follow_page_mask still has is_hugepd hugetlb checks during page table walking. This is due to these checks and follow_huge_pd being architecture specific. These can be eliminated if hugetlb_follow_page_mask can be overwritten by architectures (powerpc) that need to do follow_huge_pd processing.

But won't the

mm/gup.c static struct page *follow_page_mask(struct vm_area_struct *vma, · line 844

Quoted 3 lines

mm/gup.c +3 -0

+	/* hugetlb is special */
+	if (is_vm_hugetlb_page(vma))
+		return hugetlb_follow_page_mask(vma, address, flags);

code route everything via hugetlb_follow_page_mask() and all these (beloved) hugepd checks would essentially be unreachable?

At least my understanding is that hugepd only applies to hugetlb.

Can't we move the hugepd handling code into hugetlb_follow_page_mask() as well?

I mean, doesn't follow_hugetlb_page() also have to handle that hugepd stuff already ... ?

[...]

Quoted 55 lines · David Hildenbrand

Quoted 7 lines

Is the primary concern the locking? If so, I am not sure we have an issue. As mentioned in your commit message, current code will use pte_offset_map_lock(). pte_offset_map_lock uses pte_lockptr, and pte_lockptr will either be the mm wide lock or pmd_page lock. To me, it seems that either would provide correct synchronization for CONT-PTE entries. Am I missing something or misreading the code?

Quoted 7 lines

I started looking at code cleanup suggested by David. Here is a quick patch (not tested and likely containing errors) to see if this is a step in the right direction.

I like it because we get rid of/combine all those follow_huge_p*d routines.

Yes, see comments below.

Quoted 14 lines

From 35d117a707c1567ddf350554298697d40eace0d7 Mon Sep 17 00:00:00 2001
From: Mike Kravetz <[email protected]>
Date: Wed, 24 Aug 2022 15:59:15 -0700
Subject: [PATCH] hugetlb: call hugetlb_follow_page_mask for hugetlb pages in
follow_page_mask

At the beginning of follow_page_mask, there currently is a call to follow_huge_addr which 'may' handle hugetlb pages. ia64 is the only architecture which (incorrectly) provides a follow_huge_addr routine that does not return error. Instead, at each level of the page table a check is made for a hugetlb entry. If a hugetlb entry is found, a call to a routine associated with that page table level such as follow_huge_pmd is made.

Quoted 12 lines

All the follow_huge_p*d routines are basically the same. In addition huge page size can be derived from the vma, so we know where in the page table a huge page would reside. So, replace follow_huge_addr with a new architecture independent routine which will provide the same functionality as the follow_huge_p*d routines. We can then eliminate the p*d_huge checks in follow_page_mask page table walking as well as the follow_huge_p*d routines themselves.>
follow_page_mask still has is_hugepd hugetlb checks during page table walking. This is due to these checks and follow_huge_pd being architecture specific. These can be eliminated if hugetlb_follow_page_mask can be overwritten by architectures (powerpc) that need to do follow_huge_pd processing.

But won't the

mm/gup.c static struct page *follow_page_mask(struct vm_area_struct *vma, · line 844

Quoted 3 lines

mm/gup.c +3 -0

+	/* hugetlb is special */
+	if (is_vm_hugetlb_page(vma))
+		return hugetlb_follow_page_mask(vma, address, flags);

code route everything via hugetlb_follow_page_mask() and all these (beloved) hugepd checks would essentially be unreachable?

At least my understanding is that hugepd only applies to hugetlb.

Quoted 7 lines

Can't we move the hugepd handling code into hugetlb_follow_page_mask() as well?

I mean, doesn't follow_hugetlb_page() also have to handle that hugepd stuff already ... ?

[...]

I think so, but I got a little confused looking at the hugepd handling code. Adding Aneesh who added support to follow_page_mask in the series at:
https://lore.kernel.org/linux-mm/1494926612-23928-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com/

I believe you are correct in that follow_hugetlb_page must handle as well.

One source of my confusion is the following in follow_huge_pd:

	/*
	 * hugepage directory entries are protected by mm->page_table_lock
	 * Use this instead of huge_pte_lockptr
	 */
	ptl = &mm->page_table_lock;
	spin_lock(ptl);

Yet, if follow_hugetlb_page handles hugepd then it is using huge_pte_lockptr to get the lock pointer and is wrong?

Hoping Aneesh can help clear up the confusion.

BTW, I also noticed that the above series added the comment:

	/* make this handle hugepd */

above the call to follow_huge_addr() in follow_page_mask. Perhaps there was at one time a plan to have follow_huge_addr handle hugepd? That series removed powerpc specific follow_huge_addr routine.

Signature · 1 line

Mike Kravetz

Quoted 55 lines · David Hildenbrand

Quoted 7 lines

Is the primary concern the locking? If so, I am not sure we have an issue. As mentioned in your commit message, current code will use pte_offset_map_lock(). pte_offset_map_lock uses pte_lockptr, and pte_lockptr will either be the mm wide lock or pmd_page lock. To me, it seems that either would provide correct synchronization for CONT-PTE entries. Am I missing something or misreading the code?

Quoted 7 lines

I started looking at code cleanup suggested by David. Here is a quick patch (not tested and likely containing errors) to see if this is a step in the right direction.

I like it because we get rid of/combine all those follow_huge_p*d routines.

Yes, see comments below.

Quoted 14 lines

From 35d117a707c1567ddf350554298697d40eace0d7 Mon Sep 17 00:00:00 2001
From: Mike Kravetz <[email protected]>
Date: Wed, 24 Aug 2022 15:59:15 -0700
Subject: [PATCH] hugetlb: call hugetlb_follow_page_mask for hugetlb pages in
follow_page_mask

At the beginning of follow_page_mask, there currently is a call to follow_huge_addr which 'may' handle hugetlb pages. ia64 is the only architecture which (incorrectly) provides a follow_huge_addr routine that does not return error. Instead, at each level of the page table a check is made for a hugetlb entry. If a hugetlb entry is found, a call to a routine associated with that page table level such as follow_huge_pmd is made.

Quoted 12 lines

All the follow_huge_p*d routines are basically the same. In addition huge page size can be derived from the vma, so we know where in the page table a huge page would reside. So, replace follow_huge_addr with a new architecture independent routine which will provide the same functionality as the follow_huge_p*d routines. We can then eliminate the p*d_huge checks in follow_page_mask page table walking as well as the follow_huge_p*d routines themselves.>
follow_page_mask still has is_hugepd hugetlb checks during page table walking. This is due to these checks and follow_huge_pd being architecture specific. These can be eliminated if hugetlb_follow_page_mask can be overwritten by architectures (powerpc) that need to do follow_huge_pd processing.

But won't the

mm/gup.c static struct page *follow_page_mask(struct vm_area_struct *vma, · line 844

Quoted 3 lines

mm/gup.c +3 -0

+	/* hugetlb is special */
+	if (is_vm_hugetlb_page(vma))
+		return hugetlb_follow_page_mask(vma, address, flags);

code route everything via hugetlb_follow_page_mask() and all these (beloved) hugepd checks would essentially be unreachable?

At least my understanding is that hugepd only applies to hugetlb.

Quoted 7 lines

Can't we move the hugepd handling code into hugetlb_follow_page_mask() as well?

I mean, doesn't follow_hugetlb_page() also have to handle that hugepd stuff already ... ?

[...]

I think so, but I got a little confused looking at the hugepd handling code. Adding Aneesh who added support to follow_page_mask in the series at:
https://lore.kernel.org/linux-mm/1494926612-23928-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com/

I believe you are correct in that follow_hugetlb_page must handle as well.

One source of my confusion is the following in follow_huge_pd:

	/*
	 * hugepage directory entries are protected by mm->page_table_lock
	 * Use this instead of huge_pte_lockptr
	 */
	ptl = &mm->page_table_lock;
	spin_lock(ptl);

Yet, if follow_hugetlb_page handles hugepd then it is using huge_pte_lockptr to get the lock pointer and is wrong?

Hoping Aneesh can help clear up the confusion.

BTW, I also noticed that the above series added the comment:

	/* make this handle hugepd */

above the call to follow_huge_addr() in follow_page_mask. Perhaps there was at one time a plan to have follow_huge_addr handle hugepd? That series removed powerpc specific follow_huge_addr routine.

Signature · 1 line

Mike Kravetz

Quoted 80 lines · Mike Kravetz

Quoted 55 lines · David Hildenbrand

Quoted 7 lines

Is the primary concern the locking? If so, I am not sure we have an issue. As mentioned in your commit message, current code will use pte_offset_map_lock(). pte_offset_map_lock uses pte_lockptr, and pte_lockptr will either be the mm wide lock or pmd_page lock. To me, it seems that either would provide correct synchronization for CONT-PTE entries. Am I missing something or misreading the code?

Quoted 7 lines

I started looking at code cleanup suggested by David. Here is a quick patch (not tested and likely containing errors) to see if this is a step in the right direction.

I like it because we get rid of/combine all those follow_huge_p*d routines.

Yes, see comments below.

Quoted 14 lines

From 35d117a707c1567ddf350554298697d40eace0d7 Mon Sep 17 00:00:00 2001
From: Mike Kravetz <[email protected]>
Date: Wed, 24 Aug 2022 15:59:15 -0700
Subject: [PATCH] hugetlb: call hugetlb_follow_page_mask for hugetlb pages in
follow_page_mask

At the beginning of follow_page_mask, there currently is a call to follow_huge_addr which 'may' handle hugetlb pages. ia64 is the only architecture which (incorrectly) provides a follow_huge_addr routine that does not return error. Instead, at each level of the page table a check is made for a hugetlb entry. If a hugetlb entry is found, a call to a routine associated with that page table level such as follow_huge_pmd is made.

Quoted 12 lines

All the follow_huge_p*d routines are basically the same. In addition huge page size can be derived from the vma, so we know where in the page table a huge page would reside. So, replace follow_huge_addr with a new architecture independent routine which will provide the same functionality as the follow_huge_p*d routines. We can then eliminate the p*d_huge checks in follow_page_mask page table walking as well as the follow_huge_p*d routines themselves.>
follow_page_mask still has is_hugepd hugetlb checks during page table walking. This is due to these checks and follow_huge_pd being architecture specific. These can be eliminated if hugetlb_follow_page_mask can be overwritten by architectures (powerpc) that need to do follow_huge_pd processing.

But won't the

mm/gup.c static struct page *follow_page_mask(struct vm_area_struct *vma, · line 844

Quoted 3 lines

mm/gup.c +3 -0

+	/* hugetlb is special */
+	if (is_vm_hugetlb_page(vma))
+		return hugetlb_follow_page_mask(vma, address, flags);

code route everything via hugetlb_follow_page_mask() and all these (beloved) hugepd checks would essentially be unreachable?

At least my understanding is that hugepd only applies to hugetlb.

Quoted 7 lines

Can't we move the hugepd handling code into hugetlb_follow_page_mask() as well?

I mean, doesn't follow_hugetlb_page() also have to handle that hugepd stuff already ... ?

[...]

I think so, but I got a little confused looking at the hugepd handling code. Adding Aneesh who added support to follow_page_mask in the series at:
https://lore.kernel.org/linux-mm/1494926612-23928-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com/

I believe you are correct in that follow_hugetlb_page must handle as well.

One source of my confusion is the following in follow_huge_pd:

	/*
	 * hugepage directory entries are protected by mm->page_table_lock
	 * Use this instead of huge_pte_lockptr
	 */
	ptl = &mm->page_table_lock;
	spin_lock(ptl);

Quoted 4 lines

Yet, if follow_hugetlb_page handles hugepd then it is using huge_pte_lockptr to get the lock pointer and is wrong?

Hoping Aneesh can help clear up the confusion.

I agree it is all confusing. At some point, the goal was to teach generic kernel page table walking code about hugepd entries. But looking at this again and considering we only have hugepd entries for hugetlb, may be the effort is not worth the complexity it adds.

ie, instead of teaching generic page table walk about different hugetlb page table layouts we special case using is_vm_hugetlb_page(vma) wherever we can.

With respect to huge_pte_lockptr, it is tricky (hugepd entries are not PMD_SIZE)

static inline spinlock_t *huge_pte_lockptr(struct hstate *h,
struct mm_struct *mm, pte_t *pte)

{
	if (huge_page_size(h) == PMD_SIZE)
		return pmd_lockptr(mm, (pmd_t *) pte);
	VM_BUG_ON(huge_page_size(h) == PAGE_SIZE);
	return &mm->page_table_lock;
}

Considering the pmd entries of a CONT-PMD hugetlb can not span on multiple PMDs, we can change to use the PMD page lock, which can be much finer grain that lock in the mm.

Signed-off-by Baolin Wang <[email protected]>

---
 include/linux/hugetlb.h | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

include/linux/hugetlb.h +3 -2

index 3a96f67..d4803a89 100644
@@ -892,9 +892,17 @@ static inline gfp_t htlb_modify_alloc_mask(struct hstate *h, gfp_t gfp_mask)
 static inline spinlock_t *huge_pte_lockptr(struct hstate *h,
 					   struct mm_struct *mm, pte_t *pte)
 {
-	VM_BUG_ON(huge_page_size(h) == PAGE_SIZE);
+	unsigned long hp_size = huge_page_size(h);
 
-	if (huge_page_size(h) == PMD_SIZE) {
+	VM_BUG_ON(hp_size == PAGE_SIZE);
+

Quoted 20 lines · Baolin Wang

Considering the pmd entries of a CONT-PMD hugetlb can not span on multiple PMDs, we can change to use the PMD page lock, which can be much finer grain that lock in the mm.

Signed-off-by Baolin Wang <[email protected]>

---
  include/linux/hugetlb.h | 12 ++++++++++--
  1 file changed, 10 insertions(+), 2 deletions(-)

include/linux/hugetlb.h +1 -1

index 3a96f67..d4803a89 100644
@@ -892,9 +892,17 @@ static inline gfp_t htlb_modify_alloc_mask(struct hstate *h, gfp_t gfp_mask)
  static inline spinlock_t *huge_pte_lockptr(struct hstate *h,
  					   struct mm_struct *mm, pte_t *pte)
  {
-	VM_BUG_ON(huge_page_size(h) == PAGE_SIZE);
+	unsigned long hp_size = huge_page_size(h);
 

include/linux/hugetlb.h static inline gfp_t htlb_modify_alloc_mask(struct hstate *h, gfp_t gfp_mask) · line 908

Quoted 13 lines

include/linux/hugetlb.h +9 -1

-	if (huge_page_size(h) == PMD_SIZE) {
+	VM_BUG_ON(hp_size == PAGE_SIZE);
+
+	/*
+	 * Considering CONT-PMD size hugetlb, since the CONT-PMD entry
+	 * can not span multiple PMDs, then we can use the fine grained
+	 * PMD page lock.
+	 */
+	if (hp_size == PMD_SIZE ||
+	    (hp_size > PMD_SIZE && hp_size < PUD_SIZE)) {
  		return pmd_lockptr(mm, (pmd_t *) pte);
  	} else if (huge_page_size(h) < PMD_SIZE) {
  		unsigned long mask = ~(PTRS_PER_PTE * sizeof(pte_t) - 1);

Is there a measurable performance gain? IOW, do we really care?