Re: [PATCH v3 3/3] vfio-pci: Invalidate mmaps and block MMIO access on disabled memory

[Jason Gunthorpe]

On Mon, May 25, 2020 at 01:56:28PM -0700, John Hubbard wrote:
> On 2020-05-25 09:56, Jason Gunthorpe wrote:
> > On Mon, May 25, 2020 at 11:11:42AM -0400, Peter Xu wrote:
> > > On Mon, May 25, 2020 at 11:46:51AM -0300, Jason Gunthorpe wrote:
> > > > On Mon, May 25, 2020 at 10:28:06AM -0400, Peter Xu wrote:
> > > > > On Mon, May 25, 2020 at 09:26:07AM -0300, Jason Gunthorpe wrote:
> > > > > > On Sat, May 23, 2020 at 07:52:57PM -0400, Peter Xu wrote:
> > > > > > 
> > > > > > > For what I understand now, IMHO we should still need all those handlings of
> > > > > > > FAULT_FLAG_RETRY_NOWAIT like in the initial version.  E.g., IIUC KVM gup will
> > > > > > > try with FOLL_NOWAIT when async is allowed, before the complete slow path.  I'm
> > > > > > > not sure what would be the side effect of that if fault() blocked it.  E.g.,
> > > > > > > the caller could be in an atomic context.
> > > > > > 
> > > > > > AFAICT FAULT_FLAG_RETRY_NOWAIT only impacts what happens when
> > > > > > VM_FAULT_RETRY is returned, which this doesn't do?
> > > > > 
> > > > > Yes, that's why I think we should still properly return VM_FAULT_RETRY if
> > > > > needed..  because IMHO it is still possible that the caller calls with
> > > > > FAULT_FLAG_RETRY_NOWAIT.
> > > > > 
> > > > > My understanding is that FAULT_FLAG_RETRY_NOWAIT majorly means:
> > > > > 
> > > > >    - We cannot release the mmap_sem, and,
> > > > >    - We cannot sleep
> > > > 
> > > > Sleeping looks fine, look at any FS implementation of fault, say,
> > > > xfs. The first thing it does is xfs_ilock() which does down_write().
> > > 
> > > Yeah.  My wild guess is that maybe fs code will always be without
> > > FAULT_FLAG_RETRY_NOWAIT so it's safe to sleep unconditionally (e.g., I think
> > > the general #PF should be fine to sleep in fault(); gup should be special, but
> > > I didn't observe any gup code called upon file systems)?
> > 
> > get_user_pages is called on filesystem backed pages.
> > 
> > I have no idea what FAULT_FLAG_RETRY_NOWAIT is supposed to do. Maybe
> > John was able to guess when he reworked that stuff?
> > 
> 
> Although I didn't end up touching that particular area, I'm sure it's going
> to come up sometime soon, so I poked around just now, and found that
> FAULT_FLAG_RETRY_NOWAIT was added almost exactly 9 years ago. This flag was
> intended to make KVM and similar things behave better when doing GUP on
> file-backed pages that might, or might not be in memory.
> 
> The idea is described in the changelog, but not in the code comments or
> Documentation, sigh:
> 
> commit 318b275fbca1ab9ec0862de71420e0e92c3d1aa7
> Author: Gleb Natapov <gleb@xxxxxxxxxx>
> Date:   Tue Mar 22 16:30:51 2011 -0700
> 
>     mm: allow GUP to fail instead of waiting on a page
> 
>     GUP user may want to try to acquire a reference to a page if it is already
>     in memory, but not if IO, to bring it in, is needed.  For example KVM may
>     tell vcpu to schedule another guest process if current one is trying to
>     access swapped out page.  Meanwhile, the page will be swapped in and the
>     guest process, that depends on it, will be able to run again.
> 
>     This patch adds FAULT_FLAG_RETRY_NOWAIT (suggested by Linus) and
>     FOLL_NOWAIT follow_page flags.  FAULT_FLAG_RETRY_NOWAIT, when used in
>     conjunction with VM_FAULT_ALLOW_RETRY, indicates to handle_mm_fault that
>     it shouldn't drop mmap_sem and wait on a page, but return VM_FAULT_RETRY
>     instead.

So, from kvm's perspective it was to avoid excessively long blocking in
common paths when it could rejoin the completed IO by somehow waiting
on a page itself?

It all seems like it should not be used unless the page is going to go
to IO?

Certainly there is no reason to optimize the fringe case of vfio
sleeping if there is and incorrect concurrnent attempt to disable the
a BAR.

> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 8429d5aa31e44..e32e8e52a57ac 100644
> +++ b/include/linux/mm.h
> @@ -430,6 +430,15 @@ extern pgprot_t protection_map[16];
>   * continuous faults with flags (b).  We should always try to detect pending
>   * signals before a retry to make sure the continuous page faults can still be
>   * interrupted if necessary.
> + *
> + * About @FAULT_FLAG_RETRY_NOWAIT: this is intended for callers who would like
> + * to acquire a page, but only if the page is already in memory. If, on the
> + * other hand, the page requires IO in order to bring it into memory, then fault
> + * handlers will immediately return VM_FAULT_RETRY ("don't wait"), while leaving
> + * mmap_lock held ("don't drop mmap_lock"). For example, this is useful for
> + * virtual machines that have multiple guests running: if guest A attempts
> + * get_user_pages() on a swapped out page, another guest can be scheduled while
> + * waiting for IO to swap in guest A's page.
>   */
>  #define FAULT_FLAG_WRITE                       0x01
>  #define FAULT_FLAG_MKWRITE                     0x02

It seems reasonable but people might complain about the kvm
specifics of the explanation.

It might be better to explain how the caller is supposed to know when
it is OK to try GUP again and expect success, as it seems to me this
is really about externalizing the sleep for page wait?

Jason