From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34620) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbLig-0005zd-VM for qemu-devel@nongnu.org; Fri, 27 Mar 2015 00:18:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YbLif-0002O1-A9 for qemu-devel@nongnu.org; Fri, 27 Mar 2015 00:18:06 -0400 Received: from ozlabs.org ([2401:3900:2:1::2]:52402) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbLie-0002Nr-NE for qemu-devel@nongnu.org; Fri, 27 Mar 2015 00:18:05 -0400 Date: Fri, 27 Mar 2015 14:56:26 +1100 From: David Gibson Message-ID: <20150327035626.GC2900@voom.fritz.box> References: <20150313101953.GA2486@work-vm> <20150316061840.GE5741@voom.redhat.com> <20150320123759.GE2468@work-vm> <20150323022542.GG25043@voom.fritz.box> <20150324200414.GG2332@work-vm> <20150324223227.GK25043@voom.fritz.box> <20150325150024.GH2313@work-vm> <20150325164010.GI2313@work-vm> <20150326013508.GF28039@voom.redhat.com> <20150326114435.GB2370@work-vm> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Pk6IbRAofICFmK5e" Content-Disposition: inline In-Reply-To: <20150326114435.GB2370@work-vm> Subject: Re: [Qemu-devel] [PATCH v5 20/45] Modify savevm handlers for 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 --Pk6IbRAofICFmK5e Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Mar 26, 2015 at 11:44:36AM +0000, Dr. David Alan Gilbert wrote: > * David Gibson (david@gibson.dropbear.id.au) wrote: > > On Wed, Mar 25, 2015 at 04:40:11PM +0000, Dr. David Alan Gilbert wrote: > > > * Dr. David Alan Gilbert (dgilbert@redhat.com) wrote: > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > On Tue, Mar 24, 2015 at 08:04:14PM +0000, Dr. David Alan Gilbert = wrote: > > > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > > > On Fri, Mar 20, 2015 at 12:37:59PM +0000, Dr. David Alan Gilb= ert wrote: > > > > > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > > > > > On Fri, Mar 13, 2015 at 10:19:54AM +0000, Dr. David Alan = Gilbert wrote: > > > > > > > > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > > > > > > > > On Wed, Feb 25, 2015 at 04:51:43PM +0000, Dr. David A= lan Gilbert (git) wrote: > > > > > > > > > > > > From: "Dr. David Alan Gilbert" > > > > > > > > > > > >=20 > > > > > > > > > > > > Modify save_live_pending to return separate postcop= iable and > > > > > > > > > > > > non-postcopiable counts. > > > > > > > > > > > >=20 > > > > > > > > > > > > Add 'can_postcopy' to allow a device to state if it= can postcopy > > > > > > > > > > >=20 > > > > > > > > > > > What's the purpose of the can_postcopy callback? The= re are no callers > > > > > > > > > > > in this patch - is it still necessary with the change= to > > > > > > > > > > > save_live_pending? > > > > > > > > > >=20 > > > > > > > > > > The patch 'qemu_savevm_state_complete: Postcopy changes= ' uses > > > > > > > > > > it in qemu_savevm_state_postcopy_complete and qemu_save= vm_state_complete > > > > > > > > > > to decide which devices must be completed at that point. > > > > > > > > >=20 > > > > > > > > > Couldn't they check for non-zero postcopiable state from > > > > > > > > > save_live_pending instead? > > > > > > > >=20 > > > > > > > > That would be a bit weird. > > > > > > > >=20 > > > > > > > > At the moment for each device we call the: > > > > > > > > save_live_setup method (from qemu_savevm_state_begin) > > > > > > > >=20 > > > > > > > > 0...multiple times we call: > > > > > > > > save_live_pending > > > > > > > > save_live_iterate > > > > > > > >=20 > > > > > > > > and then we always call > > > > > > > > save_live_complete > > > > > > > >=20 > > > > > > > >=20 > > > > > > > > To my mind we have to call save_live_complete for any device > > > > > > > > that we've called save_live_setup on (maybe it allocated so= mething > > > > > > > > in _setup that it clears up in _complete). > > > > > > > >=20 > > > > > > > > save_live_pending could perfectly well return 0 remaining a= t the end of > > > > > > > > the migrate for our device, and thus if we used that then w= e wouldn't > > > > > > > > call save_live_complete. > > > > > > >=20 > > > > > > > Um.. I don't follow. I was suggesting that at the precopy->p= ostcopy > > > > > > > transition point you call save_live_complete for everything t= hat > > > > > > > reports 0 post-copiable state. > > > > > > >=20 > > > > > > >=20 > > > > > > > Then again, a different approach would be to split the > > > > > > > save_live_complete hook into (possibly NULL) "complete precop= y" and > > > > > > > "complete postcopy" hooks. The core would ensure that every = chunk of > > > > > > > state has both completion hooks called (unless NULL). That m= ight also > > > > > > > address my concerns about the no longer entirely accurate > > > > > > > save_live_complete function name. > > > > > >=20 > > > > > > OK, that one I prefer. Are you OK with: > > > > > > qemu_savevm_state_complete_precopy > > > > > > calls -> save_live_complete_precopy > > > > > >=20 > > > > > > qemu_savevm_state_complete_postcopy > > > > > > calls -> save_live_complete_postcopy > > > > > >=20 > > > > > > ? > > > > >=20 > > > > > Sounds ok to me. Fwiw, I was thinking that both the complete_pre= copy > > > > > and complete_postcopy hooks should always be called. For a > > > > > non-postcopy migration, the postcopy hooks would just be called > > > > > immediately after the precopy hooks. > > > >=20 > > > > OK, I've made the change as described in my last mail; but I haven'= t called > > > > the complete_postcopy hook in the precopy case. If it was as simpl= e as making > > > > all devices use one or the other then it would work, however there = are > > > > existing (precopy) assumptions about ordering of device state on th= e wire that > > > > I want to be careful not to alter; for example RAM must come first = is the one > > > > I know. > > >=20 > > > Actually, I spoke too soon; testing this found a bad breakage. > > >=20 > > > the functions in savevm.c add the per-section headers, and then call = the _complete > > > methods on the devices. Those _complete methods can't elect to do no= thing, because > > > a header has already been planted. > >=20 > > Hrm.. couldn't you move the test for presence of the hook earlier so > > you don't sent the header if the hook is NULL? >=20 > There's two tests that you have to make: > a) in qemu_savevm_state_complete_precopy do you call save_live_comp= lete_precopy > b) in qemu_savevm_state_complete_postcopy do you call save_live_com= plete_postcopy >=20 > The obvious case is if either hook is NULL you don't call it. > (a) is the harder cases, if we're doing postcopy then we don't want to ca= ll the > save_live_complete_precopy method on a device which isn't expecting to= complete until > postcopy. Uh.. no, I'm expecting the complete_precopy hook to be called every time if it's non-NULL. If it's a postcopy item that doesn't expect to finish at the end of precopy, it should put its code in the complete_postcopy hook instead. That should be fine for the non-postcopy case too, because the postcopy_complete hook will be called momentarily. > The code in qemu_savevm_state_complete_precopy checks for the presence= of the *postcopy* > hook, and doesn't emit the header or call the precopy commit if the po= stcopy hook > is present and we're in postcopy. >=20 > > > I've ended up with something between the two; we still have a comple= te_precopy and > > > complete_postcopy method on the devices; if the complete_postcopy met= hod exists and > > > we're in postcopy mode, the complete_precopy method isn't called at a= ll. > > > A device could decide to do something different in complete_postcopy = =66rom complete_precopy > > > but it must do something to complete the section. > > > Effectively the presence of the complete_postcopy is now doing what > > > can_postcopy() used to do. > >=20 > > Hmm.. but it means there's no per-device hook for the precopy to > > postcopy transition point. I'm not sure if that might matter. >=20 > This is true, but if we needed a generic hook for that (which might be us= eful) > it probably shouldn't be 'complete'. Well, I'm thinking of these as "state (complete precopy)" rather than "(state complete) precopy". But yeah, a different name might be less ambiguous. --=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 --Pk6IbRAofICFmK5e Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVFNTqAAoJEGw4ysog2bOSO6UP/RE0S1wARibsrOKD3wZs86LI iGZrCfZFFdLYf9JpPb8JXx61g4M8s8LW7iFTHuGVHjVlbW2OU8zvx9YexyBsAb5l y2+vgWnH/+9BgNRqN55DDOhhLzsgmBErrsxX896Mg46te5Dah3u4LE91gw6+P8qn De+o4NU9fP8ezb4V3NHI0Q77tGk/4x6KcyC611vb0MO0bH+UU8bUMtznrqrCtDgy AD8g5PKjB8SgYqCV1mjn/wiWl71sJbuHSMaMwb1SYOns41K2TbUpYqmkt3XvvHch igKyZMtg3Btx0Hob1p+LnYaPZ2u+i92PlxJgGQCCVIa8qWcZV5b1sm0mES7nilMi 372pvp63ZoHicRnOKp2v4wEgaA8nOVU73Hb4l7gCz4f86VgvHTEvYqP8YB5k0++k V++lSBPnR3SyqM7wPLeMiVXPNmtI3dYn5VDyIuHoWbzNSkjqjCzkcbtoCzqHEYhd 47uI2NynUOdgrRfLAo3Se7TrF+xtdeJczqRyBQNC+7iTc7BsOYFsvG3Q7BQBTT/B SGQDUgtKyWVhbqGGpzMQlDrAk8roYlrWP914Rkvy/uyDoS65vunRG6P+pyyV7LWD 13YOshT+tytcjReiutRzxSWH4sD04oCmsOkgJiMxzX1XHAJATRQM5tuZyuH+JrIC BmmTbFT6BAJqJyT30R6r =jid2 -----END PGP SIGNATURE----- --Pk6IbRAofICFmK5e--