From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58922) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YcR1g-0005nm-1E for qemu-devel@nongnu.org; Mon, 30 Mar 2015 00:10:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YcR1e-00045j-Ap for qemu-devel@nongnu.org; Mon, 30 Mar 2015 00:10:11 -0400 Received: from ozlabs.org ([2401:3900:2:1::2]:45645) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YcR1d-00045D-O8 for qemu-devel@nongnu.org; Mon, 30 Mar 2015 00:10:10 -0400 Date: Mon, 30 Mar 2015 15:03:11 +1100 From: David Gibson Message-ID: <20150330040311.GK9908@voom.fritz.box> References: <1424883128-9841-1-git-send-email-dgilbert@redhat.com> <1424883128-9841-18-git-send-email-dgilbert@redhat.com> <20150312093049.GW11973@voom.redhat.com> <20150326163327.GG2370@work-vm> <20150327041353.GD2900@voom.fritz.box> <20150327104851.GC2685@work-vm> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="wAI/bQb0EMvlZCHl" Content-Disposition: inline In-Reply-To: <20150327104851.GC2685@work-vm> Subject: Re: [Qemu-devel] [PATCH v5 17/45] Add wrappers and handlers for sending/receiving the postcopy-ram migration messages. 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 --wAI/bQb0EMvlZCHl Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Mar 27, 2015 at 10:48:52AM +0000, Dr. David Alan Gilbert wrote: > * David Gibson (david@gibson.dropbear.id.au) wrote: > > On Thu, Mar 26, 2015 at 04:33:28PM +0000, Dr. David Alan Gilbert wrote: > > > (Only replying to some of the items in this mail - the others I'll get > > > to another time). [snip] > > > > DISCARD will typically immediately precede LISTEN, won't it? Is th= ere > > > > a reason not to put the discard data into the LISTEN command? > > >=20 > > > Discard data can be quite large, so I potentially send multiple disca= rd > > > commands. > > > (Also as you can tell generally I've got a preference for one message= does one > > > thing, and thus I have tried to keep them separate). > >=20 > > So, I like the idea of one message per action in principle - but only > > if that action really is well-defined without reference to what > > operations come before and after it. If there are hidden dependencies > > about what actions have to come in what order, I'd rather bake that > > into the command structure. >=20 > In no way is it hidden; the commands match the state transitions. Not all of them. In particular DISCARD's relation to state transitions is not at all obvious. Likewise I don't think the connection of LISTEN to the internal state machines at each end is terribly clear. > > > > > +static int loadvm_postcopy_handle_advise(MigrationIncomingState = *mis, > > > > > + uint64_t remote_hps, > > > > > + uint64_t remote_tps) > > > > > +{ > > > > > + PostcopyState ps =3D postcopy_state_get(mis); > > > > > + trace_loadvm_postcopy_handle_advise(); > > > > > + if (ps !=3D POSTCOPY_INCOMING_NONE) { > > > > > + error_report("CMD_POSTCOPY_ADVISE in wrong postcopy stat= e (%d)", ps); > > > > > + return -1; > > > > > + } > > > > > + > > > > > + if (remote_hps !=3D getpagesize()) { > > > > > + /* > > > > > + * Some combinations of mismatch are probably possible b= ut it gets > > > > > + * a bit more complicated. In particular we need to pla= ce whole > > > > > + * host pages on the dest at once, and we need to ensure= that we > > > > > + * handle dirtying to make sure we never end up sending = part of > > > > > + * a hostpage on it's own. > > > > > + */ > > > > > + error_report("Postcopy needs matching host page sizes (s= =3D%d d=3D%d)", > > > > > + (int)remote_hps, getpagesize()); > > > > > + return -1; > > > > > + } > > > > > + > > > > > + if (remote_tps !=3D (1ul << qemu_target_page_bits())) { > > > > > + /* > > > > > + * Again, some differences could be dealt with, but for = now keep it > > > > > + * simple. > > > > > + */ > > > > > + error_report("Postcopy needs matching target page sizes = (s=3D%d d=3D%d)", > > > > > + (int)remote_tps, 1 << qemu_target_page_bits= ()); > > > > > + return -1; > > > > > + } > > > > > + > > > > > + postcopy_state_set(mis, POSTCOPY_INCOMING_ADVISE); > > > >=20 > > > > Should you be checking the return value here to make sure it's still > > > > POSTCOPY_INCOMING_NONE? Atomic xchgs seem overkill if you still ha= ve > > > > a race between the fetch at the top and the set here. > > > >=20 > > > > Or, in fact, should you be just doing an atomic exchange-then-check= at > > > > the top, rather than checking at the top, then changing at the bott= om. > > >=20 > > > There's no race at this point yet; going from None->advise we still o= nly > > > have one thread. The check at the top is a check against protocol > > > violations (e.g. getting two advise or something like that). > >=20 > > Yeah.. and having to have intimate knowledge of the thread structure > > at each point in order to analyze the atomic operation correctness is > > exactly what bothers me about it. >=20 > No, you don't; By always using the postcopy_state_set you don't need > to worry about the thread structure or protocol to understand the atomic = operation > correctness. The only reason you got into that comparison is because > you worried that it might be overkill in this case; by keeping it consist= ent > like it already is then you don't need to understand the thread structure. You really do, though. You may be using the same function in the original, but not the same idiom: in this case you don't check the return value and deal with it accordingly. Because of that, this looks pretty much exactly like the classic "didn't understand what was atomic in the atomic" race condition, and you need to know that there's only one thread at this point to realize it's correct after all. --=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 --wAI/bQb0EMvlZCHl Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVGMr/AAoJEGw4ysog2bOSPlwP/1qPGTsZZ0mxCZ6hPYIXGYCN 7HFuMF3i0hbaHR2PIIrje0XLGTa45idIDzazFIFOnhnSaABSxEDqurI959iY3603 OPXvKApLopp1InL8Z8OZRFujbZJh5+hFMIx02Hes1e3hGkqu7kUoYVWIB11kWAED ziSyMMMJBEVVitkwDM34hdx4QHdmOHmyfAweIGKEDyWQ+43edWB3fxL3DZHSNIwk ZOqybM9xC3kK3lh7+LNQb70kNVhW1lJgtLKwb/pd8Cz6VwTNBTnA5Oipq9UhAp6Q vMstAAkvo9PUyvBVyRjfaPET1dYZT7twYwTHGeRImrfU2dc1A0CRCaSlVkW+qfJL tJZJtGwVkWo9vePZ+cnotT4fU3+tobVLzR3etbZeM+Hv3RV954/+FsYIPGT4PMD+ oOlzRpv+IvItVjxBwtWTjQANOIIBOUYeXiTjGoK/lrF8kUs9M6n464V6Bf0TPkqZ 6M6MQpMk9VwfgX7ktuwNJzLvxNUjjd2W2eINc/Pn2ul8m0NKwaqFRyIbJRw8eG7j qmEu59UZil8ApxXo9i/vbKY5cbZic83lRCgPqbsH2ua35uV4DfghX2WyIRjsmJHq a0KH5dRrpXkioFN343OuH66W+lOLo+y86LkoWjR1K4yvZpeC+fWUEVKcKFksIINC mXZl6qIPZpTdLSzJOkqp =bfXv -----END PGP SIGNATURE----- --wAI/bQb0EMvlZCHl--