[for-5.0,v11,01/20] migration: Support QLIST migration
diff mbox series

Message ID 20191122182943.4656-2-eric.auger@redhat.com
State New
Headers show
Series
  • VIRTIO-IOMMU device
Related show

Commit Message

Auger Eric Nov. 22, 2019, 6:29 p.m. UTC
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(+)

Comments

Dr. David Alan Gilbert Nov. 27, 2019, 11:46 a.m. UTC | #1
* 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
Juan Quintela Jan. 8, 2020, 1:19 p.m. UTC | #2
"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.
Auger Eric Jan. 8, 2020, 1:40 p.m. UTC | #3
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.
> 
>
Juan Quintela Jan. 8, 2020, 1:51 p.m. UTC | #4
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.
Auger Eric Jan. 8, 2020, 2:02 p.m. UTC | #5
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.
> 
>

Patch
diff mbox series

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