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
