Re: [PATCH v4 4/4] mm/gup: clean up codes in fault_in_xxx() functions

[David Hildenbrand <david@redhat.com>]

On 10.04.25 05:57, Baoquan He wrote:
> The code style in fault_in_readable() and fault_in_writable() is a
> little inconsistent with fault_in_safe_writeable(). In fault_in_readable()
> and fault_in_writable(), it uses 'uaddr' passed in as loop cursor. While
> in fault_in_safe_writeable(), local variable 'start' is used as loop
> cursor. This may mislead people when reading code or making change in
> these codes.
> 
> Here define explicit loop cursor and use for loop to simplify codes in
> these three functions. These cleanup can make them be consistent in
> code style and improve readability.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>   mm/gup.c | 65 +++++++++++++++++++++++---------------------------------
>   1 file changed, 26 insertions(+), 39 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 77a5bc622567..a76bd7e90a71 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2113,28 +2113,24 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
>    */
>   size_t fault_in_writeable(char __user *uaddr, size_t size)
>   {
> -	char __user *start = uaddr, *end;
> +	const unsigned long start = (unsigned long)uaddr;
> +	const unsigned long end = start + size;
> +	unsigned long cur = start;

I would initialize cur in the for loop header, makes the loop easier to 
read.

>   
>   	if (unlikely(size == 0))
>   		return 0;
> +

Would not add that line to keep it like fault_in_readable() below.

>   	if (!user_write_access_begin(uaddr, size))
>   		return size;
> -	if (!PAGE_ALIGNED(uaddr)) {
> -		unsafe_put_user(0, uaddr, out);
> -		uaddr = (char __user *)PAGE_ALIGN((unsigned long)uaddr);
> -	}
> -	end = (char __user *)PAGE_ALIGN((unsigned long)start + size);
> -	if (unlikely(end < start))
> -		end = NULL;
> -	while (uaddr != end) {
> -		unsafe_put_user(0, uaddr, out);
> -		uaddr += PAGE_SIZE;
> -	}
> +
> +	/* Stop once we overflow to 0. */
> +	for (; cur && cur < end; cur = PAGE_ALIGN_DOWN(cur + PAGE_SIZE))
> +		unsafe_put_user(0, (char __user *)cur, out);
>   

Staring at fault_in_safe_writeable(), we could also do

/* Stop once we overflow to 0. */
end = PAGE_ALIGN(end)
if (start < end)
	end = 0;

for (cur = start; cur != end; cur = PAGE_ALIGN_DOWN(cur + PAGE_SIZE))
	unsafe_put_user(0, (char __user *)cur, out);

Essentially, removing the "cur" check from the loop condition. Not sure 
if that is better.

In any case, if all functions later look similar and clearer it's a big win.

-- 
Cheers,

David / dhildenb