From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41835) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YZsFz-0001lj-Vl for qemu-devel@nongnu.org; Sun, 22 Mar 2015 22:38:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YZsFr-0003zZ-5N for qemu-devel@nongnu.org; Sun, 22 Mar 2015 22:38:23 -0400 Received: from ozlabs.org ([2401:3900:2:1::2]:55310) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YZsFq-0003z2-Ik for qemu-devel@nongnu.org; Sun, 22 Mar 2015 22:38:15 -0400 Date: Mon, 23 Mar 2015 13:20:42 +1100 From: David Gibson Message-ID: <20150323022042.GF25043@voom.fritz.box> References: <1424883128-9841-1-git-send-email-dgilbert@redhat.com> <1424883128-9841-24-git-send-email-dgilbert@redhat.com> <20150313012652.GB11973@voom.redhat.com> <20150313111905.GC2486@work-vm> <20150316062355.GG5741@voom.redhat.com> <20150318175951.GL2355@work-vm> <20150319041830.GU5741@voom.redhat.com> <20150319093330.GA2409@work-vm> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="rMWmSaSbD7nr+du9" Content-Disposition: inline In-Reply-To: <20150319093330.GA2409@work-vm> Subject: Re: [Qemu-devel] [PATCH v5 23/45] migrate_start_postcopy: Command to trigger transition to postcopy 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 --rMWmSaSbD7nr+du9 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Mar 19, 2015 at 09:33:31AM +0000, Dr. David Alan Gilbert wrote: > * David Gibson (david@gibson.dropbear.id.au) wrote: > > On Wed, Mar 18, 2015 at 05:59:51PM +0000, Dr. David Alan Gilbert wrote: > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > On Fri, Mar 13, 2015 at 11:19:06AM +0000, Dr. David Alan Gilbert wr= ote: > > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > > On Wed, Feb 25, 2015 at 04:51:46PM +0000, Dr. David Alan Gilber= t (git) wrote: > > > > > > > From: "Dr. David Alan Gilbert" > > > > > > >=20 > > > > > > > Once postcopy is enabled (with migrate_set_capability), the m= igration > > > > > > > will still start on precopy mode. To cause a transition into= postcopy > > > > > > > the: > > > > > > >=20 > > > > > > > migrate_start_postcopy > > > > > > >=20 > > > > > > > command must be issued. Postcopy will start sometime after t= his > > > > > > > (when it's next checked in the migration loop). > > > > > > >=20 > > > > > > > Issuing the command before migration has started will error, > > > > > > > and issuing after it has finished is ignored. > > > > > > >=20 > > > > > > > Signed-off-by: Dr. David Alan Gilbert > > > > > > > Reviewed-by: Eric Blake > > > > > > > --- > > > > > > > hmp-commands.hx | 15 +++++++++++++++ > > > > > > > hmp.c | 7 +++++++ > > > > > > > hmp.h | 1 + > > > > > > > include/migration/migration.h | 3 +++ > > > > > > > migration/migration.c | 22 ++++++++++++++++++++++ > > > > > > > qapi-schema.json | 8 ++++++++ > > > > > > > qmp-commands.hx | 19 +++++++++++++++++++ > > > > > > > 7 files changed, 75 insertions(+) > > > > > > >=20 > > > > > > > diff --git a/hmp-commands.hx b/hmp-commands.hx > > > > > > > index e37bc8b..03b8b78 100644 > > > > > > > --- a/hmp-commands.hx > > > > > > > +++ b/hmp-commands.hx > > > > > > > @@ -985,6 +985,21 @@ Enable/Disable the usage of a capability= @var{capability} for migration. > > > > > > > ETEXI > > > > > > > =20 > > > > > > > { > > > > > > > + .name =3D "migrate_start_postcopy", > > > > > > > + .args_type =3D "", > > > > > > > + .params =3D "", > > > > > > > + .help =3D "Switch migration to postcopy mode", > > > > > > > + .mhandler.cmd =3D hmp_migrate_start_postcopy, > > > > > > > + }, > > > > > > > + > > > > > > > +STEXI > > > > > > > +@item migrate_start_postcopy > > > > > > > +@findex migrate_start_postcopy > > > > > > > +Switch in-progress migration to postcopy mode. Ignored after= the end of > > > > > > > +migration (or once already in postcopy). > > > > > > > +ETEXI > > > > > > > + > > > > > > > + { > > > > > > > .name =3D "client_migrate_info", > > > > > > > .args_type =3D "protocol:s,hostname:s,port:i?,tls-p= ort:i?,cert-subject:s?", > > > > > > > .params =3D "protocol hostname port tls-port cer= t-subject", > > > > > > > diff --git a/hmp.c b/hmp.c > > > > > > > index b47f331..df9736c 100644 > > > > > > > --- a/hmp.c > > > > > > > +++ b/hmp.c > > > > > > > @@ -1140,6 +1140,13 @@ void hmp_migrate_set_capability(Monito= r *mon, const QDict *qdict) > > > > > > > } > > > > > > > } > > > > > > > =20 > > > > > > > +void hmp_migrate_start_postcopy(Monitor *mon, const QDict *q= dict) > > > > > > > +{ > > > > > > > + Error *err =3D NULL; > > > > > > > + qmp_migrate_start_postcopy(&err); > > > > > > > + hmp_handle_error(mon, &err); > > > > > > > +} > > > > > > > + > > > > > > > void hmp_set_password(Monitor *mon, const QDict *qdict) > > > > > > > { > > > > > > > const char *protocol =3D qdict_get_str(qdict, "protocol= "); > > > > > > > diff --git a/hmp.h b/hmp.h > > > > > > > index 4bb5dca..da1334f 100644 > > > > > > > --- a/hmp.h > > > > > > > +++ b/hmp.h > > > > > > > @@ -64,6 +64,7 @@ void hmp_migrate_set_downtime(Monitor *mon,= const QDict *qdict); > > > > > > > void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict); > > > > > > > void hmp_migrate_set_capability(Monitor *mon, const QDict *q= dict); > > > > > > > void hmp_migrate_set_cache_size(Monitor *mon, const QDict *q= dict); > > > > > > > +void hmp_migrate_start_postcopy(Monitor *mon, const QDict *q= dict); > > > > > > > void hmp_set_password(Monitor *mon, const QDict *qdict); > > > > > > > void hmp_expire_password(Monitor *mon, const QDict *qdict); > > > > > > > void hmp_eject(Monitor *mon, const QDict *qdict); > > > > > > > diff --git a/include/migration/migration.h b/include/migratio= n/migration.h > > > > > > > index e6a814a..293c83e 100644 > > > > > > > --- a/include/migration/migration.h > > > > > > > +++ b/include/migration/migration.h > > > > > > > @@ -104,6 +104,9 @@ struct MigrationState > > > > > > > int64_t xbzrle_cache_size; > > > > > > > int64_t setup_time; > > > > > > > int64_t dirty_sync_count; > > > > > > > + > > > > > > > + /* Flag set once the migration has been asked to enter p= ostcopy */ > > > > > > > + bool start_postcopy; > > > > > > > }; > > > > > > > =20 > > > > > > > void process_incoming_migration(QEMUFile *f); > > > > > > > diff --git a/migration/migration.c b/migration/migration.c > > > > > > > index a4fc7d7..43ca656 100644 > > > > > > > --- a/migration/migration.c > > > > > > > +++ b/migration/migration.c > > > > > > > @@ -372,6 +372,28 @@ void qmp_migrate_set_capabilities(Migrat= ionCapabilityStatusList *params, > > > > > > > } > > > > > > > } > > > > > > > =20 > > > > > > > +void qmp_migrate_start_postcopy(Error **errp) > > > > > > > +{ > > > > > > > + MigrationState *s =3D migrate_get_current(); > > > > > > > + > > > > > > > + if (!migrate_postcopy_ram()) { > > > > > > > + error_setg(errp, "Enable postcopy with migration_set= _capability before" > > > > > > > + " the start of migration"); > > > > > > > + return; > > > > > > > + } > > > > > > > + > > > > > > > + if (s->state =3D=3D MIG_STATE_NONE) { > > > > > > > + error_setg(errp, "Postcopy must be started after mig= ration has been" > > > > > > > + " started"); > > > > > > > + return; > > > > > > > + } > > > > > > > + /* > > > > > > > + * we don't error if migration has finished since that w= ould be racy > > > > > > > + * with issuing this command. > > > > > > > + */ > > > > > > > + atomic_set(&s->start_postcopy, true); > > > > > >=20 > > > > > > Why atomic_set? > > > > >=20 > > > > > It's being read by the migration thread, this is happening in the= main thread. > > > > >=20 > > > > > There's no strict ordering requirement or anything. > > > >=20 > > > > I don't think you need an atomic then. AFAIK an atomic_set() in > > > > isolation without some sort of atomic on the other side is pretty m= uch > > > > meaningless. > > >=20 > > > The other side has an atomic_read: > > >=20 > > > if (migrate_postcopy_ram() && > > > s->state !=3D MIGRATION_STATUS_POSTCOPY_ACTIVE && > > > pend_nonpost <=3D max_size && > > > atomic_read(&s->start_postcopy)) { > > >=20 > > > if (!postcopy_start(s, &old_vm_running)) { > > > current_active_type =3D MIGRATION_STATUS_POST= COPY_ACTIVE; > > > entered_postcopy =3D true; > > > } > > >=20 > > > so it is at least symmetric. > >=20 > > But still pointless. Atomicity isn't magic pixie dust; it only makes > > sense if you're making atomic specific operations that need to be. > > Simple integer loads and stores are already atomic. Unless at least > > some of the atomic operations are something more complex, there's > > really no point to atomically marked operations. >=20 > OK, I'll kill it off. >=20 > It'll work in practice, but I still believe that what you're saying isn't > safe C: > 1) There's no barrier after the write, so there's no guarantee the oth= er > thread will eventually see it (in practice we've got other pthread = ops > we take so we will get a barrier somewhere, and most CPUs eventually > do propagate the store). Sorry, I should have been clearer. If you need a memory barrier, by all means include a memory barrier. But that should be explicit: atomic set/read operations often include barriers, but it's not obvious which side will include what barrier. > 2) The read side could legally be optimised out of the loop by the com= piler. > (but in practice wont be because compilers won't optimise that far). That one's a trickier question. Compilers are absolutely capable of optimizing that far, *but* the C rules about when it's allowed to assume in-memory values remain unchanged are pretty conservative. I think any function call in the loop will require it to reload the value, for example. That said, a (compiler only) memory barrier might be appropriate to ensure that reload. --=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 --rMWmSaSbD7nr+du9 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVD3h6AAoJEGw4ysog2bOS3y4P/1wKp07P5cBBmNsdQdNNHvD4 sKRx7V8/z5gR+n7NaWX/pdgi6AfrPUV4ZuXS4b5tNGGt1JrGfXFDCaMiKxN3Xv1c cnGQ0BlJMm/exhJ07J7exPEIKSK7pzI+9UoZtOh3oLcVCQ2AV0vxBZ8chS6KSjf5 O7kptVOqo0cXIIpQGLclrWXKBl8vsE6/Q7NEiKJgX8JjuoLdMp88w+/WObX+WYZ+ nuFPf0UHvDqhWD8mqLSUqArDVP1KBxg6HiVRyd5Htq3qnkluNeu5OAQedtUafrx6 FLEMv5NvdwEOIMVbXVCpgBaauN5Jy+PYLzLPbKsKZnnmoFFpOPck5zaSmsrMZ7nG /DrWjW0lyT34j7HrAzoHqKP7cQAiwt5NskvWQGOUr80G2qhsRSVbBAOiyo1o7QGK 0D7f9UIxSgjIlT5hA6uF0nH6fkDP8YS+9PUjSJ2h1QKmcuA+uyFL2B+PwnHV+uKv rw5nsyX+5nToT2YjGRq1bupCLE3+OXrvqXEVdCJWWDH4CKfOjwwA9Dyio0AjaS7Y E2eAUFGkcC0h8YAnkqtDkPGCdt+7B+NstzYWLS3lKfcijsYTei08UkB0cCbJlvX+ o2aThBXlYTK2odgvOViu9CypbO/pbmBFU+alYejuumXr8MjzikLZDvpUJHQeUFoK conuu+WdTdjv1oktGW3w =PE4k -----END PGP SIGNATURE----- --rMWmSaSbD7nr+du9--