| Message ID | 1478563599-4228-3-git-send-email-duanj@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/08/2016 01:06 AM, Jianjun Duan wrote: > Currently we cannot directly transfer a QTAILQ instance because of the > limitation in the migration code. Here we introduce an approach to > transfer such structures. We created VMStateInfo vmstate_info_qtailq > for QTAILQ. Similar VMStateInfo can be created for other data structures > such as list. > > When a QTAILQ is migrated from source to target, it is appended to the > corresponding QTAILQ structure, which is assumed to have been properly > initialized. > > 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 | 20 +++++++++++++ > include/qemu/queue.h | 60 +++++++++++++++++++++++++++++++++++++++ > migration/trace-events | 4 +++ > migration/vmstate.c | 69 +++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 153 insertions(+) > > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h > index eafc8f2..6289327 100644 > --- a/include/migration/vmstate.h > +++ b/include/migration/vmstate.h > @@ -253,6 +253,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) > @@ -664,6 +665,25 @@ extern const VMStateInfo vmstate_info_bitmap; > .offset = offsetof(_state, _field), \ > } > > +/* For QTAILQ that need customized handling. What do you mean by customized handling? How does non-customized handling look like? I would also add something like works only for movable elements/nodes to make it clear that scenarios like Juan has pointed out previously are not currently supported. > + * Target QTAILQ needs be properly initialized. Could this restriction be avoided by doing (1)? > + * _type: type of QTAILQ element > + * _next: name of QTAILQ entry field in QTAILQ element > + * _vmsd: VMSD for QTAILQ element > + * size: size of QTAILQ element > + * start: offset of QTAILQ entry in QTAILQ element > + */ > +#define VMSTATE_QTAILQ_V(_field, _state, _version, _vmsd, _type, _next) \ > +{ \ > + .name = (stringify(_field)), \ > + .version_id = (_version), \ > + .vmsd = &(_vmsd), \ > + .size = sizeof(_type), \ > + .info = &vmstate_info_qtailq, \ > + .offset = offsetof(_state, _field), \ > + .start = offsetof(_type, _next), \ > +} > + > /* _f : field name > _f_n : num of elements field_name > _n : num of elements > diff --git a/include/qemu/queue.h b/include/qemu/queue.h > index 342073f..75616e1 100644 > --- a/include/qemu/queue.h > +++ b/include/qemu/queue.h > @@ -438,4 +438,64 @@ struct { \ > #define QTAILQ_PREV(elm, headname, field) \ > (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last)) > > +#define field_at_offset(base, offset, type) \ > + ((type) (((char *) (base)) + (offset))) > + > +typedef struct DUMMY_Q_ENTRY DUMMY_Q_ENTRY; > +typedef struct DUMMY_Q DUMMY_Q; > + > +struct DUMMY_Q_ENTRY { > + QTAILQ_ENTRY(DUMMY_Q_ENTRY) next; > +}; > + > +struct DUMMY_Q { > + QTAILQ_HEAD(DUMMY_Q_HEAD, DUMMY_Q_ENTRY) head; > +}; > + > +#define dummy_q ((DUMMY_Q *) 0) > +#define dummy_qe ((DUMMY_Q_ENTRY *) 0) > + > +/* > + * Offsets of layout of a tail queue head. > + */ > +#define QTAILQ_FIRST_OFFSET (offsetof(typeof(dummy_q->head), tqh_first)) > +#define QTAILQ_LAST_OFFSET (offsetof(typeof(dummy_q->head), tqh_last)) > +/* > + * Raw access of elements of a tail queue > + */ > +#define QTAILQ_RAW_FIRST(head) \ > + (*field_at_offset(head, QTAILQ_FIRST_OFFSET, void **)) > +#define QTAILQ_RAW_LAST(head) \ > + (*field_at_offset(head, QTAILQ_LAST_OFFSET, void ***)) > + > +/* > + * Offsets of layout of a tail queue element. > + */ > +#define QTAILQ_NEXT_OFFSET (offsetof(typeof(dummy_qe->next), tqe_next)) > +#define QTAILQ_PREV_OFFSET (offsetof(typeof(dummy_qe->next), tqe_prev)) > + > +/* > + * Raw access of elements of a tail entry > + */ > +#define QTAILQ_RAW_NEXT(elm, entry) \ > + (*field_at_offset(elm, entry + QTAILQ_NEXT_OFFSET, void **)) > +#define QTAILQ_RAW_PREV(elm, entry) \ > + (*field_at_offset(elm, entry + QTAILQ_PREV_OFFSET, void ***)) AFAIU QTAILQ_RAW_PREV and QTAILQ_PREV have completely different semantic. Don't think its quite likely anybody will need QTAILQ_RAW_PREV with the QTAILQ_PREV semantic (that is pointer to the previous element -- and not to the prev next), nevertheless I think picking a different name would be a good idea. How about using QTAILQ_RAW_TQE_NEXT and QTAILQ_RAW_TQE_PREV instead? > +/* > + * Tail queue tranversal using pointer arithmetic. > + */ > +#define QTAILQ_RAW_FOREACH(elm, head, entry) \ > + for ((elm) = QTAILQ_RAW_FIRST(head); \ > + (elm); \ > + (elm) = QTAILQ_RAW_NEXT(elm, entry)) > +/* > + * Tail queue insertion using pointer arithmetic. > + */ > +#define QTAILQ_RAW_INSERT_TAIL(head, elm, entry) do { \ > + QTAILQ_RAW_NEXT(elm, entry) = NULL; \ > + QTAILQ_RAW_PREV(elm, entry) = QTAILQ_RAW_LAST(head); \ > + *QTAILQ_RAW_LAST(head) = (elm); \ > + QTAILQ_RAW_LAST(head) = &QTAILQ_RAW_NEXT(elm, entry); \ > +} while (/*CONSTCOND*/0) > + > #endif /* QEMU_SYS_QUEUE_H */ > diff --git a/migration/trace-events b/migration/trace-events > index 94134f7..c46f9e9 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 7b4bd6e..2f9d4ba 100644 > --- a/migration/vmstate.c > +++ b/migration/vmstate.c > @@ -5,6 +5,7 @@ > #include "migration/vmstate.h" > #include "qemu/bitops.h" > #include "qemu/error-report.h" > +#include "qemu/queue.h" > #include "trace.h" > #include "migration/qjson.h" > > @@ -965,3 +966,71 @@ const VMStateInfo vmstate_info_bitmap = { > .get = get_bitmap, > .put = put_bitmap, > }; > + > +/* get for QTAILQ > + * meta data about the QTAILQ is encoded in a VMStateField structure > + */ > +static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size, > + VMStateField *field) > +{ > + int ret = 0; > + const VMStateDescription *vmsd = field->vmsd; > + /* size of a QTAILQ element */ > + size_t size = field->size; > + /* offset of the QTAILQ entry in a QTAILQ element */ > + size_t entry_offset = field->start; > + int version_id = field->version_id; > + void *elm; > + > + trace_get_qtailq(vmsd->name, version_id); > + if (version_id > vmsd->version_id) { > + error_report("%s %s", vmsd->name, "too new"); > + trace_get_qtailq_end(vmsd->name, "too new", -EINVAL); > + > + return -EINVAL; > + } > + if (version_id < vmsd->minimum_version_id) { > + error_report("%s %s", vmsd->name, "too old"); > + trace_get_qtailq_end(vmsd->name, "too old", -EINVAL); > + return -EINVAL; > + } > + (1) Why not do + QTAILQ_INIT((DUMMY_Q *)pv); Halil > + 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_offset); > + } > + > + trace_get_qtailq_end(vmsd->name, "end", ret); > + return ret; > +} > + > +/* put for QTAILQ */ > +static int put_qtailq(QEMUFile *f, void *pv, size_t unused_size, > + VMStateField *field, QJSON *vmdesc) > +{ > + const VMStateDescription *vmsd = field->vmsd; > + /* offset of the QTAILQ entry in a QTAILQ element*/ > + size_t entry_offset = field->start; > + void *elm; > + > + trace_put_qtailq(vmsd->name, vmsd->version_id); > + > + QTAILQ_RAW_FOREACH(elm, pv, entry_offset) { > + qemu_put_byte(f, true); > + vmstate_save_state(f, vmsd, elm, vmdesc); > + } > + qemu_put_byte(f, false); > + > + trace_put_qtailq_end(vmsd->name, "end"); > + > + return 0; > +} > +const VMStateInfo vmstate_info_qtailq = { > + .name = "qtailq", > + .get = get_qtailq, > + .put = put_qtailq, > +}; >
On 11/10/2016 04:04 AM, Halil Pasic wrote: > > > On 11/08/2016 01:06 AM, Jianjun Duan wrote: >> Currently we cannot directly transfer a QTAILQ instance because of the >> limitation in the migration code. Here we introduce an approach to >> transfer such structures. We created VMStateInfo vmstate_info_qtailq >> for QTAILQ. Similar VMStateInfo can be created for other data structures >> such as list. >> >> When a QTAILQ is migrated from source to target, it is appended to the >> corresponding QTAILQ structure, which is assumed to have been properly >> initialized. >> >> 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 | 20 +++++++++++++ >> include/qemu/queue.h | 60 +++++++++++++++++++++++++++++++++++++++ >> migration/trace-events | 4 +++ >> migration/vmstate.c | 69 +++++++++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 153 insertions(+) >> >> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h >> index eafc8f2..6289327 100644 >> --- a/include/migration/vmstate.h >> +++ b/include/migration/vmstate.h >> @@ -253,6 +253,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) >> @@ -664,6 +665,25 @@ extern const VMStateInfo vmstate_info_bitmap; >> .offset = offsetof(_state, _field), \ >> } >> >> +/* For QTAILQ that need customized handling. > > What do you mean by customized handling? How does non-customized > handling look like? > Customized handling means a user has to implement put/get to get the job done while normally one expects to just specify a VMSD to get the job done. > I would also add something like works only for movable > elements/nodes to make it clear that scenarios like Juan has > pointed out previously are not currently supported. > Maybe you mean the backward compatibility regarding the bit stream? It is not supported. Will document it. >> + * Target QTAILQ needs be properly initialized. > > Could this restriction be avoided by doing (1)? > >> + * _type: type of QTAILQ element >> + * _next: name of QTAILQ entry field in QTAILQ element >> + * _vmsd: VMSD for QTAILQ element >> + * size: size of QTAILQ element >> + * start: offset of QTAILQ entry in QTAILQ element >> + */ >> +#define VMSTATE_QTAILQ_V(_field, _state, _version, _vmsd, _type, _next) \ >> +{ \ >> + .name = (stringify(_field)), \ >> + .version_id = (_version), \ >> + .vmsd = &(_vmsd), \ >> + .size = sizeof(_type), \ >> + .info = &vmstate_info_qtailq, \ >> + .offset = offsetof(_state, _field), \ >> + .start = offsetof(_type, _next), \ >> +} >> + >> /* _f : field name >> _f_n : num of elements field_name >> _n : num of elements >> diff --git a/include/qemu/queue.h b/include/qemu/queue.h >> index 342073f..75616e1 100644 >> --- a/include/qemu/queue.h >> +++ b/include/qemu/queue.h >> @@ -438,4 +438,64 @@ struct { \ >> #define QTAILQ_PREV(elm, headname, field) \ >> (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last)) >> >> +#define field_at_offset(base, offset, type) \ >> + ((type) (((char *) (base)) + (offset))) >> + >> +typedef struct DUMMY_Q_ENTRY DUMMY_Q_ENTRY; >> +typedef struct DUMMY_Q DUMMY_Q; >> + >> +struct DUMMY_Q_ENTRY { >> + QTAILQ_ENTRY(DUMMY_Q_ENTRY) next; >> +}; >> + >> +struct DUMMY_Q { >> + QTAILQ_HEAD(DUMMY_Q_HEAD, DUMMY_Q_ENTRY) head; >> +}; >> + >> +#define dummy_q ((DUMMY_Q *) 0) >> +#define dummy_qe ((DUMMY_Q_ENTRY *) 0) >> + >> +/* >> + * Offsets of layout of a tail queue head. >> + */ >> +#define QTAILQ_FIRST_OFFSET (offsetof(typeof(dummy_q->head), tqh_first)) >> +#define QTAILQ_LAST_OFFSET (offsetof(typeof(dummy_q->head), tqh_last)) >> +/* >> + * Raw access of elements of a tail queue >> + */ >> +#define QTAILQ_RAW_FIRST(head) \ >> + (*field_at_offset(head, QTAILQ_FIRST_OFFSET, void **)) >> +#define QTAILQ_RAW_LAST(head) \ >> + (*field_at_offset(head, QTAILQ_LAST_OFFSET, void ***)) >> + >> +/* >> + * Offsets of layout of a tail queue element. >> + */ >> +#define QTAILQ_NEXT_OFFSET (offsetof(typeof(dummy_qe->next), tqe_next)) >> +#define QTAILQ_PREV_OFFSET (offsetof(typeof(dummy_qe->next), tqe_prev)) >> + >> +/* >> + * Raw access of elements of a tail entry >> + */ >> +#define QTAILQ_RAW_NEXT(elm, entry) \ >> + (*field_at_offset(elm, entry + QTAILQ_NEXT_OFFSET, void **)) >> +#define QTAILQ_RAW_PREV(elm, entry) \ >> + (*field_at_offset(elm, entry + QTAILQ_PREV_OFFSET, void ***)) > > AFAIU QTAILQ_RAW_PREV and QTAILQ_PREV have completely different semantic. > Don't think its quite likely anybody will need QTAILQ_RAW_PREV with the > QTAILQ_PREV semantic (that is pointer to the previous element -- and not > to the prev next), nevertheless I think picking a different name would > be a good idea. How about using QTAILQ_RAW_TQE_NEXT and QTAILQ_RAW_TQE_PREV > instead? > Good catch. Will change the names even though the comments were pretty clear. >> +/* >> + * Tail queue tranversal using pointer arithmetic. >> + */ >> +#define QTAILQ_RAW_FOREACH(elm, head, entry) \ >> + for ((elm) = QTAILQ_RAW_FIRST(head); \ >> + (elm); \ >> + (elm) = QTAILQ_RAW_NEXT(elm, entry)) >> +/* >> + * Tail queue insertion using pointer arithmetic. >> + */ >> +#define QTAILQ_RAW_INSERT_TAIL(head, elm, entry) do { \ >> + QTAILQ_RAW_NEXT(elm, entry) = NULL; \ >> + QTAILQ_RAW_PREV(elm, entry) = QTAILQ_RAW_LAST(head); \ >> + *QTAILQ_RAW_LAST(head) = (elm); \ >> + QTAILQ_RAW_LAST(head) = &QTAILQ_RAW_NEXT(elm, entry); \ >> +} while (/*CONSTCOND*/0) >> + >> #endif /* QEMU_SYS_QUEUE_H */ >> diff --git a/migration/trace-events b/migration/trace-events >> index 94134f7..c46f9e9 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 7b4bd6e..2f9d4ba 100644 >> --- a/migration/vmstate.c >> +++ b/migration/vmstate.c >> @@ -5,6 +5,7 @@ >> #include "migration/vmstate.h" >> #include "qemu/bitops.h" >> #include "qemu/error-report.h" >> +#include "qemu/queue.h" >> #include "trace.h" >> #include "migration/qjson.h" >> >> @@ -965,3 +966,71 @@ const VMStateInfo vmstate_info_bitmap = { >> .get = get_bitmap, >> .put = put_bitmap, >> }; >> + >> +/* get for QTAILQ >> + * meta data about the QTAILQ is encoded in a VMStateField structure >> + */ >> +static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size, >> + VMStateField *field) >> +{ >> + int ret = 0; >> + const VMStateDescription *vmsd = field->vmsd; >> + /* size of a QTAILQ element */ >> + size_t size = field->size; >> + /* offset of the QTAILQ entry in a QTAILQ element */ >> + size_t entry_offset = field->start; >> + int version_id = field->version_id; >> + void *elm; >> + >> + trace_get_qtailq(vmsd->name, version_id); >> + if (version_id > vmsd->version_id) { >> + error_report("%s %s", vmsd->name, "too new"); >> + trace_get_qtailq_end(vmsd->name, "too new", -EINVAL); >> + >> + return -EINVAL; >> + } >> + if (version_id < vmsd->minimum_version_id) { >> + error_report("%s %s", vmsd->name, "too old"); >> + trace_get_qtailq_end(vmsd->name, "too old", -EINVAL); >> + return -EINVAL; >> + } >> + > > (1) Why not do > + QTAILQ_INIT((DUMMY_Q *)pv); > As discussed before, the qtailq on the target is assumed to be initialized and may not be empty. That is the case at least in the DRC stuff. Doing another init may cause issues and memory leak. Thanks, Jianjun > Halil > >> + 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_offset); >> + } >> + >> + trace_get_qtailq_end(vmsd->name, "end", ret); >> + return ret; >> +} >> + >> +/* put for QTAILQ */ >> +static int put_qtailq(QEMUFile *f, void *pv, size_t unused_size, >> + VMStateField *field, QJSON *vmdesc) >> +{ >> + const VMStateDescription *vmsd = field->vmsd; >> + /* offset of the QTAILQ entry in a QTAILQ element*/ >> + size_t entry_offset = field->start; >> + void *elm; >> + >> + trace_put_qtailq(vmsd->name, vmsd->version_id); >> + >> + QTAILQ_RAW_FOREACH(elm, pv, entry_offset) { >> + qemu_put_byte(f, true); >> + vmstate_save_state(f, vmsd, elm, vmdesc); >> + } >> + qemu_put_byte(f, false); >> + >> + trace_put_qtailq_end(vmsd->name, "end"); >> + >> + return 0; >> +} >> +const VMStateInfo vmstate_info_qtailq = { >> + .name = "qtailq", >> + .get = get_qtailq, >> + .put = put_qtailq, >> +}; >> >
On 11/10/2016 07:00 PM, Jianjun Duan wrote: > > > On 11/10/2016 04:04 AM, Halil Pasic wrote: >> >> >> On 11/08/2016 01:06 AM, Jianjun Duan wrote: >>> Currently we cannot directly transfer a QTAILQ instance because of the >>> limitation in the migration code. Here we introduce an approach to >>> transfer such structures. We created VMStateInfo vmstate_info_qtailq >>> for QTAILQ. Similar VMStateInfo can be created for other data structures >>> such as list. >>> >>> When a QTAILQ is migrated from source to target, it is appended to the >>> corresponding QTAILQ structure, which is assumed to have been properly >>> initialized. >>> >>> 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 | 20 +++++++++++++ >>> include/qemu/queue.h | 60 +++++++++++++++++++++++++++++++++++++++ >>> migration/trace-events | 4 +++ >>> migration/vmstate.c | 69 +++++++++++++++++++++++++++++++++++++++++++++ >>> 4 files changed, 153 insertions(+) >>> >>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h >>> index eafc8f2..6289327 100644 >>> --- a/include/migration/vmstate.h >>> +++ b/include/migration/vmstate.h >>> @@ -253,6 +253,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) >>> @@ -664,6 +665,25 @@ extern const VMStateInfo vmstate_info_bitmap; >>> .offset = offsetof(_state, _field), \ >>> } >>> >>> +/* For QTAILQ that need customized handling. >> >> What do you mean by customized handling? How does non-customized >> handling look like? >> > Customized handling means a user has to implement put/get to get the job > done while normally one expects to just specify a VMSD to get the job done. I would drop the customized since. The user does not have t o implement put/get since the (not user supplied) .info that takes care of the stuff. Now assumed I'm a random guy who wants his QTAILQ migrated, if I read 'For QTAILQ that need customized handling' I ask myself does _my_ qtailq need any customized handling. I have a pretty plain qtailq so it should not need any customized handling. It may be only my limited English skills though. I'm not anywhere near of being a native speaker. IMHO if you want to indicate that stuff is custom because you have both vmsd and info I would place a comment next to the info. I think from language perspective that should be completely OK assuming t least c99 -- or at least I hope so. The comment above the macro has an apidoc feel to me so indicating it there feels wrong. >> I would also add something like works only for movable >> elements/nodes to make it clear that scenarios like Juan has >> pointed out previously are not currently supported. >> > Maybe you mean the backward compatibility regarding the bit stream? > It is not supported. Will document it. I mean this: https://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg00314.html If you look at the code the VirtQueue instances sit in a array and are organized into lists depending on to which vector do these belong to (disclaimer: my understanding of the problem formulated very sloppily). I missed the discussion on backward compatibility of the bitstream. Could you give me a pointer? >>> + * Target QTAILQ needs be properly initialized. >> >> Could this restriction be avoided by doing (1)? >> >>> + * _type: type of QTAILQ element >>> + * _next: name of QTAILQ entry field in QTAILQ element >>> + * _vmsd: VMSD for QTAILQ element >>> + * size: size of QTAILQ element >>> + * start: offset of QTAILQ entry in QTAILQ element >>> + */ >>> +#define VMSTATE_QTAILQ_V(_field, _state, _version, _vmsd, _type, _next) \ >>> +{ \ >>> + .name = (stringify(_field)), \ >>> + .version_id = (_version), \ >>> + .vmsd = &(_vmsd), \ >>> + .size = sizeof(_type), \ >>> + .info = &vmstate_info_qtailq, \ >>> + .offset = offsetof(_state, _field), \ >>> + .start = offsetof(_type, _next), \ >>> +} >>> + >>> /* _f : field name >>> _f_n : num of elements field_name >>> _n : num of elements >>> diff --git a/include/qemu/queue.h b/include/qemu/queue.h >>> index 342073f..75616e1 100644 >>> --- a/include/qemu/queue.h >>> +++ b/include/qemu/queue.h >>> @@ -438,4 +438,64 @@ struct { \ >>> #define QTAILQ_PREV(elm, headname, field) \ >>> (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last)) >>> >>> +#define field_at_offset(base, offset, type) \ >>> + ((type) (((char *) (base)) + (offset))) >>> + >>> +typedef struct DUMMY_Q_ENTRY DUMMY_Q_ENTRY; >>> +typedef struct DUMMY_Q DUMMY_Q; >>> + >>> +struct DUMMY_Q_ENTRY { >>> + QTAILQ_ENTRY(DUMMY_Q_ENTRY) next; >>> +}; >>> + >>> +struct DUMMY_Q { >>> + QTAILQ_HEAD(DUMMY_Q_HEAD, DUMMY_Q_ENTRY) head; >>> +}; >>> + >>> +#define dummy_q ((DUMMY_Q *) 0) >>> +#define dummy_qe ((DUMMY_Q_ENTRY *) 0) >>> + >>> +/* >>> + * Offsets of layout of a tail queue head. >>> + */ >>> +#define QTAILQ_FIRST_OFFSET (offsetof(typeof(dummy_q->head), tqh_first)) >>> +#define QTAILQ_LAST_OFFSET (offsetof(typeof(dummy_q->head), tqh_last)) >>> +/* >>> + * Raw access of elements of a tail queue >>> + */ >>> +#define QTAILQ_RAW_FIRST(head) \ >>> + (*field_at_offset(head, QTAILQ_FIRST_OFFSET, void **)) >>> +#define QTAILQ_RAW_LAST(head) \ >>> + (*field_at_offset(head, QTAILQ_LAST_OFFSET, void ***)) >>> + >>> +/* >>> + * Offsets of layout of a tail queue element. >>> + */ >>> +#define QTAILQ_NEXT_OFFSET (offsetof(typeof(dummy_qe->next), tqe_next)) >>> +#define QTAILQ_PREV_OFFSET (offsetof(typeof(dummy_qe->next), tqe_prev)) >>> + >>> +/* >>> + * Raw access of elements of a tail entry >>> + */ >>> +#define QTAILQ_RAW_NEXT(elm, entry) \ >>> + (*field_at_offset(elm, entry + QTAILQ_NEXT_OFFSET, void **)) >>> +#define QTAILQ_RAW_PREV(elm, entry) \ >>> + (*field_at_offset(elm, entry + QTAILQ_PREV_OFFSET, void ***)) >> >> AFAIU QTAILQ_RAW_PREV and QTAILQ_PREV have completely different semantic. >> Don't think its quite likely anybody will need QTAILQ_RAW_PREV with the >> QTAILQ_PREV semantic (that is pointer to the previous element -- and not >> to the prev next), nevertheless I think picking a different name would >> be a good idea. How about using QTAILQ_RAW_TQE_NEXT and QTAILQ_RAW_TQE_PREV >> instead? >> > Good catch. Will change the names even though the comments were pretty > clear. >>> +/* >>> + * Tail queue tranversal using pointer arithmetic. >>> + */ >>> +#define QTAILQ_RAW_FOREACH(elm, head, entry) \ >>> + for ((elm) = QTAILQ_RAW_FIRST(head); \ >>> + (elm); \ >>> + (elm) = QTAILQ_RAW_NEXT(elm, entry)) >>> +/* >>> + * Tail queue insertion using pointer arithmetic. >>> + */ >>> +#define QTAILQ_RAW_INSERT_TAIL(head, elm, entry) do { \ >>> + QTAILQ_RAW_NEXT(elm, entry) = NULL; \ >>> + QTAILQ_RAW_PREV(elm, entry) = QTAILQ_RAW_LAST(head); \ >>> + *QTAILQ_RAW_LAST(head) = (elm); \ >>> + QTAILQ_RAW_LAST(head) = &QTAILQ_RAW_NEXT(elm, entry); \ >>> +} while (/*CONSTCOND*/0) >>> + >>> #endif /* QEMU_SYS_QUEUE_H */ >>> diff --git a/migration/trace-events b/migration/trace-events >>> index 94134f7..c46f9e9 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 7b4bd6e..2f9d4ba 100644 >>> --- a/migration/vmstate.c >>> +++ b/migration/vmstate.c >>> @@ -5,6 +5,7 @@ >>> #include "migration/vmstate.h" >>> #include "qemu/bitops.h" >>> #include "qemu/error-report.h" >>> +#include "qemu/queue.h" >>> #include "trace.h" >>> #include "migration/qjson.h" >>> >>> @@ -965,3 +966,71 @@ const VMStateInfo vmstate_info_bitmap = { >>> .get = get_bitmap, >>> .put = put_bitmap, >>> }; >>> + >>> +/* get for QTAILQ >>> + * meta data about the QTAILQ is encoded in a VMStateField structure >>> + */ >>> +static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size, >>> + VMStateField *field) >>> +{ >>> + int ret = 0; >>> + const VMStateDescription *vmsd = field->vmsd; >>> + /* size of a QTAILQ element */ >>> + size_t size = field->size; >>> + /* offset of the QTAILQ entry in a QTAILQ element */ >>> + size_t entry_offset = field->start; >>> + int version_id = field->version_id; >>> + void *elm; >>> + >>> + trace_get_qtailq(vmsd->name, version_id); >>> + if (version_id > vmsd->version_id) { >>> + error_report("%s %s", vmsd->name, "too new"); >>> + trace_get_qtailq_end(vmsd->name, "too new", -EINVAL); >>> + >>> + return -EINVAL; >>> + } >>> + if (version_id < vmsd->minimum_version_id) { >>> + error_report("%s %s", vmsd->name, "too old"); >>> + trace_get_qtailq_end(vmsd->name, "too old", -EINVAL); >>> + return -EINVAL; >>> + } >>> + >> >> (1) Why not do >> + QTAILQ_INIT((DUMMY_Q *)pv); >> > As discussed before, the qtailq on the target is assumed to be > initialized and may not be empty. That is the case at least in the DRC > stuff. Doing another init may cause issues and memory leak. > I just think it is more reasonable to assume here that the list is initialized that to assume that the list already contains element and it is supposed to be merged with the remote one. Simple example would be list of lists (without saying somebody is ever going to do that, but hash table with separate chaining could also be an example where lists can sit in a datastructure). I can't think of a scenario where this merging would be actually beneficial. Can you give me one (maybe from the code base)? I also think the risk is smaller because memory corruption is worse than memory leak, and you can prohibit non-empty target lists the same way you prohibited uninitialized ones. Bottom line the leak argument is weak in my eyes. Admittedly the uninitialized target should matter argument isn't strong either. I just wanted to point that taking the reinitialize option is very simple and IMHO slightly more relevant and conceptually much simpler. Halil > > Thanks, > Jianjun >> Halil >> >>> + 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_offset); >>> + } >>> + >>> + trace_get_qtailq_end(vmsd->name, "end", ret); >>> + return ret; >>> +} >>> + >>> +/* put for QTAILQ */ >>> +static int put_qtailq(QEMUFile *f, void *pv, size_t unused_size, >>> + VMStateField *field, QJSON *vmdesc) >>> +{ >>> + const VMStateDescription *vmsd = field->vmsd; >>> + /* offset of the QTAILQ entry in a QTAILQ element*/ >>> + size_t entry_offset = field->start; >>> + void *elm; >>> + >>> + trace_put_qtailq(vmsd->name, vmsd->version_id); >>> + >>> + QTAILQ_RAW_FOREACH(elm, pv, entry_offset) { >>> + qemu_put_byte(f, true); >>> + vmstate_save_state(f, vmsd, elm, vmdesc); >>> + } >>> + qemu_put_byte(f, false); >>> + >>> + trace_put_qtailq_end(vmsd->name, "end"); >>> + >>> + return 0; >>> +} >>> +const VMStateInfo vmstate_info_qtailq = { >>> + .name = "qtailq", >>> + .get = get_qtailq, >>> + .put = put_qtailq, >>> +}; >>> >>
On 11/10/2016 11:13 AM, Halil Pasic wrote: > > > On 11/10/2016 07:00 PM, Jianjun Duan wrote: >> >> >> On 11/10/2016 04:04 AM, Halil Pasic wrote: >>> >>> >>> On 11/08/2016 01:06 AM, Jianjun Duan wrote: >>>> Currently we cannot directly transfer a QTAILQ instance because of the >>>> limitation in the migration code. Here we introduce an approach to >>>> transfer such structures. We created VMStateInfo vmstate_info_qtailq >>>> for QTAILQ. Similar VMStateInfo can be created for other data structures >>>> such as list. >>>> >>>> When a QTAILQ is migrated from source to target, it is appended to the >>>> corresponding QTAILQ structure, which is assumed to have been properly >>>> initialized. >>>> >>>> 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 | 20 +++++++++++++ >>>> include/qemu/queue.h | 60 +++++++++++++++++++++++++++++++++++++++ >>>> migration/trace-events | 4 +++ >>>> migration/vmstate.c | 69 +++++++++++++++++++++++++++++++++++++++++++++ >>>> 4 files changed, 153 insertions(+) >>>> >>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h >>>> index eafc8f2..6289327 100644 >>>> --- a/include/migration/vmstate.h >>>> +++ b/include/migration/vmstate.h >>>> @@ -253,6 +253,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) >>>> @@ -664,6 +665,25 @@ extern const VMStateInfo vmstate_info_bitmap; >>>> .offset = offsetof(_state, _field), \ >>>> } >>>> >>>> +/* For QTAILQ that need customized handling. >>> >>> What do you mean by customized handling? How does non-customized >>> handling look like? >>> >> Customized handling means a user has to implement put/get to get the job >> done while normally one expects to just specify a VMSD to get the job done. > > I would drop the customized since. The user does not have t > o implement put/get since the (not user supplied) .info > that takes care of the stuff. Now assumed I'm a random guy > who wants his QTAILQ migrated, if I read 'For QTAILQ that > need customized handling' I ask myself does _my_ qtailq > need any customized handling. I have a pretty plain qtailq > so it should not need any customized handling. It may be > only my limited English skills though. I'm not anywhere near > of being a native speaker. > > IMHO if you want to indicate that stuff is custom because you > have both vmsd and info I would place a comment next to the > info. I think from language perspective that should be completely > OK assuming t least c99 -- or at least I hope so. The comment > above the macro has an apidoc feel to me so indicating it there > feels wrong. > I guess we mean the same thing. Since QTAILQ requires a get/put and cannot be migrated by just specifying a vmsd, I considered it "need customized handling". I will drop "customized". >>> I would also add something like works only for movable >>> elements/nodes to make it clear that scenarios like Juan has >>> pointed out previously are not currently supported. >>> >> Maybe you mean the backward compatibility regarding the bit stream? >> It is not supported. Will document it. > > I mean this: > https://lists.nongnu.org/archive/html/qemu-devel/2016-11/msg00314.html > > If you look at the code the VirtQueue instances sit in a array > and are organized into lists depending on to which vector do > these belong to (disclaimer: my understanding of the problem formulated > very sloppily). > > I missed the discussion on backward compatibility of the bitstream. > Could you give me a pointer? > It was touched in earlier discussions. http://lists.nongnu.org/archive/html/qemu-ppc/2016-10/msg00150.html >>>> + * Target QTAILQ needs be properly initialized. >>> >>> Could this restriction be avoided by doing (1)? >>> >>>> + * _type: type of QTAILQ element >>>> + * _next: name of QTAILQ entry field in QTAILQ element >>>> + * _vmsd: VMSD for QTAILQ element >>>> + * size: size of QTAILQ element >>>> + * start: offset of QTAILQ entry in QTAILQ element >>>> + */ >>>> +#define VMSTATE_QTAILQ_V(_field, _state, _version, _vmsd, _type, _next) \ >>>> +{ \ >>>> + .name = (stringify(_field)), \ >>>> + .version_id = (_version), \ >>>> + .vmsd = &(_vmsd), \ >>>> + .size = sizeof(_type), \ >>>> + .info = &vmstate_info_qtailq, \ >>>> + .offset = offsetof(_state, _field), \ >>>> + .start = offsetof(_type, _next), \ >>>> +} >>>> + >>>> /* _f : field name >>>> _f_n : num of elements field_name >>>> _n : num of elements >>>> diff --git a/include/qemu/queue.h b/include/qemu/queue.h >>>> index 342073f..75616e1 100644 >>>> --- a/include/qemu/queue.h >>>> +++ b/include/qemu/queue.h >>>> @@ -438,4 +438,64 @@ struct { \ >>>> #define QTAILQ_PREV(elm, headname, field) \ >>>> (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last)) >>>> >>>> +#define field_at_offset(base, offset, type) \ >>>> + ((type) (((char *) (base)) + (offset))) >>>> + >>>> +typedef struct DUMMY_Q_ENTRY DUMMY_Q_ENTRY; >>>> +typedef struct DUMMY_Q DUMMY_Q; >>>> + >>>> +struct DUMMY_Q_ENTRY { >>>> + QTAILQ_ENTRY(DUMMY_Q_ENTRY) next; >>>> +}; >>>> + >>>> +struct DUMMY_Q { >>>> + QTAILQ_HEAD(DUMMY_Q_HEAD, DUMMY_Q_ENTRY) head; >>>> +}; >>>> + >>>> +#define dummy_q ((DUMMY_Q *) 0) >>>> +#define dummy_qe ((DUMMY_Q_ENTRY *) 0) >>>> + >>>> +/* >>>> + * Offsets of layout of a tail queue head. >>>> + */ >>>> +#define QTAILQ_FIRST_OFFSET (offsetof(typeof(dummy_q->head), tqh_first)) >>>> +#define QTAILQ_LAST_OFFSET (offsetof(typeof(dummy_q->head), tqh_last)) >>>> +/* >>>> + * Raw access of elements of a tail queue >>>> + */ >>>> +#define QTAILQ_RAW_FIRST(head) \ >>>> + (*field_at_offset(head, QTAILQ_FIRST_OFFSET, void **)) >>>> +#define QTAILQ_RAW_LAST(head) \ >>>> + (*field_at_offset(head, QTAILQ_LAST_OFFSET, void ***)) >>>> + >>>> +/* >>>> + * Offsets of layout of a tail queue element. >>>> + */ >>>> +#define QTAILQ_NEXT_OFFSET (offsetof(typeof(dummy_qe->next), tqe_next)) >>>> +#define QTAILQ_PREV_OFFSET (offsetof(typeof(dummy_qe->next), tqe_prev)) >>>> + >>>> +/* >>>> + * Raw access of elements of a tail entry >>>> + */ >>>> +#define QTAILQ_RAW_NEXT(elm, entry) \ >>>> + (*field_at_offset(elm, entry + QTAILQ_NEXT_OFFSET, void **)) >>>> +#define QTAILQ_RAW_PREV(elm, entry) \ >>>> + (*field_at_offset(elm, entry + QTAILQ_PREV_OFFSET, void ***)) >>> >>> AFAIU QTAILQ_RAW_PREV and QTAILQ_PREV have completely different semantic. >>> Don't think its quite likely anybody will need QTAILQ_RAW_PREV with the >>> QTAILQ_PREV semantic (that is pointer to the previous element -- and not >>> to the prev next), nevertheless I think picking a different name would >>> be a good idea. How about using QTAILQ_RAW_TQE_NEXT and QTAILQ_RAW_TQE_PREV >>> instead? >>> >> Good catch. Will change the names even though the comments were pretty >> clear. >>>> +/* >>>> + * Tail queue tranversal using pointer arithmetic. >>>> + */ >>>> +#define QTAILQ_RAW_FOREACH(elm, head, entry) \ >>>> + for ((elm) = QTAILQ_RAW_FIRST(head); \ >>>> + (elm); \ >>>> + (elm) = QTAILQ_RAW_NEXT(elm, entry)) >>>> +/* >>>> + * Tail queue insertion using pointer arithmetic. >>>> + */ >>>> +#define QTAILQ_RAW_INSERT_TAIL(head, elm, entry) do { \ >>>> + QTAILQ_RAW_NEXT(elm, entry) = NULL; \ >>>> + QTAILQ_RAW_PREV(elm, entry) = QTAILQ_RAW_LAST(head); \ >>>> + *QTAILQ_RAW_LAST(head) = (elm); \ >>>> + QTAILQ_RAW_LAST(head) = &QTAILQ_RAW_NEXT(elm, entry); \ >>>> +} while (/*CONSTCOND*/0) >>>> + >>>> #endif /* QEMU_SYS_QUEUE_H */ >>>> diff --git a/migration/trace-events b/migration/trace-events >>>> index 94134f7..c46f9e9 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 7b4bd6e..2f9d4ba 100644 >>>> --- a/migration/vmstate.c >>>> +++ b/migration/vmstate.c >>>> @@ -5,6 +5,7 @@ >>>> #include "migration/vmstate.h" >>>> #include "qemu/bitops.h" >>>> #include "qemu/error-report.h" >>>> +#include "qemu/queue.h" >>>> #include "trace.h" >>>> #include "migration/qjson.h" >>>> >>>> @@ -965,3 +966,71 @@ const VMStateInfo vmstate_info_bitmap = { >>>> .get = get_bitmap, >>>> .put = put_bitmap, >>>> }; >>>> + >>>> +/* get for QTAILQ >>>> + * meta data about the QTAILQ is encoded in a VMStateField structure >>>> + */ >>>> +static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size, >>>> + VMStateField *field) >>>> +{ >>>> + int ret = 0; >>>> + const VMStateDescription *vmsd = field->vmsd; >>>> + /* size of a QTAILQ element */ >>>> + size_t size = field->size; >>>> + /* offset of the QTAILQ entry in a QTAILQ element */ >>>> + size_t entry_offset = field->start; >>>> + int version_id = field->version_id; >>>> + void *elm; >>>> + >>>> + trace_get_qtailq(vmsd->name, version_id); >>>> + if (version_id > vmsd->version_id) { >>>> + error_report("%s %s", vmsd->name, "too new"); >>>> + trace_get_qtailq_end(vmsd->name, "too new", -EINVAL); >>>> + >>>> + return -EINVAL; >>>> + } >>>> + if (version_id < vmsd->minimum_version_id) { >>>> + error_report("%s %s", vmsd->name, "too old"); >>>> + trace_get_qtailq_end(vmsd->name, "too old", -EINVAL); >>>> + return -EINVAL; >>>> + } >>>> + >>> >>> (1) Why not do >>> + QTAILQ_INIT((DUMMY_Q *)pv); >>> >> As discussed before, the qtailq on the target is assumed to be >> initialized and may not be empty. That is the case at least in the DRC >> stuff. Doing another init may cause issues and memory leak. >> > > I just think it is more reasonable to assume here that the list > is initialized that to assume that the list already contains element > and it is supposed to be merged with the remote one. > > Simple example would be list of lists (without saying somebody > is ever going to do that, but hash table with separate chaining > could also be an example where lists can sit in a datastructure). > > I can't think of a scenario where this merging would be actually > beneficial. Can you give me one (maybe from the code base)? > > I also think the risk is smaller because memory corruption > is worse than memory leak, and you can prohibit non-empty target > lists the same way you prohibited uninitialized ones. Bottom > line the leak argument is weak in my eyes. Admittedly > the uninitialized target should matter argument isn't > strong either. I just wanted to point that taking the > reinitialize option is very simple and IMHO slightly more > relevant and conceptually much simpler. > If we assume the QTAILQ is inited on target, then merging is just side effect. This has been discussed before: http://lists.nongnu.org/archive/html/qemu-ppc/2016-11/msg00025.html Thanks, Jianjun > Halil > >> >> Thanks, >> Jianjun >>> Halil >>> >>>> + 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_offset); >>>> + } >>>> + >>>> + trace_get_qtailq_end(vmsd->name, "end", ret); >>>> + return ret; >>>> +} >>>> + >>>> +/* put for QTAILQ */ >>>> +static int put_qtailq(QEMUFile *f, void *pv, size_t unused_size, >>>> + VMStateField *field, QJSON *vmdesc) >>>> +{ >>>> + const VMStateDescription *vmsd = field->vmsd; >>>> + /* offset of the QTAILQ entry in a QTAILQ element*/ >>>> + size_t entry_offset = field->start; >>>> + void *elm; >>>> + >>>> + trace_put_qtailq(vmsd->name, vmsd->version_id); >>>> + >>>> + QTAILQ_RAW_FOREACH(elm, pv, entry_offset) { >>>> + qemu_put_byte(f, true); >>>> + vmstate_save_state(f, vmsd, elm, vmdesc); >>>> + } >>>> + qemu_put_byte(f, false); >>>> + >>>> + trace_put_qtailq_end(vmsd->name, "end"); >>>> + >>>> + return 0; >>>> +} >>>> +const VMStateInfo vmstate_info_qtailq = { >>>> + .name = "qtailq", >>>> + .get = get_qtailq, >>>> + .put = put_qtailq, >>>> +}; >>>> >>> > >
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index eafc8f2..6289327 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -253,6 +253,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) @@ -664,6 +665,25 @@ extern const VMStateInfo vmstate_info_bitmap; .offset = offsetof(_state, _field), \ } +/* For QTAILQ that need customized handling. + * Target QTAILQ needs be properly initialized. + * _type: type of QTAILQ element + * _next: name of QTAILQ entry field in QTAILQ element + * _vmsd: VMSD for QTAILQ element + * size: size of QTAILQ element + * start: offset of QTAILQ entry in QTAILQ element + */ +#define VMSTATE_QTAILQ_V(_field, _state, _version, _vmsd, _type, _next) \ +{ \ + .name = (stringify(_field)), \ + .version_id = (_version), \ + .vmsd = &(_vmsd), \ + .size = sizeof(_type), \ + .info = &vmstate_info_qtailq, \ + .offset = offsetof(_state, _field), \ + .start = offsetof(_type, _next), \ +} + /* _f : field name _f_n : num of elements field_name _n : num of elements diff --git a/include/qemu/queue.h b/include/qemu/queue.h index 342073f..75616e1 100644 --- a/include/qemu/queue.h +++ b/include/qemu/queue.h @@ -438,4 +438,64 @@ struct { \ #define QTAILQ_PREV(elm, headname, field) \ (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last)) +#define field_at_offset(base, offset, type) \ + ((type) (((char *) (base)) + (offset))) + +typedef struct DUMMY_Q_ENTRY DUMMY_Q_ENTRY; +typedef struct DUMMY_Q DUMMY_Q; + +struct DUMMY_Q_ENTRY { + QTAILQ_ENTRY(DUMMY_Q_ENTRY) next; +}; + +struct DUMMY_Q { + QTAILQ_HEAD(DUMMY_Q_HEAD, DUMMY_Q_ENTRY) head; +}; + +#define dummy_q ((DUMMY_Q *) 0) +#define dummy_qe ((DUMMY_Q_ENTRY *) 0) + +/* + * Offsets of layout of a tail queue head. + */ +#define QTAILQ_FIRST_OFFSET (offsetof(typeof(dummy_q->head), tqh_first)) +#define QTAILQ_LAST_OFFSET (offsetof(typeof(dummy_q->head), tqh_last)) +/* + * Raw access of elements of a tail queue + */ +#define QTAILQ_RAW_FIRST(head) \ + (*field_at_offset(head, QTAILQ_FIRST_OFFSET, void **)) +#define QTAILQ_RAW_LAST(head) \ + (*field_at_offset(head, QTAILQ_LAST_OFFSET, void ***)) + +/* + * Offsets of layout of a tail queue element. + */ +#define QTAILQ_NEXT_OFFSET (offsetof(typeof(dummy_qe->next), tqe_next)) +#define QTAILQ_PREV_OFFSET (offsetof(typeof(dummy_qe->next), tqe_prev)) + +/* + * Raw access of elements of a tail entry + */ +#define QTAILQ_RAW_NEXT(elm, entry) \ + (*field_at_offset(elm, entry + QTAILQ_NEXT_OFFSET, void **)) +#define QTAILQ_RAW_PREV(elm, entry) \ + (*field_at_offset(elm, entry + QTAILQ_PREV_OFFSET, void ***)) +/* + * Tail queue tranversal using pointer arithmetic. + */ +#define QTAILQ_RAW_FOREACH(elm, head, entry) \ + for ((elm) = QTAILQ_RAW_FIRST(head); \ + (elm); \ + (elm) = QTAILQ_RAW_NEXT(elm, entry)) +/* + * Tail queue insertion using pointer arithmetic. + */ +#define QTAILQ_RAW_INSERT_TAIL(head, elm, entry) do { \ + QTAILQ_RAW_NEXT(elm, entry) = NULL; \ + QTAILQ_RAW_PREV(elm, entry) = QTAILQ_RAW_LAST(head); \ + *QTAILQ_RAW_LAST(head) = (elm); \ + QTAILQ_RAW_LAST(head) = &QTAILQ_RAW_NEXT(elm, entry); \ +} while (/*CONSTCOND*/0) + #endif /* QEMU_SYS_QUEUE_H */ diff --git a/migration/trace-events b/migration/trace-events index 94134f7..c46f9e9 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 7b4bd6e..2f9d4ba 100644 --- a/migration/vmstate.c +++ b/migration/vmstate.c @@ -5,6 +5,7 @@ #include "migration/vmstate.h" #include "qemu/bitops.h" #include "qemu/error-report.h" +#include "qemu/queue.h" #include "trace.h" #include "migration/qjson.h" @@ -965,3 +966,71 @@ const VMStateInfo vmstate_info_bitmap = { .get = get_bitmap, .put = put_bitmap, }; + +/* get for QTAILQ + * meta data about the QTAILQ is encoded in a VMStateField structure + */ +static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size, + VMStateField *field) +{ + int ret = 0; + const VMStateDescription *vmsd = field->vmsd; + /* size of a QTAILQ element */ + size_t size = field->size; + /* offset of the QTAILQ entry in a QTAILQ element */ + size_t entry_offset = field->start; + int version_id = field->version_id; + void *elm; + + trace_get_qtailq(vmsd->name, version_id); + if (version_id > vmsd->version_id) { + error_report("%s %s", vmsd->name, "too new"); + trace_get_qtailq_end(vmsd->name, "too new", -EINVAL); + + return -EINVAL; + } + if (version_id < vmsd->minimum_version_id) { + error_report("%s %s", vmsd->name, "too old"); + trace_get_qtailq_end(vmsd->name, "too old", -EINVAL); + return -EINVAL; + } + + while (qemu_get_byte(f)) { + elm = g_malloc(size); + ret = vmstate_load_state(f, vmsd, elm, version_id); + if (ret) { + return ret; + } + QTAILQ_RAW_INSERT_TAIL(pv, elm, entry_offset); + } + + trace_get_qtailq_end(vmsd->name, "end", ret); + return ret; +} + +/* put for QTAILQ */ +static int put_qtailq(QEMUFile *f, void *pv, size_t unused_size, + VMStateField *field, QJSON *vmdesc) +{ + const VMStateDescription *vmsd = field->vmsd; + /* offset of the QTAILQ entry in a QTAILQ element*/ + size_t entry_offset = field->start; + void *elm; + + trace_put_qtailq(vmsd->name, vmsd->version_id); + + QTAILQ_RAW_FOREACH(elm, pv, entry_offset) { + qemu_put_byte(f, true); + vmstate_save_state(f, vmsd, elm, vmdesc); + } + qemu_put_byte(f, false); + + trace_put_qtailq_end(vmsd->name, "end"); + + return 0; +} +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. We created VMStateInfo vmstate_info_qtailq for QTAILQ. Similar VMStateInfo can be created for other data structures such as list. When a QTAILQ is migrated from source to target, it is appended to the corresponding QTAILQ structure, which is assumed to have been properly initialized. 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 | 20 +++++++++++++ include/qemu/queue.h | 60 +++++++++++++++++++++++++++++++++++++++ migration/trace-events | 4 +++ migration/vmstate.c | 69 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 153 insertions(+)