Re: [PATCH v2] migration: Support gtree migration

[Auger Eric]

Hi Juan,

On 10/3/19 6:14 PM, Juan Quintela wrote:
> Eric Auger <address@hidden> 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.
OK. I implemented your suggestion, ie. using preload and it does the
job. So I don't need that field anymore.
> 
>> 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?
agreed
> 
>> 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.
I don't have a strong opinion here. At the moment I test the
key_vmsd->field and if it is NULL this means the key is a direct one.

Otherwise we could have 2 macros, a single info, but 2 different field
names. the name would allow to know the nature of the key.
> 
> The other general "consideration" that I have is that there is neither
> of:
> - marker between elements
so we have one
> - number of elements
you don't have it
> - total size/size of elements.
you just have the size of the key and the size of the value at the
moment. I could easily add the number of nodes.
> 
> 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 <address@hidden>
> 
> 
>> 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.
yep
> 
>> 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:-)
yep
> 
>> +
>> +    /* 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.
key_vmsd->fields is used
this means you set

static const VMStateDescription vmstate_id_domain[2] = {
    {}, VMSTATE_DOMAIN /* direct key, value */
};


> 
> 
>> +    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.
yep, only the else block remains
> 
>> +    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.
adding the number of nodes should do the job
> 
> 
> Later, Juan.
> 

So I think I will respin with the following modifications:
- use preload
- introduce 2 different macros
- encode/decode & test the number of nodes

Thank you for the quick feedbacks!

Eric