| Message ID | 20190121075722.7945-5-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | userfaultfd: write protection support | expand |
On Mon, Jan 21, 2019 at 03:57:02PM +0800, Peter Xu wrote: > This is the gup counterpart of the change that allows the VM_FAULT_RETRY > to happen for more than once. > > Signed-off-by: Peter Xu <peterx@redhat.com> So it would be nice to add a comment in the code and in the commit message about possible fault starvation (mostly due to previous patch changes) as if some one experience that and try to bisect it might overlook the commit. Otherwise: Reviewed-by: Jérôme Glisse <jglisse@redhat.com> > --- > mm/gup.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/mm/gup.c b/mm/gup.c > index 7b1f452cc2ef..22f1d419a849 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -528,7 +528,10 @@ static int faultin_page(struct task_struct *tsk, struct vm_area_struct *vma, > if (*flags & FOLL_NOWAIT) > fault_flags |= FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_RETRY_NOWAIT; > if (*flags & FOLL_TRIED) { > - VM_WARN_ON_ONCE(fault_flags & FAULT_FLAG_ALLOW_RETRY); > + /* > + * Note: FAULT_FLAG_ALLOW_RETRY and FAULT_FLAG_TRIED > + * can co-exist > + */ > fault_flags |= FAULT_FLAG_TRIED; > } > > @@ -943,17 +946,23 @@ static __always_inline long __get_user_pages_locked(struct task_struct *tsk, > /* VM_FAULT_RETRY triggered, so seek to the faulting offset */ > pages += ret; > start += ret << PAGE_SHIFT; > + lock_dropped = true; > > +retry: > /* > * Repeat on the address that fired VM_FAULT_RETRY > - * without FAULT_FLAG_ALLOW_RETRY but with > + * with both FAULT_FLAG_ALLOW_RETRY and > * FAULT_FLAG_TRIED. > */ > *locked = 1; > - lock_dropped = true; > down_read(&mm->mmap_sem); > ret = __get_user_pages(tsk, mm, start, 1, flags | FOLL_TRIED, > - pages, NULL, NULL); > + pages, NULL, locked); > + if (!*locked) { > + /* Continue to retry until we succeeded */ > + BUG_ON(ret != 0); > + goto retry; > + } > if (ret != 1) { > BUG_ON(ret > 1); > if (!pages_done) > -- > 2.17.1 >
On Mon, Jan 21, 2019 at 11:24:55AM -0500, Jerome Glisse wrote: > On Mon, Jan 21, 2019 at 03:57:02PM +0800, Peter Xu wrote: > > This is the gup counterpart of the change that allows the VM_FAULT_RETRY > > to happen for more than once. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > So it would be nice to add a comment in the code and in the commit message > about possible fault starvation (mostly due to previous patch changes) as > if some one experience that and try to bisect it might overlook the commit. > > Otherwise: > > Reviewed-by: Jérôme Glisse <jglisse@redhat.com> Jerome, can I still keep this r-b if I'm going to fix the starvation issue you mentioned in previous patch about lock page? Regards,
On Thu, Jan 24, 2019 at 03:05:03PM +0800, Peter Xu wrote: > On Mon, Jan 21, 2019 at 11:24:55AM -0500, Jerome Glisse wrote: > > On Mon, Jan 21, 2019 at 03:57:02PM +0800, Peter Xu wrote: > > > This is the gup counterpart of the change that allows the VM_FAULT_RETRY > > > to happen for more than once. > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > So it would be nice to add a comment in the code and in the commit message > > about possible fault starvation (mostly due to previous patch changes) as > > if some one experience that and try to bisect it might overlook the commit. > > > > Otherwise: > > > > Reviewed-by: Jérôme Glisse <jglisse@redhat.com> > > Jerome, can I still keep this r-b if I'm going to fix the starvation > issue you mentioned in previous patch about lock page? > No please, i still want to review properly the oneline ie making sure that it will not change any of the existing use of FAULT_FLAG_TRIED I am finishing a bunch of patches myself so i am bit short on time right now to take a deeper look but i will try to do that in next few days :) In anycase i will review again your next posting. Cheers, Jérôme
On Thu, Jan 24, 2019 at 10:34:32AM -0500, Jerome Glisse wrote: > On Thu, Jan 24, 2019 at 03:05:03PM +0800, Peter Xu wrote: > > On Mon, Jan 21, 2019 at 11:24:55AM -0500, Jerome Glisse wrote: > > > On Mon, Jan 21, 2019 at 03:57:02PM +0800, Peter Xu wrote: > > > > This is the gup counterpart of the change that allows the VM_FAULT_RETRY > > > > to happen for more than once. > > > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > > > So it would be nice to add a comment in the code and in the commit message > > > about possible fault starvation (mostly due to previous patch changes) as > > > if some one experience that and try to bisect it might overlook the commit. > > > > > > Otherwise: > > > > > > Reviewed-by: Jérôme Glisse <jglisse@redhat.com> > > > > Jerome, can I still keep this r-b if I'm going to fix the starvation > > issue you mentioned in previous patch about lock page? > > > > No please, i still want to review properly the oneline ie making sure > that it will not change any of the existing use of FAULT_FLAG_TRIED > I am finishing a bunch of patches myself so i am bit short on time right > now to take a deeper look but i will try to do that in next few days :) > > In anycase i will review again your next posting. Sure thing. Thank you Jerome!
diff --git a/mm/gup.c b/mm/gup.c index 7b1f452cc2ef..22f1d419a849 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -528,7 +528,10 @@ static int faultin_page(struct task_struct *tsk, struct vm_area_struct *vma, if (*flags & FOLL_NOWAIT) fault_flags |= FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_RETRY_NOWAIT; if (*flags & FOLL_TRIED) { - VM_WARN_ON_ONCE(fault_flags & FAULT_FLAG_ALLOW_RETRY); + /* + * Note: FAULT_FLAG_ALLOW_RETRY and FAULT_FLAG_TRIED + * can co-exist + */ fault_flags |= FAULT_FLAG_TRIED; } @@ -943,17 +946,23 @@ static __always_inline long __get_user_pages_locked(struct task_struct *tsk, /* VM_FAULT_RETRY triggered, so seek to the faulting offset */ pages += ret; start += ret << PAGE_SHIFT; + lock_dropped = true; +retry: /* * Repeat on the address that fired VM_FAULT_RETRY - * without FAULT_FLAG_ALLOW_RETRY but with + * with both FAULT_FLAG_ALLOW_RETRY and * FAULT_FLAG_TRIED. */ *locked = 1; - lock_dropped = true; down_read(&mm->mmap_sem); ret = __get_user_pages(tsk, mm, start, 1, flags | FOLL_TRIED, - pages, NULL, NULL); + pages, NULL, locked); + if (!*locked) { + /* Continue to retry until we succeeded */ + BUG_ON(ret != 0); + goto retry; + } if (ret != 1) { BUG_ON(ret > 1); if (!pages_done)
This is the gup counterpart of the change that allows the VM_FAULT_RETRY to happen for more than once. Signed-off-by: Peter Xu <peterx@redhat.com> --- mm/gup.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)