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@xxxxxxxxxx> > > > > --- > > > > 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
