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 <du...@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, >>>> +}; >>>> >>> > >
