| Message ID | 20200201034029.4063170-3-jhubbard@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mm/gup: track FOLL_PIN pages | expand |
On Fri, Jan 31, 2020 at 07:40:19PM -0800, John Hubbard wrote: > An upcoming patch requires reusing the implementation of > get_user_pages_remote(). Split up get_user_pages_remote() into an outer > routine that checks flags, and an implementation routine that will be > reused. This makes subsequent changes much easier to understand. > > There should be no change in behavior due to this patch. > > Signed-off-by: John Hubbard <jhubbard@nvidia.com> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
On Fri 31-01-20 19:40:19, John Hubbard wrote: > An upcoming patch requires reusing the implementation of > get_user_pages_remote(). Split up get_user_pages_remote() into an outer > routine that checks flags, and an implementation routine that will be > reused. This makes subsequent changes much easier to understand. > > There should be no change in behavior due to this patch. > > Signed-off-by: John Hubbard <jhubbard@nvidia.com> Looks good to me. You can add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > mm/gup.c | 56 +++++++++++++++++++++++++++++++++----------------------- > 1 file changed, 33 insertions(+), 23 deletions(-) > > diff --git a/mm/gup.c b/mm/gup.c > index e13f4d211475..228aa7059018 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -1557,6 +1557,37 @@ static __always_inline long __gup_longterm_locked(struct task_struct *tsk, > } > #endif /* CONFIG_FS_DAX || CONFIG_CMA */ > > +#ifdef CONFIG_MMU > +static long __get_user_pages_remote(struct task_struct *tsk, > + struct mm_struct *mm, > + unsigned long start, unsigned long nr_pages, > + unsigned int gup_flags, struct page **pages, > + struct vm_area_struct **vmas, int *locked) > +{ > + /* > + * Parts of FOLL_LONGTERM behavior are incompatible with > + * FAULT_FLAG_ALLOW_RETRY because of the FS DAX check requirement on > + * vmas. However, this only comes up if locked is set, and there are > + * callers that do request FOLL_LONGTERM, but do not set locked. So, > + * allow what we can. > + */ > + if (gup_flags & FOLL_LONGTERM) { > + if (WARN_ON_ONCE(locked)) > + return -EINVAL; > + /* > + * This will check the vmas (even if our vmas arg is NULL) > + * and return -ENOTSUPP if DAX isn't allowed in this case: > + */ > + return __gup_longterm_locked(tsk, mm, start, nr_pages, pages, > + vmas, gup_flags | FOLL_TOUCH | > + FOLL_REMOTE); > + } > + > + return __get_user_pages_locked(tsk, mm, start, nr_pages, pages, vmas, > + locked, > + gup_flags | FOLL_TOUCH | FOLL_REMOTE); > +} > + > /* > * get_user_pages_remote() - pin user pages in memory > * @tsk: the task_struct to use for page fault accounting, or > @@ -1619,7 +1650,6 @@ static __always_inline long __gup_longterm_locked(struct task_struct *tsk, > * should use get_user_pages because it cannot pass > * FAULT_FLAG_ALLOW_RETRY to handle_mm_fault. > */ > -#ifdef CONFIG_MMU > long get_user_pages_remote(struct task_struct *tsk, struct mm_struct *mm, > unsigned long start, unsigned long nr_pages, > unsigned int gup_flags, struct page **pages, > @@ -1632,28 +1662,8 @@ long get_user_pages_remote(struct task_struct *tsk, struct mm_struct *mm, > if (WARN_ON_ONCE(gup_flags & FOLL_PIN)) > return -EINVAL; > > - /* > - * Parts of FOLL_LONGTERM behavior are incompatible with > - * FAULT_FLAG_ALLOW_RETRY because of the FS DAX check requirement on > - * vmas. However, this only comes up if locked is set, and there are > - * callers that do request FOLL_LONGTERM, but do not set locked. So, > - * allow what we can. > - */ > - if (gup_flags & FOLL_LONGTERM) { > - if (WARN_ON_ONCE(locked)) > - return -EINVAL; > - /* > - * This will check the vmas (even if our vmas arg is NULL) > - * and return -ENOTSUPP if DAX isn't allowed in this case: > - */ > - return __gup_longterm_locked(tsk, mm, start, nr_pages, pages, > - vmas, gup_flags | FOLL_TOUCH | > - FOLL_REMOTE); > - } > - > - return __get_user_pages_locked(tsk, mm, start, nr_pages, pages, vmas, > - locked, > - gup_flags | FOLL_TOUCH | FOLL_REMOTE); > + return __get_user_pages_remote(tsk, mm, start, nr_pages, gup_flags, > + pages, vmas, locked); > } > EXPORT_SYMBOL(get_user_pages_remote); > > -- > 2.25.0 >
On 2/3/20 6:20 AM, Jan Kara wrote: > On Fri 31-01-20 19:40:19, John Hubbard wrote: >> An upcoming patch requires reusing the implementation of >> get_user_pages_remote(). Split up get_user_pages_remote() into an outer >> routine that checks flags, and an implementation routine that will be >> reused. This makes subsequent changes much easier to understand. >> >> There should be no change in behavior due to this patch. >> >> Signed-off-by: John Hubbard <jhubbard@nvidia.com> > > Looks good to me. You can add: > > Reviewed-by: Jan Kara <jack@suse.cz> > > Honza Thanks for reviewing this series (sort of again), Jan! thanks,
diff --git a/mm/gup.c b/mm/gup.c index e13f4d211475..228aa7059018 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1557,6 +1557,37 @@ static __always_inline long __gup_longterm_locked(struct task_struct *tsk, } #endif /* CONFIG_FS_DAX || CONFIG_CMA */ +#ifdef CONFIG_MMU +static long __get_user_pages_remote(struct task_struct *tsk, + struct mm_struct *mm, + unsigned long start, unsigned long nr_pages, + unsigned int gup_flags, struct page **pages, + struct vm_area_struct **vmas, int *locked) +{ + /* + * Parts of FOLL_LONGTERM behavior are incompatible with + * FAULT_FLAG_ALLOW_RETRY because of the FS DAX check requirement on + * vmas. However, this only comes up if locked is set, and there are + * callers that do request FOLL_LONGTERM, but do not set locked. So, + * allow what we can. + */ + if (gup_flags & FOLL_LONGTERM) { + if (WARN_ON_ONCE(locked)) + return -EINVAL; + /* + * This will check the vmas (even if our vmas arg is NULL) + * and return -ENOTSUPP if DAX isn't allowed in this case: + */ + return __gup_longterm_locked(tsk, mm, start, nr_pages, pages, + vmas, gup_flags | FOLL_TOUCH | + FOLL_REMOTE); + } + + return __get_user_pages_locked(tsk, mm, start, nr_pages, pages, vmas, + locked, + gup_flags | FOLL_TOUCH | FOLL_REMOTE); +} + /* * get_user_pages_remote() - pin user pages in memory * @tsk: the task_struct to use for page fault accounting, or @@ -1619,7 +1650,6 @@ static __always_inline long __gup_longterm_locked(struct task_struct *tsk, * should use get_user_pages because it cannot pass * FAULT_FLAG_ALLOW_RETRY to handle_mm_fault. */ -#ifdef CONFIG_MMU long get_user_pages_remote(struct task_struct *tsk, struct mm_struct *mm, unsigned long start, unsigned long nr_pages, unsigned int gup_flags, struct page **pages, @@ -1632,28 +1662,8 @@ long get_user_pages_remote(struct task_struct *tsk, struct mm_struct *mm, if (WARN_ON_ONCE(gup_flags & FOLL_PIN)) return -EINVAL; - /* - * Parts of FOLL_LONGTERM behavior are incompatible with - * FAULT_FLAG_ALLOW_RETRY because of the FS DAX check requirement on - * vmas. However, this only comes up if locked is set, and there are - * callers that do request FOLL_LONGTERM, but do not set locked. So, - * allow what we can. - */ - if (gup_flags & FOLL_LONGTERM) { - if (WARN_ON_ONCE(locked)) - return -EINVAL; - /* - * This will check the vmas (even if our vmas arg is NULL) - * and return -ENOTSUPP if DAX isn't allowed in this case: - */ - return __gup_longterm_locked(tsk, mm, start, nr_pages, pages, - vmas, gup_flags | FOLL_TOUCH | - FOLL_REMOTE); - } - - return __get_user_pages_locked(tsk, mm, start, nr_pages, pages, vmas, - locked, - gup_flags | FOLL_TOUCH | FOLL_REMOTE); + return __get_user_pages_remote(tsk, mm, start, nr_pages, gup_flags, + pages, vmas, locked); } EXPORT_SYMBOL(get_user_pages_remote);
An upcoming patch requires reusing the implementation of get_user_pages_remote(). Split up get_user_pages_remote() into an outer routine that checks flags, and an implementation routine that will be reused. This makes subsequent changes much easier to understand. There should be no change in behavior due to this patch. Signed-off-by: John Hubbard <jhubbard@nvidia.com> --- mm/gup.c | 56 +++++++++++++++++++++++++++++++++----------------------- 1 file changed, 33 insertions(+), 23 deletions(-)