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@xxxxxxx>
>> Cc: Will Deacon <will@xxxxxxxxxx>
>> Cc: Ard Biesheuvel <ardb@xxxxxxxxxx>
>> Cc: Ryan Roberts <ryan.roberts@xxxxxxx>
>> Cc: Mark Rutland <mark.rutland@xxxxxxx>
>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>> ---
>>  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
>>