| Message ID | 1464112509-21806-5-git-send-email-duanj@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Tue, May 24, 2016 at 10:55:07AM -0700, Jianjun Duan wrote: > A recursive structure has elements of the same type in itself. Currently > we cannot directly transfer a QTAILQ instance, or any recursive > structure such as lists in migration because of the limitation in the > migration code. Here we introduce a general approach to transfer such > structures. In our approach a recursive structure is tagged with VMS_CSTM. > We extend VMStateField with 3 fields: meta_data to store the meta data > about the recursive structure in question, extend_get to load the structure > from migration stream to memory, and extend_put to dump the structure into > the migration stream. This extension mirrors VMStateInfo. We then modify > vmstate_save_state and vmstate_load_state so that when VMS_CSTM is > encountered, extend_put and extend_get are called respectively with the > knowledge of the meta data. All the talk about recursive structures just obscures the issue. The point is you're building a way of migrating QTAILQs - just stick to that. > To make it work for QTAILQ in qemu/queue.h, we created the meta data > format, extend_get and extend_put for it. We will use this approach to > transfer pending_events and ccs_list in spapr state. > > We also create some macros in qemu/queue.h to get the layout information > about QTAILQ. This ensures that we do not depend on the implementation > details about QTAILQ in the migration code. I'm interested to see what Juan and Dave Gilbert think about this. > > Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com> > --- > include/migration/vmstate.h | 59 +++++++++++++++++++++++++++++++ > include/qemu/queue.h | 38 ++++++++++++++++++++ > migration/vmstate.c | 84 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 181 insertions(+) > > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h > index 1622638..bf57b25 100644 > --- a/include/migration/vmstate.h > +++ b/include/migration/vmstate.h > @@ -183,6 +183,8 @@ enum VMStateFlags { > * to determine the number of entries in the array. Only valid in > * combination with one of VMS_VARRAY*. */ > VMS_MULTIPLY_ELEMENTS = 0x4000, > + /* For fields which need customized handling, such as QTAILQ in queue.h*/ > + VMS_CSTM = 0x8000, > }; > > typedef struct { > @@ -198,6 +200,14 @@ typedef struct { > const VMStateDescription *vmsd; > int version_id; > bool (*field_exists)(void *opaque, int version_id); > + /* > + * Following 3 fields are for VMStateField which needs customized handling, > + * such as QTAILQ in qemu/queue.h, lists, and tree. > + */ > + const void *meta_data; > + int (*extend_get)(QEMUFile *f, const void *metadata, void *opaque); > + void (*extend_put)(QEMUFile *f, const void *metadata, void *opaque, > + QJSON *vmdesc); > } VMStateField; > > struct VMStateDescription { > @@ -654,6 +664,18 @@ extern const VMStateInfo vmstate_info_bitmap; > .offset = offsetof(_state, _field), \ > } > > +/* For fields that need customized handling, such as queue, list */ > +#define VMSTATE_CSTM_V(_field, _state, _version, _meta_data, _get, _put) \ > +{ \ > + .name = (stringify(_field)), \ > + .version_id = (_version), \ > + .flags = VMS_CSTM, \ > + .offset = offsetof(_state, _field), \ > + .meta_data = &(_meta_data), \ > + .extend_get = (_get), \ > + .extend_put = (_put), \ > +} > + > /* _f : field name > _f_n : num of elements field_name > _n : num of elements > @@ -970,4 +992,41 @@ int64_t self_announce_delay(int round) > > void dump_vmstate_json_to_file(FILE *out_fp); > > + > +/* Meta data for QTAILQ */ > +typedef struct QTAILQMetaData { > + /* the offset of tqh_first in QTAILQ_HEAD */ > + size_t first; > + /* the offset of tqh_last in QTAILQ_HEAD */ > + size_t last; > + /* the offset of tqe_next in QTAILQ_ENTRY */ > + size_t next; > + /* the offset of tqe_prev in QTAILQ_ENTRY */ > + size_t prev; > + /* the offset of QTAILQ_ENTRY in a QTAILQ element*/ > + size_t entry; > + /* size of a QTAILQ element */ > + size_t size; > + /* vmsd of a QTAILQ element */ > + const VMStateDescription *vmsd; > +} QTAILQMetaData; > + > +#define VMSTATE_QTAILQ_METADATA(_field, _state, _type, _next, _vmsd) { \ > + .first = QTAILQ_FIRST_OFFSET(typeof_field(_state, _field)), \ > + .last = QTAILQ_LAST_OFFSET(typeof_field(_state, _field)), \ > + .next = QTAILQ_NEXT_OFFSET(_type, _next), \ > + .prev = QTAILQ_PREV_OFFSET(_type, _next), \ > + .entry = offsetof(_type, _next), \ > + .size = sizeof(_type), \ > + .vmsd = &(_vmsd), \ > +} > + > +int vmstate_get_qtailq(QEMUFile *f, const void *metadata, void *opaque); > +void vmstate_put_qtailq(QEMUFile *f, const void *metadata, > + void *opaque, QJSON *vmdesc); > + > +/* VMStateField for QTAILQ field */ > +#define VMSTATE_QTAILQ_V(_field, _state, _version, _meta_data) \ > + VMSTATE_CSTM_V(_field, _state, _version, _meta_data, vmstate_get_qtailq, \ > + vmstate_put_qtailq) > #endif > diff --git a/include/qemu/queue.h b/include/qemu/queue.h > index f781aa2..46962d7 100644 > --- a/include/qemu/queue.h > +++ b/include/qemu/queue.h > @@ -437,3 +437,41 @@ struct { \ > (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last)) > > #endif /* !QEMU_SYS_QUEUE_H_ */ > + > +/* > + * Offsets of layout of a tail queue head. > + */ > +#define QTAILQ_FIRST_OFFSET(head_type) \ > + ((size_t) ((char *) &((head_type *)0)->tqh_first - (char *)0)) > + > +#define QTAILQ_LAST_OFFSET(head_type) \ > + ((size_t) ((char *) &((head_type *)0)->tqh_last - (char *)0)) > + > +/* > + * Offsets of layout of a tail queue element. > + */ > +#define QTAILQ_NEXT_OFFSET(ele_type, field) \ > + ((size_t) ((char *) &((ele_type *)0)->field.tqe_next - \ > + (char *) &((ele_type *)0)->field)) > + > +#define QTAILQ_PREV_OFFSET(ele_type, field) \ > + ((size_t) ((char *) &((ele_type *)0)->field.tqe_prev - \ > + (char *) &((ele_type *)0)->field)) > +/* > + * Tail queue tranversal using pointer arithmetic. > + */ > +#define QTAILQ_RAW_FOREACH(elm, head, entry, first, next) \ > + for ((elm) = *((void **) ((char *) (head) + (first))); \ > + (elm); \ > + (elm) = *((void **) ((char *) (elm) + (entry) + (next)))) > +/* > + * Tail queue insertion using pointer arithmetic. > + */ > +#define QTAILQ_RAW_INSERT_TAIL(head, elm, entry, first, last, next, prev) do { \ > + *((void **) ((char *) (elm) + (entry) + (next))) = NULL; \ > + *((void **) ((char *) (elm) + (entry) + (prev))) = \ > + *((void **) ((char *) (head) + (last))); \ > + **((void ***)((char *) (head) + (last))) = (elm); \ > + *((void **) ((char *) (head) + (last))) = \ > + (void *) ((char *) (elm) + (entry) + (next)); \ > +} while (/*CONSTCOND*/0) > diff --git a/migration/vmstate.c b/migration/vmstate.c > index bf3d5db..47cd052 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 "qjson.h" > > @@ -121,6 +122,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, > if (field->flags & VMS_STRUCT) { > ret = vmstate_load_state(f, field->vmsd, addr, > field->vmsd->version_id); > + } else if (field->flags & VMS_CSTM) { > + ret = field->extend_get(f, field->meta_data, addr); > } else { > ret = field->info->get(f, addr, size); > > @@ -193,6 +196,8 @@ static const char *vmfield_get_type_name(VMStateField *field) > > if (field->flags & VMS_STRUCT) { > type = "struct"; > + } else if (field->flags & VMS_CSTM) { > + type = "customized"; > } else if (field->info->name) { > type = field->info->name; > } > @@ -327,6 +332,8 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd, > } > if (field->flags & VMS_STRUCT) { > vmstate_save_state(f, field->vmsd, addr, vmdesc_loop); > + } else if (field->flags & VMS_CSTM) { > + field->extend_put(f, field->meta_data, addr, vmdesc_loop); > } else { > field->info->put(f, addr, size); > } > @@ -916,3 +923,80 @@ const VMStateInfo vmstate_info_bitmap = { > .get = get_bitmap, > .put = put_bitmap, > }; > + > +/* extend_get for QTAILQ */ > +int vmstate_get_qtailq(QEMUFile *f, const void *metadata, void *opaque) > +{ > + bool link; > + int ret = 0; > + size_t first, last, next, prev, entry, size; > + QTAILQMetaData *data = (QTAILQMetaData *)metadata; > + const VMStateDescription *vmsd = data->vmsd; > + int version_id = vmsd->version_id; > + void *elm; > + > + first = data->first; > + last = data->last; > + next = data->next; > + prev = data->prev; > + entry = data->entry; > + size = data->size; > + > + trace_vmstate_load_state(vmsd->name, version_id); > + if (version_id > vmsd->version_id) { > + trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL); > + return -EINVAL; > + } > + if (version_id < vmsd->minimum_version_id) { > + trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL); > + return -EINVAL; > + } > + > + while (true) { > + vmstate_info_bool.get(f, &link, sizeof(bool)); > + if (!link) { > + break; > + } > + > + elm = g_malloc(size); > + ret = vmstate_load_state(f, vmsd, elm, version_id); > + if (ret) { > + return ret; > + } > + QTAILQ_RAW_INSERT_TAIL(opaque, elm, entry, first, last, next, prev); > + } > + > + trace_vmstate_load_state_end(vmsd->name, "end", ret); > + return ret; > +} > + > +/* extend_get for QTAILQ */ > +void vmstate_put_qtailq(QEMUFile *f, const void *metadata, void *opaque, > + QJSON *vmdesc) > +{ > + bool link = true; > + size_t first, next, entry; > + QTAILQMetaData *data = (QTAILQMetaData *)metadata; > + const VMStateDescription *vmsd = data->vmsd; > + void *elm; > + > + first = data->first; > + next = data->next; > + entry = data->entry; > + > + if (vmdesc) { > + json_prop_str(vmdesc, "vmsd_name", vmsd->name); > + json_prop_int(vmdesc, "version", vmsd->version_id); > + json_start_array(vmdesc, "fields"); > + } > + > + QTAILQ_RAW_FOREACH(elm, opaque, entry, first, next) { > + vmstate_info_bool.put(f, &link, sizeof(bool)); > + vmstate_save_state(f, vmsd, elm, vmdesc); > + } > + link = false; > + vmstate_info_bool.put(f, &link, sizeof(bool)); > + if (vmdesc) { > + json_end_array(vmdesc); > + } > +}
On 24/05/2016 19:55, Jianjun Duan wrote: > +/* > + * Offsets of layout of a tail queue head. > + */ > +#define QTAILQ_FIRST_OFFSET(head_type) \ > + ((size_t) ((char *) &((head_type *)0)->tqh_first - (char *)0)) > + > +#define QTAILQ_LAST_OFFSET(head_type) \ > + ((size_t) ((char *) &((head_type *)0)->tqh_last - (char *)0)) > + > +/* > + * Offsets of layout of a tail queue element. > + */ > +#define QTAILQ_NEXT_OFFSET(ele_type, field) \ > + ((size_t) ((char *) &((ele_type *)0)->field.tqe_next - \ > + (char *) &((ele_type *)0)->field)) > + > +#define QTAILQ_PREV_OFFSET(ele_type, field) \ > + ((size_t) ((char *) &((ele_type *)0)->field.tqe_prev - \ > + (char *) &((ele_type *)0)->field)) Please use offsetof. > + /* > + * Following 3 fields are for VMStateField which needs customized handling, > + * such as QTAILQ in qemu/queue.h, lists, and tree. > + */ > + const void *meta_data; > + int (*extend_get)(QEMUFile *f, const void *metadata, void *opaque); > + void (*extend_put)(QEMUFile *f, const void *metadata, void *opaque, > + QJSON *vmdesc); > } VMStateField; Do not add these two function pointers to VMStateField, instead add QJSON* and VMStateField* arguments as needed to VMStateInfo's get and put. > +#define VMSTATE_QTAILQ_METADATA(_field, _state, _type, _next, _vmsd) { \ > + .first = QTAILQ_FIRST_OFFSET(typeof_field(_state, _field)), \ > + .last = QTAILQ_LAST_OFFSET(typeof_field(_state, _field)), \ > + .next = QTAILQ_NEXT_OFFSET(_type, _next), \ > + .prev = QTAILQ_PREV_OFFSET(_type, _next), \ > + .entry = offsetof(_type, _next), \ > + .size = sizeof(_type), \ > + .vmsd = &(_vmsd), \ > +} .last and .prev are unnecessary, since they come just after .first and .next (and their use is hidden behind QTAILQ_RAW_*). .first and .next can be placed in .offset and .num_offset respectively. So you don't really need an extra metadata struct. If you prefer you could have something like union { size_t num_offset; /* VMS_VARRAY */ size_t size_offset; /* VMS_VBUFFER */ size_t next_offset; /* VMS_TAILQ */ } offsets; Thanks, Paolo
On 05/25/2016 01:14 AM, Paolo Bonzini wrote: > > > On 24/05/2016 19:55, Jianjun Duan wrote: >> +/* >> + * Offsets of layout of a tail queue head. >> + */ >> +#define QTAILQ_FIRST_OFFSET(head_type) \ >> + ((size_t) ((char *) &((head_type *)0)->tqh_first - (char *)0)) >> + >> +#define QTAILQ_LAST_OFFSET(head_type) \ >> + ((size_t) ((char *) &((head_type *)0)->tqh_last - (char *)0)) >> + >> +/* >> + * Offsets of layout of a tail queue element. >> + */ >> +#define QTAILQ_NEXT_OFFSET(ele_type, field) \ >> + ((size_t) ((char *) &((ele_type *)0)->field.tqe_next - \ >> + (char *) &((ele_type *)0)->field)) >> + >> +#define QTAILQ_PREV_OFFSET(ele_type, field) \ >> + ((size_t) ((char *) &((ele_type *)0)->field.tqe_prev - \ >> + (char *) &((ele_type *)0)->field)) > > Please use offsetof. OK. > >> + /* >> + * Following 3 fields are for VMStateField which needs customized handling, >> + * such as QTAILQ in qemu/queue.h, lists, and tree. >> + */ >> + const void *meta_data; >> + int (*extend_get)(QEMUFile *f, const void *metadata, void *opaque); >> + void (*extend_put)(QEMUFile *f, const void *metadata, void *opaque, >> + QJSON *vmdesc); >> } VMStateField; > > Do not add these two function pointers to VMStateField, instead add > QJSON* and VMStateField* arguments as needed to VMStateInfo's get and put. > That is definitely the ideal way. However, VMStateInfo's get/put are already used extensively. If I change them, I need change all the calling sites of them. Not very sure about whether it will be welcomed. >> +#define VMSTATE_QTAILQ_METADATA(_field, _state, _type, _next, _vmsd) { \ >> + .first = QTAILQ_FIRST_OFFSET(typeof_field(_state, _field)), \ >> + .last = QTAILQ_LAST_OFFSET(typeof_field(_state, _field)), \ >> + .next = QTAILQ_NEXT_OFFSET(_type, _next), \ >> + .prev = QTAILQ_PREV_OFFSET(_type, _next), \ >> + .entry = offsetof(_type, _next), \ >> + .size = sizeof(_type), \ >> + .vmsd = &(_vmsd), \ >> +} > > .last and .prev are unnecessary, since they come just after .first and > .next (and their use is hidden behind QTAILQ_RAW_*). .first and .next > can be placed in .offset and .num_offset respectively. So you don't > really need an extra metadata struct. > > If you prefer you could have something like > > union { > size_t num_offset; /* VMS_VARRAY */ > size_t size_offset; /* VMS_VBUFFER */ > size_t next_offset; /* VMS_TAILQ */ > } offsets; > Actually I explored the approach in which I use a VMSD to encode all the information. But a VMSD describes actual memory layout. Interpreting it another way could be confusing. One of the assumption about QTAILQ is that we can only use the interfaces defined in queue.h to access it. I intend not to depend on its actually layout. From this perspective, these 6 parameters are needed. > Thanks, > > Paolo > Thanks, Jianjun
> >> + /* > >> + * Following 3 fields are for VMStateField which needs customized > >> handling, > >> + * such as QTAILQ in qemu/queue.h, lists, and tree. > >> + */ > >> + const void *meta_data; > >> + int (*extend_get)(QEMUFile *f, const void *metadata, void *opaque); > >> + void (*extend_put)(QEMUFile *f, const void *metadata, void *opaque, > >> + QJSON *vmdesc); > >> } VMStateField; > > > > Do not add these two function pointers to VMStateField, instead add > > QJSON* and VMStateField* arguments as needed to VMStateInfo's get and put. > > That is definitely the ideal way. However, VMStateInfo's get/put are > already used extensively. If I change them, I need change all the > calling sites of them. Not very sure about whether it will be welcomed. Sure, don't be worried. :) True, there are quite a few VMStateInfos (about 35) but the changes are simple. > >> +#define VMSTATE_QTAILQ_METADATA(_field, _state, _type, _next, _vmsd) { \ > >> + .first = QTAILQ_FIRST_OFFSET(typeof_field(_state, _field)), \ > >> + .last = QTAILQ_LAST_OFFSET(typeof_field(_state, _field)), \ > >> + .next = QTAILQ_NEXT_OFFSET(_type, _next), \ > >> + .prev = QTAILQ_PREV_OFFSET(_type, _next), \ > >> + .entry = offsetof(_type, _next), \ > >> + .size = sizeof(_type), \ > >> + .vmsd = &(_vmsd), \ > >> +} > > > > .last and .prev are unnecessary, since they come just after .first and > > .next (and their use is hidden behind QTAILQ_RAW_*). .first and .next ^^^^^^^^^^^^^^^^ Actually, .first and .next are always 0. I misread your changes to qemu/queue.h. See below. > > can be placed in .offset and .num_offset respectively. So you don't > > really need an extra metadata struct. > > > > If you prefer you could have something like > > > > union { > > size_t num_offset; /* VMS_VARRAY */ > > size_t size_offset; /* VMS_VBUFFER */ > > size_t next_offset; /* VMS_TAILQ */ > > } offsets; > > Actually I explored the approach in which I use a VMSD to encode all the > information. But a VMSD describes actual memory layout. Interpreting it > another way could be confusing. > > One of the assumption about QTAILQ is that we can only use the > interfaces defined in queue.h to access it. I intend not to depend on > its actually layout. From this perspective, these 6 parameters are > needed. You are already adding QTAILQ_RAW_*, aren't you? Those interfaces would need to know about the layout, and you are passing redundant information: - .next_offset should always be 0 - .prev_offset should always be sizeof(void *) - .first_offset should always be 0 - .last_offset should always be sizeof(void *) so you only need head and entry, which you can store in .offset and .num_offset. The .vmsd field you have in ->metadata can be stored in VMStateField's .vmsd, and likewise for .size (which can be stored in VMStateField's .vmsd). Thanks, Paolo
I will try to explain my design rationale in details here. 1 QTAILQ should only be accessed using the interfaces defined in queue.h. Its structs should not be directly used. So I created interfaces in queue.h to query about its layout. If the implementation is changed, these interfaces should be changed accordingly. Code using these interfaces should not break. 2 Based on point 1, vmstate_load_state/vmstate_put_state in vmstate.c doesn't exactly know the structs of QTAILAQ head and entry. So pointer arithmetic is needed to put/get a QTAILQ. To do it, we need those 6 parameters to be passed in. So it is not redundant if we only want to only use the interfaces. 3 At this moment, vmstate_load_state/vmstate_put_state couldn't handle a queue, or a list, or another recursive structure. To make it extensible, I think a metadata is needed. The idea is for any structure which needs special handling, customized metadata/put/get should provide enough flexibility to hack around. There are two issues I tried to address. One is the recursive queue, another is working around of hiding of the QTAILQ implementation details. It seems we need to agree on how the latter should be addressed. I will give it a try to fix those 35 calling sites of VMStateInfo. Thanks, Jianjun On 05/25/2016 10:51 AM, Paolo Bonzini wrote: >>>> + /* >>>> + * Following 3 fields are for VMStateField which needs customized >>>> handling, >>>> + * such as QTAILQ in qemu/queue.h, lists, and tree. >>>> + */ >>>> + const void *meta_data; >>>> + int (*extend_get)(QEMUFile *f, const void *metadata, void *opaque); >>>> + void (*extend_put)(QEMUFile *f, const void *metadata, void *opaque, >>>> + QJSON *vmdesc); >>>> } VMStateField; >>> >>> Do not add these two function pointers to VMStateField, instead add >>> QJSON* and VMStateField* arguments as needed to VMStateInfo's get and put. >> >> That is definitely the ideal way. However, VMStateInfo's get/put are >> already used extensively. If I change them, I need change all the >> calling sites of them. Not very sure about whether it will be welcomed. > > Sure, don't be worried. :) True, there are quite a few VMStateInfos (about > 35) but the changes are simple. > >>>> +#define VMSTATE_QTAILQ_METADATA(_field, _state, _type, _next, _vmsd) { \ >>>> + .first = QTAILQ_FIRST_OFFSET(typeof_field(_state, _field)), \ >>>> + .last = QTAILQ_LAST_OFFSET(typeof_field(_state, _field)), \ >>>> + .next = QTAILQ_NEXT_OFFSET(_type, _next), \ >>>> + .prev = QTAILQ_PREV_OFFSET(_type, _next), \ >>>> + .entry = offsetof(_type, _next), \ >>>> + .size = sizeof(_type), \ >>>> + .vmsd = &(_vmsd), \ >>>> +} >>> >>> .last and .prev are unnecessary, since they come just after .first and >>> .next (and their use is hidden behind QTAILQ_RAW_*). .first and .next > ^^^^^^^^^^^^^^^^ > > Actually, .first and .next are always 0. I misread your changes to qemu/queue.h. > See below. > >>> can be placed in .offset and .num_offset respectively. So you don't >>> really need an extra metadata struct. >>> >>> If you prefer you could have something like >>> >>> union { >>> size_t num_offset; /* VMS_VARRAY */ >>> size_t size_offset; /* VMS_VBUFFER */ >>> size_t next_offset; /* VMS_TAILQ */ >>> } offsets; >> >> Actually I explored the approach in which I use a VMSD to encode all the >> information. But a VMSD describes actual memory layout. Interpreting it >> another way could be confusing. >> >> One of the assumption about QTAILQ is that we can only use the >> interfaces defined in queue.h to access it. I intend not to depend on >> its actually layout. From this perspective, these 6 parameters are >> needed. > > You are already adding QTAILQ_RAW_*, aren't you? Those interfaces > would need to know about the layout, and you are passing redundant > information: > > - .next_offset should always be 0 > - .prev_offset should always be sizeof(void *) > - .first_offset should always be 0 > - .last_offset should always be sizeof(void *) > > so you only need head and entry, which you can store in .offset and > .num_offset. The .vmsd field you have in ->metadata can be stored > in VMStateField's .vmsd, and likewise for .size (which can be stored > in VMStateField's .vmsd). > > Thanks, > > Paolo >
> 1 QTAILQ should only be accessed using the interfaces defined in > queue.h. Its structs should not be directly used. So I created > interfaces in queue.h to query about its layout. If the implementation > is changed, these interfaces should be changed accordingly. Code using > these interfaces should not break. You don't need to query the layout, as long as the knowledge remains hidden in QTAILQ_RAW_* macros. And because QTAILQ_*_OFFSET returns constant values, you can just put the knowledge of the offsets directly in QTAILQ_RAW_FOREACH and QTAILQ_RAW_INSERT_TAIL. > 2 Based on point 1, vmstate_load_state/vmstate_put_state in vmstate.c > doesn't exactly know the structs of QTAILAQ head and entry. So pointer > arithmetic is needed to put/get a QTAILQ. To do it, we need those 6 > parameters to be passed in. So it is not redundant if we only want to > only use the interfaces. No, you only need two. The other four are internal to qemu/queue.h. Just like QTAILQ users do not know about tqh_* and tqe_*, they need not know about their offsets, only the fields that contain them. > 3 At this moment, vmstate_load_state/vmstate_put_state couldn't handle a > queue, or a list, or another recursive structure. To make it > extensible, I think a metadata is needed. The idea is for any > structure which needs special handling, customized metadata/put/get > should provide enough flexibility to hack around. I think your solution is a bit overengineered. If the metadata can fit in the VMStateField, you can use VMStateField. Thanks, Paolo
On 05/25/2016 12:22 PM, Paolo Bonzini wrote: >> 1 QTAILQ should only be accessed using the interfaces defined in >> queue.h. Its structs should not be directly used. So I created >> interfaces in queue.h to query about its layout. If the implementation >> is changed, these interfaces should be changed accordingly. Code using >> these interfaces should not break. > > You don't need to query the layout, as long as the knowledge > remains hidden in QTAILQ_RAW_* macros. And because QTAILQ_*_OFFSET > returns constant values, you can just put the knowledge of the offsets > directly in QTAILQ_RAW_FOREACH and QTAILQ_RAW_INSERT_TAIL. > >> 2 Based on point 1, vmstate_load_state/vmstate_put_state in vmstate.c >> doesn't exactly know the structs of QTAILAQ head and entry. So pointer >> arithmetic is needed to put/get a QTAILQ. To do it, we need those 6 >> parameters to be passed in. So it is not redundant if we only want to >> only use the interfaces. > > No, you only need two. The other four are internal to qemu/queue.h. > Just like QTAILQ users do not know about tqh_* and tqe_*, they need not > know about their offsets, only the fields that contain them. > In vmstate_load/put_state usually we use a VMSD to describe the type for dump/load purpose. The metadata for the QTAILQ is for the same purpose. Of course we can encode the information in a VMSD or VMStateField if we don't have to change much. The user may only care the position of head and entry. But to implement QTAILQ_RAW_***, we need more offset information than that. If we don't query the offsets using something like offset() and store it in a metadata, we have to make the assumption that all the pointer types have the same size. Since in vmstate_load/put_state we don't have any type information about the QTAILQ instance, we cannot use offset() in QTAILQ_RAW_*** macros. May have to stick the constants there for first/last/next/prev in QTAILQ_RAW_***. Not sure if it will work for all arches. >> 3 At this moment, vmstate_load_state/vmstate_put_state couldn't handle a >> queue, or a list, or another recursive structure. To make it >> extensible, I think a metadata is needed. The idea is for any >> structure which needs special handling, customized metadata/put/get >> should provide enough flexibility to hack around. > > I think your solution is a bit overengineered. If the metadata can > fit in the VMStateField, you can use VMStateField. > > Thanks, > > Paolo > Thanks, Jianjun
On 25/05/2016 22:17, Jianjun Duan wrote: > > > On 05/25/2016 12:22 PM, Paolo Bonzini wrote: >>> 1 QTAILQ should only be accessed using the interfaces defined in >>> queue.h. Its structs should not be directly used. So I created >>> interfaces in queue.h to query about its layout. If the implementation >>> is changed, these interfaces should be changed accordingly. Code using >>> these interfaces should not break. >> >> You don't need to query the layout, as long as the knowledge >> remains hidden in QTAILQ_RAW_* macros. And because QTAILQ_*_OFFSET >> returns constant values, you can just put the knowledge of the offsets >> directly in QTAILQ_RAW_FOREACH and QTAILQ_RAW_INSERT_TAIL. >> >>> 2 Based on point 1, vmstate_load_state/vmstate_put_state in vmstate.c >>> doesn't exactly know the structs of QTAILAQ head and entry. So pointer >>> arithmetic is needed to put/get a QTAILQ. To do it, we need those 6 >>> parameters to be passed in. So it is not redundant if we only want to >>> only use the interfaces. >> >> No, you only need two. The other four are internal to qemu/queue.h. >> Just like QTAILQ users do not know about tqh_* and tqe_*, they need not >> know about their offsets, only the fields that contain them. > > In vmstate_load/put_state usually we use a VMSD to describe the type for > dump/load purpose. The metadata for the QTAILQ is for the same purpose. > Of course we can encode the information in a VMSD or VMStateField if > we don't have to change much. A VMSD describes the "sub-structure" of the field. Here, the substructure of a QTAILQ is the structure of each entry. > The user may only care the position of head and entry. But to > implement QTAILQ_RAW_***, we need more offset information than that. > If we don't query the offsets using something like offset() and store > it in a metadata, we have to make the assumption that all the pointer > types have the same size. We make this assumption elsewhere anyway. _You_ are making it by doing loads and stores through void** in QTAILQ_RAW_*. Thanks, Paolo
On 05/26/2016 12:11 AM, Paolo Bonzini wrote: > > > On 25/05/2016 22:17, Jianjun Duan wrote: >> >> >> On 05/25/2016 12:22 PM, Paolo Bonzini wrote: >>>> 1 QTAILQ should only be accessed using the interfaces defined in >>>> queue.h. Its structs should not be directly used. So I created >>>> interfaces in queue.h to query about its layout. If the implementation >>>> is changed, these interfaces should be changed accordingly. Code using >>>> these interfaces should not break. >>> >>> You don't need to query the layout, as long as the knowledge >>> remains hidden in QTAILQ_RAW_* macros. And because QTAILQ_*_OFFSET >>> returns constant values, you can just put the knowledge of the offsets >>> directly in QTAILQ_RAW_FOREACH and QTAILQ_RAW_INSERT_TAIL. >>> >>>> 2 Based on point 1, vmstate_load_state/vmstate_put_state in vmstate.c >>>> doesn't exactly know the structs of QTAILAQ head and entry. So pointer >>>> arithmetic is needed to put/get a QTAILQ. To do it, we need those 6 >>>> parameters to be passed in. So it is not redundant if we only want to >>>> only use the interfaces. >>> >>> No, you only need two. The other four are internal to qemu/queue.h. >>> Just like QTAILQ users do not know about tqh_* and tqe_*, they need not >>> know about their offsets, only the fields that contain them. >> >> In vmstate_load/put_state usually we use a VMSD to describe the type for >> dump/load purpose. The metadata for the QTAILQ is for the same purpose. >> Of course we can encode the information in a VMSD or VMStateField if >> we don't have to change much. > > A VMSD describes the "sub-structure" of the field. Here, the > substructure of a QTAILQ is the structure of each entry. > >> The user may only care the position of head and entry. But to >> implement QTAILQ_RAW_***, we need more offset information than that. >> If we don't query the offsets using something like offset() and store >> it in a metadata, we have to make the assumption that all the pointer >> types have the same size. > > We make this assumption elsewhere anyway. _You_ are making it by doing > loads and stores through void** in QTAILQ_RAW_*. I thought every data pointer can be converted to and from void type of pointer. Anyway we can make that assumption here. How about alignment? Is it OK to assume pointers are always aligned at 4byte for 32 bit and 8byte for 64 bit? If yes then we can just add the size of a void pointer to first to get last in QTAILQ head. We will get something like: first=0 last = sizeof(void *) next=0 prev=sizeof(void *) Thanks, Jianjun > > Thanks, > > Paolo >
On 26/05/2016 18:43, Jianjun Duan wrote: >>> The user may only care the position of head and entry. But to >>> implement QTAILQ_RAW_***, we need more offset information than that. >>> If we don't query the offsets using something like offset() and store >>> it in a metadata, we have to make the assumption that all the pointer >>> types have the same size. >> >> We make this assumption elsewhere anyway. _You_ are making it by doing >> loads and stores through void** in QTAILQ_RAW_*. > > I thought every data pointer can be converted to and from void type of > pointer. Anyway we can make that assumption here. Yes, you can do that. > How about alignment? Is it OK to assume pointers are always aligned at > 4byte for 32 bit and 8byte for 64 bit? If yes then we can just add the > size of a void pointer to first to get last in QTAILQ head. We will get > something like: > first=0 > last = sizeof(void *) > next=0 > prev=sizeof(void *) Yes, that's my point. These four are constants. Thanks, Paolo
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index 1622638..bf57b25 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -183,6 +183,8 @@ enum VMStateFlags { * to determine the number of entries in the array. Only valid in * combination with one of VMS_VARRAY*. */ VMS_MULTIPLY_ELEMENTS = 0x4000, + /* For fields which need customized handling, such as QTAILQ in queue.h*/ + VMS_CSTM = 0x8000, }; typedef struct { @@ -198,6 +200,14 @@ typedef struct { const VMStateDescription *vmsd; int version_id; bool (*field_exists)(void *opaque, int version_id); + /* + * Following 3 fields are for VMStateField which needs customized handling, + * such as QTAILQ in qemu/queue.h, lists, and tree. + */ + const void *meta_data; + int (*extend_get)(QEMUFile *f, const void *metadata, void *opaque); + void (*extend_put)(QEMUFile *f, const void *metadata, void *opaque, + QJSON *vmdesc); } VMStateField; struct VMStateDescription { @@ -654,6 +664,18 @@ extern const VMStateInfo vmstate_info_bitmap; .offset = offsetof(_state, _field), \ } +/* For fields that need customized handling, such as queue, list */ +#define VMSTATE_CSTM_V(_field, _state, _version, _meta_data, _get, _put) \ +{ \ + .name = (stringify(_field)), \ + .version_id = (_version), \ + .flags = VMS_CSTM, \ + .offset = offsetof(_state, _field), \ + .meta_data = &(_meta_data), \ + .extend_get = (_get), \ + .extend_put = (_put), \ +} + /* _f : field name _f_n : num of elements field_name _n : num of elements @@ -970,4 +992,41 @@ int64_t self_announce_delay(int round) void dump_vmstate_json_to_file(FILE *out_fp); + +/* Meta data for QTAILQ */ +typedef struct QTAILQMetaData { + /* the offset of tqh_first in QTAILQ_HEAD */ + size_t first; + /* the offset of tqh_last in QTAILQ_HEAD */ + size_t last; + /* the offset of tqe_next in QTAILQ_ENTRY */ + size_t next; + /* the offset of tqe_prev in QTAILQ_ENTRY */ + size_t prev; + /* the offset of QTAILQ_ENTRY in a QTAILQ element*/ + size_t entry; + /* size of a QTAILQ element */ + size_t size; + /* vmsd of a QTAILQ element */ + const VMStateDescription *vmsd; +} QTAILQMetaData; + +#define VMSTATE_QTAILQ_METADATA(_field, _state, _type, _next, _vmsd) { \ + .first = QTAILQ_FIRST_OFFSET(typeof_field(_state, _field)), \ + .last = QTAILQ_LAST_OFFSET(typeof_field(_state, _field)), \ + .next = QTAILQ_NEXT_OFFSET(_type, _next), \ + .prev = QTAILQ_PREV_OFFSET(_type, _next), \ + .entry = offsetof(_type, _next), \ + .size = sizeof(_type), \ + .vmsd = &(_vmsd), \ +} + +int vmstate_get_qtailq(QEMUFile *f, const void *metadata, void *opaque); +void vmstate_put_qtailq(QEMUFile *f, const void *metadata, + void *opaque, QJSON *vmdesc); + +/* VMStateField for QTAILQ field */ +#define VMSTATE_QTAILQ_V(_field, _state, _version, _meta_data) \ + VMSTATE_CSTM_V(_field, _state, _version, _meta_data, vmstate_get_qtailq, \ + vmstate_put_qtailq) #endif diff --git a/include/qemu/queue.h b/include/qemu/queue.h index f781aa2..46962d7 100644 --- a/include/qemu/queue.h +++ b/include/qemu/queue.h @@ -437,3 +437,41 @@ struct { \ (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last)) #endif /* !QEMU_SYS_QUEUE_H_ */ + +/* + * Offsets of layout of a tail queue head. + */ +#define QTAILQ_FIRST_OFFSET(head_type) \ + ((size_t) ((char *) &((head_type *)0)->tqh_first - (char *)0)) + +#define QTAILQ_LAST_OFFSET(head_type) \ + ((size_t) ((char *) &((head_type *)0)->tqh_last - (char *)0)) + +/* + * Offsets of layout of a tail queue element. + */ +#define QTAILQ_NEXT_OFFSET(ele_type, field) \ + ((size_t) ((char *) &((ele_type *)0)->field.tqe_next - \ + (char *) &((ele_type *)0)->field)) + +#define QTAILQ_PREV_OFFSET(ele_type, field) \ + ((size_t) ((char *) &((ele_type *)0)->field.tqe_prev - \ + (char *) &((ele_type *)0)->field)) +/* + * Tail queue tranversal using pointer arithmetic. + */ +#define QTAILQ_RAW_FOREACH(elm, head, entry, first, next) \ + for ((elm) = *((void **) ((char *) (head) + (first))); \ + (elm); \ + (elm) = *((void **) ((char *) (elm) + (entry) + (next)))) +/* + * Tail queue insertion using pointer arithmetic. + */ +#define QTAILQ_RAW_INSERT_TAIL(head, elm, entry, first, last, next, prev) do { \ + *((void **) ((char *) (elm) + (entry) + (next))) = NULL; \ + *((void **) ((char *) (elm) + (entry) + (prev))) = \ + *((void **) ((char *) (head) + (last))); \ + **((void ***)((char *) (head) + (last))) = (elm); \ + *((void **) ((char *) (head) + (last))) = \ + (void *) ((char *) (elm) + (entry) + (next)); \ +} while (/*CONSTCOND*/0) diff --git a/migration/vmstate.c b/migration/vmstate.c index bf3d5db..47cd052 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 "qjson.h" @@ -121,6 +122,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, if (field->flags & VMS_STRUCT) { ret = vmstate_load_state(f, field->vmsd, addr, field->vmsd->version_id); + } else if (field->flags & VMS_CSTM) { + ret = field->extend_get(f, field->meta_data, addr); } else { ret = field->info->get(f, addr, size); @@ -193,6 +196,8 @@ static const char *vmfield_get_type_name(VMStateField *field) if (field->flags & VMS_STRUCT) { type = "struct"; + } else if (field->flags & VMS_CSTM) { + type = "customized"; } else if (field->info->name) { type = field->info->name; } @@ -327,6 +332,8 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd, } if (field->flags & VMS_STRUCT) { vmstate_save_state(f, field->vmsd, addr, vmdesc_loop); + } else if (field->flags & VMS_CSTM) { + field->extend_put(f, field->meta_data, addr, vmdesc_loop); } else { field->info->put(f, addr, size); } @@ -916,3 +923,80 @@ const VMStateInfo vmstate_info_bitmap = { .get = get_bitmap, .put = put_bitmap, }; + +/* extend_get for QTAILQ */ +int vmstate_get_qtailq(QEMUFile *f, const void *metadata, void *opaque) +{ + bool link; + int ret = 0; + size_t first, last, next, prev, entry, size; + QTAILQMetaData *data = (QTAILQMetaData *)metadata; + const VMStateDescription *vmsd = data->vmsd; + int version_id = vmsd->version_id; + void *elm; + + first = data->first; + last = data->last; + next = data->next; + prev = data->prev; + entry = data->entry; + size = data->size; + + trace_vmstate_load_state(vmsd->name, version_id); + if (version_id > vmsd->version_id) { + trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL); + return -EINVAL; + } + if (version_id < vmsd->minimum_version_id) { + trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL); + return -EINVAL; + } + + while (true) { + vmstate_info_bool.get(f, &link, sizeof(bool)); + if (!link) { + break; + } + + elm = g_malloc(size); + ret = vmstate_load_state(f, vmsd, elm, version_id); + if (ret) { + return ret; + } + QTAILQ_RAW_INSERT_TAIL(opaque, elm, entry, first, last, next, prev); + } + + trace_vmstate_load_state_end(vmsd->name, "end", ret); + return ret; +} + +/* extend_get for QTAILQ */ +void vmstate_put_qtailq(QEMUFile *f, const void *metadata, void *opaque, + QJSON *vmdesc) +{ + bool link = true; + size_t first, next, entry; + QTAILQMetaData *data = (QTAILQMetaData *)metadata; + const VMStateDescription *vmsd = data->vmsd; + void *elm; + + first = data->first; + next = data->next; + entry = data->entry; + + if (vmdesc) { + json_prop_str(vmdesc, "vmsd_name", vmsd->name); + json_prop_int(vmdesc, "version", vmsd->version_id); + json_start_array(vmdesc, "fields"); + } + + QTAILQ_RAW_FOREACH(elm, opaque, entry, first, next) { + vmstate_info_bool.put(f, &link, sizeof(bool)); + vmstate_save_state(f, vmsd, elm, vmdesc); + } + link = false; + vmstate_info_bool.put(f, &link, sizeof(bool)); + if (vmdesc) { + json_end_array(vmdesc); + } +}
A recursive structure has elements of the same type in itself. Currently we cannot directly transfer a QTAILQ instance, or any recursive structure such as lists in migration because of the limitation in the migration code. Here we introduce a general approach to transfer such structures. In our approach a recursive structure is tagged with VMS_CSTM. We extend VMStateField with 3 fields: meta_data to store the meta data about the recursive structure in question, extend_get to load the structure from migration stream to memory, and extend_put to dump the structure into the migration stream. This extension mirrors VMStateInfo. We then modify vmstate_save_state and vmstate_load_state so that when VMS_CSTM is encountered, extend_put and extend_get are called respectively with the knowledge of the meta data. To make it work for QTAILQ in qemu/queue.h, we created the meta data format, extend_get and extend_put for it. We will use this approach to transfer pending_events and ccs_list in spapr state. We also create some macros in qemu/queue.h to get the layout information about QTAILQ. 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 | 59 +++++++++++++++++++++++++++++++ include/qemu/queue.h | 38 ++++++++++++++++++++ migration/vmstate.c | 84 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 181 insertions(+)