| Message ID | 6-v1-dd94f8f0d5ad+716-gup_tidy_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Simplify the external interface for GUP | expand |
On 1/17/23 07: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(-) > ... Looks correct, just a few remaining comments that could be removed or fixed up: > @@ -1121,7 +1121,7 @@ static long __get_user_pages(struct mm_struct *mm, > i = follow_hugetlb_page(mm, vma, pages, vmas, > &start, &nr_pages, i, > gup_flags, locked); > - if (locked && *locked == 0) { > + if (!*locked) { Just above this function, there is a comment that can be adjusted to remove the reference to possibly NULL locked arg. This one: * If @locked != NULL, *@locked will be set to 0 when mmap_lock is * released by an up_read(). That can happen if @gup_flags does not * have FOLL_NOWAIT. There's another one above populate_vma_page_range(): * If @locked is NULL, it may be held for read or write and will * be unperturbed. * * If @locked is non-NULL, it must held for read only and may be * released. If it's released, *@locked will be set to 0. ...and another set, above faultin_vma_page_range(): * If @locked is NULL, it may be held for read or write and will be unperturbed. * * If @locked is non-NULL, it must held for read only and may be released. If * it's released, *@locked will be set to 0. thanks,
On Fri, Jan 20, 2023 at 11:19:58AM -0800, John Hubbard wrote: > On 1/17/23 07: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(-) > > > ... > > Looks correct, just a few remaining comments that could be > removed or fixed up: > > > @@ -1121,7 +1121,7 @@ static long __get_user_pages(struct mm_struct *mm, > > i = follow_hugetlb_page(mm, vma, pages, vmas, > > &start, &nr_pages, i, > > gup_flags, locked); > > - if (locked && *locked == 0) { > > + if (!*locked) { > > > Just above this function, there is a comment that can > be adjusted to remove the reference to possibly NULL locked > arg. This one: > > * If @locked != NULL, *@locked will be set to 0 when mmap_lock is > * released by an up_read(). That can happen if @gup_flags does not > * have FOLL_NOWAIT. OK > There's another one above populate_vma_page_range(): > > * If @locked is NULL, it may be held for read or write and will > * be unperturbed. > * > * If @locked is non-NULL, it must held for read only and may be > * released. If it's released, *@locked will be set to 0. > > ...and another set, above faultin_vma_page_range(): > > * If @locked is NULL, it may be held for read or write and will be unperturbed. > * > * If @locked is non-NULL, it must held for read only and may be released. If > * it's released, *@locked will be set to 0. ahh, I missed these two functions, they need to set FOLL_UNLOCK and de-null locked too. I didn't check things that were calling __get_user_pages() :\ Thanks, Jason
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?
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. It isalos much clearer to understand that a FOLL flag changes the behavior of how GUP works rather than some weirdo NULL argument. Jason
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.
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
diff --git a/mm/gup.c b/mm/gup.c index 4c360fb05cf3e4..1ccfff759a29eb 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -930,8 +930,8 @@ static int faultin_page(struct vm_area_struct *vma, * mmap lock in the page fault handler. Sanity check this. */ WARN_ON_ONCE(fault_flags & FAULT_FLAG_RETRY_NOWAIT); - if (locked) - *locked = 0; + *locked = 0; + /* * We should do the same as VM_FAULT_RETRY, but let's not * return -EBUSY since that's not reflecting the reality of @@ -951,7 +951,7 @@ static int faultin_page(struct vm_area_struct *vma, } if (ret & VM_FAULT_RETRY) { - if (locked && !(fault_flags & FAULT_FLAG_RETRY_NOWAIT)) + if (!(fault_flags & FAULT_FLAG_RETRY_NOWAIT)) *locked = 0; return -EBUSY; } @@ -1121,7 +1121,7 @@ static long __get_user_pages(struct mm_struct *mm, i = follow_hugetlb_page(mm, vma, pages, vmas, &start, &nr_pages, i, gup_flags, locked); - if (locked && *locked == 0) { + if (!*locked) { /* * We've got a VM_FAULT_RETRY * and we've lost mmap_lock. @@ -1345,7 +1345,7 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm, * The internal caller expects GUP to manage the lock internally and the * lock must be released when this returns. */ - if (locked && !*locked) { + if (!*locked) { if (mmap_read_lock_killable(mm)) return -EAGAIN; lock_dropped = true; @@ -1679,7 +1679,7 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start, * The internal caller expects GUP to manage the lock internally and the * lock must be released when this returns. */ - if (locked && !*locked) { + if (!*locked) { if (mmap_read_lock_killable(mm)) return -EAGAIN; must_unlock = true; @@ -2218,11 +2218,14 @@ long get_user_pages_remote(struct mm_struct *mm, unsigned int gup_flags, struct page **pages, struct vm_area_struct **vmas, int *locked) { + int local_locked = 1; + if (!is_valid_gup_args(pages, vmas, locked, &gup_flags, FOLL_TOUCH | FOLL_REMOTE)) return -EINVAL; - return __get_user_pages_locked(mm, start, nr_pages, pages, vmas, locked, + return __get_user_pages_locked(mm, start, nr_pages, pages, vmas, + locked ? locked : &local_locked, gup_flags); } EXPORT_SYMBOL(get_user_pages_remote); @@ -2257,11 +2260,13 @@ long get_user_pages(unsigned long start, unsigned long nr_pages, unsigned int gup_flags, struct page **pages, struct vm_area_struct **vmas) { + int locked = 1; + if (!is_valid_gup_args(pages, vmas, NULL, &gup_flags, FOLL_TOUCH)) return -EINVAL; return __get_user_pages_locked(current->mm, start, nr_pages, pages, - vmas, NULL, gup_flags); + vmas, &locked, gup_flags); } EXPORT_SYMBOL(get_user_pages); @@ -3154,10 +3159,13 @@ long pin_user_pages_remote(struct mm_struct *mm, unsigned int gup_flags, struct page **pages, struct vm_area_struct **vmas, int *locked) { + int local_locked = 1; + if (!is_valid_gup_args(pages, vmas, locked, &gup_flags, FOLL_PIN | FOLL_TOUCH | FOLL_REMOTE)) return 0; - return __gup_longterm_locked(mm, start, nr_pages, pages, vmas, locked, + return __gup_longterm_locked(mm, start, nr_pages, pages, vmas, + locked ? locked : &local_locked, gup_flags); } EXPORT_SYMBOL(pin_user_pages_remote); @@ -3183,10 +3191,12 @@ long pin_user_pages(unsigned long start, unsigned long nr_pages, unsigned int gup_flags, struct page **pages, struct vm_area_struct **vmas) { + int locked = 1; + if (!is_valid_gup_args(pages, vmas, NULL, &gup_flags, FOLL_PIN)) return 0; return __gup_longterm_locked(current->mm, start, nr_pages, - pages, vmas, NULL, gup_flags); + pages, vmas, &locked, gup_flags); } EXPORT_SYMBOL(pin_user_pages);
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(-)