[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 12/45] Return path: Source handling of return
From: |
David Gibson |
Subject: |
Re: [Qemu-devel] [PATCH v5 12/45] Return path: Source handling of return path |
Date: |
Tue, 7 Apr 2015 13:07:07 +1000 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Wed, Apr 01, 2015 at 04:14:05PM +0100, Dr. David Alan Gilbert wrote:
> * David Gibson (address@hidden) wrote:
> > On Fri, Mar 20, 2015 at 06:17:31PM +0000, Dr. David Alan Gilbert wrote:
> > > * David Gibson (address@hidden) wrote:
> > > > On Wed, Feb 25, 2015 at 04:51:35PM +0000, Dr. David Alan Gilbert (git)
> > > > wrote:
> > > > > From: "Dr. David Alan Gilbert" <address@hidden>
> > > > >
> > > > > Open a return path, and handle messages that are received upon it.
> > > > >
> > > > > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > > > > ---
> > > > > 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
pgpHm6s5WXFkc.pgp
Description: PGP signature