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

[Baoquan He <bhe@redhat.com>]

On 04/11/25 at 10:54am, David Hildenbrand wrote:
> 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.

Both is fine to me. It's to satisfy checkpatch.sh which complains about
exceeding 80 char in the line.

> 
> >   	if (unlikely(size == 0))
> >   		return 0;
> > +
> 
> Would not add that line to keep it like fault_in_readable() below.

Will remove it.

> 
> >   	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.

The current code is simpler. Your now saying may save the CPU execution
instructions a little bit. Both is fine to me.

I don't have strong preference, I can make v4 to address these concerns
if decided. Thanks for careful checking. 

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

Agreed.