From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47313) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bsDvn-000679-F7 for qemu-devel@nongnu.org; Thu, 06 Oct 2016 15:02:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bsDvh-0004fa-BA for qemu-devel@nongnu.org; Thu, 06 Oct 2016 15:02:10 -0400 Date: Thu, 6 Oct 2016 20:01:56 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20161006190156.GE3087@work-vm> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: 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: Jianjun Duan Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, dmitry@daynix.com, peter.maydell@linaro.org, kraxel@redhat.com, mst@redhat.com, david@gibson.dropbear.id.au, 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 * 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 the > >> 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_load_= state > >> so that when VMS_LINKED is encountered, put and get from VMStateInfo a= re > >> called respectively. We created VMStateInfo vmstate_info_qtailq for QT= AILQ. > >> Similar VMStateInfo can be created for other data structures such as l= ist. > >> This approach will be used to transfer pending_events and ccs_list in = spapr > >> state. > >> > >> We also create some macros in qemu/queue.h to access a QTAILQ using po= inter > >> 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 together > > 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 h= ave to > > modify every get() and put() function that already exists in the previo= us 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. Hmm OK; I don't think it's that easy but we'll see. However, can I make one much simpler request; please split this patch so that the VMSTATE_LINKED and vmstate_save_state/vmstate_load_state/vmfiel= d_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). 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) 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 vmstate_= load/save using the vmsd on the temporary; something like (untested): static int get_tmp(QEMUFile *f, void *pv, size_t unused_size, VMStateField = *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; } This can be in a generic macro; and we would impose that type2 must be a st= ruct 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 th= e data =66rom pv->parent and write it to the other fields but not have to use qemu_get_*/qemu_put_* at all. Dave >=20 > > b) slirp_state_load also uses a null byte termination but not off a = QTAILQ > > (although I think it could be flipped for one) (it uses '42' for = the > > non-0 value, but looks like it could become 1) >=20 > > c) virtio_blk also rolls it's own linked list but again with the 0/1= byte > >=20 > > Now how would I modify your QTAILQ load/store to do (a) without copyi= ng the whole > > thing? > >=20 > > Dave > >=20 > >> > >> Signed-off-by: Jianjun Duan > >> --- > >> include/migration/vmstate.h | 26 ++++++++++++++++++ > >> include/qemu/queue.h | 32 ++++++++++++++++++++++ > >> migration/trace-events | 4 +++ > >> migration/vmstate.c | 66 ++++++++++++++++++++++++++++++++++++= +++++++++ > >> 4 files changed, 128 insertions(+) > >> > >> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h > >> index 459dd4a..e60c994 100644 > >> --- a/include/migration/vmstate.h > >> +++ b/include/migration/vmstate.h > >> @@ -186,6 +186,12 @@ enum VMStateFlags { > >> * to determine the number of entries in the array. Only valid in > >> * combination with one of VMS_VARRAY*. */ > >> VMS_MULTIPLY_ELEMENTS =3D 0x4000, > >> + /* For fields which need customized handling, such as QTAILQ in q= ueue.h. > >> + * When this flag is set in VMStateField, info->get/put will > >> + * be used in vmstate_load/save_state instead of recursive call. > >> + * User should implement set info to handle the concerned data st= ructure. > >> + */ > >> + VMS_LINKED =3D 0x8000, > >> }; > >> =20 > >> struct VMStateField { > >> @@ -246,6 +252,7 @@ extern const VMStateInfo vmstate_info_timer; > >> extern const VMStateInfo vmstate_info_buffer; > >> extern const VMStateInfo vmstate_info_unused_buffer; > >> extern const VMStateInfo vmstate_info_bitmap; > >> +extern const VMStateInfo vmstate_info_qtailq; > >> =20 > >> #define type_check_2darray(t1,t2,n,m) ((t1(*)[n][m])0 - (t2*)0) > >> #define type_check_array(t1,t2,n) ((t1(*)[n])0 - (t2*)0) > >> @@ -657,6 +664,25 @@ extern const VMStateInfo vmstate_info_bitmap; > >> .offset =3D offsetof(_state, _field), = \ > >> } > >> =20 > >> +/* For QTAILQ that need customized handling > >> + * _type: type of QTAILQ element > >> + * _next: name of QTAILQ entry field in QTAILQ element > >> + * _vmsd: VMSD for QTAILQ element > >> + * size: size of QTAILQ element > >> + * start: offset of QTAILQ entry in QTAILQ element > >> + */ > >> +#define VMSTATE_QTAILQ_V(_field, _state, _version, _vmsd, _type, _nex= t) \ > >> +{ = \ > >> + .name =3D (stringify(_field)), = \ > >> + .version_id =3D (_version), = \ > >> + .vmsd =3D &(_vmsd), = \ > >> + .size =3D sizeof(_type), = \ > >> + .info =3D &vmstate_info_qtailq, = \ > >> + .flags =3D VMS_LINKED, = \ > >> + .offset =3D offsetof(_state, _field), = \ > >> + .start =3D offsetof(_type, _next), = \ > >> +} > >> + > >> /* _f : field name > >> _f_n : num of elements field_name > >> _n : num of elements > >> diff --git a/include/qemu/queue.h b/include/qemu/queue.h > >> index 342073f..12c3f80 100644 > >> --- a/include/qemu/queue.h > >> +++ b/include/qemu/queue.h > >> @@ -438,4 +438,36 @@ struct { = \ > >> #define QTAILQ_PREV(elm, headname, field) \ > >> (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last)) > >> =20 > >> +/* > >> + * Offsets of layout of a tail queue head. > >> + */ > >> +#define QTAILQ_FIRST_OFFSET 0 > >> +#define QTAILQ_LAST_OFFSET (sizeof(void *)) > >> + > >> +/* > >> + * Offsets of layout of a tail queue element. > >> + */ > >> +#define QTAILQ_NEXT_OFFSET 0 > >> +#define QTAILQ_PREV_OFFSET (sizeof(void *)) > >> + > >> +/* > >> + * Tail queue tranversal using pointer arithmetic. > >> + */ > >> +#define QTAILQ_RAW_FOREACH(elm, head, entry) = \ > >> + for ((elm) =3D *((void **) ((char *) (head) + QTAILQ_FIRST_OF= FSET)); \ > >> + (elm); = \ > >> + (elm) =3D = \ > >> + *((void **) ((char *) (elm) + (entry) + QTAILQ_NEXT_= OFFSET))) > >> +/* > >> + * Tail queue insertion using pointer arithmetic. > >> + */ > >> +#define QTAILQ_RAW_INSERT_TAIL(head, elm, entry) do { = \ > >> + *((void **) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET)) = =3D NULL; \ > >> + *((void **) ((char *) (elm) + (entry) + QTAILQ_PREV_OFFSET)) = =3D \ > >> + *((void **) ((char *) (head) +QTAILQ_LAST_OFFSET)); = \ > >> + **((void ***)((char *) (head) +QTAILQ_LAST_OFFSET)) =3D (elm)= ; \ > >> + *((void **) ((char *) (head) + QTAILQ_LAST_OFFSET)) =3D = \ > >> + (void *) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET);= \ > >> +} while (/*CONSTCOND*/0) > >> + > >> #endif /* QEMU_SYS_QUEUE_H */ > >> diff --git a/migration/trace-events b/migration/trace-events > >> index dfee75a..9a6ec59 100644 > >> --- a/migration/trace-events > >> +++ b/migration/trace-events > >> @@ -52,6 +52,10 @@ vmstate_n_elems(const char *name, int n_elems) "%s:= %d" > >> vmstate_subsection_load(const char *parent) "%s" > >> vmstate_subsection_load_bad(const char *parent, const char *sub, con= st char *sub2) "%s: %s/%s" > >> vmstate_subsection_load_good(const char *parent) "%s" > >> +get_qtailq(const char *name, int version_id) "%s v%d" > >> +get_qtailq_end(const char *name, const char *reason, int val) "%s %s/= %d" > >> +put_qtailq(const char *name, int version_id) "%s v%d" > >> +put_qtailq_end(const char *name, const char *reason) "%s %s" > >> =20 > >> # migration/qemu-file.c > >> qemu_file_fclose(void) "" > >> diff --git a/migration/vmstate.c b/migration/vmstate.c > >> index 66802cb..192db8a 100644 > >> --- a/migration/vmstate.c > >> +++ b/migration/vmstate.c > >> @@ -5,7 +5,9 @@ > >> #include "migration/vmstate.h" > >> #include "qemu/bitops.h" > >> #include "qemu/error-report.h" > >> +#include "qemu/queue.h" > >> #include "trace.h" > >> +#include "migration/qjson.h" > >> =20 > >> static void vmstate_subsection_save(QEMUFile *f, const VMStateDescrip= tion *vmsd, > >> void *opaque, QJSON *vmdesc); > >> @@ -121,6 +123,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateD= escription *vmsd, > >> if (field->flags & VMS_STRUCT) { > >> ret =3D vmstate_load_state(f, field->vmsd, addr, > >> field->vmsd->version_id); > >> + } else if (field->flags & VMS_LINKED) { > >> + ret =3D field->info->get(f, addr, size, field); > >> } else { > >> ret =3D field->info->get(f, addr, size, NULL); > >> =20 > >> @@ -193,6 +197,8 @@ static const char *vmfield_get_type_name(VMStateFi= eld *field) > >> =20 > >> if (field->flags & VMS_STRUCT) { > >> type =3D "struct"; > >> + } else if (field->flags & VMS_LINKED) { > >> + type =3D "linked"; > >> } else if (field->info->name) { > >> type =3D field->info->name; > >> } > >> @@ -327,6 +333,8 @@ void vmstate_save_state(QEMUFile *f, const VMState= Description *vmsd, > >> } > >> if (field->flags & VMS_STRUCT) { > >> vmstate_save_state(f, field->vmsd, addr, vmdesc_l= oop); > >> + } else if (field->flags & VMS_LINKED) { > >> + field->info->put(f, addr, size, field, vmdesc_loo= p); > >> } else { > >> field->info->put(f, addr, size, NULL, NULL); > >> } > >> @@ -939,3 +947,61 @@ const VMStateInfo vmstate_info_bitmap =3D { > >> .get =3D get_bitmap, > >> .put =3D put_bitmap, > >> }; > >> + > >> +/*get for QTAILQ */ > >> +static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size, > >> + VMStateField *field) > >> +{ > >> + int ret =3D 0; > >> + const VMStateDescription *vmsd =3D field->vmsd; > >> + size_t size =3D field->size; > >> + size_t entry =3D field->start; > >> + int version_id =3D field->version_id; > >> + void *elm; > >> + > >> + trace_get_qtailq(vmsd->name, version_id); > >> + if (version_id > vmsd->version_id) { > >> + trace_get_qtailq_end(vmsd->name, "too new", -EINVAL); > >=20 > > Can you make those error_report's please - if it fails we want to > > see why in the log. > >=20 > > Dave > >=20 > >> + return -EINVAL; > >> + } > >> + if (version_id < vmsd->minimum_version_id) { > >> + trace_get_qtailq_end(vmsd->name, "too old", -EINVAL); > >> + return -EINVAL; > >> + } > >> + > >> + while (qemu_get_byte(f)) { > >> + elm =3D g_malloc(size); > >> + ret =3D vmstate_load_state(f, vmsd, elm, version_id); > >> + if (ret) { > >> + return ret; > >> + } > >> + QTAILQ_RAW_INSERT_TAIL(pv, elm, entry); > >> + } > >> + > >> + trace_get_qtailq_end(vmsd->name, "end", ret); > >> + return ret; > >> +} > >> + > >> +/* put for QTAILQ */ > >> +static void put_qtailq(QEMUFile *f, void *pv, size_t unused_size, > >> + VMStateField *field, QJSON *vmdesc) > >> +{ > >> + const VMStateDescription *vmsd =3D field->vmsd; > >> + size_t entry =3D field->start; > >> + void *elm; > >> + > >> + trace_put_qtailq(vmsd->name, vmsd->version_id); > >> + > >> + QTAILQ_RAW_FOREACH(elm, pv, entry) { > >> + qemu_put_byte(f, true); > >> + vmstate_save_state(f, vmsd, elm, vmdesc); > >> + } > >> + qemu_put_byte(f, false); > >> + > >> + trace_put_qtailq_end(vmsd->name, "end"); > >> +} > >> +const VMStateInfo vmstate_info_qtailq =3D { > >> + .name =3D "qtailq", > >> + .get =3D get_qtailq, > >> + .put =3D put_qtailq, > >> +}; > >> --=20 > >> 1.9.1 > >> > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > >=20 > Thanks, > Jianjun >=20 >=20 -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK