[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3] migration: Support gtree migration
From: |
Auger Eric |
Subject: |
Re: [PATCH v3] migration: Support gtree migration |
Date: |
Thu, 10 Oct 2019 09:57:01 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
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;
>> +}
>
- [PATCH v3] migration: Support gtree migration, Eric Auger, 2019/10/04
- Re: [PATCH v3] migration: Support gtree migration, Juan Quintela, 2019/10/04
- Re: [PATCH v3] migration: Support gtree migration, Peter Xu, 2019/10/09
- Re: [PATCH v3] migration: Support gtree migration,
Auger Eric <=
- Re: [PATCH v3] migration: Support gtree migration, Peter Xu, 2019/10/10
- Re: [PATCH v3] migration: Support gtree migration, Auger Eric, 2019/10/10
- Re: [PATCH v3] migration: Support gtree migration, Peter Xu, 2019/10/10
- Re: [PATCH v3] migration: Support gtree migration, Auger Eric, 2019/10/10
- Re: [PATCH v3] migration: Support gtree migration, Dr. David Alan Gilbert, 2019/10/10
- Re: [PATCH v3] migration: Support gtree migration, Auger Eric, 2019/10/10
Re: [PATCH v3] migration: Support gtree migration, Dr. David Alan Gilbert, 2019/10/09