Re: [PATCH v3] migration: Support gtree migration

[Auger Eric]

Hi Peter,

On 10/9/19 8:28 AM, Peter Xu wrote:
> On Fri, Oct 04, 2019 at 01:20:25PM +0200, Eric Auger wrote:
>> Introduce support for GTree migration. A custom save/restore
>> is implemented. Each item is made of a key and a data.
>>
>> If the key is a pointer to an object, 2 VMSDs 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 must be handled
>> beforehand, for example in a pre_load method.
>>
>> Tests are added to test save/dump of structs containing gtrees
>> including the virtio-iommu domain/mappings scenario.
>>
>> Signed-off-by: Eric Auger <address@hidden>
> 
> Mostly looks sane to me (with Juan's comment fixed).  Some more
> trivial comments below.
> 
>> +/*
>> + * For migrating a GTree whose key is a pointer to _key_type and the
>> + * value, a pointer to _val_type
>> + * The target tree must have been properly initialized
>> + * _vmsd: Start address of the 2 element array containing the key vmsd
>> + *        and the data vmsd
>> + * _key_type: type of the key
>> + * _val_type: type of the value
>> + */
>> +#define VMSTATE_GTREE_V(_field, _state, _version, _vmsd,                    
>>    \
>> +                        _key_type, _val_type)                               
>>    \
>> +{                                                                           
>>    \
>> +    .name         = (stringify(_field)),                                    
>>    \
>> +    .version_id   = (_version),                                             
>>    \
>> +    .vmsd         = (_vmsd),                                                
>>    \
>> +    .info         = &vmstate_info_gtree,                                    
>>    \
>> +    .start        = sizeof(_key_type),                                      
>>    \
> 
> Nitpick: Are we reusing the "start" field to store the size just to
> avoid defining new field in VMStateField?  If so, not sure whether we
> can start to use unions to both keep VMStateField small while keep the
> code clean.  Like:
> 
>   union {
>     struct {
>       size_t key_size;
>       size_t value_size;
>     };
>     struct {
>       size_t start;
>       size_t size;
>     };
>   }
Indeed that's the usage. I don't have a strong preference. Juan, Dave,
what do you prefer? keep it as it is or introduce unions?

> 
> ?
> 
> This can of course also be done on top of this patch no matter what.
> 
> [...]
> 
>> +static gboolean put_gtree_elem(gpointer key, gpointer value, gpointer data)
>> +{
>> +    struct put_gtree_data *capsule = (struct put_gtree_data *)data;
>> +    QEMUFile *f = capsule->f;
>> +    int ret;
>> +
>> +    qemu_put_byte(f, true);
>> +
>> +    /* put the key */
>> +    if (!capsule->key_vmsd) {
>> +        qemu_put_be32(f, GPOINTER_TO_UINT(key)); /* direct key */
> 
> This is special code path for direct key case.  Can we simply define
> VMSTATE_GTREE_DIRECT_KEY_V() somehow better so that it just uses the
> VMSTATE_UINT32_V() as the key vmsd?  Then iiuc vmstate_save_state()
> could work well with that too.
if the key_vmsd is a VMSTATE_UINT32_V then I understand
vmstate_save_state(f, capsule->key_vmsd, key, capsule->vmdesc)
expects key to be a pointer to a uint32. But in that case of direct key,
it is a uint32. I don't figure out how to use vmstate_save_state in your
proposal.

> 
> Also, should we avoid using UINT in all cases?  But of course if we
> start to use VMSTATE_UINT32_V then we don't have this issue.
Depending on the clarification of above point, maybe I can rename
VMSTATE_GTREE_DIRECT_KEY_V into VMSTATE_GTREE_DIRECT_UINT_KEY_V

direct keys seem to be more common for hash tables actually.
https://developer.gnome.org/glib/stable/glib-Hash-Tables.html#g-hash-table-new-full

There are stand conversion macros to/from int, uint, size
https://developer.gnome.org/glib/stable/glib-Type-Conversion-Macros.html

Thanks

Eric

> 
> Thanks,
> 
>> +    } else {
>> +        ret = vmstate_save_state(f, capsule->key_vmsd, key, 
>> capsule->vmdesc);
>> +        if (ret) {
>> +            capsule->ret = ret;
>> +            return true;
>> +        }
>> +    }
>> +
>> +    /* put the data */
>> +    ret = vmstate_save_state(f, capsule->val_vmsd, value, capsule->vmdesc);
>> +    if (ret) {
>> +        capsule->ret = ret;
>> +        return true;
>> +    }
>> +    return false;
>> +}
>