| Message ID | 1431624680-20153-15-git-send-email-aarcange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 14, 2015 at 07:31:11PM +0200, Andrea Arcangeli wrote: > @@ -255,21 +259,23 @@ int handle_userfault(struct vm_area_struct *vma, unsigned long address, > * through poll/read(). > */ > __add_wait_queue(&ctx->fault_wqh, &uwq.wq); > - for (;;) { > - set_current_state(TASK_KILLABLE); > - if (!uwq.pending || ACCESS_ONCE(ctx->released) || > - fatal_signal_pending(current)) > - break; > - spin_unlock(&ctx->fault_wqh.lock); > + set_current_state(TASK_KILLABLE); > + spin_unlock(&ctx->fault_wqh.lock); > > + if (likely(!ACCESS_ONCE(ctx->released) && > + !fatal_signal_pending(current))) { > wake_up_poll(&ctx->fd_wqh, POLLIN); > schedule(); > + ret |= VM_FAULT_MAJOR; > + } So what happens here if schedule() spontaneously wakes for no reason? I'm not sure enough of userfaultfd semantics to say if that would be bad, but the code looks suspiciously like it relies on schedule() not to do that; which is wrong. > + __set_current_state(TASK_RUNNING); > + /* see finish_wait() comment for why list_empty_careful() */ > + if (!list_empty_careful(&uwq.wq.task_list)) { > spin_lock(&ctx->fault_wqh.lock); > + list_del_init(&uwq.wq.task_list); > + spin_unlock(&ctx->fault_wqh.lock); > } > - __remove_wait_queue(&ctx->fault_wqh, &uwq.wq); > - __set_current_state(TASK_RUNNING); > - spin_unlock(&ctx->fault_wqh.lock); > > /* > * ctx may go away after this if the userfault pseudo fd is -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 22, 2015 at 02:10:56PM +0200, Peter Zijlstra wrote: > On Thu, May 14, 2015 at 07:31:11PM +0200, Andrea Arcangeli wrote: > > @@ -255,21 +259,23 @@ int handle_userfault(struct vm_area_struct *vma, unsigned long address, > > * through poll/read(). > > */ > > __add_wait_queue(&ctx->fault_wqh, &uwq.wq); > > - for (;;) { > > - set_current_state(TASK_KILLABLE); > > - if (!uwq.pending || ACCESS_ONCE(ctx->released) || > > - fatal_signal_pending(current)) > > - break; > > - spin_unlock(&ctx->fault_wqh.lock); > > + set_current_state(TASK_KILLABLE); > > + spin_unlock(&ctx->fault_wqh.lock); > > > > + if (likely(!ACCESS_ONCE(ctx->released) && > > + !fatal_signal_pending(current))) { > > wake_up_poll(&ctx->fd_wqh, POLLIN); > > schedule(); > > + ret |= VM_FAULT_MAJOR; > > + } > > So what happens here if schedule() spontaneously wakes for no reason? > > I'm not sure enough of userfaultfd semantics to say if that would be > bad, but the code looks suspiciously like it relies on schedule() not to > do that; which is wrong. That would repeat the fault and trigger the DEBUG_VM printk above, complaining that FAULT_FLAG_ALLOW_RETRY is not set. It is only a problem for kernel faults (copy_user/get_user_pages*). Userland won't error out in such a way because userland would return 0 and not VM_FAULT_RETRY. So it's only required when the task schedule in TASK_KILLABLE state. If schedule spontaneously wakes up a task in TASK_KILLABLE state that would be a bug in the scheduler in my view. Luckily there doesn't seem to be such a bug, or at least we never experienced it. Overall this dependency on the scheduler will be lifted soon, as it must be lifted in order to track the write protect faults, so longer term this is not a concern and this is not a design issue, but this remains an implementation detail that avoided to change the arch code and gup. If you send a reproducer to show how the current scheduler can wake up the task in TASK_KILLABLE despite not receiving a wakeup, that would help too as we never experienced that. The reason this dependency will be lifted soon is that the userfaultfd write protect tracking may be armed at any time while the app is running so we may already be in the middle of a page fault that returned VM_FAULT_RETRY by the time we arm the write protect tracking. So longer term the arch page fault and __get_user_pages_locked must allow handle_userfault() to return VM_FAULT_RETRY even if FAULT_FLAG_TRIED is set. Then we don't care if the task is waken. Waking a task in TASK_KILLABLE state it will still waste CPU so the scheduler still shouldn't do that. All load balancing works better if the task isn't running anyway so I can't imagine a good reason for wanting to run a task in TASK_KILLABLE state before it gets the wakeup. Trying to predict that a wakeup is always happening in less time than it takes to schedule the task out of the CPU, sounds a very CPU intensive thing to measure and it's probably better to leave those heuristics in the caller like by spinning on a lock for a while before blocking. Thanks, Andrea -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 22, 2015 at 03:20:15PM +0200, Andrea Arcangeli wrote: > If schedule spontaneously wakes up a task in TASK_KILLABLE state that > would be a bug in the scheduler in my view. Luckily there doesn't seem > to be such a bug, or at least we never experienced it. Well, there will be a wakeup, just not the one you were hoping for. We have code that does: @cond = true; get_task_struct(p); queue(p) /* random wait somewhere */ for (;;) { prepare_to_wait(); if (@cond) break; ... handle_userfault() ... schedule(); ... dequeue(p) wake_up_process(p) ---> wakeup without userfault wakeup These races are (extremely) rare, but they do exist. Therefore one must never assume schedule() will not spuriously wake because of these things. Also, see: lkml.kernel.org/r/CA+55aFwHkOo+YGWKYROmce1-H_uG3KfEUmCkJUerTj=ojY2H6Q@mail.gmail.com -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 22, 2015 at 03:38:24PM +0200, Peter Zijlstra wrote: > On Thu, Oct 22, 2015 at 03:20:15PM +0200, Andrea Arcangeli wrote: > > > If schedule spontaneously wakes up a task in TASK_KILLABLE state that > > would be a bug in the scheduler in my view. Luckily there doesn't seem > > to be such a bug, or at least we never experienced it. > > Well, there will be a wakeup, just not the one you were hoping for. > > We have code that does: > > @cond = true; > get_task_struct(p); > queue(p) > > /* random wait somewhere */ > for (;;) { > prepare_to_wait(); > if (@cond) > break; > > ... > > handle_userfault() > ... > schedule(); > ... > > dequeue(p) > wake_up_process(p) ---> wakeup without userfault wakeup > > > These races are (extremely) rare, but they do exist. Therefore one must > never assume schedule() will not spuriously wake because of these > things. > > Also, see: > > lkml.kernel.org/r/CA+55aFwHkOo+YGWKYROmce1-H_uG3KfEUmCkJUerTj=ojY2H6Q@mail.gmail.com With one more spinlock taken in the fast path we could recheck if the waitqueue is still queued and this is a false positive extremely rare spurious wakeup, and in such case set the state back to TASK_KILLABLE and schedule. However in the long term such a spinlock should be removed because it's faster to stick with the current lockless list_empty_careful and not to recheck the auto-remove waitqueue, but then we must be able to re-enter handle_userfault() even if FAULT_FLAG_TRIED was set (currently we can't return VM_FAULT_RETRY if FAULT_FLAG_TRIED is set and that's the problem). This change is planned for a long time as we need it to arm the vma-less write protection while the app is running, so I'm not sure if it's worth going for the short term fix if this is extremely rare. The risk of memory corruption is still zero no matter what happens here, in the extremely rare case the app will get a SIGBUS or a syscall will return -EFAULT. The kernel also cannot crash. So it's not very severe concern if it happens extremely rarely (we never reproduced it and stress testing run for months). Of course in the longer term this would have been fixed regardless as said in previous email. I think going for the longer term fix that was already planned, is better than doing a short term fix and the real question is how I should proceed to change the arch code and gup to cope with handle_userfault() being re-entered. The simplest thing is to drop FAULT_FLAG_TRIED as a whole. Or I could add a new VM_FAULT_USERFAULT flag specific to handle_userfault that would be returned even if FAULT_FLAG_TRIED is set, so that only userfaults will be allowed to be repeated indefinitely (and then VM_FAULT_USERFAULT shouldn't trigger a transition to FAULT_FLAG_TRIED, unlike VM_FAULT_RETRY does). This is all about being allowed to drop the mmap_sem. If we'd check the waitqueue with the spinlock (to be sure the wakeup isn't happening from under us while we check if we got an userfault wakeup or if this is a spurious schedule), we could also limit the VM_FAULT_RETRY to 2 max events if I add a FAULT_FLAG_TRIED2 and I still use VM_FAULT_RETRY (instead of VM_FAULT_USERFAULT). Being able to return VM_FAULT_RETRY indefinitely is only needed if we don't handle the extremely wakeup race condition in handle_userfault by taking the spinlock once more time in the fast path (i.e. after the schedule). I'm not exactly sure why we allow VM_FAULT_RETRY only once currently so I'm tempted to drop FAULT_FLAG_TRIED entirely. I've no real preference on how to tweak the page fault code to be able to return VM_FAULT_RETRY indefinitely and I would aim for the smallest change possible, so if you've suggestions now it's good time. Thanks, Andrea -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 22, 2015 at 04:18:31PM +0200, Andrea Arcangeli wrote: > The risk of memory corruption is still zero no matter what happens > here, in the extremely rare case the app will get a SIGBUS or a That might still upset people, SIGBUS isn't something an app can really recover from. > I'm not exactly sure why we allow VM_FAULT_RETRY only once currently > so I'm tempted to drop FAULT_FLAG_TRIED entirely. I think to ensure we make forward progress. > I've no real preference on how to tweak the page fault code to be able > to return VM_FAULT_RETRY indefinitely and I would aim for the smallest > change possible, so if you've suggestions now it's good time. Indefinitely is such a long time, we should try and finish computation before the computer dies etc. :-) Yes, yes.. I know, extremely unlikely etc. Still guarantees are good. In any case, I'm not really too bothered how you fix it, just figured I'd let you know. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 22, 2015 at 05:15:09PM +0200, Peter Zijlstra wrote: > Indefinitely is such a long time, we should try and finish > computation before the computer dies etc. :-) Indefinitely as read_seqcount_retry, eventually it makes progress. Even returning 0 from the page fault can trigger it again indefinitely, so VM_FAULT_RETRY isn't fundamentally different from returning 0 and retrying the page fault again later. So it's not clear why VM_FAULT_RETRY can only try once more. FAULT_FLAG_TRIED as a message to the VM so it starts to do heavy locking and block more aggressively is actually useful as such, but it shouldn't be a replacement of FAULT_FLAG_ALLOW_RETRY. What I meant with removing FAULT_FLAG_TRIED is really about converting it to an hint, but not controlling if the page fault can keep retrying in-kernel. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c index b45cefe..50edbd8 100644 --- a/fs/userfaultfd.c +++ b/fs/userfaultfd.c @@ -52,6 +52,10 @@ struct userfaultfd_ctx { struct userfaultfd_wait_queue { struct uffd_msg msg; wait_queue_t wq; + /* + * Only relevant when queued in fault_wqh and only used by the + * read operation to avoid reading the same userfault twice. + */ bool pending; struct userfaultfd_ctx *ctx; }; @@ -71,9 +75,6 @@ static int userfaultfd_wake_function(wait_queue_t *wq, unsigned mode, uwq = container_of(wq, struct userfaultfd_wait_queue, wq); ret = 0; - /* don't wake the pending ones to avoid reads to block */ - if (uwq->pending && !ACCESS_ONCE(uwq->ctx->released)) - goto out; /* len == 0 means wake all */ start = range->start; len = range->len; @@ -183,12 +184,14 @@ int handle_userfault(struct vm_area_struct *vma, unsigned long address, struct mm_struct *mm = vma->vm_mm; struct userfaultfd_ctx *ctx; struct userfaultfd_wait_queue uwq; + int ret; BUG_ON(!rwsem_is_locked(&mm->mmap_sem)); + ret = VM_FAULT_SIGBUS; ctx = vma->vm_userfaultfd_ctx.ctx; if (!ctx) - return VM_FAULT_SIGBUS; + goto out; BUG_ON(ctx->mm != mm); @@ -201,7 +204,7 @@ int handle_userfault(struct vm_area_struct *vma, unsigned long address, * caller of handle_userfault to release the mmap_sem. */ if (unlikely(ACCESS_ONCE(ctx->released))) - return VM_FAULT_SIGBUS; + goto out; /* * Check that we can return VM_FAULT_RETRY. @@ -227,15 +230,16 @@ int handle_userfault(struct vm_area_struct *vma, unsigned long address, dump_stack(); } #endif - return VM_FAULT_SIGBUS; + goto out; } /* * Handle nowait, not much to do other than tell it to retry * and wait. */ + ret = VM_FAULT_RETRY; if (flags & FAULT_FLAG_RETRY_NOWAIT) - return VM_FAULT_RETRY; + goto out; /* take the reference before dropping the mmap_sem */ userfaultfd_ctx_get(ctx); @@ -255,21 +259,23 @@ int handle_userfault(struct vm_area_struct *vma, unsigned long address, * through poll/read(). */ __add_wait_queue(&ctx->fault_wqh, &uwq.wq); - for (;;) { - set_current_state(TASK_KILLABLE); - if (!uwq.pending || ACCESS_ONCE(ctx->released) || - fatal_signal_pending(current)) - break; - spin_unlock(&ctx->fault_wqh.lock); + set_current_state(TASK_KILLABLE); + spin_unlock(&ctx->fault_wqh.lock); + if (likely(!ACCESS_ONCE(ctx->released) && + !fatal_signal_pending(current))) { wake_up_poll(&ctx->fd_wqh, POLLIN); schedule(); + ret |= VM_FAULT_MAJOR; + } + __set_current_state(TASK_RUNNING); + /* see finish_wait() comment for why list_empty_careful() */ + if (!list_empty_careful(&uwq.wq.task_list)) { spin_lock(&ctx->fault_wqh.lock); + list_del_init(&uwq.wq.task_list); + spin_unlock(&ctx->fault_wqh.lock); } - __remove_wait_queue(&ctx->fault_wqh, &uwq.wq); - __set_current_state(TASK_RUNNING); - spin_unlock(&ctx->fault_wqh.lock); /* * ctx may go away after this if the userfault pseudo fd is @@ -277,7 +283,8 @@ int handle_userfault(struct vm_area_struct *vma, unsigned long address, */ userfaultfd_ctx_put(ctx); - return VM_FAULT_RETRY; +out: + return ret; } static int userfaultfd_release(struct inode *inode, struct file *file) @@ -391,6 +398,12 @@ static unsigned int userfaultfd_poll(struct file *file, poll_table *wait) case UFFD_STATE_WAIT_API: return POLLERR; case UFFD_STATE_RUNNING: + /* + * poll() never guarantees that read won't block. + * userfaults can be waken before they're read(). + */ + if (unlikely(!(file->f_flags & O_NONBLOCK))) + return POLLERR; spin_lock(&ctx->fault_wqh.lock); ret = find_userfault(ctx, NULL); spin_unlock(&ctx->fault_wqh.lock); @@ -806,11 +819,19 @@ out: } /* - * This is mostly needed to re-wakeup those userfaults that were still - * pending when userland wake them up the first time. We don't wake - * the pending one to avoid blocking reads to block, or non blocking - * read to return -EAGAIN, if used with POLLIN, to avoid userland - * doubts on why POLLIN wasn't reliable. + * userfaultfd_wake is needed in case an userfault is in flight by the + * time a UFFDIO_COPY (or other ioctl variants) completes. The page + * may be well get mapped and the page fault if repeated wouldn't lead + * to a userfault anymore, but before scheduling in TASK_KILLABLE mode + * handle_userfault() doesn't recheck the pagetables and it doesn't + * serialize against UFFDO_COPY (or other ioctl variants). Ultimately + * the knowledge of which pages are mapped is left to userland who is + * responsible for handling the race between read() userfaults and + * background UFFDIO_COPY (or other ioctl variants), if done by + * separate concurrent threads. + * + * userfaultfd_wake may be used in combination with the + * UFFDIO_*_MODE_DONTWAKE to wakeup userfaults in batches. */ static int userfaultfd_wake(struct userfaultfd_ctx *ctx, unsigned long arg)
This is an optimization but it's a userland visible one and it affects the API. The downside of this optimization is that if you call poll() and you get POLLIN, read(ufd) may still return -EAGAIN. The blocked userfault may be waken by a different thread, before read(ufd) comes around. This in short means that poll() isn't really usable if the userfaultfd is opened in blocking mode. userfaults won't wait in "pending" state to be read anymore and any UFFDIO_WAKE or similar operations that has the objective of waking userfaults after their resolution, will wake all blocked userfaults for the resolved range, including those that haven't been read() by userland yet. The behavior of poll() becomes not standard, but this obviates the need of "spurious" UFFDIO_WAKE and it lets the userland threads to restart immediately without requiring an UFFDIO_WAKE. This is even more significant in case of repeated faults on the same address from multiple threads. This optimization is justified by the measurement that the number of spurious UFFDIO_WAKE accounts for 5% and 10% of the total userfaults for heavy workloads, so it's worth optimizing those away. Signed-off-by: Andrea Arcangeli <aarcange@redhat.com> --- fs/userfaultfd.c | 65 +++++++++++++++++++++++++++++++++++++------------------- 1 file changed, 43 insertions(+), 22 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html