| Message ID | 20191122182943.4656-2-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | VIRTIO-IOMMU device | expand |
* Eric Auger (eric.auger@redhat.com) wrote: > Support QLIST migration using the same principle as QTAILQ: > 94869d5c52 ("migration: migrate QTAILQ"). > > The VMSTATE_QLIST_V macro has the same proto as VMSTATE_QTAILQ_V. > The change mainly resides in QLIST RAW macros: QLIST_RAW_INSERT_HEAD > and QLIST_RAW_REVERSE. > > Tests also are provided. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > --- > > v5 - v6: > - by doing more advanced testing with virtio-iommu migration > I noticed this was broken. "prev" field was not set properly. > I improved the tests to manipulate both the next and prev > fields. > - Removed Peter and Juan's R-b > --- > include/migration/vmstate.h | 21 +++++ > include/qemu/queue.h | 39 +++++++++ > migration/trace-events | 5 ++ > migration/vmstate-types.c | 70 +++++++++++++++ > tests/test-vmstate.c | 170 ++++++++++++++++++++++++++++++++++++ > 5 files changed, 305 insertions(+) > > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h > index ac4f46a67d..08683d93c6 100644 > --- a/include/migration/vmstate.h > +++ b/include/migration/vmstate.h > @@ -227,6 +227,7 @@ extern const VMStateInfo vmstate_info_tmp; > extern const VMStateInfo vmstate_info_bitmap; > extern const VMStateInfo vmstate_info_qtailq; > extern const VMStateInfo vmstate_info_gtree; > +extern const VMStateInfo vmstate_info_qlist; > > #define type_check_2darray(t1,t2,n,m) ((t1(*)[n][m])0 - (t2*)0) > /* > @@ -796,6 +797,26 @@ extern const VMStateInfo vmstate_info_gtree; > .offset = offsetof(_state, _field), \ > } > > +/* > + * For migrating a QLIST > + * Target QLIST needs be properly initialized. > + * _type: type of QLIST element > + * _next: name of QLIST_ENTRY entry field in QLIST element > + * _vmsd: VMSD for QLIST element > + * size: size of QLIST element > + * start: offset of QLIST_ENTRY in QTAILQ element > + */ > +#define VMSTATE_QLIST_V(_field, _state, _version, _vmsd, _type, _next) \ > +{ \ > + .name = (stringify(_field)), \ > + .version_id = (_version), \ > + .vmsd = &(_vmsd), \ > + .size = sizeof(_type), \ > + .info = &vmstate_info_qlist, \ > + .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 4764d93ea3..4d4554a7ce 100644 > --- a/include/qemu/queue.h > +++ b/include/qemu/queue.h > @@ -501,4 +501,43 @@ union { \ > QTAILQ_RAW_TQH_CIRC(head)->tql_prev = QTAILQ_RAW_TQE_CIRC(elm, entry); \ > } while (/*CONSTCOND*/0) > > +#define QLIST_RAW_FIRST(head) \ > + field_at_offset(head, 0, void *) > + > +#define QLIST_RAW_NEXT(elm, entry) \ > + field_at_offset(elm, entry, void *) > + > +#define QLIST_RAW_PREVIOUS(elm, entry) \ > + field_at_offset(elm, entry + sizeof(void *), void *) > + > +#define QLIST_RAW_FOREACH(elm, head, entry) \ > + for ((elm) = *QLIST_RAW_FIRST(head); \ > + (elm); \ > + (elm) = *QLIST_RAW_NEXT(elm, entry)) > + > +#define QLIST_RAW_INSERT_HEAD(head, elm, entry) do { \ > + void *first = *QLIST_RAW_FIRST(head); \ > + *QLIST_RAW_FIRST(head) = elm; \ > + *QLIST_RAW_PREVIOUS(elm, entry) = QLIST_RAW_FIRST(head); \ > + if (first) { \ > + *QLIST_RAW_NEXT(elm, entry) = first; \ > + *QLIST_RAW_PREVIOUS(first, entry) = QLIST_RAW_NEXT(elm, entry); \ > + } else { \ > + *QLIST_RAW_NEXT(elm, entry) = NULL; \ > + } \ > +} while (0) > + > +#define QLIST_RAW_REVERSE(head, elm, entry) do { \ > + void *iter = *QLIST_RAW_FIRST(head), *prev = NULL, *next; \ > + while (iter) { \ > + next = *QLIST_RAW_NEXT(iter, entry); \ > + *QLIST_RAW_PREVIOUS(iter, entry) = QLIST_RAW_NEXT(next, entry); \ > + *QLIST_RAW_NEXT(iter, entry) = prev; \ > + prev = iter; \ > + iter = next; \ > + } \ > + *QLIST_RAW_FIRST(head) = prev; \ > + *QLIST_RAW_PREVIOUS(prev, entry) = QLIST_RAW_FIRST(head); \ > +} while (0) > + > #endif /* QEMU_SYS_QUEUE_H */ > diff --git a/migration/trace-events b/migration/trace-events > index 6dee7b5389..e0a33cffca 100644 > --- a/migration/trace-events > +++ b/migration/trace-events > @@ -76,6 +76,11 @@ get_gtree_end(const char *field_name, const char *key_vmsd_name, const char *val > put_gtree(const char *field_name, const char *key_vmsd_name, const char *val_vmsd_name, uint32_t nnodes) "%s(%s/%s) nnodes=%d" > put_gtree_end(const char *field_name, const char *key_vmsd_name, const char *val_vmsd_name, int ret) "%s(%s/%s) %d" > > +get_qlist(const char *field_name, const char *vmsd_name, int version_id) "%s(%s v%d)" > +get_qlist_end(const char *field_name, const char *vmsd_name) "%s(%s)" > +put_qlist(const char *field_name, const char *vmsd_name, int version_id) "%s(%s v%d)" > +put_qlist_end(const char *field_name, const char *vmsd_name) "%s(%s)" > + > # qemu-file.c > qemu_file_fclose(void) "" > > diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c > index 7236cf92bc..1eee36773a 100644 > --- a/migration/vmstate-types.c > +++ b/migration/vmstate-types.c > @@ -843,3 +843,73 @@ const VMStateInfo vmstate_info_gtree = { > .get = get_gtree, > .put = put_gtree, > }; > + > +static int put_qlist(QEMUFile *f, void *pv, size_t unused_size, > + const 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; > + int ret; > + > + trace_put_qlist(field->name, vmsd->name, vmsd->version_id); > + QLIST_RAW_FOREACH(elm, pv, entry_offset) { > + qemu_put_byte(f, true); > + ret = vmstate_save_state(f, vmsd, elm, vmdesc); > + if (ret) { > + error_report("%s: failed to save %s (%d)", field->name, > + vmsd->name, ret); > + return ret; > + } > + } > + qemu_put_byte(f, false); > + trace_put_qlist_end(field->name, vmsd->name); > + > + return 0; > +} > + > +static int get_qlist(QEMUFile *f, void *pv, size_t unused_size, > + const VMStateField *field) > +{ > + int ret = 0; > + const VMStateDescription *vmsd = field->vmsd; > + /* size of a QLIST element */ > + size_t size = field->size; > + /* offset of the QLIST entry in a QLIST element */ > + size_t entry_offset = field->start; > + int version_id = field->version_id; > + void *elm; > + > + trace_get_qlist(field->name, vmsd->name, vmsd->version_id); > + if (version_id > vmsd->version_id) { > + error_report("%s %s", vmsd->name, "too new"); > + return -EINVAL; > + } > + if (version_id < vmsd->minimum_version_id) { > + error_report("%s %s", vmsd->name, "too old"); > + return -EINVAL; > + } > + > + while (qemu_get_byte(f)) { > + elm = g_malloc(size); > + ret = vmstate_load_state(f, vmsd, elm, version_id); > + if (ret) { > + error_report("%s: failed to load %s (%d)", field->name, > + vmsd->name, ret); > + g_free(elm); > + return ret; > + } > + QLIST_RAW_INSERT_HEAD(pv, elm, entry_offset); > + } > + QLIST_RAW_REVERSE(pv, elm, entry_offset); Can you explain why you need to do a REVERSE on the loaded list, rather than using doing a QLIST_INSERT_AFTER to always insert at the end? Other than that it looks good. Dave > + trace_get_qlist_end(field->name, vmsd->name); > + > + return ret; > +} > + > +const VMStateInfo vmstate_info_qlist = { > + .name = "qlist", > + .get = get_qlist, > + .put = put_qlist, > +}; > diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c > index 1e5be1d4ff..9660f932b9 100644 > --- a/tests/test-vmstate.c > +++ b/tests/test-vmstate.c > @@ -926,6 +926,28 @@ static const VMStateDescription vmstate_domain = { > } > }; > > +/* test QLIST Migration */ > + > +typedef struct TestQListElement { > + uint32_t id; > + QLIST_ENTRY(TestQListElement) next; > +} TestQListElement; > + > +typedef struct TestQListContainer { > + uint32_t id; > + QLIST_HEAD(, TestQListElement) list; > +} TestQListContainer; > + > +static const VMStateDescription vmstate_qlist_element = { > + .name = "test/queue list", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT32(id, TestQListElement), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static const VMStateDescription vmstate_iommu = { > .name = "iommu", > .version_id = 1, > @@ -939,6 +961,18 @@ static const VMStateDescription vmstate_iommu = { > } > }; > > +static const VMStateDescription vmstate_container = { > + .name = "test/container/qlist", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT32(id, TestQListContainer), > + VMSTATE_QLIST_V(list, TestQListContainer, 1, vmstate_qlist_element, > + TestQListElement, next), > + VMSTATE_END_OF_LIST() > + } > +}; > + > uint8_t first_domain_dump[] = { > /* id */ > 0x00, 0x0, 0x0, 0x6, > @@ -1229,6 +1263,140 @@ static void test_gtree_load_iommu(void) > qemu_fclose(fload); > } > > +static uint8_t qlist_dump[] = { > + 0x00, 0x00, 0x00, 0x01, /* container id */ > + 0x1, /* start of a */ > + 0x00, 0x00, 0x00, 0x0a, > + 0x1, /* start of b */ > + 0x00, 0x00, 0x0b, 0x00, > + 0x1, /* start of c */ > + 0x00, 0x0c, 0x00, 0x00, > + 0x1, /* start of d */ > + 0x0d, 0x00, 0x00, 0x00, > + 0x0, /* end of list */ > + QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */ > +}; > + > +static TestQListContainer *alloc_container(void) > +{ > + TestQListElement *a = g_malloc(sizeof(TestQListElement)); > + TestQListElement *b = g_malloc(sizeof(TestQListElement)); > + TestQListElement *c = g_malloc(sizeof(TestQListElement)); > + TestQListElement *d = g_malloc(sizeof(TestQListElement)); > + TestQListContainer *container = g_malloc(sizeof(TestQListContainer)); > + > + a->id = 0x0a; > + b->id = 0x0b00; > + c->id = 0xc0000; > + d->id = 0xd000000; > + container->id = 1; > + > + QLIST_INIT(&container->list); > + QLIST_INSERT_HEAD(&container->list, d, next); > + QLIST_INSERT_HEAD(&container->list, c, next); > + QLIST_INSERT_HEAD(&container->list, b, next); > + QLIST_INSERT_HEAD(&container->list, a, next); > + return container; > +} > + > +static void free_container(TestQListContainer *container) > +{ > + TestQListElement *iter, *tmp; > + > + QLIST_FOREACH_SAFE(iter, &container->list, next, tmp) { > + QLIST_REMOVE(iter, next); > + g_free(iter); > + } > + g_free(container); > +} > + > +static void compare_containers(TestQListContainer *c1, TestQListContainer *c2) > +{ > + TestQListElement *first_item_c1, *first_item_c2; > + > + while (!QLIST_EMPTY(&c1->list)) { > + first_item_c1 = QLIST_FIRST(&c1->list); > + first_item_c2 = QLIST_FIRST(&c2->list); > + assert(first_item_c2); > + assert(first_item_c1->id == first_item_c2->id); > + QLIST_REMOVE(first_item_c1, next); > + QLIST_REMOVE(first_item_c2, next); > + g_free(first_item_c1); > + g_free(first_item_c2); > + } > + assert(QLIST_EMPTY(&c2->list)); > +} > + > +/* > + * Check the prev & next fields are correct by doing list > + * manipulations on the container. We will do that for both > + * the source and the destination containers > + */ > +static void manipulate_container(TestQListContainer *c) > +{ > + TestQListElement *prev, *iter = QLIST_FIRST(&c->list); > + TestQListElement *elem; > + > + elem = g_malloc(sizeof(TestQListElement)); > + elem->id = 0x12; > + QLIST_INSERT_AFTER(iter, elem, next); > + > + elem = g_malloc(sizeof(TestQListElement)); > + elem->id = 0x13; > + QLIST_INSERT_HEAD(&c->list, elem, next); > + > + while (iter) { > + prev = iter; > + iter = QLIST_NEXT(iter, next); > + } > + > + elem = g_malloc(sizeof(TestQListElement)); > + elem->id = 0x14; > + QLIST_INSERT_BEFORE(prev, elem, next); > + > + elem = g_malloc(sizeof(TestQListElement)); > + elem->id = 0x15; > + QLIST_INSERT_AFTER(prev, elem, next); > + > + QLIST_REMOVE(prev, next); > + g_free(prev); > +} > + > +static void test_save_qlist(void) > +{ > + TestQListContainer *container = alloc_container(); > + > + save_vmstate(&vmstate_container, container); > + compare_vmstate(qlist_dump, sizeof(qlist_dump)); > + free_container(container); > +} > + > +static void test_load_qlist(void) > +{ > + QEMUFile *fsave, *fload; > + TestQListContainer *orig_container = alloc_container(); > + TestQListContainer *dest_container = g_malloc0(sizeof(TestQListContainer)); > + char eof; > + > + QLIST_INIT(&dest_container->list); > + > + fsave = open_test_file(true); > + qemu_put_buffer(fsave, qlist_dump, sizeof(qlist_dump)); > + g_assert(!qemu_file_get_error(fsave)); > + qemu_fclose(fsave); > + > + fload = open_test_file(false); > + vmstate_load_state(fload, &vmstate_container, dest_container, 1); > + eof = qemu_get_byte(fload); > + g_assert(!qemu_file_get_error(fload)); > + g_assert_cmpint(eof, ==, QEMU_VM_EOF); > + manipulate_container(orig_container); > + manipulate_container(dest_container); > + compare_containers(orig_container, dest_container); > + free_container(orig_container); > + free_container(dest_container); > +} > + > typedef struct TmpTestStruct { > TestStruct *parent; > int64_t diff; > @@ -1353,6 +1521,8 @@ int main(int argc, char **argv) > g_test_add_func("/vmstate/gtree/load/loaddomain", test_gtree_load_domain); > g_test_add_func("/vmstate/gtree/save/saveiommu", test_gtree_save_iommu); > g_test_add_func("/vmstate/gtree/load/loadiommu", test_gtree_load_iommu); > + g_test_add_func("/vmstate/qlist/save/saveqlist", test_save_qlist); > + g_test_add_func("/vmstate/qlist/load/loadqlist", test_load_qlist); > g_test_add_func("/vmstate/tmp_struct", test_tmp_struct); > g_test_run(); > > -- > 2.20.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Eric Auger (eric.auger@redhat.com) wrote: >> Support QLIST migration using the same principle as QTAILQ: >> 94869d5c52 ("migration: migrate QTAILQ"). >> >> The VMSTATE_QLIST_V macro has the same proto as VMSTATE_QTAILQ_V. >> The change mainly resides in QLIST RAW macros: QLIST_RAW_INSERT_HEAD >> and QLIST_RAW_REVERSE. >> >> Tests also are provided. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> >> + while (qemu_get_byte(f)) { >> + elm = g_malloc(size); >> + ret = vmstate_load_state(f, vmsd, elm, version_id); >> + if (ret) { >> + error_report("%s: failed to load %s (%d)", field->name, >> + vmsd->name, ret); >> + g_free(elm); >> + return ret; >> + } >> + QLIST_RAW_INSERT_HEAD(pv, elm, entry_offset); >> + } >> + QLIST_RAW_REVERSE(pv, elm, entry_offset); > > Can you explain why you need to do a REVERSE on the loaded list, > rather than using doing a QLIST_INSERT_AFTER to always insert at > the end? > > Other than that it looks good. This was my fault (integrated as this is). Old code had a "walk to the end of the list" and then insert. I told it was way faster just to insert and the beggining and then reverse. I didn't noticed that we had the previous element to know where to insert. Eric, feel free to send a patch on top of this, or I will do it. Later, Juan.
Hi Juan, On 1/8/20 2:19 PM, Juan Quintela wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: >> * Eric Auger (eric.auger@redhat.com) wrote: >>> Support QLIST migration using the same principle as QTAILQ: >>> 94869d5c52 ("migration: migrate QTAILQ"). >>> >>> The VMSTATE_QLIST_V macro has the same proto as VMSTATE_QTAILQ_V. >>> The change mainly resides in QLIST RAW macros: QLIST_RAW_INSERT_HEAD >>> and QLIST_RAW_REVERSE. >>> >>> Tests also are provided. >>> >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>> >>> + while (qemu_get_byte(f)) { >>> + elm = g_malloc(size); >>> + ret = vmstate_load_state(f, vmsd, elm, version_id); >>> + if (ret) { >>> + error_report("%s: failed to load %s (%d)", field->name, >>> + vmsd->name, ret); >>> + g_free(elm); >>> + return ret; >>> + } >>> + QLIST_RAW_INSERT_HEAD(pv, elm, entry_offset); >>> + } >>> + QLIST_RAW_REVERSE(pv, elm, entry_offset); >> >> Can you explain why you need to do a REVERSE on the loaded list, >> rather than using doing a QLIST_INSERT_AFTER to always insert at >> the end? >> >> Other than that it looks good. > > This was my fault (integrated as this is). > > Old code had a "walk to the end of the list" and then insert. > I told it was way faster just to insert and the beggining and then > reverse. I didn't noticed that we had the previous element to know > where to insert. Not sure I get your comment. To insert at the end one needs to walk though the list. The head has no prev pointer pointing to the tail as opposed to the queue. So I understood Dave's comment as "just explain why you prefered this solution against the QLIST_INSERT_AFTER alternative. Thanks Eric > > Eric, feel free to send a patch on top of this, or I will do it. > > Later, Juan. > >
Auger Eric <eric.auger@redhat.com> wrote: > Hi Juan, > > On 1/8/20 2:19 PM, Juan Quintela wrote: >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: >>> * Eric Auger (eric.auger@redhat.com) wrote: >>>> Support QLIST migration using the same principle as QTAILQ: >>>> 94869d5c52 ("migration: migrate QTAILQ"). >>>> >>>> The VMSTATE_QLIST_V macro has the same proto as VMSTATE_QTAILQ_V. >>>> The change mainly resides in QLIST RAW macros: QLIST_RAW_INSERT_HEAD >>>> and QLIST_RAW_REVERSE. >>>> >>>> Tests also are provided. >>>> >>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>> >>>> + while (qemu_get_byte(f)) { >>>> + elm = g_malloc(size); >>>> + ret = vmstate_load_state(f, vmsd, elm, version_id); >>>> + if (ret) { >>>> + error_report("%s: failed to load %s (%d)", field->name, >>>> + vmsd->name, ret); >>>> + g_free(elm); >>>> + return ret; >>>> + } >>>> + QLIST_RAW_INSERT_HEAD(pv, elm, entry_offset); >>>> + } >>>> + QLIST_RAW_REVERSE(pv, elm, entry_offset); >>> >>> Can you explain why you need to do a REVERSE on the loaded list, >>> rather than using doing a QLIST_INSERT_AFTER to always insert at >>> the end? >>> >>> Other than that it looks good. >> >> This was my fault (integrated as this is). >> >> Old code had a "walk to the end of the list" and then insert. >> I told it was way faster just to insert and the beggining and then >> reverse. I didn't noticed that we had the previous element to know >> where to insert. > > Not sure I get your comment. To insert at the end one needs to walk > though the list. The head has no prev pointer pointing to the tail as > opposed to the queue. So I understood Dave's comment as "just explain > why you prefered this solution against the QLIST_INSERT_AFTER alternative. You have the previous inserted element, so it is kind of easy O:-) prev = NULL; while (qemu_get_byte(f)) { elm = g_malloc(size); ret = vmstate_load_state(f, vmsd, elm, version_id); if (ret) { error_report("%s: failed to load %s (%d)", field->name, vmsd->name, ret); g_free(elm); return ret; } if (!prev) { QLIST_RAW_INSERT_HEAD(pv, elm, entry_offset); } else { QLIST_RAW_INSERT_AFTER(prev, elm, entry_offset); } prev = elm; } And yes, I realize that there is no QLIST_RAW_INSTERT_AFTER() (it is QLIST_INSERT_AFTER). And no, I haven't took the time to understand the different between QLIST and QLIST_RAW. From a quick look, it seems that QLIST_RAW is embededed inside other structure. But as said, we can move that to another patch. Later, Juan.
Hi Juan, On 1/8/20 2:51 PM, Juan Quintela wrote: > Auger Eric <eric.auger@redhat.com> wrote: >> Hi Juan, >> >> On 1/8/20 2:19 PM, Juan Quintela wrote: >>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: >>>> * Eric Auger (eric.auger@redhat.com) wrote: >>>>> Support QLIST migration using the same principle as QTAILQ: >>>>> 94869d5c52 ("migration: migrate QTAILQ"). >>>>> >>>>> The VMSTATE_QLIST_V macro has the same proto as VMSTATE_QTAILQ_V. >>>>> The change mainly resides in QLIST RAW macros: QLIST_RAW_INSERT_HEAD >>>>> and QLIST_RAW_REVERSE. >>>>> >>>>> Tests also are provided. >>>>> >>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>>> >>>>> + while (qemu_get_byte(f)) { >>>>> + elm = g_malloc(size); >>>>> + ret = vmstate_load_state(f, vmsd, elm, version_id); >>>>> + if (ret) { >>>>> + error_report("%s: failed to load %s (%d)", field->name, >>>>> + vmsd->name, ret); >>>>> + g_free(elm); >>>>> + return ret; >>>>> + } >>>>> + QLIST_RAW_INSERT_HEAD(pv, elm, entry_offset); >>>>> + } >>>>> + QLIST_RAW_REVERSE(pv, elm, entry_offset); >>>> >>>> Can you explain why you need to do a REVERSE on the loaded list, >>>> rather than using doing a QLIST_INSERT_AFTER to always insert at >>>> the end? >>>> >>>> Other than that it looks good. >>> >>> This was my fault (integrated as this is). >>> >>> Old code had a "walk to the end of the list" and then insert. >>> I told it was way faster just to insert and the beggining and then >>> reverse. I didn't noticed that we had the previous element to know >>> where to insert. >> >> Not sure I get your comment. To insert at the end one needs to walk >> though the list. The head has no prev pointer pointing to the tail as >> opposed to the queue. So I understood Dave's comment as "just explain >> why you prefered this solution against the QLIST_INSERT_AFTER alternative. > > You have the previous inserted element, so it is kind of easy O:-) > > prev = NULL; > while (qemu_get_byte(f)) { > elm = g_malloc(size); > ret = vmstate_load_state(f, vmsd, elm, version_id); > if (ret) { > error_report("%s: failed to load %s (%d)", field->name, > vmsd->name, ret); > g_free(elm); > return ret; > } > if (!prev) { > QLIST_RAW_INSERT_HEAD(pv, elm, entry_offset); > } else { > QLIST_RAW_INSERT_AFTER(prev, elm, entry_offset); > } > prev = elm; > } > > And yes, I realize that there is no QLIST_RAW_INSTERT_AFTER() (it is > QLIST_INSERT_AFTER). And no, I haven't took the time to understand the > different between QLIST and QLIST_RAW. From a quick look, it seems that > QLIST_RAW is embededed inside other structure. Ah OK I get it now. Yes indeed that looks simpler. > > But as said, we can move that to another patch. OK. Thanks Eric > > Later, Juan. > >
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index ac4f46a67d..08683d93c6 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -227,6 +227,7 @@ extern const VMStateInfo vmstate_info_tmp; extern const VMStateInfo vmstate_info_bitmap; extern const VMStateInfo vmstate_info_qtailq; extern const VMStateInfo vmstate_info_gtree; +extern const VMStateInfo vmstate_info_qlist; #define type_check_2darray(t1,t2,n,m) ((t1(*)[n][m])0 - (t2*)0) /* @@ -796,6 +797,26 @@ extern const VMStateInfo vmstate_info_gtree; .offset = offsetof(_state, _field), \ } +/* + * For migrating a QLIST + * Target QLIST needs be properly initialized. + * _type: type of QLIST element + * _next: name of QLIST_ENTRY entry field in QLIST element + * _vmsd: VMSD for QLIST element + * size: size of QLIST element + * start: offset of QLIST_ENTRY in QTAILQ element + */ +#define VMSTATE_QLIST_V(_field, _state, _version, _vmsd, _type, _next) \ +{ \ + .name = (stringify(_field)), \ + .version_id = (_version), \ + .vmsd = &(_vmsd), \ + .size = sizeof(_type), \ + .info = &vmstate_info_qlist, \ + .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 4764d93ea3..4d4554a7ce 100644 --- a/include/qemu/queue.h +++ b/include/qemu/queue.h @@ -501,4 +501,43 @@ union { \ QTAILQ_RAW_TQH_CIRC(head)->tql_prev = QTAILQ_RAW_TQE_CIRC(elm, entry); \ } while (/*CONSTCOND*/0) +#define QLIST_RAW_FIRST(head) \ + field_at_offset(head, 0, void *) + +#define QLIST_RAW_NEXT(elm, entry) \ + field_at_offset(elm, entry, void *) + +#define QLIST_RAW_PREVIOUS(elm, entry) \ + field_at_offset(elm, entry + sizeof(void *), void *) + +#define QLIST_RAW_FOREACH(elm, head, entry) \ + for ((elm) = *QLIST_RAW_FIRST(head); \ + (elm); \ + (elm) = *QLIST_RAW_NEXT(elm, entry)) + +#define QLIST_RAW_INSERT_HEAD(head, elm, entry) do { \ + void *first = *QLIST_RAW_FIRST(head); \ + *QLIST_RAW_FIRST(head) = elm; \ + *QLIST_RAW_PREVIOUS(elm, entry) = QLIST_RAW_FIRST(head); \ + if (first) { \ + *QLIST_RAW_NEXT(elm, entry) = first; \ + *QLIST_RAW_PREVIOUS(first, entry) = QLIST_RAW_NEXT(elm, entry); \ + } else { \ + *QLIST_RAW_NEXT(elm, entry) = NULL; \ + } \ +} while (0) + +#define QLIST_RAW_REVERSE(head, elm, entry) do { \ + void *iter = *QLIST_RAW_FIRST(head), *prev = NULL, *next; \ + while (iter) { \ + next = *QLIST_RAW_NEXT(iter, entry); \ + *QLIST_RAW_PREVIOUS(iter, entry) = QLIST_RAW_NEXT(next, entry); \ + *QLIST_RAW_NEXT(iter, entry) = prev; \ + prev = iter; \ + iter = next; \ + } \ + *QLIST_RAW_FIRST(head) = prev; \ + *QLIST_RAW_PREVIOUS(prev, entry) = QLIST_RAW_FIRST(head); \ +} while (0) + #endif /* QEMU_SYS_QUEUE_H */ diff --git a/migration/trace-events b/migration/trace-events index 6dee7b5389..e0a33cffca 100644 --- a/migration/trace-events +++ b/migration/trace-events @@ -76,6 +76,11 @@ get_gtree_end(const char *field_name, const char *key_vmsd_name, const char *val put_gtree(const char *field_name, const char *key_vmsd_name, const char *val_vmsd_name, uint32_t nnodes) "%s(%s/%s) nnodes=%d" put_gtree_end(const char *field_name, const char *key_vmsd_name, const char *val_vmsd_name, int ret) "%s(%s/%s) %d" +get_qlist(const char *field_name, const char *vmsd_name, int version_id) "%s(%s v%d)" +get_qlist_end(const char *field_name, const char *vmsd_name) "%s(%s)" +put_qlist(const char *field_name, const char *vmsd_name, int version_id) "%s(%s v%d)" +put_qlist_end(const char *field_name, const char *vmsd_name) "%s(%s)" + # qemu-file.c qemu_file_fclose(void) "" diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c index 7236cf92bc..1eee36773a 100644 --- a/migration/vmstate-types.c +++ b/migration/vmstate-types.c @@ -843,3 +843,73 @@ const VMStateInfo vmstate_info_gtree = { .get = get_gtree, .put = put_gtree, }; + +static int put_qlist(QEMUFile *f, void *pv, size_t unused_size, + const 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; + int ret; + + trace_put_qlist(field->name, vmsd->name, vmsd->version_id); + QLIST_RAW_FOREACH(elm, pv, entry_offset) { + qemu_put_byte(f, true); + ret = vmstate_save_state(f, vmsd, elm, vmdesc); + if (ret) { + error_report("%s: failed to save %s (%d)", field->name, + vmsd->name, ret); + return ret; + } + } + qemu_put_byte(f, false); + trace_put_qlist_end(field->name, vmsd->name); + + return 0; +} + +static int get_qlist(QEMUFile *f, void *pv, size_t unused_size, + const VMStateField *field) +{ + int ret = 0; + const VMStateDescription *vmsd = field->vmsd; + /* size of a QLIST element */ + size_t size = field->size; + /* offset of the QLIST entry in a QLIST element */ + size_t entry_offset = field->start; + int version_id = field->version_id; + void *elm; + + trace_get_qlist(field->name, vmsd->name, vmsd->version_id); + if (version_id > vmsd->version_id) { + error_report("%s %s", vmsd->name, "too new"); + return -EINVAL; + } + if (version_id < vmsd->minimum_version_id) { + error_report("%s %s", vmsd->name, "too old"); + return -EINVAL; + } + + while (qemu_get_byte(f)) { + elm = g_malloc(size); + ret = vmstate_load_state(f, vmsd, elm, version_id); + if (ret) { + error_report("%s: failed to load %s (%d)", field->name, + vmsd->name, ret); + g_free(elm); + return ret; + } + QLIST_RAW_INSERT_HEAD(pv, elm, entry_offset); + } + QLIST_RAW_REVERSE(pv, elm, entry_offset); + trace_get_qlist_end(field->name, vmsd->name); + + return ret; +} + +const VMStateInfo vmstate_info_qlist = { + .name = "qlist", + .get = get_qlist, + .put = put_qlist, +}; diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c index 1e5be1d4ff..9660f932b9 100644 --- a/tests/test-vmstate.c +++ b/tests/test-vmstate.c @@ -926,6 +926,28 @@ static const VMStateDescription vmstate_domain = { } }; +/* test QLIST Migration */ + +typedef struct TestQListElement { + uint32_t id; + QLIST_ENTRY(TestQListElement) next; +} TestQListElement; + +typedef struct TestQListContainer { + uint32_t id; + QLIST_HEAD(, TestQListElement) list; +} TestQListContainer; + +static const VMStateDescription vmstate_qlist_element = { + .name = "test/queue list", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT32(id, TestQListElement), + VMSTATE_END_OF_LIST() + } +}; + static const VMStateDescription vmstate_iommu = { .name = "iommu", .version_id = 1, @@ -939,6 +961,18 @@ static const VMStateDescription vmstate_iommu = { } }; +static const VMStateDescription vmstate_container = { + .name = "test/container/qlist", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT32(id, TestQListContainer), + VMSTATE_QLIST_V(list, TestQListContainer, 1, vmstate_qlist_element, + TestQListElement, next), + VMSTATE_END_OF_LIST() + } +}; + uint8_t first_domain_dump[] = { /* id */ 0x00, 0x0, 0x0, 0x6, @@ -1229,6 +1263,140 @@ static void test_gtree_load_iommu(void) qemu_fclose(fload); } +static uint8_t qlist_dump[] = { + 0x00, 0x00, 0x00, 0x01, /* container id */ + 0x1, /* start of a */ + 0x00, 0x00, 0x00, 0x0a, + 0x1, /* start of b */ + 0x00, 0x00, 0x0b, 0x00, + 0x1, /* start of c */ + 0x00, 0x0c, 0x00, 0x00, + 0x1, /* start of d */ + 0x0d, 0x00, 0x00, 0x00, + 0x0, /* end of list */ + QEMU_VM_EOF, /* just to ensure we won't get EOF reported prematurely */ +}; + +static TestQListContainer *alloc_container(void) +{ + TestQListElement *a = g_malloc(sizeof(TestQListElement)); + TestQListElement *b = g_malloc(sizeof(TestQListElement)); + TestQListElement *c = g_malloc(sizeof(TestQListElement)); + TestQListElement *d = g_malloc(sizeof(TestQListElement)); + TestQListContainer *container = g_malloc(sizeof(TestQListContainer)); + + a->id = 0x0a; + b->id = 0x0b00; + c->id = 0xc0000; + d->id = 0xd000000; + container->id = 1; + + QLIST_INIT(&container->list); + QLIST_INSERT_HEAD(&container->list, d, next); + QLIST_INSERT_HEAD(&container->list, c, next); + QLIST_INSERT_HEAD(&container->list, b, next); + QLIST_INSERT_HEAD(&container->list, a, next); + return container; +} + +static void free_container(TestQListContainer *container) +{ + TestQListElement *iter, *tmp; + + QLIST_FOREACH_SAFE(iter, &container->list, next, tmp) { + QLIST_REMOVE(iter, next); + g_free(iter); + } + g_free(container); +} + +static void compare_containers(TestQListContainer *c1, TestQListContainer *c2) +{ + TestQListElement *first_item_c1, *first_item_c2; + + while (!QLIST_EMPTY(&c1->list)) { + first_item_c1 = QLIST_FIRST(&c1->list); + first_item_c2 = QLIST_FIRST(&c2->list); + assert(first_item_c2); + assert(first_item_c1->id == first_item_c2->id); + QLIST_REMOVE(first_item_c1, next); + QLIST_REMOVE(first_item_c2, next); + g_free(first_item_c1); + g_free(first_item_c2); + } + assert(QLIST_EMPTY(&c2->list)); +} + +/* + * Check the prev & next fields are correct by doing list + * manipulations on the container. We will do that for both + * the source and the destination containers + */ +static void manipulate_container(TestQListContainer *c) +{ + TestQListElement *prev, *iter = QLIST_FIRST(&c->list); + TestQListElement *elem; + + elem = g_malloc(sizeof(TestQListElement)); + elem->id = 0x12; + QLIST_INSERT_AFTER(iter, elem, next); + + elem = g_malloc(sizeof(TestQListElement)); + elem->id = 0x13; + QLIST_INSERT_HEAD(&c->list, elem, next); + + while (iter) { + prev = iter; + iter = QLIST_NEXT(iter, next); + } + + elem = g_malloc(sizeof(TestQListElement)); + elem->id = 0x14; + QLIST_INSERT_BEFORE(prev, elem, next); + + elem = g_malloc(sizeof(TestQListElement)); + elem->id = 0x15; + QLIST_INSERT_AFTER(prev, elem, next); + + QLIST_REMOVE(prev, next); + g_free(prev); +} + +static void test_save_qlist(void) +{ + TestQListContainer *container = alloc_container(); + + save_vmstate(&vmstate_container, container); + compare_vmstate(qlist_dump, sizeof(qlist_dump)); + free_container(container); +} + +static void test_load_qlist(void) +{ + QEMUFile *fsave, *fload; + TestQListContainer *orig_container = alloc_container(); + TestQListContainer *dest_container = g_malloc0(sizeof(TestQListContainer)); + char eof; + + QLIST_INIT(&dest_container->list); + + fsave = open_test_file(true); + qemu_put_buffer(fsave, qlist_dump, sizeof(qlist_dump)); + g_assert(!qemu_file_get_error(fsave)); + qemu_fclose(fsave); + + fload = open_test_file(false); + vmstate_load_state(fload, &vmstate_container, dest_container, 1); + eof = qemu_get_byte(fload); + g_assert(!qemu_file_get_error(fload)); + g_assert_cmpint(eof, ==, QEMU_VM_EOF); + manipulate_container(orig_container); + manipulate_container(dest_container); + compare_containers(orig_container, dest_container); + free_container(orig_container); + free_container(dest_container); +} + typedef struct TmpTestStruct { TestStruct *parent; int64_t diff; @@ -1353,6 +1521,8 @@ int main(int argc, char **argv) g_test_add_func("/vmstate/gtree/load/loaddomain", test_gtree_load_domain); g_test_add_func("/vmstate/gtree/save/saveiommu", test_gtree_save_iommu); g_test_add_func("/vmstate/gtree/load/loadiommu", test_gtree_load_iommu); + g_test_add_func("/vmstate/qlist/save/saveqlist", test_save_qlist); + g_test_add_func("/vmstate/qlist/load/loadqlist", test_load_qlist); g_test_add_func("/vmstate/tmp_struct", test_tmp_struct); g_test_run();
Support QLIST migration using the same principle as QTAILQ: 94869d5c52 ("migration: migrate QTAILQ"). The VMSTATE_QLIST_V macro has the same proto as VMSTATE_QTAILQ_V. The change mainly resides in QLIST RAW macros: QLIST_RAW_INSERT_HEAD and QLIST_RAW_REVERSE. Tests also are provided. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- v5 - v6: - by doing more advanced testing with virtio-iommu migration I noticed this was broken. "prev" field was not set properly. I improved the tests to manipulate both the next and prev fields. - Removed Peter and Juan's R-b --- include/migration/vmstate.h | 21 +++++ include/qemu/queue.h | 39 +++++++++ migration/trace-events | 5 ++ migration/vmstate-types.c | 70 +++++++++++++++ tests/test-vmstate.c | 170 ++++++++++++++++++++++++++++++++++++ 5 files changed, 305 insertions(+)