Hi, John,
Thanks for your comments!
On Mon, Jun 27, 2022 at 07:07:28PM -0700, John Hubbard wrote:
[...]
> > @@ -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/ ?
Sure.
> > 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?
I put it here to be after FAULT_FLAG_KILLABLE we just set.
Only if we have "locked" will we set FAULT_FLAG_KILLABLE. That's also the
key we grant "killable" attribute to this GUP. So I thought it'll be good
to put here because I want to have FOLL_INTERRUPTIBLE dependent on "locked"
being set.
>
> > + 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...) ?
Note: I introduced the new FOLL_INTERRUPTIBLE flag, so only if the caller
explicitly passing in that flag could there be a functional change.
IOW, no functional change intended for this single patch, not before I
start to let KVM code passing over that flag.
>
> 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.
Yes as I mentioned anyway it'll be an opt-in flag, so by default we don't
need to worry at all, IMHO, because it should really work exactly like
before, otherwise I had a bug somewhere else.. :)
Thanks,
--
Peter Xu
