Re: [PATCH 6/8] mm/gup: make locked never NULL in the internal GUP functions

[David Hildenbrand <david@redhat.com>]

On 23.01.23 13:44, Jason Gunthorpe wrote:
> On Mon, Jan 23, 2023 at 12:35:28PM +0100, David Hildenbrand wrote:
>> On 17.01.23 16:58, Jason Gunthorpe wrote:
>>> Now that NULL locked doesn't have a special meaning we can just make it
>>> non-NULL in all cases and remove the special tests.
>>>
>>> get_user_pages() and pin_user_pages() can safely pass in a locked = 1
>>>
>>> get_user_pages_remote) and pin_user_pages_remote() can swap in a local
>>> variable for locked if NULL is passed.
>>>
>>> Remove all the NULL checks.
>>>
>>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>>> ---
>>>    mm/gup.c | 30 ++++++++++++++++++++----------
>>>    1 file changed, 20 insertions(+), 10 deletions(-)
>>
>> ... doesn't really look like a real cleanup. Especially with the
>>
>>   2 files changed, 20 insertions(+), 12 deletions(-)
>>
>> on the previous patch and a new internal flag ....
>>
>> What's the benefit?
> 
> There are all kinds of unnecessary branches on the faster paths,
> inside loops, etc to check for NULL when all we really needed was a
> single bit flag.

... call me skeptical that this optimization matters in practice ;)

> 
> It isalos much clearer to understand that a FOLL flag changes the
> behavior of how GUP works rather than some weirdo NULL argument.

The whole "locked" parameter handling is weird. I don't think adding a 
new FOLL_ internal flag on top particularly improves the situation ... 
at least before, the logic around that "locked" parameter handling was 
self-contained. Self-contained weirdness.

-- 
Thanks,

David / dhildenb