Hi Juan,
On 10/3/19 6:14 PM, Juan Quintela wrote:
> Eric Auger <eric.au...@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.
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 <eric.au...@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.
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
