[PATCH] mm/debug_vm_pgtable: Ensure THP availability via has_transparent_hugepage()

[PATCH] mm/debug_vm_pgtable: Ensure THP availability via has_transparent_hugepage()

[Anshuman Khandual]

On certain platforms, THP support could not just be validated via the build
option CONFIG_TRANSPARENT_HUGEPAGE. Instead has_transparent_hugepage() also
needs to be called upon to verify THP runtime support. Otherwise the debug
test might just run into unusable THP helpers like in the case of a 4K hash
config on powerpc platform [1]. This just moves all pfn_pmd() and pfn_pud()
after THP runtime validation with has_transparent_hugepage() which prevents
the mentioned problem.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=213069

Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
This applies on v5.13-rc2 after the following patches.

[1] https://lore.kernel.org/linux-mm/20210419071820.750217-1-liushixin2@huawei.com/
[2] https://lore.kernel.org/linux-mm/20210419071820.750217-2-liushixin2@huawei.com/

 mm/debug_vm_pgtable.c | 58 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 10 deletions(-)

diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
index e2f35db8ba69..6ff92c8b0a00 100644
--- a/mm/debug_vm_pgtable.c
+++ b/mm/debug_vm_pgtable.c
@@ -146,13 +146,14 @@ static void __init pte_savedwrite_tests(unsigned long pfn, pgprot_t prot)
 static void __init pmd_basic_tests(unsigned long pfn, int idx)
 {
 	pgprot_t prot = protection_map[idx];
-	pmd_t pmd = pfn_pmd(pfn, prot);
 	unsigned long val = idx, *ptr = &val;
+	pmd_t pmd;
 
 	if (!has_transparent_hugepage())
 		return;
 
 	pr_debug("Validating PMD basic (%pGv)\n", ptr);
+	pmd = pfn_pmd(pfn, prot);
 
 	/*
 	 * This test needs to be executed after the given page table entry
@@ -232,9 +233,14 @@ static void __init pmd_advanced_tests(struct mm_struct *mm,
 
 static void __init pmd_leaf_tests(unsigned long pfn, pgprot_t prot)
 {
-	pmd_t pmd = pfn_pmd(pfn, prot);
+	pmd_t pmd;
+
+	if (!has_transparent_hugepage())
+		return;
 
 	pr_debug("Validating PMD leaf\n");
+	pmd = pfn_pmd(pfn, prot);
+
 	/*
 	 * PMD based THP is a leaf entry.
 	 */
@@ -244,12 +250,16 @@ static void __init pmd_leaf_tests(unsigned long pfn, pgprot_t prot)
 
 static void __init pmd_savedwrite_tests(unsigned long pfn, pgprot_t prot)
 {
-	pmd_t pmd = pfn_pmd(pfn, prot);
+	pmd_t pmd;
 
 	if (!IS_ENABLED(CONFIG_NUMA_BALANCING))
 		return;
 
+	if (!has_transparent_hugepage())
+		return;
+
 	pr_debug("Validating PMD saved write\n");
+	pmd = pfn_pmd(pfn, prot);
 	WARN_ON(!pmd_savedwrite(pmd_mk_savedwrite(pmd_clear_savedwrite(pmd))));
 	WARN_ON(pmd_savedwrite(pmd_clear_savedwrite(pmd_mk_savedwrite(pmd))));
 }
@@ -258,13 +268,14 @@ static void __init pmd_savedwrite_tests(unsigned long pfn, pgprot_t prot)
 static void __init pud_basic_tests(struct mm_struct *mm, unsigned long pfn, int idx)
 {
 	pgprot_t prot = protection_map[idx];
-	pud_t pud = pfn_pud(pfn, prot);
 	unsigned long val = idx, *ptr = &val;
+	pud_t pud;
 
 	if (!has_transparent_hugepage())
 		return;
 
 	pr_debug("Validating PUD basic (%pGv)\n", ptr);
+	pud = pfn_pud(pfn, prot);
 
 	/*
 	 * This test needs to be executed after the given page table entry
@@ -348,9 +359,13 @@ static void __init pud_advanced_tests(struct mm_struct *mm,
 
 static void __init pud_leaf_tests(unsigned long pfn, pgprot_t prot)
 {
-	pud_t pud = pfn_pud(pfn, prot);
+	pud_t pud;
+
+	if (!has_transparent_hugepage())
+		return;
 
 	pr_debug("Validating PUD leaf\n");
+	pud = pfn_pud(pfn, prot);
 	/*
 	 * PUD based THP is a leaf entry.
 	 */
@@ -642,12 +657,16 @@ static void __init pte_protnone_tests(unsigned long pfn, pgprot_t prot)
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 static void __init pmd_protnone_tests(unsigned long pfn, pgprot_t prot)
 {
-	pmd_t pmd = pmd_mkhuge(pfn_pmd(pfn, prot));
+	pmd_t pmd;
 
 	if (!IS_ENABLED(CONFIG_NUMA_BALANCING))
 		return;
 
+	if (!has_transparent_hugepage())
+		return;
+
 	pr_debug("Validating PMD protnone\n");
+	pmd = pmd_mkhuge(pfn_pmd(pfn, prot));
 	WARN_ON(!pmd_protnone(pmd));
 	WARN_ON(!pmd_present(pmd));
 }
@@ -667,18 +686,26 @@ static void __init pte_devmap_tests(unsigned long pfn, pgprot_t prot)
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 static void __init pmd_devmap_tests(unsigned long pfn, pgprot_t prot)
 {
-	pmd_t pmd = pfn_pmd(pfn, prot);
+	pmd_t pmd;
+
+	if (!has_transparent_hugepage())
+		return;
 
 	pr_debug("Validating PMD devmap\n");
+	pmd = pfn_pmd(pfn, prot);
 	WARN_ON(!pmd_devmap(pmd_mkdevmap(pmd)));
 }
 
 #ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
 static void __init pud_devmap_tests(unsigned long pfn, pgprot_t prot)
 {
-	pud_t pud = pfn_pud(pfn, prot);
+	pud_t pud;
+
+	if (!has_transparent_hugepage())
+		return;
 
 	pr_debug("Validating PUD devmap\n");
+	pud = pfn_pud(pfn, prot);
 	WARN_ON(!pud_devmap(pud_mkdevmap(pud)));
 }
 #else  /* !CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD */
@@ -721,25 +748,33 @@ static void __init pte_swap_soft_dirty_tests(unsigned long pfn, pgprot_t prot)
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 static void __init pmd_soft_dirty_tests(unsigned long pfn, pgprot_t prot)
 {
-	pmd_t pmd = pfn_pmd(pfn, prot);
+	pmd_t pmd;
 
 	if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY))
 		return;
 
+	if (!has_transparent_hugepage())
+		return;
+
 	pr_debug("Validating PMD soft dirty\n");
+	pmd = pfn_pmd(pfn, prot);
 	WARN_ON(!pmd_soft_dirty(pmd_mksoft_dirty(pmd)));
 	WARN_ON(pmd_soft_dirty(pmd_clear_soft_dirty(pmd)));
 }
 
 static void __init pmd_swap_soft_dirty_tests(unsigned long pfn, pgprot_t prot)
 {
-	pmd_t pmd = pfn_pmd(pfn, prot);
+	pmd_t pmd;
 
 	if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY) ||
 		!IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION))
 		return;
 
+	if (!has_transparent_hugepage())
+		return;
+
 	pr_debug("Validating PMD swap soft dirty\n");
+	pmd = pfn_pmd(pfn, prot);
 	WARN_ON(!pmd_swp_soft_dirty(pmd_swp_mksoft_dirty(pmd)));
 	WARN_ON(pmd_swp_soft_dirty(pmd_swp_clear_soft_dirty(pmd)));
 }
@@ -768,6 +803,9 @@ static void __init pmd_swap_tests(unsigned long pfn, pgprot_t prot)
 	swp_entry_t swp;
 	pmd_t pmd;
 
+	if (!has_transparent_hugepage())
+		return;
+
 	pr_debug("Validating PMD swap\n");
 	pmd = pfn_pmd(pfn, prot);
 	swp = __pmd_to_swp_entry(pmd);
-- 
2.20.1

Re: [PATCH] mm/debug_vm_pgtable: Ensure THP availability via has_transparent_hugepage()

[Christophe Leroy]

Le 18/05/2021 à 10:13, Anshuman Khandual a écrit :
> On certain platforms, THP support could not just be validated via the build
> option CONFIG_TRANSPARENT_HUGEPAGE. Instead has_transparent_hugepage() also
> needs to be called upon to verify THP runtime support. Otherwise the debug
> test might just run into unusable THP helpers like in the case of a 4K hash

s/might/will/

> config on powerpc platform [1]. This just moves all pfn_pmd() and pfn_pud()
> after THP runtime validation with has_transparent_hugepage() which prevents
> the mentioned problem.
> 
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=213069
> 
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>

There should be a Fixes:  tag

> ---
> This applies on v5.13-rc2 after the following patches.
> 
> [1] https://lore.kernel.org/linux-mm/20210419071820.750217-1-liushixin2@huawei.com/
> [2] https://lore.kernel.org/linux-mm/20210419071820.750217-2-liushixin2@huawei.com/

I can't see any fixes: tag in those patches, and their subject line even targets them to -next. Are 
they meant to go to 5.13 and stable ?

If not, how do you coordinate between your patch that must go in 5.13 and in stable, and those two 
patches ? Shouldn't your patch go first and those other patches be rebased on top ?

> 
>   mm/debug_vm_pgtable.c | 58 +++++++++++++++++++++++++++++++++++--------
>   1 file changed, 48 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
> index e2f35db8ba69..6ff92c8b0a00 100644
> --- a/mm/debug_vm_pgtable.c
> +++ b/mm/debug_vm_pgtable.c
> @@ -146,13 +146,14 @@ static void __init pte_savedwrite_tests(unsigned long pfn, pgprot_t prot)
>   static void __init pmd_basic_tests(unsigned long pfn, int idx)
>   {
>   	pgprot_t prot = protection_map[idx];
> -	pmd_t pmd = pfn_pmd(pfn, prot);
>   	unsigned long val = idx, *ptr = &val;
> +	pmd_t pmd;
>   
>   	if (!has_transparent_hugepage())
>   		return;
>   
>   	pr_debug("Validating PMD basic (%pGv)\n", ptr);
> +	pmd = pfn_pmd(pfn, prot);
>   
>   	/*
>   	 * This test needs to be executed after the given page table entry
> @@ -232,9 +233,14 @@ static void __init pmd_advanced_tests(struct mm_struct *mm,
>   
>   static void __init pmd_leaf_tests(unsigned long pfn, pgprot_t prot)
>   {
> -	pmd_t pmd = pfn_pmd(pfn, prot);
> +	pmd_t pmd;
> +
> +	if (!has_transparent_hugepage())
> +		return;
>   
>   	pr_debug("Validating PMD leaf\n");
> +	pmd = pfn_pmd(pfn, prot);
> +
>   	/*
>   	 * PMD based THP is a leaf entry.
>   	 */
> @@ -244,12 +250,16 @@ static void __init pmd_leaf_tests(unsigned long pfn, pgprot_t prot)
>   
>   static void __init pmd_savedwrite_tests(unsigned long pfn, pgprot_t prot)
>   {
> -	pmd_t pmd = pfn_pmd(pfn, prot);
> +	pmd_t pmd;
>   
>   	if (!IS_ENABLED(CONFIG_NUMA_BALANCING))
>   		return;
>   
> +	if (!has_transparent_hugepage())
> +		return;
> +
>   	pr_debug("Validating PMD saved write\n");
> +	pmd = pfn_pmd(pfn, prot);
>   	WARN_ON(!pmd_savedwrite(pmd_mk_savedwrite(pmd_clear_savedwrite(pmd))));
>   	WARN_ON(pmd_savedwrite(pmd_clear_savedwrite(pmd_mk_savedwrite(pmd))));
>   }
> @@ -258,13 +268,14 @@ static void __init pmd_savedwrite_tests(unsigned long pfn, pgprot_t prot)
>   static void __init pud_basic_tests(struct mm_struct *mm, unsigned long pfn, int idx)
>   {
>   	pgprot_t prot = protection_map[idx];
> -	pud_t pud = pfn_pud(pfn, prot);
>   	unsigned long val = idx, *ptr = &val;
> +	pud_t pud;
>   
>   	if (!has_transparent_hugepage())
>   		return;
>   
>   	pr_debug("Validating PUD basic (%pGv)\n", ptr);
> +	pud = pfn_pud(pfn, prot);
>   
>   	/*
>   	 * This test needs to be executed after the given page table entry
> @@ -348,9 +359,13 @@ static void __init pud_advanced_tests(struct mm_struct *mm,
>   
>   static void __init pud_leaf_tests(unsigned long pfn, pgprot_t prot)
>   {
> -	pud_t pud = pfn_pud(pfn, prot);
> +	pud_t pud;
> +
> +	if (!has_transparent_hugepage())
> +		return;
>   
>   	pr_debug("Validating PUD leaf\n");
> +	pud = pfn_pud(pfn, prot);
>   	/*
>   	 * PUD based THP is a leaf entry.
>   	 */
> @@ -642,12 +657,16 @@ static void __init pte_protnone_tests(unsigned long pfn, pgprot_t prot)
>   #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>   static void __init pmd_protnone_tests(unsigned long pfn, pgprot_t prot)
>   {
> -	pmd_t pmd = pmd_mkhuge(pfn_pmd(pfn, prot));
> +	pmd_t pmd;
>   
>   	if (!IS_ENABLED(CONFIG_NUMA_BALANCING))
>   		return;
>   
> +	if (!has_transparent_hugepage())
> +		return;
> +
>   	pr_debug("Validating PMD protnone\n");
> +	pmd = pmd_mkhuge(pfn_pmd(pfn, prot));
>   	WARN_ON(!pmd_protnone(pmd));
>   	WARN_ON(!pmd_present(pmd));
>   }
> @@ -667,18 +686,26 @@ static void __init pte_devmap_tests(unsigned long pfn, pgprot_t prot)
>   #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>   static void __init pmd_devmap_tests(unsigned long pfn, pgprot_t prot)
>   {
> -	pmd_t pmd = pfn_pmd(pfn, prot);
> +	pmd_t pmd;
> +
> +	if (!has_transparent_hugepage())
> +		return;
>   
>   	pr_debug("Validating PMD devmap\n");
> +	pmd = pfn_pmd(pfn, prot);
>   	WARN_ON(!pmd_devmap(pmd_mkdevmap(pmd)));
>   }
>   
>   #ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
>   static void __init pud_devmap_tests(unsigned long pfn, pgprot_t prot)
>   {
> -	pud_t pud = pfn_pud(pfn, prot);
> +	pud_t pud;
> +
> +	if (!has_transparent_hugepage())
> +		return;
>   
>   	pr_debug("Validating PUD devmap\n");
> +	pud = pfn_pud(pfn, prot);
>   	WARN_ON(!pud_devmap(pud_mkdevmap(pud)));
>   }
>   #else  /* !CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD */
> @@ -721,25 +748,33 @@ static void __init pte_swap_soft_dirty_tests(unsigned long pfn, pgprot_t prot)
>   #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>   static void __init pmd_soft_dirty_tests(unsigned long pfn, pgprot_t prot)
>   {
> -	pmd_t pmd = pfn_pmd(pfn, prot);
> +	pmd_t pmd;
>   
>   	if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY))
>   		return;
>   
> +	if (!has_transparent_hugepage())
> +		return;
> +
>   	pr_debug("Validating PMD soft dirty\n");
> +	pmd = pfn_pmd(pfn, prot);
>   	WARN_ON(!pmd_soft_dirty(pmd_mksoft_dirty(pmd)));
>   	WARN_ON(pmd_soft_dirty(pmd_clear_soft_dirty(pmd)));
>   }
>   
>   static void __init pmd_swap_soft_dirty_tests(unsigned long pfn, pgprot_t prot)
>   {
> -	pmd_t pmd = pfn_pmd(pfn, prot);
> +	pmd_t pmd;
>   
>   	if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY) ||
>   		!IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION))
>   		return;
>   
> +	if (!has_transparent_hugepage())
> +		return;
> +
>   	pr_debug("Validating PMD swap soft dirty\n");
> +	pmd = pfn_pmd(pfn, prot);
>   	WARN_ON(!pmd_swp_soft_dirty(pmd_swp_mksoft_dirty(pmd)));
>   	WARN_ON(pmd_swp_soft_dirty(pmd_swp_clear_soft_dirty(pmd)));
>   }
> @@ -768,6 +803,9 @@ static void __init pmd_swap_tests(unsigned long pfn, pgprot_t prot)
>   	swp_entry_t swp;
>   	pmd_t pmd;
>   
> +	if (!has_transparent_hugepage())
> +		return;
> +
>   	pr_debug("Validating PMD swap\n");
>   	pmd = pfn_pmd(pfn, prot);
>   	swp = __pmd_to_swp_entry(pmd);
> 

Re: [PATCH] mm/debug_vm_pgtable: Ensure THP availability via has_transparent_hugepage()

[Anshuman Khandual]

On 5/18/21 2:20 PM, Christophe Leroy wrote:
> 
> 
> Le 18/05/2021 à 10:13, Anshuman Khandual a écrit :
>> On certain platforms, THP support could not just be validated via the build
>> option CONFIG_TRANSPARENT_HUGEPAGE. Instead has_transparent_hugepage() also
>> needs to be called upon to verify THP runtime support. Otherwise the debug
>> test might just run into unusable THP helpers like in the case of a 4K hash
> 
> s/might/will/

Sure, will replace.

> 
>> config on powerpc platform [1]. This just moves all pfn_pmd() and pfn_pud()
>> after THP runtime validation with has_transparent_hugepage() which prevents
>> the mentioned problem.
>>
>> [1] https://bugzilla.kernel.org/show_bug.cgi?id=213069
>>
>> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: linux-mm@kvack.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> 
> There should be a Fixes:  tag

Considering pmd_basic_tests() as the earliest test which is being
impacted here, this actually fixes an earlier fix which tried the
very same thing but was probably not complete. But it also applies
to portions of advanced tests which came later on as well, which
should have taken this problem into account.

Fixes: 787d563b8642 ("mm/debug_vm_pgtable: fix kernel crash by checking for THP support")

> 
>> ---
>> This applies on v5.13-rc2 after the following patches.
>>
>> [1] https://lore.kernel.org/linux-mm/20210419071820.750217-1-liushixin2@huawei.com/
>> [2] https://lore.kernel.org/linux-mm/20210419071820.750217-2-liushixin2@huawei.com/
> 
> I can't see any fixes: tag in those patches, and their subject line even targets them to -next. Are they meant to go to 5.13 and stable ?
> 
> If not, how do you coordinate between your patch that must go in 5.13 and in stable, and those two patches ? Shouldn't your patch go first and those other patches be rebased on top ?

Right, will rebase this patch on v5.13-rc2 directly without those two
patches. Hence this can be merged in v5.13 and backported to stable
if required.