Re: [PATCH] mm/debug: Add tests validating arch page table helpers for core features

[Anshuman Khandual]

On 03/03/2020 02:54 AM, Christophe Leroy wrote:
> Anshuman Khandual <anshuman.khandual@xxxxxxx> a ÃcritÂ:
> 
>> On 02/27/2020 04:59 PM, Christophe Leroy wrote:
>>>
>>>
>>> Le 27/02/2020 Ã 11:33, Anshuman Khandual a ÃcritÂ:
>>>> This adds new tests validating arch page table helpers for these following
>>>> core memory features. These tests create and test specific mapping types at
>>>> various page table levels.
>>>>
>>>> * SPECIAL mapping
>>>> * PROTNONE mapping
>>>> * DEVMAP mapping
>>>> * SOFTDIRTY mapping
>>>> * SWAP mapping
>>>> * MIGRATION mapping
>>>> * HUGETLB mapping
>>>> * THP mapping
>>>>
>>>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>>>> Cc: Mike Rapoport <rppt@xxxxxxxxxxxxx>
>>>> Cc: Vineet Gupta <vgupta@xxxxxxxxxxxx>
>>>> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>>>> Cc: Will Deacon <will@xxxxxxxxxx>
>>>> Cc: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>
>>>> Cc: Paul Mackerras <paulus@xxxxxxxxx>
>>>> Cc: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
>>>> Cc: Heiko Carstens <heiko.carstens@xxxxxxxxxx>
>>>> Cc: Vasily Gorbik <gor@xxxxxxxxxxxxx>
>>>> Cc: Christian Borntraeger <borntraeger@xxxxxxxxxx>
>>>> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>>>> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>>>> Cc: Borislav Petkov <bp@xxxxxxxxx>
>>>> Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
>>>> Cc: Kirill A. Shutemov <kirill@xxxxxxxxxxxxx>
>>>> Cc: Paul Walmsley <paul.walmsley@xxxxxxxxxx>
>>>> Cc: Palmer Dabbelt <palmer@xxxxxxxxxxx>
>>>> Cc: linux-snps-arc@xxxxxxxxxxxxxxxxxxx
>>>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>>>> Cc: linuxppc-dev@xxxxxxxxxxxxxxxx
>>>> Cc: linux-s390@xxxxxxxxxxxxxxx
>>>> Cc: linux-riscv@xxxxxxxxxxxxxxxxxxx
>>>> Cc: x86@xxxxxxxxxx
>>>> Cc: linux-arch@xxxxxxxxxxxxxxx
>>>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>>>> Suggested-by: Catalin Marinas <catalin.marinas@xxxxxxx>
>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>>>> ---
>>>> Tested on arm64 and x86 platforms without any test failures. But this has
>>>> only been built tested on several other platforms. Individual tests need
>>>> to be verified on all current enabling platforms for the test i.e s390,
>>>> ppc32, arc etc.
>>>>
>>>> This patch must be applied on v5.6-rc3 after these patches
>>>>
>>>> 1. https://patchwork.kernel.org/patch/11385057/
>>>> 2. https://patchwork.kernel.org/patch/11407715/
>>>>
>>>> OR
>>>>
>>>> This patch must be applied on linux-next (next-20200227) after this patch
>>>>
>>>> 2. https://patchwork.kernel.org/patch/11407715/
>>>>
>>>> Â mm/debug_vm_pgtable.c | 310 +++++++++++++++++++++++++++++++++++++++++-
>>>> Â 1 file changed, 309 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>>> index 96dd7d574cef..3fb90d5b604e 100644
>>>> --- a/mm/debug_vm_pgtable.c
>>>> +++ b/mm/debug_vm_pgtable.c
>>>> @@ -41,6 +41,44 @@
>>>> ÂÂ * wrprotect(entry)ÂÂÂÂÂÂÂ = A write protected and not a write entry
>>>> ÂÂ * pxx_bad(entry)ÂÂÂÂÂÂÂ = A mapped and non-table entry
>>>> ÂÂ * pxx_same(entry1, entry2)ÂÂÂ = Both entries hold the exact same value
>>>> + *
>>>> + * Specific feature operations
>>>> + *
>>>> + * pte_mkspecial(entry)ÂÂÂÂÂÂÂ = Creates a special entry at PTE level
>>>> + * pte_special(entry)ÂÂÂÂÂÂÂ = Tests a special entry at PTE level
>>>> + *
>>>> + * pte_protnone(entry)ÂÂÂÂÂÂÂ = Tests a no access entry at PTE level
>>>> + * pmd_protnone(entry)ÂÂÂÂÂÂÂ = Tests a no access entry at PMD level
>>>> + *
>>>> + * pte_mkdevmap(entry)ÂÂÂÂÂÂÂ = Creates a device entry at PTE level
>>>> + * pmd_mkdevmap(entry)ÂÂÂÂÂÂÂ = Creates a device entry at PMD level
>>>> + * pud_mkdevmap(entry)ÂÂÂÂÂÂÂ = Creates a device entry at PUD level
>>>> + * pte_devmap(entry)ÂÂÂÂÂÂÂ = Tests a device entry at PTE level
>>>> + * pmd_devmap(entry)ÂÂÂÂÂÂÂ = Tests a device entry at PMD level
>>>> + * pud_devmap(entry)ÂÂÂÂÂÂÂ = Tests a device entry at PUD level
>>>> + *
>>>> + * pte_mksoft_dirty(entry)ÂÂÂ = Creates a soft dirty entry at PTE level
>>>> + * pmd_mksoft_dirty(entry)ÂÂÂ = Creates a soft dirty entry at PMD level
>>>> + * pte_swp_mksoft_dirty(entry)ÂÂÂ = Creates a soft dirty swap entry at PTE level
>>>> + * pmd_swp_mksoft_dirty(entry)ÂÂÂ = Creates a soft dirty swap entry at PMD level
>>>> + * pte_soft_dirty(entry)ÂÂÂ = Tests a soft dirty entry at PTE level
>>>> + * pmd_soft_dirty(entry)ÂÂÂ = Tests a soft dirty entry at PMD level
>>>> + * pte_swp_soft_dirty(entry)ÂÂÂ = Tests a soft dirty swap entry at PTE level
>>>> + * pmd_swp_soft_dirty(entry)ÂÂÂ = Tests a soft dirty swap entry at PMD level
>>>> + * pte_clear_soft_dirty(entry)ÂÂÂÂÂÂ = Clears a soft dirty entry at PTE level
>>>> + * pmd_clear_soft_dirty(entry)ÂÂÂÂÂÂ = Clears a soft dirty entry at PMD level
>>>> + * pte_swp_clear_soft_dirty(entry) = Clears a soft dirty swap entry at PTE level
>>>> + * pmd_swp_clear_soft_dirty(entry) = Clears a soft dirty swap entry at PMD level
>>>> + *
>>>> + * pte_mkhuge(entry)ÂÂÂÂÂÂÂ = Creates a HugeTLB entry at given level
>>>> + * pte_huge(entry)ÂÂÂÂÂÂÂ = Tests a HugeTLB entry at given level
>>>> + *
>>>> + * pmd_trans_huge(entry)ÂÂÂ = Tests a trans huge page at PMD level
>>>> + * pud_trans_huge(entry)ÂÂÂ = Tests a trans huge page at PUD level
>>>> + * pmd_present(entry)ÂÂÂÂÂÂÂ = Tests an entry points to memory at PMD level
>>>> + * pud_present(entry)ÂÂÂÂÂÂÂ = Tests an entry points to memory at PUD level
>>>> + * pmd_mknotpresent(entry)ÂÂÂ = Invalidates an PMD entry for MMU
>>>> + * pud_mknotpresent(entry)ÂÂÂ = Invalidates an PUD entry for MMU
>>>> ÂÂ */
>>>> Â #define VMFLAGSÂÂÂ (VM_READ|VM_WRITE|VM_EXEC)
>>>> Â @@ -287,6 +325,233 @@ static void __init pmd_populate_tests(struct mm_struct *mm, pmd_t *pmdp,
>>>> ÂÂÂÂÂ WARN_ON(pmd_bad(pmd));
>>>> Â }
>>>> Â +#ifdef CONFIG_ARCH_HAS_PTE_SPECIAL
>>>
>>> Can we avoid ifdefs unless necessary ?
>>>
>>> In mm/memory.c I see things like the following, it means pte_special() always exist and a #ifdef is not necessary.
>>
>> True, #ifdef here can be dropped here, done.
>>
>>>
>>> ÂÂÂÂif (IS_ENABLED(CONFIG_ARCH_HAS_PTE_SPECIAL)) {
>>> ÂÂÂÂÂÂÂ if (likely(!pte_special(pte)))
>>> ÂÂÂÂÂÂÂÂÂÂÂ goto check_pfn;
>>> ÂÂÂÂÂÂÂ if (vma->vm_ops && vma->vm_ops->find_special_page)
>>> ÂÂÂÂÂÂÂÂÂÂÂ return vma->vm_ops->find_special_page(vma, addr);
>>> ÂÂÂÂÂÂÂ if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
>>> ÂÂÂÂÂÂÂÂÂÂÂ return NULL;
>>> ÂÂÂÂÂÂÂ if (is_zero_pfn(pfn))
>>> ÂÂÂÂÂÂÂÂÂÂÂ return NULL;
>>> ÂÂÂÂÂÂÂ if (pte_devmap(pte))
>>> ÂÂÂÂÂÂÂÂÂÂÂ return NULL;
>>>
>>> ÂÂÂÂÂÂÂ print_bad_pte(vma, addr, pte, NULL);
>>> ÂÂÂÂÂÂÂ return NULL;
>>> ÂÂÂÂ}
>>>
>>>> +static void __init pte_special_tests(unsigned long pfn, pgprot_t prot)
>>>> +{
>>>> +ÂÂÂ pte_t pte = pfn_pte(pfn, prot);
>>>> +
>>>> +ÂÂÂ WARN_ON(!pte_special(pte_mkspecial(pte)));
>>>> +}
>>>> +#else
>>>> +static void __init pte_special_tests(unsigned long pfn, pgprot_t prot) { }
>>>> +#endif
>>>> +
>>>> +#ifdef CONFIG_NUMA_BALANCING
>>>
>>> Same here, this ifdef shouldn't be necessary because in /include/asm-generic/pgtable.h we have the following, so a if (IS_ENABLED()) should be enough.
>>>
>>> #ifndef CONFIG_NUMA_BALANCING
>>> /*
>>> Â* Technically a PTE can be PROTNONE even when not doing NUMA balancing but
>>> Â* the only case the kernel cares is for NUMA balancing and is only ever set
>>> Â* when the VMA is accessible. For PROT_NONE VMAs, the PTEs are not marked
>>> Â* _PAGE_PROTNONE so by by default, implement the helper as "always no". It
>>> Â* is the responsibility of the caller to distinguish between PROT_NONE
>>> Â* protections and NUMA hinting fault protections.
>>> Â*/
>>> static inline int pte_protnone(pte_t pte)
>>> {
>>> ÂÂÂÂreturn 0;
>>> }
>>>
>>> static inline int pmd_protnone(pmd_t pmd)
>>> {
>>> ÂÂÂÂreturn 0;
>>> }
>>> #endif /* CONFIG_NUMA_BALANCING */
>>
>> True, #ifdef here can be dropped, done. There is something I had missed
>> before, pfn_pmd() requires #ifdef CONFIG_TRANSPARENT_HUGEPAGE instead. We
>> need a pmd_t here with given prot. We cannot go via pfn_pte() followed by
>> pte_pmd(), as the later is platform specific and not available in general.
> 
> As many things require CONFIG_TRANSPARENT_HUGEPAGE, maybe it would be worth creating an additional C file with the related functions and build it conditionnaly to CONFIG_TRANSPARENT_HUGEPAGE
> 

Apologies for the delayed response here. Any split in the test will break it's
monolithic structure which is not desirable. Also lack of an explicit dependency
between HAVE_ARCH_TRANSPARENT_HUGEPAGE and HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
makes it difficult to group together fallback dummy stubs from various THP
related test functions here. I am planning to re-spin this patch sooner with
some more tests while also accommodating other previous comments. Hence, will
probably note down this aspect which can then be discussed further if required.

> Christophe