[PATCH] mmzone: remove redundant exception handler on pfn_valid()

[PATCH] mmzone: remove redundant exception handler on pfn_valid()

[Leesoo Ahn]

1). It can ensure the upper PAGE_SHIFT'ed bits of PHYS_PFN(..) are clear
by the C11 standard:

    6.5.7 Bitwise shift operator

    ...

    The result of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has
    an unsigned type or if E1 has a signed type and a nonnegative value,
    the value of the result is the integral part of the quotient of E1 / 2^E2.

@pfn parameter of pfn_valid(..) is unsigned long and PHYS_PFN(..) makes
the upper bits clear(zero) always with it. Otherwise, the result of E1 / 2^E2
doesn't make sense in the document.

2). x86 instructions

The following x86 instructions are the part of the redundant exception handler:

    ...
    movabs  $0xfffffffffffff,%rax
    and     %rdi,%rax
    cmp     %rax,%rdi
    je      22
    xor     %eax,%eax
    jmp     __x86_return_thunk
22: push    %rbp
    ...

%rdi would be the @pfn parameter of pfn_valid(..) and -1 assigned to %rax.
'je' always takes place by %rdi AND and CMP ops with %rax.

Therefore, the exception handler is unnecessary.

Signed-off-by: Leesoo Ahn <lsahn@ooseel.net>
---
 include/linux/mmzone.h | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 17506e4a2835..164ae80977d6 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -2028,15 +2028,6 @@ static inline int pfn_valid(unsigned long pfn)
 	struct mem_section *ms;
 	int ret;
 
-	/*
-	 * Ensure the upper PAGE_SHIFT bits are clear in the
-	 * pfn. Else it might lead to false positives when
-	 * some of the upper bits are set, but the lower bits
-	 * match a valid pfn.
-	 */
-	if (PHYS_PFN(PFN_PHYS(pfn)) != pfn)
-		return 0;
-
 	if (pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS)
 		return 0;
 	ms = __pfn_to_section(pfn);
-- 
2.46.0

Re: [PATCH] mmzone: remove redundant exception handler on pfn_valid()

[Matthew Wilcox]

On Thu, Sep 26, 2024 at 12:55:28AM +0900, Leesoo Ahn wrote:
> 1). It can ensure the upper PAGE_SHIFT'ed bits of PHYS_PFN(..) are clear
> by the C11 standard:

I don't think you understand the purpose of the code you're removing.

> +++ b/include/linux/mmzone.h
> @@ -2028,15 +2028,6 @@ static inline int pfn_valid(unsigned long pfn)
>  	struct mem_section *ms;
>  	int ret;
>  
> -	/*
> -	 * Ensure the upper PAGE_SHIFT bits are clear in the
> -	 * pfn. Else it might lead to false positives when
> -	 * some of the upper bits are set, but the lower bits
> -	 * match a valid pfn.
> -	 */
> -	if (PHYS_PFN(PFN_PHYS(pfn)) != pfn)
> -		return 0;

Let's assume that the pfn being asked about is 0x8000'0000'0000'1234.
The actually valid PFNs are only 0-0x1000'0000.  This function should
return false.  With your change, it will retrun true because it'll
only check the bottom 52 bits, and ignore that some of the top 12 bits
are set.