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

[Peter Xu <peterx@redhat.com>]

On Mon, Mar 05, 2018 at 07:55:13PM +0000, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > On Fri, Feb 16, 2018 at 01:16:16PM +0000, Dr. David Alan Gilbert (git) wrote:
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > 
> > > 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 <dgilbert@redhat.com>
> > > ---
> > >  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?

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?

> 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).

(Hope I didn't miss anything important along the way)

Thanks,

-- 
Peter Xu