From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41826) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YZsFy-0001jv-Ir for qemu-devel@nongnu.org; Sun, 22 Mar 2015 22:38:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YZsFr-0003zY-5Y for qemu-devel@nongnu.org; Sun, 22 Mar 2015 22:38:22 -0400 Received: from ozlabs.org ([103.22.144.67]:54326) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YZsFq-0003z1-Iu for qemu-devel@nongnu.org; Sun, 22 Mar 2015 22:38:15 -0400 Date: Mon, 23 Mar 2015 13:37:39 +1100 From: David Gibson Message-ID: <20150323023739.GH25043@voom.fritz.box> References: <1424883128-9841-1-git-send-email-dgilbert@redhat.com> <1424883128-9841-13-git-send-email-dgilbert@redhat.com> <20150310060824.GD11973@voom.redhat.com> <20150320181730.GI2468@work-vm> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="tT3UgwmDxwvOMqfu" Content-Disposition: inline In-Reply-To: <20150320181730.GI2468@work-vm> Subject: Re: [Qemu-devel] [PATCH v5 12/45] Return path: Source handling of return path List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" Cc: aarcange@redhat.com, yamahata@private.email.ne.jp, quintela@redhat.com, qemu-devel@nongnu.org, amit.shah@redhat.com, pbonzini@redhat.com, yanghy@cn.fujitsu.com --tT3UgwmDxwvOMqfu Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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" > > >=20 > > > Open a return path, and handle messages that are received upon it. > > >=20 > > > Signed-off-by: Dr. David Alan Gilbert > > > --- > > > include/migration/migration.h | 8 ++ > > > migration/migration.c | 178 ++++++++++++++++++++++++++++++++= +++++++++- > > > trace-events | 13 +++ > > > 3 files changed, 198 insertions(+), 1 deletion(-) > > >=20 > > > diff --git a/include/migration/migration.h b/include/migration/migrat= ion.h > > > index 6775747..5242ead 100644 > > > --- a/include/migration/migration.h > > > +++ b/include/migration/migration.h > > > @@ -73,6 +73,14 @@ struct MigrationState > > > =20 > > > 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; > > > } > > > =20 > > > +/* > > > + * 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) > > > } > > > } > > > =20 > > > +static void migrate_fd_cleanup_src_rp(MigrationState *ms) > > > +{ > > > + QEMUFile *rp =3D ms->rp_state.file; > > > + > > > + /* > > > + * When stuff goes wrong (e.g. failing destination) on the rp, i= t can get > > > + * cleaned up from a few threads; make sure not to do it twice i= n parallel > > > + */ > > > + rp =3D atomic_cmpxchg(&ms->rp_state.file, rp, NULL); > >=20 > > A cmpxchg seems dangerously subtle for such a basic and infrequent > > operation, but ok. >=20 > 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 =3D opaque; > > > @@ -369,6 +401,8 @@ static void migrate_fd_cleanup(void *opaque) > > > qemu_bh_delete(s->cleanup_bh); > > > s->cleanup_bh =3D NULL; > > > =20 > > > + 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 =3D migrate_get_current()->file; > > > trace_migrate_fd_cancel(); > > > =20 > > > + if (s->rp_state.file) { > > > + /* shutdown the rp socket, so causing the rp thread to shutd= own */ > > > + qemu_file_shutdown(s->rp_state.file); > >=20 > > I missed where qemu_file_shutdown() was implemented. Does this > > introduce a leftover socket dependency? >=20 > 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? > > > + } > > > + > > > do { > > > old_state =3D s->state; > > > if (old_state !=3D MIG_STATE_SETUP && old_state !=3D MIG_STA= TE_ACTIVE) { > > > @@ -658,8 +697,145 @@ int64_t migrate_xbzrle_cache_size(void) > > > return s->xbzrle_cache_size; > > > } > > > =20 > > > -/* migration thread support */ > > > +/* > > > + * Something bad happened to the RP stream, mark an error > > > + * The caller shall print something to indicate why > > > + */ > > > +static void source_return_path_bad(MigrationState *s) > > > +{ > > > + s->rp_state.error =3D true; > > > + migrate_fd_cleanup_src_rp(s); > > > +} > > > + > > > +/* > > > + * Handles messages sent on the return path towards the source VM > > > + * > > > + */ > > > +static void *source_return_path_thread(void *opaque) > > > +{ > > > + MigrationState *ms =3D opaque; > > > + QEMUFile *rp =3D ms->rp_state.file; > > > + uint16_t expected_len, header_len, header_com; > > > + const int max_len =3D 512; > > > + uint8_t buf[max_len]; > > > + uint32_t tmp32; > > > + int res; > > > + > > > + trace_source_return_path_thread_entry(); > > > + while (rp && !qemu_file_get_error(rp) && > > > + migration_already_active(ms)) { > > > + trace_source_return_path_thread_loop_top(); > > > + header_com =3D qemu_get_be16(rp); > > > + header_len =3D qemu_get_be16(rp); > > > + > > > + switch (header_com) { > > > + case MIG_RP_CMD_SHUT: > > > + case MIG_RP_CMD_PONG: > > > + expected_len =3D 4; > >=20 > > Could the knowledge of expected lengths be folded into the switch > > below? Switching twice on the same thing is a bit icky. >=20 > No, because the length at this point is used to valdiate the > length field in the header prior to reading the body. > The other switch processes the contents of the body that > have been read. Ok. > > > + break; > > > + > > > + default: > > > + error_report("RP: Received invalid cmd 0x%04x length 0x%= 04x", > > > + header_com, header_len); > > > + source_return_path_bad(ms); > > > + goto out; > > > + } > > > =20 > > > + if (header_len > expected_len) { > > > + error_report("RP: Received command 0x%04x with" > > > + "incorrect length %d expecting %d", > > > + header_com, header_len, > > > + expected_len); > > > + source_return_path_bad(ms); > > > + goto out; > > > + } > > > + > > > + /* We know we've got a valid header by this point */ > > > + res =3D qemu_get_buffer(rp, buf, header_len); > > > + if (res !=3D header_len) { > > > + trace_source_return_path_thread_failed_read_cmd_data(); > > > + source_return_path_bad(ms); > > > + goto out; > > > + } > > > + > > > + /* OK, we have the command and the data */ > > > + switch (header_com) { > > > + case MIG_RP_CMD_SHUT: > > > + tmp32 =3D be32_to_cpup((uint32_t *)buf); > > > + trace_source_return_path_thread_shut(tmp32); > > > + if (tmp32) { > > > + error_report("RP: Sibling indicated error %d", tmp32= ); > > > + source_return_path_bad(ms); > > > + } > > > + /* > > > + * We'll let the main thread deal with closing the RP > > > + * we could do a shutdown(2) on it, but we're the only u= ser > > > + * anyway, so there's nothing gained. > > > + */ > > > + goto out; > > > + > > > + case MIG_RP_CMD_PONG: > > > + tmp32 =3D be32_to_cpup((uint32_t *)buf); > > > + trace_source_return_path_thread_pong(tmp32); > > > + break; > > > + > > > + default: > > > + /* This shouldn't happen because we should catch this ab= ove */ > > > + trace_source_return_path_bad_header_com(); > > > + } > > > + /* Latest command processed, now leave a gap for the next on= e */ > > > + header_com =3D MIG_RP_CMD_INVALID; > >=20 > > This assignment will always get overwritten. >=20 > Thanks; gone - it's a left over from an old version. >=20 > > > + } > > > + if (rp && qemu_file_get_error(rp)) { > > > + trace_source_return_path_thread_bad_end(); > > > + source_return_path_bad(ms); > > > + } > > > + > > > + trace_source_return_path_thread_end(); > > > +out: > > > + return NULL; > > > +} > > > + > > > +__attribute__ (( unused )) /* Until later in patch series */ > > > +static int open_outgoing_return_path(MigrationState *ms) > >=20 > > Uh.. surely this should be open_incoming_return_path(); it's designed > > to be used on the source side, AFAICT. > >=20 > > > +{ > > > + > > > + ms->rp_state.file =3D qemu_file_get_return_path(ms->file); > > > + if (!ms->rp_state.file) { > > > + return -1; > > > + } > > > + > > > + trace_open_outgoing_return_path(); > > > + qemu_thread_create(&ms->rp_state.rp_thread, "return path", > > > + source_return_path_thread, ms, QEMU_THREAD_JO= INABLE); > > > + > > > + trace_open_outgoing_return_path_continue(); > > > + > > > + return 0; > > > +} > > > + > > > +__attribute__ (( unused )) /* Until later in patch series */ > > > +static void await_outgoing_return_path_close(MigrationState *ms) > >=20 > > Likewise "incoming" here, surely. >=20 > I've changed those two to open_source_return_path() which seems less am= biguous; > that OK? Uh.. not really, it just moves the ambiguity to a different place (is "source return path" the return path *on* the source or *to* the source). Perhaps "open_return_path_on_source" and "await_return_path_close_on_source"? I'm not particularly fond of those, but they're the best I've come up with yet. --=20 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 --tT3UgwmDxwvOMqfu Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVD3xzAAoJEGw4ysog2bOSJckP/3f3mf+6+i5xLn2/eOXkRSpW 4xG/RS+EP3b9w0RDQqWZt0KGzvAHdd9ivLEjDLGoLyGhKnfSCeguZ72cbTojFMvp TajE5Lb6ot2ecA4heKYJfY+x8X1TDoNWyH72jHn0hD7LFH7heib3Jo9HioEg1stT P4IK4baCoVGIIXGRYCgHaGuTwK81mppONRythCzcWiv/FRUPriGludcUmh1U1LZ+ aXvCJQ5I0vs1cScMZ4a8T3/mthWsrtjw2wMyYRAmbiYg+h81JFUHllgB1M6AIWzI JDVcpX45+kw1SY/H93sfMFYxm4s6Ey9Op5wWdv+SkFpdApqiRejgMfROe3yXn1ZR pJtoNw2sKsofvTYTZolrMk5sOros4pRLBSnaVwMSRcPQhPz8uSW8aVNUiuQwhaiA m6S/W1mpI5uFl+ewEof+r72vFZrmAfyKVeRdvhB2L1rhDNmUD/H6yIK4btLa65cg 4zJVOSSftgPE47lYLfaqCtlIouRDU4EYl++Uzt9BSWmZ+XZPlQaHpbvfAFqG3cI5 x5cSw+ttXCiSUuQ3jQ93wQ6e4/FqykqOmqR30E3BshcM9d2HQkv8Zx6zP4w2Nvam cp+W79albmVADJD+OuM4SHPYXIFWPoL/xNYyizH9XX8b4nzuS+oN4pvpwKrDhoDK cuYB6CmVyyO7k+sH0GK1 =cb2M -----END PGP SIGNATURE----- --tT3UgwmDxwvOMqfu--