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

["zhen.ni" <zhen.ni@easystack.cn>]

在 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