[PATCH v3 00/13] mm/debug_vm_pgtable fixes

[PATCH v3 00/13] mm/debug_vm_pgtable fixes

[Aneesh Kumar K.V]

This patch series includes fixes for debug_vm_pgtable test code so that
they follow page table updates rules correctly. The first two patches introduce
changes w.r.t ppc64. The patches are included in this series for completeness. We can
merge them via ppc64 tree if required.

Hugetlb test is disabled on ppc64 because that needs larger change to satisfy
page table update rules.

The patches are on top of 15bc20c6af4ceee97a1f90b43c0e386643c071b4 (linus/master)

Changes from v2:
* Fix build failure with different configs and architecture.

Changes from v1:
* Address review feedback
* drop test specific pfn_pte and pfn_pmd.
* Update ppc64 page table helper to add _PAGE_PTE 


Aneesh Kumar K.V (13):
  powerpc/mm: Add DEBUG_VM WARN for pmd_clear
  powerpc/mm: Move setting pte specific flags to pfn_pte
  mm/debug_vm_pgtable/ppc64: Avoid setting top bits in radom value
  mm/debug_vm_pgtables/hugevmap: Use the arch helper to identify huge
    vmap support.
  mm/debug_vm_pgtable/savedwrite: Enable savedwrite test with
    CONFIG_NUMA_BALANCING
  mm/debug_vm_pgtable/THP: Mark the pte entry huge before using
    set_pmd/pud_at
  mm/debug_vm_pgtable/set_pte/pmd/pud: Don't use set_*_at to update an
    existing pte entry
  mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP
  mm/debug_vm_pgtable/locks: Move non page table modifying test together
  mm/debug_vm_pgtable/locks: Take correct page table lock
  mm/debug_vm_pgtable/pmd_clear: Don't use pmd/pud_clear on pte entries
  mm/debug_vm_pgtable/hugetlb: Disable hugetlb test on ppc64
  mm/debug_vm_pgtable: populate a pte entry before fetching it

 arch/powerpc/include/asm/book3s/64/pgtable.h |  29 +++-
 arch/powerpc/include/asm/nohash/pgtable.h    |   5 -
 arch/powerpc/mm/pgtable.c                    |   5 -
 mm/debug_vm_pgtable.c                        | 170 ++++++++++++-------
 4 files changed, 131 insertions(+), 78 deletions(-)

-- 
2.26.2

[PATCH v3 01/13] powerpc/mm: Add DEBUG_VM WARN for pmd_clear

[Aneesh Kumar K.V]

With the hash page table, the kernel should not use pmd_clear for clearing
huge pte entries. Add a DEBUG_VM WARN to catch the wrong usage.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
 arch/powerpc/include/asm/book3s/64/pgtable.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 6de56c3b33c4..079211968987 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -868,6 +868,13 @@ static inline bool pte_ci(pte_t pte)
 
 static inline void pmd_clear(pmd_t *pmdp)
 {
+	if (IS_ENABLED(CONFIG_DEBUG_VM) && !radix_enabled()) {
+		/*
+		 * Don't use this if we can possibly have a hash page table
+		 * entry mapping this.
+		 */
+		WARN_ON((pmd_val(*pmdp) & (H_PAGE_HASHPTE | _PAGE_PTE)) == (H_PAGE_HASHPTE | _PAGE_PTE));
+	}
 	*pmdp = __pmd(0);
 }
 
@@ -916,6 +923,13 @@ static inline int pmd_bad(pmd_t pmd)
 
 static inline void pud_clear(pud_t *pudp)
 {
+	if (IS_ENABLED(CONFIG_DEBUG_VM) && !radix_enabled()) {
+		/*
+		 * Don't use this if we can possibly have a hash page table
+		 * entry mapping this.
+		 */
+		WARN_ON((pud_val(*pudp) & (H_PAGE_HASHPTE | _PAGE_PTE)) == (H_PAGE_HASHPTE | _PAGE_PTE));
+	}
 	*pudp = __pud(0);
 }
 
-- 
2.26.2

[PATCH v3 02/13] powerpc/mm: Move setting pte specific flags to pfn_pte

[Aneesh Kumar K.V]

powerpc used to set the pte specific flags in set_pte_at(). This is different
from other architectures. To be consistent with other architecture update
pfn_pte to set _PAGE_PTE on ppc64. Also, drop now unused pte_mkpte.

We add a VM_WARN_ON() to catch the usage of calling set_pte_at() without setting
_PAGE_PTE bit. We will remove that after a few releases.

With respect to huge pmd entries, pmd_mkhuge() takes care of adding the
_PAGE_PTE bit.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
 arch/powerpc/include/asm/book3s/64/pgtable.h | 15 +++++++++------
 arch/powerpc/include/asm/nohash/pgtable.h    |  5 -----
 arch/powerpc/mm/pgtable.c                    |  5 -----
 3 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 079211968987..2382fd516f6b 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -619,7 +619,7 @@ static inline pte_t pfn_pte(unsigned long pfn, pgprot_t pgprot)
 	VM_BUG_ON(pfn >> (64 - PAGE_SHIFT));
 	VM_BUG_ON((pfn << PAGE_SHIFT) & ~PTE_RPN_MASK);
 
-	return __pte(((pte_basic_t)pfn << PAGE_SHIFT) | pgprot_val(pgprot));
+	return __pte(((pte_basic_t)pfn << PAGE_SHIFT) | pgprot_val(pgprot) | _PAGE_PTE);
 }
 
 static inline unsigned long pte_pfn(pte_t pte)
@@ -655,11 +655,6 @@ static inline pte_t pte_mkexec(pte_t pte)
 	return __pte_raw(pte_raw(pte) | cpu_to_be64(_PAGE_EXEC));
 }
 
-static inline pte_t pte_mkpte(pte_t pte)
-{
-	return __pte_raw(pte_raw(pte) | cpu_to_be64(_PAGE_PTE));
-}
-
 static inline pte_t pte_mkwrite(pte_t pte)
 {
 	/*
@@ -823,6 +818,14 @@ static inline int pte_none(pte_t pte)
 static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
 				pte_t *ptep, pte_t pte, int percpu)
 {
+
+	VM_WARN_ON(!(pte_raw(pte) & cpu_to_be64(_PAGE_PTE)));
+	/*
+	 * Keep the _PAGE_PTE added till we are sure we handle _PAGE_PTE
+	 * in all the callers.
+	 */
+	 pte = __pte_raw(pte_raw(pte) | cpu_to_be64(_PAGE_PTE));
+
 	if (radix_enabled())
 		return radix__set_pte_at(mm, addr, ptep, pte, percpu);
 	return hash__set_pte_at(mm, addr, ptep, pte, percpu);
diff --git a/arch/powerpc/include/asm/nohash/pgtable.h b/arch/powerpc/include/asm/nohash/pgtable.h
index 4b7c3472eab1..6277e7596ae5 100644
--- a/arch/powerpc/include/asm/nohash/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/pgtable.h
@@ -140,11 +140,6 @@ static inline pte_t pte_mkold(pte_t pte)
 	return __pte(pte_val(pte) & ~_PAGE_ACCESSED);
 }
 
-static inline pte_t pte_mkpte(pte_t pte)
-{
-	return pte;
-}
-
 static inline pte_t pte_mkspecial(pte_t pte)
 {
 	return __pte(pte_val(pte) | _PAGE_SPECIAL);
diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
index 9c0547d77af3..ab57b07ef39a 100644
--- a/arch/powerpc/mm/pgtable.c
+++ b/arch/powerpc/mm/pgtable.c
@@ -184,9 +184,6 @@ void set_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep,
 	 */
 	VM_WARN_ON(pte_hw_valid(*ptep) && !pte_protnone(*ptep));
 
-	/* Add the pte bit when trying to set a pte */
-	pte = pte_mkpte(pte);
-
 	/* Note: mm->context.id might not yet have been assigned as
 	 * this context might not have been activated yet when this
 	 * is called.
@@ -275,8 +272,6 @@ void set_huge_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep, pte_
 	 */
 	VM_WARN_ON(pte_hw_valid(*ptep) && !pte_protnone(*ptep));
 
-	pte = pte_mkpte(pte);
-
 	pte = set_pte_filter(pte);
 
 	val = pte_val(pte);
-- 
2.26.2

[PATCH v3 03/13] mm/debug_vm_pgtable/ppc64: Avoid setting top bits in radom value

[Aneesh Kumar K.V]

ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
random value.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
 mm/debug_vm_pgtable.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
index 086309fb9b6f..bbf9df0e64c6 100644
--- a/mm/debug_vm_pgtable.c
+++ b/mm/debug_vm_pgtable.c
@@ -44,10 +44,17 @@
  * entry type. But these bits might affect the ability to clear entries with
  * pxx_clear() because of how dynamic page table folding works on s390. So
  * while loading up the entries do not change the lower 4 bits. It does not
- * have affect any other platform.
+ * have affect any other platform. Also avoid the 62nd bit on ppc64 that is
+ * used to mark a pte entry.
  */
-#define S390_MASK_BITS	4
-#define RANDOM_ORVALUE	GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS)
+#define S390_SKIP_MASK		GENMASK(3, 0)
+#ifdef CONFIG_PPC_BOOK3S_64
+#define PPC64_SKIP_MASK		GENMASK(62, 62)
+#else
+#define PPC64_SKIP_MASK		0x0
+#endif
+#define ARCH_SKIP_MASK (S390_SKIP_MASK | PPC64_SKIP_MASK)
+#define RANDOM_ORVALUE (GENMASK(BITS_PER_LONG - 1, 0) & ~ARCH_SKIP_MASK)
 #define RANDOM_NZVALUE	GENMASK(7, 0)
 
 static void __init pte_basic_tests(unsigned long pfn, pgprot_t prot)
-- 
2.26.2

[PATCH v3 04/13] mm/debug_vm_pgtables/hugevmap: Use the arch helper to identify huge vmap support.

[Aneesh Kumar K.V]

ppc64 supports huge vmap only with radix translation. Hence use arch helper
to determine the huge vmap support.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
 mm/debug_vm_pgtable.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
index bbf9df0e64c6..28f9d0558c20 100644
--- a/mm/debug_vm_pgtable.c
+++ b/mm/debug_vm_pgtable.c
@@ -28,6 +28,7 @@
 #include <linux/swapops.h>
 #include <linux/start_kernel.h>
 #include <linux/sched/mm.h>
+#include <linux/io.h>
 #include <asm/pgalloc.h>
 #include <asm/tlbflush.h>
 
@@ -206,11 +207,12 @@ static void __init pmd_leaf_tests(unsigned long pfn, pgprot_t prot)
 	WARN_ON(!pmd_leaf(pmd));
 }
 
+#ifdef CONFIG_HAVE_ARCH_HUGE_VMAP
 static void __init pmd_huge_tests(pmd_t *pmdp, unsigned long pfn, pgprot_t prot)
 {
 	pmd_t pmd;
 
-	if (!IS_ENABLED(CONFIG_HAVE_ARCH_HUGE_VMAP))
+	if (!arch_ioremap_pmd_supported())
 		return;
 
 	pr_debug("Validating PMD huge\n");
@@ -224,6 +226,10 @@ static void __init pmd_huge_tests(pmd_t *pmdp, unsigned long pfn, pgprot_t prot)
 	pmd = READ_ONCE(*pmdp);
 	WARN_ON(!pmd_none(pmd));
 }
+#else /* !CONFIG_HAVE_ARCH_HUGE_VMAP */
+static void __init pmd_huge_tests(pmd_t *pmdp, unsigned long pfn, pgprot_t prot) { }
+#endif /* !CONFIG_HAVE_ARCH_HUGE_VMAP */
+
 
 static void __init pmd_savedwrite_tests(unsigned long pfn, pgprot_t prot)
 {
@@ -320,11 +326,12 @@ static void __init pud_leaf_tests(unsigned long pfn, pgprot_t prot)
 	WARN_ON(!pud_leaf(pud));
 }
 
+#ifdef CONFIG_HAVE_ARCH_HUGE_VMAP
 static void __init pud_huge_tests(pud_t *pudp, unsigned long pfn, pgprot_t prot)
 {
 	pud_t pud;
 
-	if (!IS_ENABLED(CONFIG_HAVE_ARCH_HUGE_VMAP))
+	if (!arch_ioremap_pud_supported())
 		return;
 
 	pr_debug("Validating PUD huge\n");
@@ -338,6 +345,10 @@ static void __init pud_huge_tests(pud_t *pudp, unsigned long pfn, pgprot_t prot)
 	pud = READ_ONCE(*pudp);
 	WARN_ON(!pud_none(pud));
 }
+#else /* !CONFIG_HAVE_ARCH_HUGE_VMAP */
+static void __init pud_huge_tests(pud_t *pudp, unsigned long pfn, pgprot_t prot) { }
+#endif /* !CONFIG_HAVE_ARCH_HUGE_VMAP */
+
 #else  /* !CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD */
 static void __init pud_basic_tests(unsigned long pfn, pgprot_t prot) { }
 static void __init pud_advanced_tests(struct mm_struct *mm,
-- 
2.26.2

[PATCH v3 05/13] mm/debug_vm_pgtable/savedwrite: Enable savedwrite test with CONFIG_NUMA_BALANCING

[Aneesh Kumar K.V]

Saved write support was added to track the write bit of a pte after marking the
pte protnone. This was done so that AUTONUMA can convert a write pte to protnone
and still track the old write bit. When converting it back we set the pte write
bit correctly thereby avoiding a write fault again. Hence enable the test only
when CONFIG_NUMA_BALANCING is enabled and use protnone protflags.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
 mm/debug_vm_pgtable.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
index 28f9d0558c20..5c0680836fe9 100644
--- a/mm/debug_vm_pgtable.c
+++ b/mm/debug_vm_pgtable.c
@@ -119,10 +119,14 @@ static void __init pte_savedwrite_tests(unsigned long pfn, pgprot_t prot)
 {
 	pte_t pte = pfn_pte(pfn, prot);
 
+	if (!IS_ENABLED(CONFIG_NUMA_BALANCING))
+		return;
+
 	pr_debug("Validating PTE saved write\n");
 	WARN_ON(!pte_savedwrite(pte_mk_savedwrite(pte_clear_savedwrite(pte))));
 	WARN_ON(pte_savedwrite(pte_clear_savedwrite(pte_mk_savedwrite(pte))));
 }
+
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 static void __init pmd_basic_tests(unsigned long pfn, pgprot_t prot)
 {
@@ -235,6 +239,9 @@ static void __init pmd_savedwrite_tests(unsigned long pfn, pgprot_t prot)
 {
 	pmd_t pmd = pfn_pmd(pfn, prot);
 
+	if (!IS_ENABLED(CONFIG_NUMA_BALANCING))
+		return;
+
 	pr_debug("Validating PMD saved write\n");
 	WARN_ON(!pmd_savedwrite(pmd_mk_savedwrite(pmd_clear_savedwrite(pmd))));
 	WARN_ON(pmd_savedwrite(pmd_clear_savedwrite(pmd_mk_savedwrite(pmd))));
@@ -1020,8 +1027,8 @@ static int __init debug_vm_pgtable(void)
 	pmd_huge_tests(pmdp, pmd_aligned, prot);
 	pud_huge_tests(pudp, pud_aligned, prot);
 
-	pte_savedwrite_tests(pte_aligned, prot);
-	pmd_savedwrite_tests(pmd_aligned, prot);
+	pte_savedwrite_tests(pte_aligned, protnone);
+	pmd_savedwrite_tests(pmd_aligned, protnone);
 
 	pte_unmap_unlock(ptep, ptl);
 
-- 
2.26.2

[PATCH v3 06/13] mm/debug_vm_pgtable/THP: Mark the pte entry huge before using set_pmd/pud_at

[Aneesh Kumar K.V]

kernel expects entries to be marked huge before we use set_pmd_at()/set_pud_at().

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
 mm/debug_vm_pgtable.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
index 5c0680836fe9..de83a20c1b30 100644
--- a/mm/debug_vm_pgtable.c
+++ b/mm/debug_vm_pgtable.c
@@ -155,7 +155,7 @@ static void __init pmd_advanced_tests(struct mm_struct *mm,
 				      unsigned long pfn, unsigned long vaddr,
 				      pgprot_t prot)
 {
-	pmd_t pmd = pfn_pmd(pfn, prot);
+	pmd_t pmd;
 
 	if (!has_transparent_hugepage())
 		return;
@@ -164,19 +164,19 @@ static void __init pmd_advanced_tests(struct mm_struct *mm,
 	/* Align the address wrt HPAGE_PMD_SIZE */
 	vaddr = (vaddr & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE;
 
-	pmd = pfn_pmd(pfn, prot);
+	pmd = pmd_mkhuge(pfn_pmd(pfn, prot));
 	set_pmd_at(mm, vaddr, pmdp, pmd);
 	pmdp_set_wrprotect(mm, vaddr, pmdp);
 	pmd = READ_ONCE(*pmdp);
 	WARN_ON(pmd_write(pmd));
 
-	pmd = pfn_pmd(pfn, prot);
+	pmd = pmd_mkhuge(pfn_pmd(pfn, prot));
 	set_pmd_at(mm, vaddr, pmdp, pmd);
 	pmdp_huge_get_and_clear(mm, vaddr, pmdp);
 	pmd = READ_ONCE(*pmdp);
 	WARN_ON(!pmd_none(pmd));
 
-	pmd = pfn_pmd(pfn, prot);
+	pmd = pmd_mkhuge(pfn_pmd(pfn, prot));
 	pmd = pmd_wrprotect(pmd);
 	pmd = pmd_mkclean(pmd);
 	set_pmd_at(mm, vaddr, pmdp, pmd);
@@ -237,7 +237,7 @@ static void __init pmd_huge_tests(pmd_t *pmdp, unsigned long pfn, pgprot_t prot)
 
 static void __init pmd_savedwrite_tests(unsigned long pfn, pgprot_t prot)
 {
-	pmd_t pmd = pfn_pmd(pfn, prot);
+	pmd_t pmd = pmd_mkhuge(pfn_pmd(pfn, prot));
 
 	if (!IS_ENABLED(CONFIG_NUMA_BALANCING))
 		return;
@@ -277,7 +277,7 @@ static void __init pud_advanced_tests(struct mm_struct *mm,
 				      unsigned long pfn, unsigned long vaddr,
 				      pgprot_t prot)
 {
-	pud_t pud = pfn_pud(pfn, prot);
+	pud_t pud;
 
 	if (!has_transparent_hugepage())
 		return;
@@ -286,25 +286,28 @@ static void __init pud_advanced_tests(struct mm_struct *mm,
 	/* Align the address wrt HPAGE_PUD_SIZE */
 	vaddr = (vaddr & HPAGE_PUD_MASK) + HPAGE_PUD_SIZE;
 
+	pud = pud_mkhuge(pfn_pud(pfn, prot));
 	set_pud_at(mm, vaddr, pudp, pud);
 	pudp_set_wrprotect(mm, vaddr, pudp);
 	pud = READ_ONCE(*pudp);
 	WARN_ON(pud_write(pud));
 
 #ifndef __PAGETABLE_PMD_FOLDED
-	pud = pfn_pud(pfn, prot);
+
+	pud = pud_mkhuge(pfn_pud(pfn, prot));
 	set_pud_at(mm, vaddr, pudp, pud);
 	pudp_huge_get_and_clear(mm, vaddr, pudp);
 	pud = READ_ONCE(*pudp);
 	WARN_ON(!pud_none(pud));
 
-	pud = pfn_pud(pfn, prot);
+	pud = pud_mkhuge(pfn_pud(pfn, prot));
 	set_pud_at(mm, vaddr, pudp, pud);
 	pudp_huge_get_and_clear_full(mm, vaddr, pudp, 1);
 	pud = READ_ONCE(*pudp);
 	WARN_ON(!pud_none(pud));
 #endif /* __PAGETABLE_PMD_FOLDED */
-	pud = pfn_pud(pfn, prot);
+
+	pud = pud_mkhuge(pfn_pud(pfn, prot));
 	pud = pud_wrprotect(pud);
 	pud = pud_mkclean(pud);
 	set_pud_at(mm, vaddr, pudp, pud);
-- 
2.26.2

[PATCH v3 07/13] mm/debug_vm_pgtable/set_pte/pmd/pud: Don't use set_*_at to update an existing pte entry

[Aneesh Kumar K.V]

set_pte_at() should not be used to set a pte entry at locations that
already holds a valid pte entry. Architectures like ppc64 don't do TLB
invalidate in set_pte_at() and hence expect it to be used to set locations
that are not a valid PTE.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
 mm/debug_vm_pgtable.c | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
index de83a20c1b30..f9f6358899a8 100644
--- a/mm/debug_vm_pgtable.c
+++ b/mm/debug_vm_pgtable.c
@@ -79,15 +79,18 @@ static void __init pte_advanced_tests(struct mm_struct *mm,
 {
 	pte_t pte = pfn_pte(pfn, prot);
 
+	/*
+	 * Architectures optimize set_pte_at by avoiding TLB flush.
+	 * This requires set_pte_at to be not used to update an
+	 * existing pte entry. Clear pte before we do set_pte_at
+	 */
+
 	pr_debug("Validating PTE advanced\n");
 	pte = pfn_pte(pfn, prot);
 	set_pte_at(mm, vaddr, ptep, pte);
 	ptep_set_wrprotect(mm, vaddr, ptep);
 	pte = ptep_get(ptep);
 	WARN_ON(pte_write(pte));
-
-	pte = pfn_pte(pfn, prot);
-	set_pte_at(mm, vaddr, ptep, pte);
 	ptep_get_and_clear(mm, vaddr, ptep);
 	pte = ptep_get(ptep);
 	WARN_ON(!pte_none(pte));
@@ -101,13 +104,11 @@ static void __init pte_advanced_tests(struct mm_struct *mm,
 	ptep_set_access_flags(vma, vaddr, ptep, pte, 1);
 	pte = ptep_get(ptep);
 	WARN_ON(!(pte_write(pte) && pte_dirty(pte)));
-
-	pte = pfn_pte(pfn, prot);
-	set_pte_at(mm, vaddr, ptep, pte);
 	ptep_get_and_clear_full(mm, vaddr, ptep, 1);
 	pte = ptep_get(ptep);
 	WARN_ON(!pte_none(pte));
 
+	pte = pfn_pte(pfn, prot);
 	pte = pte_mkyoung(pte);
 	set_pte_at(mm, vaddr, ptep, pte);
 	ptep_test_and_clear_young(vma, vaddr, ptep);
@@ -169,9 +170,6 @@ static void __init pmd_advanced_tests(struct mm_struct *mm,
 	pmdp_set_wrprotect(mm, vaddr, pmdp);
 	pmd = READ_ONCE(*pmdp);
 	WARN_ON(pmd_write(pmd));
-
-	pmd = pmd_mkhuge(pfn_pmd(pfn, prot));
-	set_pmd_at(mm, vaddr, pmdp, pmd);
 	pmdp_huge_get_and_clear(mm, vaddr, pmdp);
 	pmd = READ_ONCE(*pmdp);
 	WARN_ON(!pmd_none(pmd));
@@ -185,13 +183,11 @@ static void __init pmd_advanced_tests(struct mm_struct *mm,
 	pmdp_set_access_flags(vma, vaddr, pmdp, pmd, 1);
 	pmd = READ_ONCE(*pmdp);
 	WARN_ON(!(pmd_write(pmd) && pmd_dirty(pmd)));
-
-	pmd = pmd_mkhuge(pfn_pmd(pfn, prot));
-	set_pmd_at(mm, vaddr, pmdp, pmd);
 	pmdp_huge_get_and_clear_full(vma, vaddr, pmdp, 1);
 	pmd = READ_ONCE(*pmdp);
 	WARN_ON(!pmd_none(pmd));
 
+	pmd = pmd_mkhuge(pfn_pmd(pfn, prot));
 	pmd = pmd_mkyoung(pmd);
 	set_pmd_at(mm, vaddr, pmdp, pmd);
 	pmdp_test_and_clear_young(vma, vaddr, pmdp);
@@ -293,18 +289,10 @@ static void __init pud_advanced_tests(struct mm_struct *mm,
 	WARN_ON(pud_write(pud));
 
 #ifndef __PAGETABLE_PMD_FOLDED
-
-	pud = pud_mkhuge(pfn_pud(pfn, prot));
-	set_pud_at(mm, vaddr, pudp, pud);
 	pudp_huge_get_and_clear(mm, vaddr, pudp);
 	pud = READ_ONCE(*pudp);
 	WARN_ON(!pud_none(pud));
 
-	pud = pud_mkhuge(pfn_pud(pfn, prot));
-	set_pud_at(mm, vaddr, pudp, pud);
-	pudp_huge_get_and_clear_full(mm, vaddr, pudp, 1);
-	pud = READ_ONCE(*pudp);
-	WARN_ON(!pud_none(pud));
 #endif /* __PAGETABLE_PMD_FOLDED */
 
 	pud = pud_mkhuge(pfn_pud(pfn, prot));
@@ -317,6 +305,13 @@ static void __init pud_advanced_tests(struct mm_struct *mm,
 	pud = READ_ONCE(*pudp);
 	WARN_ON(!(pud_write(pud) && pud_dirty(pud)));
 
+#ifndef __PAGETABLE_PMD_FOLDED
+	pudp_huge_get_and_clear_full(mm, vaddr, pudp, 1);
+	pud = READ_ONCE(*pudp);
+	WARN_ON(!pud_none(pud));
+#endif /* __PAGETABLE_PMD_FOLDED */
+
+	pud = pud_mkhuge(pfn_pud(pfn, prot));
 	pud = pud_mkyoung(pud);
 	set_pud_at(mm, vaddr, pudp, pud);
 	pudp_test_and_clear_young(vma, vaddr, pudp);
-- 
2.26.2

[PATCH v3 08/13] mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP

[Aneesh Kumar K.V]

Architectures like ppc64 use deposited page table while updating the huge pte
entries.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
 mm/debug_vm_pgtable.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
index f9f6358899a8..0ce5c6a24c5b 100644
--- a/mm/debug_vm_pgtable.c
+++ b/mm/debug_vm_pgtable.c
@@ -154,7 +154,7 @@ static void __init pmd_basic_tests(unsigned long pfn, pgprot_t prot)
 static void __init pmd_advanced_tests(struct mm_struct *mm,
 				      struct vm_area_struct *vma, pmd_t *pmdp,
 				      unsigned long pfn, unsigned long vaddr,
-				      pgprot_t prot)
+				      pgprot_t prot, pgtable_t pgtable)
 {
 	pmd_t pmd;
 
@@ -165,6 +165,8 @@ static void __init pmd_advanced_tests(struct mm_struct *mm,
 	/* Align the address wrt HPAGE_PMD_SIZE */
 	vaddr = (vaddr & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE;
 
+	pgtable_trans_huge_deposit(mm, pmdp, pgtable);
+
 	pmd = pmd_mkhuge(pfn_pmd(pfn, prot));
 	set_pmd_at(mm, vaddr, pmdp, pmd);
 	pmdp_set_wrprotect(mm, vaddr, pmdp);
@@ -193,6 +195,8 @@ static void __init pmd_advanced_tests(struct mm_struct *mm,
 	pmdp_test_and_clear_young(vma, vaddr, pmdp);
 	pmd = READ_ONCE(*pmdp);
 	WARN_ON(pmd_young(pmd));
+
+	pgtable = pgtable_trans_huge_withdraw(mm, pmdp);
 }
 
 static void __init pmd_leaf_tests(unsigned long pfn, pgprot_t prot)
@@ -373,7 +377,7 @@ static void __init pud_basic_tests(unsigned long pfn, pgprot_t prot) { }
 static void __init pmd_advanced_tests(struct mm_struct *mm,
 				      struct vm_area_struct *vma, pmd_t *pmdp,
 				      unsigned long pfn, unsigned long vaddr,
-				      pgprot_t prot)
+				      pgprot_t prot, pgtable_t pgtable)
 {
 }
 static void __init pud_advanced_tests(struct mm_struct *mm,
@@ -1015,7 +1019,7 @@ static int __init debug_vm_pgtable(void)
 	pgd_clear_tests(mm, pgdp);
 
 	pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
-	pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot);
+	pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep);
 	pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot);
 	hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
 
-- 
2.26.2

[PATCH v3 09/13] mm/debug_vm_pgtable/locks: Move non page table modifying test together

[Aneesh Kumar K.V]

This will help in adding proper locks in a later patch

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
 mm/debug_vm_pgtable.c | 52 ++++++++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 23 deletions(-)

diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
index 0ce5c6a24c5b..78c8af3445ac 100644
--- a/mm/debug_vm_pgtable.c
+++ b/mm/debug_vm_pgtable.c
@@ -992,7 +992,7 @@ static int __init debug_vm_pgtable(void)
 	p4dp = p4d_alloc(mm, pgdp, vaddr);
 	pudp = pud_alloc(mm, p4dp, vaddr);
 	pmdp = pmd_alloc(mm, pudp, vaddr);
-	ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
+	ptep = pte_alloc_map(mm, pmdp, vaddr);
 
 	/*
 	 * Save all the page table page addresses as the page table
@@ -1012,33 +1012,12 @@ static int __init debug_vm_pgtable(void)
 	p4d_basic_tests(p4d_aligned, prot);
 	pgd_basic_tests(pgd_aligned, prot);
 
-	pte_clear_tests(mm, ptep, vaddr);
-	pmd_clear_tests(mm, pmdp);
-	pud_clear_tests(mm, pudp);
-	p4d_clear_tests(mm, p4dp);
-	pgd_clear_tests(mm, pgdp);
-
-	pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
-	pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep);
-	pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot);
-	hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
-
 	pmd_leaf_tests(pmd_aligned, prot);
 	pud_leaf_tests(pud_aligned, prot);
 
-	pmd_huge_tests(pmdp, pmd_aligned, prot);
-	pud_huge_tests(pudp, pud_aligned, prot);
-
 	pte_savedwrite_tests(pte_aligned, protnone);
 	pmd_savedwrite_tests(pmd_aligned, protnone);
 
-	pte_unmap_unlock(ptep, ptl);
-
-	pmd_populate_tests(mm, pmdp, saved_ptep);
-	pud_populate_tests(mm, pudp, saved_pmdp);
-	p4d_populate_tests(mm, p4dp, saved_pudp);
-	pgd_populate_tests(mm, pgdp, saved_p4dp);
-
 	pte_special_tests(pte_aligned, prot);
 	pte_protnone_tests(pte_aligned, protnone);
 	pmd_protnone_tests(pmd_aligned, protnone);
@@ -1056,11 +1035,38 @@ static int __init debug_vm_pgtable(void)
 	pmd_swap_tests(pmd_aligned, prot);
 
 	swap_migration_tests();
-	hugetlb_basic_tests(pte_aligned, prot);
 
 	pmd_thp_tests(pmd_aligned, prot);
 	pud_thp_tests(pud_aligned, prot);
 
+	/*
+	 * Page table modifying tests
+	 */
+	pte_clear_tests(mm, ptep, vaddr);
+	pmd_clear_tests(mm, pmdp);
+	pud_clear_tests(mm, pudp);
+	p4d_clear_tests(mm, p4dp);
+	pgd_clear_tests(mm, pgdp);
+
+	ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
+	pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
+	pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep);
+	pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot);
+	hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
+
+
+	pmd_huge_tests(pmdp, pmd_aligned, prot);
+	pud_huge_tests(pudp, pud_aligned, prot);
+
+	pte_unmap_unlock(ptep, ptl);
+
+	pmd_populate_tests(mm, pmdp, saved_ptep);
+	pud_populate_tests(mm, pudp, saved_pmdp);
+	p4d_populate_tests(mm, p4dp, saved_pudp);
+	pgd_populate_tests(mm, pgdp, saved_p4dp);
+
+	hugetlb_basic_tests(pte_aligned, prot);
+
 	p4d_free(mm, saved_p4dp);
 	pud_free(mm, saved_pudp);
 	pmd_free(mm, saved_pmdp);
-- 
2.26.2

[PATCH v3 10/13] mm/debug_vm_pgtable/locks: Take correct page table lock

[Aneesh Kumar K.V]

Make sure we call pte accessors with correct lock held.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
 mm/debug_vm_pgtable.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
index 78c8af3445ac..0a6e771ebd13 100644
--- a/mm/debug_vm_pgtable.c
+++ b/mm/debug_vm_pgtable.c
@@ -1039,33 +1039,39 @@ static int __init debug_vm_pgtable(void)
 	pmd_thp_tests(pmd_aligned, prot);
 	pud_thp_tests(pud_aligned, prot);
 
+	hugetlb_basic_tests(pte_aligned, prot);
+
 	/*
 	 * Page table modifying tests
 	 */
-	pte_clear_tests(mm, ptep, vaddr);
-	pmd_clear_tests(mm, pmdp);
-	pud_clear_tests(mm, pudp);
-	p4d_clear_tests(mm, p4dp);
-	pgd_clear_tests(mm, pgdp);
 
 	ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
+	pte_clear_tests(mm, ptep, vaddr);
 	pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
-	pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep);
-	pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot);
-	hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
-
+	pte_unmap_unlock(ptep, ptl);
 
+	ptl = pmd_lock(mm, pmdp);
+	pmd_clear_tests(mm, pmdp);
+	pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep);
 	pmd_huge_tests(pmdp, pmd_aligned, prot);
+	pmd_populate_tests(mm, pmdp, saved_ptep);
+	spin_unlock(ptl);
+
+	ptl = pud_lock(mm, pudp);
+	pud_clear_tests(mm, pudp);
+	pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot);
 	pud_huge_tests(pudp, pud_aligned, prot);
+	pud_populate_tests(mm, pudp, saved_pmdp);
+	spin_unlock(ptl);
 
-	pte_unmap_unlock(ptep, ptl);
+	hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
 
-	pmd_populate_tests(mm, pmdp, saved_ptep);
-	pud_populate_tests(mm, pudp, saved_pmdp);
+	spin_lock(&mm->page_table_lock);
+	p4d_clear_tests(mm, p4dp);
+	pgd_clear_tests(mm, pgdp);
 	p4d_populate_tests(mm, p4dp, saved_pudp);
 	pgd_populate_tests(mm, pgdp, saved_p4dp);
-
-	hugetlb_basic_tests(pte_aligned, prot);
+	spin_unlock(&mm->page_table_lock);
 
 	p4d_free(mm, saved_p4dp);
 	pud_free(mm, saved_pudp);
-- 
2.26.2

[PATCH v3 11/13] mm/debug_vm_pgtable/pmd_clear: Don't use pmd/pud_clear on pte entries

[Aneesh Kumar K.V]

pmd_clear() should not be used to clear pmd level pte entries.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
 mm/debug_vm_pgtable.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
index 0a6e771ebd13..a188b6e4e37e 100644
--- a/mm/debug_vm_pgtable.c
+++ b/mm/debug_vm_pgtable.c
@@ -196,6 +196,8 @@ static void __init pmd_advanced_tests(struct mm_struct *mm,
 	pmd = READ_ONCE(*pmdp);
 	WARN_ON(pmd_young(pmd));
 
+	/*  Clear the pte entries  */
+	pmdp_huge_get_and_clear(mm, vaddr, pmdp);
 	pgtable = pgtable_trans_huge_withdraw(mm, pmdp);
 }
 
@@ -321,6 +323,8 @@ static void __init pud_advanced_tests(struct mm_struct *mm,
 	pudp_test_and_clear_young(vma, vaddr, pudp);
 	pud = READ_ONCE(*pudp);
 	WARN_ON(pud_young(pud));
+
+	pudp_huge_get_and_clear(mm, vaddr, pudp);
 }
 
 static void __init pud_leaf_tests(unsigned long pfn, pgprot_t prot)
@@ -444,8 +448,6 @@ static void __init pud_populate_tests(struct mm_struct *mm, pud_t *pudp,
 	 * This entry points to next level page table page.
 	 * Hence this must not qualify as pud_bad().
 	 */
-	pmd_clear(pmdp);
-	pud_clear(pudp);
 	pud_populate(mm, pudp, pmdp);
 	pud = READ_ONCE(*pudp);
 	WARN_ON(pud_bad(pud));
@@ -577,7 +579,6 @@ static void __init pmd_populate_tests(struct mm_struct *mm, pmd_t *pmdp,
 	 * This entry points to next level page table page.
 	 * Hence this must not qualify as pmd_bad().
 	 */
-	pmd_clear(pmdp);
 	pmd_populate(mm, pmdp, pgtable);
 	pmd = READ_ONCE(*pmdp);
 	WARN_ON(pmd_bad(pmd));
-- 
2.26.2

[PATCH v3 12/13] mm/debug_vm_pgtable/hugetlb: Disable hugetlb test on ppc64

[Aneesh Kumar K.V]

The seems to be missing quite a lot of details w.r.t allocating
the correct pgtable_t page (huge_pte_alloc()), holding the right
lock (huge_pte_lock()) etc. The vma used is also not a hugetlb VMA.

ppc64 do have runtime checks within CONFIG_DEBUG_VM for most of these.
Hence disable the test on ppc64.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
 mm/debug_vm_pgtable.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
index a188b6e4e37e..21329c7d672f 100644
--- a/mm/debug_vm_pgtable.c
+++ b/mm/debug_vm_pgtable.c
@@ -813,6 +813,7 @@ static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot)
 #endif /* CONFIG_ARCH_WANT_GENERAL_HUGETLB */
 }
 
+#ifndef CONFIG_PPC_BOOK3S_64
 static void __init hugetlb_advanced_tests(struct mm_struct *mm,
 					  struct vm_area_struct *vma,
 					  pte_t *ptep, unsigned long pfn,
@@ -855,6 +856,7 @@ static void __init hugetlb_advanced_tests(struct mm_struct *mm,
 	pte = huge_ptep_get(ptep);
 	WARN_ON(!(huge_pte_write(pte) && huge_pte_dirty(pte)));
 }
+#endif
 #else  /* !CONFIG_HUGETLB_PAGE */
 static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot) { }
 static void __init hugetlb_advanced_tests(struct mm_struct *mm,
@@ -1065,7 +1067,9 @@ static int __init debug_vm_pgtable(void)
 	pud_populate_tests(mm, pudp, saved_pmdp);
 	spin_unlock(ptl);
 
+#ifndef CONFIG_PPC_BOOK3S_64
 	hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
+#endif
 
 	spin_lock(&mm->page_table_lock);
 	p4d_clear_tests(mm, p4dp);
-- 
2.26.2

[PATCH v3 13/13] mm/debug_vm_pgtable: populate a pte entry before fetching it

[Aneesh Kumar K.V]

pte_clear_tests operate on an existing pte entry. Make sure that is not a none
pte entry.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
 mm/debug_vm_pgtable.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
index 21329c7d672f..8527ebb75f2c 100644
--- a/mm/debug_vm_pgtable.c
+++ b/mm/debug_vm_pgtable.c
@@ -546,7 +546,7 @@ static void __init pgd_populate_tests(struct mm_struct *mm, pgd_t *pgdp,
 static void __init pte_clear_tests(struct mm_struct *mm, pte_t *ptep,
 				   unsigned long vaddr)
 {
-	pte_t pte = ptep_get(ptep);
+	pte_t pte =  ptep_get_and_clear(mm, vaddr, ptep);
 
 	pr_debug("Validating PTE clear\n");
 	pte = __pte(pte_val(pte) | RANDOM_ORVALUE);
@@ -944,7 +944,7 @@ static int __init debug_vm_pgtable(void)
 	p4d_t *p4dp, *saved_p4dp;
 	pud_t *pudp, *saved_pudp;
 	pmd_t *pmdp, *saved_pmdp, pmd;
-	pte_t *ptep;
+	pte_t *ptep, pte;
 	pgtable_t saved_ptep;
 	pgprot_t prot, protnone;
 	phys_addr_t paddr;
@@ -1049,6 +1049,8 @@ static int __init debug_vm_pgtable(void)
 	 */
 
 	ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
+	pte = pfn_pte(pte_aligned, prot);
+	set_pte_at(mm, vaddr, ptep, pte);
 	pte_clear_tests(mm, ptep, vaddr);
 	pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
 	pte_unmap_unlock(ptep, ptl);
-- 
2.26.2

Re: [PATCH v3 13/13] mm/debug_vm_pgtable: populate a pte entry before fetching it

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3593 bytes --]

Hi "Aneesh,

I love your patch! Perhaps something to improve:

[auto build test WARNING on hnaz-linux-mm/master]
[also build test WARNING on powerpc/next v5.9-rc2 next-20200827]
[cannot apply to mmotm/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Aneesh-Kumar-K-V/mm-debug_vm_pgtable-fixes/20200827-160758
base:   https://github.com/hnaz/linux-mm master
config: x86_64-randconfig-s022-20200827 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.2-191-g10164920-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

   mm/debug_vm_pgtable.c:509:9: sparse: sparse: incompatible types in conditional expression (different base types):
   mm/debug_vm_pgtable.c:509:9: sparse:    void
   mm/debug_vm_pgtable.c:509:9: sparse:    int
   mm/debug_vm_pgtable.c:528:9: sparse: sparse: incompatible types in conditional expression (different base types):
   mm/debug_vm_pgtable.c:528:9: sparse:    void
   mm/debug_vm_pgtable.c:528:9: sparse:    int
   mm/debug_vm_pgtable.c: note: in included file (through include/linux/pgtable.h, include/linux/mm.h, include/linux/highmem.h):
>> arch/x86/include/asm/pgtable.h:587:27: sparse: sparse: context imbalance in 'debug_vm_pgtable' - unexpected unlock

# https://github.com/0day-ci/linux/commit/9370726f47eaffdf772fdc273d180ec03b245cca
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Aneesh-Kumar-K-V/mm-debug_vm_pgtable-fixes/20200827-160758
git checkout 9370726f47eaffdf772fdc273d180ec03b245cca
vim +/debug_vm_pgtable +587 arch/x86/include/asm/pgtable.h

b534816b552d35 Jeremy Fitzhardinge 2009-02-04  586  
fb43d6cb91ef57 Dave Hansen         2018-04-06 @587  static inline pgprotval_t check_pgprot(pgprot_t pgprot)
fb43d6cb91ef57 Dave Hansen         2018-04-06  588  {
fb43d6cb91ef57 Dave Hansen         2018-04-06  589  	pgprotval_t massaged_val = massage_pgprot(pgprot);
fb43d6cb91ef57 Dave Hansen         2018-04-06  590  
fb43d6cb91ef57 Dave Hansen         2018-04-06  591  	/* mmdebug.h can not be included here because of dependencies */
fb43d6cb91ef57 Dave Hansen         2018-04-06  592  #ifdef CONFIG_DEBUG_VM
fb43d6cb91ef57 Dave Hansen         2018-04-06  593  	WARN_ONCE(pgprot_val(pgprot) != massaged_val,
fb43d6cb91ef57 Dave Hansen         2018-04-06  594  		  "attempted to set unsupported pgprot: %016llx "
fb43d6cb91ef57 Dave Hansen         2018-04-06  595  		  "bits: %016llx supported: %016llx\n",
fb43d6cb91ef57 Dave Hansen         2018-04-06  596  		  (u64)pgprot_val(pgprot),
fb43d6cb91ef57 Dave Hansen         2018-04-06  597  		  (u64)pgprot_val(pgprot) ^ massaged_val,
fb43d6cb91ef57 Dave Hansen         2018-04-06  598  		  (u64)__supported_pte_mask);
fb43d6cb91ef57 Dave Hansen         2018-04-06  599  #endif
fb43d6cb91ef57 Dave Hansen         2018-04-06  600  
fb43d6cb91ef57 Dave Hansen         2018-04-06  601  	return massaged_val;
fb43d6cb91ef57 Dave Hansen         2018-04-06  602  }
fb43d6cb91ef57 Dave Hansen         2018-04-06  603  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30318 bytes --]

Re: [PATCH v3 01/13] powerpc/mm: Add DEBUG_VM WARN for pmd_clear

[Anshuman Khandual]

On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
> With the hash page table, the kernel should not use pmd_clear for clearing
> huge pte entries. Add a DEBUG_VM WARN to catch the wrong usage.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/book3s/64/pgtable.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
> index 6de56c3b33c4..079211968987 100644
> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> @@ -868,6 +868,13 @@ static inline bool pte_ci(pte_t pte)
>  
>  static inline void pmd_clear(pmd_t *pmdp)
>  {
> +	if (IS_ENABLED(CONFIG_DEBUG_VM) && !radix_enabled()) {
> +		/*
> +		 * Don't use this if we can possibly have a hash page table
> +		 * entry mapping this.
> +		 */
> +		WARN_ON((pmd_val(*pmdp) & (H_PAGE_HASHPTE | _PAGE_PTE)) == (H_PAGE_HASHPTE | _PAGE_PTE));
> +	}
>  	*pmdp = __pmd(0);
>  }
>  
> @@ -916,6 +923,13 @@ static inline int pmd_bad(pmd_t pmd)
>  
>  static inline void pud_clear(pud_t *pudp)
>  {
> +	if (IS_ENABLED(CONFIG_DEBUG_VM) && !radix_enabled()) {
> +		/*
> +		 * Don't use this if we can possibly have a hash page table
> +		 * entry mapping this.
> +		 */
> +		WARN_ON((pud_val(*pudp) & (H_PAGE_HASHPTE | _PAGE_PTE)) == (H_PAGE_HASHPTE | _PAGE_PTE));
> +	}
>  	*pudp = __pud(0);
>  }

There are two checkpatch.pl warnings for this patch.

WARNING: line length of 105 exceeds 100 columns
#27: FILE: arch/powerpc/include/asm/book3s/64/pgtable.h:876:
+               WARN_ON((pmd_val(*pmdp) & (H_PAGE_HASHPTE | _PAGE_PTE)) == (H_PAGE_HASHPTE | _PAGE_PTE));

WARNING: line length of 105 exceeds 100 columns
#41: FILE: arch/powerpc/include/asm/book3s/64/pgtable.h:931:
+               WARN_ON((pud_val(*pudp) & (H_PAGE_HASHPTE | _PAGE_PTE)) == (H_PAGE_HASHPTE | _PAGE_PTE));

total: 0 errors, 2 warnings, 26 lines checked

Re: [PATCH v3 02/13] powerpc/mm: Move setting pte specific flags to pfn_pte

[Anshuman Khandual]

On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
> powerpc used to set the pte specific flags in set_pte_at(). This is different
> from other architectures. To be consistent with other architecture update
> pfn_pte to set _PAGE_PTE on ppc64. Also, drop now unused pte_mkpte.
> 
> We add a VM_WARN_ON() to catch the usage of calling set_pte_at() without setting
> _PAGE_PTE bit. We will remove that after a few releases.
> 
> With respect to huge pmd entries, pmd_mkhuge() takes care of adding the
> _PAGE_PTE bit.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/book3s/64/pgtable.h | 15 +++++++++------
>  arch/powerpc/include/asm/nohash/pgtable.h    |  5 -----
>  arch/powerpc/mm/pgtable.c                    |  5 -----
>  3 files changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
> index 079211968987..2382fd516f6b 100644
> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> @@ -619,7 +619,7 @@ static inline pte_t pfn_pte(unsigned long pfn, pgprot_t pgprot)
>  	VM_BUG_ON(pfn >> (64 - PAGE_SHIFT));
>  	VM_BUG_ON((pfn << PAGE_SHIFT) & ~PTE_RPN_MASK);
>  
> -	return __pte(((pte_basic_t)pfn << PAGE_SHIFT) | pgprot_val(pgprot));
> +	return __pte(((pte_basic_t)pfn << PAGE_SHIFT) | pgprot_val(pgprot) | _PAGE_PTE);
>  }
>  
>  static inline unsigned long pte_pfn(pte_t pte)
> @@ -655,11 +655,6 @@ static inline pte_t pte_mkexec(pte_t pte)
>  	return __pte_raw(pte_raw(pte) | cpu_to_be64(_PAGE_EXEC));
>  }
>  
> -static inline pte_t pte_mkpte(pte_t pte)
> -{
> -	return __pte_raw(pte_raw(pte) | cpu_to_be64(_PAGE_PTE));
> -}
> -
>  static inline pte_t pte_mkwrite(pte_t pte)
>  {
>  	/*
> @@ -823,6 +818,14 @@ static inline int pte_none(pte_t pte)
>  static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
>  				pte_t *ptep, pte_t pte, int percpu)
>  {
> +
> +	VM_WARN_ON(!(pte_raw(pte) & cpu_to_be64(_PAGE_PTE)));
> +	/*
> +	 * Keep the _PAGE_PTE added till we are sure we handle _PAGE_PTE
> +	 * in all the callers.
> +	 */
> +	 pte = __pte_raw(pte_raw(pte) | cpu_to_be64(_PAGE_PTE));
> +
>  	if (radix_enabled())
>  		return radix__set_pte_at(mm, addr, ptep, pte, percpu);
>  	return hash__set_pte_at(mm, addr, ptep, pte, percpu);
> diff --git a/arch/powerpc/include/asm/nohash/pgtable.h b/arch/powerpc/include/asm/nohash/pgtable.h
> index 4b7c3472eab1..6277e7596ae5 100644
> --- a/arch/powerpc/include/asm/nohash/pgtable.h
> +++ b/arch/powerpc/include/asm/nohash/pgtable.h
> @@ -140,11 +140,6 @@ static inline pte_t pte_mkold(pte_t pte)
>  	return __pte(pte_val(pte) & ~_PAGE_ACCESSED);
>  }
>  
> -static inline pte_t pte_mkpte(pte_t pte)
> -{
> -	return pte;
> -}
> -
>  static inline pte_t pte_mkspecial(pte_t pte)
>  {
>  	return __pte(pte_val(pte) | _PAGE_SPECIAL);
> diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
> index 9c0547d77af3..ab57b07ef39a 100644
> --- a/arch/powerpc/mm/pgtable.c
> +++ b/arch/powerpc/mm/pgtable.c
> @@ -184,9 +184,6 @@ void set_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep,
>  	 */
>  	VM_WARN_ON(pte_hw_valid(*ptep) && !pte_protnone(*ptep));
>  
> -	/* Add the pte bit when trying to set a pte */
> -	pte = pte_mkpte(pte);
> -
>  	/* Note: mm->context.id might not yet have been assigned as
>  	 * this context might not have been activated yet when this
>  	 * is called.
> @@ -275,8 +272,6 @@ void set_huge_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep, pte_
>  	 */
>  	VM_WARN_ON(pte_hw_valid(*ptep) && !pte_protnone(*ptep));
>  
> -	pte = pte_mkpte(pte);
> -
>  	pte = set_pte_filter(pte);
>  
>  	val = pte_val(pte);
> 

There is one checkpatch.pl warning for this patch.

WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#6: 
powerpc used to set the pte specific flags in set_pte_at(). This is different

total: 0 errors, 1 warnings, 61 lines checked

Re: [PATCH v3 03/13] mm/debug_vm_pgtable/ppc64: Avoid setting top bits in radom value

[Anshuman Khandual]

On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
> ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
> random value.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  mm/debug_vm_pgtable.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
> index 086309fb9b6f..bbf9df0e64c6 100644
> --- a/mm/debug_vm_pgtable.c
> +++ b/mm/debug_vm_pgtable.c
> @@ -44,10 +44,17 @@
>   * entry type. But these bits might affect the ability to clear entries with
>   * pxx_clear() because of how dynamic page table folding works on s390. So
>   * while loading up the entries do not change the lower 4 bits. It does not
> - * have affect any other platform.
> + * have affect any other platform. Also avoid the 62nd bit on ppc64 that is
> + * used to mark a pte entry.
>   */
> -#define S390_MASK_BITS	4
> -#define RANDOM_ORVALUE	GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS)
> +#define S390_SKIP_MASK		GENMASK(3, 0)
> +#ifdef CONFIG_PPC_BOOK3S_64
> +#define PPC64_SKIP_MASK		GENMASK(62, 62)
> +#else
> +#define PPC64_SKIP_MASK		0x0
> +#endif

Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip
bits for a s390 platform requirement and can also do so for ppc64 as well. As
mentioned before, please avoid adding any platform specific constructs in the
test.

> +#define ARCH_SKIP_MASK (S390_SKIP_MASK | PPC64_SKIP_MASK)
> +#define RANDOM_ORVALUE (GENMASK(BITS_PER_LONG - 1, 0) & ~ARCH_SKIP_MASK)
>  #define RANDOM_NZVALUE	GENMASK(7, 0)

Please fix the alignments here. Feel free to consider following changes after
this patch.

diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
index 122416464e0f..f969031152bb 100644
--- a/mm/debug_vm_pgtable.c
+++ b/mm/debug_vm_pgtable.c
@@ -48,14 +48,11 @@
  * have affect any other platform. Also avoid the 62nd bit on ppc64 that is
  * used to mark a pte entry.
  */
-#define S390_SKIP_MASK         GENMASK(3, 0)
-#ifdef CONFIG_PPC_BOOK3S_64
-#define PPC64_SKIP_MASK                GENMASK(62, 62)
-#else
-#define PPC64_SKIP_MASK                0x0
-#endif
-#define ARCH_SKIP_MASK (S390_SKIP_MASK | PPC64_SKIP_MASK)
-#define RANDOM_ORVALUE (GENMASK(BITS_PER_LONG - 1, 0) & ~ARCH_SKIP_MASK)
+#define S390_SKIP_MASK GENMASK(3, 0)
+#define PPC64_SKIP_MASK        GENMASK(62, 62)
+#define ARCH_SKIP_MASK (S390_SKIP_MASK | PPC64_SKIP_MASK)
+#define RANDOM_ORVALUE (GENMASK(BITS_PER_LONG - 1, 0) & ~ARCH_SKIP_MASK)
+
 #define RANDOM_NZVALUE GENMASK(7, 0)
 
>  
>  static void __init pte_basic_tests(unsigned long pfn, pgprot_t prot)
> 

Besides, there is also one checkpatch.pl warning here.

WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#7: 
ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in

total: 0 errors, 1 warnings, 20 lines checked

Re: [PATCH v3 04/13] mm/debug_vm_pgtables/hugevmap: Use the arch helper to identify huge vmap support.

[Anshuman Khandual]

On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
> ppc64 supports huge vmap only with radix translation. Hence use arch helper
> to determine the huge vmap support.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  mm/debug_vm_pgtable.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
> index bbf9df0e64c6..28f9d0558c20 100644
> --- a/mm/debug_vm_pgtable.c
> +++ b/mm/debug_vm_pgtable.c
> @@ -28,6 +28,7 @@
>  #include <linux/swapops.h>
>  #include <linux/start_kernel.h>
>  #include <linux/sched/mm.h>
> +#include <linux/io.h>
>  #include <asm/pgalloc.h>
>  #include <asm/tlbflush.h>
>  
> @@ -206,11 +207,12 @@ static void __init pmd_leaf_tests(unsigned long pfn, pgprot_t prot)
>  	WARN_ON(!pmd_leaf(pmd));
>  }
>  
> +#ifdef CONFIG_HAVE_ARCH_HUGE_VMAP
>  static void __init pmd_huge_tests(pmd_t *pmdp, unsigned long pfn, pgprot_t prot)
>  {
>  	pmd_t pmd;
>  
> -	if (!IS_ENABLED(CONFIG_HAVE_ARCH_HUGE_VMAP))
> +	if (!arch_ioremap_pmd_supported())
>  		return;
>  
>  	pr_debug("Validating PMD huge\n");
> @@ -224,6 +226,10 @@ static void __init pmd_huge_tests(pmd_t *pmdp, unsigned long pfn, pgprot_t prot)
>  	pmd = READ_ONCE(*pmdp);
>  	WARN_ON(!pmd_none(pmd));
>  }
> +#else /* !CONFIG_HAVE_ARCH_HUGE_VMAP */
> +static void __init pmd_huge_tests(pmd_t *pmdp, unsigned long pfn, pgprot_t prot) { }
> +#endif /* !CONFIG_HAVE_ARCH_HUGE_VMAP */
> +

Some small nits here.

- s/!CONFIG_HAVE_ARCH_HUGE_VMAP/CONFIG_HAVE_ARCH_HUGE_VMAP
- Drop the extra line at the end
- Move the { } down just to be consistent with existing stub for pmd_huge_tests()

>  
>  static void __init pmd_savedwrite_tests(unsigned long pfn, pgprot_t prot)
>  {
> @@ -320,11 +326,12 @@ static void __init pud_leaf_tests(unsigned long pfn, pgprot_t prot)
>  	WARN_ON(!pud_leaf(pud));
>  }
>  
> +#ifdef CONFIG_HAVE_ARCH_HUGE_VMAP
>  static void __init pud_huge_tests(pud_t *pudp, unsigned long pfn, pgprot_t prot)
>  {
>  	pud_t pud;
>  
> -	if (!IS_ENABLED(CONFIG_HAVE_ARCH_HUGE_VMAP))
> +	if (!arch_ioremap_pud_supported())
>  		return;
>  
>  	pr_debug("Validating PUD huge\n");
> @@ -338,6 +345,10 @@ static void __init pud_huge_tests(pud_t *pudp, unsigned long pfn, pgprot_t prot)
>  	pud = READ_ONCE(*pudp);
>  	WARN_ON(!pud_none(pud));
>  }
> +#else /* !CONFIG_HAVE_ARCH_HUGE_VMAP */
> +static void __init pud_huge_tests(pud_t *pudp, unsigned long pfn, pgprot_t prot) { }
> +#endif /* !CONFIG_HAVE_ARCH_HUGE_VMAP */
> +

Some small nits here.

- s/!CONFIG_HAVE_ARCH_HUGE_VMAP/CONFIG_HAVE_ARCH_HUGE_VMAP
- Drop the extra line at the end
- Move the { } down just to be consistent with existing stub for pud_huge_tests()

>  #else  /* !CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD */
>  static void __init pud_basic_tests(unsigned long pfn, pgprot_t prot) { }
>  static void __init pud_advanced_tests(struct mm_struct *mm,
>

Re: [PATCH v3 05/13] mm/debug_vm_pgtable/savedwrite: Enable savedwrite test with CONFIG_NUMA_BALANCING

[Anshuman Khandual]

On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
> Saved write support was added to track the write bit of a pte after marking the
> pte protnone. This was done so that AUTONUMA can convert a write pte to protnone
> and still track the old write bit. When converting it back we set the pte write
> bit correctly thereby avoiding a write fault again. Hence enable the test only
> when CONFIG_NUMA_BALANCING is enabled and use protnone protflags.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  mm/debug_vm_pgtable.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
> index 28f9d0558c20..5c0680836fe9 100644
> --- a/mm/debug_vm_pgtable.c
> +++ b/mm/debug_vm_pgtable.c
> @@ -119,10 +119,14 @@ static void __init pte_savedwrite_tests(unsigned long pfn, pgprot_t prot)
>  {
>  	pte_t pte = pfn_pte(pfn, prot);
>  
> +	if (!IS_ENABLED(CONFIG_NUMA_BALANCING))
> +		return;
> +
>  	pr_debug("Validating PTE saved write\n");
>  	WARN_ON(!pte_savedwrite(pte_mk_savedwrite(pte_clear_savedwrite(pte))));
>  	WARN_ON(pte_savedwrite(pte_clear_savedwrite(pte_mk_savedwrite(pte))));
>  }
> +
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>  static void __init pmd_basic_tests(unsigned long pfn, pgprot_t prot)
>  {
> @@ -235,6 +239,9 @@ static void __init pmd_savedwrite_tests(unsigned long pfn, pgprot_t prot)
>  {
>  	pmd_t pmd = pfn_pmd(pfn, prot);
>  
> +	if (!IS_ENABLED(CONFIG_NUMA_BALANCING))
> +		return;
> +
>  	pr_debug("Validating PMD saved write\n");
>  	WARN_ON(!pmd_savedwrite(pmd_mk_savedwrite(pmd_clear_savedwrite(pmd))));
>  	WARN_ON(pmd_savedwrite(pmd_clear_savedwrite(pmd_mk_savedwrite(pmd))));
> @@ -1020,8 +1027,8 @@ static int __init debug_vm_pgtable(void)
>  	pmd_huge_tests(pmdp, pmd_aligned, prot);
>  	pud_huge_tests(pudp, pud_aligned, prot);
>  
> -	pte_savedwrite_tests(pte_aligned, prot);
> -	pmd_savedwrite_tests(pmd_aligned, prot);
> +	pte_savedwrite_tests(pte_aligned, protnone);
> +	pmd_savedwrite_tests(pmd_aligned, protnone);
>  
>  	pte_unmap_unlock(ptep, ptl);
>  
> 

There is a checkpatch.pl warning here.

WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#7: 
Saved write support was added to track the write bit of a pte after marking the

total: 0 errors, 1 warnings, 33 lines checked

Otherwise this looks good. With the checkpatch.pl warning fixed

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

Re: [PATCH v3 06/13] mm/debug_vm_pgtable/THP: Mark the pte entry huge before using set_pmd/pud_at

[Anshuman Khandual]

On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
> kernel expects entries to be marked huge before we use set_pmd_at()/set_pud_at().
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  mm/debug_vm_pgtable.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
> index 5c0680836fe9..de83a20c1b30 100644
> --- a/mm/debug_vm_pgtable.c
> +++ b/mm/debug_vm_pgtable.c
> @@ -155,7 +155,7 @@ static void __init pmd_advanced_tests(struct mm_struct *mm,
>  				      unsigned long pfn, unsigned long vaddr,
>  				      pgprot_t prot)
>  {
> -	pmd_t pmd = pfn_pmd(pfn, prot);
> +	pmd_t pmd;
>  
>  	if (!has_transparent_hugepage())
>  		return;
> @@ -164,19 +164,19 @@ static void __init pmd_advanced_tests(struct mm_struct *mm,
>  	/* Align the address wrt HPAGE_PMD_SIZE */
>  	vaddr = (vaddr & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE;
>  
> -	pmd = pfn_pmd(pfn, prot);
> +	pmd = pmd_mkhuge(pfn_pmd(pfn, prot));
>  	set_pmd_at(mm, vaddr, pmdp, pmd);
>  	pmdp_set_wrprotect(mm, vaddr, pmdp);
>  	pmd = READ_ONCE(*pmdp);
>  	WARN_ON(pmd_write(pmd));
>  
> -	pmd = pfn_pmd(pfn, prot);
> +	pmd = pmd_mkhuge(pfn_pmd(pfn, prot));
>  	set_pmd_at(mm, vaddr, pmdp, pmd);
>  	pmdp_huge_get_and_clear(mm, vaddr, pmdp);
>  	pmd = READ_ONCE(*pmdp);
>  	WARN_ON(!pmd_none(pmd));
>  
> -	pmd = pfn_pmd(pfn, prot);
> +	pmd = pmd_mkhuge(pfn_pmd(pfn, prot));
>  	pmd = pmd_wrprotect(pmd);
>  	pmd = pmd_mkclean(pmd);
>  	set_pmd_at(mm, vaddr, pmdp, pmd);
> @@ -237,7 +237,7 @@ static void __init pmd_huge_tests(pmd_t *pmdp, unsigned long pfn, pgprot_t prot)
>  
>  static void __init pmd_savedwrite_tests(unsigned long pfn, pgprot_t prot)
>  {
> -	pmd_t pmd = pfn_pmd(pfn, prot);
> +	pmd_t pmd = pmd_mkhuge(pfn_pmd(pfn, prot));

There is no set_pmd_at() in this particular test, why change ?

>  
>  	if (!IS_ENABLED(CONFIG_NUMA_BALANCING))
>  		return;
> @@ -277,7 +277,7 @@ static void __init pud_advanced_tests(struct mm_struct *mm,
>  				      unsigned long pfn, unsigned long vaddr,
>  				      pgprot_t prot)
>  {
> -	pud_t pud = pfn_pud(pfn, prot);
> +	pud_t pud;
>  
>  	if (!has_transparent_hugepage())
>  		return;
> @@ -286,25 +286,28 @@ static void __init pud_advanced_tests(struct mm_struct *mm,
>  	/* Align the address wrt HPAGE_PUD_SIZE */
>  	vaddr = (vaddr & HPAGE_PUD_MASK) + HPAGE_PUD_SIZE;
>  
> +	pud = pud_mkhuge(pfn_pud(pfn, prot));
>  	set_pud_at(mm, vaddr, pudp, pud);
>  	pudp_set_wrprotect(mm, vaddr, pudp);
>  	pud = READ_ONCE(*pudp);
>  	WARN_ON(pud_write(pud));
>  
>  #ifndef __PAGETABLE_PMD_FOLDED
> -	pud = pfn_pud(pfn, prot);
> +

Please drop the extra line here.

> +	pud = pud_mkhuge(pfn_pud(pfn, prot));
>  	set_pud_at(mm, vaddr, pudp, pud);
>  	pudp_huge_get_and_clear(mm, vaddr, pudp);
>  	pud = READ_ONCE(*pudp);
>  	WARN_ON(!pud_none(pud));
>  
> -	pud = pfn_pud(pfn, prot);
> +	pud = pud_mkhuge(pfn_pud(pfn, prot));
>  	set_pud_at(mm, vaddr, pudp, pud);
>  	pudp_huge_get_and_clear_full(mm, vaddr, pudp, 1);
>  	pud = READ_ONCE(*pudp);
>  	WARN_ON(!pud_none(pud));
>  #endif /* __PAGETABLE_PMD_FOLDED */
> -	pud = pfn_pud(pfn, prot);
> +

Please drop the extra line here.

> +	pud = pud_mkhuge(pfn_pud(pfn, prot));
>  	pud = pud_wrprotect(pud);
>  	pud = pud_mkclean(pud);
>  	set_pud_at(mm, vaddr, pudp, pud);
>

There is a checkpatch.pl warning here.

WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#7: 
kernel expects entries to be marked huge before we use set_pmd_at()/set_pud_at().

total: 0 errors, 1 warnings, 77 lines checked

Re: [PATCH v3 08/13] mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP

[Anshuman Khandual]

On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
> Architectures like ppc64 use deposited page table while updating the huge pte
> entries.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  mm/debug_vm_pgtable.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
> index f9f6358899a8..0ce5c6a24c5b 100644
> --- a/mm/debug_vm_pgtable.c
> +++ b/mm/debug_vm_pgtable.c
> @@ -154,7 +154,7 @@ static void __init pmd_basic_tests(unsigned long pfn, pgprot_t prot)
>  static void __init pmd_advanced_tests(struct mm_struct *mm,
>  				      struct vm_area_struct *vma, pmd_t *pmdp,
>  				      unsigned long pfn, unsigned long vaddr,
> -				      pgprot_t prot)
> +				      pgprot_t prot, pgtable_t pgtable)
>  {
>  	pmd_t pmd;
>  
> @@ -165,6 +165,8 @@ static void __init pmd_advanced_tests(struct mm_struct *mm,
>  	/* Align the address wrt HPAGE_PMD_SIZE */
>  	vaddr = (vaddr & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE;
>  
> +	pgtable_trans_huge_deposit(mm, pmdp, pgtable);
> +
>  	pmd = pmd_mkhuge(pfn_pmd(pfn, prot));
>  	set_pmd_at(mm, vaddr, pmdp, pmd);
>  	pmdp_set_wrprotect(mm, vaddr, pmdp);
> @@ -193,6 +195,8 @@ static void __init pmd_advanced_tests(struct mm_struct *mm,
>  	pmdp_test_and_clear_young(vma, vaddr, pmdp);
>  	pmd = READ_ONCE(*pmdp);
>  	WARN_ON(pmd_young(pmd));
> +
> +	pgtable = pgtable_trans_huge_withdraw(mm, pmdp);

Should the call sites here be wrapped with __HAVE_ARCH_PGTABLE_DEPOSIT and
__HAVE_ARCH_PGTABLE_WITHDRAW respectively. Though there are generic fallback
definitions, wondering whether they are indeed essential for all platforms.

>  }
>  
>  static void __init pmd_leaf_tests(unsigned long pfn, pgprot_t prot)
> @@ -373,7 +377,7 @@ static void __init pud_basic_tests(unsigned long pfn, pgprot_t prot) { }
>  static void __init pmd_advanced_tests(struct mm_struct *mm,
>  				      struct vm_area_struct *vma, pmd_t *pmdp,
>  				      unsigned long pfn, unsigned long vaddr,
> -				      pgprot_t prot)
> +				      pgprot_t prot, pgtable_t pgtable)
>  {
>  }
>  static void __init pud_advanced_tests(struct mm_struct *mm,
> @@ -1015,7 +1019,7 @@ static int __init debug_vm_pgtable(void)
>  	pgd_clear_tests(mm, pgdp);
>  
>  	pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
> -	pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot);
> +	pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep);
>  	pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot);
>  	hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>  
> 

There is a checkpatch.pl warning here.

WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#7: 
Architectures like ppc64 use deposited page table while updating the huge pte

total: 0 errors, 1 warnings, 40 lines checked

Re: [PATCH v3 13/13] mm/debug_vm_pgtable: populate a pte entry before fetching it

[Anshuman Khandual]

On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
> pte_clear_tests operate on an existing pte entry. Make sure that is not a none
> pte entry.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  mm/debug_vm_pgtable.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
> index 21329c7d672f..8527ebb75f2c 100644
> --- a/mm/debug_vm_pgtable.c
> +++ b/mm/debug_vm_pgtable.c
> @@ -546,7 +546,7 @@ static void __init pgd_populate_tests(struct mm_struct *mm, pgd_t *pgdp,
>  static void __init pte_clear_tests(struct mm_struct *mm, pte_t *ptep,
>  				   unsigned long vaddr)
>  {
> -	pte_t pte = ptep_get(ptep);
> +	pte_t pte =  ptep_get_and_clear(mm, vaddr, ptep);

Seems like ptep_get_and_clear() here just clears the entry in preparation
for a following set_pte_at() which otherwise would have been a problem on
ppc64 as you had pointed out earlier i.e set_pte_at() should not update an
existing valid entry. So the commit message here is bit misleading.

>  
>  	pr_debug("Validating PTE clear\n");
>  	pte = __pte(pte_val(pte) | RANDOM_ORVALUE);
> @@ -944,7 +944,7 @@ static int __init debug_vm_pgtable(void)
>  	p4d_t *p4dp, *saved_p4dp;
>  	pud_t *pudp, *saved_pudp;
>  	pmd_t *pmdp, *saved_pmdp, pmd;
> -	pte_t *ptep;
> +	pte_t *ptep, pte;
>  	pgtable_t saved_ptep;
>  	pgprot_t prot, protnone;
>  	phys_addr_t paddr;
> @@ -1049,6 +1049,8 @@ static int __init debug_vm_pgtable(void)
>  	 */
>  
>  	ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
> +	pte = pfn_pte(pte_aligned, prot);
> +	set_pte_at(mm, vaddr, ptep, pte);

Not here, creating and populating an entry must be done in respective
test functions itself. Besides, this seems bit redundant as well. The
test pte_clear_tests() with the above change added, already

- Clears the PTEP entry with ptep_get_and_clear()
- Creates and populates the PTEP with set_pte_at()
- Clears with pte_clear()
- Checks for pte_none()

If the objective is to clear the PTEP entry before calling set_pte_at(),
then only the first chunk is required and it should also be merged with
a previous patch. 

[PATCH v3 07/13] mm/debug_vm_pgtable/set_pte/pmd/pud: Don't use set_*_at to update an existing pte entry 


>  	pte_clear_tests(mm, ptep, vaddr);
>  	pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>  	pte_unmap_unlock(ptep, ptl);
> 

There is a checkpatch.pl warning here.

WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#7: 
pte_clear_tests operate on an existing pte entry. Make sure that is not a none

total: 0 errors, 1 warnings, 24 lines checked

There is also a build failure on x86 reported from kernel test robot.

Re: [PATCH v3 09/13] mm/debug_vm_pgtable/locks: Move non page table modifying test together

[Anshuman Khandual]

On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
> This will help in adding proper locks in a later patch

It really makes sense to classify these tests here as static and dynamic.
Static are the ones that test via page table entry values modification
without changing anything on the actual page table itself. Where as the
dynamic tests do change the page table. Static tests would probably never
require any lock synchronization but the dynamic ones will do.

> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  mm/debug_vm_pgtable.c | 52 ++++++++++++++++++++++++-------------------
>  1 file changed, 29 insertions(+), 23 deletions(-)
> 
> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
> index 0ce5c6a24c5b..78c8af3445ac 100644
> --- a/mm/debug_vm_pgtable.c
> +++ b/mm/debug_vm_pgtable.c
> @@ -992,7 +992,7 @@ static int __init debug_vm_pgtable(void)
>  	p4dp = p4d_alloc(mm, pgdp, vaddr);
>  	pudp = pud_alloc(mm, p4dp, vaddr);
>  	pmdp = pmd_alloc(mm, pudp, vaddr);
> -	ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
> +	ptep = pte_alloc_map(mm, pmdp, vaddr);
>  
>  	/*
>  	 * Save all the page table page addresses as the page table
> @@ -1012,33 +1012,12 @@ static int __init debug_vm_pgtable(void)
>  	p4d_basic_tests(p4d_aligned, prot);
>  	pgd_basic_tests(pgd_aligned, prot);
>  
> -	pte_clear_tests(mm, ptep, vaddr);
> -	pmd_clear_tests(mm, pmdp);
> -	pud_clear_tests(mm, pudp);
> -	p4d_clear_tests(mm, p4dp);
> -	pgd_clear_tests(mm, pgdp);
> -
> -	pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
> -	pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep);
> -	pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot);
> -	hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
> -
>  	pmd_leaf_tests(pmd_aligned, prot);
>  	pud_leaf_tests(pud_aligned, prot);
>  
> -	pmd_huge_tests(pmdp, pmd_aligned, prot);
> -	pud_huge_tests(pudp, pud_aligned, prot);
> -
>  	pte_savedwrite_tests(pte_aligned, protnone);
>  	pmd_savedwrite_tests(pmd_aligned, protnone);
>  
> -	pte_unmap_unlock(ptep, ptl);
> -
> -	pmd_populate_tests(mm, pmdp, saved_ptep);
> -	pud_populate_tests(mm, pudp, saved_pmdp);
> -	p4d_populate_tests(mm, p4dp, saved_pudp);
> -	pgd_populate_tests(mm, pgdp, saved_p4dp);
> -
>  	pte_special_tests(pte_aligned, prot);
>  	pte_protnone_tests(pte_aligned, protnone);
>  	pmd_protnone_tests(pmd_aligned, protnone);
> @@ -1056,11 +1035,38 @@ static int __init debug_vm_pgtable(void)
>  	pmd_swap_tests(pmd_aligned, prot);
>  
>  	swap_migration_tests();
> -	hugetlb_basic_tests(pte_aligned, prot);
>  
>  	pmd_thp_tests(pmd_aligned, prot);
>  	pud_thp_tests(pud_aligned, prot);
>  
> +	/*
> +	 * Page table modifying tests
> +	 */

In this comment, please do add some more details about how these tests
need proper locking mechanism unlike the previous static ones. Also add
a similar comment section for the static tests that dont really change
page table and need not require corresponding locking mechanism. Both
comment sections will make this classification clear.

> +	pte_clear_tests(mm, ptep, vaddr);
> +	pmd_clear_tests(mm, pmdp);
> +	pud_clear_tests(mm, pudp);
> +	p4d_clear_tests(mm, p4dp);
> +	pgd_clear_tests(mm, pgdp);
> +
> +	ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
> +	pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
> +	pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep);
> +	pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot);
> +	hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
> +
> +
> +	pmd_huge_tests(pmdp, pmd_aligned, prot);
> +	pud_huge_tests(pudp, pud_aligned, prot);
> +
> +	pte_unmap_unlock(ptep, ptl);
> +
> +	pmd_populate_tests(mm, pmdp, saved_ptep);
> +	pud_populate_tests(mm, pudp, saved_pmdp);
> +	p4d_populate_tests(mm, p4dp, saved_pudp);
> +	pgd_populate_tests(mm, pgdp, saved_p4dp);
> +
> +	hugetlb_basic_tests(pte_aligned, prot);

hugetlb_basic_tests() is a non page table modifying static test and
should be classified accordingly.

> +
>  	p4d_free(mm, saved_p4dp);
>  	pud_free(mm, saved_pudp);
>  	pmd_free(mm, saved_pmdp);
> 

Changes till this patch fails to boot on arm64 platform. This should be
folded with the next patch.

[   17.080644] ------------[ cut here ]------------
[   17.081342] kernel BUG at mm/pgtable-generic.c:164!
[   17.082091] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
[   17.082977] Modules linked in:
[   17.083481] CPU: 79 PID: 1 Comm: swapper/0 Tainted: G        W         5.9.0-rc2-00105-g740e72cd6717 #14
[   17.084998] Hardware name: linux,dummy-virt (DT)
[   17.085745] pstate: 60400005 (nZCv daif +PAN -UAO BTYPE=--)
[   17.086645] pc : pgtable_trans_huge_deposit+0x58/0x60
[   17.087462] lr : debug_vm_pgtable+0x4dc/0x8f0
[   17.088168] sp : ffff80001219bcf0
[   17.088710] x29: ffff80001219bcf0 x28: ffff8000114ac000 
[   17.089574] x27: ffff8000114ac000 x26: 0020000000000fd3 
[   17.090431] x25: 0020000081400fd3 x24: fffffe00175c12c0 
[   17.091286] x23: ffff0005df04d1a8 x22: 0000f18d6e035000 
[   17.092143] x21: ffff0005dd790000 x20: ffff0005df18e1a8 
[   17.093003] x19: ffff0005df04cb80 x18: 0000000000000014 
[   17.093859] x17: 00000000b76667d0 x16: 00000000fd4e6611 
[   17.094716] x15: 0000000000000001 x14: 0000000000000002 
[   17.095572] x13: 000000000055d966 x12: fffffe001755e400 
[   17.096429] x11: 0000000000000008 x10: ffff0005fcb6e210 
[   17.097292] x9 : ffff0005fcb6e210 x8 : 0020000081590fd3 
[   17.098149] x7 : ffff0005dd71e0c0 x6 : ffff0005df18e1a8 
[   17.099006] x5 : 0020000081590fd3 x4 : 0020000081590fd3 
[   17.099862] x3 : ffff0005df18e1a8 x2 : fffffe00175c12c0 
[   17.100718] x1 : fffffe00175c1300 x0 : 0000000000000000 
[   17.101583] Call trace:
[   17.101993]  pgtable_trans_huge_deposit+0x58/0x60
[   17.102754]  do_one_initcall+0x74/0x1cc
[   17.103381]  kernel_init_freeable+0x1d0/0x238
[   17.104089]  kernel_init+0x14/0x118
[   17.104658]  ret_from_fork+0x10/0x34
[   17.105251] Code: f9000443 f9000843 f9000822 d65f03c0 (d4210000) 
[   17.106303] ---[ end trace e63471e00f8c147f ]---

Re: [PATCH v3 10/13] mm/debug_vm_pgtable/locks: Take correct page table lock

[Anshuman Khandual]

On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
> Make sure we call pte accessors with correct lock held.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  mm/debug_vm_pgtable.c | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
> index 78c8af3445ac..0a6e771ebd13 100644
> --- a/mm/debug_vm_pgtable.c
> +++ b/mm/debug_vm_pgtable.c
> @@ -1039,33 +1039,39 @@ static int __init debug_vm_pgtable(void)
>  	pmd_thp_tests(pmd_aligned, prot);
>  	pud_thp_tests(pud_aligned, prot);
>  
> +	hugetlb_basic_tests(pte_aligned, prot);
> +

This moved back again. As pointed out earlier, there will be a bisect
problem and so this patch must be folded back with the previous one.

>  	/*
>  	 * Page table modifying tests
>  	 */
> -	pte_clear_tests(mm, ptep, vaddr);
> -	pmd_clear_tests(mm, pmdp);
> -	pud_clear_tests(mm, pudp);
> -	p4d_clear_tests(mm, p4dp);
> -	pgd_clear_tests(mm, pgdp);
>  
>  	ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
> +	pte_clear_tests(mm, ptep, vaddr);
>  	pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
> -	pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep);
> -	pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot);
> -	hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
> -
> +	pte_unmap_unlock(ptep, ptl);
>  
> +	ptl = pmd_lock(mm, pmdp);
> +	pmd_clear_tests(mm, pmdp);
> +	pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep);
>  	pmd_huge_tests(pmdp, pmd_aligned, prot);
> +	pmd_populate_tests(mm, pmdp, saved_ptep);
> +	spin_unlock(ptl);
> +
> +	ptl = pud_lock(mm, pudp);
> +	pud_clear_tests(mm, pudp);
> +	pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot);
>  	pud_huge_tests(pudp, pud_aligned, prot);
> +	pud_populate_tests(mm, pudp, saved_pmdp);
> +	spin_unlock(ptl);
>  
> -	pte_unmap_unlock(ptep, ptl);
> +	hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>  
> -	pmd_populate_tests(mm, pmdp, saved_ptep);
> -	pud_populate_tests(mm, pudp, saved_pmdp);
> +	spin_lock(&mm->page_table_lock);
> +	p4d_clear_tests(mm, p4dp);
> +	pgd_clear_tests(mm, pgdp);
>  	p4d_populate_tests(mm, p4dp, saved_pudp);
>  	pgd_populate_tests(mm, pgdp, saved_p4dp);
> -
> -	hugetlb_basic_tests(pte_aligned, prot);
> +	spin_unlock(&mm->page_table_lock);
>  
>  	p4d_free(mm, saved_p4dp);
>  	pud_free(mm, saved_pudp);
> 

Overall, grouping together dynamic tests at various page table levels and
taking a corresponding lock, makes sense. Commit message for the resultant
patch should include (a) Test classification (b) Grouping by function for
the static tests, by page table level for the dynamic tests (c) Locking
requirement for the dynamic tests each level etc.

Re: [PATCH v3 12/13] mm/debug_vm_pgtable/hugetlb: Disable hugetlb test on ppc64

[Anshuman Khandual]

On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
> The seems to be missing quite a lot of details w.r.t allocating
> the correct pgtable_t page (huge_pte_alloc()), holding the right
> lock (huge_pte_lock()) etc. The vma used is also not a hugetlb VMA.
> 
> ppc64 do have runtime checks within CONFIG_DEBUG_VM for most of these.
> Hence disable the test on ppc64.

Would really like this to get resolved in an uniform and better way
instead, i.e a modified hugetlb_advanced_tests() which works on all
platforms including ppc64.

In absence of a modified version, I do realize the situation here,
where DEBUG_VM_PGTABLE test either runs on ppc64 or just completely
remove hugetlb_advanced_tests() from other platforms as well.

> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  mm/debug_vm_pgtable.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
> index a188b6e4e37e..21329c7d672f 100644
> --- a/mm/debug_vm_pgtable.c
> +++ b/mm/debug_vm_pgtable.c
> @@ -813,6 +813,7 @@ static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot)
>  #endif /* CONFIG_ARCH_WANT_GENERAL_HUGETLB */
>  }
>  
> +#ifndef CONFIG_PPC_BOOK3S_64
>  static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>  					  struct vm_area_struct *vma,
>  					  pte_t *ptep, unsigned long pfn,
> @@ -855,6 +856,7 @@ static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>  	pte = huge_ptep_get(ptep);
>  	WARN_ON(!(huge_pte_write(pte) && huge_pte_dirty(pte)));
>  }
> +#endif

In the worst case if we could not get a new hugetlb_advanced_tests() test
that works on all platforms, this might be the last fallback option. In
which case, it will require a proper comment section with a "FIXME: ",
explaining the current situation (and that #ifdef is temporary in nature)
and a hugetlb_advanced_tests() stub when CONFIG_PPC_BOOK3S_64 is enabled.

>  #else  /* !CONFIG_HUGETLB_PAGE */
>  static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot) { }
>  static void __init hugetlb_advanced_tests(struct mm_struct *mm,
> @@ -1065,7 +1067,9 @@ static int __init debug_vm_pgtable(void)
>  	pud_populate_tests(mm, pudp, saved_pmdp);
>  	spin_unlock(ptl);
>  
> +#ifndef CONFIG_PPC_BOOK3S_64
>  	hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
> +#endif

#ifdef will not be required here as there would be a stub definition
for hugetlb_advanced_tests() when CONFIG_PPC_BOOK3S_64 is enabled.

>  
>  	spin_lock(&mm->page_table_lock);
>  	p4d_clear_tests(mm, p4dp);
> 

But again, we should really try and avoid taking this path.

Re: [PATCH v3 03/13] mm/debug_vm_pgtable/ppc64: Avoid setting top bits in radom value

[Aneesh Kumar K.V]

On 9/1/20 8:45 AM, Anshuman Khandual wrote:
> 
> 
> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>> ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
>> random value.
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>>   mm/debug_vm_pgtable.c | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>> index 086309fb9b6f..bbf9df0e64c6 100644
>> --- a/mm/debug_vm_pgtable.c
>> +++ b/mm/debug_vm_pgtable.c
>> @@ -44,10 +44,17 @@
>>    * entry type. But these bits might affect the ability to clear entries with
>>    * pxx_clear() because of how dynamic page table folding works on s390. So
>>    * while loading up the entries do not change the lower 4 bits. It does not
>> - * have affect any other platform.
>> + * have affect any other platform. Also avoid the 62nd bit on ppc64 that is
>> + * used to mark a pte entry.
>>    */
>> -#define S390_MASK_BITS	4
>> -#define RANDOM_ORVALUE	GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS)
>> +#define S390_SKIP_MASK		GENMASK(3, 0)
>> +#ifdef CONFIG_PPC_BOOK3S_64
>> +#define PPC64_SKIP_MASK		GENMASK(62, 62)
>> +#else
>> +#define PPC64_SKIP_MASK		0x0
>> +#endif
> 
> Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip
> bits for a s390 platform requirement and can also do so for ppc64 as well. As
> mentioned before, please avoid adding any platform specific constructs in the
> test.
> 


that is needed so that it can be built on 32 bit architectures.I did 
face build errors with arch-linux

>> +#define ARCH_SKIP_MASK (S390_SKIP_MASK | PPC64_SKIP_MASK)
>> +#define RANDOM_ORVALUE (GENMASK(BITS_PER_LONG - 1, 0) & ~ARCH_SKIP_MASK)
>>   #define RANDOM_NZVALUE	GENMASK(7, 0)
> 
> Please fix the alignments here. Feel free to consider following changes after
> this patch.
> 
> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
> index 122416464e0f..f969031152bb 100644
> --- a/mm/debug_vm_pgtable.c
> +++ b/mm/debug_vm_pgtable.c
> @@ -48,14 +48,11 @@
>    * have affect any other platform. Also avoid the 62nd bit on ppc64 that is
>    * used to mark a pte entry.
>    */
> -#define S390_SKIP_MASK         GENMASK(3, 0)
> -#ifdef CONFIG_PPC_BOOK3S_64
> -#define PPC64_SKIP_MASK                GENMASK(62, 62)
> -#else
> -#define PPC64_SKIP_MASK                0x0
> -#endif
> -#define ARCH_SKIP_MASK (S390_SKIP_MASK | PPC64_SKIP_MASK)
> -#define RANDOM_ORVALUE (GENMASK(BITS_PER_LONG - 1, 0) & ~ARCH_SKIP_MASK)
> +#define S390_SKIP_MASK GENMASK(3, 0)
> +#define PPC64_SKIP_MASK        GENMASK(62, 62)
> +#define ARCH_SKIP_MASK (S390_SKIP_MASK | PPC64_SKIP_MASK)
> +#define RANDOM_ORVALUE (GENMASK(BITS_PER_LONG - 1, 0) & ~ARCH_SKIP_MASK)
> +
>   #define RANDOM_NZVALUE GENMASK(7, 0)
>   
>>   
>>   static void __init pte_basic_tests(unsigned long pfn, pgprot_t prot)
>>
> 
> Besides, there is also one checkpatch.pl warning here.
> 
> WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
> #7:
> ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
> 
> total: 0 errors, 1 warnings, 20 lines checked
> 


These warnings are not valid. They are mostly long lines (upto 100) . or 
some details mentioned in the () as above.

-aneesh

Re: [PATCH v3 06/13] mm/debug_vm_pgtable/THP: Mark the pte entry huge before using set_pmd/pud_at

[Aneesh Kumar K.V]

On 9/1/20 8:51 AM, Anshuman Khandual wrote:
> 
> 
> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>> kernel expects entries to be marked huge before we use set_pmd_at()/set_pud_at().
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>>   mm/debug_vm_pgtable.c | 21 ++++++++++++---------
>>   1 file changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>> index 5c0680836fe9..de83a20c1b30 100644
>> --- a/mm/debug_vm_pgtable.c
>> +++ b/mm/debug_vm_pgtable.c
>> @@ -155,7 +155,7 @@ static void __init pmd_advanced_tests(struct mm_struct *mm,
>>   				      unsigned long pfn, unsigned long vaddr,
>>   				      pgprot_t prot)
>>   {
>> -	pmd_t pmd = pfn_pmd(pfn, prot);
>> +	pmd_t pmd;
>>   
>>   	if (!has_transparent_hugepage())
>>   		return;
>> @@ -164,19 +164,19 @@ static void __init pmd_advanced_tests(struct mm_struct *mm,
>>   	/* Align the address wrt HPAGE_PMD_SIZE */
>>   	vaddr = (vaddr & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE;
>>   
>> -	pmd = pfn_pmd(pfn, prot);
>> +	pmd = pmd_mkhuge(pfn_pmd(pfn, prot));
>>   	set_pmd_at(mm, vaddr, pmdp, pmd);
>>   	pmdp_set_wrprotect(mm, vaddr, pmdp);
>>   	pmd = READ_ONCE(*pmdp);
>>   	WARN_ON(pmd_write(pmd));
>>   
>> -	pmd = pfn_pmd(pfn, prot);
>> +	pmd = pmd_mkhuge(pfn_pmd(pfn, prot));
>>   	set_pmd_at(mm, vaddr, pmdp, pmd);
>>   	pmdp_huge_get_and_clear(mm, vaddr, pmdp);
>>   	pmd = READ_ONCE(*pmdp);
>>   	WARN_ON(!pmd_none(pmd));
>>   
>> -	pmd = pfn_pmd(pfn, prot);
>> +	pmd = pmd_mkhuge(pfn_pmd(pfn, prot));
>>   	pmd = pmd_wrprotect(pmd);
>>   	pmd = pmd_mkclean(pmd);
>>   	set_pmd_at(mm, vaddr, pmdp, pmd);
>> @@ -237,7 +237,7 @@ static void __init pmd_huge_tests(pmd_t *pmdp, unsigned long pfn, pgprot_t prot)
>>   
>>   static void __init pmd_savedwrite_tests(unsigned long pfn, pgprot_t prot)
>>   {
>> -	pmd_t pmd = pfn_pmd(pfn, prot);
>> +	pmd_t pmd = pmd_mkhuge(pfn_pmd(pfn, prot));
> 
> There is no set_pmd_at() in this particular test, why change ?


because if you are building a hugepage you should use pmd_mkhuge(). That 
is what is setting _PAGE_PTE with this series. We don't make pfn_pmd set 
_PAGE_PTE


-aneesh

Re: [PATCH v3 08/13] mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP

[Aneesh Kumar K.V]

On 9/1/20 8:52 AM, Anshuman Khandual wrote:
> 
> 
> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>> Architectures like ppc64 use deposited page table while updating the huge pte
>> entries.
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>>   mm/debug_vm_pgtable.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>> index f9f6358899a8..0ce5c6a24c5b 100644
>> --- a/mm/debug_vm_pgtable.c
>> +++ b/mm/debug_vm_pgtable.c
>> @@ -154,7 +154,7 @@ static void __init pmd_basic_tests(unsigned long pfn, pgprot_t prot)
>>   static void __init pmd_advanced_tests(struct mm_struct *mm,
>>   				      struct vm_area_struct *vma, pmd_t *pmdp,
>>   				      unsigned long pfn, unsigned long vaddr,
>> -				      pgprot_t prot)
>> +				      pgprot_t prot, pgtable_t pgtable)
>>   {
>>   	pmd_t pmd;
>>   
>> @@ -165,6 +165,8 @@ static void __init pmd_advanced_tests(struct mm_struct *mm,
>>   	/* Align the address wrt HPAGE_PMD_SIZE */
>>   	vaddr = (vaddr & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE;
>>   
>> +	pgtable_trans_huge_deposit(mm, pmdp, pgtable);
>> +
>>   	pmd = pmd_mkhuge(pfn_pmd(pfn, prot));
>>   	set_pmd_at(mm, vaddr, pmdp, pmd);
>>   	pmdp_set_wrprotect(mm, vaddr, pmdp);
>> @@ -193,6 +195,8 @@ static void __init pmd_advanced_tests(struct mm_struct *mm,
>>   	pmdp_test_and_clear_young(vma, vaddr, pmdp);
>>   	pmd = READ_ONCE(*pmdp);
>>   	WARN_ON(pmd_young(pmd));
>> +
>> +	pgtable = pgtable_trans_huge_withdraw(mm, pmdp);
> 
> Should the call sites here be wrapped with __HAVE_ARCH_PGTABLE_DEPOSIT and
> __HAVE_ARCH_PGTABLE_WITHDRAW respectively. Though there are generic fallback
> definitions, wondering whether they are indeed essential for all platforms.
> 

No, because Any page table helpers operating on pmd level THP can expect 
a deposited page table.

__HAVE_ARCH_PGTABLE_DEPOSIT indicates that fallback to generic version.

>>   }
>>   
>>   static void __init pmd_leaf_tests(unsigned long pfn, pgprot_t prot)
>> @@ -373,7 +377,7 @@ static void __init pud_basic_tests(unsigned long pfn, pgprot_t prot) { }
>>   static void __init pmd_advanced_tests(struct mm_struct *mm,
>>   				      struct vm_area_struct *vma, pmd_t *pmdp,
>>   				      unsigned long pfn, unsigned long vaddr,
>> -				      pgprot_t prot)
>> +				      pgprot_t prot, pgtable_t pgtable)
>>   {
>>   }
>>   static void __init pud_advanced_tests(struct mm_struct *mm,
>> @@ -1015,7 +1019,7 @@ static int __init debug_vm_pgtable(void)
>>   	pgd_clear_tests(mm, pgdp);
>>   
>>   	pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>> -	pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot);
>> +	pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep);
>>   	pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot);
>>   	hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>>   
>>
> 
> There is a checkpatch.pl warning here.
> 
> WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
> #7:
> Architectures like ppc64 use deposited page table while updating the huge pte
> 
> total: 0 errors, 1 warnings, 40 lines checked
> 

I will ignore all these, because they are not really important IMHO.

-aneesh

Re: [PATCH v3 12/13] mm/debug_vm_pgtable/hugetlb: Disable hugetlb test on ppc64

[Aneesh Kumar K.V]

On 9/1/20 9:33 AM, Anshuman Khandual wrote:
> 
> 
> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>> The seems to be missing quite a lot of details w.r.t allocating
>> the correct pgtable_t page (huge_pte_alloc()), holding the right
>> lock (huge_pte_lock()) etc. The vma used is also not a hugetlb VMA.
>>
>> ppc64 do have runtime checks within CONFIG_DEBUG_VM for most of these.
>> Hence disable the test on ppc64.
> 
> Would really like this to get resolved in an uniform and better way
> instead, i.e a modified hugetlb_advanced_tests() which works on all
> platforms including ppc64.
> 
> In absence of a modified version, I do realize the situation here,
> where DEBUG_VM_PGTABLE test either runs on ppc64 or just completely
> remove hugetlb_advanced_tests() from other platforms as well.
> 
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>>   mm/debug_vm_pgtable.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>> index a188b6e4e37e..21329c7d672f 100644
>> --- a/mm/debug_vm_pgtable.c
>> +++ b/mm/debug_vm_pgtable.c
>> @@ -813,6 +813,7 @@ static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot)
>>   #endif /* CONFIG_ARCH_WANT_GENERAL_HUGETLB */
>>   }
>>   
>> +#ifndef CONFIG_PPC_BOOK3S_64
>>   static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>>   					  struct vm_area_struct *vma,
>>   					  pte_t *ptep, unsigned long pfn,
>> @@ -855,6 +856,7 @@ static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>>   	pte = huge_ptep_get(ptep);
>>   	WARN_ON(!(huge_pte_write(pte) && huge_pte_dirty(pte)));
>>   }
>> +#endif
> 
> In the worst case if we could not get a new hugetlb_advanced_tests() test
> that works on all platforms, this might be the last fallback option. In
> which case, it will require a proper comment section with a "FIXME: ",
> explaining the current situation (and that #ifdef is temporary in nature)
> and a hugetlb_advanced_tests() stub when CONFIG_PPC_BOOK3S_64 is enabled.
> 
>>   #else  /* !CONFIG_HUGETLB_PAGE */
>>   static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot) { }
>>   static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>> @@ -1065,7 +1067,9 @@ static int __init debug_vm_pgtable(void)
>>   	pud_populate_tests(mm, pudp, saved_pmdp);
>>   	spin_unlock(ptl);
>>   
>> +#ifndef CONFIG_PPC_BOOK3S_64
>>   	hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>> +#endif
>

I actually wanted to add #ifdef BROKEN. That test is completely broken. 
Infact I would suggest to remove that test completely.



> #ifdef will not be required here as there would be a stub definition
> for hugetlb_advanced_tests() when CONFIG_PPC_BOOK3S_64 is enabled.
> 
>>   
>>   	spin_lock(&mm->page_table_lock);
>>   	p4d_clear_tests(mm, p4dp);
>>
> 
> But again, we should really try and avoid taking this path.
> 

To be frank i am kind of frustrated with how this patch series is being 
looked at. We pushed a completely broken test to upstream and right now 
we have a code in upstream that crash when booted on ppc64. My attempt 
has been to make progress here and you definitely seems to be not in 
agreement to that.

At this point I am tempted to suggest we remove the DEBUG_VM_PGTABLE 
support on ppc64 because AFAIU it doesn't add any value.


-aneesh

Re: [PATCH v3 13/13] mm/debug_vm_pgtable: populate a pte entry before fetching it

[Aneesh Kumar K.V]

On 9/1/20 8:55 AM, Anshuman Khandual wrote:
> 
> 
> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>> pte_clear_tests operate on an existing pte entry. Make sure that is not a none
>> pte entry.
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>>   mm/debug_vm_pgtable.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>> index 21329c7d672f..8527ebb75f2c 100644
>> --- a/mm/debug_vm_pgtable.c
>> +++ b/mm/debug_vm_pgtable.c
>> @@ -546,7 +546,7 @@ static void __init pgd_populate_tests(struct mm_struct *mm, pgd_t *pgdp,
>>   static void __init pte_clear_tests(struct mm_struct *mm, pte_t *ptep,
>>   				   unsigned long vaddr)
>>   {
>> -	pte_t pte = ptep_get(ptep);
>> +	pte_t pte =  ptep_get_and_clear(mm, vaddr, ptep);
> 
> Seems like ptep_get_and_clear() here just clears the entry in preparation
> for a following set_pte_at() which otherwise would have been a problem on
> ppc64 as you had pointed out earlier i.e set_pte_at() should not update an
> existing valid entry. So the commit message here is bit misleading.
> 

and also fetch the pte value which is used further.


>>   
>>   	pr_debug("Validating PTE clear\n");
>>   	pte = __pte(pte_val(pte) | RANDOM_ORVALUE);
>> @@ -944,7 +944,7 @@ static int __init debug_vm_pgtable(void)
>>   	p4d_t *p4dp, *saved_p4dp;
>>   	pud_t *pudp, *saved_pudp;
>>   	pmd_t *pmdp, *saved_pmdp, pmd;
>> -	pte_t *ptep;
>> +	pte_t *ptep, pte;
>>   	pgtable_t saved_ptep;
>>   	pgprot_t prot, protnone;
>>   	phys_addr_t paddr;
>> @@ -1049,6 +1049,8 @@ static int __init debug_vm_pgtable(void)
>>   	 */
>>   
>>   	ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
>> +	pte = pfn_pte(pte_aligned, prot);
>> +	set_pte_at(mm, vaddr, ptep, pte);
> 
> Not here, creating and populating an entry must be done in respective
> test functions itself. Besides, this seems bit redundant as well. The
> test pte_clear_tests() with the above change added, already
> 
> - Clears the PTEP entry with ptep_get_and_clear()

and fetch the old value set previously.

> - Creates and populates the PTEP with set_pte_at()
> - Clears with pte_clear()
> - Checks for pte_none()
> 
> If the objective is to clear the PTEP entry before calling set_pte_at(),
> then only the first chunk is required and it should also be merged with
> a previous patch.
> 
> [PATCH v3 07/13] mm/debug_vm_pgtable/set_pte/pmd/pud: Don't use set_*_at to update an existing pte entry
> 
> 
>>   	pte_clear_tests(mm, ptep, vaddr);
>>   	pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>>   	pte_unmap_unlock(ptep, ptl);
>>
> 
> There is a checkpatch.pl warning here.
> 
> WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
> #7:
> pte_clear_tests operate on an existing pte entry. Make sure that is not a none
> 
> total: 0 errors, 1 warnings, 24 lines checked
> 
> There is also a build failure on x86 reported from kernel test robot.
> 

Re: [PATCH v3 09/13] mm/debug_vm_pgtable/locks: Move non page table modifying test together

[Aneesh Kumar K.V]

On 9/1/20 9:11 AM, Anshuman Khandual wrote:
> 
> 
> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>> This will help in adding proper locks in a later patch
> 
> It really makes sense to classify these tests here as static and dynamic.
> Static are the ones that test via page table entry values modification
> without changing anything on the actual page table itself. Where as the
> dynamic tests do change the page table. Static tests would probably never
> require any lock synchronization but the dynamic ones will do.
> 
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>>   mm/debug_vm_pgtable.c | 52 ++++++++++++++++++++++++-------------------
>>   1 file changed, 29 insertions(+), 23 deletions(-)
>>
>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>> index 0ce5c6a24c5b..78c8af3445ac 100644
>> --- a/mm/debug_vm_pgtable.c
>> +++ b/mm/debug_vm_pgtable.c
>> @@ -992,7 +992,7 @@ static int __init debug_vm_pgtable(void)
>>   	p4dp = p4d_alloc(mm, pgdp, vaddr);
>>   	pudp = pud_alloc(mm, p4dp, vaddr);
>>   	pmdp = pmd_alloc(mm, pudp, vaddr);
>> -	ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
>> +	ptep = pte_alloc_map(mm, pmdp, vaddr);
>>   
>>   	/*
>>   	 * Save all the page table page addresses as the page table
>> @@ -1012,33 +1012,12 @@ static int __init debug_vm_pgtable(void)
>>   	p4d_basic_tests(p4d_aligned, prot);
>>   	pgd_basic_tests(pgd_aligned, prot);
>>   
>> -	pte_clear_tests(mm, ptep, vaddr);
>> -	pmd_clear_tests(mm, pmdp);
>> -	pud_clear_tests(mm, pudp);
>> -	p4d_clear_tests(mm, p4dp);
>> -	pgd_clear_tests(mm, pgdp);
>> -
>> -	pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>> -	pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep);
>> -	pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot);
>> -	hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>> -
>>   	pmd_leaf_tests(pmd_aligned, prot);
>>   	pud_leaf_tests(pud_aligned, prot);
>>   
>> -	pmd_huge_tests(pmdp, pmd_aligned, prot);
>> -	pud_huge_tests(pudp, pud_aligned, prot);
>> -
>>   	pte_savedwrite_tests(pte_aligned, protnone);
>>   	pmd_savedwrite_tests(pmd_aligned, protnone);
>>   
>> -	pte_unmap_unlock(ptep, ptl);
>> -
>> -	pmd_populate_tests(mm, pmdp, saved_ptep);
>> -	pud_populate_tests(mm, pudp, saved_pmdp);
>> -	p4d_populate_tests(mm, p4dp, saved_pudp);
>> -	pgd_populate_tests(mm, pgdp, saved_p4dp);
>> -
>>   	pte_special_tests(pte_aligned, prot);
>>   	pte_protnone_tests(pte_aligned, protnone);
>>   	pmd_protnone_tests(pmd_aligned, protnone);
>> @@ -1056,11 +1035,38 @@ static int __init debug_vm_pgtable(void)
>>   	pmd_swap_tests(pmd_aligned, prot);
>>   
>>   	swap_migration_tests();
>> -	hugetlb_basic_tests(pte_aligned, prot);
>>   
>>   	pmd_thp_tests(pmd_aligned, prot);
>>   	pud_thp_tests(pud_aligned, prot);
>>   
>> +	/*
>> +	 * Page table modifying tests
>> +	 */
> 
> In this comment, please do add some more details about how these tests
> need proper locking mechanism unlike the previous static ones. Also add
> a similar comment section for the static tests that dont really change
> page table and need not require corresponding locking mechanism. Both
> comment sections will make this classification clear.
> 
>> +	pte_clear_tests(mm, ptep, vaddr);
>> +	pmd_clear_tests(mm, pmdp);
>> +	pud_clear_tests(mm, pudp);
>> +	p4d_clear_tests(mm, p4dp);
>> +	pgd_clear_tests(mm, pgdp);
>> +
>> +	ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
>> +	pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>> +	pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep);
>> +	pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot);
>> +	hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>> +
>> +
>> +	pmd_huge_tests(pmdp, pmd_aligned, prot);
>> +	pud_huge_tests(pudp, pud_aligned, prot);
>> +
>> +	pte_unmap_unlock(ptep, ptl);
>> +
>> +	pmd_populate_tests(mm, pmdp, saved_ptep);
>> +	pud_populate_tests(mm, pudp, saved_pmdp);
>> +	p4d_populate_tests(mm, p4dp, saved_pudp);
>> +	pgd_populate_tests(mm, pgdp, saved_p4dp);
>> +
>> +	hugetlb_basic_tests(pte_aligned, prot);
> 
> hugetlb_basic_tests() is a non page table modifying static test and
> should be classified accordingly.
> 
>> +
>>   	p4d_free(mm, saved_p4dp);
>>   	pud_free(mm, saved_pudp);
>>   	pmd_free(mm, saved_pmdp);
>>
> 
> Changes till this patch fails to boot on arm64 platform. This should be
> folded with the next patch.
> 
> [   17.080644] ------------[ cut here ]------------
> [   17.081342] kernel BUG at mm/pgtable-generic.c:164!
> [   17.082091] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
> [   17.082977] Modules linked in:
> [   17.083481] CPU: 79 PID: 1 Comm: swapper/0 Tainted: G        W         5.9.0-rc2-00105-g740e72cd6717 #14
> [   17.084998] Hardware name: linux,dummy-virt (DT)
> [   17.085745] pstate: 60400005 (nZCv daif +PAN -UAO BTYPE=--)
> [   17.086645] pc : pgtable_trans_huge_deposit+0x58/0x60
> [   17.087462] lr : debug_vm_pgtable+0x4dc/0x8f0
> [   17.088168] sp : ffff80001219bcf0
> [   17.088710] x29: ffff80001219bcf0 x28: ffff8000114ac000
> [   17.089574] x27: ffff8000114ac000 x26: 0020000000000fd3
> [   17.090431] x25: 0020000081400fd3 x24: fffffe00175c12c0
> [   17.091286] x23: ffff0005df04d1a8 x22: 0000f18d6e035000
> [   17.092143] x21: ffff0005dd790000 x20: ffff0005df18e1a8
> [   17.093003] x19: ffff0005df04cb80 x18: 0000000000000014
> [   17.093859] x17: 00000000b76667d0 x16: 00000000fd4e6611
> [   17.094716] x15: 0000000000000001 x14: 0000000000000002
> [   17.095572] x13: 000000000055d966 x12: fffffe001755e400
> [   17.096429] x11: 0000000000000008 x10: ffff0005fcb6e210
> [   17.097292] x9 : ffff0005fcb6e210 x8 : 0020000081590fd3
> [   17.098149] x7 : ffff0005dd71e0c0 x6 : ffff0005df18e1a8
> [   17.099006] x5 : 0020000081590fd3 x4 : 0020000081590fd3
> [   17.099862] x3 : ffff0005df18e1a8 x2 : fffffe00175c12c0
> [   17.100718] x1 : fffffe00175c1300 x0 : 0000000000000000
> [   17.101583] Call trace:
> [   17.101993]  pgtable_trans_huge_deposit+0x58/0x60
> [   17.102754]  do_one_initcall+0x74/0x1cc
> [   17.103381]  kernel_init_freeable+0x1d0/0x238
> [   17.104089]  kernel_init+0x14/0x118
> [   17.104658]  ret_from_fork+0x10/0x34
> [   17.105251] Code: f9000443 f9000843 f9000822 d65f03c0 (d4210000)
> [   17.106303] ---[ end trace e63471e00f8c147f ]---
> 

IIUC, this should happen even without the series right? This is

	assert_spin_locked(pmd_lockptr(mm, pmdp));


-aneesh

Re: [PATCH v3 08/13] mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP

[Christophe Leroy]

Le 01/09/2020 à 08:25, Aneesh Kumar K.V a écrit :
> On 9/1/20 8:52 AM, Anshuman Khandual wrote:
>>
>>
>>
>> There is a checkpatch.pl warning here.
>>
>> WARNING: Possible unwrapped commit description (prefer a maximum 75 
>> chars per line)
>> #7:
>> Architectures like ppc64 use deposited page table while updating the 
>> huge pte
>>
>> total: 0 errors, 1 warnings, 40 lines checked
>>
> 
> I will ignore all these, because they are not really important IMHO.
> 

When doing a git log in a 80 chars terminal window, having wrapping 
lines is not really convenient. It should be easy to avoid it.

Christophe

Re: [PATCH v3 03/13] mm/debug_vm_pgtable/ppc64: Avoid setting top bits in radom value

[Anshuman Khandual]

On 09/01/2020 11:51 AM, Aneesh Kumar K.V wrote:
> On 9/1/20 8:45 AM, Anshuman Khandual wrote:
>>
>>
>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>>> ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
>>> random value.
>>>
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>> ---
>>>   mm/debug_vm_pgtable.c | 13 ++++++++++---
>>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>> index 086309fb9b6f..bbf9df0e64c6 100644
>>> --- a/mm/debug_vm_pgtable.c
>>> +++ b/mm/debug_vm_pgtable.c
>>> @@ -44,10 +44,17 @@
>>>    * entry type. But these bits might affect the ability to clear entries with
>>>    * pxx_clear() because of how dynamic page table folding works on s390. So
>>>    * while loading up the entries do not change the lower 4 bits. It does not
>>> - * have affect any other platform.
>>> + * have affect any other platform. Also avoid the 62nd bit on ppc64 that is
>>> + * used to mark a pte entry.
>>>    */
>>> -#define S390_MASK_BITS    4
>>> -#define RANDOM_ORVALUE    GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS)
>>> +#define S390_SKIP_MASK        GENMASK(3, 0)
>>> +#ifdef CONFIG_PPC_BOOK3S_64
>>> +#define PPC64_SKIP_MASK        GENMASK(62, 62)
>>> +#else
>>> +#define PPC64_SKIP_MASK        0x0
>>> +#endif
>>
>> Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip
>> bits for a s390 platform requirement and can also do so for ppc64 as well. As
>> mentioned before, please avoid adding any platform specific constructs in the
>> test.
>>
> 
> 
> that is needed so that it can be built on 32 bit architectures.I did face build errors with arch-linux

Could not (#if __BITS_PER_LONG == 32) be used instead or something like
that. But should be a generic conditional check identifying 32 bit arch
not anything platform specific.

Re: [PATCH v3 03/13] mm/debug_vm_pgtable/ppc64: Avoid setting top bits in radom value

[Aneesh Kumar K.V]

On 9/1/20 1:02 PM, Anshuman Khandual wrote:
> 
> 
> On 09/01/2020 11:51 AM, Aneesh Kumar K.V wrote:
>> On 9/1/20 8:45 AM, Anshuman Khandual wrote:
>>>
>>>
>>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>>>> ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
>>>> random value.
>>>>
>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>> ---
>>>>    mm/debug_vm_pgtable.c | 13 ++++++++++---
>>>>    1 file changed, 10 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>>> index 086309fb9b6f..bbf9df0e64c6 100644
>>>> --- a/mm/debug_vm_pgtable.c
>>>> +++ b/mm/debug_vm_pgtable.c
>>>> @@ -44,10 +44,17 @@
>>>>     * entry type. But these bits might affect the ability to clear entries with
>>>>     * pxx_clear() because of how dynamic page table folding works on s390. So
>>>>     * while loading up the entries do not change the lower 4 bits. It does not
>>>> - * have affect any other platform.
>>>> + * have affect any other platform. Also avoid the 62nd bit on ppc64 that is
>>>> + * used to mark a pte entry.
>>>>     */
>>>> -#define S390_MASK_BITS    4
>>>> -#define RANDOM_ORVALUE    GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS)
>>>> +#define S390_SKIP_MASK        GENMASK(3, 0)
>>>> +#ifdef CONFIG_PPC_BOOK3S_64
>>>> +#define PPC64_SKIP_MASK        GENMASK(62, 62)
>>>> +#else
>>>> +#define PPC64_SKIP_MASK        0x0
>>>> +#endif
>>>
>>> Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip
>>> bits for a s390 platform requirement and can also do so for ppc64 as well. As
>>> mentioned before, please avoid adding any platform specific constructs in the
>>> test.
>>>
>>
>>
>> that is needed so that it can be built on 32 bit architectures.I did face build errors with arch-linux
> 
> Could not (#if __BITS_PER_LONG == 32) be used instead or something like
> that. But should be a generic conditional check identifying 32 bit arch
> not anything platform specific.
> 

that _PAGE_PTE bit is pretty much specific to PPC BOOK3S_64.  Not sure 
why other architectures need to bothered about ignoring bit 62.

-aneesh

Re: [PATCH v3 13/13] mm/debug_vm_pgtable: populate a pte entry before fetching it

[Anshuman Khandual]

On 09/01/2020 12:07 PM, Aneesh Kumar K.V wrote:
> On 9/1/20 8:55 AM, Anshuman Khandual wrote:
>>
>>
>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>>> pte_clear_tests operate on an existing pte entry. Make sure that is not a none
>>> pte entry.
>>>
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>> ---
>>>   mm/debug_vm_pgtable.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>> index 21329c7d672f..8527ebb75f2c 100644
>>> --- a/mm/debug_vm_pgtable.c
>>> +++ b/mm/debug_vm_pgtable.c
>>> @@ -546,7 +546,7 @@ static void __init pgd_populate_tests(struct mm_struct *mm, pgd_t *pgdp,
>>>   static void __init pte_clear_tests(struct mm_struct *mm, pte_t *ptep,
>>>                      unsigned long vaddr)
>>>   {
>>> -    pte_t pte = ptep_get(ptep);
>>> +    pte_t pte =  ptep_get_and_clear(mm, vaddr, ptep);
>>
>> Seems like ptep_get_and_clear() here just clears the entry in preparation
>> for a following set_pte_at() which otherwise would have been a problem on
>> ppc64 as you had pointed out earlier i.e set_pte_at() should not update an
>> existing valid entry. So the commit message here is bit misleading.
>>
> 
> and also fetch the pte value which is used further.
> 
> 
>>>         pr_debug("Validating PTE clear\n");
>>>       pte = __pte(pte_val(pte) | RANDOM_ORVALUE);
>>> @@ -944,7 +944,7 @@ static int __init debug_vm_pgtable(void)
>>>       p4d_t *p4dp, *saved_p4dp;
>>>       pud_t *pudp, *saved_pudp;
>>>       pmd_t *pmdp, *saved_pmdp, pmd;
>>> -    pte_t *ptep;
>>> +    pte_t *ptep, pte;
>>>       pgtable_t saved_ptep;
>>>       pgprot_t prot, protnone;
>>>       phys_addr_t paddr;
>>> @@ -1049,6 +1049,8 @@ static int __init debug_vm_pgtable(void)
>>>        */
>>>         ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
>>> +    pte = pfn_pte(pte_aligned, prot);
>>> +    set_pte_at(mm, vaddr, ptep, pte);
>>
>> Not here, creating and populating an entry must be done in respective
>> test functions itself. Besides, this seems bit redundant as well. The
>> test pte_clear_tests() with the above change added, already
>>
>> - Clears the PTEP entry with ptep_get_and_clear()
> 
> and fetch the old value set previously.

In that case, please move above two lines i.e

pte = pfn_pte(pte_aligned, prot);
set_pte_at(mm, vaddr, ptep, pte);

from debug_vm_pgtable() to pte_clear_tests() and update it's arguments
as required.

Re: [PATCH v3 08/13] mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP

[Aneesh Kumar K.V]

On 9/1/20 12:20 PM, Christophe Leroy wrote:
> 
> 
> Le 01/09/2020 à 08:25, Aneesh Kumar K.V a écrit :
>> On 9/1/20 8:52 AM, Anshuman Khandual wrote:
>>>
>>>
>>>
>>> There is a checkpatch.pl warning here.
>>>
>>> WARNING: Possible unwrapped commit description (prefer a maximum 75 
>>> chars per line)
>>> #7:
>>> Architectures like ppc64 use deposited page table while updating the 
>>> huge pte
>>>
>>> total: 0 errors, 1 warnings, 40 lines checked
>>>
>>
>> I will ignore all these, because they are not really important IMHO.
>>
> 
> When doing a git log in a 80 chars terminal window, having wrapping 
> lines is not really convenient. It should be easy to avoid it.
> 

We have been ignoring that for a long time  isn't it?

For example ppc64 checkpatch already had
--max-line-length=90


There was also recent discussion whether 80 character limit is valid any 
more. But I do keep it restricted to 80 character where ever it is 
easy/make sense.

-aneesh

Re: [PATCH v3 03/13] mm/debug_vm_pgtable/ppc64: Avoid setting top bits in radom value

[Anshuman Khandual]

On 09/01/2020 01:06 PM, Aneesh Kumar K.V wrote:
> On 9/1/20 1:02 PM, Anshuman Khandual wrote:
>>
>>
>> On 09/01/2020 11:51 AM, Aneesh Kumar K.V wrote:
>>> On 9/1/20 8:45 AM, Anshuman Khandual wrote:
>>>>
>>>>
>>>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>>>>> ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
>>>>> random value.
>>>>>
>>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>>> ---
>>>>>    mm/debug_vm_pgtable.c | 13 ++++++++++---
>>>>>    1 file changed, 10 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>>>> index 086309fb9b6f..bbf9df0e64c6 100644
>>>>> --- a/mm/debug_vm_pgtable.c
>>>>> +++ b/mm/debug_vm_pgtable.c
>>>>> @@ -44,10 +44,17 @@
>>>>>     * entry type. But these bits might affect the ability to clear entries with
>>>>>     * pxx_clear() because of how dynamic page table folding works on s390. So
>>>>>     * while loading up the entries do not change the lower 4 bits. It does not
>>>>> - * have affect any other platform.
>>>>> + * have affect any other platform. Also avoid the 62nd bit on ppc64 that is
>>>>> + * used to mark a pte entry.
>>>>>     */
>>>>> -#define S390_MASK_BITS    4
>>>>> -#define RANDOM_ORVALUE    GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS)
>>>>> +#define S390_SKIP_MASK        GENMASK(3, 0)
>>>>> +#ifdef CONFIG_PPC_BOOK3S_64
>>>>> +#define PPC64_SKIP_MASK        GENMASK(62, 62)
>>>>> +#else
>>>>> +#define PPC64_SKIP_MASK        0x0
>>>>> +#endif
>>>>
>>>> Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip
>>>> bits for a s390 platform requirement and can also do so for ppc64 as well. As
>>>> mentioned before, please avoid adding any platform specific constructs in the
>>>> test.
>>>>
>>>
>>>
>>> that is needed so that it can be built on 32 bit architectures.I did face build errors with arch-linux
>>
>> Could not (#if __BITS_PER_LONG == 32) be used instead or something like
>> that. But should be a generic conditional check identifying 32 bit arch
>> not anything platform specific.
>>
> 
> that _PAGE_PTE bit is pretty much specific to PPC BOOK3S_64.  Not sure why other architectures need to bothered about ignoring bit 62.

Thats okay as long it does not adversely affect other architectures, ignoring
some more bits is acceptable. Like existing S390_MASK_BITS gets ignored on all
other platforms even if it is a s390 specific constraint. Not having platform
specific #ifdef here, is essential.

Re: [PATCH v3 08/13] mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP

[Christophe Leroy]

Le 01/09/2020 à 09:40, Aneesh Kumar K.V a écrit :
> On 9/1/20 12:20 PM, Christophe Leroy wrote:
>>
>>
>> Le 01/09/2020 à 08:25, Aneesh Kumar K.V a écrit :
>>> On 9/1/20 8:52 AM, Anshuman Khandual wrote:
>>>>
>>>>
>>>>
>>>> There is a checkpatch.pl warning here.
>>>>
>>>> WARNING: Possible unwrapped commit description (prefer a maximum 75 
>>>> chars per line)
>>>> #7:
>>>> Architectures like ppc64 use deposited page table while updating the 
>>>> huge pte
>>>>
>>>> total: 0 errors, 1 warnings, 40 lines checked
>>>>
>>>
>>> I will ignore all these, because they are not really important IMHO.
>>>
>>
>> When doing a git log in a 80 chars terminal window, having wrapping 
>> lines is not really convenient. It should be easy to avoid it.
>>
> 
> We have been ignoring that for a long time  isn't it?
> 
> For example ppc64 checkpatch already had
> --max-line-length=90
> 
> 
> There was also recent discussion whether 80 character limit is valid any 
> more. But I do keep it restricted to 80 character where ever it is 
> easy/make sense.
> 

Here we are not talking about the code, but the commit log.

As far as I know, the discussions about 80 character lines, 90 lines in 
powerpc etc ... is for the code.

We still aim at keeping lines not longer than 75 chars in the commit log.

Christophe

Christophe

Re: [PATCH v3 03/13] mm/debug_vm_pgtable/ppc64: Avoid setting top bits in radom value

[Aneesh Kumar K.V]

On 9/1/20 1:16 PM, Anshuman Khandual wrote:
> 
> 
> On 09/01/2020 01:06 PM, Aneesh Kumar K.V wrote:
>> On 9/1/20 1:02 PM, Anshuman Khandual wrote:
>>>
>>>
>>> On 09/01/2020 11:51 AM, Aneesh Kumar K.V wrote:
>>>> On 9/1/20 8:45 AM, Anshuman Khandual wrote:
>>>>>
>>>>>
>>>>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>>>>>> ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
>>>>>> random value.
>>>>>>
>>>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>>>> ---
>>>>>>     mm/debug_vm_pgtable.c | 13 ++++++++++---
>>>>>>     1 file changed, 10 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>>>>> index 086309fb9b6f..bbf9df0e64c6 100644
>>>>>> --- a/mm/debug_vm_pgtable.c
>>>>>> +++ b/mm/debug_vm_pgtable.c
>>>>>> @@ -44,10 +44,17 @@
>>>>>>      * entry type. But these bits might affect the ability to clear entries with
>>>>>>      * pxx_clear() because of how dynamic page table folding works on s390. So
>>>>>>      * while loading up the entries do not change the lower 4 bits. It does not
>>>>>> - * have affect any other platform.
>>>>>> + * have affect any other platform. Also avoid the 62nd bit on ppc64 that is
>>>>>> + * used to mark a pte entry.
>>>>>>      */
>>>>>> -#define S390_MASK_BITS    4
>>>>>> -#define RANDOM_ORVALUE    GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS)
>>>>>> +#define S390_SKIP_MASK        GENMASK(3, 0)
>>>>>> +#ifdef CONFIG_PPC_BOOK3S_64
>>>>>> +#define PPC64_SKIP_MASK        GENMASK(62, 62)
>>>>>> +#else
>>>>>> +#define PPC64_SKIP_MASK        0x0
>>>>>> +#endif
>>>>>
>>>>> Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip
>>>>> bits for a s390 platform requirement and can also do so for ppc64 as well. As
>>>>> mentioned before, please avoid adding any platform specific constructs in the
>>>>> test.
>>>>>
>>>>
>>>>
>>>> that is needed so that it can be built on 32 bit architectures.I did face build errors with arch-linux
>>>
>>> Could not (#if __BITS_PER_LONG == 32) be used instead or something like
>>> that. But should be a generic conditional check identifying 32 bit arch
>>> not anything platform specific.
>>>
>>
>> that _PAGE_PTE bit is pretty much specific to PPC BOOK3S_64.  Not sure why other architectures need to bothered about ignoring bit 62.
> 
> Thats okay as long it does not adversely affect other architectures, ignoring
> some more bits is acceptable. Like existing S390_MASK_BITS gets ignored on all
> other platforms even if it is a s390 specific constraint. Not having platform
> specific #ifdef here, is essential.
> 

Why is it essential?

-aneesh

Re: [PATCH v3 12/13] mm/debug_vm_pgtable/hugetlb: Disable hugetlb test on ppc64

[Christophe Leroy]

Le 01/09/2020 à 08:30, Aneesh Kumar K.V a écrit :
>>
> 
> I actually wanted to add #ifdef BROKEN. That test is completely broken. 
> Infact I would suggest to remove that test completely.
> 
> 
> 
>> #ifdef will not be required here as there would be a stub definition
>> for hugetlb_advanced_tests() when CONFIG_PPC_BOOK3S_64 is enabled.
>>
>>>       spin_lock(&mm->page_table_lock);
>>>       p4d_clear_tests(mm, p4dp);
>>>
>>
>> But again, we should really try and avoid taking this path.
>>
> 
> To be frank i am kind of frustrated with how this patch series is being 
> looked at. We pushed a completely broken test to upstream and right now 
> we have a code in upstream that crash when booted on ppc64. My attempt 
> has been to make progress here and you definitely seems to be not in 
> agreement to that.
> 
> At this point I am tempted to suggest we remove the DEBUG_VM_PGTABLE 
> support on ppc64 because AFAIU it doesn't add any value.
> 

Note that a bug has been filed at 
https://bugzilla.kernel.org/show_bug.cgi?id=209029

Christophe

Re: [PATCH v3 09/13] mm/debug_vm_pgtable/locks: Move non page table modifying test together

[Anshuman Khandual]

On 09/01/2020 12:08 PM, Aneesh Kumar K.V wrote:
> On 9/1/20 9:11 AM, Anshuman Khandual wrote:
>>
>>
>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>>> This will help in adding proper locks in a later patch
>>
>> It really makes sense to classify these tests here as static and dynamic.
>> Static are the ones that test via page table entry values modification
>> without changing anything on the actual page table itself. Where as the
>> dynamic tests do change the page table. Static tests would probably never
>> require any lock synchronization but the dynamic ones will do.
>>
>>>
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>> ---
>>>   mm/debug_vm_pgtable.c | 52 ++++++++++++++++++++++++-------------------
>>>   1 file changed, 29 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>> index 0ce5c6a24c5b..78c8af3445ac 100644
>>> --- a/mm/debug_vm_pgtable.c
>>> +++ b/mm/debug_vm_pgtable.c
>>> @@ -992,7 +992,7 @@ static int __init debug_vm_pgtable(void)
>>>       p4dp = p4d_alloc(mm, pgdp, vaddr);
>>>       pudp = pud_alloc(mm, p4dp, vaddr);
>>>       pmdp = pmd_alloc(mm, pudp, vaddr);
>>> -    ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
>>> +    ptep = pte_alloc_map(mm, pmdp, vaddr);
>>>         /*
>>>        * Save all the page table page addresses as the page table
>>> @@ -1012,33 +1012,12 @@ static int __init debug_vm_pgtable(void)
>>>       p4d_basic_tests(p4d_aligned, prot);
>>>       pgd_basic_tests(pgd_aligned, prot);
>>>   -    pte_clear_tests(mm, ptep, vaddr);
>>> -    pmd_clear_tests(mm, pmdp);
>>> -    pud_clear_tests(mm, pudp);
>>> -    p4d_clear_tests(mm, p4dp);
>>> -    pgd_clear_tests(mm, pgdp);
>>> -
>>> -    pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>>> -    pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep);
>>> -    pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot);
>>> -    hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>>> -
>>>       pmd_leaf_tests(pmd_aligned, prot);
>>>       pud_leaf_tests(pud_aligned, prot);
>>>   -    pmd_huge_tests(pmdp, pmd_aligned, prot);
>>> -    pud_huge_tests(pudp, pud_aligned, prot);
>>> -
>>>       pte_savedwrite_tests(pte_aligned, protnone);
>>>       pmd_savedwrite_tests(pmd_aligned, protnone);
>>>   -    pte_unmap_unlock(ptep, ptl);
>>> -
>>> -    pmd_populate_tests(mm, pmdp, saved_ptep);
>>> -    pud_populate_tests(mm, pudp, saved_pmdp);
>>> -    p4d_populate_tests(mm, p4dp, saved_pudp);
>>> -    pgd_populate_tests(mm, pgdp, saved_p4dp);
>>> -
>>>       pte_special_tests(pte_aligned, prot);
>>>       pte_protnone_tests(pte_aligned, protnone);
>>>       pmd_protnone_tests(pmd_aligned, protnone);
>>> @@ -1056,11 +1035,38 @@ static int __init debug_vm_pgtable(void)
>>>       pmd_swap_tests(pmd_aligned, prot);
>>>         swap_migration_tests();
>>> -    hugetlb_basic_tests(pte_aligned, prot);
>>>         pmd_thp_tests(pmd_aligned, prot);
>>>       pud_thp_tests(pud_aligned, prot);
>>>   +    /*
>>> +     * Page table modifying tests
>>> +     */
>>
>> In this comment, please do add some more details about how these tests
>> need proper locking mechanism unlike the previous static ones. Also add
>> a similar comment section for the static tests that dont really change
>> page table and need not require corresponding locking mechanism. Both
>> comment sections will make this classification clear.
>>
>>> +    pte_clear_tests(mm, ptep, vaddr);
>>> +    pmd_clear_tests(mm, pmdp);
>>> +    pud_clear_tests(mm, pudp);
>>> +    p4d_clear_tests(mm, p4dp);
>>> +    pgd_clear_tests(mm, pgdp);
>>> +
>>> +    ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
>>> +    pte_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>>> +    pmd_advanced_tests(mm, vma, pmdp, pmd_aligned, vaddr, prot, saved_ptep);
>>> +    pud_advanced_tests(mm, vma, pudp, pud_aligned, vaddr, prot);
>>> +    hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>>> +
>>> +
>>> +    pmd_huge_tests(pmdp, pmd_aligned, prot);
>>> +    pud_huge_tests(pudp, pud_aligned, prot);
>>> +
>>> +    pte_unmap_unlock(ptep, ptl);
>>> +
>>> +    pmd_populate_tests(mm, pmdp, saved_ptep);
>>> +    pud_populate_tests(mm, pudp, saved_pmdp);
>>> +    p4d_populate_tests(mm, p4dp, saved_pudp);
>>> +    pgd_populate_tests(mm, pgdp, saved_p4dp);
>>> +
>>> +    hugetlb_basic_tests(pte_aligned, prot);
>>
>> hugetlb_basic_tests() is a non page table modifying static test and
>> should be classified accordingly.
>>
>>> +
>>>       p4d_free(mm, saved_p4dp);
>>>       pud_free(mm, saved_pudp);
>>>       pmd_free(mm, saved_pmdp);
>>>
>>
>> Changes till this patch fails to boot on arm64 platform. This should be
>> folded with the next patch.
>>
>> [   17.080644] ------------[ cut here ]------------
>> [   17.081342] kernel BUG at mm/pgtable-generic.c:164!
>> [   17.082091] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>> [   17.082977] Modules linked in:
>> [   17.083481] CPU: 79 PID: 1 Comm: swapper/0 Tainted: G        W         5.9.0-rc2-00105-g740e72cd6717 #14
>> [   17.084998] Hardware name: linux,dummy-virt (DT)
>> [   17.085745] pstate: 60400005 (nZCv daif +PAN -UAO BTYPE=--)
>> [   17.086645] pc : pgtable_trans_huge_deposit+0x58/0x60
>> [   17.087462] lr : debug_vm_pgtable+0x4dc/0x8f0
>> [   17.088168] sp : ffff80001219bcf0
>> [   17.088710] x29: ffff80001219bcf0 x28: ffff8000114ac000
>> [   17.089574] x27: ffff8000114ac000 x26: 0020000000000fd3
>> [   17.090431] x25: 0020000081400fd3 x24: fffffe00175c12c0
>> [   17.091286] x23: ffff0005df04d1a8 x22: 0000f18d6e035000
>> [   17.092143] x21: ffff0005dd790000 x20: ffff0005df18e1a8
>> [   17.093003] x19: ffff0005df04cb80 x18: 0000000000000014
>> [   17.093859] x17: 00000000b76667d0 x16: 00000000fd4e6611
>> [   17.094716] x15: 0000000000000001 x14: 0000000000000002
>> [   17.095572] x13: 000000000055d966 x12: fffffe001755e400
>> [   17.096429] x11: 0000000000000008 x10: ffff0005fcb6e210
>> [   17.097292] x9 : ffff0005fcb6e210 x8 : 0020000081590fd3
>> [   17.098149] x7 : ffff0005dd71e0c0 x6 : ffff0005df18e1a8
>> [   17.099006] x5 : 0020000081590fd3 x4 : 0020000081590fd3
>> [   17.099862] x3 : ffff0005df18e1a8 x2 : fffffe00175c12c0
>> [   17.100718] x1 : fffffe00175c1300 x0 : 0000000000000000
>> [   17.101583] Call trace:
>> [   17.101993]  pgtable_trans_huge_deposit+0x58/0x60
>> [   17.102754]  do_one_initcall+0x74/0x1cc
>> [   17.103381]  kernel_init_freeable+0x1d0/0x238
>> [   17.104089]  kernel_init+0x14/0x118
>> [   17.104658]  ret_from_fork+0x10/0x34
>> [   17.105251] Code: f9000443 f9000843 f9000822 d65f03c0 (d4210000)
>> [   17.106303] ---[ end trace e63471e00f8c147f ]---
>>
> 
> IIUC, this should happen even without the series right? This is
> 
>     assert_spin_locked(pmd_lockptr(mm, pmdp));

Crash does not happen without this series. A previous patch [PATCH v3 08/13]
added pgtable_trans_huge_deposit/withdraw() in pmd_advanced_tests(). arm64
does not define __HAVE_ARCH_PGTABLE_DEPOSIT and hence falls back on generic
pgtable_trans_huge_deposit() which the asserts the lock.

Re: [PATCH v3 09/13] mm/debug_vm_pgtable/locks: Move non page table modifying test together

[Aneesh Kumar K.V]

>>>
>>> [   17.080644] ------------[ cut here ]------------
>>> [   17.081342] kernel BUG at mm/pgtable-generic.c:164!
>>> [   17.082091] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>>> [   17.082977] Modules linked in:
>>> [   17.083481] CPU: 79 PID: 1 Comm: swapper/0 Tainted: G        W         5.9.0-rc2-00105-g740e72cd6717 #14
>>> [   17.084998] Hardware name: linux,dummy-virt (DT)
>>> [   17.085745] pstate: 60400005 (nZCv daif +PAN -UAO BTYPE=--)
>>> [   17.086645] pc : pgtable_trans_huge_deposit+0x58/0x60
>>> [   17.087462] lr : debug_vm_pgtable+0x4dc/0x8f0
>>> [   17.088168] sp : ffff80001219bcf0
>>> [   17.088710] x29: ffff80001219bcf0 x28: ffff8000114ac000
>>> [   17.089574] x27: ffff8000114ac000 x26: 0020000000000fd3
>>> [   17.090431] x25: 0020000081400fd3 x24: fffffe00175c12c0
>>> [   17.091286] x23: ffff0005df04d1a8 x22: 0000f18d6e035000
>>> [   17.092143] x21: ffff0005dd790000 x20: ffff0005df18e1a8
>>> [   17.093003] x19: ffff0005df04cb80 x18: 0000000000000014
>>> [   17.093859] x17: 00000000b76667d0 x16: 00000000fd4e6611
>>> [   17.094716] x15: 0000000000000001 x14: 0000000000000002
>>> [   17.095572] x13: 000000000055d966 x12: fffffe001755e400
>>> [   17.096429] x11: 0000000000000008 x10: ffff0005fcb6e210
>>> [   17.097292] x9 : ffff0005fcb6e210 x8 : 0020000081590fd3
>>> [   17.098149] x7 : ffff0005dd71e0c0 x6 : ffff0005df18e1a8
>>> [   17.099006] x5 : 0020000081590fd3 x4 : 0020000081590fd3
>>> [   17.099862] x3 : ffff0005df18e1a8 x2 : fffffe00175c12c0
>>> [   17.100718] x1 : fffffe00175c1300 x0 : 0000000000000000
>>> [   17.101583] Call trace:
>>> [   17.101993]  pgtable_trans_huge_deposit+0x58/0x60
>>> [   17.102754]  do_one_initcall+0x74/0x1cc
>>> [   17.103381]  kernel_init_freeable+0x1d0/0x238
>>> [   17.104089]  kernel_init+0x14/0x118
>>> [   17.104658]  ret_from_fork+0x10/0x34
>>> [   17.105251] Code: f9000443 f9000843 f9000822 d65f03c0 (d4210000)
>>> [   17.106303] ---[ end trace e63471e00f8c147f ]---
>>>
>>
>> IIUC, this should happen even without the series right? This is
>>
>>      assert_spin_locked(pmd_lockptr(mm, pmdp));
> 
> Crash does not happen without this series. A previous patch [PATCH v3 08/13]
> added pgtable_trans_huge_deposit/withdraw() in pmd_advanced_tests(). arm64
> does not define __HAVE_ARCH_PGTABLE_DEPOSIT and hence falls back on generic
> pgtable_trans_huge_deposit() which the asserts the lock.
> 


I fixed that by moving the pgtable deposit after adding the pmd locks 
correctly.

mm/debug_vm_pgtable/locks: Move non page table modifying test together
mm/debug_vm_pgtable/locks: Take correct page table lock
mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP

-aneesh

Re: [PATCH v3 13/13] mm/debug_vm_pgtable: populate a pte entry before fetching it

[Aneesh Kumar K.V]

On 9/1/20 1:08 PM, Anshuman Khandual wrote:
> 
> 
> On 09/01/2020 12:07 PM, Aneesh Kumar K.V wrote:
>> On 9/1/20 8:55 AM, Anshuman Khandual wrote:
>>>
>>>
>>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>>>> pte_clear_tests operate on an existing pte entry. Make sure that is not a none
>>>> pte entry.
>>>>
>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>> ---
>>>>    mm/debug_vm_pgtable.c | 6 ++++--
>>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>>> index 21329c7d672f..8527ebb75f2c 100644
>>>> --- a/mm/debug_vm_pgtable.c
>>>> +++ b/mm/debug_vm_pgtable.c
>>>> @@ -546,7 +546,7 @@ static void __init pgd_populate_tests(struct mm_struct *mm, pgd_t *pgdp,
>>>>    static void __init pte_clear_tests(struct mm_struct *mm, pte_t *ptep,
>>>>                       unsigned long vaddr)
>>>>    {
>>>> -    pte_t pte = ptep_get(ptep);
>>>> +    pte_t pte =  ptep_get_and_clear(mm, vaddr, ptep);
>>>
>>> Seems like ptep_get_and_clear() here just clears the entry in preparation
>>> for a following set_pte_at() which otherwise would have been a problem on
>>> ppc64 as you had pointed out earlier i.e set_pte_at() should not update an
>>> existing valid entry. So the commit message here is bit misleading.
>>>
>>
>> and also fetch the pte value which is used further.
>>
>>
>>>>          pr_debug("Validating PTE clear\n");
>>>>        pte = __pte(pte_val(pte) | RANDOM_ORVALUE);
>>>> @@ -944,7 +944,7 @@ static int __init debug_vm_pgtable(void)
>>>>        p4d_t *p4dp, *saved_p4dp;
>>>>        pud_t *pudp, *saved_pudp;
>>>>        pmd_t *pmdp, *saved_pmdp, pmd;
>>>> -    pte_t *ptep;
>>>> +    pte_t *ptep, pte;
>>>>        pgtable_t saved_ptep;
>>>>        pgprot_t prot, protnone;
>>>>        phys_addr_t paddr;
>>>> @@ -1049,6 +1049,8 @@ static int __init debug_vm_pgtable(void)
>>>>         */
>>>>          ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
>>>> +    pte = pfn_pte(pte_aligned, prot);
>>>> +    set_pte_at(mm, vaddr, ptep, pte);
>>>
>>> Not here, creating and populating an entry must be done in respective
>>> test functions itself. Besides, this seems bit redundant as well. The
>>> test pte_clear_tests() with the above change added, already
>>>
>>> - Clears the PTEP entry with ptep_get_and_clear()
>>
>> and fetch the old value set previously.
> 
> In that case, please move above two lines i.e
> 
> pte = pfn_pte(pte_aligned, prot);
> set_pte_at(mm, vaddr, ptep, pte);
> 
> from debug_vm_pgtable() to pte_clear_tests() and update it's arguments
> as required.
> 

Frankly, I don't understand what these tests are testing. It all looks 
like some random clear and set.

static void __init pte_clear_tests(struct mm_struct *mm, pte_t *ptep,
				   unsigned long vaddr, unsigned long pfn,
				   pgprot_t prot)
{

	pte_t pte = pfn_pte(pfn, prot);
	set_pte_at(mm, vaddr, ptep, pte);

	pte =  ptep_get_and_clear(mm, vaddr, ptep);

	pr_debug("Validating PTE clear\n");
	pte = __pte(pte_val(pte) | RANDOM_ORVALUE);
	set_pte_at(mm, vaddr, ptep, pte);
	barrier();
	pte_clear(mm, vaddr, ptep);
	pte = ptep_get(ptep);
	WARN_ON(!pte_none(pte));
}


-aneesh

Re: [PATCH v3 09/13] mm/debug_vm_pgtable/locks: Move non page table modifying test together

[Anshuman Khandual]

On 09/01/2020 03:06 PM, Aneesh Kumar K.V wrote:
> 
>>>>
>>>> [   17.080644] ------------[ cut here ]------------
>>>> [   17.081342] kernel BUG at mm/pgtable-generic.c:164!
>>>> [   17.082091] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>>>> [   17.082977] Modules linked in:
>>>> [   17.083481] CPU: 79 PID: 1 Comm: swapper/0 Tainted: G        W         5.9.0-rc2-00105-g740e72cd6717 #14
>>>> [   17.084998] Hardware name: linux,dummy-virt (DT)
>>>> [   17.085745] pstate: 60400005 (nZCv daif +PAN -UAO BTYPE=--)
>>>> [   17.086645] pc : pgtable_trans_huge_deposit+0x58/0x60
>>>> [   17.087462] lr : debug_vm_pgtable+0x4dc/0x8f0
>>>> [   17.088168] sp : ffff80001219bcf0
>>>> [   17.088710] x29: ffff80001219bcf0 x28: ffff8000114ac000
>>>> [   17.089574] x27: ffff8000114ac000 x26: 0020000000000fd3
>>>> [   17.090431] x25: 0020000081400fd3 x24: fffffe00175c12c0
>>>> [   17.091286] x23: ffff0005df04d1a8 x22: 0000f18d6e035000
>>>> [   17.092143] x21: ffff0005dd790000 x20: ffff0005df18e1a8
>>>> [   17.093003] x19: ffff0005df04cb80 x18: 0000000000000014
>>>> [   17.093859] x17: 00000000b76667d0 x16: 00000000fd4e6611
>>>> [   17.094716] x15: 0000000000000001 x14: 0000000000000002
>>>> [   17.095572] x13: 000000000055d966 x12: fffffe001755e400
>>>> [   17.096429] x11: 0000000000000008 x10: ffff0005fcb6e210
>>>> [   17.097292] x9 : ffff0005fcb6e210 x8 : 0020000081590fd3
>>>> [   17.098149] x7 : ffff0005dd71e0c0 x6 : ffff0005df18e1a8
>>>> [   17.099006] x5 : 0020000081590fd3 x4 : 0020000081590fd3
>>>> [   17.099862] x3 : ffff0005df18e1a8 x2 : fffffe00175c12c0
>>>> [   17.100718] x1 : fffffe00175c1300 x0 : 0000000000000000
>>>> [   17.101583] Call trace:
>>>> [   17.101993]  pgtable_trans_huge_deposit+0x58/0x60
>>>> [   17.102754]  do_one_initcall+0x74/0x1cc
>>>> [   17.103381]  kernel_init_freeable+0x1d0/0x238
>>>> [   17.104089]  kernel_init+0x14/0x118
>>>> [   17.104658]  ret_from_fork+0x10/0x34
>>>> [   17.105251] Code: f9000443 f9000843 f9000822 d65f03c0 (d4210000)
>>>> [   17.106303] ---[ end trace e63471e00f8c147f ]---
>>>>
>>>
>>> IIUC, this should happen even without the series right? This is
>>>
>>>      assert_spin_locked(pmd_lockptr(mm, pmdp));
>>
>> Crash does not happen without this series. A previous patch [PATCH v3 08/13]
>> added pgtable_trans_huge_deposit/withdraw() in pmd_advanced_tests(). arm64
>> does not define __HAVE_ARCH_PGTABLE_DEPOSIT and hence falls back on generic
>> pgtable_trans_huge_deposit() which the asserts the lock.
>>
> 
> 
> I fixed that by moving the pgtable deposit after adding the pmd locks correctly.
> 
> mm/debug_vm_pgtable/locks: Move non page table modifying test together
> mm/debug_vm_pgtable/locks: Take correct page table lock
> mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP

Right, it does fix. But then both those patches should be folded/merged in
order to preserve git bisect ability, besides test classification reasons
as mentioned in a previous response and a possible redundant movement of
hugetlb_basic_tests().

Re: [PATCH v3 03/13] mm/debug_vm_pgtable/ppc64: Avoid setting top bits in radom value

[Anshuman Khandual]

On 09/01/2020 01:25 PM, Aneesh Kumar K.V wrote:
> On 9/1/20 1:16 PM, Anshuman Khandual wrote:
>>
>>
>> On 09/01/2020 01:06 PM, Aneesh Kumar K.V wrote:
>>> On 9/1/20 1:02 PM, Anshuman Khandual wrote:
>>>>
>>>>
>>>> On 09/01/2020 11:51 AM, Aneesh Kumar K.V wrote:
>>>>> On 9/1/20 8:45 AM, Anshuman Khandual wrote:
>>>>>>
>>>>>>
>>>>>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>>>>>>> ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in
>>>>>>> random value.
>>>>>>>
>>>>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>>>>> ---
>>>>>>>     mm/debug_vm_pgtable.c | 13 ++++++++++---
>>>>>>>     1 file changed, 10 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>>>>>> index 086309fb9b6f..bbf9df0e64c6 100644
>>>>>>> --- a/mm/debug_vm_pgtable.c
>>>>>>> +++ b/mm/debug_vm_pgtable.c
>>>>>>> @@ -44,10 +44,17 @@
>>>>>>>      * entry type. But these bits might affect the ability to clear entries with
>>>>>>>      * pxx_clear() because of how dynamic page table folding works on s390. So
>>>>>>>      * while loading up the entries do not change the lower 4 bits. It does not
>>>>>>> - * have affect any other platform.
>>>>>>> + * have affect any other platform. Also avoid the 62nd bit on ppc64 that is
>>>>>>> + * used to mark a pte entry.
>>>>>>>      */
>>>>>>> -#define S390_MASK_BITS    4
>>>>>>> -#define RANDOM_ORVALUE    GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS)
>>>>>>> +#define S390_SKIP_MASK        GENMASK(3, 0)
>>>>>>> +#ifdef CONFIG_PPC_BOOK3S_64
>>>>>>> +#define PPC64_SKIP_MASK        GENMASK(62, 62)
>>>>>>> +#else
>>>>>>> +#define PPC64_SKIP_MASK        0x0
>>>>>>> +#endif
>>>>>>
>>>>>> Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip
>>>>>> bits for a s390 platform requirement and can also do so for ppc64 as well. As
>>>>>> mentioned before, please avoid adding any platform specific constructs in the
>>>>>> test.
>>>>>>
>>>>>
>>>>>
>>>>> that is needed so that it can be built on 32 bit architectures.I did face build errors with arch-linux
>>>>
>>>> Could not (#if __BITS_PER_LONG == 32) be used instead or something like
>>>> that. But should be a generic conditional check identifying 32 bit arch
>>>> not anything platform specific.
>>>>
>>>
>>> that _PAGE_PTE bit is pretty much specific to PPC BOOK3S_64.  Not sure why other architectures need to bothered about ignoring bit 62.
>>
>> Thats okay as long it does not adversely affect other architectures, ignoring
>> some more bits is acceptable. Like existing S390_MASK_BITS gets ignored on all
>> other platforms even if it is a s390 specific constraint. Not having platform
>> specific #ifdef here, is essential.
>>
> 
> Why is it essential?
IIRC, I might have already replied on this couple of times. But let me try once more.
It is a generic test aimed at finding inconsistencies between different architectures
in terms of the page table helper semantics. Any platform specific construct here, to
'make things work' has the potential to hide such inconsistencies and defeat the very
purpose. The test/file here follows this rule consistently i.e there is not a single
platform specific #ifdef right now and would really like to continue maintaining this
property, unless until absolutely necessary. Current situation here wrt 32 bit archs
can easily be accommodated with a generic check such as __BITS_PER_LONG.

Re: [PATCH v3 08/13] mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP

[Anshuman Khandual]

On 09/01/2020 01:21 PM, Christophe Leroy wrote:
> 
> 
> Le 01/09/2020 à 09:40, Aneesh Kumar K.V a écrit :
>> On 9/1/20 12:20 PM, Christophe Leroy wrote:
>>>
>>>
>>> Le 01/09/2020 à 08:25, Aneesh Kumar K.V a écrit :
>>>> On 9/1/20 8:52 AM, Anshuman Khandual wrote:
>>>>>
>>>>>
>>>>>
>>>>> There is a checkpatch.pl warning here.
>>>>>
>>>>> WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
>>>>> #7:
>>>>> Architectures like ppc64 use deposited page table while updating the huge pte
>>>>>
>>>>> total: 0 errors, 1 warnings, 40 lines checked
>>>>>
>>>>
>>>> I will ignore all these, because they are not really important IMHO.
>>>>
>>>
>>> When doing a git log in a 80 chars terminal window, having wrapping lines is not really convenient. It should be easy to avoid it.
>>>
>>
>> We have been ignoring that for a long time  isn't it?
>>
>> For example ppc64 checkpatch already had
>> --max-line-length=90
>>
>>
>> There was also recent discussion whether 80 character limit is valid any more. But I do keep it restricted to 80 character where ever it is easy/make sense.
>>
> 
> Here we are not talking about the code, but the commit log.
> 
> As far as I know, the discussions about 80 character lines, 90 lines in powerpc etc ... is for the code.
> 
> We still aim at keeping lines not longer than 75 chars in the commit log.

Agreed.

Re: [PATCH v3 13/13] mm/debug_vm_pgtable: populate a pte entry before fetching it

[Anshuman Khandual]

On 09/01/2020 03:28 PM, Aneesh Kumar K.V wrote:
> On 9/1/20 1:08 PM, Anshuman Khandual wrote:
>>
>>
>> On 09/01/2020 12:07 PM, Aneesh Kumar K.V wrote:
>>> On 9/1/20 8:55 AM, Anshuman Khandual wrote:
>>>>
>>>>
>>>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>>>>> pte_clear_tests operate on an existing pte entry. Make sure that is not a none
>>>>> pte entry.
>>>>>
>>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>>> ---
>>>>>    mm/debug_vm_pgtable.c | 6 ++++--
>>>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>>>> index 21329c7d672f..8527ebb75f2c 100644
>>>>> --- a/mm/debug_vm_pgtable.c
>>>>> +++ b/mm/debug_vm_pgtable.c
>>>>> @@ -546,7 +546,7 @@ static void __init pgd_populate_tests(struct mm_struct *mm, pgd_t *pgdp,
>>>>>    static void __init pte_clear_tests(struct mm_struct *mm, pte_t *ptep,
>>>>>                       unsigned long vaddr)
>>>>>    {
>>>>> -    pte_t pte = ptep_get(ptep);
>>>>> +    pte_t pte =  ptep_get_and_clear(mm, vaddr, ptep);
>>>>
>>>> Seems like ptep_get_and_clear() here just clears the entry in preparation
>>>> for a following set_pte_at() which otherwise would have been a problem on
>>>> ppc64 as you had pointed out earlier i.e set_pte_at() should not update an
>>>> existing valid entry. So the commit message here is bit misleading.
>>>>
>>>
>>> and also fetch the pte value which is used further.
>>>
>>>
>>>>>          pr_debug("Validating PTE clear\n");
>>>>>        pte = __pte(pte_val(pte) | RANDOM_ORVALUE);
>>>>> @@ -944,7 +944,7 @@ static int __init debug_vm_pgtable(void)
>>>>>        p4d_t *p4dp, *saved_p4dp;
>>>>>        pud_t *pudp, *saved_pudp;
>>>>>        pmd_t *pmdp, *saved_pmdp, pmd;
>>>>> -    pte_t *ptep;
>>>>> +    pte_t *ptep, pte;
>>>>>        pgtable_t saved_ptep;
>>>>>        pgprot_t prot, protnone;
>>>>>        phys_addr_t paddr;
>>>>> @@ -1049,6 +1049,8 @@ static int __init debug_vm_pgtable(void)
>>>>>         */
>>>>>          ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
>>>>> +    pte = pfn_pte(pte_aligned, prot);
>>>>> +    set_pte_at(mm, vaddr, ptep, pte);
>>>>
>>>> Not here, creating and populating an entry must be done in respective
>>>> test functions itself. Besides, this seems bit redundant as well. The
>>>> test pte_clear_tests() with the above change added, already
>>>>
>>>> - Clears the PTEP entry with ptep_get_and_clear()
>>>
>>> and fetch the old value set previously.
>>
>> In that case, please move above two lines i.e
>>
>> pte = pfn_pte(pte_aligned, prot);
>> set_pte_at(mm, vaddr, ptep, pte);
>>
>> from debug_vm_pgtable() to pte_clear_tests() and update it's arguments
>> as required.
>>
> 
> Frankly, I don't understand what these tests are testing. It all looks like some random clear and set.

The idea here is to have some value with some randomness preferably, in
a given PTEP before attempting to clear the entry, in order to make sure
that pte_clear() is indeed clearing something of non-zero value.

> 
> static void __init pte_clear_tests(struct mm_struct *mm, pte_t *ptep,
>                    unsigned long vaddr, unsigned long pfn,
>                    pgprot_t prot)
> {
> 
>     pte_t pte = pfn_pte(pfn, prot);
>     set_pte_at(mm, vaddr, ptep, pte);
> 
>     pte =  ptep_get_and_clear(mm, vaddr, ptep);

Looking at this again, this preceding pfn_pte() followed by set_pte_at()
is not really required. Its reasonable to start with what ever was there
in the PTEP as a seed value which anyway gets added with RANDOM_ORVALUE.
s/ptep_get/ptep_get_and_clear is sufficient to take care of the powerpc
set_pte_at() constraint.

Re: [PATCH v3 13/13] mm/debug_vm_pgtable: populate a pte entry before fetching it

[Aneesh Kumar K.V]

On 9/2/20 9:19 AM, Anshuman Khandual wrote:
> 
> 
> On 09/01/2020 03:28 PM, Aneesh Kumar K.V wrote:
>> On 9/1/20 1:08 PM, Anshuman Khandual wrote:
>>>
>>>
>>> On 09/01/2020 12:07 PM, Aneesh Kumar K.V wrote:
>>>> On 9/1/20 8:55 AM, Anshuman Khandual wrote:
>>>>>
>>>>>
>>>>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>>>>>> pte_clear_tests operate on an existing pte entry. Make sure that is not a none
>>>>>> pte entry.
>>>>>>
>>>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>>>> ---
>>>>>>     mm/debug_vm_pgtable.c | 6 ++++--
>>>>>>     1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>>>>> index 21329c7d672f..8527ebb75f2c 100644
>>>>>> --- a/mm/debug_vm_pgtable.c
>>>>>> +++ b/mm/debug_vm_pgtable.c
>>>>>> @@ -546,7 +546,7 @@ static void __init pgd_populate_tests(struct mm_struct *mm, pgd_t *pgdp,
>>>>>>     static void __init pte_clear_tests(struct mm_struct *mm, pte_t *ptep,
>>>>>>                        unsigned long vaddr)
>>>>>>     {
>>>>>> -    pte_t pte = ptep_get(ptep);
>>>>>> +    pte_t pte =  ptep_get_and_clear(mm, vaddr, ptep);
>>>>>
>>>>> Seems like ptep_get_and_clear() here just clears the entry in preparation
>>>>> for a following set_pte_at() which otherwise would have been a problem on
>>>>> ppc64 as you had pointed out earlier i.e set_pte_at() should not update an
>>>>> existing valid entry. So the commit message here is bit misleading.
>>>>>
>>>>
>>>> and also fetch the pte value which is used further.
>>>>
>>>>
>>>>>>           pr_debug("Validating PTE clear\n");
>>>>>>         pte = __pte(pte_val(pte) | RANDOM_ORVALUE);
>>>>>> @@ -944,7 +944,7 @@ static int __init debug_vm_pgtable(void)
>>>>>>         p4d_t *p4dp, *saved_p4dp;
>>>>>>         pud_t *pudp, *saved_pudp;
>>>>>>         pmd_t *pmdp, *saved_pmdp, pmd;
>>>>>> -    pte_t *ptep;
>>>>>> +    pte_t *ptep, pte;
>>>>>>         pgtable_t saved_ptep;
>>>>>>         pgprot_t prot, protnone;
>>>>>>         phys_addr_t paddr;
>>>>>> @@ -1049,6 +1049,8 @@ static int __init debug_vm_pgtable(void)
>>>>>>          */
>>>>>>           ptep = pte_alloc_map_lock(mm, pmdp, vaddr, &ptl);
>>>>>> +    pte = pfn_pte(pte_aligned, prot);
>>>>>> +    set_pte_at(mm, vaddr, ptep, pte);
>>>>>
>>>>> Not here, creating and populating an entry must be done in respective
>>>>> test functions itself. Besides, this seems bit redundant as well. The
>>>>> test pte_clear_tests() with the above change added, already
>>>>>
>>>>> - Clears the PTEP entry with ptep_get_and_clear()
>>>>
>>>> and fetch the old value set previously.
>>>
>>> In that case, please move above two lines i.e
>>>
>>> pte = pfn_pte(pte_aligned, prot);
>>> set_pte_at(mm, vaddr, ptep, pte);
>>>
>>> from debug_vm_pgtable() to pte_clear_tests() and update it's arguments
>>> as required.
>>>
>>
>> Frankly, I don't understand what these tests are testing. It all looks like some random clear and set.
> 
> The idea here is to have some value with some randomness preferably, in
> a given PTEP before attempting to clear the entry, in order to make sure
> that pte_clear() is indeed clearing something of non-zero value.
> 
>>
>> static void __init pte_clear_tests(struct mm_struct *mm, pte_t *ptep,
>>                     unsigned long vaddr, unsigned long pfn,
>>                     pgprot_t prot)
>> {
>>
>>      pte_t pte = pfn_pte(pfn, prot);
>>      set_pte_at(mm, vaddr, ptep, pte);
>>
>>      pte =  ptep_get_and_clear(mm, vaddr, ptep);
> 
> Looking at this again, this preceding pfn_pte() followed by set_pte_at()
> is not really required. Its reasonable to start with what ever was there
> in the PTEP as a seed value which anyway gets added with RANDOM_ORVALUE.
> s/ptep_get/ptep_get_and_clear is sufficient to take care of the powerpc
> set_pte_at() constraint.
> 

But the way test is written we had none pte before. That is why I added 
that set_pte_at to put something there. With none pte the below sequence 
fails.

	pte = __pte(pte_val(pte) | RANDOM_ORVALUE);
	set_pte_at(mm, vaddr, ptep, pte);


because nobody is marking a _PAGE_PTE there.

	pte_t pte = pfn_pte(pfn, prot);

	pr_debug("Validating PTE clear\n");
	pte = __pte(pte_val(pte) | RANDOM_ORVALUE);
	set_pte_at(mm, vaddr, ptep, pte);
	barrier();
	pte_clear(mm, vaddr, ptep);
	pte = ptep_get(ptep);
	WARN_ON(!pte_none(pte));

will that work for you?

-aneesh

Re: [PATCH v3 12/13] mm/debug_vm_pgtable/hugetlb: Disable hugetlb test on ppc64

[Anshuman Khandual]

On 09/01/2020 12:00 PM, Aneesh Kumar K.V wrote:
> On 9/1/20 9:33 AM, Anshuman Khandual wrote:
>>
>>
>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>>> The seems to be missing quite a lot of details w.r.t allocating
>>> the correct pgtable_t page (huge_pte_alloc()), holding the right
>>> lock (huge_pte_lock()) etc. The vma used is also not a hugetlb VMA.
>>>
>>> ppc64 do have runtime checks within CONFIG_DEBUG_VM for most of these.
>>> Hence disable the test on ppc64.
>>
>> Would really like this to get resolved in an uniform and better way
>> instead, i.e a modified hugetlb_advanced_tests() which works on all
>> platforms including ppc64.
>>
>> In absence of a modified version, I do realize the situation here,
>> where DEBUG_VM_PGTABLE test either runs on ppc64 or just completely
>> remove hugetlb_advanced_tests() from other platforms as well.
>>
>>>
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>> ---
>>>   mm/debug_vm_pgtable.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>> index a188b6e4e37e..21329c7d672f 100644
>>> --- a/mm/debug_vm_pgtable.c
>>> +++ b/mm/debug_vm_pgtable.c
>>> @@ -813,6 +813,7 @@ static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot)
>>>   #endif /* CONFIG_ARCH_WANT_GENERAL_HUGETLB */
>>>   }
>>>   +#ifndef CONFIG_PPC_BOOK3S_64
>>>   static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>>>                         struct vm_area_struct *vma,
>>>                         pte_t *ptep, unsigned long pfn,
>>> @@ -855,6 +856,7 @@ static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>>>       pte = huge_ptep_get(ptep);
>>>       WARN_ON(!(huge_pte_write(pte) && huge_pte_dirty(pte)));
>>>   }
>>> +#endif
>>
>> In the worst case if we could not get a new hugetlb_advanced_tests() test
>> that works on all platforms, this might be the last fallback option. In
>> which case, it will require a proper comment section with a "FIXME: ",
>> explaining the current situation (and that #ifdef is temporary in nature)
>> and a hugetlb_advanced_tests() stub when CONFIG_PPC_BOOK3S_64 is enabled.
>>
>>>   #else  /* !CONFIG_HUGETLB_PAGE */
>>>   static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot) { }
>>>   static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>>> @@ -1065,7 +1067,9 @@ static int __init debug_vm_pgtable(void)
>>>       pud_populate_tests(mm, pudp, saved_pmdp);
>>>       spin_unlock(ptl);
>>>   +#ifndef CONFIG_PPC_BOOK3S_64
>>>       hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>>> +#endif
>>
> 
> I actually wanted to add #ifdef BROKEN. That test is completely broken. Infact I would suggest to remove that test completely.
> 
> 
> 
>> #ifdef will not be required here as there would be a stub definition
>> for hugetlb_advanced_tests() when CONFIG_PPC_BOOK3S_64 is enabled.
>>
>>>         spin_lock(&mm->page_table_lock);
>>>       p4d_clear_tests(mm, p4dp);
>>>
>>
>> But again, we should really try and avoid taking this path.
>>
> 
> To be frank i am kind of frustrated with how this patch series is being looked at. We pushed a completely broken test to upstream and right now we have a code in upstream that crash when booted on ppc64. My attempt has been to make progress here and you definitely seems to be not in agreement to that.
> 

I am afraid, this does not accurately represent the situation.

- The second set patch series got merged in it's V5 after accommodating almost
  all reviews and objections during previous discussion cycles. For a complete
  development log, please refer https://patchwork.kernel.org/cover/11658627/.

- The series has been repeatedly tested on arm64 and x86 platforms for multiple
  configurations but build tested on all other enabled platforms. I have always
  been dependent on voluntary help from folks on the list to get this tested on
  other enabled platforms as I dont have access to such systems. Always assumed
  that is the way to go for anything which runs on multiple platforms. So, am I
  expected to test on platforms that I dont have access to ? But I am ready to
  be corrected here, if the community protocol is not what I have always assumed
  it to be.

- Each and every version of the series had appropriately copied all the enabled
  platform's mailing list. Also, I had explicitly asked for volunteers to test
  this out on platforms apart from x86 and arm64. We had positive response from
  all platforms i.e arc, s390, ppc32 but except for ppc64.

  https://patchwork.kernel.org/cover/11644771/
  https://patchwork.kernel.org/cover/11603713/

- The development cycle provided sufficient time window for any detailed review
  and test. I have always been willing to address almost all the issues brought
  forward during these discussions. From past experience on this test, there is
  an inherent need to understand platform specific details while trying to come
  up with something generic enough that works on all platforms. It necessitates
  participation from relevant folks to enable this test on a given platform. We
  were able to enable this on arm64, x86, arc, s390, powerpc following a similar
  model.

- I have to disagree here that the concerned test i.e hugetlb_advanced_tests()
  is completely broken. As mentioned before, the idea here has always been to
  emulate enough MM objects, so that a given page table helper could be tested.
  hugetlb_advanced_tests() seems to be insufficient on ppc64 platform causing it
  to crash, which is not the case on other platforms. But it is not perfect and
  can be improved upon. Given the constraints i.e limited emulation of objects,
  the test tries to do the right thing. Calling it broken is not an appropriate
  description.

> At this point I am tempted to suggest we remove the DEBUG_VM_PGTABLE support on ppc64 because AFAIU it doesn't add any value.

Now that this has been proposed [1], along with it any possible urgency factor
out of the way, we could review the proposal with a more long term view making
sure that it improves existing tests and benefits all enabled platforms. Will
look forward to it.

[1] https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20200901094423.100149-1-aneesh.kumar@linux.ibm.com/

- Anshuman

Re: [PATCH v3 12/13] mm/debug_vm_pgtable/hugetlb: Disable hugetlb test on ppc64

[Aneesh Kumar K.V]

Anshuman Khandual <anshuman.khandual@arm.com> writes:

> On 09/01/2020 12:00 PM, Aneesh Kumar K.V wrote:
>> On 9/1/20 9:33 AM, Anshuman Khandual wrote:
>>>
>>>
>>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote:
>>>> The seems to be missing quite a lot of details w.r.t allocating
>>>> the correct pgtable_t page (huge_pte_alloc()), holding the right
>>>> lock (huge_pte_lock()) etc. The vma used is also not a hugetlb VMA.
>>>>
>>>> ppc64 do have runtime checks within CONFIG_DEBUG_VM for most of these.
>>>> Hence disable the test on ppc64.
>>>
>>> Would really like this to get resolved in an uniform and better way
>>> instead, i.e a modified hugetlb_advanced_tests() which works on all
>>> platforms including ppc64.
>>>
>>> In absence of a modified version, I do realize the situation here,
>>> where DEBUG_VM_PGTABLE test either runs on ppc64 or just completely
>>> remove hugetlb_advanced_tests() from other platforms as well.
>>>
>>>>
>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>> ---
>>>>   mm/debug_vm_pgtable.c | 4 ++++
>>>>   1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>>> index a188b6e4e37e..21329c7d672f 100644
>>>> --- a/mm/debug_vm_pgtable.c
>>>> +++ b/mm/debug_vm_pgtable.c
>>>> @@ -813,6 +813,7 @@ static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot)
>>>>   #endif /* CONFIG_ARCH_WANT_GENERAL_HUGETLB */
>>>>   }
>>>>   +#ifndef CONFIG_PPC_BOOK3S_64
>>>>   static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>>>>                         struct vm_area_struct *vma,
>>>>                         pte_t *ptep, unsigned long pfn,
>>>> @@ -855,6 +856,7 @@ static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>>>>       pte = huge_ptep_get(ptep);
>>>>       WARN_ON(!(huge_pte_write(pte) && huge_pte_dirty(pte)));
>>>>   }
>>>> +#endif
>>>
>>> In the worst case if we could not get a new hugetlb_advanced_tests() test
>>> that works on all platforms, this might be the last fallback option. In
>>> which case, it will require a proper comment section with a "FIXME: ",
>>> explaining the current situation (and that #ifdef is temporary in nature)
>>> and a hugetlb_advanced_tests() stub when CONFIG_PPC_BOOK3S_64 is enabled.
>>>
>>>>   #else  /* !CONFIG_HUGETLB_PAGE */
>>>>   static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot) { }
>>>>   static void __init hugetlb_advanced_tests(struct mm_struct *mm,
>>>> @@ -1065,7 +1067,9 @@ static int __init debug_vm_pgtable(void)
>>>>       pud_populate_tests(mm, pudp, saved_pmdp);
>>>>       spin_unlock(ptl);
>>>>   +#ifndef CONFIG_PPC_BOOK3S_64
>>>>       hugetlb_advanced_tests(mm, vma, ptep, pte_aligned, vaddr, prot);
>>>> +#endif
>>>
>> 
>> I actually wanted to add #ifdef BROKEN. That test is completely broken. Infact I would suggest to remove that test completely.
>> 
>> 
>> 
>>> #ifdef will not be required here as there would be a stub definition
>>> for hugetlb_advanced_tests() when CONFIG_PPC_BOOK3S_64 is enabled.
>>>
>>>>         spin_lock(&mm->page_table_lock);
>>>>       p4d_clear_tests(mm, p4dp);
>>>>
>>>
>>> But again, we should really try and avoid taking this path.
>>>
>> 
>> To be frank i am kind of frustrated with how this patch series is being looked at. We pushed a completely broken test to upstream and right now we have a code in upstream that crash when booted on ppc64. My attempt has been to make progress here and you definitely seems to be not in agreement to that.
>> 
>
> I am afraid, this does not accurately represent the situation.
>
> - The second set patch series got merged in it's V5 after accommodating almost
>   all reviews and objections during previous discussion cycles. For a complete
>   development log, please refer https://patchwork.kernel.org/cover/11658627/.
>
> - The series has been repeatedly tested on arm64 and x86 platforms for multiple
>   configurations but build tested on all other enabled platforms. I have always
>   been dependent on voluntary help from folks on the list to get this tested on
>   other enabled platforms as I dont have access to such systems. Always assumed
>   that is the way to go for anything which runs on multiple platforms. So, am I
>   expected to test on platforms that I dont have access to ? But I am ready to
>   be corrected here, if the community protocol is not what I have always assumed
>   it to be.
>
> - Each and every version of the series had appropriately copied all the enabled
>   platform's mailing list. Also, I had explicitly asked for volunteers to test
>   this out on platforms apart from x86 and arm64. We had positive response from
>   all platforms i.e arc, s390, ppc32 but except for ppc64.
>
>   https://patchwork.kernel.org/cover/11644771/
>   https://patchwork.kernel.org/cover/11603713/
>
> - The development cycle provided sufficient time window for any detailed review
>   and test. I have always been willing to address almost all the issues brought
>   forward during these discussions. From past experience on this test, there is
>   an inherent need to understand platform specific details while trying to come
>   up with something generic enough that works on all platforms. It necessitates
>   participation from relevant folks to enable this test on a given platform. We
>   were able to enable this on arm64, x86, arc, s390, powerpc following a similar
>   model.
>
> - I have to disagree here that the concerned test i.e hugetlb_advanced_tests()
>   is completely broken. As mentioned before, the idea here has always been to
>   emulate enough MM objects, so that a given page table helper could be tested.
>   hugetlb_advanced_tests() seems to be insufficient on ppc64 platform causing it
>   to crash, which is not the case on other platforms. But it is not perfect and
>   can be improved upon. Given the constraints i.e limited emulation of objects,
>   the test tries to do the right thing. Calling it broken is not an appropriate
>   description.
>


None of the fixes done here are specific to ppc64. I am not sure why you
keep suggesting ppc64 specific issues. One should not do page table
updates without holding locks. A hugetlb pte updates expect a vma marked
hugetlb.

As explained in the patch, I see very little value in a bunch of tests
like this and the only reason I started to fix this up is because of
multiple crash reports on ppc64.

Considering the hugetlb tests require much larger change and as it is
currently written is broken, I wanted to remove that test and let you
come up with a proper test. But since you had it "working", I disabled
this only on ppc64.

But you keep suggesting that the hugetlb test need to be fixed as part
of the patch series review. I don't have enough motivation to fix that,
because I don't see much value in a bunch of tests like these. As shown
already these tests already reported success till now without even
following any page table update rules.

-aneesh