Messages in this thread | | | Date | Mon, 27 Jun 2022 19:07:28 -0700 | Subject | Re: [PATCH 1/4] mm/gup: Add FOLL_INTERRUPTIBLE | From | John Hubbard <> |
| |
On 6/22/22 14:36, Peter Xu wrote: > We have had FAULT_FLAG_INTERRUPTIBLE but it was never applied to GUPs. One > issue with it is that not all GUP paths are able to handle signal delivers > besides SIGKILL. > > That's not ideal for the GUP users who are actually able to handle these > cases, like KVM. > > KVM uses GUP extensively on faulting guest pages, during which we've got > existing infrastructures to retry a page fault at a later time. Allowing > the GUP to be interrupted by generic signals can make KVM related threads > to be more responsive. For examples: > > (1) SIGUSR1: which QEMU/KVM uses to deliver an inter-process IPI, > e.g. when the admin issues a vm_stop QMP command, SIGUSR1 can be > generated to kick the vcpus out of kernel context immediately, > > (2) SIGINT: which can be used with interactive hypervisor users to stop a > virtual machine with Ctrl-C without any delays/hangs, > > (3) SIGTRAP: which grants GDB capability even during page faults that are > stuck for a long time. > > Normally hypervisor will be able to receive these signals properly, but not > if we're stuck in a GUP for a long time for whatever reason. It happens > easily with a stucked postcopy migration when e.g. a network temp failure > happens, then some vcpu threads can hang death waiting for the pages. With > the new FOLL_INTERRUPTIBLE, we can allow GUP users like KVM to selectively > enable the ability to trap these signals. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > include/linux/mm.h | 1 + > mm/gup.c | 33 +++++++++++++++++++++++++++++---- > 2 files changed, 30 insertions(+), 4 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index bc8f326be0ce..ebdf8a6b86c1 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -2941,6 +2941,7 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address, > #define FOLL_SPLIT_PMD 0x20000 /* split huge pmd before returning */ > #define FOLL_PIN 0x40000 /* pages must be released via unpin_user_page */ > #define FOLL_FAST_ONLY 0x80000 /* gup_fast: prevent fall-back to slow gup */ > +#define FOLL_INTERRUPTIBLE 0x100000 /* allow interrupts from generic signals */
Perhaps, s/generic/non-fatal/ ? > > /* > * FOLL_PIN and FOLL_LONGTERM may be used in various combinations with each > diff --git a/mm/gup.c b/mm/gup.c > index 551264407624..ad74b137d363 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -933,8 +933,17 @@ 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 (locked) { > fault_flags |= FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE; > + /* > + * We should only grant FAULT_FLAG_INTERRUPTIBLE when we're > + * (at least) killable. It also mostly means we're not > + * with NOWAIT. Otherwise ignore FOLL_INTERRUPTIBLE since > + * it won't make a lot of sense to be used alone. > + */
This comment seems a little confusing due to its location. We've just checked "locked", but the comment is talking about other constraints.
Not sure what to suggest. Maybe move it somewhere else?
> + if (*flags & FOLL_INTERRUPTIBLE) > + fault_flags |= FAULT_FLAG_INTERRUPTIBLE; > + } > if (*flags & FOLL_NOWAIT) > fault_flags |= FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_RETRY_NOWAIT; > if (*flags & FOLL_TRIED) { > @@ -1322,6 +1331,22 @@ int fixup_user_fault(struct mm_struct *mm, > } > EXPORT_SYMBOL_GPL(fixup_user_fault); > > +/* > + * GUP always responds to fatal signals. When FOLL_INTERRUPTIBLE is > + * specified, it'll also respond to generic signals. The caller of GUP > + * that has FOLL_INTERRUPTIBLE should take care of the GUP interruption. > + */ > +static bool gup_signal_pending(unsigned int flags) > +{ > + if (fatal_signal_pending(current)) > + return true; > + > + if (!(flags & FOLL_INTERRUPTIBLE)) > + return false; > + > + return signal_pending(current); > +} > +
OK.
> /* > * Please note that this function, unlike __get_user_pages will not > * return 0 for nr_pages > 0 without FOLL_NOWAIT > @@ -1403,11 +1428,11 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm, > * Repeat on the address that fired VM_FAULT_RETRY > * with both FAULT_FLAG_ALLOW_RETRY and > * FAULT_FLAG_TRIED. Note that GUP can be interrupted > - * by fatal signals, so we need to check it before we > + * by fatal signals of even common signals, depending on > + * the caller's request. So we need to check it before we > * start trying again otherwise it can loop forever. > */ > - > - if (fatal_signal_pending(current)) { > + if (gup_signal_pending(flags)) {
This is new and bold. :) Signals that an application was prepared to handle can now cause gup to quit early. I wonder if that will break any use cases out there (SIGPIPE...) ?
Generally, gup callers handle failures pretty well, so it's probably not too bad. But I wanted to mention the idea that handled interrupts might be a little surprising here.
thanks, -- John Hubbard NVIDIA
> if (!pages_done) > pages_done = -EINTR; > break;
| |