Re: [PATCH 2/2] mm/debug_vm_pgtable/basic: Iterate over entire protection_map[]

[Steven Price <steven.price@xxxxxxx>]

On 27/11/2020 05:06, Anshuman Khandual wrote:
> Currently the basic tests just validate various page table transformations
> after starting with vm_get_page_prot(VM_READ|VM_WRITE|VM_EXEC) protection.
> Instead scan over the entire protection_map[] for better coverage. It also
> makes sure that all these basic page table tranformations checks hold true
> irrespective of the starting protection value for the page table entry.
> There is also a slight change in the debug print format for basic tests to
> capture the protection value it is being tested with. The modified output
> looks something like
>
> [pte_basic_tests          ]: Validating PTE basic ()
> [pte_basic_tests          ]: Validating PTE basic (read)
> [pte_basic_tests          ]: Validating PTE basic (write)
> [pte_basic_tests          ]: Validating PTE basic (read|write)
> [pte_basic_tests          ]: Validating PTE basic (exec)
> [pte_basic_tests          ]: Validating PTE basic (read|exec)
> [pte_basic_tests          ]: Validating PTE basic (write|exec)
> [pte_basic_tests          ]: Validating PTE basic (read|write|exec)
> [pte_basic_tests          ]: Validating PTE basic (shared)
> [pte_basic_tests          ]: Validating PTE basic (read|shared)
> [pte_basic_tests          ]: Validating PTE basic (write|shared)
> [pte_basic_tests          ]: Validating PTE basic (read|write|shared)
> [pte_basic_tests          ]: Validating PTE basic (exec|shared)
> [pte_basic_tests          ]: Validating PTE basic (read|exec|shared)
> [pte_basic_tests          ]: Validating PTE basic (write|exec|shared)
> [pte_basic_tests          ]: Validating PTE basic (read|write|exec|shared)
>
> This adds a missing argument 'struct mm_struct *' in pud_basic_tests() test
> . This never got exposed before as PUD based THP is available only on X86
> platform where mm_pmd_folded(mm) call gets macro replaced without requiring
> the mm_struct i.e __is_defined(__PAGETABLE_PMD_FOLDED).
>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: linux-mm@xxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Suggested-by: Catalin Marinas <catalin.marinas@xxxxxxx>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>

Reviewed-by: Steven Price <steven.price@xxxxxxx>

> ---
>   mm/debug_vm_pgtable.c | 47 ++++++++++++++++++++++++++++++++-----------
>   1 file changed, 35 insertions(+), 12 deletions(-)
>
> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
> index a5be11210597..92b4a53d622b 100644
> --- a/mm/debug_vm_pgtable.c
> +++ b/mm/debug_vm_pgtable.c
> @@ -58,11 +58,13 @@
>   #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)
> +static void __init pte_basic_tests(unsigned long pfn, int idx)
>   {
> +	pgprot_t prot = protection_map[idx];
>   	pte_t pte = pfn_pte(pfn, prot);
> +	unsigned long val = idx, *ptr = &val;
>   
> -	pr_debug("Validating PTE basic\n");
> +	pr_debug("Validating PTE basic (%pGv)\n", ptr);
>   	WARN_ON(!pte_same(pte, pte));
>   	WARN_ON(!pte_young(pte_mkyoung(pte_mkold(pte))));
>   	WARN_ON(!pte_dirty(pte_mkdirty(pte_mkclean(pte))));
> @@ -130,14 +132,16 @@ static void __init pte_savedwrite_tests(unsigned long pfn, pgprot_t prot)
>   }
>   
>   #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> -static void __init pmd_basic_tests(unsigned long pfn, pgprot_t prot)
> +static void __init pmd_basic_tests(unsigned long pfn, int idx)
>   {
> +	pgprot_t prot = protection_map[idx];
>   	pmd_t pmd = pfn_pmd(pfn, prot);
> +	unsigned long val = idx, *ptr = &val;
>   
>   	if (!has_transparent_hugepage())
>   		return;
>   
> -	pr_debug("Validating PMD basic\n");
> +	pr_debug("Validating PMD basic (%pGv)\n", ptr);
>   	WARN_ON(!pmd_same(pmd, pmd));
>   	WARN_ON(!pmd_young(pmd_mkyoung(pmd_mkold(pmd))));
>   	WARN_ON(!pmd_dirty(pmd_mkdirty(pmd_mkclean(pmd))));
> @@ -251,14 +255,16 @@ static void __init pmd_savedwrite_tests(unsigned long pfn, pgprot_t prot)
>   }
>   
>   #ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
> -static void __init pud_basic_tests(unsigned long pfn, pgprot_t prot)
> +static void __init pud_basic_tests(struct mm_struct *mm, unsigned long pfn, int idx)
>   {
> +	pgprot_t prot = protection_map[idx];
>   	pud_t pud = pfn_pud(pfn, prot);
> +	unsigned long val = idx, *ptr = &val;
>   
>   	if (!has_transparent_hugepage())
>   		return;
>   
> -	pr_debug("Validating PUD basic\n");
> +	pr_debug("Validating PUD basic (%pGv)\n", ptr);
>   	WARN_ON(!pud_same(pud, pud));
>   	WARN_ON(!pud_young(pud_mkyoung(pud_mkold(pud))));
>   	WARN_ON(!pud_write(pud_mkwrite(pud_wrprotect(pud))));
> @@ -362,7 +368,7 @@ 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_basic_tests(struct mm_struct *mm, unsigned long pfn, int idx) { }
>   static void __init pud_advanced_tests(struct mm_struct *mm,
>   				      struct vm_area_struct *vma, pud_t *pudp,
>   				      unsigned long pfn, unsigned long vaddr,
> @@ -375,8 +381,8 @@ static void __init pud_huge_tests(pud_t *pudp, unsigned long pfn, pgprot_t prot)
>   }
>   #endif /* CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD */
>   #else  /* !CONFIG_TRANSPARENT_HUGEPAGE */
> -static void __init pmd_basic_tests(unsigned long pfn, pgprot_t prot) { }
> -static void __init pud_basic_tests(unsigned long pfn, pgprot_t prot) { }
> +static void __init pmd_basic_tests(unsigned long pfn, int idx) { }
> +static void __init pud_basic_tests(struct mm_struct *mm, unsigned long pfn, int idx) { }
>   static void __init pmd_advanced_tests(struct mm_struct *mm,
>   				      struct vm_area_struct *vma, pmd_t *pmdp,
>   				      unsigned long pfn, unsigned long vaddr,
> @@ -902,6 +908,7 @@ static int __init debug_vm_pgtable(void)
>   	unsigned long vaddr, pte_aligned, pmd_aligned;
>   	unsigned long pud_aligned, p4d_aligned, pgd_aligned;
>   	spinlock_t *ptl = NULL;
> +	int idx;
>   
>   	pr_info("Validating architecture page table helpers\n");
>   	prot = vm_get_page_prot(VMFLAGS);
> @@ -966,9 +973,25 @@ static int __init debug_vm_pgtable(void)
>   	saved_pmdp = pmd_offset(pudp, 0UL);
>   	saved_ptep = pmd_pgtable(pmd);
>   
> -	pte_basic_tests(pte_aligned, prot);
> -	pmd_basic_tests(pmd_aligned, prot);
> -	pud_basic_tests(pud_aligned, prot);
> +	/*
> +	 * Iterate over the protection_map[] to make sure that all
> +	 * the basic page table transformation validations just hold
> +	 * true irrespective of the starting protection value for a
> +	 * given page table entry.
> +	 */
> +	for (idx = 0; idx < ARRAY_SIZE(protection_map); idx++) {
> +		pte_basic_tests(pte_aligned, idx);
> +		pmd_basic_tests(pmd_aligned, idx);
> +		pud_basic_tests(mm, pud_aligned, idx);
> +	}
> +
> +	/*
> +	 * Both P4D and PGD level tests are very basic which do not
> +	 * involve creating page table entries from the protection
> +	 * value and the given pfn. Hence just keep them out from
> +	 * the above iteration for now to save some test execution
> +	 * time.
> +	 */
>   	p4d_basic_tests(p4d_aligned, prot);
>   	pgd_basic_tests(pgd_aligned, prot);
>