Re: [PATCH v2] migration: Support gtree migration

[Juan Quintela <quintela@redhat.com>]

Eric Auger <eric.auger@redhat.com> wrote:
> Introduce support for GTree migration. A custom save/restore
> is implemented. Each item is made of a key and a data. For that
> reason, 2 VMSD objects are passed into the GTree VMStateField.
>
> When putting the items, the tree is traversed in sorted order by
> g_tree_foreach.
>
> On the get() path, gtrees must be allocated using the proper
> key compare, key destroy and value destroy. This can be done
> externally of automatically. If done automatically, the set of
             ^^

or.

> functions must be stored within the VMStateField in a new opaque
> pointer.

I am not fully convinced that the automatic mode is needed.  Especially
the ->data field.  I *fear* it being abused for other cases.

> Automatic allocation is needed for complex state save/restore.
> For instance the virtio-iommu uses a gtree of domain and each
> domain has a gtree of mappings.

There is a pre_load() function for the VMState that creates this.  it
can be used to initualize the field value, no?  That way the data part
is not needed.  I think this will make the code less complex, what do
you think?

> Special care was taken about direct key (ie. when the key is not
> a pointer to an object but is directly a value).

I am wondering if for this, it is better to add two VMSTATE (at least at
the macro level).  SIMPLE_TREE, and TREE, or whataver oyou want to call
it.  But I haven't fully looked into it.

The other general "consideration" that I have is that there is neither
of:
- marker between elements
- number of elements
- total size/size of elements.

That makes completelly impractical to "walk" the migration stream
without understanding exactyl what is there.  (Now, to be fair, there
are other parts of qemu that are like that.  PCI cames to mind.)

> Tests are added to test save/dump of structs containing gtrees
> including the virtio-iommu domain/mappings scenario.

Really nice to have the tests.  Thanks.

> Signed-off-by: Eric Auger <eric.auger@redhat.com>


> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 1fbfd099dd..4d9698eaab 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -171,6 +171,7 @@ struct VMStateField {
>      int version_id;
>      int struct_version_id;
>      bool (*field_exists)(void *opaque, int version_id);
> +    void *data;
>  };

This is the bit that I don't really like :p

>  
> +typedef struct GTreeInitData {
> +    GCompareDataFunc key_compare_func;
> +    gpointer key_compare_data;
> +    GDestroyNotify key_destroy_func;
> +    GDestroyNotify value_destroy_func;
> +} GTreeInitData;

My understanding is that if you do this on the pre_load() function, you
don't need this at all.

> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index bee658a1b2..06c4663de6 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -17,6 +17,7 @@
>  #include "qemu/error-report.h"
>  #include "qemu/queue.h"
>  #include "trace.h"
> +#include <glib.h>
>  
>  /* bool */
>  
> @@ -691,3 +692,135 @@ const VMStateInfo vmstate_info_qtailq = {
>      .get  = get_qtailq,
>      .put  = put_qtailq,
>  };
> +
> +struct put_gtree_data {
> +    QEMUFile *f;
> +    const VMStateField *field;
> +    QJSON *vmdesc;
> +};
> +
> +static gboolean put_gtree_elem(gpointer key, gpointer value, gpointer data)
> +{
> +    struct put_gtree_data *capsule = (struct put_gtree_data *)data;
> +    const VMStateField *field = capsule->field;
> +    QEMUFile *f = capsule->f;
> +    const VMStateDescription *key_vmsd = &field->vmsd[0];
> +    const VMStateDescription *data_vmsd = &field->vmsd[1];
> +
> +    qemu_put_byte(f, true);

Ok.  there is a marker O:-)

> +
> +    /* put the key */
> +    if (!key_vmsd->fields) {
> +        qemu_put_be32(f, GPOINTER_TO_UINT(key));
> +    } else {
> +        if (vmstate_save_state(f, key_vmsd, key, capsule->vmdesc)) {
> +            return true;
> +        }
> +    }

But it is magic to know if it is a simple or complex key.


> +    if (field->data) {
> +        init_data = (GTreeInitData *)field->data;
> +        tree = g_tree_new_full(init_data->key_compare_func,
> +                               init_data->key_compare_data,
> +                               init_data->key_destroy_func,
> +                               init_data->value_destroy_func);
> +        *pval = tree;
> +    } else {
> +        /* tree is externally allocated */
> +        tree = *pval;
> +    }

This can be simplified while we are at it.

> +    while (qemu_get_byte(f)) {

If we get out of sync, for any reason, we have no way to recover.  The
only way to recover is that we don't get a "false" in this position.


Later, Juan.