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

David Hildenbrand
2025-04-11T11:41:20Z

On 11.04.25 13:15, Baoquan He wrote:
> 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.

Did checkpatch.sh actually complain? You might be happy to learn that
the new limit is 100. :)

[...]

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

Whatever you prefer!

--
Cheers,

David / dhildenb