[PATCH] mm: Refactor vm_mixed_ok() for more maintainable

[PATCH] mm: Refactor vm_mixed_ok() for more maintainable

[Zhen Ni]

Restructure the function without altering its logic:
- Check the VM_MIXEDMAP flag first as a fast path.
- Consolidate the zero page handling logic into a single, dedicated
block.

These changes improve code organization and maintainability.

Signed-off-by: Zhen Ni <zhen.ni@easystack.cn>
---
 mm/memory.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 74b45e258323..4f882c58dbc2 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2706,14 +2706,17 @@ EXPORT_SYMBOL(vmf_insert_pfn);
 static bool vm_mixed_ok(struct vm_area_struct *vma, unsigned long pfn,
 			bool mkwrite)
 {
-	if (unlikely(is_zero_pfn(pfn)) &&
-	    (mkwrite || !vm_mixed_zeropage_allowed(vma)))
-		return false;
 	/* these checks mirror the abort conditions in vm_normal_page */
 	if (vma->vm_flags & VM_MIXEDMAP)
 		return true;
-	if (is_zero_pfn(pfn))
+	if (unlikely(is_zero_pfn(pfn))) {
+		/* Zero pages can only be mapped read-only and if VMA
+		 * allows them.
+		 */
+		if (mkwrite || !vm_mixed_zeropage_allowed(vma))
+			return false;
 		return true;
+	}
 	return false;
 }
 
-- 
2.20.1

Re: [PATCH] mm: Refactor vm_mixed_ok() for more maintainable

[Lorenzo Stoakes]

On Tue, Oct 21, 2025 at 04:53:28PM +0800, Zhen Ni wrote:
> Restructure the function without altering its logic:
> - Check the VM_MIXEDMAP flag first as a fast path.
> - Consolidate the zero page handling logic into a single, dedicated
> block.
>
> These changes improve code organization and maintainability.
>
> Signed-off-by: Zhen Ni <zhen.ni@easystack.cn>

The below is just incorrect and you're making the function _harder_ to read
not easier.

> ---
>  mm/memory.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 74b45e258323..4f882c58dbc2 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -2706,14 +2706,17 @@ EXPORT_SYMBOL(vmf_insert_pfn);
>  static bool vm_mixed_ok(struct vm_area_struct *vma, unsigned long pfn,
>  			bool mkwrite)
>  {
> -	if (unlikely(is_zero_pfn(pfn)) &&
> -	    (mkwrite || !vm_mixed_zeropage_allowed(vma)))
> -		return false;
>  	/* these checks mirror the abort conditions in vm_normal_page */
>  	if (vma->vm_flags & VM_MIXEDMAP)
>  		return true;

You're changing the logic now? This is incorrect? If the conditions you removed
are true but also vma->vm_flags & VM_MIXEDMAP is true you now return true rather
than false?

> -	if (is_zero_pfn(pfn))
> +	if (unlikely(is_zero_pfn(pfn))) {
> +		/* Zero pages can only be mapped read-only and if VMA
> +		 * allows them.
> +		 */

We don't start comments with /* xxxx, we do:

/*
 * Zero pages ...
 */

> +		if (mkwrite || !vm_mixed_zeropage_allowed(vma))
> +			return false;

Yeah this is just wrong. Plus you're now inserting an additional level of
indentation?

>  		return true;
> +	}
>  	return false;
>  }
>
> --
> 2.20.1
>
>

Re: [PATCH] mm: Refactor vm_mixed_ok() for more maintainable

[zhen.ni]

在 2025/10/21 17:14, Lorenzo Stoakes 写道:
> On Tue, Oct 21, 2025 at 04:53:28PM +0800, Zhen Ni wrote:
>> Restructure the function without altering its logic:
>> - Check the VM_MIXEDMAP flag first as a fast path.
>> - Consolidate the zero page handling logic into a single, dedicated
>> block.
>>
>> These changes improve code organization and maintainability.
>>
>> Signed-off-by: Zhen Ni <zhen.ni@easystack.cn>
> 
> The below is just incorrect and you're making the function _harder_ to read
> not easier.
> 
>> ---
>>   mm/memory.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
> 
> You're changing the logic now? This is incorrect? If the conditions you removed
> are true but also vma->vm_flags & VM_MIXEDMAP is true you now return true rather
> than false?
> 
>> -	if (is_zero_pfn(pfn))
>> +	if (unlikely(is_zero_pfn(pfn))) {
>> +		/* Zero pages can only be mapped read-only and if VMA
>> +		 * allows them.
>> +		 */
> 
> We don't start comments with /* xxxx, we do:
> 
> /*
>   * Zero pages ...
>   */
> 
>> +		if (mkwrite || !vm_mixed_zeropage_allowed(vma))
>> +			return false;
> 
> Yeah this is just wrong. Plus you're now inserting an additional level of
> indentation?
> 
>>   		return true;
>> +	}
>>   	return false;
>>   }
>>
>> --
>> 2.20.1
>>
>>
> 
Hi,

Thank you for the thorough review and for pointing out the critical 
logicerror I introduced.

You are absolutely right - my refactoring incorrectly changed the 
function behavior when both VM_MIXEDMAP is set and we have a zero page 
with mkwrite or mixed mapping restrictions. I apologize for this oversight.

I will drop this patch since:
1. The refactoring introduced a functional regression
2. As you noted, the original code is actually more readable
3. The change doesn't provide meaningful maintainability improvement

Best regards,
Zhen