On 06/01/2016 08:29 AM, Paolo Bonzini wrote: > > > On 31/05/2016 23:53, Jianjun Duan wrote: >> >> >> On 05/31/2016 12:54 PM, Paolo Bonzini wrote: >>> >>> >>> ----- Original Message ----- >>>> From: "Jianjun Duan" <du...@linux.vnet.ibm.com> >>>> To: qemu-devel@nongnu.org >>>> Cc: qemu-...@nongnu.org, du...@linux.vnet.ibm.com, dmi...@daynix.com, >>>> "peter maydell" <peter.mayd...@linaro.org>, >>>> kra...@redhat.com, m...@redhat.com, da...@gibson.dropbear.id.au, >>>> pbonz...@redhat.com, veroniaba...@gmail.com, >>>> quint...@redhat.com, "amit shah" <amit.s...@redhat.com>, >>>> mre...@redhat.com, kw...@redhat.com, r...@twiddle.net, >>>> aurel...@aurel32.net, "leon alrae" <leon.al...@imgtec.com>, >>>> blauwir...@gmail.com, "mark cave-ayland" >>>> <mark.cave-ayl...@ilande.co.uk>, mdr...@linux.vnet.ibm.com >>>> Sent: Tuesday, May 31, 2016 8:02:42 PM >>>> Subject: [QEMU RFC PATCH v3 4/6] Migration: migrate QTAILQ >>>> >>>> Currently we cannot directly transfer a QTAILQ instance because of the >>>> limitation in the migration code. Here we introduce an approach to >>>> transfer such structures. In our approach such a structure is tagged >>>> with VMS_CSTM. We then modified vmstate_save_state and vmstate_load_state >>>> so that when VMS_CSTM is encountered, put and get from VMStateInfo are >>>> called respectively. This approach will be used to transfer pending_events >>>> and ccs_list in spapr state. >>>> >>>> We also create some macros in qemu/queue.h to access a QTAILQ using pointer >>>> arithmetic. This ensures that we do not depend on the implementation >>>> details about QTAILQ in the migration code. >>>> >>>> Signed-off-by: Jianjun Duan <du...@linux.vnet.ibm.com> >>>> --- >>>> include/migration/vmstate.h | 22 +++++++++++++ >>>> include/qemu/queue.h | 32 ++++++++++++++++++ >>>> migration/vmstate.c | 79 >>>> +++++++++++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 133 insertions(+) >>>> >>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h >>>> index 56a4171..da4ef7f 100644 >>>> --- a/include/migration/vmstate.h >>>> +++ b/include/migration/vmstate.h >>>> @@ -185,6 +185,8 @@ enum VMStateFlags { >>>> * to determine the number of entries in the array. Only valid in >>>> * combination with one of VMS_VARRAY*. */ >>>> VMS_MULTIPLY_ELEMENTS = 0x4000, >>>> + /* For fields which need customized handling, such as QTAILQ in >>>> queue.h*/ >>>> + VMS_CSTM = 0x8000, >>> >>> Please call this VMS_LINKED. It can be adapted to other data >>> structures in qemu/queue.h if there is a need later on. >>> >>>> }; >>>> >>>> struct VMStateField { >>>> @@ -245,6 +247,7 @@ extern const VMStateInfo vmstate_info_timer; >>>> extern const VMStateInfo vmstate_info_buffer; >>>> extern const VMStateInfo vmstate_info_unused_buffer; >>>> extern const VMStateInfo vmstate_info_bitmap; >>>> +extern const VMStateInfo vmstate_info_qtailq; >>>> >>>> #define type_check_2darray(t1,t2,n,m) ((t1(*)[n][m])0 - (t2*)0) >>>> #define type_check_array(t1,t2,n) ((t1(*)[n])0 - (t2*)0) >>>> @@ -656,6 +659,25 @@ extern const VMStateInfo vmstate_info_bitmap; >>>> .offset = offsetof(_state, _field), \ >>>> } >>>> >>>> +/* For QTAILQ that need customized handling >>>> + * _type: type of QTAILQ element >>>> + * _next: name of QTAILQ entry field in QTAILQ element >>>> + * _vmsd: VMSD for QTAILQ element >>>> + * size: size of QTAILQ element >>>> + * start: offset of QTAILQ entry in QTAILQ element >>>> + */ >>>> +#define VMSTATE_QTAILQ_V(_field, _state, _version, _vmsd, _type, _next) \ >>>> +{ \ >>>> + .name = (stringify(_field)), \ >>>> + .version_id = (_version), \ >>>> + .vmsd = &(_vmsd), \ >>>> + .size = sizeof(_type), \ >>>> + .info = &vmstate_info_qtailq, \ >>>> + .flags = VMS_CSTM, \ >>>> + .offset = offsetof(_state, _field), \ >>>> + .start = offsetof(_type, _next), \ >>>> +} >>>> + >>>> /* _f : field name >>>> _f_n : num of elements field_name >>>> _n : num of elements >>>> diff --git a/include/qemu/queue.h b/include/qemu/queue.h >>>> index f781aa2..003e368 100644 >>>> --- a/include/qemu/queue.h >>>> +++ b/include/qemu/queue.h >>>> @@ -437,3 +437,35 @@ struct { >>>> \ >>>> (*(((struct headname *)((elm)->field.tqe_prev))->tqh_last)) >>>> >>>> #endif /* !QEMU_SYS_QUEUE_H_ */ >>>> + >>>> +/* >>>> + * Offsets of layout of a tail queue head. >>>> + */ >>>> +#define QTAILQ_FIRST_OFFSET 0 >>>> +#define QTAILQ_LAST_OFFSET (sizeof(void *)) >>>> + >>>> +/* >>>> + * Offsets of layout of a tail queue element. >>>> + */ >>>> +#define QTAILQ_NEXT_OFFSET 0 >>>> +#define QTAILQ_PREV_OFFSET (sizeof(void *)) >>>> + >>>> +/* >>>> + * Tail queue tranversal using pointer arithmetic. >>>> + */ >>>> +#define QTAILQ_RAW_FOREACH(elm, head, entry) >>>> \ >>>> + for ((elm) = *((void **) ((char *) (head) + QTAILQ_FIRST_OFFSET)); >>>> \ >>>> + (elm); >>>> \ >>>> + (elm) = >>>> \ >>>> + *((void **) ((char *) (elm) + (entry) + >>>> QTAILQ_NEXT_OFFSET))) >>>> +/* >>>> + * Tail queue insertion using pointer arithmetic. >>>> + */ >>>> +#define QTAILQ_RAW_INSERT_TAIL(head, elm, entry) do { >>>> \ >>>> + *((void **) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET)) = >>>> NULL; >>>> \ >>>> + *((void **) ((char *) (elm) + (entry) + QTAILQ_PREV_OFFSET)) = >>>> \ >>>> + *((void **) ((char *) (head) +QTAILQ_LAST_OFFSET)); >>>> \ >>>> + **((void ***)((char *) (head) +QTAILQ_LAST_OFFSET)) = (elm); >>>> \ >>>> + *((void **) ((char *) (head) + QTAILQ_LAST_OFFSET)) = >>>> \ >>>> + (void *) ((char *) (elm) + (entry) + QTAILQ_NEXT_OFFSET); >>>> \ >>>> +} while (/*CONSTCOND*/0) >>>> diff --git a/migration/vmstate.c b/migration/vmstate.c >>>> index 644ba1f..ff56650 100644 >>>> --- a/migration/vmstate.c >>>> +++ b/migration/vmstate.c >>>> @@ -5,7 +5,9 @@ >>>> #include "migration/vmstate.h" >>>> #include "qemu/bitops.h" >>>> #include "qemu/error-report.h" >>>> +#include "qemu/queue.h" >>>> #include "trace.h" >>>> +#include "migration/qjson.h" >>>> >>>> static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription >>>> *vmsd, >>>> void *opaque, QJSON *vmdesc); >>>> @@ -120,6 +122,8 @@ int vmstate_load_state(QEMUFile *f, const >>>> VMStateDescription *vmsd, >>>> if (field->flags & VMS_STRUCT) { >>>> ret = vmstate_load_state(f, field->vmsd, addr, >>>> field->vmsd->version_id); >>>> + } else if (field->flags & VMS_CSTM) { >>>> + ret = field->info->get(f, addr, size, field); >>>> } else { >>>> ret = field->info->get(f, addr, size, NULL); >>>> >>>> @@ -192,6 +196,8 @@ static const char *vmfield_get_type_name(VMStateField >>>> *field) >>>> >>>> if (field->flags & VMS_STRUCT) { >>>> type = "struct"; >>>> + } else if (field->flags & VMS_CSTM) { >>>> + type = "customized"; >>>> } else if (field->info->name) { >>>> type = field->info->name; >>>> } >>>> @@ -326,6 +332,8 @@ void vmstate_save_state(QEMUFile *f, const >>>> VMStateDescription *vmsd, >>>> } >>>> if (field->flags & VMS_STRUCT) { >>>> vmstate_save_state(f, field->vmsd, addr, vmdesc_loop); >>>> + } else if (field->flags & VMS_CSTM) { >>>> + field->info->put(f, addr, size, field, vmdesc_loop); >>>> } else { >>>> field->info->put(f, addr, size, NULL, NULL); >>>> } >>>> @@ -938,3 +946,74 @@ const VMStateInfo vmstate_info_bitmap = { >>>> .get = get_bitmap, >>>> .put = put_bitmap, >>>> }; >>>> + >>>> +/*get for QTAILQ */ >>>> +static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size, >>>> + VMStateField *field) >>>> +{ >>>> + bool link; >>>> + int ret = 0; >>>> + const VMStateDescription *vmsd = field->vmsd; >>>> + size_t size = field->size; >>>> + size_t entry = field->start; >>>> + int version_id = field->version_id; >>>> + void *elm; >>>> + >>>> + trace_vmstate_load_state(vmsd->name, version_id); >>>> + if (version_id > vmsd->version_id) { >>>> + trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL); >>>> + return -EINVAL; >>>> + } >>>> + if (version_id < vmsd->minimum_version_id) { >>>> + trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + while (true) { >>>> + vmstate_info_bool.get(f, &link, sizeof(bool), NULL); >>> >>> You can use qemu_get_byte here, and likewise qemu_put_byte below in >>> put_qtailq. >> >> qemu_get/put is indeed better choice. >>> >>>> + if (!link) { >>>> + break; >>>> + } >>>> + >>>> + elm = g_malloc(size); >>>> + ret = vmstate_load_state(f, vmsd, elm, version_id); >>>> + if (ret) { >>>> + return ret; >>>> + } >>>> + QTAILQ_RAW_INSERT_TAIL(pv, elm, entry); >>>> + } >>>> + >>>> + trace_vmstate_load_state_end(vmsd->name, "end", ret); >>>> + return ret; >>>> +} >>>> + >>>> +/* put for QTAILQ */ >>>> +static void put_qtailq(QEMUFile *f, void *pv, size_t unused_size, >>>> + VMStateField *field, QJSON *vmdesc) >>>> +{ >>>> + bool link = true; >>>> + const VMStateDescription *vmsd = field->vmsd; >>>> + size_t entry = field->start; >>>> + void *elm; >>>> + >>>> + if (vmdesc) { >>>> + json_prop_str(vmdesc, "vmsd_name", vmsd->name); >>>> + json_prop_int(vmdesc, "version", vmsd->version_id); >>>> + json_start_array(vmdesc, "fields"); >>> >>> You need to store the fields exactly once here, even if there are >>> 0 or >1 elements. >>> >> Do you mean the json entries? I think it is already doing that. > > In the case of 0 entries we don't go through the loop, so the JSON > entries are definitely missing in that case. I'm not sure if QJSON > handles duplicates in the case of 2+ entries. The vmsd here is the vmsd for the queue elements. Not for the queue. Maybe the stuff written here should be information about the qtailq instead, but we don't have a vmsd for the queue as a whole.
As it stands, it will record the name, version of the element type. If the queue is empty, it records nothing in the fields. otherwise it will record each element and repeat. In vmstate_save_state, the vmsd of the array element for a uncompressed array is recorded every time an array element is saved. The number of written bytes is also recorded. If the array has 0 elements, it is not recorded. If we want to make the behavior consistent, we should not do any json stuff in put_qtatilq function. If we want to record the vmsd of the queue element exactly once, I can set the vmdesc to null after the first iteration. But we may need a recursive function just to dump out the vmsd when the queue is empty, and record that 0 bytes are written for each field. I would say that let's remove the json stuff here to be consistent with array behavior. > > Thanks, > > Paolo > Thanks, Jianjun