[Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Peter Xu]

Currently, dump-guest-memory supports synchronous operation only. This patch
sets are adding "detach" support for it (just like "migrate -d" for
migration). When "-d" is provided, dump-guest-memory command will return
immediately without hanging user. This should be useful when the backend
storage for the dump file is very slow.

Peter Xu (2):
  dump-guest-memory: add "detach" flag for QMP/HMP interfaces
  dump-guest-memory: add basic "detach" support.

 dump.c                | 62 ++++++++++++++++++++++++++++++++++++++++++++++-----
 hmp-commands.hx       |  5 +++--
 hmp.c                 |  3 ++-
 include/sysemu/dump.h |  4 ++++
 qapi-schema.json      |  3 ++-
 qmp-commands.hx       |  4 ++--
 qmp.c                 |  9 ++++++++
 vl.c                  |  3 +++
 8 files changed, 81 insertions(+), 12 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH REPOST 1/2] dump-guest-memory: add "detach" flag for QMP/HMP interfaces

[Peter Xu]

This patch only add the interfaces, but not implementing them.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 dump.c           | 3 ++-
 hmp-commands.hx  | 5 +++--
 hmp.c            | 3 ++-
 qapi-schema.json | 3 ++-
 qmp-commands.hx  | 4 ++--
 5 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/dump.c b/dump.c
index 78b7d84..3ec3423 100644
--- a/dump.c
+++ b/dump.c
@@ -1598,7 +1598,8 @@ cleanup:
 void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
                            int64_t begin, bool has_length,
                            int64_t length, bool has_format,
-                           DumpGuestMemoryFormat format, Error **errp)
+                           DumpGuestMemoryFormat format, bool detach,
+                           Error **errp)
 {
     const char *p;
     int fd = -1;
diff --git a/hmp-commands.hx b/hmp-commands.hx
index bb52e4d..8e27671 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1056,10 +1056,11 @@ ETEXI
 
     {
         .name       = "dump-guest-memory",
-        .args_type  = "paging:-p,zlib:-z,lzo:-l,snappy:-s,filename:F,begin:i?,length:i?",
-        .params     = "[-p] [-z|-l|-s] filename [begin length]",
+        .args_type  = "paging:-p,detach:-d,zlib:-z,lzo:-l,snappy:-s,filename:F,begin:i?,length:i?",
+        .params     = "[-p] [-d] [-z|-l|-s] filename [begin length]",
         .help       = "dump guest memory into file 'filename'.\n\t\t\t"
                       "-p: do paging to get guest's memory mapping.\n\t\t\t"
+                      "-d: return immediately (not wait for completion).\n\t\t\t"
                       "-z: dump in kdump-compressed format, with zlib compression.\n\t\t\t"
                       "-l: dump in kdump-compressed format, with lzo compression.\n\t\t\t"
                       "-s: dump in kdump-compressed format, with snappy compression.\n\t\t\t"
diff --git a/hmp.c b/hmp.c
index 2140605..4c5480d 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1580,6 +1580,7 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
     bool paging = qdict_get_try_bool(qdict, "paging", false);
+    bool detach = qdict_get_try_bool(qdict, "detach", false);
     bool zlib = qdict_get_try_bool(qdict, "zlib", false);
     bool lzo = qdict_get_try_bool(qdict, "lzo", false);
     bool snappy = qdict_get_try_bool(qdict, "snappy", false);
@@ -1619,7 +1620,7 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
     prot = g_strconcat("file:", file, NULL);
 
     qmp_dump_guest_memory(paging, prot, has_begin, begin, has_length, length,
-                          true, dump_format, &err);
+                          true, dump_format, detach, &err);
     hmp_handle_error(mon, &err);
     g_free(prot);
 }
diff --git a/qapi-schema.json b/qapi-schema.json
index 8b1a423..1211082 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2132,7 +2132,8 @@
 ##
 { 'command': 'dump-guest-memory',
   'data': { 'paging': 'bool', 'protocol': 'str', '*begin': 'int',
-            '*length': 'int', '*format': 'DumpGuestMemoryFormat' } }
+            '*length': 'int', '*format': 'DumpGuestMemoryFormat',
+            'detach': 'bool'} }
 
 ##
 # @DumpGuestMemoryCapability:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 9d8b42f..86864f6 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -840,8 +840,8 @@ EQMP
 
     {
         .name       = "dump-guest-memory",
-        .args_type  = "paging:b,protocol:s,begin:i?,end:i?,format:s?",
-        .params     = "-p protocol [begin] [length] [format]",
+        .args_type  = "paging:b,detach:d,protocol:s,begin:i?,end:i?,format:s?",
+        .params     = "-p protocol [-d] [begin] [length] [format]",
         .help       = "dump guest memory to file",
         .mhandler.cmd_new = qmp_marshal_dump_guest_memory,
     },
-- 
2.4.3

Re: [Qemu-devel] [PATCH REPOST 1/2] dump-guest-memory: add "detach" flag for QMP/HMP interfaces

[Andrew Jones]

On Mon, Nov 23, 2015 at 06:07:41PM +0800, Peter Xu wrote:
> This patch only add the interfaces, but not implementing them.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  dump.c           | 3 ++-
>  hmp-commands.hx  | 5 +++--
>  hmp.c            | 3 ++-
>  qapi-schema.json | 3 ++-
>  qmp-commands.hx  | 4 ++--
>  5 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index 78b7d84..3ec3423 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -1598,7 +1598,8 @@ cleanup:
>  void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
>                             int64_t begin, bool has_length,
>                             int64_t length, bool has_format,
> -                           DumpGuestMemoryFormat format, Error **errp)
> +                           DumpGuestMemoryFormat format, bool detach,
> +                           Error **errp)
>  {
>      const char *p;
>      int fd = -1;
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index bb52e4d..8e27671 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1056,10 +1056,11 @@ ETEXI
>  
>      {
>          .name       = "dump-guest-memory",
> -        .args_type  = "paging:-p,zlib:-z,lzo:-l,snappy:-s,filename:F,begin:i?,length:i?",
> -        .params     = "[-p] [-z|-l|-s] filename [begin length]",
> +        .args_type  = "paging:-p,detach:-d,zlib:-z,lzo:-l,snappy:-s,filename:F,begin:i?,length:i?",
> +        .params     = "[-p] [-d] [-z|-l|-s] filename [begin length]",
>          .help       = "dump guest memory into file 'filename'.\n\t\t\t"
>                        "-p: do paging to get guest's memory mapping.\n\t\t\t"
> +                      "-d: return immediately (not wait for completion).\n\t\t\t"

(do not wait...)

>                        "-z: dump in kdump-compressed format, with zlib compression.\n\t\t\t"
>                        "-l: dump in kdump-compressed format, with lzo compression.\n\t\t\t"
>                        "-s: dump in kdump-compressed format, with snappy compression.\n\t\t\t"
> diff --git a/hmp.c b/hmp.c
> index 2140605..4c5480d 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1580,6 +1580,7 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
>      bool paging = qdict_get_try_bool(qdict, "paging", false);
> +    bool detach = qdict_get_try_bool(qdict, "detach", false);
>      bool zlib = qdict_get_try_bool(qdict, "zlib", false);
>      bool lzo = qdict_get_try_bool(qdict, "lzo", false);
>      bool snappy = qdict_get_try_bool(qdict, "snappy", false);
> @@ -1619,7 +1620,7 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
>      prot = g_strconcat("file:", file, NULL);
>  
>      qmp_dump_guest_memory(paging, prot, has_begin, begin, has_length, length,
> -                          true, dump_format, &err);
> +                          true, dump_format, detach, &err);
>      hmp_handle_error(mon, &err);
>      g_free(prot);
>  }
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 8b1a423..1211082 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2132,7 +2132,8 @@
>  ##
>  { 'command': 'dump-guest-memory',
>    'data': { 'paging': 'bool', 'protocol': 'str', '*begin': 'int',
> -            '*length': 'int', '*format': 'DumpGuestMemoryFormat' } }
> +            '*length': 'int', '*format': 'DumpGuestMemoryFormat',
> +            'detach': 'bool'} }

For consistency I would put detach after paging.

>  
>  ##
>  # @DumpGuestMemoryCapability:
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 9d8b42f..86864f6 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -840,8 +840,8 @@ EQMP
>  
>      {
>          .name       = "dump-guest-memory",
> -        .args_type  = "paging:b,protocol:s,begin:i?,end:i?,format:s?",
> -        .params     = "-p protocol [begin] [length] [format]",
> +        .args_type  = "paging:b,detach:d,protocol:s,begin:i?,end:i?,format:s?",
> +        .params     = "-p protocol [-d] [begin] [length] [format]",

shouldn't this be -p -d protocol [begin] [length] [format]

>          .help       = "dump guest memory to file",
>          .mhandler.cmd_new = qmp_marshal_dump_guest_memory,
>      },
> -- 
> 2.4.3
> 
> 

Re: [Qemu-devel] [PATCH REPOST 1/2] dump-guest-memory: add "detach" flag for QMP/HMP interfaces

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 939 bytes --]

On 11/23/2015 03:07 AM, Peter Xu wrote:
> This patch only add the interfaces, but not implementing them.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  dump.c           | 3 ++-
>  hmp-commands.hx  | 5 +++--
>  hmp.c            | 3 ++-
>  qapi-schema.json | 3 ++-
>  qmp-commands.hx  | 4 ++--
>  5 files changed, 11 insertions(+), 7 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -2132,7 +2132,8 @@
>  ##
>  { 'command': 'dump-guest-memory',
>    'data': { 'paging': 'bool', 'protocol': 'str', '*begin': 'int',
> -            '*length': 'int', '*format': 'DumpGuestMemoryFormat' } }
> +            '*length': 'int', '*format': 'DumpGuestMemoryFormat',
> +            'detach': 'bool'} }

Missing documentation of the new flag, of its default state, and with a
mention that it is '(since 2.6)'.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH REPOST 1/2] dump-guest-memory: add "detach" flag for QMP/HMP interfaces

[Fam Zheng]

On Mon, 11/23 09:10, Eric Blake wrote:
> On 11/23/2015 03:07 AM, Peter Xu wrote:
> > This patch only add the interfaces, but not implementing them.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  dump.c           | 3 ++-
> >  hmp-commands.hx  | 5 +++--
> >  hmp.c            | 3 ++-
> >  qapi-schema.json | 3 ++-
> >  qmp-commands.hx  | 4 ++--
> >  5 files changed, 11 insertions(+), 7 deletions(-)
> > 
> 
> > +++ b/qapi-schema.json
> > @@ -2132,7 +2132,8 @@
> >  ##
> >  { 'command': 'dump-guest-memory',
> >    'data': { 'paging': 'bool', 'protocol': 'str', '*begin': 'int',
> > -            '*length': 'int', '*format': 'DumpGuestMemoryFormat' } }
> > +            '*length': 'int', '*format': 'DumpGuestMemoryFormat',
> > +            'detach': 'bool'} }
> 
> Missing documentation of the new flag, of its default state, and with a
> mention that it is '(since 2.6)'.
> 

Yes, also new flags must be added as optional to keep backward-compatibility.

Fam

[Qemu-devel] [PATCH REPOST 2/2] dump-guest-memory: add basic "detach" support.

[Peter Xu]

This will allow the user specify "-d" (just like command
"migrate") when using "dump-guest-memory" command. When
specified, one background thread is created to do the dump work.
One flag is added to show whether there is a background dump
work in progress.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 dump.c                | 59 ++++++++++++++++++++++++++++++++++++++++++++++-----
 include/sysemu/dump.h |  4 ++++
 qmp.c                 |  9 ++++++++
 vl.c                  |  3 +++
 4 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/dump.c b/dump.c
index 3ec3423..c2e14cd 100644
--- a/dump.c
+++ b/dump.c
@@ -1442,6 +1442,9 @@ static void dump_init(DumpState *s, int fd, bool has_format,
     Error *err = NULL;
     int ret;
 
+    s->has_format = has_format;
+    s->format = format;
+
     /* kdump-compressed is conflict with paging and filter */
     if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
         assert(!paging && !has_filter);
@@ -1595,6 +1598,45 @@ cleanup:
     dump_cleanup(s);
 }
 
+/* whether there is a dump in progress */
+extern bool dump_in_progress_p;
+
+static bool dump_ownership_set(bool value)
+{
+    return atomic_xchg(&dump_in_progress_p, value);
+}
+
+/* return true when owner taken, false otherwise */
+static bool dump_ownership_take(void)
+{
+    bool ret = dump_ownership_set(1);
+    return ret == 0;
+}
+
+/* we should only call this after all dump work finished */
+static void dump_ownership_release(void)
+{
+    dump_ownership_set(0);
+}
+
+static void dump_process(DumpState *s, Error **errp)
+{
+    if (s->has_format && s->format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
+        create_kdump_vmcore(s, errp);
+    } else {
+        create_vmcore(s, errp);
+    }
+    dump_ownership_release();
+}
+
+static void *dump_thread(void *data)
+{
+    DumpState *s = (DumpState *)data;
+    dump_process(s, NULL);
+    g_free(s);
+    return NULL;
+}
+
 void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
                            int64_t begin, bool has_length,
                            int64_t length, bool has_format,
@@ -1662,6 +1704,12 @@ void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
         return;
     }
 
+    /* we could only allow one dump at a time. */
+    if (!dump_ownership_take()) {
+        error_setg(errp, "another dump is in progress.");
+        return;
+    }
+
     s = g_malloc0(sizeof(DumpState));
 
     dump_init(s, fd, has_format, format, paging, has_begin,
@@ -1672,13 +1720,14 @@ void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
         return;
     }
 
-    if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
-        create_kdump_vmcore(s, errp);
+    if (!detach) {
+        dump_process(s, errp);
+        g_free(s);
     } else {
-        create_vmcore(s, errp);
+        qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
+                           s, QEMU_THREAD_DETACHED);
+        /* DumpState is freed in dump thread */
     }
-
-    g_free(s);
 }
 
 DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 7e4ec5c..2aeffc8 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -183,6 +183,10 @@ typedef struct DumpState {
     off_t offset_page;          /* offset of page part in vmcore */
     size_t num_dumpable;        /* number of page that can be dumped */
     uint32_t flag_compress;     /* indicate the compression format */
+
+    QemuThread dump_thread;     /* only used when do async dump */
+    bool has_format;            /* whether format is provided */
+    DumpGuestMemoryFormat format; /* valid only if has_format == true */
 } DumpState;
 
 uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
diff --git a/qmp.c b/qmp.c
index 0a1fa19..ea57b57 100644
--- a/qmp.c
+++ b/qmp.c
@@ -168,12 +168,21 @@ SpiceInfo *qmp_query_spice(Error **errp)
 };
 #endif
 
+extern bool dump_in_progress_p;
+
 void qmp_cont(Error **errp)
 {
     Error *local_err = NULL;
     BlockBackend *blk;
     BlockDriverState *bs;
 
+    /* if there is a dump in background, we should wait until the dump
+     * finished */
+    if (dump_in_progress_p) {
+        error_setg(errp, "there is a dump in process, please wait.");
+        return;
+    }
+
     if (runstate_needs_reset()) {
         error_setg(errp, "Resetting the Virtual Machine is required");
         return;
diff --git a/vl.c b/vl.c
index 525929b..ef7a936 100644
--- a/vl.c
+++ b/vl.c
@@ -204,6 +204,9 @@ bool xen_allowed;
 uint32_t xen_domid;
 enum xen_mode xen_mode = XEN_EMULATE;
 
+/* whether dump is in progress */
+bool dump_in_progress_p = false;
+
 static int has_defaults = 1;
 static int default_serial = 1;
 static int default_parallel = 1;
-- 
2.4.3

Re: [Qemu-devel] [PATCH REPOST 2/2] dump-guest-memory: add basic "detach" support.

[Daniel P. Berrange]

On Mon, Nov 23, 2015 at 06:07:42PM +0800, Peter Xu wrote:
> This will allow the user specify "-d" (just like command
> "migrate") when using "dump-guest-memory" command. When
> specified, one background thread is created to do the dump work.
> One flag is added to show whether there is a background dump
> work in progress.

Your comparison to the 'migrate' command is not entirely
accurate. While the 'detach' flag exist in the QMP parameters
schema, it is invalid to use it - migration is always backgrounded
in QMP.

[quote src="qmp-commands.hx"]
  (3) The user Monitor's "detach" argument is invalid in QMP and should not
    be used
[/quote]

A further difference is that with migrate, you can use
'info migrate' to determine if the operation is complete.
AFAIK, with this proposal there's no way to see if the
dump is complete - you just have to keep runing 'cont'
until it doesn't return an error.

I'm curious about the intended usage of this change
and whether this design satisfies the requirements
it may have

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH REPOST 2/2] dump-guest-memory: add basic "detach" support.

[Andrew Jones]

On Mon, Nov 23, 2015 at 06:07:42PM +0800, Peter Xu wrote:
> This will allow the user specify "-d" (just like command
> "migrate") when using "dump-guest-memory" command. When
> specified, one background thread is created to do the dump work.
> One flag is added to show whether there is a background dump
> work in progress.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  dump.c                | 59 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  include/sysemu/dump.h |  4 ++++
>  qmp.c                 |  9 ++++++++
>  vl.c                  |  3 +++
>  4 files changed, 70 insertions(+), 5 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index 3ec3423..c2e14cd 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -1442,6 +1442,9 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>      Error *err = NULL;
>      int ret;
>  
> +    s->has_format = has_format;
> +    s->format = format;
> +
>      /* kdump-compressed is conflict with paging and filter */
>      if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
>          assert(!paging && !has_filter);
> @@ -1595,6 +1598,45 @@ cleanup:
>      dump_cleanup(s);
>  }
>  
> +/* whether there is a dump in progress */
> +extern bool dump_in_progress_p;
> +
> +static bool dump_ownership_set(bool value)
> +{
> +    return atomic_xchg(&dump_in_progress_p, value);
> +}
> +
> +/* return true when owner taken, false otherwise */
returns true when ownership is taken

> +static bool dump_ownership_take(void)
> +{
> +    bool ret = dump_ownership_set(1);
> +    return ret == 0;

return dump_ownership_set(1) == 0;

> +}
> +
> +/* we should only call this after all dump work finished */
                                                  ^has
> +static void dump_ownership_release(void)
> +{
> +    dump_ownership_set(0);
> +}
> +
> +static void dump_process(DumpState *s, Error **errp)
> +{
> +    if (s->has_format && s->format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
> +        create_kdump_vmcore(s, errp);
> +    } else {
> +        create_vmcore(s, errp);
> +    }
> +    dump_ownership_release();
> +}
> +
> +static void *dump_thread(void *data)
> +{
> +    DumpState *s = (DumpState *)data;
> +    dump_process(s, NULL);

How do errors work when errp is NULL? It looks like we won't get any.
Could we duplicate the errp we get from qmp and add it to the DumpState?
Or just use a local_err? (I know not of what I speak, this is Markus
territory.)

> +    g_free(s);
> +    return NULL;
> +}
> +
>  void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
>                             int64_t begin, bool has_length,
>                             int64_t length, bool has_format,
> @@ -1662,6 +1704,12 @@ void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
>          return;
>      }
>  
> +    /* we could only allow one dump at a time. */
s/could/can/

> +    if (!dump_ownership_take()) {
> +        error_setg(errp, "another dump is in progress.");
> +        return;
> +    }
> +
>      s = g_malloc0(sizeof(DumpState));
>  
>      dump_init(s, fd, has_format, format, paging, has_begin,
> @@ -1672,13 +1720,14 @@ void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
>          return;
>      }
>  
> -    if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
> -        create_kdump_vmcore(s, errp);
> +    if (!detach) {
> +        dump_process(s, errp);
> +        g_free(s);
>      } else {
> -        create_vmcore(s, errp);
> +        qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
> +                           s, QEMU_THREAD_DETACHED);
> +        /* DumpState is freed in dump thread */
>      }
> -
> -    g_free(s);
>  }
>  
>  DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index 7e4ec5c..2aeffc8 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -183,6 +183,10 @@ typedef struct DumpState {
>      off_t offset_page;          /* offset of page part in vmcore */
>      size_t num_dumpable;        /* number of page that can be dumped */
>      uint32_t flag_compress;     /* indicate the compression format */
> +
> +    QemuThread dump_thread;     /* only used when do async dump */
                                                     ^doing an
> +    bool has_format;            /* whether format is provided */
> +    DumpGuestMemoryFormat format; /* valid only if has_format == true */
>  } DumpState;
>  
>  uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
> diff --git a/qmp.c b/qmp.c
> index 0a1fa19..ea57b57 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -168,12 +168,21 @@ SpiceInfo *qmp_query_spice(Error **errp)
>  };
>  #endif
>  
> +extern bool dump_in_progress_p;
> +
>  void qmp_cont(Error **errp)
>  {
>      Error *local_err = NULL;
>      BlockBackend *blk;
>      BlockDriverState *bs;
>  
> +    /* if there is a dump in background, we should wait until the dump
          ^If                  ^the
> +     * finished */
          finishes
> +    if (dump_in_progress_p) {
> +        error_setg(errp, "there is a dump in process, please wait.");

If there's a period in the error message then I think there should be
capitalized. If it's supposed to be a phrase, then either no period or
three of them '...'

> +        return;
> +    }
> +
>      if (runstate_needs_reset()) {
>          error_setg(errp, "Resetting the Virtual Machine is required");
>          return;
> diff --git a/vl.c b/vl.c
> index 525929b..ef7a936 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -204,6 +204,9 @@ bool xen_allowed;
>  uint32_t xen_domid;
>  enum xen_mode xen_mode = XEN_EMULATE;
>  
> +/* whether dump is in progress */
             ^ a
> +bool dump_in_progress_p = false;
> +
>  static int has_defaults = 1;
>  static int default_serial = 1;
>  static int default_parallel = 1;
> -- 
> 2.4.3
> 
>

Thanks,
drew 

Re: [Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 755 bytes --]

On 11/23/2015 03:07 AM, Peter Xu wrote:
> Currently, dump-guest-memory supports synchronous operation only. This patch
> sets are adding "detach" support for it (just like "migrate -d" for
> migration). When "-d" is provided, dump-guest-memory command will return
> immediately without hanging user. This should be useful when the backend
> storage for the dump file is very slow.

Where is the added event that fires when the command completes?  Where
can a user poll for completion if they don't want to wait for an event?

You cannot convert a blocking command to asynchronous without some
user-visible way of tracking progress.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Laszlo Ersek]

On 11/23/15 11:07, Peter Xu wrote:
> Currently, dump-guest-memory supports synchronous operation only. This patch
> sets are adding "detach" support for it (just like "migrate -d" for
> migration). When "-d" is provided, dump-guest-memory command will return
> immediately without hanging user. This should be useful when the backend
> storage for the dump file is very slow.
> 
> Peter Xu (2):
>   dump-guest-memory: add "detach" flag for QMP/HMP interfaces
>   dump-guest-memory: add basic "detach" support.
> 
>  dump.c                | 62 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  hmp-commands.hx       |  5 +++--
>  hmp.c                 |  3 ++-
>  include/sysemu/dump.h |  4 ++++
>  qapi-schema.json      |  3 ++-
>  qmp-commands.hx       |  4 ++--
>  qmp.c                 |  9 ++++++++
>  vl.c                  |  3 +++
>  8 files changed, 81 insertions(+), 12 deletions(-)
> 

I'm not seeing anything that would prevent races between the new thread
and any other existing threads that manipulate the MemoryRegion objects
(in response to guest actions), or the guest RAM contents (by way of
executing guest code).

The dump_init() function has

    if (runstate_is_running()) {
        vm_stop(RUN_STATE_SAVE_VM);
        s->resume = true;
    } else {
        s->resume = false;
    }

Whereas dump_cleanup() has:

    if (s->resume) {
        vm_start();
    }

If you return control to the QEMU monitor's user before the dump
completes, they could issue the "cont" command, and unleash the VCPU
threads again. (Of course, this is just one example where things could
go wrong.)

Also, the live migration analogy is not a good one IMO. For live
migration, a whole infrastructure exists for tracking asynchronous guest
state changes (dirty bitmap, locking, whatever).

The good analogy with live migration would be continuous streaming of
guest memory changes into the dump file, until it converges, or a cutoff
is reached (at which point the guest would be frozen, same as now). Of
course, such streaming could generate huge amounts of traffic and
entirely defeat the original purpose.

In total, I don't think this is a good idea. I find it possible that
this would lead to QEMU crashes, and/or wildly inconsistent guest memory
images.

As for the goal itself... People also tend to cope with *kdump* being
slow on physical machines.

My recommendation would be to use the dump compression feature
(especially lzo and snappy). In my experience, even when people are
aware of their existence, they don't always realize that shrinking the
dump file size by a given factor also shrinks the dumping *time* by the
same factor, provided that the dumping process remains IO-bound even in
the compressed case.

Which it should, assuming a "very slow storage" -- lzo and snappy are
very CPU-efficient.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Andrew Jones]

On Mon, Nov 23, 2015 at 05:22:29PM +0100, Laszlo Ersek wrote:
> On 11/23/15 11:07, Peter Xu wrote:
> > Currently, dump-guest-memory supports synchronous operation only. This patch
> > sets are adding "detach" support for it (just like "migrate -d" for
> > migration). When "-d" is provided, dump-guest-memory command will return
> > immediately without hanging user. This should be useful when the backend
> > storage for the dump file is very slow.
> > 
> > Peter Xu (2):
> >   dump-guest-memory: add "detach" flag for QMP/HMP interfaces
> >   dump-guest-memory: add basic "detach" support.
> > 
> >  dump.c                | 62 ++++++++++++++++++++++++++++++++++++++++++++++-----
> >  hmp-commands.hx       |  5 +++--
> >  hmp.c                 |  3 ++-
> >  include/sysemu/dump.h |  4 ++++
> >  qapi-schema.json      |  3 ++-
> >  qmp-commands.hx       |  4 ++--
> >  qmp.c                 |  9 ++++++++
> >  vl.c                  |  3 +++
> >  8 files changed, 81 insertions(+), 12 deletions(-)
> > 
> 
> I'm not seeing anything that would prevent races between the new thread
> and any other existing threads that manipulate the MemoryRegion objects
> (in response to guest actions), or the guest RAM contents (by way of
> executing guest code).
> 
> The dump_init() function has
> 
>     if (runstate_is_running()) {
>         vm_stop(RUN_STATE_SAVE_VM);
>         s->resume = true;
>     } else {
>         s->resume = false;
>     }
> 
> Whereas dump_cleanup() has:
> 
>     if (s->resume) {
>         vm_start();
>     }
> 
> If you return control to the QEMU monitor's user before the dump
> completes, they could issue the "cont" command, and unleash the VCPU
> threads again. (Of course, this is just one example where things could
> go wrong.)
> 
> Also, the live migration analogy is not a good one IMO. For live
> migration, a whole infrastructure exists for tracking asynchronous guest
> state changes (dirty bitmap, locking, whatever).
> 
> The good analogy with live migration would be continuous streaming of
> guest memory changes into the dump file, until it converges, or a cutoff
> is reached (at which point the guest would be frozen, same as now). Of
> course, such streaming could generate huge amounts of traffic and
> entirely defeat the original purpose.
> 
> In total, I don't think this is a good idea. I find it possible that
> this would lead to QEMU crashes, and/or wildly inconsistent guest memory
> images.

Despite having already run through both patches giving review comments,
I agree with Laszlo. At first blush it seems like a good idea, but I
can't think of how it would be safe. Also, an admin can always background
the task that invokes the dump if they need that particular terminal
back. So, this looks more like a management tool problem to solve, if
anything.

> 
> As for the goal itself... People also tend to cope with *kdump* being
> slow on physical machines.
> 
> My recommendation would be to use the dump compression feature
> (especially lzo and snappy). In my experience, even when people are
> aware of their existence, they don't always realize that shrinking the
> dump file size by a given factor also shrinks the dumping *time* by the
> same factor, provided that the dumping process remains IO-bound even in
> the compressed case.
> 
> Which it should, assuming a "very slow storage" -- lzo and snappy are
> very CPU-efficient.

This has been my experience, i.e. using lzo or snappy tends to be much,
much faster.

Thanks,
drew

Re: [Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Peter Xu]

Hi, all,

CC'ing all the reviewers.

Sorry to not respond one by one, just trying to avoid spliting into
several emails and people spend extra time reading them, especially
if this patch is to be dropped.

Please check bottom reply.

On 11/24/2015 01:57 AM, Andrew Jones wrote:
> On Mon, Nov 23, 2015 at 05:22:29PM +0100, Laszlo Ersek wrote:
>> On 11/23/15 11:07, Peter Xu wrote:
>>> Currently, dump-guest-memory supports synchronous operation only. This patch
>>> sets are adding "detach" support for it (just like "migrate -d" for
>>> migration). When "-d" is provided, dump-guest-memory command will return
>>> immediately without hanging user. This should be useful when the backend
>>> storage for the dump file is very slow.
>>>
>>> Peter Xu (2):
>>>   dump-guest-memory: add "detach" flag for QMP/HMP interfaces
>>>   dump-guest-memory: add basic "detach" support.
>>>
>>>  dump.c                | 62 ++++++++++++++++++++++++++++++++++++++++++++++-----
>>>  hmp-commands.hx       |  5 +++--
>>>  hmp.c                 |  3 ++-
>>>  include/sysemu/dump.h |  4 ++++
>>>  qapi-schema.json      |  3 ++-
>>>  qmp-commands.hx       |  4 ++--
>>>  qmp.c                 |  9 ++++++++
>>>  vl.c                  |  3 +++
>>>  8 files changed, 81 insertions(+), 12 deletions(-)
>>>
>>
>> I'm not seeing anything that would prevent races between the new thread
>> and any other existing threads that manipulate the MemoryRegion objects
>> (in response to guest actions), or the guest RAM contents (by way of
>> executing guest code).
>>
>> The dump_init() function has
>>
>>     if (runstate_is_running()) {
>>         vm_stop(RUN_STATE_SAVE_VM);
>>         s->resume = true;
>>     } else {
>>         s->resume = false;
>>     }
>>
>> Whereas dump_cleanup() has:
>>
>>     if (s->resume) {
>>         vm_start();
>>     }
>>
>> If you return control to the QEMU monitor's user before the dump
>> completes, they could issue the "cont" command, and unleash the VCPU
>> threads again. (Of course, this is just one example where things could
>> go wrong.)

Yes, I added the global flag "dump_in_progress_p" to do this. For
now, what I found might be affected was "dump-guest-memory" itself,
and "cont". Please check patch 2/2 modification for qmp_cont(). I
failed to find any other place that might be influenced by this
asynchronous operation (you are right, maybe it still exists, and it
might introduce extra bugs, actually that's what I was looking for
to see whether I missed something in the review session).

>>
>> Also, the live migration analogy is not a good one IMO. For live
>> migration, a whole infrastructure exists for tracking asynchronous guest
>> state changes (dirty bitmap, locking, whatever).
>>
>> The good analogy with live migration would be continuous streaming of
>> guest memory changes into the dump file, until it converges, or a cutoff
>> is reached (at which point the guest would be frozen, same as now). Of
>> course, such streaming could generate huge amounts of traffic and
>> entirely defeat the original purpose.

Yes, I see that migration is much more complex scenario, so that's
why I am trying to add "basic detach" support, just as I mentioned
in the patch title. :)

Before doing anything like that complex, I will send a mail asking
about it, to first know whether we need to do that.

>>
>> In total, I don't think this is a good idea. I find it possible that
>> this would lead to QEMU crashes, and/or wildly inconsistent guest memory
>> images.
> 
> Despite having already run through both patches giving review comments,
> I agree with Laszlo. At first blush it seems like a good idea, but I
> can't think of how it would be safe. Also, an admin can always background
> the task that invokes the dump if they need that particular terminal
> back. So, this looks more like a management tool problem to solve, if
> anything.
> 
>>
>> As for the goal itself... People also tend to cope with *kdump* being
>> slow on physical machines.
>>
>> My recommendation would be to use the dump compression feature
>> (especially lzo and snappy). In my experience, even when people are
>> aware of their existence, they don't always realize that shrinking the
>> dump file size by a given factor also shrinks the dumping *time* by the
>> same factor, provided that the dumping process remains IO-bound even in
>> the compressed case.
>>
>> Which it should, assuming a "very slow storage" -- lzo and snappy are
>> very CPU-efficient.
> 
> This has been my experience, i.e. using lzo or snappy tends to be much,
> much faster.

Sorry that I am not the daily user of dump-guest-memory, so I may
have not tried to compare how time would save when compression
techniques are used. Thanks (Drew & Laszlo) to let me know this.
Actually, what I am coping with is the bz:

https://bugzilla.redhat.com/show_bug.cgi?id=1193826

I just feel like it would be nice to offer something extra, when
people are using the stdio monitor, they could have another choice
when dump. Also, this is my first patch to QEMU. That's all I
thought about.

Thanks you all (especially Drew and Laszlo) for leaving mass review
comments. After knowing that more than one of you would suggest not
taking the risk comparing to the feature it brings, I'd totally
agree to drop this patch.

Thanks.
Peter

> 
> Thanks,
> drew
> 

Re: [Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Fam Zheng]

On Tue, 11/24 09:57, Peter Xu wrote:
> On 11/24/2015 01:57 AM, Andrew Jones wrote:
> > On Mon, Nov 23, 2015 at 05:22:29PM +0100, Laszlo Ersek wrote:
> >> On 11/23/15 11:07, Peter Xu wrote:
> >>> Currently, dump-guest-memory supports synchronous operation only. This patch
> >>> sets are adding "detach" support for it (just like "migrate -d" for
> >>> migration). When "-d" is provided, dump-guest-memory command will return
> >>> immediately without hanging user. This should be useful when the backend
> >>> storage for the dump file is very slow.
> >>>
> >>> Peter Xu (2):
> >>>   dump-guest-memory: add "detach" flag for QMP/HMP interfaces
> >>>   dump-guest-memory: add basic "detach" support.
> >>>
> >>>  dump.c                | 62 ++++++++++++++++++++++++++++++++++++++++++++++-----
> >>>  hmp-commands.hx       |  5 +++--
> >>>  hmp.c                 |  3 ++-
> >>>  include/sysemu/dump.h |  4 ++++
> >>>  qapi-schema.json      |  3 ++-
> >>>  qmp-commands.hx       |  4 ++--
> >>>  qmp.c                 |  9 ++++++++
> >>>  vl.c                  |  3 +++
> >>>  8 files changed, 81 insertions(+), 12 deletions(-)
> >>>
> >>
> >> I'm not seeing anything that would prevent races between the new thread
> >> and any other existing threads that manipulate the MemoryRegion objects
> >> (in response to guest actions), or the guest RAM contents (by way of
> >> executing guest code).
> >>
> >> The dump_init() function has
> >>
> >>     if (runstate_is_running()) {
> >>         vm_stop(RUN_STATE_SAVE_VM);
> >>         s->resume = true;
> >>     } else {
> >>         s->resume = false;
> >>     }
> >>
> >> Whereas dump_cleanup() has:
> >>
> >>     if (s->resume) {
> >>         vm_start();
> >>     }
> >>
> >> If you return control to the QEMU monitor's user before the dump
> >> completes, they could issue the "cont" command, and unleash the VCPU
> >> threads again. (Of course, this is just one example where things could
> >> go wrong.)
> 
> Yes, I added the global flag "dump_in_progress_p" to do this. For
> now, what I found might be affected was "dump-guest-memory" itself,
> and "cont". Please check patch 2/2 modification for qmp_cont(). I
> failed to find any other place that might be influenced by this
> asynchronous operation (you are right, maybe it still exists, and it
> might introduce extra bugs, actually that's what I was looking for
> to see whether I missed something in the review session).

What about all the hot-plug commands that changes the memory layout?

Another question is what if user issued "stop" during dump, should you still
resume when dump completes?

> 
> >>
> >> Also, the live migration analogy is not a good one IMO. For live
> >> migration, a whole infrastructure exists for tracking asynchronous guest
> >> state changes (dirty bitmap, locking, whatever).
> >>
> >> The good analogy with live migration would be continuous streaming of
> >> guest memory changes into the dump file, until it converges, or a cutoff
> >> is reached (at which point the guest would be frozen, same as now). Of
> >> course, such streaming could generate huge amounts of traffic and
> >> entirely defeat the original purpose.
> 
> Yes, I see that migration is much more complex scenario, so that's
> why I am trying to add "basic detach" support, just as I mentioned
> in the patch title. :)
> 
> Before doing anything like that complex, I will send a mail asking
> about it, to first know whether we need to do that.
> 
> >>
> >> In total, I don't think this is a good idea. I find it possible that
> >> this would lead to QEMU crashes, and/or wildly inconsistent guest memory
> >> images.
> > 
> > Despite having already run through both patches giving review comments,
> > I agree with Laszlo. At first blush it seems like a good idea, but I
> > can't think of how it would be safe. Also, an admin can always background
> > the task that invokes the dump if they need that particular terminal
> > back. So, this looks more like a management tool problem to solve, if
> > anything.
> > 
> >>
> >> As for the goal itself... People also tend to cope with *kdump* being
> >> slow on physical machines.
> >>
> >> My recommendation would be to use the dump compression feature
> >> (especially lzo and snappy). In my experience, even when people are
> >> aware of their existence, they don't always realize that shrinking the
> >> dump file size by a given factor also shrinks the dumping *time* by the
> >> same factor, provided that the dumping process remains IO-bound even in
> >> the compressed case.
> >>
> >> Which it should, assuming a "very slow storage" -- lzo and snappy are
> >> very CPU-efficient.
> > 
> > This has been my experience, i.e. using lzo or snappy tends to be much,
> > much faster.
> 
> Sorry that I am not the daily user of dump-guest-memory, so I may
> have not tried to compare how time would save when compression
> techniques are used. Thanks (Drew & Laszlo) to let me know this.
> Actually, what I am coping with is the bz:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1193826

I don't think this mode is very helpful to fix this issue unless there is a way
to query the dump progress.

> 
> I just feel like it would be nice to offer something extra, when
> people are using the stdio monitor, they could have another choice
> when dump. Also, this is my first patch to QEMU. That's all I
> thought about.
> 
> Thanks you all (especially Drew and Laszlo) for leaving mass review
> comments. After knowing that more than one of you would suggest not
> taking the risk comparing to the feature it brings, I'd totally
> agree to drop this patch.
> 
> Thanks.
> Peter
> 
> > 
> > Thanks,
> > drew
> > 
> 

Re: [Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Laszlo Ersek]

On 11/24/15 04:10, Fam Zheng wrote:
> On Tue, 11/24 09:57, Peter Xu wrote:
>> On 11/24/2015 01:57 AM, Andrew Jones wrote:
>>> On Mon, Nov 23, 2015 at 05:22:29PM +0100, Laszlo Ersek wrote:
>>>> On 11/23/15 11:07, Peter Xu wrote:
>>>>> Currently, dump-guest-memory supports synchronous operation only. This patch
>>>>> sets are adding "detach" support for it (just like "migrate -d" for
>>>>> migration). When "-d" is provided, dump-guest-memory command will return
>>>>> immediately without hanging user. This should be useful when the backend
>>>>> storage for the dump file is very slow.
>>>>>
>>>>> Peter Xu (2):
>>>>>   dump-guest-memory: add "detach" flag for QMP/HMP interfaces
>>>>>   dump-guest-memory: add basic "detach" support.
>>>>>
>>>>>  dump.c                | 62 ++++++++++++++++++++++++++++++++++++++++++++++-----
>>>>>  hmp-commands.hx       |  5 +++--
>>>>>  hmp.c                 |  3 ++-
>>>>>  include/sysemu/dump.h |  4 ++++
>>>>>  qapi-schema.json      |  3 ++-
>>>>>  qmp-commands.hx       |  4 ++--
>>>>>  qmp.c                 |  9 ++++++++
>>>>>  vl.c                  |  3 +++
>>>>>  8 files changed, 81 insertions(+), 12 deletions(-)
>>>>>
>>>>
>>>> I'm not seeing anything that would prevent races between the new thread
>>>> and any other existing threads that manipulate the MemoryRegion objects
>>>> (in response to guest actions), or the guest RAM contents (by way of
>>>> executing guest code).
>>>>
>>>> The dump_init() function has
>>>>
>>>>     if (runstate_is_running()) {
>>>>         vm_stop(RUN_STATE_SAVE_VM);
>>>>         s->resume = true;
>>>>     } else {
>>>>         s->resume = false;
>>>>     }
>>>>
>>>> Whereas dump_cleanup() has:
>>>>
>>>>     if (s->resume) {
>>>>         vm_start();
>>>>     }
>>>>
>>>> If you return control to the QEMU monitor's user before the dump
>>>> completes, they could issue the "cont" command, and unleash the VCPU
>>>> threads again. (Of course, this is just one example where things could
>>>> go wrong.)
>>
>> Yes, I added the global flag "dump_in_progress_p" to do this. For
>> now, what I found might be affected was "dump-guest-memory" itself,
>> and "cont". Please check patch 2/2 modification for qmp_cont(). I
>> failed to find any other place that might be influenced by this
>> asynchronous operation (you are right, maybe it still exists, and it
>> might introduce extra bugs, actually that's what I was looking for
>> to see whether I missed something in the review session).
> 
> What about all the hot-plug commands that changes the memory layout?
> 
> Another question is what if user issued "stop" during dump, should you still
> resume when dump completes?

I agree completely. Synchronization requires design; it is not (well,
should not be) a whack-a-mole game where you smack down races as they
pop up. The possible interactions here are just to complex, and making
the dump feature truly multi-threaded would require a lot of work. I
don't think that is warranted for in this instance (see below).

> 
>>
>>>>
>>>> Also, the live migration analogy is not a good one IMO. For live
>>>> migration, a whole infrastructure exists for tracking asynchronous guest
>>>> state changes (dirty bitmap, locking, whatever).
>>>>
>>>> The good analogy with live migration would be continuous streaming of
>>>> guest memory changes into the dump file, until it converges, or a cutoff
>>>> is reached (at which point the guest would be frozen, same as now). Of
>>>> course, such streaming could generate huge amounts of traffic and
>>>> entirely defeat the original purpose.
>>
>> Yes, I see that migration is much more complex scenario, so that's
>> why I am trying to add "basic detach" support, just as I mentioned
>> in the patch title. :)
>>
>> Before doing anything like that complex, I will send a mail asking
>> about it, to first know whether we need to do that.
>>
>>>>
>>>> In total, I don't think this is a good idea. I find it possible that
>>>> this would lead to QEMU crashes, and/or wildly inconsistent guest memory
>>>> images.
>>>
>>> Despite having already run through both patches giving review comments,
>>> I agree with Laszlo. At first blush it seems like a good idea, but I
>>> can't think of how it would be safe. Also, an admin can always background
>>> the task that invokes the dump if they need that particular terminal
>>> back. So, this looks more like a management tool problem to solve, if
>>> anything.
>>>
>>>>
>>>> As for the goal itself... People also tend to cope with *kdump* being
>>>> slow on physical machines.
>>>>
>>>> My recommendation would be to use the dump compression feature
>>>> (especially lzo and snappy). In my experience, even when people are
>>>> aware of their existence, they don't always realize that shrinking the
>>>> dump file size by a given factor also shrinks the dumping *time* by the
>>>> same factor, provided that the dumping process remains IO-bound even in
>>>> the compressed case.
>>>>
>>>> Which it should, assuming a "very slow storage" -- lzo and snappy are
>>>> very CPU-efficient.
>>>
>>> This has been my experience, i.e. using lzo or snappy tends to be much,
>>> much faster.
>>
>> Sorry that I am not the daily user of dump-guest-memory, so I may
>> have not tried to compare how time would save when compression
>> techniques are used. Thanks (Drew & Laszlo) to let me know this.
>> Actually, what I am coping with is the bz:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1193826
> 
> I don't think this mode is very helpful to fix this issue unless there is a way
> to query the dump progress.

Ah, so all it is about is progress information for the interactive user.
I think that should be doable with periodic messages written to the
monitor. I can think of some well-placed error_printf() calls that
report progress info to the monitor, after every N megabytes of guest
RAM processed (or something similar), or perhaps even a new QEMU event.

I think the libvirt and QMP developers should be able to help with this.

> 
>>
>> I just feel like it would be nice to offer something extra, when
>> people are using the stdio monitor, they could have another choice
>> when dump. Also, this is my first patch to QEMU. That's all I
>> thought about.
>>
>> Thanks you all (especially Drew and Laszlo) for leaving mass review
>> comments. After knowing that more than one of you would suggest not
>> taking the risk comparing to the feature it brings, I'd totally
>> agree to drop this patch.

I think the patch should be dropped, and periodic progress reports
should be emitted from within the dump loops that do the heavy lifting.

For the ELF format dumps, that loop appears to reside in dump_iterate()
[dump.c].

For the compressed format dumps, the loop seems to live in
write_dump_pages() [dump.c].

Thanks!
Laszlo

>>
>> Thanks.
>> Peter
>>
>>>
>>> Thanks,
>>> drew
>>>
>>

Re: [Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Fam Zheng]

On Tue, 11/24 12:14, Laszlo Ersek wrote:
> On 11/24/15 04:10, Fam Zheng wrote:
> > On Tue, 11/24 09:57, Peter Xu wrote:
> >> On 11/24/2015 01:57 AM, Andrew Jones wrote:
> >>> On Mon, Nov 23, 2015 at 05:22:29PM +0100, Laszlo Ersek wrote:
> >>>> On 11/23/15 11:07, Peter Xu wrote:
> >>>>> Currently, dump-guest-memory supports synchronous operation only. This patch
> >>>>> sets are adding "detach" support for it (just like "migrate -d" for
> >>>>> migration). When "-d" is provided, dump-guest-memory command will return
> >>>>> immediately without hanging user. This should be useful when the backend
> >>>>> storage for the dump file is very slow.
> >>>>>
> >>>>> Peter Xu (2):
> >>>>>   dump-guest-memory: add "detach" flag for QMP/HMP interfaces
> >>>>>   dump-guest-memory: add basic "detach" support.
> >>>>>
> >>>>>  dump.c                | 62 ++++++++++++++++++++++++++++++++++++++++++++++-----
> >>>>>  hmp-commands.hx       |  5 +++--
> >>>>>  hmp.c                 |  3 ++-
> >>>>>  include/sysemu/dump.h |  4 ++++
> >>>>>  qapi-schema.json      |  3 ++-
> >>>>>  qmp-commands.hx       |  4 ++--
> >>>>>  qmp.c                 |  9 ++++++++
> >>>>>  vl.c                  |  3 +++
> >>>>>  8 files changed, 81 insertions(+), 12 deletions(-)
> >>>>>
> >>>>
> >>>> I'm not seeing anything that would prevent races between the new thread
> >>>> and any other existing threads that manipulate the MemoryRegion objects
> >>>> (in response to guest actions), or the guest RAM contents (by way of
> >>>> executing guest code).
> >>>>
> >>>> The dump_init() function has
> >>>>
> >>>>     if (runstate_is_running()) {
> >>>>         vm_stop(RUN_STATE_SAVE_VM);
> >>>>         s->resume = true;
> >>>>     } else {
> >>>>         s->resume = false;
> >>>>     }
> >>>>
> >>>> Whereas dump_cleanup() has:
> >>>>
> >>>>     if (s->resume) {
> >>>>         vm_start();
> >>>>     }
> >>>>
> >>>> If you return control to the QEMU monitor's user before the dump
> >>>> completes, they could issue the "cont" command, and unleash the VCPU
> >>>> threads again. (Of course, this is just one example where things could
> >>>> go wrong.)
> >>
> >> Yes, I added the global flag "dump_in_progress_p" to do this. For
> >> now, what I found might be affected was "dump-guest-memory" itself,
> >> and "cont". Please check patch 2/2 modification for qmp_cont(). I
> >> failed to find any other place that might be influenced by this
> >> asynchronous operation (you are right, maybe it still exists, and it
> >> might introduce extra bugs, actually that's what I was looking for
> >> to see whether I missed something in the review session).
> > 
> > What about all the hot-plug commands that changes the memory layout?
> > 
> > Another question is what if user issued "stop" during dump, should you still
> > resume when dump completes?
> 
> I agree completely. Synchronization requires design; it is not (well,
> should not be) a whack-a-mole game where you smack down races as they
> pop up. The possible interactions here are just to complex, and making
> the dump feature truly multi-threaded would require a lot of work. I
> don't think that is warranted for in this instance (see below).
> 
> > 
> >>
> >>>>
> >>>> Also, the live migration analogy is not a good one IMO. For live
> >>>> migration, a whole infrastructure exists for tracking asynchronous guest
> >>>> state changes (dirty bitmap, locking, whatever).
> >>>>
> >>>> The good analogy with live migration would be continuous streaming of
> >>>> guest memory changes into the dump file, until it converges, or a cutoff
> >>>> is reached (at which point the guest would be frozen, same as now). Of
> >>>> course, such streaming could generate huge amounts of traffic and
> >>>> entirely defeat the original purpose.
> >>
> >> Yes, I see that migration is much more complex scenario, so that's
> >> why I am trying to add "basic detach" support, just as I mentioned
> >> in the patch title. :)
> >>
> >> Before doing anything like that complex, I will send a mail asking
> >> about it, to first know whether we need to do that.
> >>
> >>>>
> >>>> In total, I don't think this is a good idea. I find it possible that
> >>>> this would lead to QEMU crashes, and/or wildly inconsistent guest memory
> >>>> images.
> >>>
> >>> Despite having already run through both patches giving review comments,
> >>> I agree with Laszlo. At first blush it seems like a good idea, but I
> >>> can't think of how it would be safe. Also, an admin can always background
> >>> the task that invokes the dump if they need that particular terminal
> >>> back. So, this looks more like a management tool problem to solve, if
> >>> anything.
> >>>
> >>>>
> >>>> As for the goal itself... People also tend to cope with *kdump* being
> >>>> slow on physical machines.
> >>>>
> >>>> My recommendation would be to use the dump compression feature
> >>>> (especially lzo and snappy). In my experience, even when people are
> >>>> aware of their existence, they don't always realize that shrinking the
> >>>> dump file size by a given factor also shrinks the dumping *time* by the
> >>>> same factor, provided that the dumping process remains IO-bound even in
> >>>> the compressed case.
> >>>>
> >>>> Which it should, assuming a "very slow storage" -- lzo and snappy are
> >>>> very CPU-efficient.
> >>>
> >>> This has been my experience, i.e. using lzo or snappy tends to be much,
> >>> much faster.
> >>
> >> Sorry that I am not the daily user of dump-guest-memory, so I may
> >> have not tried to compare how time would save when compression
> >> techniques are used. Thanks (Drew & Laszlo) to let me know this.
> >> Actually, what I am coping with is the bz:
> >>
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1193826
> > 
> > I don't think this mode is very helpful to fix this issue unless there is a way
> > to query the dump progress.
> 
> Ah, so all it is about is progress information for the interactive user.
> I think that should be doable with periodic messages written to the
> monitor. I can think of some well-placed error_printf() calls that
> report progress info to the monitor, after every N megabytes of guest
> RAM processed (or something similar), or perhaps even a new QEMU event.
> 
> I think the libvirt and QMP developers should be able to help with this.
> 
> > 
> >>
> >> I just feel like it would be nice to offer something extra, when
> >> people are using the stdio monitor, they could have another choice
> >> when dump. Also, this is my first patch to QEMU. That's all I
> >> thought about.
> >>
> >> Thanks you all (especially Drew and Laszlo) for leaving mass review
> >> comments. After knowing that more than one of you would suggest not
> >> taking the risk comparing to the feature it brings, I'd totally
> >> agree to drop this patch.
> 
> I think the patch should be dropped, and periodic progress reports
> should be emitted from within the dump loops that do the heavy lifting.
> 
> For the ELF format dumps, that loop appears to reside in dump_iterate()
> [dump.c].
> 
> For the compressed format dumps, the loop seems to live in
> write_dump_pages() [dump.c].

This is a good idea!

What I'm not sure is where to report the progress. Can it be the monitor where
the dump-guest-memory command was issued? In other words, do we support raising
events before the previous command returns? If yes, can libvirt handle this
correctly? (But the worst case is using another channel to communicate the
progress, it is ad-hocery but it must be better than all the risk and effort to
enable multi-threaded dump.)

Eric, Markus, have any idea with the progress reporting?

Fam

Re: [Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1678 bytes --]

On 11/24/2015 04:37 AM, Fam Zheng wrote:

>> I think the patch should be dropped, and periodic progress reports
>> should be emitted from within the dump loops that do the heavy lifting.
>>
>> For the ELF format dumps, that loop appears to reside in dump_iterate()
>> [dump.c].
>>
>> For the compressed format dumps, the loop seems to live in
>> write_dump_pages() [dump.c].
> 
> This is a good idea!
> 
> What I'm not sure is where to report the progress. Can it be the monitor where
> the dump-guest-memory command was issued? In other words, do we support raising
> events before the previous command returns? If yes, can libvirt handle this
> correctly? (But the worst case is using another channel to communicate the
> progress, it is ad-hocery but it must be better than all the risk and effort to
> enable multi-threaded dump.)
> 
> Eric, Markus, have any idea with the progress reporting?

I'm fairly certain we support raising events prior to completion of a
synchronous command; what I'm not sure of is whether the event hits the
wire right away or whether it piles up waiting for the next synchronous
command completion.  If the latter, then we need to rework it (since the
whole point of this exercise is that we are trying to give progress of a
long-running synchronous command that hasn't completed yet).  But we
only have the one monitor connection for libvirt - the only way to pass
events through a second channel is to open a second monitor connection,
but that feels wrong to make libvirt have to track two monitors.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Fam Zheng]

On Tue, 11/24 06:49, Eric Blake wrote:
> On 11/24/2015 04:37 AM, Fam Zheng wrote:
> 
> >> I think the patch should be dropped, and periodic progress reports
> >> should be emitted from within the dump loops that do the heavy lifting.
> >>
> >> For the ELF format dumps, that loop appears to reside in dump_iterate()
> >> [dump.c].
> >>
> >> For the compressed format dumps, the loop seems to live in
> >> write_dump_pages() [dump.c].
> > 
> > This is a good idea!
> > 
> > What I'm not sure is where to report the progress. Can it be the monitor where
> > the dump-guest-memory command was issued? In other words, do we support raising
> > events before the previous command returns? If yes, can libvirt handle this
> > correctly? (But the worst case is using another channel to communicate the
> > progress, it is ad-hocery but it must be better than all the risk and effort to
> > enable multi-threaded dump.)
> > 
> > Eric, Markus, have any idea with the progress reporting?
> 
> I'm fairly certain we support raising events prior to completion of a
> synchronous command; what I'm not sure of is whether the event hits the
> wire right away or whether it piles up waiting for the next synchronous
> command completion.  If the latter, then we need to rework it (since the
> whole point of this exercise is that we are trying to give progress of a
> long-running synchronous command that hasn't completed yet).

So in that case we may want some "flush" operation of events. That sounds OK to
me.

> But we
> only have the one monitor connection for libvirt - the only way to pass
> events through a second channel is to open a second monitor connection,
> but that feels wrong to make libvirt have to track two monitors.

OK, that's a fair point, but FWIW I was thinking about adding an optional
argument:

    "*progress": "fd:dump-progress"

into which dump.c talks in a mini-protocol, to send progress information. It's
just an crazily hacky idea, not anything I'm advocating.

Thanks,

Fam

Re: [Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Peter Xu]

On 11/25/2015 10:46 AM, Fam Zheng wrote:
> On Tue, 11/24 06:49, Eric Blake wrote:
>> On 11/24/2015 04:37 AM, Fam Zheng wrote:
>>
>>>> I think the patch should be dropped, and periodic progress reports
>>>> should be emitted from within the dump loops that do the heavy lifting.
>>>>
>>>> For the ELF format dumps, that loop appears to reside in dump_iterate()
>>>> [dump.c].
>>>>
>>>> For the compressed format dumps, the loop seems to live in
>>>> write_dump_pages() [dump.c].
>>>
>>> This is a good idea!
>>>
>>> What I'm not sure is where to report the progress. Can it be the monitor where
>>> the dump-guest-memory command was issued? In other words, do we support raising
>>> events before the previous command returns? If yes, can libvirt handle this
>>> correctly? (But the worst case is using another channel to communicate the
>>> progress, it is ad-hocery but it must be better than all the risk and effort to
>>> enable multi-threaded dump.)
>>>
>>> Eric, Markus, have any idea with the progress reporting?
>>
>> I'm fairly certain we support raising events prior to completion of a
>> synchronous command; what I'm not sure of is whether the event hits the
>> wire right away or whether it piles up waiting for the next synchronous
>> command completion.  If the latter, then we need to rework it (since the
>> whole point of this exercise is that we are trying to give progress of a
>> long-running synchronous command that hasn't completed yet).
> 
> So in that case we may want some "flush" operation of events. That sounds OK to
> me.
> 
>> But we
>> only have the one monitor connection for libvirt - the only way to pass
>> events through a second channel is to open a second monitor connection,
>> but that feels wrong to make libvirt have to track two monitors.
> 
> OK, that's a fair point, but FWIW I was thinking about adding an optional
> argument:
> 
>     "*progress": "fd:dump-progress"
> 
> into which dump.c talks in a mini-protocol, to send progress information. It's
> just an crazily hacky idea, not anything I'm advocating.

If query status is necessary, what about adding one command:
"query-dump"? Which could be a simplified version of "query-migration":

1. before first dump:

-> { "execute": "query-dump" }
<- { "return": {} }

2. one background dump in progress:

-> { "execute": "query-dump" }
<- {
      "return":{
         "status":"active",
         "percentage": {0..99},
      }
   }

3. after first dump, and not running background dump (substraction
   of case 1 and 2)

-> { "execute": "query-dump" }
<- {
     "return": {
        "status": "completed|failed",
     }
   }

All these would be based on the fact that this patch might not be
dropped though. :)

Thanks.
Peter

> 
> Thanks,
> 
> Fam
> 

Re: [Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Fam Zheng]

On Wed, 11/25 12:48, Peter Xu wrote:
> 
> 
> On 11/25/2015 10:46 AM, Fam Zheng wrote:
> > On Tue, 11/24 06:49, Eric Blake wrote:
> >> On 11/24/2015 04:37 AM, Fam Zheng wrote:
> >>
> >>>> I think the patch should be dropped, and periodic progress reports
> >>>> should be emitted from within the dump loops that do the heavy lifting.
> >>>>
> >>>> For the ELF format dumps, that loop appears to reside in dump_iterate()
> >>>> [dump.c].
> >>>>
> >>>> For the compressed format dumps, the loop seems to live in
> >>>> write_dump_pages() [dump.c].
> >>>
> >>> This is a good idea!
> >>>
> >>> What I'm not sure is where to report the progress. Can it be the monitor where
> >>> the dump-guest-memory command was issued? In other words, do we support raising
> >>> events before the previous command returns? If yes, can libvirt handle this
> >>> correctly? (But the worst case is using another channel to communicate the
> >>> progress, it is ad-hocery but it must be better than all the risk and effort to
> >>> enable multi-threaded dump.)
> >>>
> >>> Eric, Markus, have any idea with the progress reporting?
> >>
> >> I'm fairly certain we support raising events prior to completion of a
> >> synchronous command; what I'm not sure of is whether the event hits the
> >> wire right away or whether it piles up waiting for the next synchronous
> >> command completion.  If the latter, then we need to rework it (since the
> >> whole point of this exercise is that we are trying to give progress of a
> >> long-running synchronous command that hasn't completed yet).
> > 
> > So in that case we may want some "flush" operation of events. That sounds OK to
> > me.
> > 
> >> But we
> >> only have the one monitor connection for libvirt - the only way to pass
> >> events through a second channel is to open a second monitor connection,
> >> but that feels wrong to make libvirt have to track two monitors.
> > 
> > OK, that's a fair point, but FWIW I was thinking about adding an optional
> > argument:
> > 
> >     "*progress": "fd:dump-progress"
> > 
> > into which dump.c talks in a mini-protocol, to send progress information. It's
> > just an crazily hacky idea, not anything I'm advocating.
> 
> If query status is necessary, what about adding one command:
> "query-dump"? Which could be a simplified version of "query-migration":
> 
> 1. before first dump:
> 
> -> { "execute": "query-dump" }
> <- { "return": {} }
> 
> 2. one background dump in progress:
> 
> -> { "execute": "query-dump" }
> <- {
>       "return":{
>          "status":"active",
>          "percentage": {0..99},
>       }
>    }
> 
> 3. after first dump, and not running background dump (substraction
>    of case 1 and 2)
> 
> -> { "execute": "query-dump" }
> <- {
>      "return": {
>         "status": "completed|failed",
>      }
>    }
> 
> All these would be based on the fact that this patch might not be
> dropped though. :)

This is okay, if you're going to use threaded dump. And it needs an QAPI event
like other async operations. See migrate_generate_event.

Fam

Re: [Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/24/2015 04:37 AM, Fam Zheng wrote:
>
>>> I think the patch should be dropped, and periodic progress reports
>>> should be emitted from within the dump loops that do the heavy lifting.
>>>
>>> For the ELF format dumps, that loop appears to reside in dump_iterate()
>>> [dump.c].
>>>
>>> For the compressed format dumps, the loop seems to live in
>>> write_dump_pages() [dump.c].
>> 
>> This is a good idea!
>> 
>> What I'm not sure is where to report the progress. Can it be the monitor where
>> the dump-guest-memory command was issued? In other words, do we
>> support raising
>> events before the previous command returns? If yes, can libvirt handle this
>> correctly? (But the worst case is using another channel to communicate the
>> progress, it is ad-hocery but it must be better than all the risk
>> and effort to
>> enable multi-threaded dump.)
>> 
>> Eric, Markus, have any idea with the progress reporting?
>
> I'm fairly certain we support raising events prior to completion of a
> synchronous command; what I'm not sure of is whether the event hits the
> wire right away or whether it piles up waiting for the next synchronous
> command completion.  If the latter, then we need to rework it (since the
> whole point of this exercise is that we are trying to give progress of a
> long-running synchronous command that hasn't completed yet).  But we
> only have the one monitor connection for libvirt - the only way to pass
> events through a second channel is to open a second monitor connection,
> but that feels wrong to make libvirt have to track two monitors.

Short answer: events can be sent at any time.  Delay can come only from
taking the necessary lock, or rate limiting.

Longer answer: code triggers an event FOO by calling (QAPI-generated)
qapi_event_send_FOO().  This marshalls the event's arguments and calls
monitor_qapi_event_queue() (you have to peel off some obfuscating
abstraction to see that).  Assuming no rate limiting, this simply takes
the monitor_lock to call monitor_qapi_event_emit(), which immediately
sends the event to all QMP monitors.

Re: [Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Paolo Bonzini]

On 24/11/2015 04:10, Fam Zheng wrote:
> What about all the hot-plug commands that changes the memory layout?

If the guest is stopped, they shouldn't.  device_add does not enable new
BARs for example, the guest does that after it receives the ACPI event
for PCI hotplug (or similarly an interrupt for SHPC or PCIe hotplug).

Actually I like the idea of background dump, and a separate thread is an
obvious way to do it since QEMU's memory API is mostly thread safe.

However, qmp_dump_guest_memory should only proceed if the VM is stopped
and is not in incoming migration (RUN_STATE_INMIGRATE); as you prefer.
If the VM is stopped, there is no whack-a-mole; the memory should not be
touched after vm_stop returns.  The only special case is incoming migration.

Regarding thread-safety, the thread needs to take
qemu_mutex_ram_list_lock or rcu_read_lock in order to get the list of
RAM regions.  Even better, build a list of MemoryRegions in advance
(protecting them with memory_region_ref) in the iothread, and consult it
during the dump.  At the end you can use memory_region_unref to release
them.

Paolo

Re: [Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Laszlo Ersek]

On 11/24/15 13:05, Paolo Bonzini wrote:
> 
> 
> On 24/11/2015 04:10, Fam Zheng wrote:
>> What about all the hot-plug commands that changes the memory layout?
> 
> If the guest is stopped, they shouldn't.  device_add does not enable new
> BARs for example, the guest does that after it receives the ACPI event
> for PCI hotplug (or similarly an interrupt for SHPC or PCIe hotplug).
> 
> Actually I like the idea of background dump, and a separate thread is an
> obvious way to do it since QEMU's memory API is mostly thread safe.

I'm not trying to reject this patch just because "I don't like it". I
perceive it as extremely risky, and I don't know enough to review it
with *full coverage*. If you're willing to review it, and Peter can
assume the responsibility of supporting it down the road, feel free to
go ahead.

> However, qmp_dump_guest_memory should only proceed if the VM is stopped
> and is not in incoming migration (RUN_STATE_INMIGRATE); as you prefer.
> If the VM is stopped, there is no whack-a-mole; the memory should not be
> touched after vm_stop returns.  The only special case is incoming migration.
> 
> Regarding thread-safety, the thread needs to take
> qemu_mutex_ram_list_lock or rcu_read_lock in order to get the list of
> RAM regions.  Even better, build a list of MemoryRegions in advance
> (protecting them with memory_region_ref) in the iothread, and consult it
> during the dump.  At the end you can use memory_region_unref to release
> them.
> 
> Paolo
> 

Re: [Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Paolo Bonzini]

On 24/11/2015 14:36, Laszlo Ersek wrote:
> On 11/24/15 13:05, Paolo Bonzini wrote:
>>
>>
>> On 24/11/2015 04:10, Fam Zheng wrote:
>>> What about all the hot-plug commands that changes the memory layout?
>>
>> If the guest is stopped, they shouldn't.  device_add does not enable new
>> BARs for example, the guest does that after it receives the ACPI event
>> for PCI hotplug (or similarly an interrupt for SHPC or PCIe hotplug).
>>
>> Actually I like the idea of background dump, and a separate thread is an
>> obvious way to do it since QEMU's memory API is mostly thread safe.
> 
> I'm not trying to reject this patch just because "I don't like it". I
> perceive it as extremely risky, and I don't know enough to review it
> with *full coverage*. If you're willing to review it, and Peter can
> assume the responsibility of supporting it down the road, feel free to
> go ahead.

Understood.  That's always the case on both the submitter and the
maintainer sides.  (That said, I'm not dump-guest-memory maintainer; but
I _can_ make some promises on memory changes).

Paolo

Re: [Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Peter Xu]

On 11/24/2015 08:05 PM, Paolo Bonzini wrote:
> 
> 
> On 24/11/2015 04:10, Fam Zheng wrote:
>> What about all the hot-plug commands that changes the memory layout?
> 
> If the guest is stopped, they shouldn't.  device_add does not enable new
> BARs for example, the guest does that after it receives the ACPI event
> for PCI hotplug (or similarly an interrupt for SHPC or PCIe hotplug).
> 
> Actually I like the idea of background dump, and a separate thread is an
> obvious way to do it since QEMU's memory API is mostly thread safe.
> 
> However, qmp_dump_guest_memory should only proceed if the VM is stopped
> and is not in incoming migration (RUN_STATE_INMIGRATE); as you prefer.
> If the VM is stopped, there is no whack-a-mole; the memory should not be
> touched after vm_stop returns.  The only special case is incoming migration.
> 
> Regarding thread-safety, the thread needs to take
> qemu_mutex_ram_list_lock or rcu_read_lock in order to get the list of
> RAM regions.  Even better, build a list of MemoryRegions in advance
> (protecting them with memory_region_ref) in the iothread, and consult it
> during the dump.  At the end you can use memory_region_unref to release
> them.

Hi, Paolo,

Thanks for the comments.

If any of you are interested in this function, I would like to make
bold to take the ownership to move on to v2, with all the review
comments adopted (it might be necessary to contain a basic query
mechanism if there is a v2 patch, since I see that it's the one that
most to be complaint about besides the risk that it might bring).

Thanks.
Peter

> 
> Paolo
>