[Qemu-devel] [PATCH] vmstate: Add VMSTATE_OPAQUE to save/load complex data structures

rkir--- via Qemu-devel posted 1 patch 4 years, 10 months ago
Test FreeBSD passed
Test docker-clang@ubuntu passed
Test s390x passed
Test docker-mingw@fedora passed
Test checkpatch passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190522222224.244714-1-rkir@google.com
Maintainers: "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Juan Quintela <quintela@redhat.com>
include/migration/vmstate.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
[Qemu-devel] [PATCH] vmstate: Add VMSTATE_OPAQUE to save/load complex data structures
Posted by rkir--- via Qemu-devel 4 years, 10 months ago
From: Roman Kiryanov <rkir@google.com>

VMSTATE_OPAQUE allows passing user defined functions to save
and load vmstate for cases when data structures do not fit
into int/struct/array terms.

Signed-off-by: Roman Kiryanov <rkir@google.com>
---
 include/migration/vmstate.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 9224370ed5..2736daef17 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -737,6 +737,19 @@ extern const VMStateInfo vmstate_info_qtailq;
     .start        = offsetof(_type, _next),                              \
 }
 
+/* Provides a way to save/load complex data structures that do not
+ * fit into int/struct/array terms.
+ * _info: a user defined instance of VMStateInfo to handle saving and loading.
+ */
+#define VMSTATE_OPAQUE(_name, _version, _info) {                      \
+    .name         = _name,                                            \
+    .version_id   = (_version),                                       \
+    .size         = 0,                                                \
+    .info         = &(_info),                                         \
+    .flags        = VMS_SINGLE,                                       \
+    .offset       = 0,                                                \
+}
+
 /* _f : field name
    _f_n : num of elements field_name
    _n : num of elements
-- 
2.21.0.1020.gf2820cf01a-goog


Re: [Qemu-devel] [PATCH] vmstate: Add VMSTATE_OPAQUE to save/load complex data structures
Posted by Dr. David Alan Gilbert 4 years, 10 months ago
* rkir--- via Qemu-devel (qemu-devel@nongnu.org) wrote:
> From: Roman Kiryanov <rkir@google.com>
> 
> VMSTATE_OPAQUE allows passing user defined functions to save
> and load vmstate for cases when data structures do not fit
> into int/struct/array terms.
> 
> Signed-off-by: Roman Kiryanov <rkir@google.com>

Hi Roman,
  Thanks for the patch.

Can you give me an example of where you would use it?  I've been
trying to get rid as many of the open-coded cases as possible
and try and make sure vmstate can handle it.

Having said that;  would it be easier to pass the get/put functions
rather than the info ?

Dave

> ---
>  include/migration/vmstate.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 9224370ed5..2736daef17 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -737,6 +737,19 @@ extern const VMStateInfo vmstate_info_qtailq;
>      .start        = offsetof(_type, _next),                              \
>  }
>  
> +/* Provides a way to save/load complex data structures that do not
> + * fit into int/struct/array terms.
> + * _info: a user defined instance of VMStateInfo to handle saving and loading.
> + */
> +#define VMSTATE_OPAQUE(_name, _version, _info) {                      \
> +    .name         = _name,                                            \
> +    .version_id   = (_version),                                       \
> +    .size         = 0,                                                \
> +    .info         = &(_info),                                         \
> +    .flags        = VMS_SINGLE,                                       \
> +    .offset       = 0,                                                \
> +}
> +
>  /* _f : field name
>     _f_n : num of elements field_name
>     _n : num of elements
> -- 
> 2.21.0.1020.gf2820cf01a-goog
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] vmstate: Add VMSTATE_OPAQUE to save/load complex data structures
Posted by Roman Kiryanov via Qemu-devel 4 years, 10 months ago
Hi Dave, thank you for looking.

> Can you give me an example of where you would use it?

We use it in our host memory sharing device. I used the existing
macros for all fields I could, but unfortunately some state does not fit
into them. We use this new macro to save/load memory
allocators (for now we have malloc, but we are working on adding
Vulkan calls). For now the state looks this way:

class Allocator;
unordered_map<int, Allocator *> state;

class MallocAllocator: public Allocator {
    unordered_map<int, vector<char>> state;
};

class VulkanAllocator: public Allocator {
    // TBD
};

>  I've been trying to get rid as many of the open-coded cases as possible

I saw this in the migration document. I used VMSTATE_INT32,
VMSTATE_STRUCT and VMSTATE_STRUCT_VARRAY_ALLOC
where I could.

> Having said that;  would it be easier to pass the get/put functions
> rather than the info ?

Sure, function pointer will be even better, but since VMStateField
takes "const VMStateInfo *", I added this way.

Regards,
Roman.

Re: [Qemu-devel] [PATCH] vmstate: Add VMSTATE_OPAQUE to save/load complex data structures
Posted by Peter Maydell 4 years, 10 months ago
On Thu, 23 May 2019 at 18:02, Roman Kiryanov via Qemu-devel
<qemu-devel@nongnu.org> wrote:
>
> Hi Dave, thank you for looking.
>
> > Can you give me an example of where you would use it?
>
> We use it in our host memory sharing device. I used the existing
> macros for all fields I could, but unfortunately some state does not fit
> into them. We use this new macro to save/load memory
> allocators (for now we have malloc, but we are working on adding
> Vulkan calls). For now the state looks this way:
>
> class Allocator;
> unordered_map<int, Allocator *> state;
>
> class MallocAllocator: public Allocator {
>     unordered_map<int, vector<char>> state;
> };
>
> class VulkanAllocator: public Allocator {
>     // TBD
> };

This is all C++, so it's not relevant for upstream QEMU...

In any case, migration state on the wire needs to be
architecture/endianness/implementation-independent,
so you can't just send raw complex data structures -- you
need to marshal these into something else (by having
pre-load/post-save functions in whatever device is using
these to marshal between the complex data structures and a
plain-old-array-of-whatevers, or by having the migration
code specifically support migration of the complex data
structures.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] vmstate: Add VMSTATE_OPAQUE to save/load complex data structures
Posted by Roman Kiryanov via Qemu-devel 4 years, 10 months ago
Hi Peter,

> In any case, migration state on the wire needs to be
> architecture/endianness/

Could you please point how the proposed change is
architecture/endianness/ dependent?

> implementation-independent,

Could you please elaborate, what "implementation"
you mean here?

> so you can't just send raw complex data structures

Do we need to serialize (in pre_save and then release in
post_save) our state into a buffer and to write it as one
piece using the existing macro? This looks ok, but how
is this different from what we are doing?

Regards,
Roman.

Re: [Qemu-devel] [PATCH] vmstate: Add VMSTATE_OPAQUE to save/load complex data structures
Posted by Peter Maydell 4 years, 10 months ago
On Fri, 24 May 2019 at 18:00, Roman Kiryanov <rkir@google.com> wrote:
>
> Hi Peter,
>
> > In any case, migration state on the wire needs to be
> > architecture/endianness/
>
> Could you please point how the proposed change is
> architecture/endianness/ dependent?

That's really hard to say, because this patch doesn't
come with any example of its use. So I'm basically
guessing that when you say "load/save complex data
structures" and call your macro OPAQUE that you mean
"I am going to just feed the raw in-memory representation
of this data structure into the migration stream in an
opaque way". Perhaps my assumptions here are wrong ?

> > implementation-independent,
>
> Could you please elaborate, what "implementation"
> you mean here?

I had in mind the C++ implementation of unordered_map<>.

> > so you can't just send raw complex data structures
>
> Do we need to serialize (in pre_save and then release in
> post_save) our state into a buffer and to write it as one
> piece using the existing macro? This looks ok, but how
> is this different from what we are doing?

I guess essentially what I'm asking for here is that
this patch comes as part of a series which makes
use of it. It's really hard to evaluate the design
of a utility feature without a concrete example of
how it's being used. That then makes it easier to understand
the abstract feature and also allows us to sometimes
suggest better ways to achieve the underlying aim
(and to avoid making suggestions which don't make sense!)

A corollary of this is that in general we don't like to
take patches upstream that implement facilities that don't
have a use upstream. Is there some existing vmstate
handling in upstream QEMU that we could refactor to
be more cleanly implemented using this?

thanks
-- PMM