From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55058) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bsLyi-0001Ln-Vx for qemu-devel@nongnu.org; Thu, 06 Oct 2016 23:37:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bsLyf-00082w-PN for qemu-devel@nongnu.org; Thu, 06 Oct 2016 23:37:44 -0400 Date: Fri, 7 Oct 2016 14:25:52 +1100 From: David Gibson Message-ID: <20161007032552.GN18490@umbus.fritz.box> References: <1475519097-27611-1-git-send-email-duanj@linux.vnet.ibm.com> <1475519097-27611-5-git-send-email-duanj@linux.vnet.ibm.com> <20161005165638.GC11921@work-vm> <20161006190156.GE3087@work-vm> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="TXIPBuAs4GDcsx9K" Content-Disposition: inline In-Reply-To: <20161006190156.GE3087@work-vm> Subject: Re: [Qemu-devel] [QEMU PATCH v5 4/6] migration: migrate QTAILQ List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" Cc: Jianjun Duan , qemu-devel@nongnu.org, qemu-ppc@nongnu.org, dmitry@daynix.com, peter.maydell@linaro.org, kraxel@redhat.com, mst@redhat.com, pbonzini@redhat.com, veroniabahaa@gmail.com, quintela@redhat.com, amit.shah@redhat.com, mreitz@redhat.com, kwolf@redhat.com, rth@twiddle.net, aurelien@aurel32.net, leon.alrae@imgtec.com, blauwirbel@gmail.com, mark.cave-ayland@ilande.co.uk, mdroth@linux.vnet.ibm.com --TXIPBuAs4GDcsx9K Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Oct 06, 2016 at 08:01:56PM +0100, Dr. David Alan Gilbert wrote: > * Jianjun Duan (duanj@linux.vnet.ibm.com) wrote: > >=20 > >=20 > > On 10/05/2016 09:56 AM, Dr. David Alan Gilbert wrote: > > > * Jianjun Duan (duanj@linux.vnet.ibm.com) wrote: > > >> Currently we cannot directly transfer a QTAILQ instance because of t= he > > >> limitation in the migration code. Here we introduce an approach to > > >> transfer such structures. In our approach such a structure is tagged > > >> with VMS_LINKED. We then modified vmstate_save_state and vmstate_loa= d_state > > >> so that when VMS_LINKED is encountered, put and get from VMStateInfo= are > > >> called respectively. We created VMStateInfo vmstate_info_qtailq for = QTAILQ. > > >> Similar VMStateInfo can be created for other data structures such as= list. > > >> This approach will be used to transfer pending_events and ccs_list i= n spapr > > >> state. > > >> > > >> We also create some macros in qemu/queue.h to access a QTAILQ using = pointer > > >> arithmetic. This ensures that we do not depend on the implementation > > >> details about QTAILQ in the migration code. > > >=20 > > > I think we're going to need a way to have a more flexible > > > loops; and thus my choice here wouldn't be to use the .get/.put toget= her > > > with the VMSD; but I think we'll end up needing a new > > > data structure, maybe a VMStateLoop *loop in VMStateField. > > >=20 > > > So would it be easier if you added that new member, then you wouldn't= have to > > > modify every get() and put() function that already exists in the prev= ious patch. > > >=20 > > > Specifically, your format of QTAILQ is perfectly reasonable - a > > > byte before each entry which is 1 to indicate there's an entry or 0 > > > to indicate termination, but there are lots of other variants, e.g. > > >=20 > > > a) put_scsi_requests uses that byte to hold a flag, so it's 0,1,2 > > > 0 still means terminate but 1 or 2 set a flag in the structure. > >=20 > > I quickly take a look of put_scsi_requests. It is transferring a QTAILQ= of > > SCSIRequest. However it goes into the structure inside to dump the > > elements out. > > If using my approach, I would have a VMSD for SCSIRequest. The > > additional byte used to indicate the end of the queue would lie outside > > the SCSCIRequest data block, so there would be no confusion. >=20 > Hmm OK; I don't think it's that easy but we'll see. >=20 > However, can I make one much simpler request; please split this patch > so that the VMSTATE_LINKED and vmstate_save_state/vmstate_load_state/vmfi= eld_get_type_name > are in one patch, while the QTAILQ patches are in a separate patch. > (I'd be OK if you moved the VMSTATE_LINKED into the previous patch). >=20 > I've just been thinking about a different use for the same mechanism; > I want to do a: > VMSTATE_WITH_TMP(t1*, type1, type2, vmsd) >=20 > which also sets the LINKED, where the .get/.put allocate a temporary > structure (of type/size type2), set up *tmp =3D t1 and then do the vmstat= e_load/save > using the vmsd on the temporary; something like (untested): >=20 > static int get_tmp(QEMUFile *f, void *pv, size_t unused_size, VMStateFiel= d *field) > { > const VMStateDescription *vmsd =3D field->vmsd; > size_t size =3D field->size; > int version_id =3D field->version_id; > void *tmp =3D gmalloc(size); > int ret; > =20 > *(void **)tmp =3D pv; > ret =3D vmstate_load_state(f, vmsd, tmp, version_id); > gfree(tmp); > return ret; > } >=20 > This can be in a generic macro; and we would impose that type2 must be a = struct > with the first element is 'type1* parent' (compile checked). > This would work nicely for where we have to do some maths to generate some > temporary results prior to migration; the .pre_save of the vmsd can read = the data > from pv->parent and write it to the other fields but not have to use > qemu_get_*/qemu_put_* at all. >=20 > Dave Oh, I like this idea. I know there are a number of places where should-be-obsolete fields are still present in structures purely to catch incoming migration info which is then converted to the modern representation in post_load. This would allow cleaning a bunch of those up. It would also mean we don't necessarily need explicit handling of queues/lists. I objected to early versions of this series which dumped the qtailq into an array and used the existing array vmstate types, because it meant not just an only-for-migration field in the structure, but a substantial slab of only-for-migration data. If we added the concept of temporary "catching" structures to the vmsd, that objection would go away. I'd be happy enough to temporarily dump the queue into an array, transfer that over the stream into another temporary array, then load it into the destination queue. --=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 --TXIPBuAs4GDcsx9K Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJX9xW/AAoJEGw4ysog2bOS+V0P/2ZV/bJPpEu7300SGTlDla/J 6aiINLNT++UG+9qoiKXvqUTxMrrua1dH02+I/oBoO74P3AJUnjqBYhW0Rg6+ulUS 2+nB3ezX+rzaVMJNxq5IZe+cdcY/ZQkAKfQJl1NhUetRnYvjsowc514yGLQssrDC n9lQkytsZzaB6kzVNpuUYUlVYXy3SkXDHC8bM9acspnq/pHTWx0pMiRKFEcDXmKc Vx+ZaKiJK7NvPF04E5FbRSf9w2ToKgxQAYpXCnL4sMd8vjYh+s+rHSwGnfg92av9 geuh+3penEFiwJzYqZgF2tbtMXwZ8ZRL6zfircLDBhimheMdvXXIWtHsFo3CFqaW m4gudGW4/WJqoXAVotSgSWU0se8mfRKuj5WBB0nujG4o4j6fUGKIafHlBNHdL1Oo 4IvomprCxiYP+Sn9LJu0Hp7PcM6m8Cdoim0hRXP4DNZvze7ORqa2GYMMemUHd0Xg FX0pgEaYU3hX7pB4P+WGKoBaUVbod+WeN5dqGCw0bupUYjK34lbUGfKg+jqpTdkm r4BWa2dYUnUTAlbFKrqjt+JQvXUwoFnzN2Iky9kcfWYf91qgMFnOnddaUH3APYzN pbeajE8Y1TwT2rKAvZwwllzMqa67PBIqzLc4HCA4TN4bDaIbh3D2uKScRg+2oNjY 73tzGrIal7ONRN3lddkI =eLa2 -----END PGP SIGNATURE----- --TXIPBuAs4GDcsx9K--