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

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Jan 23, 2023 at 01:59:55PM +0100, David Hildenbrand wrote:
> 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 ;)

Oh no doubt, just noting that it isn't just about line count.

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

Well it has become less self contained now. Locked is sprinkled
everywhere, and the main purpose of locked has now changed to
primarily be about managing the mmap_sem, ie we now lock it
automatically for *locked=0

So the special meaning of 'you can unlock while guppping' has been
really obscured. With FOLL_UNLOCKABLE it is clear, search on that and
you will find the few users and the one place that implements it.

Jason