| Message ID | 1464717764-9040-5-git-send-email-duanj@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
----- Original Message ----- > From: "Jianjun Duan" <duanj@linux.vnet.ibm.com> > To: qemu-devel@nongnu.org > Cc: qemu-ppc@nongnu.org, duanj@linux.vnet.ibm.com, dmitry@daynix.com, "peter maydell" <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" <amit.shah@redhat.com>, mreitz@redhat.com, kwolf@redhat.com, rth@twiddle.net, > aurelien@aurel32.net, "leon alrae" <leon.alrae@imgtec.com>, blauwirbel@gmail.com, "mark cave-ayland" > <mark.cave-ayland@ilande.co.uk>, mdroth@linux.vnet.ibm.com > Sent: Tuesday, May 31, 2016 8:02:42 PM > Subject: [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ > > 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_CSTM. We then modified vmstate_save_state and vmstate_load_state > so that when VMS_CSTM is encountered, put and get from VMStateInfo are > called respectively. 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 <duanj@linux.vnet.ibm.com> > --- > include/migration/vmstate.h | 22 +++++++++++++ > include/qemu/queue.h | 32 ++++++++++++++++++ > migration/vmstate.c | 79 > +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 133 insertions(+) > > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h > index 56a4171..da4ef7f 100644 > --- a/include/migration/vmstate.h > +++ b/include/migration/vmstate.h > @@ -185,6 +185,8 @@ enum VMStateFlags { > * to determine the number of entries in the array. Only valid in > * combination with one of VMS_VARRAY*. */ > VMS_MULTIPLY_ELEMENTS = 0x4000, > + /* For fields which need customized handling, such as QTAILQ in > queue.h*/ > + VMS_CSTM = 0x8000, Please call this VMS_LINKED. It can be adapted to other data structures in qemu/queue.h if there is a need later on. > }; > > struct VMStateField { > @@ -245,6 +247,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) > @@ -656,6 +659,25 @@ extern const VMStateInfo vmstate_info_bitmap; > .offset = offsetof(_state, _field), \ > } > > +/* 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, _next) \ > +{ \ > + .name = (stringify(_field)), \ > + .version_id = (_version), \ > + .vmsd = &(_vmsd), \ > + .size = sizeof(_type), \ > + .info = &vmstate_info_qtailq, \ > + .flags = VMS_CSTM, \ > + .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 f781aa2..003e368 100644 > --- a/include/qemu/queue.h > +++ b/include/qemu/queue.h > @@ -437,3 +437,35 @@ struct { > \ > (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last)) > > #endif /* !QEMU_SYS_QUEUE_H_ */ > + > +/* > + * 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) = *((void **) ((char *) (head) + QTAILQ_FIRST_OFFSET)); > \ > + (elm); > \ > + (elm) = > \ > + *((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)) = NULL; > \ > + *((void **) ((char *) (elm) + (entry) + QTAILQ_PREV_OFFSET)) = > \ > + *((void **) ((char *) (head) +QTAILQ_LAST_OFFSET)); > \ > + **((void ***)((char *) (head) +QTAILQ_LAST_OFFSET)) = (elm); > \ > + *((void **) ((char *) (head) + QTAILQ_LAST_OFFSET)) = > \ > + (void *) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET); > \ > +} while (/*CONSTCOND*/0) > diff --git a/migration/vmstate.c b/migration/vmstate.c > index 644ba1f..ff56650 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" > > static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription > *vmsd, > void *opaque, QJSON *vmdesc); > @@ -120,6 +122,8 @@ int vmstate_load_state(QEMUFile *f, const > VMStateDescription *vmsd, > if (field->flags & VMS_STRUCT) { > ret = vmstate_load_state(f, field->vmsd, addr, > field->vmsd->version_id); > + } else if (field->flags & VMS_CSTM) { > + ret = field->info->get(f, addr, size, field); > } else { > ret = field->info->get(f, addr, size, NULL); > > @@ -192,6 +196,8 @@ static const char *vmfield_get_type_name(VMStateField > *field) > > if (field->flags & VMS_STRUCT) { > type = "struct"; > + } else if (field->flags & VMS_CSTM) { > + type = "customized"; > } else if (field->info->name) { > type = field->info->name; > } > @@ -326,6 +332,8 @@ void vmstate_save_state(QEMUFile *f, const > VMStateDescription *vmsd, > } > if (field->flags & VMS_STRUCT) { > vmstate_save_state(f, field->vmsd, addr, vmdesc_loop); > + } else if (field->flags & VMS_CSTM) { > + field->info->put(f, addr, size, field, vmdesc_loop); > } else { > field->info->put(f, addr, size, NULL, NULL); > } > @@ -938,3 +946,74 @@ const VMStateInfo vmstate_info_bitmap = { > .get = get_bitmap, > .put = put_bitmap, > }; > + > +/*get for QTAILQ */ > +static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size, > + VMStateField *field) > +{ > + bool link; > + int ret = 0; > + const VMStateDescription *vmsd = field->vmsd; > + size_t size = field->size; > + size_t entry = field->start; > + int version_id = field->version_id; > + void *elm; > + > + trace_vmstate_load_state(vmsd->name, version_id); > + if (version_id > vmsd->version_id) { > + trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL); > + return -EINVAL; > + } > + if (version_id < vmsd->minimum_version_id) { > + trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL); > + return -EINVAL; > + } > + > + while (true) { > + vmstate_info_bool.get(f, &link, sizeof(bool), NULL); You can use qemu_get_byte here, and likewise qemu_put_byte below in put_qtailq. > + if (!link) { > + break; > + } > + > + elm = g_malloc(size); > + ret = vmstate_load_state(f, vmsd, elm, version_id); > + if (ret) { > + return ret; > + } > + QTAILQ_RAW_INSERT_TAIL(pv, elm, entry); > + } > + > + trace_vmstate_load_state_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) > +{ > + bool link = true; > + const VMStateDescription *vmsd = field->vmsd; > + size_t entry = field->start; > + void *elm; > + > + if (vmdesc) { > + json_prop_str(vmdesc, "vmsd_name", vmsd->name); > + json_prop_int(vmdesc, "version", vmsd->version_id); > + json_start_array(vmdesc, "fields"); You need to store the fields exactly once here, even if there are 0 or >1 elements. Otherwise looks great now! Paolo > + } > + > + QTAILQ_RAW_FOREACH(elm, pv, entry) { > + vmstate_info_bool.put(f, &link, sizeof(bool), NULL, NULL); > + vmstate_save_state(f, vmsd, elm, vmdesc); > + } > + link = false; > + vmstate_info_bool.put(f, &link, sizeof(bool), NULL, NULL); > + if (vmdesc) { > + json_end_array(vmdesc); > + } > +} > +const VMStateInfo vmstate_info_qtailq = { > + .name = "qtailq", > + .get = get_qtailq, > + .put = put_qtailq, > +}; > -- > 1.9.1 > >
On 05/31/2016 12:54 PM, Paolo Bonzini wrote: > > > ----- Original Message ----- >> From: "Jianjun Duan" <duanj@linux.vnet.ibm.com> >> To: qemu-devel@nongnu.org >> Cc: qemu-ppc@nongnu.org, duanj@linux.vnet.ibm.com, dmitry@daynix.com, "peter maydell" <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" <amit.shah@redhat.com>, mreitz@redhat.com, kwolf@redhat.com, rth@twiddle.net, >> aurelien@aurel32.net, "leon alrae" <leon.alrae@imgtec.com>, blauwirbel@gmail.com, "mark cave-ayland" >> <mark.cave-ayland@ilande.co.uk>, mdroth@linux.vnet.ibm.com >> Sent: Tuesday, May 31, 2016 8:02:42 PM >> Subject: [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ >> >> 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_CSTM. We then modified vmstate_save_state and vmstate_load_state >> so that when VMS_CSTM is encountered, put and get from VMStateInfo are >> called respectively. 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 <duanj@linux.vnet.ibm.com> >> --- >> include/migration/vmstate.h | 22 +++++++++++++ >> include/qemu/queue.h | 32 ++++++++++++++++++ >> migration/vmstate.c | 79 >> +++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 133 insertions(+) >> >> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h >> index 56a4171..da4ef7f 100644 >> --- a/include/migration/vmstate.h >> +++ b/include/migration/vmstate.h >> @@ -185,6 +185,8 @@ enum VMStateFlags { >> * to determine the number of entries in the array. Only valid in >> * combination with one of VMS_VARRAY*. */ >> VMS_MULTIPLY_ELEMENTS = 0x4000, >> + /* For fields which need customized handling, such as QTAILQ in >> queue.h*/ >> + VMS_CSTM = 0x8000, > > Please call this VMS_LINKED. It can be adapted to other data > structures in qemu/queue.h if there is a need later on. > >> }; >> >> struct VMStateField { >> @@ -245,6 +247,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) >> @@ -656,6 +659,25 @@ extern const VMStateInfo vmstate_info_bitmap; >> .offset = offsetof(_state, _field), \ >> } >> >> +/* 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, _next) \ >> +{ \ >> + .name = (stringify(_field)), \ >> + .version_id = (_version), \ >> + .vmsd = &(_vmsd), \ >> + .size = sizeof(_type), \ >> + .info = &vmstate_info_qtailq, \ >> + .flags = VMS_CSTM, \ >> + .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 f781aa2..003e368 100644 >> --- a/include/qemu/queue.h >> +++ b/include/qemu/queue.h >> @@ -437,3 +437,35 @@ struct { >> \ >> (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last)) >> >> #endif /* !QEMU_SYS_QUEUE_H_ */ >> + >> +/* >> + * 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) = *((void **) ((char *) (head) + QTAILQ_FIRST_OFFSET)); >> \ >> + (elm); >> \ >> + (elm) = >> \ >> + *((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)) = NULL; >> \ >> + *((void **) ((char *) (elm) + (entry) + QTAILQ_PREV_OFFSET)) = >> \ >> + *((void **) ((char *) (head) +QTAILQ_LAST_OFFSET)); >> \ >> + **((void ***)((char *) (head) +QTAILQ_LAST_OFFSET)) = (elm); >> \ >> + *((void **) ((char *) (head) + QTAILQ_LAST_OFFSET)) = >> \ >> + (void *) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET); >> \ >> +} while (/*CONSTCOND*/0) >> diff --git a/migration/vmstate.c b/migration/vmstate.c >> index 644ba1f..ff56650 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" >> >> static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription >> *vmsd, >> void *opaque, QJSON *vmdesc); >> @@ -120,6 +122,8 @@ int vmstate_load_state(QEMUFile *f, const >> VMStateDescription *vmsd, >> if (field->flags & VMS_STRUCT) { >> ret = vmstate_load_state(f, field->vmsd, addr, >> field->vmsd->version_id); >> + } else if (field->flags & VMS_CSTM) { >> + ret = field->info->get(f, addr, size, field); >> } else { >> ret = field->info->get(f, addr, size, NULL); >> >> @@ -192,6 +196,8 @@ static const char *vmfield_get_type_name(VMStateField >> *field) >> >> if (field->flags & VMS_STRUCT) { >> type = "struct"; >> + } else if (field->flags & VMS_CSTM) { >> + type = "customized"; >> } else if (field->info->name) { >> type = field->info->name; >> } >> @@ -326,6 +332,8 @@ void vmstate_save_state(QEMUFile *f, const >> VMStateDescription *vmsd, >> } >> if (field->flags & VMS_STRUCT) { >> vmstate_save_state(f, field->vmsd, addr, vmdesc_loop); >> + } else if (field->flags & VMS_CSTM) { >> + field->info->put(f, addr, size, field, vmdesc_loop); >> } else { >> field->info->put(f, addr, size, NULL, NULL); >> } >> @@ -938,3 +946,74 @@ const VMStateInfo vmstate_info_bitmap = { >> .get = get_bitmap, >> .put = put_bitmap, >> }; >> + >> +/*get for QTAILQ */ >> +static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size, >> + VMStateField *field) >> +{ >> + bool link; >> + int ret = 0; >> + const VMStateDescription *vmsd = field->vmsd; >> + size_t size = field->size; >> + size_t entry = field->start; >> + int version_id = field->version_id; >> + void *elm; >> + >> + trace_vmstate_load_state(vmsd->name, version_id); >> + if (version_id > vmsd->version_id) { >> + trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL); >> + return -EINVAL; >> + } >> + if (version_id < vmsd->minimum_version_id) { >> + trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL); >> + return -EINVAL; >> + } >> + >> + while (true) { >> + vmstate_info_bool.get(f, &link, sizeof(bool), NULL); > > You can use qemu_get_byte here, and likewise qemu_put_byte below in > put_qtailq. qemu_get/put is indeed better choice. > >> + if (!link) { >> + break; >> + } >> + >> + elm = g_malloc(size); >> + ret = vmstate_load_state(f, vmsd, elm, version_id); >> + if (ret) { >> + return ret; >> + } >> + QTAILQ_RAW_INSERT_TAIL(pv, elm, entry); >> + } >> + >> + trace_vmstate_load_state_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) >> +{ >> + bool link = true; >> + const VMStateDescription *vmsd = field->vmsd; >> + size_t entry = field->start; >> + void *elm; >> + >> + if (vmdesc) { >> + json_prop_str(vmdesc, "vmsd_name", vmsd->name); >> + json_prop_int(vmdesc, "version", vmsd->version_id); >> + json_start_array(vmdesc, "fields"); > > You need to store the fields exactly once here, even if there are > 0 or >1 elements. > Do you mean the json entries? I think it is already doing that. > Otherwise looks great now! > > Paolo > >> + } >> + >> + QTAILQ_RAW_FOREACH(elm, pv, entry) { >> + vmstate_info_bool.put(f, &link, sizeof(bool), NULL, NULL); >> + vmstate_save_state(f, vmsd, elm, vmdesc); >> + } >> + link = false; >> + vmstate_info_bool.put(f, &link, sizeof(bool), NULL, NULL); >> + if (vmdesc) { >> + json_end_array(vmdesc); >> + } >> +} >> +const VMStateInfo vmstate_info_qtailq = { >> + .name = "qtailq", >> + .get = get_qtailq, >> + .put = put_qtailq, >> +}; >> -- >> 1.9.1 >> >> > Thanks, Jianjun
On 31/05/2016 23:53, Jianjun Duan wrote: > > > On 05/31/2016 12:54 PM, Paolo Bonzini wrote: >> >> >> ----- Original Message ----- >>> From: "Jianjun Duan" <duanj@linux.vnet.ibm.com> >>> To: qemu-devel@nongnu.org >>> Cc: qemu-ppc@nongnu.org, duanj@linux.vnet.ibm.com, dmitry@daynix.com, "peter maydell" <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" <amit.shah@redhat.com>, mreitz@redhat.com, kwolf@redhat.com, rth@twiddle.net, >>> aurelien@aurel32.net, "leon alrae" <leon.alrae@imgtec.com>, blauwirbel@gmail.com, "mark cave-ayland" >>> <mark.cave-ayland@ilande.co.uk>, mdroth@linux.vnet.ibm.com >>> Sent: Tuesday, May 31, 2016 8:02:42 PM >>> Subject: [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ >>> >>> 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_CSTM. We then modified vmstate_save_state and vmstate_load_state >>> so that when VMS_CSTM is encountered, put and get from VMStateInfo are >>> called respectively. 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 <duanj@linux.vnet.ibm.com> >>> --- >>> include/migration/vmstate.h | 22 +++++++++++++ >>> include/qemu/queue.h | 32 ++++++++++++++++++ >>> migration/vmstate.c | 79 >>> +++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 133 insertions(+) >>> >>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h >>> index 56a4171..da4ef7f 100644 >>> --- a/include/migration/vmstate.h >>> +++ b/include/migration/vmstate.h >>> @@ -185,6 +185,8 @@ enum VMStateFlags { >>> * to determine the number of entries in the array. Only valid in >>> * combination with one of VMS_VARRAY*. */ >>> VMS_MULTIPLY_ELEMENTS = 0x4000, >>> + /* For fields which need customized handling, such as QTAILQ in >>> queue.h*/ >>> + VMS_CSTM = 0x8000, >> >> Please call this VMS_LINKED. It can be adapted to other data >> structures in qemu/queue.h if there is a need later on. >> >>> }; >>> >>> struct VMStateField { >>> @@ -245,6 +247,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) >>> @@ -656,6 +659,25 @@ extern const VMStateInfo vmstate_info_bitmap; >>> .offset = offsetof(_state, _field), \ >>> } >>> >>> +/* 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, _next) \ >>> +{ \ >>> + .name = (stringify(_field)), \ >>> + .version_id = (_version), \ >>> + .vmsd = &(_vmsd), \ >>> + .size = sizeof(_type), \ >>> + .info = &vmstate_info_qtailq, \ >>> + .flags = VMS_CSTM, \ >>> + .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 f781aa2..003e368 100644 >>> --- a/include/qemu/queue.h >>> +++ b/include/qemu/queue.h >>> @@ -437,3 +437,35 @@ struct { >>> \ >>> (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last)) >>> >>> #endif /* !QEMU_SYS_QUEUE_H_ */ >>> + >>> +/* >>> + * 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) = *((void **) ((char *) (head) + QTAILQ_FIRST_OFFSET)); >>> \ >>> + (elm); >>> \ >>> + (elm) = >>> \ >>> + *((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)) = NULL; >>> \ >>> + *((void **) ((char *) (elm) + (entry) + QTAILQ_PREV_OFFSET)) = >>> \ >>> + *((void **) ((char *) (head) +QTAILQ_LAST_OFFSET)); >>> \ >>> + **((void ***)((char *) (head) +QTAILQ_LAST_OFFSET)) = (elm); >>> \ >>> + *((void **) ((char *) (head) + QTAILQ_LAST_OFFSET)) = >>> \ >>> + (void *) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET); >>> \ >>> +} while (/*CONSTCOND*/0) >>> diff --git a/migration/vmstate.c b/migration/vmstate.c >>> index 644ba1f..ff56650 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" >>> >>> static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription >>> *vmsd, >>> void *opaque, QJSON *vmdesc); >>> @@ -120,6 +122,8 @@ int vmstate_load_state(QEMUFile *f, const >>> VMStateDescription *vmsd, >>> if (field->flags & VMS_STRUCT) { >>> ret = vmstate_load_state(f, field->vmsd, addr, >>> field->vmsd->version_id); >>> + } else if (field->flags & VMS_CSTM) { >>> + ret = field->info->get(f, addr, size, field); >>> } else { >>> ret = field->info->get(f, addr, size, NULL); >>> >>> @@ -192,6 +196,8 @@ static const char *vmfield_get_type_name(VMStateField >>> *field) >>> >>> if (field->flags & VMS_STRUCT) { >>> type = "struct"; >>> + } else if (field->flags & VMS_CSTM) { >>> + type = "customized"; >>> } else if (field->info->name) { >>> type = field->info->name; >>> } >>> @@ -326,6 +332,8 @@ void vmstate_save_state(QEMUFile *f, const >>> VMStateDescription *vmsd, >>> } >>> if (field->flags & VMS_STRUCT) { >>> vmstate_save_state(f, field->vmsd, addr, vmdesc_loop); >>> + } else if (field->flags & VMS_CSTM) { >>> + field->info->put(f, addr, size, field, vmdesc_loop); >>> } else { >>> field->info->put(f, addr, size, NULL, NULL); >>> } >>> @@ -938,3 +946,74 @@ const VMStateInfo vmstate_info_bitmap = { >>> .get = get_bitmap, >>> .put = put_bitmap, >>> }; >>> + >>> +/*get for QTAILQ */ >>> +static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size, >>> + VMStateField *field) >>> +{ >>> + bool link; >>> + int ret = 0; >>> + const VMStateDescription *vmsd = field->vmsd; >>> + size_t size = field->size; >>> + size_t entry = field->start; >>> + int version_id = field->version_id; >>> + void *elm; >>> + >>> + trace_vmstate_load_state(vmsd->name, version_id); >>> + if (version_id > vmsd->version_id) { >>> + trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL); >>> + return -EINVAL; >>> + } >>> + if (version_id < vmsd->minimum_version_id) { >>> + trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL); >>> + return -EINVAL; >>> + } >>> + >>> + while (true) { >>> + vmstate_info_bool.get(f, &link, sizeof(bool), NULL); >> >> You can use qemu_get_byte here, and likewise qemu_put_byte below in >> put_qtailq. > > qemu_get/put is indeed better choice. >> >>> + if (!link) { >>> + break; >>> + } >>> + >>> + elm = g_malloc(size); >>> + ret = vmstate_load_state(f, vmsd, elm, version_id); >>> + if (ret) { >>> + return ret; >>> + } >>> + QTAILQ_RAW_INSERT_TAIL(pv, elm, entry); >>> + } >>> + >>> + trace_vmstate_load_state_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) >>> +{ >>> + bool link = true; >>> + const VMStateDescription *vmsd = field->vmsd; >>> + size_t entry = field->start; >>> + void *elm; >>> + >>> + if (vmdesc) { >>> + json_prop_str(vmdesc, "vmsd_name", vmsd->name); >>> + json_prop_int(vmdesc, "version", vmsd->version_id); >>> + json_start_array(vmdesc, "fields"); >> >> You need to store the fields exactly once here, even if there are >> 0 or >1 elements. >> > Do you mean the json entries? I think it is already doing that. In the case of 0 entries we don't go through the loop, so the JSON entries are definitely missing in that case. I'm not sure if QJSON handles duplicates in the case of 2+ entries. Thanks, Paolo
On 06/01/2016 08:29 AM, Paolo Bonzini wrote: > > > On 31/05/2016 23:53, Jianjun Duan wrote: >> >> >> On 05/31/2016 12:54 PM, Paolo Bonzini wrote: >>> >>> >>> ----- Original Message ----- >>>> From: "Jianjun Duan" <duanj@linux.vnet.ibm.com> >>>> To: qemu-devel@nongnu.org >>>> Cc: qemu-ppc@nongnu.org, duanj@linux.vnet.ibm.com, dmitry@daynix.com, "peter maydell" <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" <amit.shah@redhat.com>, mreitz@redhat.com, kwolf@redhat.com, rth@twiddle.net, >>>> aurelien@aurel32.net, "leon alrae" <leon.alrae@imgtec.com>, blauwirbel@gmail.com, "mark cave-ayland" >>>> <mark.cave-ayland@ilande.co.uk>, mdroth@linux.vnet.ibm.com >>>> Sent: Tuesday, May 31, 2016 8:02:42 PM >>>> Subject: [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ >>>> >>>> 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_CSTM. We then modified vmstate_save_state and vmstate_load_state >>>> so that when VMS_CSTM is encountered, put and get from VMStateInfo are >>>> called respectively. 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 <duanj@linux.vnet.ibm.com> >>>> --- >>>> include/migration/vmstate.h | 22 +++++++++++++ >>>> include/qemu/queue.h | 32 ++++++++++++++++++ >>>> migration/vmstate.c | 79 >>>> +++++++++++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 133 insertions(+) >>>> >>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h >>>> index 56a4171..da4ef7f 100644 >>>> --- a/include/migration/vmstate.h >>>> +++ b/include/migration/vmstate.h >>>> @@ -185,6 +185,8 @@ enum VMStateFlags { >>>> * to determine the number of entries in the array. Only valid in >>>> * combination with one of VMS_VARRAY*. */ >>>> VMS_MULTIPLY_ELEMENTS = 0x4000, >>>> + /* For fields which need customized handling, such as QTAILQ in >>>> queue.h*/ >>>> + VMS_CSTM = 0x8000, >>> >>> Please call this VMS_LINKED. It can be adapted to other data >>> structures in qemu/queue.h if there is a need later on. >>> >>>> }; >>>> >>>> struct VMStateField { >>>> @@ -245,6 +247,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) >>>> @@ -656,6 +659,25 @@ extern const VMStateInfo vmstate_info_bitmap; >>>> .offset = offsetof(_state, _field), \ >>>> } >>>> >>>> +/* 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, _next) \ >>>> +{ \ >>>> + .name = (stringify(_field)), \ >>>> + .version_id = (_version), \ >>>> + .vmsd = &(_vmsd), \ >>>> + .size = sizeof(_type), \ >>>> + .info = &vmstate_info_qtailq, \ >>>> + .flags = VMS_CSTM, \ >>>> + .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 f781aa2..003e368 100644 >>>> --- a/include/qemu/queue.h >>>> +++ b/include/qemu/queue.h >>>> @@ -437,3 +437,35 @@ struct { >>>> \ >>>> (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last)) >>>> >>>> #endif /* !QEMU_SYS_QUEUE_H_ */ >>>> + >>>> +/* >>>> + * 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) = *((void **) ((char *) (head) + QTAILQ_FIRST_OFFSET)); >>>> \ >>>> + (elm); >>>> \ >>>> + (elm) = >>>> \ >>>> + *((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)) = NULL; >>>> \ >>>> + *((void **) ((char *) (elm) + (entry) + QTAILQ_PREV_OFFSET)) = >>>> \ >>>> + *((void **) ((char *) (head) +QTAILQ_LAST_OFFSET)); >>>> \ >>>> + **((void ***)((char *) (head) +QTAILQ_LAST_OFFSET)) = (elm); >>>> \ >>>> + *((void **) ((char *) (head) + QTAILQ_LAST_OFFSET)) = >>>> \ >>>> + (void *) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET); >>>> \ >>>> +} while (/*CONSTCOND*/0) >>>> diff --git a/migration/vmstate.c b/migration/vmstate.c >>>> index 644ba1f..ff56650 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" >>>> >>>> static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription >>>> *vmsd, >>>> void *opaque, QJSON *vmdesc); >>>> @@ -120,6 +122,8 @@ int vmstate_load_state(QEMUFile *f, const >>>> VMStateDescription *vmsd, >>>> if (field->flags & VMS_STRUCT) { >>>> ret = vmstate_load_state(f, field->vmsd, addr, >>>> field->vmsd->version_id); >>>> + } else if (field->flags & VMS_CSTM) { >>>> + ret = field->info->get(f, addr, size, field); >>>> } else { >>>> ret = field->info->get(f, addr, size, NULL); >>>> >>>> @@ -192,6 +196,8 @@ static const char *vmfield_get_type_name(VMStateField >>>> *field) >>>> >>>> if (field->flags & VMS_STRUCT) { >>>> type = "struct"; >>>> + } else if (field->flags & VMS_CSTM) { >>>> + type = "customized"; >>>> } else if (field->info->name) { >>>> type = field->info->name; >>>> } >>>> @@ -326,6 +332,8 @@ void vmstate_save_state(QEMUFile *f, const >>>> VMStateDescription *vmsd, >>>> } >>>> if (field->flags & VMS_STRUCT) { >>>> vmstate_save_state(f, field->vmsd, addr, vmdesc_loop); >>>> + } else if (field->flags & VMS_CSTM) { >>>> + field->info->put(f, addr, size, field, vmdesc_loop); >>>> } else { >>>> field->info->put(f, addr, size, NULL, NULL); >>>> } >>>> @@ -938,3 +946,74 @@ const VMStateInfo vmstate_info_bitmap = { >>>> .get = get_bitmap, >>>> .put = put_bitmap, >>>> }; >>>> + >>>> +/*get for QTAILQ */ >>>> +static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size, >>>> + VMStateField *field) >>>> +{ >>>> + bool link; >>>> + int ret = 0; >>>> + const VMStateDescription *vmsd = field->vmsd; >>>> + size_t size = field->size; >>>> + size_t entry = field->start; >>>> + int version_id = field->version_id; >>>> + void *elm; >>>> + >>>> + trace_vmstate_load_state(vmsd->name, version_id); >>>> + if (version_id > vmsd->version_id) { >>>> + trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL); >>>> + return -EINVAL; >>>> + } >>>> + if (version_id < vmsd->minimum_version_id) { >>>> + trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + while (true) { >>>> + vmstate_info_bool.get(f, &link, sizeof(bool), NULL); >>> >>> You can use qemu_get_byte here, and likewise qemu_put_byte below in >>> put_qtailq. >> >> qemu_get/put is indeed better choice. >>> >>>> + if (!link) { >>>> + break; >>>> + } >>>> + >>>> + elm = g_malloc(size); >>>> + ret = vmstate_load_state(f, vmsd, elm, version_id); >>>> + if (ret) { >>>> + return ret; >>>> + } >>>> + QTAILQ_RAW_INSERT_TAIL(pv, elm, entry); >>>> + } >>>> + >>>> + trace_vmstate_load_state_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) >>>> +{ >>>> + bool link = true; >>>> + const VMStateDescription *vmsd = field->vmsd; >>>> + size_t entry = field->start; >>>> + void *elm; >>>> + >>>> + if (vmdesc) { >>>> + json_prop_str(vmdesc, "vmsd_name", vmsd->name); >>>> + json_prop_int(vmdesc, "version", vmsd->version_id); >>>> + json_start_array(vmdesc, "fields"); >>> >>> You need to store the fields exactly once here, even if there are >>> 0 or >1 elements. >>> >> Do you mean the json entries? I think it is already doing that. > > In the case of 0 entries we don't go through the loop, so the JSON > entries are definitely missing in that case. I'm not sure if QJSON > handles duplicates in the case of 2+ entries. The vmsd here is the vmsd for the queue elements. Not for the queue. Maybe the stuff written here should be information about the qtailq instead, but we don't have a vmsd for the queue as a whole. As it stands, it will record the name, version of the element type. If the queue is empty, it records nothing in the fields. otherwise it will record each element and repeat. In vmstate_save_state, the vmsd of the array element for a uncompressed array is recorded every time an array element is saved. The number of written bytes is also recorded. If the array has 0 elements, it is not recorded. If we want to make the behavior consistent, we should not do any json stuff in put_qtatilq function. If we want to record the vmsd of the queue element exactly once, I can set the vmdesc to null after the first iteration. But we may need a recursive function just to dump out the vmsd when the queue is empty, and record that 0 bytes are written for each field. I would say that let's remove the json stuff here to be consistent with array behavior. > > Thanks, > > Paolo > Thanks, Jianjun
On 01/06/2016 19:06, Jianjun Duan wrote: > On 06/01/2016 08:29 AM, Paolo Bonzini wrote: >> On 31/05/2016 23:53, Jianjun Duan wrote: >>> On 05/31/2016 12:54 PM, Paolo Bonzini wrote: >>>>> +/* put for QTAILQ */ >>>>> +static void put_qtailq(QEMUFile *f, void *pv, size_t unused_size, >>>>> + VMStateField *field, QJSON *vmdesc) >>>>> +{ >>>>> + bool link = true; >>>>> + const VMStateDescription *vmsd = field->vmsd; >>>>> + size_t entry = field->start; >>>>> + void *elm; >>>>> + >>>>> + if (vmdesc) { >>>>> + json_prop_str(vmdesc, "vmsd_name", vmsd->name); >>>>> + json_prop_int(vmdesc, "version", vmsd->version_id); >>>>> + json_start_array(vmdesc, "fields"); >>>> >>>> You need to store the fields exactly once here, even if there are >>>> 0 or >1 elements. >>>> >>> Do you mean the json entries? I think it is already doing that. >> >> In the case of 0 entries we don't go through the loop, so the JSON >> entries are definitely missing in that case. I'm not sure if QJSON >> handles duplicates in the case of 2+ entries. > > The vmsd here is the vmsd for the queue elements. Not for the queue. > Maybe the stuff written here should be information about the qtailq > instead, but we don't have a vmsd for the queue as a whole. You're right, you could use vmsd_can_compress but it's not necessary to do so. Your code is fine. Paolo
On Tue, May 31, 2016 at 11:02:42AM -0700, Jianjun Duan 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_CSTM. We then modified vmstate_save_state and vmstate_load_state > so that when VMS_CSTM is encountered, put and get from VMStateInfo are > called respectively. 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 <duanj@linux.vnet.ibm.com> I'm comfortable with 3&4 of 6, but I'd prefer to see them merged via the migration tree. Juan, Dave, are you ok to merge these? > --- > include/migration/vmstate.h | 22 +++++++++++++ > include/qemu/queue.h | 32 ++++++++++++++++++ > migration/vmstate.c | 79 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 133 insertions(+) > > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h > index 56a4171..da4ef7f 100644 > --- a/include/migration/vmstate.h > +++ b/include/migration/vmstate.h > @@ -185,6 +185,8 @@ enum VMStateFlags { > * to determine the number of entries in the array. Only valid in > * combination with one of VMS_VARRAY*. */ > VMS_MULTIPLY_ELEMENTS = 0x4000, > + /* For fields which need customized handling, such as QTAILQ in queue.h*/ > + VMS_CSTM = 0x8000, > }; > > struct VMStateField { > @@ -245,6 +247,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) > @@ -656,6 +659,25 @@ extern const VMStateInfo vmstate_info_bitmap; > .offset = offsetof(_state, _field), \ > } > > +/* 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, _next) \ > +{ \ > + .name = (stringify(_field)), \ > + .version_id = (_version), \ > + .vmsd = &(_vmsd), \ > + .size = sizeof(_type), \ > + .info = &vmstate_info_qtailq, \ > + .flags = VMS_CSTM, \ > + .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 f781aa2..003e368 100644 > --- a/include/qemu/queue.h > +++ b/include/qemu/queue.h > @@ -437,3 +437,35 @@ struct { \ > (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last)) > > #endif /* !QEMU_SYS_QUEUE_H_ */ > + > +/* > + * 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) = *((void **) ((char *) (head) + QTAILQ_FIRST_OFFSET)); \ > + (elm); \ > + (elm) = \ > + *((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)) = NULL; \ > + *((void **) ((char *) (elm) + (entry) + QTAILQ_PREV_OFFSET)) = \ > + *((void **) ((char *) (head) +QTAILQ_LAST_OFFSET)); \ > + **((void ***)((char *) (head) +QTAILQ_LAST_OFFSET)) = (elm); \ > + *((void **) ((char *) (head) + QTAILQ_LAST_OFFSET)) = \ > + (void *) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET); \ > +} while (/*CONSTCOND*/0) > diff --git a/migration/vmstate.c b/migration/vmstate.c > index 644ba1f..ff56650 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" > > static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd, > void *opaque, QJSON *vmdesc); > @@ -120,6 +122,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, > if (field->flags & VMS_STRUCT) { > ret = vmstate_load_state(f, field->vmsd, addr, > field->vmsd->version_id); > + } else if (field->flags & VMS_CSTM) { > + ret = field->info->get(f, addr, size, field); > } else { > ret = field->info->get(f, addr, size, NULL); > > @@ -192,6 +196,8 @@ static const char *vmfield_get_type_name(VMStateField *field) > > if (field->flags & VMS_STRUCT) { > type = "struct"; > + } else if (field->flags & VMS_CSTM) { > + type = "customized"; > } else if (field->info->name) { > type = field->info->name; > } > @@ -326,6 +332,8 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd, > } > if (field->flags & VMS_STRUCT) { > vmstate_save_state(f, field->vmsd, addr, vmdesc_loop); > + } else if (field->flags & VMS_CSTM) { > + field->info->put(f, addr, size, field, vmdesc_loop); > } else { > field->info->put(f, addr, size, NULL, NULL); > } > @@ -938,3 +946,74 @@ const VMStateInfo vmstate_info_bitmap = { > .get = get_bitmap, > .put = put_bitmap, > }; > + > +/*get for QTAILQ */ > +static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size, > + VMStateField *field) > +{ > + bool link; > + int ret = 0; > + const VMStateDescription *vmsd = field->vmsd; > + size_t size = field->size; > + size_t entry = field->start; > + int version_id = field->version_id; > + void *elm; > + > + trace_vmstate_load_state(vmsd->name, version_id); > + if (version_id > vmsd->version_id) { > + trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL); > + return -EINVAL; > + } > + if (version_id < vmsd->minimum_version_id) { > + trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL); > + return -EINVAL; > + } > + > + while (true) { > + vmstate_info_bool.get(f, &link, sizeof(bool), NULL); > + if (!link) { > + break; > + } > + > + elm = g_malloc(size); > + ret = vmstate_load_state(f, vmsd, elm, version_id); > + if (ret) { > + return ret; > + } > + QTAILQ_RAW_INSERT_TAIL(pv, elm, entry); > + } > + > + trace_vmstate_load_state_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) > +{ > + bool link = true; > + const VMStateDescription *vmsd = field->vmsd; > + size_t entry = field->start; > + void *elm; > + > + if (vmdesc) { > + json_prop_str(vmdesc, "vmsd_name", vmsd->name); > + json_prop_int(vmdesc, "version", vmsd->version_id); > + json_start_array(vmdesc, "fields"); > + } > + > + QTAILQ_RAW_FOREACH(elm, pv, entry) { > + vmstate_info_bool.put(f, &link, sizeof(bool), NULL, NULL); > + vmstate_save_state(f, vmsd, elm, vmdesc); > + } > + link = false; > + vmstate_info_bool.put(f, &link, sizeof(bool), NULL, NULL); > + if (vmdesc) { > + json_end_array(vmdesc); > + } > +} > +const VMStateInfo vmstate_info_qtailq = { > + .name = "qtailq", > + .get = get_qtailq, > + .put = put_qtailq, > +};
Dear Jianjun, Jianjun Duan <duanj@linux.vnet.ibm.com> writes: [include/migration/vmstate.h] > @@ -185,6 +185,8 @@ enum VMStateFlags { > * to determine the number of entries in the array. Only valid in > * combination with one of VMS_VARRAY*. */ > VMS_MULTIPLY_ELEMENTS = 0x4000, > + /* For fields which need customized handling, such as QTAILQ in queue.h*/ > + VMS_CSTM = 0x8000, Can you describe (in the comment) how this customised handling is performed, please? I.e. describe what exactly happens if the flag is set, from the point of view of an API consumer. Also, why do you need this flag at all? The only change I can see is that you pass additional information to VMStateInfo.get() / .put(), using NULL if it's not set. Why don't you just always pass the additional information? If the additional information is not needed by get() / put() the parameter will be unused anyway. Sascha
Hi Sascha, On 06/02/2016 08:01 AM, Sascha Silbe wrote: > Dear Jianjun, > > Jianjun Duan <duanj@linux.vnet.ibm.com> writes: > > [include/migration/vmstate.h] >> @@ -185,6 +185,8 @@ enum VMStateFlags { >> * to determine the number of entries in the array. Only valid in >> * combination with one of VMS_VARRAY*. */ >> VMS_MULTIPLY_ELEMENTS = 0x4000, >> + /* For fields which need customized handling, such as QTAILQ in queue.h*/ >> + VMS_CSTM = 0x8000, > > Can you describe (in the comment) how this customised handling is > performed, please? I.e. describe what exactly happens if the flag is > set, from the point of view of an API consumer. I will add more comments. When this flag is set VMStateInfo.get/put will be used in vmstate_load/save_state instead of recursive call. And the user should implement VMStateInfo.get/put to handle the concerned data structure. > Also, why do you need this flag at all? The only change I can see is > that you pass additional information to VMStateInfo.get() / .put(), > using NULL if it's not set. Why don't you just always pass the > additional information? If the additional information is not needed by > get() / put() the parameter will be unused anyway. You can do it without creating this flag. Instead just to check if info is set in the field. However I think it is more readable and more robust to check this flag in vmstate_load/get_state. > Sascha > Thanks, Jianjun
On 06/02/2016 08:01 AM, Sascha Silbe wrote: > Dear Jianjun, > > Jianjun Duan <duanj@linux.vnet.ibm.com> writes: > > [include/migration/vmstate.h] >> @@ -185,6 +185,8 @@ enum VMStateFlags { >> * to determine the number of entries in the array. Only valid in >> * combination with one of VMS_VARRAY*. */ >> VMS_MULTIPLY_ELEMENTS = 0x4000, >> + /* For fields which need customized handling, such as QTAILQ in queue.h*/ >> + VMS_CSTM = 0x8000, > > Can you describe (in the comment) how this customised handling is > performed, please? I.e. describe what exactly happens if the flag is > set, from the point of view of an API consumer. > > Also, why do you need this flag at all? The only change I can see is > that you pass additional information to VMStateInfo.get() / .put(), > using NULL if it's not set. Why don't you just always pass the > additional information? If the additional information is not needed by > get() / put() the parameter will be unused anyway. > I now agree with you after some thought. > Sascha > Thanks, Jianjun
On 02/06/2016 17:01, Sascha Silbe wrote: >> > + /* For fields which need customized handling, such as QTAILQ in queue.h*/ >> > + VMS_CSTM = 0x8000, > Can you describe (in the comment) how this customised handling is > performed, please? I.e. describe what exactly happens if the flag is > set, from the point of view of an API consumer. > > Also, why do you need this flag at all? The only change I can see is > that you pass additional information to VMStateInfo.get() / .put(), > using NULL if it's not set. Why don't you just always pass the > additional information? If the additional information is not needed by > get() / put() the parameter will be unused anyway. Having the flag seemed like a good idea to me, because it gives a specific meaning to the offset fields in the VMStateField. On the other hand, it's true that it's unused... Paolo
* Jianjun Duan (duanj@linux.vnet.ibm.com) wrote: > Hi Sascha, > > On 06/02/2016 08:01 AM, Sascha Silbe wrote: > > Dear Jianjun, > > > > Jianjun Duan <duanj@linux.vnet.ibm.com> writes: > > > > [include/migration/vmstate.h] > >> @@ -185,6 +185,8 @@ enum VMStateFlags { > >> * to determine the number of entries in the array. Only valid in > >> * combination with one of VMS_VARRAY*. */ > >> VMS_MULTIPLY_ELEMENTS = 0x4000, > >> + /* For fields which need customized handling, such as QTAILQ in queue.h*/ > >> + VMS_CSTM = 0x8000, > > > > Can you describe (in the comment) how this customised handling is > > performed, please? I.e. describe what exactly happens if the flag is > > set, from the point of view of an API consumer. > > I will add more comments. When this flag is set VMStateInfo.get/put will > be used in vmstate_load/save_state instead of recursive call. And the > user should implement VMStateInfo.get/put to handle the concerned data > structure. > > Also, why do you need this flag at all? The only change I can see is > > that you pass additional information to VMStateInfo.get() / .put(), > > using NULL if it's not set. Why don't you just always pass the > > additional information? If the additional information is not needed by > > get() / put() the parameter will be unused anyway. > You can do it without creating this flag. Instead just to check if info > is set in the field. However I think it is more readable and more robust > to check this flag in vmstate_load/get_state. Apologies for not getting around to this sooner; I was on holiday when you first sent it. But: a) Is there a reason to use the 'bool' between each element; can't you count the length of the queue, send the length and then send the contents? In that case it should look a lot more like an ARRAY case on the wire. b) I think you should really try and split it into two parts: 1) A VMSTATE_ARRAY_CUSTOM (?) - so it's an array of elements but we've got a special way of allocating/counting/reading the elements 2) A version of that which is for a QTAILQ. It's important that whatever ends up on the migration stream doesn't reflect that it happens to be implemented as a QTAILQ; so if you decide to change it later the migration compatibility doesn't break. c) Use new trace_ names for get_qtailq rather than misusing trace_vmstate_load_state* d) Add a trace_ for put_qtailq as well - that way when it goes horribly wrong we can turn the tracing on on both sides :-) e) Is there anyway to make QTAILQ_RAW_INSERT_TAIL any more readable? I don't think I understand why you can't use the standard QTAILQ macros. f) You might need to fix up Amit's scripts/vmstate-static-checker.py Dave > > Sascha > > > > Thanks, > Jianjun > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 07/06/2016 16:43, Dr. David Alan Gilbert wrote: > b) I think you should really try and split it into two parts: > 1) A VMSTATE_ARRAY_CUSTOM (?) - so it's an array of elements but we've got a special > way of allocating/counting/reading the elements > 2) A version of that which is for a QTAILQ. > It's important that whatever ends up on the migration stream doesn't reflect > that it happens to be implemented as a QTAILQ; so if you decide to change > it later the migration compatibility doesn't break. (Just to clarify before Jianjun wakes up) a VMSTATE_ARRAY is just a sequence of elements. The count, if needed as in a VARRAY, is stored earlier and separately. Currently lists (including this QTAILQ) are usually represented in the migration stream as a sequence of elements preceded by "1" and terminated by a "0". Would you like to change it to a count + sequence as well? This would prevent using the new QTAILQ support for other existing lists such as virtio-blk and scsi's request lists. > c) Use new trace_ names for get_qtailq rather than misusing trace_vmstate_load_state* > d) Add a trace_ for put_qtailq as well - that way when it goes horribly wrong we can > turn the tracing on on both sides :-) > e) Is there anyway to make QTAILQ_RAW_INSERT_TAIL any more readable? > I don't think I understand why you can't use the standard QTAILQ macros. Because they assume a particular type. The "raw" version need to work on void*. Thanks, Paolo > f) You might need to fix up Amit's scripts/vmstate-static-checker.py
On Tue, Jun 07, 2016 at 06:31:41PM +0200, Paolo Bonzini wrote: > > > On 07/06/2016 16:43, Dr. David Alan Gilbert wrote: > > b) I think you should really try and split it into two parts: > > 1) A VMSTATE_ARRAY_CUSTOM (?) - so it's an array of elements but we've got a special > > way of allocating/counting/reading the elements > > 2) A version of that which is for a QTAILQ. > > It's important that whatever ends up on the migration stream doesn't reflect > > that it happens to be implemented as a QTAILQ; so if you decide to change > > it later the migration compatibility doesn't break. > > (Just to clarify before Jianjun wakes up) a VMSTATE_ARRAY is just a > sequence of elements. The count, if needed as in a VARRAY, is stored > earlier and separately. And if you migrate the count, you must validate that it's not bigger than an actual array size before VARRAY. > Currently lists (including this QTAILQ) are > usually represented in the migration stream as a sequence of elements > preceded by "1" and terminated by a "0". Would you like to change it to > a count + sequence as well? > > This would prevent using the new QTAILQ support for other existing lists > such as virtio-blk and scsi's request lists. > > > c) Use new trace_ names for get_qtailq rather than misusing trace_vmstate_load_state* > > d) Add a trace_ for put_qtailq as well - that way when it goes horribly wrong we can > > turn the tracing on on both sides :-) > > e) Is there anyway to make QTAILQ_RAW_INSERT_TAIL any more readable? > > I don't think I understand why you can't use the standard QTAILQ macros. > > Because they assume a particular type. The "raw" version need to work on > void*. > > Thanks, > > Paolo > > > f) You might need to fix up Amit's scripts/vmstate-static-checker.py
* Paolo Bonzini (pbonzini@redhat.com) wrote: > > > On 07/06/2016 16:43, Dr. David Alan Gilbert wrote: > > b) I think you should really try and split it into two parts: > > 1) A VMSTATE_ARRAY_CUSTOM (?) - so it's an array of elements but we've got a special > > way of allocating/counting/reading the elements > > 2) A version of that which is for a QTAILQ. > > It's important that whatever ends up on the migration stream doesn't reflect > > that it happens to be implemented as a QTAILQ; so if you decide to change > > it later the migration compatibility doesn't break. > > (Just to clarify before Jianjun wakes up) a VMSTATE_ARRAY is just a > sequence of elements. The count, if needed as in a VARRAY, is stored > earlier and separately. Currently lists (including this QTAILQ) are > usually represented in the migration stream as a sequence of elements > preceded by "1" and terminated by a "0". Would you like to change it to > a count + sequence as well? > > This would prevent using the new QTAILQ support for other existing lists > such as virtio-blk and scsi's request lists. That depends if it's something you're trying to keep migration compatibility with; if you're trying to keep format compaibility then sure keep it as is. If you're not trying to keep compatibility, then why can't virtio-blk or scsi request lists also use a count rather than the markers? > > c) Use new trace_ names for get_qtailq rather than misusing trace_vmstate_load_state* > > d) Add a trace_ for put_qtailq as well - that way when it goes horribly wrong we can > > turn the tracing on on both sides :-) > > e) Is there anyway to make QTAILQ_RAW_INSERT_TAIL any more readable? > > I don't think I understand why you can't use the standard QTAILQ macros. > > Because they assume a particular type. The "raw" version need to work on > void*. Ah OK. Dave > > Thanks, > > Paolo > > > f) You might need to fix up Amit's scripts/vmstate-static-checker.py -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 07/06/2016 18:34, Dr. David Alan Gilbert wrote: > * Paolo Bonzini (pbonzini@redhat.com) wrote: >> >> >> On 07/06/2016 16:43, Dr. David Alan Gilbert wrote: >>> b) I think you should really try and split it into two parts: >>> 1) A VMSTATE_ARRAY_CUSTOM (?) - so it's an array of elements but we've got a special >>> way of allocating/counting/reading the elements >>> 2) A version of that which is for a QTAILQ. >>> It's important that whatever ends up on the migration stream doesn't reflect >>> that it happens to be implemented as a QTAILQ; so if you decide to change >>> it later the migration compatibility doesn't break. >> >> (Just to clarify before Jianjun wakes up) a VMSTATE_ARRAY is just a >> sequence of elements. The count, if needed as in a VARRAY, is stored >> earlier and separately. Currently lists (including this QTAILQ) are >> usually represented in the migration stream as a sequence of elements >> preceded by "1" and terminated by a "0". Would you like to change it to >> a count + sequence as well? >> >> This would prevent using the new QTAILQ support for other existing lists >> such as virtio-blk and scsi's request lists. > > That depends if it's something you're trying to keep migration compatibility > with; if you're trying to keep format compaibility then sure keep it as is. > If you're not trying to keep compatibility, then why can't virtio-blk or > scsi request lists also use a count rather than the markers? We're trying to keep compatibility, and I think it's among the last bits that are resisting conversion to vmstate. Which explains my interest in Jianjun's patches. :) Paolo >>> c) Use new trace_ names for get_qtailq rather than misusing trace_vmstate_load_state* >>> d) Add a trace_ for put_qtailq as well - that way when it goes horribly wrong we can >>> turn the tracing on on both sides :-) >>> e) Is there anyway to make QTAILQ_RAW_INSERT_TAIL any more readable? >>> I don't think I understand why you can't use the standard QTAILQ macros. >> >> Because they assume a particular type. The "raw" version need to work on >> void*. > > Ah OK. > > Dave > >> >> Thanks, >> >> Paolo >> >>> f) You might need to fix up Amit's scripts/vmstate-static-checker.py > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >
On 06/07/2016 07:43 AM, Dr. David Alan Gilbert wrote: > * Jianjun Duan (duanj@linux.vnet.ibm.com) wrote: >> Hi Sascha, >> >> On 06/02/2016 08:01 AM, Sascha Silbe wrote: >>> Dear Jianjun, >>> >>> Jianjun Duan <duanj@linux.vnet.ibm.com> writes: >>> >>> [include/migration/vmstate.h] >>>> @@ -185,6 +185,8 @@ enum VMStateFlags { >>>> * to determine the number of entries in the array. Only valid in >>>> * combination with one of VMS_VARRAY*. */ >>>> VMS_MULTIPLY_ELEMENTS = 0x4000, >>>> + /* For fields which need customized handling, such as QTAILQ in queue.h*/ >>>> + VMS_CSTM = 0x8000, >>> >>> Can you describe (in the comment) how this customised handling is >>> performed, please? I.e. describe what exactly happens if the flag is >>> set, from the point of view of an API consumer. >> >> I will add more comments. When this flag is set VMStateInfo.get/put will >> be used in vmstate_load/save_state instead of recursive call. And the >> user should implement VMStateInfo.get/put to handle the concerned data >> structure. >>> Also, why do you need this flag at all? The only change I can see is >>> that you pass additional information to VMStateInfo.get() / .put(), >>> using NULL if it's not set. Why don't you just always pass the >>> additional information? If the additional information is not needed by >>> get() / put() the parameter will be unused anyway. >> You can do it without creating this flag. Instead just to check if info >> is set in the field. However I think it is more readable and more robust >> to check this flag in vmstate_load/get_state. > > Apologies for not getting around to this sooner; I was on holiday when you first > sent it. > > But: > a) Is there a reason to use the 'bool' between each element; can't you count the > length of the queue, send the length and then send the contents? In that case > it should look a lot more like an ARRAY case on the wire. > b) I think you should really try and split it into two parts: > 1) A VMSTATE_ARRAY_CUSTOM (?) - so it's an array of elements but we've got a special > way of allocating/counting/reading the elements > 2) A version of that which is for a QTAILQ. > It's important that whatever ends up on the migration stream doesn't reflect > that it happens to be implemented as a QTAILQ; so if you decide to change > it later the migration compatibility doesn't break. There are certainly more than one way to do this. I have thought about the way you suggested and decided not to do it that way. We either need to track its size, which means new element in the structure containing the queue, or go through the queue and count it. I don't want to add an element unless it is necessary. Counting and dumping the queue will go through the queue twice. Current solution only goes through the queue once. Besides, the queue is recursive and current solution is a natural one. > c) Use new trace_ names for get_qtailq rather than misusing trace_vmstate_load_state* > d) Add a trace_ for put_qtailq as well - that way when it goes horribly wrong we can > turn the tracing on on both sides :-) > e) Is there anyway to make QTAILQ_RAW_INSERT_TAIL any more readable? > I don't think I understand why you can't use the standard QTAILQ macros. I agree with this one. Need to make the trace more specific. > f) You might need to fix up Amit's scripts/vmstate-static-checker.py Will check it. > > Dave > >>> Sascha >>> >> >> Thanks, >> Jianjun >> >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > Thanks, Jianjun
* Paolo Bonzini (pbonzini@redhat.com) wrote: > > > On 07/06/2016 18:34, Dr. David Alan Gilbert wrote: > > * Paolo Bonzini (pbonzini@redhat.com) wrote: > >> > >> > >> On 07/06/2016 16:43, Dr. David Alan Gilbert wrote: > >>> b) I think you should really try and split it into two parts: > >>> 1) A VMSTATE_ARRAY_CUSTOM (?) - so it's an array of elements but we've got a special > >>> way of allocating/counting/reading the elements > >>> 2) A version of that which is for a QTAILQ. > >>> It's important that whatever ends up on the migration stream doesn't reflect > >>> that it happens to be implemented as a QTAILQ; so if you decide to change > >>> it later the migration compatibility doesn't break. > >> > >> (Just to clarify before Jianjun wakes up) a VMSTATE_ARRAY is just a > >> sequence of elements. The count, if needed as in a VARRAY, is stored > >> earlier and separately. Currently lists (including this QTAILQ) are > >> usually represented in the migration stream as a sequence of elements > >> preceded by "1" and terminated by a "0". Would you like to change it to > >> a count + sequence as well? > >> > >> This would prevent using the new QTAILQ support for other existing lists > >> such as virtio-blk and scsi's request lists. > > > > That depends if it's something you're trying to keep migration compatibility > > with; if you're trying to keep format compaibility then sure keep it as is. > > If you're not trying to keep compatibility, then why can't virtio-blk or > > scsi request lists also use a count rather than the markers? > > We're trying to keep compatibility, and I think it's among the last bits > that are resisting conversion to vmstate. Which explains my interest in > Jianjun's patches. :) OK, fine - if you're trying to keep format compatibility then I agree. Although I'm not entirely sure that things like virtio-blk, scsi and everything else are consistent in their structure. Dave > > Paolo > > >>> c) Use new trace_ names for get_qtailq rather than misusing trace_vmstate_load_state* > >>> d) Add a trace_ for put_qtailq as well - that way when it goes horribly wrong we can > >>> turn the tracing on on both sides :-) > >>> e) Is there anyway to make QTAILQ_RAW_INSERT_TAIL any more readable? > >>> I don't think I understand why you can't use the standard QTAILQ macros. > >> > >> Because they assume a particular type. The "raw" version need to work on > >> void*. > > > > Ah OK. > > > > Dave > > > >> > >> Thanks, > >> > >> Paolo > >> > >>> f) You might need to fix up Amit's scripts/vmstate-static-checker.py > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index 56a4171..da4ef7f 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -185,6 +185,8 @@ enum VMStateFlags { * to determine the number of entries in the array. Only valid in * combination with one of VMS_VARRAY*. */ VMS_MULTIPLY_ELEMENTS = 0x4000, + /* For fields which need customized handling, such as QTAILQ in queue.h*/ + VMS_CSTM = 0x8000, }; struct VMStateField { @@ -245,6 +247,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) @@ -656,6 +659,25 @@ extern const VMStateInfo vmstate_info_bitmap; .offset = offsetof(_state, _field), \ } +/* 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, _next) \ +{ \ + .name = (stringify(_field)), \ + .version_id = (_version), \ + .vmsd = &(_vmsd), \ + .size = sizeof(_type), \ + .info = &vmstate_info_qtailq, \ + .flags = VMS_CSTM, \ + .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 f781aa2..003e368 100644 --- a/include/qemu/queue.h +++ b/include/qemu/queue.h @@ -437,3 +437,35 @@ struct { \ (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last)) #endif /* !QEMU_SYS_QUEUE_H_ */ + +/* + * 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) = *((void **) ((char *) (head) + QTAILQ_FIRST_OFFSET)); \ + (elm); \ + (elm) = \ + *((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)) = NULL; \ + *((void **) ((char *) (elm) + (entry) + QTAILQ_PREV_OFFSET)) = \ + *((void **) ((char *) (head) +QTAILQ_LAST_OFFSET)); \ + **((void ***)((char *) (head) +QTAILQ_LAST_OFFSET)) = (elm); \ + *((void **) ((char *) (head) + QTAILQ_LAST_OFFSET)) = \ + (void *) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET); \ +} while (/*CONSTCOND*/0) diff --git a/migration/vmstate.c b/migration/vmstate.c index 644ba1f..ff56650 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" static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd, void *opaque, QJSON *vmdesc); @@ -120,6 +122,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, if (field->flags & VMS_STRUCT) { ret = vmstate_load_state(f, field->vmsd, addr, field->vmsd->version_id); + } else if (field->flags & VMS_CSTM) { + ret = field->info->get(f, addr, size, field); } else { ret = field->info->get(f, addr, size, NULL); @@ -192,6 +196,8 @@ static const char *vmfield_get_type_name(VMStateField *field) if (field->flags & VMS_STRUCT) { type = "struct"; + } else if (field->flags & VMS_CSTM) { + type = "customized"; } else if (field->info->name) { type = field->info->name; } @@ -326,6 +332,8 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd, } if (field->flags & VMS_STRUCT) { vmstate_save_state(f, field->vmsd, addr, vmdesc_loop); + } else if (field->flags & VMS_CSTM) { + field->info->put(f, addr, size, field, vmdesc_loop); } else { field->info->put(f, addr, size, NULL, NULL); } @@ -938,3 +946,74 @@ const VMStateInfo vmstate_info_bitmap = { .get = get_bitmap, .put = put_bitmap, }; + +/*get for QTAILQ */ +static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size, + VMStateField *field) +{ + bool link; + int ret = 0; + const VMStateDescription *vmsd = field->vmsd; + size_t size = field->size; + size_t entry = field->start; + int version_id = field->version_id; + void *elm; + + trace_vmstate_load_state(vmsd->name, version_id); + if (version_id > vmsd->version_id) { + trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL); + return -EINVAL; + } + if (version_id < vmsd->minimum_version_id) { + trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL); + return -EINVAL; + } + + while (true) { + vmstate_info_bool.get(f, &link, sizeof(bool), NULL); + if (!link) { + break; + } + + elm = g_malloc(size); + ret = vmstate_load_state(f, vmsd, elm, version_id); + if (ret) { + return ret; + } + QTAILQ_RAW_INSERT_TAIL(pv, elm, entry); + } + + trace_vmstate_load_state_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) +{ + bool link = true; + const VMStateDescription *vmsd = field->vmsd; + size_t entry = field->start; + void *elm; + + if (vmdesc) { + json_prop_str(vmdesc, "vmsd_name", vmsd->name); + json_prop_int(vmdesc, "version", vmsd->version_id); + json_start_array(vmdesc, "fields"); + } + + QTAILQ_RAW_FOREACH(elm, pv, entry) { + vmstate_info_bool.put(f, &link, sizeof(bool), NULL, NULL); + vmstate_save_state(f, vmsd, elm, vmdesc); + } + link = false; + vmstate_info_bool.put(f, &link, sizeof(bool), NULL, NULL); + if (vmdesc) { + json_end_array(vmdesc); + } +} +const VMStateInfo vmstate_info_qtailq = { + .name = "qtailq", + .get = get_qtailq, + .put = put_qtailq, +};
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_CSTM. We then modified vmstate_save_state and vmstate_load_state so that when VMS_CSTM is encountered, put and get from VMStateInfo are called respectively. 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 <duanj@linux.vnet.ibm.com> --- include/migration/vmstate.h | 22 +++++++++++++ include/qemu/queue.h | 32 ++++++++++++++++++ migration/vmstate.c | 79 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 133 insertions(+)