| Message ID | 5-v1-dd94f8f0d5ad+716-gup_tidy_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Simplify the external interface for GUP | expand |
On Tue, Jan 17, 2023 at 11:58:36AM -0400, Jason Gunthorpe wrote: > Setting FOLL_UNLOCK allows GUP to lock/unlock the mmap lock on its own. It > is a more explicit replacement for locked != NULL. This clears the way for > passing in locked = 1, without intending that the lock can be unlocked. > > Set the flag in all cases where it is used, eg locked is present in the > external interface or locked is used internally with locked = 0. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > include/linux/mm.h | 1 + > mm/gup.c | 31 +++++++++++++++++++------------ > 2 files changed, 20 insertions(+), 12 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index f3f196e4d66d6f..7496a5c8acede1 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -3089,6 +3089,7 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address, > #define FOLL_FAST_ONLY 0x80000 /* gup_fast: prevent fall-back to slow gup */ > #define FOLL_PCI_P2PDMA 0x100000 /* allow returning PCI P2PDMA pages */ > #define FOLL_INTERRUPTIBLE 0x200000 /* allow interrupts from generic signals */ > +#define FOLL_UNLOCK 0x400000 /* allow unlocking the mmap lock */ Please add a comment that this is an internal flag. > /* > * FOLL_PIN and FOLL_LONGTERM may be used in various combinations with each
On Thu, Jan 19, 2023 at 01:18:08PM +0200, Mike Rapoport wrote: > On Tue, Jan 17, 2023 at 11:58:36AM -0400, Jason Gunthorpe wrote: > > Setting FOLL_UNLOCK allows GUP to lock/unlock the mmap lock on its own. It > > is a more explicit replacement for locked != NULL. This clears the way for > > passing in locked = 1, without intending that the lock can be unlocked. > > > > Set the flag in all cases where it is used, eg locked is present in the > > external interface or locked is used internally with locked = 0. > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > --- > > include/linux/mm.h | 1 + > > mm/gup.c | 31 +++++++++++++++++++------------ > > 2 files changed, 20 insertions(+), 12 deletions(-) > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index f3f196e4d66d6f..7496a5c8acede1 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -3089,6 +3089,7 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address, > > #define FOLL_FAST_ONLY 0x80000 /* gup_fast: prevent fall-back to slow gup */ > > #define FOLL_PCI_P2PDMA 0x100000 /* allow returning PCI P2PDMA pages */ > > #define FOLL_INTERRUPTIBLE 0x200000 /* allow interrupts from generic signals */ > > +#define FOLL_UNLOCK 0x400000 /* allow unlocking the mmap lock */ > > Please add a comment that this is an internal flag. #define FOLL_UNLOCK 0x400000 /* allow unlocking the mmap lock (internal only) */ OK? Jason
On Fri, Jan 20, 2023 at 09:45:02AM -0400, Jason Gunthorpe wrote: > On Thu, Jan 19, 2023 at 01:18:08PM +0200, Mike Rapoport wrote: > > On Tue, Jan 17, 2023 at 11:58:36AM -0400, Jason Gunthorpe wrote: > > > Setting FOLL_UNLOCK allows GUP to lock/unlock the mmap lock on its own. It > > > is a more explicit replacement for locked != NULL. This clears the way for > > > passing in locked = 1, without intending that the lock can be unlocked. > > > > > > Set the flag in all cases where it is used, eg locked is present in the > > > external interface or locked is used internally with locked = 0. > > > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > > --- > > > include/linux/mm.h | 1 + > > > mm/gup.c | 31 +++++++++++++++++++------------ > > > 2 files changed, 20 insertions(+), 12 deletions(-) > > > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > > index f3f196e4d66d6f..7496a5c8acede1 100644 > > > --- a/include/linux/mm.h > > > +++ b/include/linux/mm.h > > > @@ -3089,6 +3089,7 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address, > > > #define FOLL_FAST_ONLY 0x80000 /* gup_fast: prevent fall-back to slow gup */ > > > #define FOLL_PCI_P2PDMA 0x100000 /* allow returning PCI P2PDMA pages */ > > > #define FOLL_INTERRUPTIBLE 0x200000 /* allow interrupts from generic signals */ > > > +#define FOLL_UNLOCK 0x400000 /* allow unlocking the mmap lock */ > > > > Please add a comment that this is an internal flag. > > #define FOLL_UNLOCK 0x400000 /* allow unlocking the mmap lock (internal only) */ > > OK? Yep. > Jason
On 1/17/23 07:58, Jason Gunthorpe wrote: > Setting FOLL_UNLOCK allows GUP to lock/unlock the mmap lock on its own. It > is a more explicit replacement for locked != NULL. This clears the way for > passing in locked = 1, without intending that the lock can be unlocked. > > Set the flag in all cases where it is used, eg locked is present in the > external interface or locked is used internally with locked = 0. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > include/linux/mm.h | 1 + > mm/gup.c | 31 +++++++++++++++++++------------ > 2 files changed, 20 insertions(+), 12 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index f3f196e4d66d6f..7496a5c8acede1 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -3089,6 +3089,7 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address, > #define FOLL_FAST_ONLY 0x80000 /* gup_fast: prevent fall-back to slow gup */ > #define FOLL_PCI_P2PDMA 0x100000 /* allow returning PCI P2PDMA pages */ > #define FOLL_INTERRUPTIBLE 0x200000 /* allow interrupts from generic signals */ > +#define FOLL_UNLOCK 0x400000 /* allow unlocking the mmap lock */ Looks good. Admin note: that this will conflict with commit b5054174ac7c ("mm: move FOLL_* defs to mm_types.h), which is in mm-stable. Because the FOLL_* items were moved. Reviewed-by: John Hubbard <jhubbard@nvidia.com> thanks,
On 17.01.23 16:58, Jason Gunthorpe wrote: > Setting FOLL_UNLOCK allows GUP to lock/unlock the mmap lock on its own. It > is a more explicit replacement for locked != NULL. This clears the way for > passing in locked = 1, without intending that the lock can be unlocked. > > Set the flag in all cases where it is used, eg locked is present in the > external interface or locked is used internally with locked = 0. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > include/linux/mm.h | 1 + > mm/gup.c | 31 +++++++++++++++++++------------ > 2 files changed, 20 insertions(+), 12 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index f3f196e4d66d6f..7496a5c8acede1 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -3089,6 +3089,7 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address, > #define FOLL_FAST_ONLY 0x80000 /* gup_fast: prevent fall-back to slow gup */ > #define FOLL_PCI_P2PDMA 0x100000 /* allow returning PCI P2PDMA pages */ > #define FOLL_INTERRUPTIBLE 0x200000 /* allow interrupts from generic signals */ > +#define FOLL_UNLOCK 0x400000 /* allow unlocking the mmap lock */ So it's more like "FOLL_UNLOCKABLE" ? FOLL_UNLOCK sounds like an instruction instead. Not that I particularly like adding new internal FOLL flags where avoidable :P
On Mon, Jan 23, 2023 at 12:37:17PM +0100, David Hildenbrand wrote: > On 17.01.23 16:58, Jason Gunthorpe wrote: > > Setting FOLL_UNLOCK allows GUP to lock/unlock the mmap lock on its own. It > > is a more explicit replacement for locked != NULL. This clears the way for > > passing in locked = 1, without intending that the lock can be unlocked. > > > > Set the flag in all cases where it is used, eg locked is present in the > > external interface or locked is used internally with locked = 0. > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > --- > > include/linux/mm.h | 1 + > > mm/gup.c | 31 +++++++++++++++++++------------ > > 2 files changed, 20 insertions(+), 12 deletions(-) > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index f3f196e4d66d6f..7496a5c8acede1 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -3089,6 +3089,7 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address, > > #define FOLL_FAST_ONLY 0x80000 /* gup_fast: prevent fall-back to slow gup */ > > #define FOLL_PCI_P2PDMA 0x100000 /* allow returning PCI P2PDMA pages */ > > #define FOLL_INTERRUPTIBLE 0x200000 /* allow interrupts from generic signals */ > > +#define FOLL_UNLOCK 0x400000 /* allow unlocking the mmap lock */ > > So it's more like "FOLL_UNLOCKABLE" ? FOLL_UNLOCK sounds like an instruction > instead. Sure, I renamed it > Not that I particularly like adding new internal FOLL flags where avoidable > :P How about, I made a patch to move all the internal FOLL_ flags into mm/internal.h ? Jason
On 1/23/23 09:58, Jason Gunthorpe wrote: ... > How about, I made a patch to move all the internal FOLL_ flags into > mm/internal.h ? Yes, after thinking through the internal and external APIs, and the FOLL_* situation, that sounds like the best overall approach right now, IMHO. I also considered whether adding some naming hints on top (suffixes such as _INTERNAL) might help, but so far, it looks like it would add more clutter than is justified. Instead, just looking at where the FOLL_* flag is defined seems like it could be enough. And there aren't *too* many flags, after all. thanks,
On 23.01.23 18:58, Jason Gunthorpe wrote: > On Mon, Jan 23, 2023 at 12:37:17PM +0100, David Hildenbrand wrote: >> On 17.01.23 16:58, Jason Gunthorpe wrote: >>> Setting FOLL_UNLOCK allows GUP to lock/unlock the mmap lock on its own. It >>> is a more explicit replacement for locked != NULL. This clears the way for >>> passing in locked = 1, without intending that the lock can be unlocked. >>> >>> Set the flag in all cases where it is used, eg locked is present in the >>> external interface or locked is used internally with locked = 0. >>> >>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> >>> --- >>> include/linux/mm.h | 1 + >>> mm/gup.c | 31 +++++++++++++++++++------------ >>> 2 files changed, 20 insertions(+), 12 deletions(-) >>> >>> diff --git a/include/linux/mm.h b/include/linux/mm.h >>> index f3f196e4d66d6f..7496a5c8acede1 100644 >>> --- a/include/linux/mm.h >>> +++ b/include/linux/mm.h >>> @@ -3089,6 +3089,7 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address, >>> #define FOLL_FAST_ONLY 0x80000 /* gup_fast: prevent fall-back to slow gup */ >>> #define FOLL_PCI_P2PDMA 0x100000 /* allow returning PCI P2PDMA pages */ >>> #define FOLL_INTERRUPTIBLE 0x200000 /* allow interrupts from generic signals */ >>> +#define FOLL_UNLOCK 0x400000 /* allow unlocking the mmap lock */ >> >> So it's more like "FOLL_UNLOCKABLE" ? FOLL_UNLOCK sounds like an instruction >> instead. > > Sure, I renamed it > >> Not that I particularly like adding new internal FOLL flags where avoidable >> :P > > How about, I made a patch to move all the internal FOLL_ flags into > mm/internal.h ? That would be a significant improvement. Maybe we could compress each set and move the internal ones to the highest bits.
diff --git a/include/linux/mm.h b/include/linux/mm.h index f3f196e4d66d6f..7496a5c8acede1 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -3089,6 +3089,7 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address, #define FOLL_FAST_ONLY 0x80000 /* gup_fast: prevent fall-back to slow gup */ #define FOLL_PCI_P2PDMA 0x100000 /* allow returning PCI P2PDMA pages */ #define FOLL_INTERRUPTIBLE 0x200000 /* allow interrupts from generic signals */ +#define FOLL_UNLOCK 0x400000 /* allow unlocking the mmap lock */ /* * FOLL_PIN and FOLL_LONGTERM may be used in various combinations with each diff --git a/mm/gup.c b/mm/gup.c index d203e268793b9c..4c360fb05cf3e4 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -896,7 +896,7 @@ static int faultin_page(struct vm_area_struct *vma, fault_flags |= FAULT_FLAG_WRITE; if (*flags & FOLL_REMOTE) fault_flags |= FAULT_FLAG_REMOTE; - if (locked) { + if (*flags & FOLL_UNLOCK) { fault_flags |= FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE; /* * FAULT_FLAG_INTERRUPTIBLE is opt-in. GUP callers must set @@ -1379,9 +1379,11 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm, for (;;) { ret = __get_user_pages(mm, start, nr_pages, flags, pages, vmas, locked); - if (!locked) + if (!(flags & FOLL_UNLOCK)) { /* VM_FAULT_RETRY couldn't trigger, bypass */ - return ret; + pages_done = ret; + break; + } /* VM_FAULT_RETRY or VM_FAULT_COMPLETED cannot return errors */ if (!*locked) { @@ -2106,12 +2108,20 @@ static bool is_valid_gup_args(struct page **pages, struct vm_area_struct **vmas, * interfaces: * - FOLL_PIN/FOLL_TRIED/FOLL_FAST_ONLY is internal only * - FOLL_REMOTE is internal only and used on follow_page() + * - FOLL_UNLOCK is internal only and used if locked is !NULL */ - if (WARN_ON_ONCE(gup_flags & (FOLL_PIN | FOLL_TRIED | + if (WARN_ON_ONCE(gup_flags & (FOLL_PIN | FOLL_TRIED | FOLL_UNLOCK | FOLL_REMOTE | FOLL_FAST_ONLY))) return false; gup_flags |= to_set; + if (locked) { + /* At the external interface locked must be set */ + if (WARN_ON_ONCE(*locked != 1)) + return false; + + gup_flags |= FOLL_UNLOCK; + } /* FOLL_GET and FOLL_PIN are mutually exclusive. */ if (WARN_ON_ONCE((gup_flags & (FOLL_PIN | FOLL_GET)) == @@ -2126,10 +2136,6 @@ static bool is_valid_gup_args(struct page **pages, struct vm_area_struct **vmas, if (WARN_ON_ONCE((gup_flags & (FOLL_GET | FOLL_PIN)) && !pages)) return false; - /* At the external interface locked must be set */ - if (WARN_ON_ONCE(locked && *locked != 1)) - return false; - /* We want to allow the pgmap to be hot-unplugged at all times */ if (WARN_ON_ONCE((gup_flags & FOLL_LONGTERM) && (gup_flags & FOLL_PCI_P2PDMA))) @@ -2139,7 +2145,7 @@ static bool is_valid_gup_args(struct page **pages, struct vm_area_struct **vmas, * Can't use VMAs with locked, as locked allows GUP to unlock * which invalidates the vmas array */ - if (WARN_ON_ONCE(vmas && locked)) + if (WARN_ON_ONCE(vmas && (gup_flags & FOLL_UNLOCK))) return false; *gup_flags_p = gup_flags; @@ -2279,7 +2285,8 @@ long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages, { int locked = 0; - if (!is_valid_gup_args(pages, NULL, NULL, &gup_flags, FOLL_TOUCH)) + if (!is_valid_gup_args(pages, NULL, NULL, &gup_flags, + FOLL_TOUCH | FOLL_UNLOCK)) return -EINVAL; return __get_user_pages_locked(current->mm, start, nr_pages, pages, @@ -2967,7 +2974,7 @@ static int internal_get_user_pages_fast(unsigned long start, pages += nr_pinned; ret = __gup_longterm_locked(current->mm, start, nr_pages - nr_pinned, pages, NULL, &locked, - gup_flags | FOLL_TOUCH); + gup_flags | FOLL_TOUCH | FOLL_UNLOCK); if (ret < 0) { /* * The caller has to unpin the pages we already pinned so @@ -3194,7 +3201,7 @@ long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages, int locked = 0; if (!is_valid_gup_args(pages, NULL, NULL, &gup_flags, - FOLL_PIN | FOLL_TOUCH)) + FOLL_PIN | FOLL_TOUCH | FOLL_UNLOCK)) return 0; return __gup_longterm_locked(current->mm, start, nr_pages, pages, NULL,
Setting FOLL_UNLOCK allows GUP to lock/unlock the mmap lock on its own. It is a more explicit replacement for locked != NULL. This clears the way for passing in locked = 1, without intending that the lock can be unlocked. Set the flag in all cases where it is used, eg locked is present in the external interface or locked is used internally with locked = 0. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- include/linux/mm.h | 1 + mm/gup.c | 31 +++++++++++++++++++------------ 2 files changed, 20 insertions(+), 12 deletions(-)