From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48010) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGLIE-0003xu-7w for qemu-devel@nongnu.org; Wed, 28 Jan 2015 00:35:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YGLI3-0001Vz-E2 for qemu-devel@nongnu.org; Wed, 28 Jan 2015 00:35:58 -0500 Received: from ozlabs.org ([2401:3900:2:1::2]:49158) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGLI2-0001VY-QM for qemu-devel@nongnu.org; Wed, 28 Jan 2015 00:35:47 -0500 Date: Wed, 28 Jan 2015 16:36:04 +1100 From: David Gibson Message-ID: <20150128053604.GD14681@voom> References: <1412358473-31398-1-git-send-email-dgilbert@redhat.com> <1412358473-31398-44-git-send-email-dgilbert@redhat.com> <20141113031039.GE7291@voom.fritz.box> <20141217182131.GF31071@work-vm> <20150127045055.GC19081@voom.fritz.box> <20150127100430.GB2425@work-vm> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="n/aVsWSeQ4JHkrmm" Content-Disposition: inline In-Reply-To: <20150127100430.GB2425@work-vm> Subject: Re: [Qemu-devel] [PATCH v4 43/47] Host page!=target page: Cleanup bitmaps 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, cristian.klein@cs.umu.se, qemu-devel@nongnu.org, amit.shah@redhat.com, yanghy@cn.fujitsu.com --n/aVsWSeQ4JHkrmm Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jan 27, 2015 at 10:04:31AM +0000, Dr. David Alan Gilbert wrote: > * David Gibson (david@gibson.dropbear.id.au) wrote: > > On Wed, Dec 17, 2014 at 06:21:34PM +0000, Dr. David Alan Gilbert wrote: > > > * David Gibson (david@gibson.dropbear.id.au) wrote: > > > > On Fri, Oct 03, 2014 at 06:47:49PM +0100, Dr. David Alan Gilbert (g= it) wrote: > > > > > From: "Dr. David Alan Gilbert" > > > > >=20 > > > > > Prior to the start of postcopy, ensure that everything that will > > > > > be transferred later is a whole host-page in size. > > > > >=20 > > > > > This is accomplished by discarding partially transferred host pag= es > > > > > and marking any that are partially dirty as fully dirty. > > > > >=20 > > > > > Signed-off-by: Dr. David Alan Gilbert > > > > > --- > > > > > arch_init.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++= +++++++++++++- > > > > > 1 file changed, 111 insertions(+), 1 deletion(-) > > > > >=20 > > > > > diff --git a/arch_init.c b/arch_init.c > > > > > index 1fe4fab..aac250c 100644 > > > > > --- a/arch_init.c > > > > > +++ b/arch_init.c > > > > > @@ -1024,7 +1024,6 @@ static uint32_t get_32bits_map(unsigned lon= g *map, int64_t start) > > > > > * A helper to put 32 bits into a bit map; trivial for HOST_LONG= _BITS=3D32 > > > > > * messier for 64; the bitmaps are actually long's that are 32 o= r 64bit > > > > > */ > > > > > -__attribute__ (( unused )) /* Until later in patch series */ > > > > > static void put_32bits_map(unsigned long *map, int64_t start, > > > > > uint32_t v) > > > > > { > > > > > @@ -1153,15 +1152,126 @@ static int pc_each_ram_discard(Migration= State *ms) > > > > > } > > > > > =20 > > > > > /* > > > > > + * Utility for the outgoing postcopy code. > > > > > + * > > > > > + * Discard any partially sent host-page size chunks, mark any pa= rtially > > > > > + * dirty host-page size chunks as all dirty. > > > > > + * > > > > > + * Returns: 0 on success > > > > > + */ > > > > > +static int postcopy_chunk_hostpages(MigrationState *ms) > > > > > +{ > > > > > + struct RAMBlock *block; > > > > > + unsigned int host_bits =3D sysconf(_SC_PAGESIZE) / TARGET_PA= GE_SIZE; > > > > > + uint32_t host_mask; > > > > > + > > > > > + /* Should be a power of 2 */ > > > > > + assert(host_bits && !(host_bits & (host_bits - 1))); > > > > > + /* > > > > > + * If the host_bits isn't a division of 32 (the minimum long= size) > > > > > + * then the code gets a lot more complex; disallow for now > > > > > + * (I'm not aware of a system where it's true anyway) > > > > > + */ > > > > > + assert((32 % host_bits) =3D=3D 0); > > > >=20 > > > > This assert makes the first one redundant. > > >=20 > > > True I guess, removed the power of 2 check. > > >=20 > > > > > >=20 > > > > > +/* > > > > > * Transmit the set of pages to be discarded after precopy to th= e target > > > > > * these are pages that have been sent previously but have been = dirtied > > > > > * Hopefully this is pretty sparse > > > > > */ > > > > > int ram_postcopy_send_discard_bitmap(MigrationState *ms) > > > > > { > > > > > + int ret; > > > > > + > > > > > /* This should be our last sync, the src is now paused */ > > > > > migration_bitmap_sync(); > > > > > =20 > > > > > + /* Deal with TPS !=3D HPS */ > > > > > + ret =3D postcopy_chunk_hostpages(ms); > > > > > + if (ret) { > > > > > + return ret; > > > > > + } > > > >=20 > > > > This really seems like a bogus thing to be doing on the outgoing > > > > migration side. Doesn't the host page size constraint come from the > > > > destination (due to the need to atomically instate pages). Source > > > > host page size =3D=3D destination host page size doesn't seem like = it > > > > should be an inherent constraint > > >=20 > > > It's not an inherent constraint; it just makes life messier. I had > > > some code to deal with it but it complicates things even more, and > > > I've not got anything to test that rare case with; if someone is > > > desperate for it then it can be added. > >=20 > > So, I'm all for deferring implementation improvements that we don't > > need for the time being. > >=20 > > What worries me though, is having the source have to make assumptions > > about how the migration stream will be processed on the destination > > that aren't somehow baked into the protocol itself. i.e. I think we > > should really try to avoid the possibility of migration streams that > > are structurally sound, and look like they should be valid, but > > aren't, because of subtle constraints in the order and manner in which > > the destination needs to process the individual chunks. >=20 > Agreed; see below. >=20 > > > > and it's not clear why you can't do > > > > this rounding out to host page sized chunks on the receive end. > > >=20 > > > The source keeps track of which pages still need sending, and so > > > has to update that list when it tells the destination to perform > > > a discard. > >=20 > > Ah. > >=20 > > > If the destination discards more than the source told it to (for > > > example because it has bigger host-pages) the source would need > > > to update it's map of the pages that still need sending. > >=20 > > I'm beginning to wonder if what we really need is for early in the > > migration process the destination to tell the host what granularity of > > updates it can handle (based on its page size). > >=20 > > Perhaps the short summary is that I don't think we need to actually > > handle the case of different source and dest host page sizes. BUT, > > if that does happen the migration process should be able to detect > > that that's what's gone wrong and print a meaningful error, rather > > than having the destination blow up part way through and deep the code > > because chunk constraints necessary for the dest host page size > > haven't been met by the source. >=20 > Right; I cut the problem in the opposite direction and made the source > send the destination its page sizes in the 'advise' message and > the destination validates it and spits a: > Postcopy needs matching host page sizes (s=3D%d d=3D%d)" >=20 > (That's in 21/47 Add wrappers and handlers....) > It's just a little easier to do it that way rather than having to > make the source wait for the destination. I guess that's ok. It's still a bit ugly, because it means if we were ever to support different source and dest host page sizes, it would necessarily require a protocol change (unless I've missed something). But at least the errors would be obvious. --=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 --n/aVsWSeQ4JHkrmm Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUyHVEAAoJEGw4ysog2bOSi3AP+wbAsSIRZ+yDW29Js3U5ha0a dfAm3KMSYE9FFjs0wK3ewHfjLMjn0XDwG5IlcqBQgo07aJ4mOe7ukYaRt5DlgBh6 AU6nHyOd3Lgh33yjSL45Aar/vwq8T6hT9UCHmRl23iQzkvDtx1/LTZ6TxEchFDZr 2zLj/5twi8LRB/Qz98A4ahoTPmFf9hSrIxQmrRNGiTL7m4oitxAKeRPrVawiHucp jrsMQgso1WhX6ZMLsS7iZuNz03hA2vFQ70CpLKZ9qsqbOnfZebFIe7xPhRUU6sJ3 OUlWr0DA/yQTylgN3EKjagU2Q0Zpn3hmIKsOkMQlRPlzYvQGGyLeMfAUVfHzl5XW txjdFu2tuKsZ4bpPVmOnWBuI0Cj1CwIJfKePiCxDTlCsc+HhZepOxrtrnC9m04+y qOSgsi5sVnxwhYCTtqNX+tohfQj2d8N9jAB+kbBSzKYKK788PqfFSIv3z8/OC9uw Jjut0maFFLKwJb11N1OVFtjrEt1UgfcQ/MkBvBVwUxCeNvpteEaMzT39FcT4dG4q sRu7UUjLaUfm+c70PT3IZkk+36C0ODlKj4sskXJ9zHzmwrCJ1MQ+c5VtbmQFpHLM cMNpPZnEz0t4EEcmBmNjk03ksdMcSQ05QQF9ECiJ0h5Ng+wbkGmhrJlCrvDiKZIS wugLLB2wf3o/EWebQu1H =j3zT -----END PGP SIGNATURE----- --n/aVsWSeQ4JHkrmm--