diff mbox

[QEMU,v10,2/3] migration: migrate QTAILQ

Message ID 1477943636-21024-3-git-send-email-duanj@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jianjun Duan Oct. 31, 2016, 7:53 p.m. UTC
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.

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        | 61 +++++++++++++++++++++++++++++++++++++++++
 migration/trace-events      |  4 +++
 migration/vmstate.c         | 67 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 152 insertions(+)

Comments

Juan Quintela Nov. 2, 2016, 10:45 a.m. UTC | #1
Jianjun Duan <duanj@linux.vnet.ibm.com> wrote:
> Currently we cannot directly transfer a QTAILQ instance because of the
> limitation in the migration code. Here we introduce an approach to
> transfer such structures. We created VMStateInfo vmstate_info_qtailq
> for QTAILQ. Similar VMStateInfo can be created for other data structures
> such as list.
>
> This approach will be used to transfer pending_events and ccs_list in spapr
> state.
>
> We also create some macros in qemu/queue.h to access a QTAILQ using pointer
> arithmetic. This ensures that we do not depend on the implementation
> details about QTAILQ in the migration code.
>
> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>


> +
> +    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);

I think this is not generic enough.  We really need to allocate a new
element, and then fill it with default values.

virtio list code use it in this way.


Thanks, Juan.
Paolo Bonzini Nov. 2, 2016, 4:38 p.m. UTC | #2
On 02/11/2016 11:45, Juan Quintela wrote:
>> > +    while (qemu_get_byte(f)) {
>> > +        elm = g_malloc(size);
> I think this is not generic enough.  We really need to allocate a new
> element, and then fill it with default values.
> 
> virtio list code use it in this way.

One thing at a time?  The allocation and "looping" are different
mechanisms.  We can customize the allocation later.

Paolo
Jianjun Duan Nov. 2, 2016, 5:05 p.m. UTC | #3
On 11/02/2016 03:45 AM, Juan Quintela wrote:
> Jianjun Duan <duanj@linux.vnet.ibm.com> wrote:
>> Currently we cannot directly transfer a QTAILQ instance because of the
>> limitation in the migration code. Here we introduce an approach to
>> transfer such structures. We created VMStateInfo vmstate_info_qtailq
>> for QTAILQ. Similar VMStateInfo can be created for other data structures
>> such as list.
>>
>> This approach will be used to transfer pending_events and ccs_list in spapr
>> state.
>>
>> We also create some macros in qemu/queue.h to access a QTAILQ using pointer
>> arithmetic. This ensures that we do not depend on the implementation
>> details about QTAILQ in the migration code.
>>
>> Signed-off-by: Jianjun Duan <duanj@linux.vnet.ibm.com>
> 
> 
>> +
>> +    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);
> 
> I think this is not generic enough.  We really need to allocate a new
> element, and then fill it with default values.
> 
> virtio list code use it in this way.
> 
> 
To do that we need probably to expand VMStateDescription and/or
VMStateField so that a default value can be supplied. Or we can always
fill the untouched fields in post_load.

Thanks,
Jianjun

> Thanks, Juan.
>
Halil Pasic Nov. 3, 2016, 11:14 a.m. UTC | #4
On 11/02/2016 05:38 PM, Paolo Bonzini wrote:
> 
> 
> On 02/11/2016 11:45, Juan Quintela wrote:
>>>> +    while (qemu_get_byte(f)) {
>>>> +        elm = g_malloc(size);
>> I think this is not generic enough.  We really need to allocate a new
>> element, and then fill it with default values.
>>
>> virtio list code use it in this way.
> 
> One thing at a time?  The allocation and "looping" are different
> mechanisms.  We can customize the allocation later.
> 
> Paolo
> 

+1

And while at it how about s/g_malloc/gmalloc0? This would possibly make
something like mismatch in .start between source and target easier to
detect, along with the 'untouched' fields mentioned by Jianjun.
(Although I'm not sure how untouched fields and the virtio list use is
connected. In my understanding the problem pointed out by Juan has
something to do with the address of a element/node having significance
beyond the list.)

Halil
Halil Pasic Nov. 3, 2016, 11:32 a.m. UTC | #5
On 11/03/2016 12:14 PM, Halil Pasic wrote:
> 
> 
> On 11/02/2016 05:38 PM, Paolo Bonzini wrote:
>>
>>
>> On 02/11/2016 11:45, Juan Quintela wrote:
>>>>> +    while (qemu_get_byte(f)) {
>>>>> +        elm = g_malloc(size);
>>> I think this is not generic enough.  We really need to allocate a new
>>> element, and then fill it with default values.
>>>
>>> virtio list code use it in this way.
>>
>> One thing at a time?  The allocation and "looping" are different
>> mechanisms.  We can customize the allocation later.
>>
>> Paolo
>>
> 

[..]

> 
> And while at it how about s/g_malloc/gmalloc0? This would possibly make
> something like mismatch in .start between source and target easier to
> detect

Sorry guys, I just realized this was stupid of me. Please ignore.
diff mbox

Patch

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index d0e37b5..318a6f1 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -251,6 +251,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)
@@ -662,6 +663,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..aae23f2 100644
--- a/include/qemu/queue.h
+++ b/include/qemu/queue.h
@@ -438,4 +438,65 @@  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_qh ((typeof(dummy_q->head) *) 0)
+#define dummy_qe ((DUMMY_Q_ENTRY *) 0)
+
+/*
+ * Offsets of layout of a tail queue head.
+ */
+#define QTAILQ_FIRST_OFFSET ((size_t) &(QTAILQ_FIRST(dummy_qh)))
+#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 ((size_t) (&QTAILQ_NEXT(dummy_qe, 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 dfee75a..9a6ec59 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -52,6 +52,10 @@  vmstate_n_elems(const char *name, int n_elems) "%s: %d"
 vmstate_subsection_load(const char *parent) "%s"
 vmstate_subsection_load_bad(const char *parent,  const char *sub, const char *sub2) "%s: %s/%s"
 vmstate_subsection_load_good(const char *parent) "%s"
+get_qtailq(const char *name, int version_id) "%s v%d"
+get_qtailq_end(const char *name, const char *reason, int val) "%s %s/%d"
+put_qtailq(const char *name, int version_id) "%s v%d"
+put_qtailq_end(const char *name, const char *reason) "%s %s"
 
 # migration/qemu-file.c
 qemu_file_fclose(void) ""
diff --git a/migration/vmstate.c b/migration/vmstate.c
index d188afa..70e0472 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"
 
@@ -948,3 +949,69 @@  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 void 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");
+}
+const VMStateInfo vmstate_info_qtailq = {
+    .name = "qtailq",
+    .get  = get_qtailq,
+    .put  = put_qtailq,
+};