Re: [PATCH v4 04/13] mm/debug_vm_pgtables/hugevmap: Use the arch helper to identify huge vmap support.

["Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>]

On 9/2/20 6:10 PM, Christophe Leroy wrote:
> 
> 
> Le 02/09/2020 à 13:42, Aneesh Kumar K.V a écrit :
>> ppc64 supports huge vmap only with radix translation. Hence use arch 
>> helper
>> to determine the huge vmap support.
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>>   mm/debug_vm_pgtable.c | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>> index 00649b47f6e0..4c73e63b4ceb 100644
>> --- a/mm/debug_vm_pgtable.c
>> +++ b/mm/debug_vm_pgtable.c
>> @@ -28,6 +28,7 @@
>>   #include <linux/swapops.h>
>>   #include <linux/start_kernel.h>
>>   #include <linux/sched/mm.h>
>> +#include <linux/io.h>
>>   #include <asm/pgalloc.h>
>>   #include <asm/tlbflush.h>
>> @@ -206,11 +207,12 @@ static void __init pmd_leaf_tests(unsigned long 
>> pfn, pgprot_t prot)
>>       WARN_ON(!pmd_leaf(pmd));
>>   }
>> +#ifdef CONFIG_HAVE_ARCH_HUGE_VMAP
>>   static void __init pmd_huge_tests(pmd_t *pmdp, unsigned long pfn, 
>> pgprot_t prot)
>>   {
>>       pmd_t pmd;
>> -    if (!IS_ENABLED(CONFIG_HAVE_ARCH_HUGE_VMAP))
>> +    if (!arch_ioremap_pmd_supported())
> 
> What about moving ioremap_pmd_enabled() from mm/ioremap.c to some .h, 
> and using it ?
> As ioremap_pmd_enabled() is defined at all time, no need of #ifdef
> 

yes. This was discussed earlier too. IMHO we should do that outside this 
series. I guess figuring out ioremap_pmd/pud support can definitely be 
simplified. With a generic version like

#ifndef arch_ioremap_pmd_supported
static inline bool arch_ioremap_pmd_supported(void)
{
	return false;
}
#endif


>>           return;
>>       pr_debug("Validating PMD huge\n");
>> @@ -224,6 +226,9 @@ static void __init pmd_huge_tests(pmd_t *pmdp, 
>> unsigned long pfn, pgprot_t prot)
>>       pmd = READ_ONCE(*pmdp);
>>       WARN_ON(!pmd_none(pmd));
>>   }
>> +#else /* CONFIG_HAVE_ARCH_HUGE_VMAP */
>> +static void __init pmd_huge_tests(pmd_t *pmdp, unsigned long pfn, 
>> pgprot_t prot) { }
>> +#endif /* CONFIG_HAVE_ARCH_HUGE_VMAP */
>>   static void __init pmd_savedwrite_tests(unsigned long pfn, pgprot_t 
>> prot)
>>   {
>> @@ -320,11 +325,12 @@ static void __init pud_leaf_tests(unsigned long 
>> pfn, pgprot_t prot)
>>       WARN_ON(!pud_leaf(pud));
>>   }
>> +#ifdef CONFIG_HAVE_ARCH_HUGE_VMAP
>>   static void __init pud_huge_tests(pud_t *pudp, unsigned long pfn, 
>> pgprot_t prot)
>>   {
>>       pud_t pud;
>> -    if (!IS_ENABLED(CONFIG_HAVE_ARCH_HUGE_VMAP))
>> +    if (!arch_ioremap_pud_supported())
> 
> What about moving ioremap_pud_enabled() from mm/ioremap.c to some .h, 
> and using it ?
> As ioremap_pud_enabled() is defined at all time, no need of #ifdef
> 
>>           return;
>>       pr_debug("Validating PUD huge\n");
>> @@ -338,6 +344,10 @@ static void __init pud_huge_tests(pud_t *pudp, 
>> unsigned long pfn, pgprot_t prot)
>>       pud = READ_ONCE(*pudp);
>>       WARN_ON(!pud_none(pud));
>>   }
>> +#else /* !CONFIG_HAVE_ARCH_HUGE_VMAP */
>> +static void __init pud_huge_tests(pud_t *pudp, unsigned long pfn, 
>> pgprot_t prot) { }
>> +#endif /* !CONFIG_HAVE_ARCH_HUGE_VMAP */
>> +
>>   #else  /* !CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD */
>>   static void __init pud_basic_tests(unsigned long pfn, pgprot_t prot) 
>> { }
>>   static void __init pud_advanced_tests(struct mm_struct *mm,
>>
> 
> Christophe