[PATCH 0/2] arm64/mm/hotplug: Drop some redundant WARN_ON()

[PATCH 0/2] arm64/mm/hotplug: Drop some redundant WARN_ON()

[Anshuman Khandual]

This series drops some redundant WARN_ON() tests which are not required any
more while unmapping and freeing up kernel page table pages during a memory
hot remove operation.

This series applies on v6.14-rc3

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Ryan Roberts <ryan.roberts@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org

Anshuman Khandual (2):
  arm64/mm/hotplug: Drop redundant [pgd|p4d]_present()
  arm64/mm/hotplug: Replace pxx_present() with pxx_valid()

 arch/arm64/mm/mmu.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

-- 
2.30.2

[PATCH 1/2] arm64/mm/hotplug: Drop redundant [pgd|p4d]_present()

[Anshuman Khandual]

[pgd|p4d]_present() are inverse to their corresponding [pgd|p4d]_none(). So
[pgd|p4d]_present() test right after corresponding [pgd|p4d]_none() inverse
test does not make sense. Hence just drop these redundant checks.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Ryan Roberts <ryan.roberts@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/mm/mmu.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index b4df5bc5b1b8..66906c45c7f6 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -952,7 +952,6 @@ static void unmap_hotplug_p4d_range(pgd_t *pgdp, unsigned long addr,
 		if (p4d_none(p4d))
 			continue;
 
-		WARN_ON(!p4d_present(p4d));
 		unmap_hotplug_pud_range(p4dp, addr, next, free_mapped, altmap);
 	} while (addr = next, addr < end);
 }
@@ -978,7 +977,6 @@ static void unmap_hotplug_range(unsigned long addr, unsigned long end,
 		if (pgd_none(pgd))
 			continue;
 
-		WARN_ON(!pgd_present(pgd));
 		unmap_hotplug_p4d_range(pgdp, addr, next, free_mapped, altmap);
 	} while (addr = next, addr < end);
 }
@@ -1114,7 +1112,6 @@ static void free_empty_p4d_table(pgd_t *pgdp, unsigned long addr,
 		if (p4d_none(p4d))
 			continue;
 
-		WARN_ON(!p4d_present(p4d));
 		free_empty_pud_table(p4dp, addr, next, floor, ceiling);
 	} while (addr = next, addr < end);
 
@@ -1153,7 +1150,6 @@ static void free_empty_tables(unsigned long addr, unsigned long end,
 		if (pgd_none(pgd))
 			continue;
 
-		WARN_ON(!pgd_present(pgd));
 		free_empty_p4d_table(pgdp, addr, next, floor, ceiling);
 	} while (addr = next, addr < end);
 }
-- 
2.30.2

[PATCH 2/2] arm64/mm/hotplug: Replace pxx_present() with pxx_valid()

[Anshuman Khandual]

pte_present() asserts either the entry has PTE_VALID or PTE_PRESENT_INVALID
bit set. Although PTE_PRESENT_INVALID bit only gets set on user space page
table entries to represent pxx_present_invalid() state. So present invalid
state is not really possible in kernel page table entries including linear
and vmemap mapping which get teared down during memory hot remove operation
. Hence just check for pxx_valid() instead of pxx_present() in all relevant
places.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Ryan Roberts <ryan.roberts@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/mm/mmu.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 66906c45c7f6..33a8b77b5e6b 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -863,7 +863,7 @@ static void unmap_hotplug_pte_range(pmd_t *pmdp, unsigned long addr,
 		if (pte_none(pte))
 			continue;
 
-		WARN_ON(!pte_present(pte));
+		WARN_ON(!pte_valid(pte));
 		__pte_clear(&init_mm, addr, ptep);
 		flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
 		if (free_mapped)
@@ -886,7 +886,7 @@ static void unmap_hotplug_pmd_range(pud_t *pudp, unsigned long addr,
 		if (pmd_none(pmd))
 			continue;
 
-		WARN_ON(!pmd_present(pmd));
+		WARN_ON(!pmd_valid(pmd));
 		if (pmd_sect(pmd)) {
 			pmd_clear(pmdp);
 
@@ -919,7 +919,7 @@ static void unmap_hotplug_pud_range(p4d_t *p4dp, unsigned long addr,
 		if (pud_none(pud))
 			continue;
 
-		WARN_ON(!pud_present(pud));
+		WARN_ON(!pud_valid(pud));
 		if (pud_sect(pud)) {
 			pud_clear(pudp);
 
@@ -1032,7 +1032,7 @@ static void free_empty_pmd_table(pud_t *pudp, unsigned long addr,
 		if (pmd_none(pmd))
 			continue;
 
-		WARN_ON(!pmd_present(pmd) || !pmd_table(pmd) || pmd_sect(pmd));
+		WARN_ON(!pmd_valid(pmd) || !pmd_table(pmd) || pmd_sect(pmd));
 		free_empty_pte_table(pmdp, addr, next, floor, ceiling);
 	} while (addr = next, addr < end);
 
@@ -1072,7 +1072,7 @@ static void free_empty_pud_table(p4d_t *p4dp, unsigned long addr,
 		if (pud_none(pud))
 			continue;
 
-		WARN_ON(!pud_present(pud) || !pud_table(pud) || pud_sect(pud));
+		WARN_ON(!pud_valid(pud) || !pud_table(pud) || pud_sect(pud));
 		free_empty_pmd_table(pudp, addr, next, floor, ceiling);
 	} while (addr = next, addr < end);
 
-- 
2.30.2

Re: [PATCH 1/2] arm64/mm/hotplug: Drop redundant [pgd|p4d]_present()

[Dev Jain]

On 21/02/25 3:14 pm, Anshuman Khandual wrote:
> [pgd|p4d]_present() are inverse to their corresponding [pgd|p4d]_none(). So
> [pgd|p4d]_present() test right after corresponding [pgd|p4d]_none() inverse
> test does not make sense. Hence just drop these redundant checks.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Ryan Roberts <ryan.roberts@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>

LGTM

Reviewed-by: Dev Jain <dev.jain@arm.com>

Re: [PATCH 0/2] arm64/mm/hotplug: Drop some redundant WARN_ON()

[Anshuman Khandual]

On 2/21/25 15:14, Anshuman Khandual wrote:
> This series drops some redundant WARN_ON() tests which are not required any
> more while unmapping and freeing up kernel page table pages during a memory
> hot remove operation.
> 
> This series applies on v6.14-rc3
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Ryan Roberts <ryan.roberts@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> 
> Anshuman Khandual (2):
>   arm64/mm/hotplug: Drop redundant [pgd|p4d]_present()
>   arm64/mm/hotplug: Replace pxx_present() with pxx_valid()
> 
>  arch/arm64/mm/mmu.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 

Gentle ping, any updates on this series ? This simplifies WARN_ON()
checks during memory hotplug operation, after subsequent changes to
page table helpers such as pxd_present() etc.

Re: [PATCH 1/2] arm64/mm/hotplug: Drop redundant [pgd|p4d]_present()

[Mark Rutland]

On Fri, Feb 21, 2025 at 03:14:48PM +0530, Anshuman Khandual wrote:
> [pgd|p4d]_present() are inverse to their corresponding [pgd|p4d]_none().

Maybe their implementations happen to be inverse today, but the semantic
of pXX_present() is not the inverse of the semantic of pXX_none(). In
general, !pXX_none() does not imply pXX_present().

> So [pgd|p4d]_present() test right after corresponding [pgd|p4d]_none()
> inverse test does not make sense. Hence just drop these redundant
> checks.

I think the checks make sense in abstract, even if they're redundant
today

Is there any reason to remove these specific case?

Surely the compiler optimizes these out when they're redundant?

Mark.

> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Ryan Roberts <ryan.roberts@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/arm64/mm/mmu.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index b4df5bc5b1b8..66906c45c7f6 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -952,7 +952,6 @@ static void unmap_hotplug_p4d_range(pgd_t *pgdp, unsigned long addr,
>  		if (p4d_none(p4d))
>  			continue;
>  
> -		WARN_ON(!p4d_present(p4d));
>  		unmap_hotplug_pud_range(p4dp, addr, next, free_mapped, altmap);
>  	} while (addr = next, addr < end);
>  }
> @@ -978,7 +977,6 @@ static void unmap_hotplug_range(unsigned long addr, unsigned long end,
>  		if (pgd_none(pgd))
>  			continue;
>  
> -		WARN_ON(!pgd_present(pgd));
>  		unmap_hotplug_p4d_range(pgdp, addr, next, free_mapped, altmap);
>  	} while (addr = next, addr < end);
>  }
> @@ -1114,7 +1112,6 @@ static void free_empty_p4d_table(pgd_t *pgdp, unsigned long addr,
>  		if (p4d_none(p4d))
>  			continue;
>  
> -		WARN_ON(!p4d_present(p4d));
>  		free_empty_pud_table(p4dp, addr, next, floor, ceiling);
>  	} while (addr = next, addr < end);
>  
> @@ -1153,7 +1150,6 @@ static void free_empty_tables(unsigned long addr, unsigned long end,
>  		if (pgd_none(pgd))
>  			continue;
>  
> -		WARN_ON(!pgd_present(pgd));
>  		free_empty_p4d_table(pgdp, addr, next, floor, ceiling);
>  	} while (addr = next, addr < end);
>  }
> -- 
> 2.30.2
> 

Re: [PATCH 1/2] arm64/mm/hotplug: Drop redundant [pgd|p4d]_present()

[Anshuman Khandual]

On 3/18/25 16:46, Mark Rutland wrote:
> On Fri, Feb 21, 2025 at 03:14:48PM +0530, Anshuman Khandual wrote:
>> [pgd|p4d]_present() are inverse to their corresponding [pgd|p4d]_none().
> 
> Maybe their implementations happen to be inverse today, but the semantic
> of pXX_present() is not the inverse of the semantic of pXX_none(). In
> general, !pXX_none() does not imply pXX_present().

Agreed.

> 
>> So [pgd|p4d]_present() test right after corresponding [pgd|p4d]_none()
>> inverse test does not make sense. Hence just drop these redundant
>> checks.
> 
> I think the checks make sense in abstract, even if they're redundant
> today

Okay.

> 
> Is there any reason to remove these specific case?

Just for optimization.

> 
> Surely the compiler optimizes these out when they're redundant?

Agreed. Although wondering if p4d_present() is the right thing to test.
Should it be replaced with a p4d_bad() check instead because subsequent
calls into either unmap_hotplug_pud_range() or free_empty_pud_table() 
requires given entry to be a table to proceed further successfully.

> 
> Mark.
> 
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Ard Biesheuvel <ardb@kernel.org>
>> Cc: Ryan Roberts <ryan.roberts@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  arch/arm64/mm/mmu.c | 4 ----
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index b4df5bc5b1b8..66906c45c7f6 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -952,7 +952,6 @@ static void unmap_hotplug_p4d_range(pgd_t *pgdp, unsigned long addr,
>>  		if (p4d_none(p4d))
>>  			continue;
>>  
>> -		WARN_ON(!p4d_present(p4d));
>>  		unmap_hotplug_pud_range(p4dp, addr, next, free_mapped, altmap);
>>  	} while (addr = next, addr < end);
>>  }
>> @@ -978,7 +977,6 @@ static void unmap_hotplug_range(unsigned long addr, unsigned long end,
>>  		if (pgd_none(pgd))
>>  			continue;
>>  
>> -		WARN_ON(!pgd_present(pgd));
>>  		unmap_hotplug_p4d_range(pgdp, addr, next, free_mapped, altmap);
>>  	} while (addr = next, addr < end);
>>  }
>> @@ -1114,7 +1112,6 @@ static void free_empty_p4d_table(pgd_t *pgdp, unsigned long addr,
>>  		if (p4d_none(p4d))
>>  			continue;
>>  
>> -		WARN_ON(!p4d_present(p4d));
>>  		free_empty_pud_table(p4dp, addr, next, floor, ceiling);
>>  	} while (addr = next, addr < end);
>>  
>> @@ -1153,7 +1150,6 @@ static void free_empty_tables(unsigned long addr, unsigned long end,
>>  		if (pgd_none(pgd))
>>  			continue;
>>  
>> -		WARN_ON(!pgd_present(pgd));
>>  		free_empty_p4d_table(pgdp, addr, next, floor, ceiling);
>>  	} while (addr = next, addr < end);
>>  }
>> -- 
>> 2.30.2
>>