From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50388) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bzRhr-0005nX-0n for qemu-devel@nongnu.org; Wed, 26 Oct 2016 13:09:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bzRhm-0005Bl-Fq for qemu-devel@nongnu.org; Wed, 26 Oct 2016 13:09:38 -0400 Date: Wed, 26 Oct 2016 18:09:26 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20161026170926.GD6456@work-vm> References: <1477428463-10569-1-git-send-email-duanj@linux.vnet.ibm.com> <1477428463-10569-3-git-send-email-duanj@linux.vnet.ibm.com> <20161026162906.GA6456@work-vm> <20161026165328.GC6456@work-vm> <26c90413-bc4f-e23b-3d9d-08dc665f60ee@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <26c90413-bc4f-e23b-3d9d-08dc665f60ee@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [QEMU PATCH v8 2/3] 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: > > > On 10/26/2016 09:53 AM, Dr. David Alan Gilbert wrote: > > * Jianjun Duan (duanj@linux.vnet.ibm.com) wrote: > >> > >> > >> On 10/26/2016 09:29 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. 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 in 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. > >>>> > >>>> Signed-off-by: Jianjun Duan > >>>> --- > >>>> include/migration/vmstate.h | 20 ++++++++++++++ > >>>> include/qemu/queue.h | 46 +++++++++++++++++++++++++++++++ > >>>> migration/trace-events | 4 +++ > >>>> migration/vmstate.c | 67 +++++++++++++++++++++++++++++++++++++++++++++ > >>>> 4 files changed, 137 insertions(+) > >>>> > >>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h > >>>> index d0e37b5..318a6f1 100644 > >>>> --- a/include/migration/vmstate.h > >>>> +++ b/include/migration/vmstate.h > >>>> @@ -251,6 +251,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; > >>>> > >>>> #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) > >>>> @@ -662,6 +663,25 @@ extern const VMStateInfo vmstate_info_bitmap; > >>>> .offset = offsetof(_state, _field), \ > >>>> } > >>>> > >>>> +/* For QTAILQ that need customized handling. > >>>> + * Target QTAILQ needs be properly initialized. > >>>> + * _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, _next) \ > >>>> +{ \ > >>>> + .name = (stringify(_field)), \ > >>>> + .version_id = (_version), \ > >>>> + .vmsd = &(_vmsd), \ > >>>> + .size = sizeof(_type), \ > >>>> + .info = &vmstate_info_qtailq, \ > >>>> + .offset = offsetof(_state, _field), \ > >>>> + .start = 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..e9378fa 100644 > >>>> --- a/include/qemu/queue.h > >>>> +++ b/include/qemu/queue.h > >>>> @@ -438,4 +438,50 @@ struct { \ > >>>> #define QTAILQ_PREV(elm, headname, field) \ > >>>> (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last)) > >>>> > >>>> +#define RAW_FIELD(base, offset) \ > >>>> + ((char *) (base) + offset) > >>>> + > >>>> +/* > >>>> + * Offsets of layout of a tail queue head. > >>>> + */ > >>>> +#define QTAILQ_FIRST_OFFSET 0 > >>>> +#define QTAILQ_LAST_OFFSET (sizeof(void *)) > >>> > >>> OK, you might want to add some asserts somewhere in a .c just to catch if > >>> any of these offsets change. > >>> > >> if the layout of QTAILQ changes, the version id for the vmsd should also > >> be changed. It will break migration between different versions. However > >> the operation doesn't depend on these values. > > > > No, changing layout of QTAILQ doesn't need to change the version id of vmsd; > > it's an internal change. But if someone does make the change and forgets > > to update your OFFSET macros it'll cause memory corruption. > > You could catch that with an assert (possibly build time). > > > If we use these constant I would agree an assertion is necessary. By > using a macro we leave the door open for change. and if someone changes > the layout, he or she better change the macros and the version id. If an > assertion is added, then that assertion needs to be changed to reflect > the change, then in the unlikely situations we could have several > version of layout/macro/assertions floating around. That is too much. SO > version id is the best guard here. Version id's are irrelevant here. The macros are irrelevant here. I'm talking about a potential mismatch between the definition of QTAILQ_LAST_OFFSET and the definition of Q_TAILQ_HEAD. Dave > > Thanks, > Jianjun > > >>>> + * Raw access of elements of a tail queue > >>>> + */ > >>>> +#define QTAILQ_RAW_FIRST(head) \ > >>>> + (*((void **) (RAW_FIELD(head, QTAILQ_FIRST_OFFSET)))) > >>>> +#define QTAILQ_RAW_LAST(head) \ > >>>> + (*((void ***) (RAW_FIELD(head, QTAILQ_LAST_OFFSET)))) > >>>> + > >>>> +/* > >>>> + * Offsets of layout of a tail queue element. > >>>> + */ > >>>> +#define QTAILQ_NEXT_OFFSET 0 > >>>> +#define QTAILQ_PREV_OFFSET (sizeof(void *)) > >>>> + > >>>> +/* > >>>> + * Raw access of elements of a tail entry > >>>> + */ > >>>> +#define QTAILQ_RAW_NEXT(elm, entry) \ > >>>> + (*((void **) (RAW_FIELD(elm, entry + QTAILQ_NEXT_OFFSET)))) > >>>> +#define QTAILQ_RAW_PREV(elm, entry) \ > >>>> + (*((void ***) (RAW_FIELD(elm, entry + QTAILQ_PREV_OFFSET)))) > >>>> +/* > >>>> + * Tail queue tranversal using pointer arithmetic. > >>>> + */ > >>>> +#define QTAILQ_RAW_FOREACH(elm, head, entry) \ > >>>> + for ((elm) = QTAILQ_RAW_FIRST(head); \ > >>>> + (elm); \ > >>>> + (elm) = QTAILQ_RAW_NEXT(elm, entry)) > >>>> +/* > >>>> + * Tail queue insertion using pointer arithmetic. > >>>> + */ > >>>> +#define QTAILQ_RAW_INSERT_TAIL(head, elm, entry) do { \ > >>>> + QTAILQ_RAW_NEXT(elm, entry) = NULL; \ > >>>> + QTAILQ_RAW_PREV(elm, entry) = QTAILQ_RAW_LAST(head); \ > >>>> + *QTAILQ_RAW_LAST(head) = (elm); \ > >>>> + QTAILQ_RAW_LAST(head) = &QTAILQ_RAW_NEXT(elm, entry); \ > >>>> +} 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, const 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" > >>>> > >>>> # migration/qemu-file.c > >>>> qemu_file_fclose(void) "" > >>>> diff --git a/migration/vmstate.c b/migration/vmstate.c > >>>> index d188afa..fcf808e 100644 > >>>> --- a/migration/vmstate.c > >>>> +++ b/migration/vmstate.c > >>>> @@ -5,6 +5,7 @@ > >>>> #include "migration/vmstate.h" > >>>> #include "qemu/bitops.h" > >>>> #include "qemu/error-report.h" > >>>> +#include "qemu/queue.h" > >>>> #include "trace.h" > >>>> #include "migration/qjson.h" > >>>> > >>>> @@ -948,3 +949,69 @@ const VMStateInfo vmstate_info_bitmap = { > >>>> .get = get_bitmap, > >>>> .put = put_bitmap, > >>>> }; > >>>> + > >>>> +/* get for QTAILQ > >>>> + * meta data about the QTAILQ is encoded in a VMStateField structure > >>>> + */ > >>>> +static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size, > >>>> + VMStateField *field) > >>>> +{ > >>>> + int ret = 0; > >>>> + const VMStateDescription *vmsd = field->vmsd; > >>>> + /* size of a QTAILQ element */ > >>>> + size_t size = field->size; > >>>> + /* offset of the QTAILQ entry in a QTAILQ element */ > >>>> + size_t entry_offset = field->start; > >>>> + int version_id = field->version_id; > >>>> + void *elm; > >>>> + > >>>> + trace_get_qtailq(vmsd->name, version_id); > >>>> + if (version_id > vmsd->version_id) { > >>>> + error_report("%s %s", vmsd->name, "too new"); > >>>> + trace_get_qtailq_end(vmsd->name, "too new", -EINVAL); > >>>> + > >>>> + return -EINVAL; > >>>> + } > >>>> + if (version_id < vmsd->minimum_version_id) { > >>>> + error_report("%s %s", vmsd->name, "too old"); > >>>> + trace_get_qtailq_end(vmsd->name, "too old", -EINVAL); > >>>> + return -EINVAL; > >>>> + } > >>>> + > >>>> + while (qemu_get_byte(f)) { > >>>> + elm = g_malloc(size); > >>>> + ret = vmstate_load_state(f, vmsd, elm, version_id); > >>>> + if (ret) { > >>>> + return ret; > >>> > >>> You could just make that a break since you're returning ret > >>> (that's otherwise all 0). Still, not important, but could > >>> be tidied if you need to regenerate the patch. > >>> (and you could remove the 2nd space after elm =). > > > >> By returning it, the trace file will miss "get_qtailq_end', that > >> is useful debug information. > >> You are right about the space. > > > > But that's what you're doing now, you're missing the trace call > > and returning 'ret' that will always be 0 if it gets there. > > > > Dave > > > >> > >> Thanks, > >> Jianjun > >> > >>> > >>>> + } > >>>> + QTAILQ_RAW_INSERT_TAIL(pv, elm, entry_offset); > >>>> + } > >>>> + > >>>> + 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 = field->vmsd; > >>>> + /* offset of the QTAILQ entry in a QTAILQ element*/ > >>>> + size_t entry_offset = field->start; > >>>> + void *elm; > >>>> + > >>>> + trace_put_qtailq(vmsd->name, vmsd->version_id); > >>>> + > >>>> + QTAILQ_RAW_FOREACH(elm, pv, entry_offset) { > >>>> + 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 = { > >>>> + .name = "qtailq", > >>>> + .get = get_qtailq, > >>>> + .put = put_qtailq, > >>>> +}; > >>> > >>> Reviewed-by: Dr. David Alan Gilbert > >>> > >>>> -- > >>>> 1.9.1 > >>>> > >>> -- > >>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > >>> > >> > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK