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

[Vineet Gupta <Vineet.Gupta1@synopsys.com>]

On 11/7/19 8:27 PM, Anshuman Khandual wrote:
>
> 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 :)

Sorry my bad. I applied your manual hunk mindlessly and missed the nested #else.
So indeed it works. Although the stub for pud_basic_tests() is now defined twice
which makes it a bit ugly. But I'll leave that to you.

Thx,
-Vineet


_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc