On Wed, Apr 01, 2015 at 04:14:05PM +0100, Dr. David Alan Gilbert wrote:
> * David Gibson (david@gibson.dropbear.id.au) wrote:
> > On Fri, Mar 20, 2015 at 06:17:31PM +0000, Dr. David Alan Gilbert wrote:
> > > * David Gibson (david@gibson.dropbear.id.au) wrote:
> > > > On Wed, Feb 25, 2015 at 04:51:35PM +0000, Dr. David Alan Gilbert (git) wrote:
> > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > > > 
> > > > > Open a return path, and handle messages that are received upon it.
> > > > > 
> > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > > ---
> > > > >  include/migration/migration.h |   8 ++
> > > > >  migration/migration.c         | 178 +++++++++++++++++++++++++++++++++++++++++-
> > > > >  trace-events                  |  13 +++
> > > > >  3 files changed, 198 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/include/migration/migration.h b/include/migration/migration.h
> > > > > index 6775747..5242ead 100644
> > > > > --- a/include/migration/migration.h
> > > > > +++ b/include/migration/migration.h
> > > > > @@ -73,6 +73,14 @@ struct MigrationState
> > > > >  
> > > > >      int state;
> > > > >      MigrationParams params;
> > > > > +
> > > > > +    /* State related to return path */
> > > > > +    struct {
> > > > > +        QEMUFile     *file;
> > > > > +        QemuThread    rp_thread;
> > > > > +        bool          error;
> > > > > +    } rp_state;
> > > > > +
> > > > >      double mbps;
> > > > >      int64_t total_time;
> > > > >      int64_t downtime;
> > > > > diff --git a/migration/migration.c b/migration/migration.c
> > > > > index 80d234c..34cd4fe 100644
> > > > > --- a/migration/migration.c
> > > > > +++ b/migration/migration.c
> > > > > @@ -237,6 +237,23 @@ MigrationCapabilityStatusList *qmp_query_migrate_capabilities(Error **errp)
> > > > >      return head;
> > > > >  }
> > > > >  
> > > > > +/*
> > > > > + * Return true if we're already in the middle of a migration
> > > > > + * (i.e. any of the active or setup states)
> > > > > + */
> > > > > +static bool migration_already_active(MigrationState *ms)
> > > > > +{
> > > > > +    switch (ms->state) {
> > > > > +    case MIG_STATE_ACTIVE:
> > > > > +    case MIG_STATE_SETUP:
> > > > > +        return true;
> > > > > +
> > > > > +    default:
> > > > > +        return false;
> > > > > +
> > > > > +    }
> > > > > +}
> > > > > +
> > > > >  static void get_xbzrle_cache_stats(MigrationInfo *info)
> > > > >  {
> > > > >      if (migrate_use_xbzrle()) {
> > > > > @@ -362,6 +379,21 @@ static void migrate_set_state(MigrationState *s, int old_state, int new_state)
> > > > >      }
> > > > >  }
> > > > >  
> > > > > +static void migrate_fd_cleanup_src_rp(MigrationState *ms)
> > > > > +{
> > > > > +    QEMUFile *rp = ms->rp_state.file;
> > > > > +
> > > > > +    /*
> > > > > +     * When stuff goes wrong (e.g. failing destination) on the rp, it can get
> > > > > +     * cleaned up from a few threads; make sure not to do it twice in parallel
> > > > > +     */
> > > > > +    rp = atomic_cmpxchg(&ms->rp_state.file, rp, NULL);
> > > > 
> > > > A cmpxchg seems dangerously subtle for such a basic and infrequent
> > > > operation, but ok.
> > > 
> > > I'll take other suggestions; but I'm trying to just do
> > > 'if the qemu_file still exists close it', and it didn't seem
> > > worth introducing another state variable to atomically update
> > > when we've already got the file pointer itself.
> > 
> > Yes, I see the rationale.  My concern is just that the more atomicity
> > mechanisms are scattered through the code, the harder it is to analyze
> > and be sure you haven't missed race cases (or introduced then with a
> > future change).
> > 
> > In short, I prefer to see a simple-as-possible, and preferably
> > documented, consistent overall concurrency scheme for a data
> > structure, rather than scattered atomic ops for various variable where
> > it's difficult to see how all the pieces might relate together.
> > 
> > > > > +    if (rp) {
> > > > > +        trace_migrate_fd_cleanup_src_rp();
> > > > > +        qemu_fclose(rp);
> > > > > +    }
> > > > > +}
> > > > > +
> > > > >  static void migrate_fd_cleanup(void *opaque)
> > > > >  {
> > > > >      MigrationState *s = opaque;
> > > > > @@ -369,6 +401,8 @@ static void migrate_fd_cleanup(void *opaque)
> > > > >      qemu_bh_delete(s->cleanup_bh);
> > > > >      s->cleanup_bh = NULL;
> > > > >  
> > > > > +    migrate_fd_cleanup_src_rp(s);
> > > > > +
> > > > >      if (s->file) {
> > > > >          trace_migrate_fd_cleanup();
> > > > >          qemu_mutex_unlock_iothread();
> > > > > @@ -406,6 +440,11 @@ static void migrate_fd_cancel(MigrationState *s)
> > > > >      QEMUFile *f = migrate_get_current()->file;
> > > > >      trace_migrate_fd_cancel();
> > > > >  
> > > > > +    if (s->rp_state.file) {
> > > > > +        /* shutdown the rp socket, so causing the rp thread to shutdown */
> > > > > +        qemu_file_shutdown(s->rp_state.file);
> > > > 
> > > > I missed where qemu_file_shutdown() was implemented.  Does this
> > > > introduce a leftover socket dependency?
> > > 
> > > No, it shouldn't.  The shutdown() causes a shutdown(2) syscall to
> > > be issued on the socket stopping anything blocking on it; it then
> > > gets closed at the end after the rp thread has exited.
> > 
> > 
> > Sorry, that's not what I meant.  I mean is this a hole in the
> > abstraction of the QemuFile, because it assumes that what you're
> > dealing with here is indeed a socket, rather than something else?
> 
> It's just a dependency that we have a shutdown method on the qemu_file
> we're using; if it's not a socket then whatever it is, if we're going
> to use it for a rp then it needs to implement something equivalent.

Um, yeah, except I don't think most file types really have an
operation that's semantically similar to socket shutdown.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson