Re: [PATCH v4] migration: Support gtree migration

2019-10-11 Thread Auger Eric 
Hi Juan,

On 10/11/19 12:18 PM, Juan Quintela wrote:
> 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 
> 
> I found a bug, you have to respin, go to BUG
> (here was a reviewed-by)
> 
> But, 
> 
> I didn't noticed on the 1st look
> 
>> +const VMStateDescription *key_vmsd = direct_key ? NULL : 
>> >vmsd[0];
>> +const VMStateDescription *val_vmsd = direct_key ? >vmsd[0] :
>> +  >vmsd[1];
>> +const char *key_vmsd_name = direct_key ? "direct" : key_vmsd->name;
> 
> This is ugly as hell.
> We are using a pointer to pass to mean an array, and abuse it.

I agree. My first attempt was using subsections to pass the second vmsd
but it was similarly ugly.
> 
> But once told that, it is not trivial to do this in a proper way.
> 
> On the other hand, if you have to respin for any other reason, please
> consider changing the meaning of vmsd[0] and [1].
> 
> vmsd[0] -> val_t
> vmsd[1] -> key_t

OK. I chose that order since usually the pair is expressed as key/value
and I found it more logical from the qemu user perspective. But I have
no strong preference.

> 
> if (vmsd[1] == NULL)
>direct_key = true;
> 
> const VMStateDescription *val_vmsd = >vmsd[0];
> const VMStateDescription *key_vmsd = >vmsd[1]
> const char *key_vmsd_name = key_vmsd ? "direct" : key_vmsd->name;
> 
> Same for get_gtree().
OK
> 
> 
>> +if (direct_key) {
>> +key = (void *)(uintptr_t)qemu_get_be64(f);
> 
> no g_malloc().
> 
>> +} else {
>> +key = g_malloc0(key_size);
>> +ret = vmstate_load_state(f, key_vmsd, key, version_id);
>> +if (ret) {
>> +error_report("%s : failed to load %s (%d)",
>> + field->name, key_vmsd->name, ret);
>> +g_free(key);
>> +return ret;
>> +}
>> +}
>> +val = g_malloc0(val_size);
>> +ret = vmstate_load_state(f, val_vmsd, val, version_id);
>> +if (ret) {
>> +error_report("%s : failed to load %s (%d)",
>> + field->name, val_vmsd->name, ret);
>> +g_free(key);
> 
> BUG: Allways free.  This need to be protected by a direct_key(), no?
ouch yes
> 
>> +g_free(val);
>> +return ret;
>> +}
>> +g_tree_insert(tree, key, val);
>> +}
>> +if (count != nnodes) {
>> +error_report("%s inconsistent stream when loading the gtree",
>> + field->name);
> 
> BUG2:  we need to return an error here, right?
yep
> 
>> +}
>> +trace_get_gtree_end(field->name, key_vmsd_name, val_vmsd->name, ret);
>> +return ret;
>> +}
>> +
> 
> Later, Juan.
> 

Thanks for the review

Eric

Re: [PATCH v4] migration: Support gtree migration

2019-10-11 Thread Juan Quintela 
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 

I found a bug, you have to respin, go to BUG
(here was a reviewed-by)

But, 

I didn't noticed on the 1st look

> +const VMStateDescription *key_vmsd = direct_key ? NULL : >vmsd[0];
> +const VMStateDescription *val_vmsd = direct_key ? >vmsd[0] :
> +  >vmsd[1];
> +const char *key_vmsd_name = direct_key ? "direct" : key_vmsd->name;

This is ugly as hell.
We are using a pointer to pass to mean an array, and abuse it.

But once told that, it is not trivial to do this in a proper way.

On the other hand, if you have to respin for any other reason, please
consider changing the meaning of vmsd[0] and [1].

vmsd[0] -> val_t
vmsd[1] -> key_t

if (vmsd[1] == NULL)
   direct_key = true;

const VMStateDescription *val_vmsd = >vmsd[0];
const VMStateDescription *key_vmsd = >vmsd[1]
const char *key_vmsd_name = key_vmsd ? "direct" : key_vmsd->name;

Same for get_gtree().


> +if (direct_key) {
> +key = (void *)(uintptr_t)qemu_get_be64(f);

no g_malloc().

> +} else {
> +key = g_malloc0(key_size);
> +ret = vmstate_load_state(f, key_vmsd, key, version_id);
> +if (ret) {
> +error_report("%s : failed to load %s (%d)",
> + field->name, key_vmsd->name, ret);
> +g_free(key);
> +return ret;
> +}
> +}
> +val = g_malloc0(val_size);
> +ret = vmstate_load_state(f, val_vmsd, val, version_id);
> +if (ret) {
> +error_report("%s : failed to load %s (%d)",
> + field->name, val_vmsd->name, ret);
> +g_free(key);

BUG: Allways free.  This need to be protected by a direct_key(), no?

> +g_free(val);
> +return ret;
> +}
> +g_tree_insert(tree, key, val);
> +}
> +if (count != nnodes) {
> +error_report("%s inconsistent stream when loading the gtree",
> + field->name);

BUG2:  we need to return an error here, right?

> +}
> +trace_get_gtree_end(field->name, key_vmsd_name, val_vmsd->name, ret);
> +return ret;
> +}
> +

Later, Juan.

Sphinx UnpicklingError (was Re: [Patchew-devel] Fwd: Re: [PATCH v4] migration: Support gtree migration)

2019-10-11 Thread Paolo Bonzini 
Peter, this UnpicklingError from Sphinx seems to be caused by building
manuals in parallel.  Can you take a look?

Thanks,

Paolo

On 11/10/19 07:20, Auger Eric wrote:
> Hi,
> 
> This failure looks unrelated to this series; and the above script
> succeeds on my side.
> 
> Best Regards
> 
> Eric
> 
> 
>  Forwarded Message ----
> Subject: Re: [PATCH v4] migration: Support gtree migration
> Date: Fri, 11 Oct 2019 07:19:29 +0200
> From: Auger Eric 
> To: qemu-devel@nongnu.org, no-re...@patchew.org
> CC: quint...@redhat.com, pet...@redhat.com, dgilb...@redhat.com,
> eric.auger@gmail.com
> 
> Hi,
> 
> On 10/11/19 1:51 AM, no-re...@patchew.org wrote:
>> Patchew URL: 
>> https://patchew.org/QEMU/20191010205242.711-1-eric.au...@redhat.com/
>>
>>
>>
>> Hi,
>>
>> This series failed the docker-mingw@fedora build test. Please find the 
>> testing commands and
>> their output below. If you have Docker installed, you can probably reproduce 
>> it
>> locally.
>>
>> === TEST SCRIPT BEGIN ===
>> #! /bin/bash
>> export ARCH=x86_64
>> make docker-image-fedora V=1 NETWORK=1
>> time make docker-test-mingw@fedora J=14 NETWORK=1
>> === TEST SCRIPT END ===
>>
>>   CC  block/nbd.o
>>   CC  block/sheepdog.o
>>   CC  block/accounting.o
>> make: *** [Makefile:994: docs/interop/index.html] Error 2
> 
> 
>> make: *** Deleting file 'docs/interop/index.html'
>> make: *** Waiting for unfinished jobs
>> Traceback (most recent call last):
>> ---
>> raise CalledProcessError(retcode, cmd)
>> subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
>> '--label', 'com.qemu.instance.uuid=b0665a6b133e47c781238452d53aabf1', '-u', 
>> '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 
>> 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', 
>> '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
>> '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
>> '/var/tmp/patchew-tester-tmp-f9u21j2v/src/docker-src.2019-10-10-19.48.45.2708:/var/tmp/qemu:z,ro',
>>  'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit 
>> status 2.
>> filter=--filter=label=com.qemu.instance.uuid=b0665a6b133e47c781238452d53aabf1
>> make[1]: *** [docker-run] Error 1
>> make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-f9u21j2v/src'
>> make: *** [docker-run-test-mingw@fedora] Error 2
>>
>> real2m25.181s
>> user0m7.455s
>>
>>
>> The full log is available at
>> http://patchew.org/logs/20191010205242.711-1-eric.au...@redhat.com/testing.docker-mingw@fedora/?type=message.
>> ---
>> Email generated automatically by Patchew [https://patchew.org/].
>> Please send your feedback to patchew-de...@redhat.com
>>
> 
> ___
> Patchew-devel mailing list
> patchew-de...@redhat.com
> https://www.redhat.com/mailman/listinfo/patchew-devel
>

Re: [PATCH v4] migration: Support gtree migration

2019-10-10 Thread Peter Xu 
On Thu, Oct 10, 2019 at 10:52:42PM +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 

Reviewed-by: Peter Xu 

-- 
Peter Xu

Re: [PATCH v4] migration: Support gtree migration

2019-10-10 Thread Auger Eric 
Hi,

On 10/11/19 1:51 AM, no-re...@patchew.org wrote:
> Patchew URL: 
> https://patchew.org/QEMU/20191010205242.711-1-eric.au...@redhat.com/
> 
> 
> 
> Hi,
> 
> This series failed the docker-mingw@fedora build test. Please find the 
> testing commands and
> their output below. If you have Docker installed, you can probably reproduce 
> it
> locally.
> 
> === TEST SCRIPT BEGIN ===
> #! /bin/bash
> export ARCH=x86_64
> make docker-image-fedora V=1 NETWORK=1
> time make docker-test-mingw@fedora J=14 NETWORK=1
> === TEST SCRIPT END ===
> 
>   CC  block/nbd.o
>   CC  block/sheepdog.o
>   CC  block/accounting.o
> make: *** [Makefile:994: docs/interop/index.html] Error 2

This failure looks unrelated to this series; and the above script
succeeds on my side.

Best Regards

Eric
> make: *** Deleting file 'docs/interop/index.html'
> make: *** Waiting for unfinished jobs
> Traceback (most recent call last):
> ---
> raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
> '--label', 'com.qemu.instance.uuid=b0665a6b133e47c781238452d53aabf1', '-u', 
> '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
> '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', 
> '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
> '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
> '/var/tmp/patchew-tester-tmp-f9u21j2v/src/docker-src.2019-10-10-19.48.45.2708:/var/tmp/qemu:z,ro',
>  'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit 
> status 2.
> filter=--filter=label=com.qemu.instance.uuid=b0665a6b133e47c781238452d53aabf1
> make[1]: *** [docker-run] Error 1
> make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-f9u21j2v/src'
> make: *** [docker-run-test-mingw@fedora] Error 2
> 
> real2m25.181s
> user0m7.455s
> 
> 
> The full log is available at
> http://patchew.org/logs/20191010205242.711-1-eric.au...@redhat.com/testing.docker-mingw@fedora/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-de...@redhat.com
>

Re: [PATCH v4] migration: Support gtree migration

2019-10-10 Thread no-reply 
Patchew URL: 
https://patchew.org/QEMU/20191010205242.711-1-eric.au...@redhat.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing 
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC  block/nbd.o
  CC  block/sheepdog.o
  CC  block/accounting.o
make: *** [Makefile:994: docs/interop/index.html] Error 2
make: *** Deleting file 'docs/interop/index.html'
make: *** Waiting for unfinished jobs
Traceback (most recent call last):
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=b0665a6b133e47c781238452d53aabf1', '-u', 
'1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
'-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 
'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
'/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
'/var/tmp/patchew-tester-tmp-f9u21j2v/src/docker-src.2019-10-10-19.48.45.2708:/var/tmp/qemu:z,ro',
 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit 
status 2.
filter=--filter=label=com.qemu.instance.uuid=b0665a6b133e47c781238452d53aabf1
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-f9u21j2v/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real2m25.181s
user0m7.455s


The full log is available at
http://patchew.org/logs/20191010205242.711-1-eric.au...@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

[PATCH v4] migration: Support gtree migration

2019-10-10 Thread Eric Auger 
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 

---

v3 -> v4:
- properly initialize capsule.vmdesc in put_gtree()
- use uintptr_t
- add trace points
- I did not use unions in VMStateField as suggested by Peter as
  this may complexify the struct. This can be added later on
  though.

v2 -> v3:
- do not include glib.h anymore
- introduce VMSTATE_GTREE_DIRECT_KEY_V
- use pre_load to allocate the tree and remove data pointer
- dump the number of nnodes and add checks on get path

v1 -> v2:
- fix compilation issues reported by robots
- fixed init of VMSD array
- direct key now dumped as a 32b
- removed useless cast from/to pointer
---
 include/migration/vmstate.h |  40 
 migration/trace-events  |   5 +
 migration/vmstate-types.c   | 148 +
 tests/test-vmstate.c| 421 
 4 files changed, 614 insertions(+)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 1fbfd099dd..78caff55ef 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -224,6 +224,7 @@ extern const VMStateInfo vmstate_info_unused_buffer;
 extern const VMStateInfo vmstate_info_tmp;
 extern const VMStateInfo vmstate_info_bitmap;
 extern const VMStateInfo vmstate_info_qtailq;
+extern const VMStateInfo vmstate_info_gtree;
 
 #define type_check_2darray(t1,t2,n,m) ((t1(*)[n][m])0 - (t2*)0)
 /*
@@ -754,6 +755,45 @@ extern const VMStateInfo vmstate_info_qtailq;
 .start= offsetof(_type, _next),  \
 }
 
+/*
+ * 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 = _info_gtree,   
\
+.start= sizeof(_key_type), 
\
+.size = sizeof(_val_type), 
\
+.offset   = offsetof(_state, _field),  
\
+}
+
+/*
+ * For migrating a GTree with direct key and the value a pointer
+ * to _val_type
+ * The target tree must have been properly initialized
+ * _vmsd: data vmsd
+ * _val_type: type of the value
+ */
+#define VMSTATE_GTREE_DIRECT_KEY_V(_field, _state, _version, _vmsd, _val_type) 
\
+{  
\
+.name = (stringify(_field)),   
\
+.version_id   = (_version),
\
+.vmsd = (_vmsd),   
\
+.info = _info_gtree,   
\
+.start= 0, 
\
+.size = sizeof(_val_type), 
\
+.offset   = offsetof(_state, _field),  
\
+}
+
 /* _f : field name
_f_n : num of elements field_name
_n : num of elements
diff --git a/migration/trace-events b/migration/trace-events
index 858d415d56..6dee7b5389 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -71,6 +71,11 @@ get_qtailq_end(const char *name, const char *reason, int 
val) "%s %s/%d"
 put_qtailq(const char *name, int version_id) "%s v%d"
 put_qtailq_end(const char *name, const char *reason) "%s %s"
 
+get_gtree(const char *field_name, const char *key_vmsd_name, const char 
*val_vmsd_name, uint32_t nnodes) "%s(%s/%s) nnodes=%d"
+get_gtree_end(const char *field_name, const char *key_vmsd_name, const char 
*val_vmsd_name, int ret) "%s(%s/%s) %d"
+put_gtree(const char *field_name, const char *key_vmsd_name, const char 
*val_vmsd_name, uint32_t nnodes)