| Message ID | 1475519097-27611-5-git-send-email-duanj@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* 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 are > called respectively. We created VMStateInfo vmstate_info_qtailq for QTAILQ. > Similar VMStateInfo can be created for other data structures such as list. > This approach will be used to transfer pending_events and ccs_list 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. 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. So would it be easier if you added that new member, then you wouldn't have to modify every get() and put() function that already exists in the previous patch. 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. 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. 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) c) virtio_blk also rolls it's own linked list but again with the 0/1 byte Now how would I modify your QTAILQ load/store to do (a) without copying the whole thing? Dave > > Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com> > --- > 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 = 0x4000, > + /* For fields which need customized handling, such as QTAILQ in queue.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 structure. > + */ > + VMS_LINKED = 0x8000, > }; > > 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; > > #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 = 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_LINKED, \ > + .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..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)) > > +/* > + * 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) > + > #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 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" > > static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd, > void *opaque, QJSON *vmdesc); > @@ -121,6 +123,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_LINKED) { > + ret = field->info->get(f, addr, size, field); > } else { > ret = field->info->get(f, addr, size, NULL); > > @@ -193,6 +197,8 @@ static const char *vmfield_get_type_name(VMStateField *field) > > if (field->flags & VMS_STRUCT) { > type = "struct"; > + } else if (field->flags & VMS_LINKED) { > + type = "linked"; > } else if (field->info->name) { > type = field->info->name; > } > @@ -327,6 +333,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_LINKED) { > + field->info->put(f, addr, size, field, vmdesc_loop); > } else { > field->info->put(f, addr, size, NULL, NULL); > } > @@ -939,3 +947,61 @@ 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) > +{ > + 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_get_qtailq(vmsd->name, version_id); > + if (version_id > vmsd->version_id) { > + trace_get_qtailq_end(vmsd->name, "too new", -EINVAL); Can you make those error_report's please - if it fails we want to see why in the log. Dave > + 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 = g_malloc(size); > + ret = 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 = field->vmsd; > + size_t entry = 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 = { > + .name = "qtailq", > + .get = get_qtailq, > + .put = put_qtailq, > +}; > -- > 1.9.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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 are >> called respectively. We created VMStateInfo vmstate_info_qtailq for QTAILQ. >> Similar VMStateInfo can be created for other data structures such as list. >> This approach will be used to transfer pending_events and ccs_list 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. > > 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. > > So would it be easier if you added that new member, then you wouldn't have to > modify every get() and put() function that already exists in the previous patch. > > 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. > > 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. 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. > 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) > c) virtio_blk also rolls it's own linked list but again with the 0/1 byte > > Now how would I modify your QTAILQ load/store to do (a) without copying the whole > thing? > > Dave > >> >> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com> >> --- >> 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 = 0x4000, >> + /* For fields which need customized handling, such as QTAILQ in queue.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 structure. >> + */ >> + VMS_LINKED = 0x8000, >> }; >> >> 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; >> >> #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 = 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_LINKED, \ >> + .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..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)) >> >> +/* >> + * 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) >> + >> #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 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" >> >> static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd, >> void *opaque, QJSON *vmdesc); >> @@ -121,6 +123,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_LINKED) { >> + ret = field->info->get(f, addr, size, field); >> } else { >> ret = field->info->get(f, addr, size, NULL); >> >> @@ -193,6 +197,8 @@ static const char *vmfield_get_type_name(VMStateField *field) >> >> if (field->flags & VMS_STRUCT) { >> type = "struct"; >> + } else if (field->flags & VMS_LINKED) { >> + type = "linked"; >> } else if (field->info->name) { >> type = field->info->name; >> } >> @@ -327,6 +333,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_LINKED) { >> + field->info->put(f, addr, size, field, vmdesc_loop); >> } else { >> field->info->put(f, addr, size, NULL, NULL); >> } >> @@ -939,3 +947,61 @@ 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) >> +{ >> + 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_get_qtailq(vmsd->name, version_id); >> + if (version_id > vmsd->version_id) { >> + trace_get_qtailq_end(vmsd->name, "too new", -EINVAL); > > Can you make those error_report's please - if it fails we want to > see why in the log. > > Dave > >> + 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 = g_malloc(size); >> + ret = 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 = field->vmsd; >> + size_t entry = 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 = { >> + .name = "qtailq", >> + .get = get_qtailq, >> + .put = put_qtailq, >> +}; >> -- >> 1.9.1 >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > Thanks, Jianjun
On 05/10/2016 18:56, 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 are >> called respectively. We created VMStateInfo vmstate_info_qtailq for QTAILQ. >> Similar VMStateInfo can be created for other data structures such as list. >> This approach will be used to transfer pending_events and ccs_list 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. > > 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. Or just realize that we already have a Turing-complete programming language at our disposal, and it's called C. :) > 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. > > 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. Seriously, the way I'd implement (a) is to make the QTAILQ reader peek for the next byte. If 0, eat it and exit the loop. Otherwise, go ahead with vmstate_load_state which will parse the byte again. Likewise, on saving let the VMState write a non-zero byte at the beginning, and then write a zero at the end. Yes, it's sickening but that's what you do to honor backwards compatibility. The other possibility is just to bump the version and make the SCSI request flag a separate byte after the "is there another entry" byte. Paolo > 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) > > c) virtio_blk also rolls it's own linked list but again with the 0/1 byte > > Now how would I modify your QTAILQ load/store to do (a) without copying the whole > thing? > > Dave > >> >> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com> >> --- >> 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 = 0x4000, >> + /* For fields which need customized handling, such as QTAILQ in queue.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 structure. >> + */ >> + VMS_LINKED = 0x8000, >> }; >> >> 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; >> >> #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 = 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_LINKED, \ >> + .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..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)) >> >> +/* >> + * 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) >> + >> #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 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" >> >> static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd, >> void *opaque, QJSON *vmdesc); >> @@ -121,6 +123,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_LINKED) { >> + ret = field->info->get(f, addr, size, field); >> } else { >> ret = field->info->get(f, addr, size, NULL); >> >> @@ -193,6 +197,8 @@ static const char *vmfield_get_type_name(VMStateField *field) >> >> if (field->flags & VMS_STRUCT) { >> type = "struct"; >> + } else if (field->flags & VMS_LINKED) { >> + type = "linked"; >> } else if (field->info->name) { >> type = field->info->name; >> } >> @@ -327,6 +333,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_LINKED) { >> + field->info->put(f, addr, size, field, vmdesc_loop); >> } else { >> field->info->put(f, addr, size, NULL, NULL); >> } >> @@ -939,3 +947,61 @@ 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) >> +{ >> + 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_get_qtailq(vmsd->name, version_id); >> + if (version_id > vmsd->version_id) { >> + trace_get_qtailq_end(vmsd->name, "too new", -EINVAL); > > Can you make those error_report's please - if it fails we want to > see why in the log. > > Dave > >> + 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 = g_malloc(size); >> + ret = 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 = field->vmsd; >> + size_t entry = 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 = { >> + .name = "qtailq", >> + .get = get_qtailq, >> + .put = put_qtailq, >> +}; >> -- >> 1.9.1 >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > >
* Paolo Bonzini (pbonzini@redhat.com) wrote: > > > On 05/10/2016 18:56, 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 are > >> called respectively. We created VMStateInfo vmstate_info_qtailq for QTAILQ. > >> Similar VMStateInfo can be created for other data structures such as list. > >> This approach will be used to transfer pending_events and ccs_list 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. > > > > 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. > > Or just realize that we already have a Turing-complete programming > language at our disposal, and it's called C. :) Yes, and it does seem that the virtio migration code has used it to its full abilities. > > 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. > > > > 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. > > Seriously, the way I'd implement (a) is to make the QTAILQ reader peek > for the next byte. If 0, eat it and exit the loop. Otherwise, go ahead > with vmstate_load_state which will parse the byte again. > > Likewise, on saving let the VMState write a non-zero byte at the > beginning, and then write a zero at the end. > > Yes, it's sickening but that's what you do to honor backwards compatibility. Actually, that's not *that* bad an idea. Lets go with Jianjun's structure for the moment; we can always expand on it. It seems we have ~3 concepts that feel partially independent: a) The format of the loop on the wire (eg one byte per iteration, 0 terminates) b) The way the list is represented (QTAILQ, simple array, device specific linked-list) c) The data gathered in each iteration d) The allocation of (c) This patch has a,b,d all wrapped up together in the get/put functions - where I was hoping to find a way to separate them a bit so that we could say; I want a loop, with this format, into this data structure, using this allocator. Dave > The other possibility is just to bump the version and make the SCSI > request flag a separate byte after the "is there another entry" byte. > > Paolo > > > 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) > > > > c) virtio_blk also rolls it's own linked list but again with the 0/1 byte > > > > Now how would I modify your QTAILQ load/store to do (a) without copying the whole > > thing? > > > > Dave > > > >> > >> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com> > >> --- > >> 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 = 0x4000, > >> + /* For fields which need customized handling, such as QTAILQ in queue.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 structure. > >> + */ > >> + VMS_LINKED = 0x8000, > >> }; > >> > >> 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; > >> > >> #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 = 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_LINKED, \ > >> + .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..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)) > >> > >> +/* > >> + * 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) > >> + > >> #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 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" > >> > >> static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd, > >> void *opaque, QJSON *vmdesc); > >> @@ -121,6 +123,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_LINKED) { > >> + ret = field->info->get(f, addr, size, field); > >> } else { > >> ret = field->info->get(f, addr, size, NULL); > >> > >> @@ -193,6 +197,8 @@ static const char *vmfield_get_type_name(VMStateField *field) > >> > >> if (field->flags & VMS_STRUCT) { > >> type = "struct"; > >> + } else if (field->flags & VMS_LINKED) { > >> + type = "linked"; > >> } else if (field->info->name) { > >> type = field->info->name; > >> } > >> @@ -327,6 +333,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_LINKED) { > >> + field->info->put(f, addr, size, field, vmdesc_loop); > >> } else { > >> field->info->put(f, addr, size, NULL, NULL); > >> } > >> @@ -939,3 +947,61 @@ 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) > >> +{ > >> + 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_get_qtailq(vmsd->name, version_id); > >> + if (version_id > vmsd->version_id) { > >> + trace_get_qtailq_end(vmsd->name, "too new", -EINVAL); > > > > Can you make those error_report's please - if it fails we want to > > see why in the log. > > > > Dave > > > >> + 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 = g_malloc(size); > >> + ret = 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 = field->vmsd; > >> + size_t entry = 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 = { > >> + .name = "qtailq", > >> + .get = get_qtailq, > >> + .put = put_qtailq, > >> +}; > >> -- > >> 1.9.1 > >> > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 06/10/2016 13:56, Dr. David Alan Gilbert wrote: >> > Yes, it's sickening but that's what you do to honor backwards compatibility. > Actually, that's not *that* bad an idea. > > Lets go with Jianjun's structure for the moment; we can always expand on it. > > It seems we have ~3 concepts that feel partially independent: > > a) The format of the loop on the wire (eg one byte per iteration, 0 terminates) > b) The way the list is represented (QTAILQ, simple array, device specific linked-list) > c) The data gathered in each iteration > d) The allocation of (c) > > This patch has a,b,d all wrapped up together in the get/put functions - > where I was hoping to find a way to separate them a bit so that we > could say; I want a loop, with this format, into this data structure, using this allocator. Yes, the sickening part is when the format of the loop intersects with the format of the datastructure. I agree with moving the allocator out of VMStateInfo and back into VMStateField, but only as long as VMStateAllocator could replace other VMS_* flags. I'm not sure about the value in separating (a) and (b), but we can do things one step at a time. By the way, regarding this: > The other possibility is just to bump the version and make the SCSI > request flag a separate byte after the "is there another entry" byte. There is another way to do it that is much more backwards-compatible. Choose a "default" value of retry corresponding to what QEMU encodes as a "1". If it's different, use a subsection to encode that. Migration from old to new will fail if the wrong value of retry is used, because it will see a 2 where the QTAILQ loop expects a zero or one. Migration from new to old will fail if the wrong value of retry is used, because it will see a subsection header where the QTAILQ loop expects a zero or one. I think this is acceptable, and it would only affect migration of USB storage devices. Paolo
* Paolo Bonzini (pbonzini@redhat.com) wrote: > > > On 06/10/2016 13:56, Dr. David Alan Gilbert wrote: > >> > Yes, it's sickening but that's what you do to honor backwards compatibility. > > Actually, that's not *that* bad an idea. > > > > Lets go with Jianjun's structure for the moment; we can always expand on it. > > > > It seems we have ~3 concepts that feel partially independent: > > > > a) The format of the loop on the wire (eg one byte per iteration, 0 terminates) > > b) The way the list is represented (QTAILQ, simple array, device specific linked-list) > > c) The data gathered in each iteration > > d) The allocation of (c) > > > > This patch has a,b,d all wrapped up together in the get/put functions - > > where I was hoping to find a way to separate them a bit so that we > > could say; I want a loop, with this format, into this data structure, using this allocator. > > Yes, the sickening part is when the format of the loop intersects with > the format of the datastructure. Yes. > I agree with moving the allocator out of VMStateInfo and back into > VMStateField, but only as long as VMStateAllocator could replace other > VMS_* flags. > > I'm not sure about the value in separating (a) and (b), but we can do > things one step at a time. The other observation is that in many of the cases the loop body uses some state present in the outer state or in a value read prior to the start of the loop. For example virtio_blk_load_device uses the vdev pointer inside the loop during the initialisation of each loaded request (I can see some hacky ways of avoiding it but it's messy). > By the way, regarding this: > > > The other possibility is just to bump the version and make the SCSI > > request flag a separate byte after the "is there another entry" byte. > > There is another way to do it that is much more backwards-compatible. > Choose a "default" value of retry corresponding to what QEMU encodes as > a "1". If it's different, use a subsection to encode that. Migration > from old to new will fail if the wrong value of retry is used, because > it will see a 2 where the QTAILQ loop expects a zero or one. Migration > from new to old will fail if the wrong value of retry is used, because > it will see a subsection header where the QTAILQ loop expects a zero or one. > > I think this is acceptable, and it would only affect migration of USB > storage devices. Yes, it might be worth it in some of these cases; although I do try and avoid breaking format at almost all costs. Dave > Paolo -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Jianjun Duan (duanj@linux.vnet.ibm.com) wrote: > > > 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 are > >> called respectively. We created VMStateInfo vmstate_info_qtailq for QTAILQ. > >> Similar VMStateInfo can be created for other data structures such as list. > >> This approach will be used to transfer pending_events and ccs_list 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. > > > > 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. > > > > So would it be easier if you added that new member, then you wouldn't have to > > modify every get() and put() function that already exists in the previous patch. > > > > 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. > > > > 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. > > 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/vmfield_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 = 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 = field->vmsd; size_t size = field->size; int version_id = field->version_id; void *tmp = gmalloc(size); int ret; *(void **)tmp = pv; ret = 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 struct with the first element is 'type1* parent' (compile checked). This would work nicely for where we have to do some maths to generate some temporary results prior to migration; the .pre_save of the vmsd can read the data from pv->parent and write it to the other fields but not have to use qemu_get_*/qemu_put_* at all. Dave > > > 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) > > > c) virtio_blk also rolls it's own linked list but again with the 0/1 byte > > > > Now how would I modify your QTAILQ load/store to do (a) without copying the whole > > thing? > > > > Dave > > > >> > >> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com> > >> --- > >> 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 = 0x4000, > >> + /* For fields which need customized handling, such as QTAILQ in queue.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 structure. > >> + */ > >> + VMS_LINKED = 0x8000, > >> }; > >> > >> 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; > >> > >> #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 = 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_LINKED, \ > >> + .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..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)) > >> > >> +/* > >> + * 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) > >> + > >> #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 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" > >> > >> static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd, > >> void *opaque, QJSON *vmdesc); > >> @@ -121,6 +123,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_LINKED) { > >> + ret = field->info->get(f, addr, size, field); > >> } else { > >> ret = field->info->get(f, addr, size, NULL); > >> > >> @@ -193,6 +197,8 @@ static const char *vmfield_get_type_name(VMStateField *field) > >> > >> if (field->flags & VMS_STRUCT) { > >> type = "struct"; > >> + } else if (field->flags & VMS_LINKED) { > >> + type = "linked"; > >> } else if (field->info->name) { > >> type = field->info->name; > >> } > >> @@ -327,6 +333,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_LINKED) { > >> + field->info->put(f, addr, size, field, vmdesc_loop); > >> } else { > >> field->info->put(f, addr, size, NULL, NULL); > >> } > >> @@ -939,3 +947,61 @@ 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) > >> +{ > >> + 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_get_qtailq(vmsd->name, version_id); > >> + if (version_id > vmsd->version_id) { > >> + trace_get_qtailq_end(vmsd->name, "too new", -EINVAL); > > > > Can you make those error_report's please - if it fails we want to > > see why in the log. > > > > Dave > > > >> + 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 = g_malloc(size); > >> + ret = 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 = field->vmsd; > >> + size_t entry = 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 = { > >> + .name = "qtailq", > >> + .get = get_qtailq, > >> + .put = put_qtailq, > >> +}; > >> -- > >> 1.9.1 > >> > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > > Thanks, > Jianjun > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 10/06/2016 12:01 PM, Dr. David Alan Gilbert wrote: > * Jianjun Duan (duanj@linux.vnet.ibm.com) wrote: >> >> >> 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 are >>>> called respectively. We created VMStateInfo vmstate_info_qtailq for QTAILQ. >>>> Similar VMStateInfo can be created for other data structures such as list. >>>> This approach will be used to transfer pending_events and ccs_list 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. >>> >>> 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. >>> >>> So would it be easier if you added that new member, then you wouldn't have to >>> modify every get() and put() function that already exists in the previous patch. >>> >>> 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. >>> >>> 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. >> >> 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. > It is more complicated if we want to use the exact stream as is now. IMO VMStateInfo provides enough flexibility and is used for migrating scsi_requests. I would stick with it if the same stream layout is to be used. > However, can I make one much simpler request; please split this patch > so that the VMSTATE_LINKED and vmstate_save_state/vmstate_load_state/vmfield_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). > OK. > 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 = 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 = field->vmsd; > size_t size = field->size; > int version_id = field->version_id; > void *tmp = gmalloc(size); > int ret; > > *(void **)tmp = pv; > ret = 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 struct > with the first element is 'type1* parent' (compile checked). > This would work nicely for where we have to do some maths to generate some > temporary results prior to migration; the .pre_save of the vmsd can read the data > from pv->parent and write it to the other fields but not have to use > qemu_get_*/qemu_put_* at all. > This could be a special instance of VMStateInfo. > Dave > >> >>> 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) >> >>> c) virtio_blk also rolls it's own linked list but again with the 0/1 byte >>> >>> Now how would I modify your QTAILQ load/store to do (a) without copying the whole >>> thing? >>> >>> Dave >>> >>>> >>>> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com> >>>> --- >>>> 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 = 0x4000, >>>> + /* For fields which need customized handling, such as QTAILQ in queue.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 structure. >>>> + */ >>>> + VMS_LINKED = 0x8000, >>>> }; >>>> >>>> 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; >>>> >>>> #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 = 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_LINKED, \ >>>> + .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..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)) >>>> >>>> +/* >>>> + * 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) >>>> + >>>> #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 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" >>>> >>>> static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd, >>>> void *opaque, QJSON *vmdesc); >>>> @@ -121,6 +123,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_LINKED) { >>>> + ret = field->info->get(f, addr, size, field); >>>> } else { >>>> ret = field->info->get(f, addr, size, NULL); >>>> >>>> @@ -193,6 +197,8 @@ static const char *vmfield_get_type_name(VMStateField *field) >>>> >>>> if (field->flags & VMS_STRUCT) { >>>> type = "struct"; >>>> + } else if (field->flags & VMS_LINKED) { >>>> + type = "linked"; >>>> } else if (field->info->name) { >>>> type = field->info->name; >>>> } >>>> @@ -327,6 +333,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_LINKED) { >>>> + field->info->put(f, addr, size, field, vmdesc_loop); >>>> } else { >>>> field->info->put(f, addr, size, NULL, NULL); >>>> } >>>> @@ -939,3 +947,61 @@ 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) >>>> +{ >>>> + 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_get_qtailq(vmsd->name, version_id); >>>> + if (version_id > vmsd->version_id) { >>>> + trace_get_qtailq_end(vmsd->name, "too new", -EINVAL); >>> >>> Can you make those error_report's please - if it fails we want to >>> see why in the log. >>> >>> Dave >>> >>>> + 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 = g_malloc(size); >>>> + ret = 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 = field->vmsd; >>>> + size_t entry = 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 = { >>>> + .name = "qtailq", >>>> + .get = get_qtailq, >>>> + .put = put_qtailq, >>>> +}; >>>> -- >>>> 1.9.1 >>>> >>> -- >>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >>> >> Thanks, >> Jianjun >> >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > Thanks, Jianjun
On Thu, Oct 06, 2016 at 08:01:56PM +0100, Dr. David Alan Gilbert wrote: > * Jianjun Duan (duanj@linux.vnet.ibm.com) wrote: > > > > > > 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 are > > >> called respectively. We created VMStateInfo vmstate_info_qtailq for QTAILQ. > > >> Similar VMStateInfo can be created for other data structures such as list. > > >> This approach will be used to transfer pending_events and ccs_list 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. > > > > > > 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. > > > > > > So would it be easier if you added that new member, then you wouldn't have to > > > modify every get() and put() function that already exists in the previous patch. > > > > > > 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. > > > > > > 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. > > > > 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/vmfield_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 = 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 = field->vmsd; > size_t size = field->size; > int version_id = field->version_id; > void *tmp = gmalloc(size); > int ret; > > *(void **)tmp = pv; > ret = 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 struct > with the first element is 'type1* parent' (compile checked). > This would work nicely for where we have to do some maths to generate some > temporary results prior to migration; the .pre_save of the vmsd can read the data > from pv->parent and write it to the other fields but not have to use > qemu_get_*/qemu_put_* at all. > > Dave Oh, I like this idea. I know there are a number of places where should-be-obsolete fields are still present in structures purely to catch incoming migration info which is then converted to the modern representation in post_load. This would allow cleaning a bunch of those up. It would also mean we don't necessarily need explicit handling of queues/lists. I objected to early versions of this series which dumped the qtailq into an array and used the existing array vmstate types, because it meant not just an only-for-migration field in the structure, but a substantial slab of only-for-migration data. If we added the concept of temporary "catching" structures to the vmsd, that objection would go away. I'd be happy enough to temporarily dump the queue into an array, transfer that over the stream into another temporary array, then load it into the destination queue.
On 06/10/2016 21:01, Dr. David Alan Gilbert wrote: >> >> + } else if (field->flags & VMS_LINKED) { >> >> + ret = field->info->get(f, addr, size, field); >> >> } else { >> >> ret = field->info->get(f, addr, size, NULL); >> >> >> >> @@ -193,6 +197,8 @@ static const char *vmfield_get_type_name(VMStateField *field) >> >> >> >> if (field->flags & VMS_STRUCT) { >> >> type = "struct"; >> >> + } else if (field->flags & VMS_LINKED) { >> >> + type = "linked"; >> >> } else if (field->info->name) { >> >> type = field->info->name; >> >> } >> >> @@ -327,6 +333,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_LINKED) { >> >> + field->info->put(f, addr, size, field, vmdesc_loop); >> >> } else { >> >> field->info->put(f, addr, size, NULL, NULL); >> >> } Is VMS_LINKED needed at all, since the fields are unused for every VMStateInfo except qtailq? Paolo
* Paolo Bonzini (pbonzini@redhat.com) wrote: > > > On 06/10/2016 21:01, Dr. David Alan Gilbert wrote: > >> >> + } else if (field->flags & VMS_LINKED) { > >> >> + ret = field->info->get(f, addr, size, field); > >> >> } else { > >> >> ret = field->info->get(f, addr, size, NULL); > >> >> > >> >> @@ -193,6 +197,8 @@ static const char *vmfield_get_type_name(VMStateField *field) > >> >> > >> >> if (field->flags & VMS_STRUCT) { > >> >> type = "struct"; > >> >> + } else if (field->flags & VMS_LINKED) { > >> >> + type = "linked"; > >> >> } else if (field->info->name) { > >> >> type = field->info->name; > >> >> } > >> >> @@ -327,6 +333,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_LINKED) { > >> >> + field->info->put(f, addr, size, field, vmdesc_loop); > >> >> } else { > >> >> field->info->put(f, addr, size, NULL, NULL); > >> >> } > > Is VMS_LINKED needed at all, since the fields are unused for every > VMStateInfo except qtailq? No, I think you could easily drop the VMS_LINKED and just always pass them in. Dave > Paolo -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 10/07/2016 07:34 AM, Dr. David Alan Gilbert wrote: > * Paolo Bonzini (pbonzini@redhat.com) wrote: >> >> >> On 06/10/2016 21:01, Dr. David Alan Gilbert wrote: >>>>>> + } else if (field->flags & VMS_LINKED) { >>>>>> + ret = field->info->get(f, addr, size, field); >>>>>> } else { >>>>>> ret = field->info->get(f, addr, size, NULL); >>>>>> >>>>>> @@ -193,6 +197,8 @@ static const char *vmfield_get_type_name(VMStateField *field) >>>>>> >>>>>> if (field->flags & VMS_STRUCT) { >>>>>> type = "struct"; >>>>>> + } else if (field->flags & VMS_LINKED) { >>>>>> + type = "linked"; >>>>>> } else if (field->info->name) { >>>>>> type = field->info->name; >>>>>> } >>>>>> @@ -327,6 +333,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_LINKED) { >>>>>> + field->info->put(f, addr, size, field, vmdesc_loop); >>>>>> } else { >>>>>> field->info->put(f, addr, size, NULL, NULL); >>>>>> } >> >> Is VMS_LINKED needed at all, since the fields are unused for every >> VMStateInfo except qtailq? > > No, I think you could easily drop the VMS_LINKED and just always pass them in. It is needed if we want to use vmdesc_loop. Thanks, Jianjun > Dave > >> Paolo > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >
On 07/10/2016 18:31, Jianjun Duan wrote: > > > On 10/07/2016 07:34 AM, Dr. David Alan Gilbert wrote: >> * Paolo Bonzini (pbonzini@redhat.com) wrote: >>> >>> >>> On 06/10/2016 21:01, Dr. David Alan Gilbert wrote: >>>>>>> + } else if (field->flags & VMS_LINKED) { >>>>>>> + ret = field->info->get(f, addr, size, field); >>>>>>> } else { >>>>>>> ret = field->info->get(f, addr, size, NULL); >>>>>>> >>>>>>> @@ -193,6 +197,8 @@ static const char *vmfield_get_type_name(VMStateField *field) >>>>>>> >>>>>>> if (field->flags & VMS_STRUCT) { >>>>>>> type = "struct"; >>>>>>> + } else if (field->flags & VMS_LINKED) { >>>>>>> + type = "linked"; >>>>>>> } else if (field->info->name) { >>>>>>> type = field->info->name; >>>>>>> } >>>>>>> @@ -327,6 +333,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_LINKED) { >>>>>>> + field->info->put(f, addr, size, field, vmdesc_loop); >>>>>>> } else { >>>>>>> field->info->put(f, addr, size, NULL, NULL); >>>>>>> } >>> >>> Is VMS_LINKED needed at all, since the fields are unused for every >>> VMStateInfo except qtailq? >> >> No, I think you could easily drop the VMS_LINKED and just always pass them in. > > It is needed if we want to use vmdesc_loop. Just always pass it in, can't you? Paolo
On 10/07/2016 09:32 AM, Paolo Bonzini wrote: > > > On 07/10/2016 18:31, Jianjun Duan wrote: >> >> >> On 10/07/2016 07:34 AM, Dr. David Alan Gilbert wrote: >>> * Paolo Bonzini (pbonzini@redhat.com) wrote: >>>> >>>> >>>> On 06/10/2016 21:01, Dr. David Alan Gilbert wrote: >>>>>>>> + } else if (field->flags & VMS_LINKED) { >>>>>>>> + ret = field->info->get(f, addr, size, field); >>>>>>>> } else { >>>>>>>> ret = field->info->get(f, addr, size, NULL); >>>>>>>> >>>>>>>> @@ -193,6 +197,8 @@ static const char *vmfield_get_type_name(VMStateField *field) >>>>>>>> >>>>>>>> if (field->flags & VMS_STRUCT) { >>>>>>>> type = "struct"; >>>>>>>> + } else if (field->flags & VMS_LINKED) { >>>>>>>> + type = "linked"; >>>>>>>> } else if (field->info->name) { >>>>>>>> type = field->info->name; >>>>>>>> } >>>>>>>> @@ -327,6 +333,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_LINKED) { >>>>>>>> + field->info->put(f, addr, size, field, vmdesc_loop); >>>>>>>> } else { >>>>>>>> field->info->put(f, addr, size, NULL, NULL); >>>>>>>> } >>>> >>>> Is VMS_LINKED needed at all, since the fields are unused for every >>>> VMStateInfo except qtailq? >>> >>> No, I think you could easily drop the VMS_LINKED and just always pass them in. >> >> It is needed if we want to use vmdesc_loop. > > Just always pass it in, can't you? > Could that lead to repetition of array elements? I know it depends on individual put details. Thanks, Jianjun > Paolo >
* Jianjun Duan (duanj@linux.vnet.ibm.com) wrote: > > > On 10/07/2016 09:32 AM, Paolo Bonzini wrote: > > > > > > On 07/10/2016 18:31, Jianjun Duan wrote: > >> > >> > >> On 10/07/2016 07:34 AM, Dr. David Alan Gilbert wrote: > >>> * Paolo Bonzini (pbonzini@redhat.com) wrote: > >>>> > >>>> > >>>> On 06/10/2016 21:01, Dr. David Alan Gilbert wrote: > >>>>>>>> + } else if (field->flags & VMS_LINKED) { > >>>>>>>> + ret = field->info->get(f, addr, size, field); > >>>>>>>> } else { > >>>>>>>> ret = field->info->get(f, addr, size, NULL); > >>>>>>>> > >>>>>>>> @@ -193,6 +197,8 @@ static const char *vmfield_get_type_name(VMStateField *field) > >>>>>>>> > >>>>>>>> if (field->flags & VMS_STRUCT) { > >>>>>>>> type = "struct"; > >>>>>>>> + } else if (field->flags & VMS_LINKED) { > >>>>>>>> + type = "linked"; > >>>>>>>> } else if (field->info->name) { > >>>>>>>> type = field->info->name; > >>>>>>>> } > >>>>>>>> @@ -327,6 +333,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_LINKED) { > >>>>>>>> + field->info->put(f, addr, size, field, vmdesc_loop); > >>>>>>>> } else { > >>>>>>>> field->info->put(f, addr, size, NULL, NULL); > >>>>>>>> } > >>>> > >>>> Is VMS_LINKED needed at all, since the fields are unused for every > >>>> VMStateInfo except qtailq? > >>> > >>> No, I think you could easily drop the VMS_LINKED and just always pass them in. > >> > >> It is needed if we want to use vmdesc_loop. > > > > Just always pass it in, can't you? > > > Could that lead to repetition of array elements? I know it depends on > individual put details. I don't think so; Paolo is just suggesting replacing: + } else if (field->flags & VMS_LINKED) { + field->info->put(f, addr, size, field, vmdesc_loop); } else { field->info->put(f, addr, size, NULL, NULL); } by: } else { - field->info->put(f, addr, size, NULL, NULL); + field->info->put(f, addr, size, field, vmdesc_loop); } most get/put won't use the field/vmdesc_loop so it wont matter. Dave > > Thanks, > Jianjun > > Paolo > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 10/07/2016 10:34 AM, Dr. David Alan Gilbert wrote: > * Jianjun Duan (duanj@linux.vnet.ibm.com) wrote: >> >> >> On 10/07/2016 09:32 AM, Paolo Bonzini wrote: >>> >>> >>> On 07/10/2016 18:31, Jianjun Duan wrote: >>>> >>>> >>>> On 10/07/2016 07:34 AM, Dr. David Alan Gilbert wrote: >>>>> * Paolo Bonzini (pbonzini@redhat.com) wrote: >>>>>> >>>>>> >>>>>> On 06/10/2016 21:01, Dr. David Alan Gilbert wrote: >>>>>>>>>> + } else if (field->flags & VMS_LINKED) { >>>>>>>>>> + ret = field->info->get(f, addr, size, field); >>>>>>>>>> } else { >>>>>>>>>> ret = field->info->get(f, addr, size, NULL); >>>>>>>>>> >>>>>>>>>> @@ -193,6 +197,8 @@ static const char *vmfield_get_type_name(VMStateField *field) >>>>>>>>>> >>>>>>>>>> if (field->flags & VMS_STRUCT) { >>>>>>>>>> type = "struct"; >>>>>>>>>> + } else if (field->flags & VMS_LINKED) { >>>>>>>>>> + type = "linked"; >>>>>>>>>> } else if (field->info->name) { >>>>>>>>>> type = field->info->name; >>>>>>>>>> } >>>>>>>>>> @@ -327,6 +333,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_LINKED) { >>>>>>>>>> + field->info->put(f, addr, size, field, vmdesc_loop); >>>>>>>>>> } else { >>>>>>>>>> field->info->put(f, addr, size, NULL, NULL); >>>>>>>>>> } >>>>>> >>>>>> Is VMS_LINKED needed at all, since the fields are unused for every >>>>>> VMStateInfo except qtailq? >>>>> >>>>> No, I think you could easily drop the VMS_LINKED and just always pass them in. >>>> >>>> It is needed if we want to use vmdesc_loop. >>> >>> Just always pass it in, can't you? >>> >> Could that lead to repetition of array elements? I know it depends on >> individual put details. > > I don't think so; Paolo is just suggesting replacing: > > + } else if (field->flags & VMS_LINKED) { > + field->info->put(f, addr, size, field, vmdesc_loop); > } else { > field->info->put(f, addr, size, NULL, NULL); > } > > by: > > } else { > - field->info->put(f, addr, size, NULL, NULL); > + field->info->put(f, addr, size, field, vmdesc_loop); > } > > most get/put won't use the field/vmdesc_loop so it wont matter. > > Dave > Even though most put/get have no issues now, when somebody writes a new put, he or she could run into issues if only checking the type signature. It makes the code more readable. I am OK either way. Thanks, Jianjun >> >> Thanks, >> Jianjun >>> Paolo >>> >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >
On 07/10/2016 19:43, Jianjun Duan wrote: > Even though most put/get have no issues now, when somebody writes a new > put, he or she could run into issues if only checking the type > signature. It makes the code more readable. No, it doesn't because one is left wondering what is VMS_LINKED about. What is the relation between linked datastructures and passing some arguments as NULL/noon-NULL? When somebody writes a new get/put, they'll make an informed choice on what arguments they need. :) Paolo
On 10/08/2016 01:37 PM, Paolo Bonzini wrote: >> Even though most put/get have no issues now, when somebody writes a new >> > put, he or she could run into issues if only checking the type >> > signature. It makes the code more readable. > No, it doesn't because one is left wondering what is VMS_LINKED about. I agree with Paolo. IMHO VMS_LINKED is conceptually difficult in a sense that it's quite different that what we have currently. I have the feeling, conceptually, we are trying to fit in something like data structures with a type parameter (the element type) here. AFAIU what vmstate currently can is directed acyclic graphs of certain stuff (and also completely custom handling based on custom put/get). > What is the relation between linked datastructures and passing some > arguments as NULL/noon-NULL? IFAIU we need those for the datastructure because it's linked and has a type parameter (element type). The two last arguments are for the element type. These were added by the previous patch because the old VMStateInfo did not need these. So we drastically changed the scope of VMStateInfo with the previous patch and I'm not sure I like this. I will have to stare at this a bit longer to bring something more constructive than these (largely feeling-based) remarks. Cheers, Halil
On 10/08/2016 12:28 PM, Halil Pasic wrote: > > > On 10/08/2016 01:37 PM, Paolo Bonzini wrote: >>> Even though most put/get have no issues now, when somebody writes a new >>>> put, he or she could run into issues if only checking the type >>>> signature. It makes the code more readable. > >> No, it doesn't because one is left wondering what is VMS_LINKED about. > > I agree with Paolo. IMHO VMS_LINKED is conceptually difficult in a sense > that it's quite different that what we have currently. I have the feeling, > conceptually, we are trying to fit in something like data structures with a > type parameter (the element type) here. AFAIU what vmstate currently can is > directed acyclic graphs of certain stuff (and also completely custom > handling based on custom put/get). > You are right. What we have in VMSTATE now cannot handle a recursive (or cyclic as you call it) structure. The idea was to use VMS_LINKED to indicate a recursive structure. In this patch is used on a queue. It can also be used on list or trees. In this regard, VMS_LINKED does represent something. Thanks, Jianjun >> What is the relation between linked datastructures and passing some >> arguments as NULL/noon-NULL? > > IFAIU we need those for the datastructure because it's linked and has > a type parameter (element type). The two last arguments are for > the element type. These were added by the previous patch because > the old VMStateInfo did not need these. So we drastically changed > the scope of VMStateInfo with the previous patch and I'm not > sure I like this. > > I will have to stare at this a bit longer to bring something more > constructive than these (largely feeling-based) remarks. > > Cheers, > Halil >
On 10/08/2016 12:28 PM, Halil Pasic wrote: > > > On 10/08/2016 01:37 PM, Paolo Bonzini wrote: >>> Even though most put/get have no issues now, when somebody writes a new >>>> put, he or she could run into issues if only checking the type >>>> signature. It makes the code more readable. > >> No, it doesn't because one is left wondering what is VMS_LINKED about. > > I agree with Paolo. IMHO VMS_LINKED is conceptually difficult in a sense > that it's quite different that what we have currently. I have the feeling, > conceptually, we are trying to fit in something like data structures with a > type parameter (the element type) here. AFAIU what vmstate currently can is > directed acyclic graphs of certain stuff (and also completely custom > handling based on custom put/get). > You are right. What we have in vmstate now cannot handle a recursive or cyclic structure in a generic way. VMS_LINKED is intended to indicate a recursive structure, with the newly added parameters of put/get providing information about the element type in the recursive structure. Thanks, Jianjun >> What is the relation between linked datastructures and passing some >> arguments as NULL/noon-NULL? > > IFAIU we need those for the datastructure because it's linked and has > a type parameter (element type). The two last arguments are for > the element type. These were added by the previous patch because > the old VMStateInfo did not need these. So we drastically changed > the scope of VMStateInfo with the previous patch and I'm not > sure I like this. > > I will have to stare at this a bit longer to bring something more > constructive than these (largely feeling-based) remarks. > > Cheers, > Halil >
On 10/10/2016 23:29, Jianjun Duan wrote: > You are right. What we have in VMSTATE now cannot handle a recursive (or > cyclic as you call it) structure. The idea was to use VMS_LINKED to > indicate a recursive structure. Sure, but it's unnecessary. If you didn't have VMS_LINKED, no one would notice the difference. Yes, existing VMStateInfos didn't need the new arguments, the new one does. So you add the new arguments. There's no need to add the flag. Paolo > In this patch is used on a queue. It can > also be used on list or trees. > In this regard, VMS_LINKED does represent something.
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 = 0x4000, + /* For fields which need customized handling, such as QTAILQ in queue.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 structure. + */ + VMS_LINKED = 0x8000, }; 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; #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 = 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_LINKED, \ + .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..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)) +/* + * 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) + #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 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" static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd, void *opaque, QJSON *vmdesc); @@ -121,6 +123,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_LINKED) { + ret = field->info->get(f, addr, size, field); } else { ret = field->info->get(f, addr, size, NULL); @@ -193,6 +197,8 @@ static const char *vmfield_get_type_name(VMStateField *field) if (field->flags & VMS_STRUCT) { type = "struct"; + } else if (field->flags & VMS_LINKED) { + type = "linked"; } else if (field->info->name) { type = field->info->name; } @@ -327,6 +333,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_LINKED) { + field->info->put(f, addr, size, field, vmdesc_loop); } else { field->info->put(f, addr, size, NULL, NULL); } @@ -939,3 +947,61 @@ 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) +{ + 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_get_qtailq(vmsd->name, version_id); + if (version_id > vmsd->version_id) { + trace_get_qtailq_end(vmsd->name, "too new", -EINVAL); + 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 = g_malloc(size); + ret = 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 = field->vmsd; + size_t entry = 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 = { + .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_LINKED. We then modified vmstate_save_state and vmstate_load_state so that when VMS_LINKED is encountered, put and get from VMStateInfo are called respectively. We created VMStateInfo vmstate_info_qtailq for QTAILQ. Similar VMStateInfo can be created for other data structures such as list. This approach will be used to transfer pending_events and ccs_list 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 | 26 ++++++++++++++++++ include/qemu/queue.h | 32 ++++++++++++++++++++++ migration/trace-events | 4 +++ migration/vmstate.c | 66 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 128 insertions(+)