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

[John Hubbard]

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.

If that helps, maybe documentation approximately like this might be welcome
(against linux-next, so I'm using mmap_lock, instead of mmap_sem), below.
Or is this overkill? People like minimal documentation in the code, so maybe
this belongs in Documentation, if anywhere:

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 8429d5aa31e44..e32e8e52a57ac 100644
--- a/include/linux/mm.h
+++ 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


thanks,
--
John Hubbard
NVIDIA