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

David Hildenbrand
2025-04-11T08:54:14Z

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