Re: [PATCH V8] mm/debug: Add tests validating architecture page table helpers

Anshuman Khandual
2019-11-07T04:44:22Z

On 11/06/2019 11:37 PM, Vineet Gupta wrote:
> On 11/5/19 7:03 PM, Anshuman Khandual wrote:
>> But should not pfn_pmd() be encapsulated inside HAVE_ARCH_TRANSPARENT_HUGEPAGE
>> at the minimum (but I would say it should be available always, nonetheless) when
>> the platform subscribes to THP irrespective of whether THP is enabled or not.
>
> For ARC it was only introduced/needed when I added THP support so it is dependent
> in some way.
Right, it is dependent.

>
>> I could see in the file (arch/arc/include/asm/pgtable.h) that fetching pfn_pmd()
>> and all other basic PMD definitions is conditional on CONFIG_TRANSPARENT_HUGEPAGE.
>>
>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> #include <asm/hugepage.h>
>> #endif
>>
>> IIUC, CONFIG_TRANSPARENT_HUGEPAGE should only encapsulate PMD page table helpers
>> which are expected from generic THP code (pmd_trans_huge, pmdp_set_access_flags
>> etc) but not the basic PMD helpers like pmd_pfn, pmd_mkyoung, pmd_mkdirty,
>> pmd_mkclean etc.
>
> ARC only has 2 levels of paging, so these don't make any sense in general and
> needed only for THP case.
> I case of arch/arm you see it is only defined in pgtable-3level.h

There is no uniformity for all these across architectures. It has been bit
difficult to get some of these required helpers right (compile and run) on
different platforms.

>
>> Hence wondering will it be possible to accommodate following
>> code change on arc platform (not even compiled) in order to fix the problem ?
>
> I'm open to making changes in ARC code but lets do the right thing.
>
>> */
>> -#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> +#ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE
>> #include <asm/hugepage.h>
>> #endif
>
> This in wrong.  CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE is a just a glue toggle,
> used only in Kconfig files (and not in any "C" code).  It enables generic Kconfig
> code to allow visibility of CONFIG_TRANSPARENT_HUGEPAGE w/o every arch needing to
> do a me too.
>
> I think you need to use CONFIG_TRANSPARENT_HUGEPAGE to guard appropriate tests. I
> understand that it only

We can probably replace CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE wrapper with
CONFIG_TRANSPARENT_HUGEPAGE. But CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
explicitly depends on CONFIG_TRANSPARENT_HUGEPAGE as a prerequisite. Could
you please confirm if the following change on this test will work on ARC
platform for both THP and !THP cases ? Thank you.

diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
index 621ac09..99ebc7c 100644
--- a/mm/debug_vm_pgtable.c
+++ b/mm/debug_vm_pgtable.c
@@ -67,7 +67,7 @@ static void __init pte_basic_tests(unsigned long pfn, pgprot_t prot)
WARN_ON(pte_write(pte_wrprotect(pte)));
}

-#ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
static void __init pmd_basic_tests(unsigned long pfn, pgprot_t prot)
{
pmd_t pmd = pfn_pmd(pfn, prot);
@@ -85,9 +85,6 @@ static void __init pmd_basic_tests(unsigned long pfn, pgprot_t prot)
*/
WARN_ON(!pmd_bad(pmd_mkhuge(pmd)));
}
-#else
-static void __init pmd_basic_tests(unsigned long pfn, pgprot_t prot) { }
-#endif

#ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
static void __init pud_basic_tests(unsigned long pfn, pgprot_t prot)
@@ -112,6 +109,10 @@ static void __init pud_basic_tests(unsigned long pfn, pgprot_t prot)
#else
static void __init pud_basic_tests(unsigned long pfn, pgprot_t prot) { }
#endif
+#else
+static void __init pmd_basic_tests(unsigned long pfn, pgprot_t prot) { }
+static void __init pud_basic_tests(unsigned long pfn, pgprot_t prot) { }
+#endif

static void __init p4d_basic_tests(unsigned long pfn, pgprot_t prot)
{

> -Vineet
>