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

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 11/08/2019 12:35 AM, Vineet Gupta wrote:
> On 11/6/19 8:44 PM, Anshuman Khandual wrote:
>>
>>>
>>>>   */
>>>> -#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
> 
> Fails to build for THP case since
> 
> CONFIG_TRANSPARENT_HUGEPAGE=y
> CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD=n
> 
> ../mm/debug_vm_pgtable.c:112:20: error: redefinition of ‘pmd_basic_tests’
> 

Hmm, really ? With arm64 defconfig we have the same default combination
where it builds.

CONFIG_TRANSPARENT_HUGEPAGE=y
CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE=y
CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD=n	/* It should not even appear */

With the above change, we have now

#ifdef CONFIG_TRANSPARENT_HUGEPAGE
static void __init pmd_basic_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)
{
----
----
}
#else /* !CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD */
static void __init pud_basic_tests(unsigned long pfn, pgprot_t prot) { }
#endif
#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) { }
#endif

When !CONFIG_TRANSPARENT_HUGEPAGE

- Dummy definitions for pmd_basic_tests() and pud_basic_tests()

When CONFIG_TRANSPARENT_HUGEPAGE and !CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD

- Actual pmd_basic_tests() and dummy pud_basic_tests()

When CONFIG_TRANSPARENT_HUGEPAGE and CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD

- Actual pmd_basic_tests() and pud_basic_tests()

Tested this on arm64 which does not have CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
for THP and !THP and on x86 which has CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
for THP and !THP which basically covered all combination for these configs.

Is there something I am still missing in plain sight :)

- Anshuman

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel