Re: [PATCH 1/4] mm/gup: Add FOLL_INTERRUPTIBLE

[Peter Xu <peterx@redhat.com>]

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