Re: [Qemu-devel] [PATCH v3 20/29] postcopy: postcopy_notify_shared_wake

[Dr. David Alan Gilbert]

* Peter Xu (address@hidden) wrote:
> On Mon, Mar 05, 2018 at 07:55:13PM +0000, Dr. David Alan Gilbert wrote:
> > * Peter Xu (address@hidden) wrote:
> > > On Fri, Feb 16, 2018 at 01:16:16PM +0000, Dr. David Alan Gilbert (git) 
> > > wrote:
> > > > From: "Dr. David Alan Gilbert" <address@hidden>
> > > > 
> > > > Add a hook to allow a client userfaultfd to be 'woken'
> > > > when a page arrives, and a walker that calls that
> > > > hook for relevant clients given a RAMBlock and offset.
> > > > 
> > > > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > > > ---
> > > >  migration/postcopy-ram.c | 16 ++++++++++++++++
> > > >  migration/postcopy-ram.h | 10 ++++++++++
> > > >  2 files changed, 26 insertions(+)
> > > > 
> > > > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > > > index 67deae7e1c..879711968c 100644
> > > > --- a/migration/postcopy-ram.c
> > > > +++ b/migration/postcopy-ram.c
> > > > @@ -824,6 +824,22 @@ static int qemu_ufd_copy_ioctl(int userfault_fd, 
> > > > void *host_addr,
> > > >      return ret;
> > > >  }
> > > >  
> > > > +int postcopy_notify_shared_wake(RAMBlock *rb, uint64_t offset)
> > > > +{
> > > > +    int i;
> > > > +    MigrationIncomingState *mis = migration_incoming_get_current();
> > > > +    GArray *pcrfds = mis->postcopy_remote_fds;
> > > > +
> > > > +    for (i = 0; i < pcrfds->len; i++) {
> > > > +        struct PostCopyFD *cur = &g_array_index(pcrfds, struct 
> > > > PostCopyFD, i);
> > > > +        int ret = cur->waker(cur, rb, offset);
> > > > +        if (ret) {
> > > > +            return ret;
> > > > +        }
> > > > +    }
> > > > +    return 0;
> > > > +}
> > > > +
> > > 
> > > We should know that which FD needs what pages, right?  If with that
> > > information, we can only notify the ones who have page faulted on
> > > exactly the same page?  Otherwise we do UFFDIO_WAKE once for each
> > > client when a page is ready, even if the clients have not page faulted
> > > at all?
> > 
> > The 'waker' function we call knows that, we don't; see the
> > 'vhost_user_postcopy_waker' in the next patch, and it hunts down whether
> > the address the waker is called for is one it's responsible for.
> 
> For vhost-user devices, they should be always responsible for mostly
> all RAM exported on the guest?  If so, they will always be notified to
> wake up if a page is copied?

Right; but this patch isn't vhost-user specific; this is more general.

> Here I was thinking not only about responsible ranges - It was about
> whether each PostcopyFD could note down the faulted addresses that
> were waiting to be service.  Then when we do the wake up, we could
> possibly skip notifying the PostcopyFD when the copied page is not
> covering any of the faulted addresses on that PostcopyFD?

Yes, that would be possible - in this case I made that the job of
the device that had registered (i.e. the waker method) rather than
the core postcopy code.

> > Also note that a shared page might be shared between multiple other
> > programs - not just one.  In our case that could be two vhost-user
> > devices wired to two separate processes.
> 
> Yeah, but the idea still stands IMHO - we can notify only those
> PostcopyFDs that have faulted on the page already and skip the rest.
> For sure there can be more than one candidate for the wakeup, since
> there can be multiple PostcopyFDs that captured page fault on the same
> page (or even, same address).
> 
> > 
> > > But for the first version, I think it's fine.  And I believe if we
> > > maintain the faulted addresses we need some way to sync between the
> > > wake thread and fault thread too.
> > 
> > Hmm can you explain that a bit more?
> 
> Basically above was what I thought - to record the faulted addresses
> with specific PostcopyFD when page fault happened, then we may know
> which page(s) will a PostcopyFD need.  But when with that, we'll
> possibly need a lock to protect the information (or any other sync
> method).

OK, but I think you're suggesting building a whole new data structure to
know which ones need notifying;  that sounds like a lot of extra
complexity for not much gain.

Dave

> (Hope I didn't miss anything important along the way)
> 
> Thanks,
> 
> -- 
> Peter Xu
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK