[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 
---
 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(_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(>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 
> ---
>  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(_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(>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