* [Qemu-devel] [PATCH v2 1/8] dump-guest-memory: cleanup: removing dump_{error|cleanup}().
2015-11-27 2:48 [Qemu-devel] [PATCH v2 0/8] Add basic "detach" support for dump-guest-memory Peter Xu
@ 2015-11-27 2:48 ` Peter Xu
2015-11-27 4:28 ` Fam Zheng
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 2/8] dump-guest-memory: add "detach" flag for QMP/HMP interfaces Peter Xu
` (8 subsequent siblings)
9 siblings, 1 reply; 43+ messages in thread
From: Peter Xu @ 2015-11-27 2:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Xu
It might be a little bit confusing to do dump_cleanup() in these two
functions (and error prone, please see section below). A better way
is to do dump_cleanup() before dump finish, no matter whether dump
has succeeded or not.
Meanwhile, this patch will also fix the potential memory leaks that
might happen when dump got errors within the process (e.g., when
write_dump_header() fails, it will skip calling dump_cleanup(),
which will leads to memory leak for list elements in DumpState).
Signed-off-by: Peter Xu <peterx@redhat.com>
---
dump.c | 78 +++++++++++++++++++++++++++---------------------------------------
1 file changed, 32 insertions(+), 46 deletions(-)
diff --git a/dump.c b/dump.c
index 78b7d84..445e739 100644
--- a/dump.c
+++ b/dump.c
@@ -82,12 +82,6 @@ static int dump_cleanup(DumpState *s)
return 0;
}
-static void dump_error(DumpState *s, const char *reason, Error **errp)
-{
- dump_cleanup(s);
- error_setg(errp, "%s", reason);
-}
-
static int fd_write_vmcore(const void *buf, size_t size, void *opaque)
{
DumpState *s = opaque;
@@ -128,7 +122,7 @@ static void write_elf64_header(DumpState *s, Error **errp)
ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
if (ret < 0) {
- dump_error(s, "dump: failed to write elf header", errp);
+ error_setg(errp, "dump: failed to write elf header");
}
}
@@ -159,7 +153,7 @@ static void write_elf32_header(DumpState *s, Error **errp)
ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
if (ret < 0) {
- dump_error(s, "dump: failed to write elf header", errp);
+ error_setg(errp, "dump: failed to write elf header");
}
}
@@ -182,7 +176,7 @@ static void write_elf64_load(DumpState *s, MemoryMapping *memory_mapping,
ret = fd_write_vmcore(&phdr, sizeof(Elf64_Phdr), s);
if (ret < 0) {
- dump_error(s, "dump: failed to write program header table", errp);
+ error_setg(errp, "dump: failed to write program header table");
}
}
@@ -205,7 +199,7 @@ static void write_elf32_load(DumpState *s, MemoryMapping *memory_mapping,
ret = fd_write_vmcore(&phdr, sizeof(Elf32_Phdr), s);
if (ret < 0) {
- dump_error(s, "dump: failed to write program header table", errp);
+ error_setg(errp, "dump: failed to write program header table");
}
}
@@ -225,7 +219,7 @@ static void write_elf64_note(DumpState *s, Error **errp)
ret = fd_write_vmcore(&phdr, sizeof(Elf64_Phdr), s);
if (ret < 0) {
- dump_error(s, "dump: failed to write program header table", errp);
+ error_setg(errp, "dump: failed to write program header table");
}
}
@@ -245,7 +239,7 @@ static void write_elf64_notes(WriteCoreDumpFunction f, DumpState *s,
id = cpu_index(cpu);
ret = cpu_write_elf64_note(f, cpu, id, s);
if (ret < 0) {
- dump_error(s, "dump: failed to write elf notes", errp);
+ error_setg(errp, "dump: failed to write elf notes");
return;
}
}
@@ -253,7 +247,7 @@ static void write_elf64_notes(WriteCoreDumpFunction f, DumpState *s,
CPU_FOREACH(cpu) {
ret = cpu_write_elf64_qemunote(f, cpu, s);
if (ret < 0) {
- dump_error(s, "dump: failed to write CPU status", errp);
+ error_setg(errp, "dump: failed to write CPU status");
return;
}
}
@@ -275,7 +269,7 @@ static void write_elf32_note(DumpState *s, Error **errp)
ret = fd_write_vmcore(&phdr, sizeof(Elf32_Phdr), s);
if (ret < 0) {
- dump_error(s, "dump: failed to write program header table", errp);
+ error_setg(errp, "dump: failed to write program header table");
}
}
@@ -290,7 +284,7 @@ static void write_elf32_notes(WriteCoreDumpFunction f, DumpState *s,
id = cpu_index(cpu);
ret = cpu_write_elf32_note(f, cpu, id, s);
if (ret < 0) {
- dump_error(s, "dump: failed to write elf notes", errp);
+ error_setg(errp, "dump: failed to write elf notes");
return;
}
}
@@ -298,7 +292,7 @@ static void write_elf32_notes(WriteCoreDumpFunction f, DumpState *s,
CPU_FOREACH(cpu) {
ret = cpu_write_elf32_qemunote(f, cpu, s);
if (ret < 0) {
- dump_error(s, "dump: failed to write CPU status", errp);
+ error_setg(errp, "dump: failed to write CPU status");
return;
}
}
@@ -326,7 +320,7 @@ static void write_elf_section(DumpState *s, int type, Error **errp)
ret = fd_write_vmcore(&shdr, shdr_size, s);
if (ret < 0) {
- dump_error(s, "dump: failed to write section header table", errp);
+ error_setg(errp, "dump: failed to write section header table");
}
}
@@ -336,7 +330,7 @@ static void write_data(DumpState *s, void *buf, int length, Error **errp)
ret = fd_write_vmcore(buf, length, s);
if (ret < 0) {
- dump_error(s, "dump: failed to save memory", errp);
+ error_setg(errp, "dump: failed to save memory");
}
}
@@ -568,11 +562,6 @@ static void dump_begin(DumpState *s, Error **errp)
}
}
-static void dump_completed(DumpState *s)
-{
- dump_cleanup(s);
-}
-
static int get_next_block(DumpState *s, GuestPhysBlock *block)
{
while (1) {
@@ -624,8 +613,6 @@ static void dump_iterate(DumpState *s, Error **errp)
}
} while (!get_next_block(s, block));
-
- dump_completed(s);
}
static void create_vmcore(DumpState *s, Error **errp)
@@ -765,7 +752,7 @@ static void create_header32(DumpState *s, Error **errp)
dh->status = cpu_to_dump32(s, status);
if (write_buffer(s->fd, 0, dh, size) < 0) {
- dump_error(s, "dump: failed to write disk dump header", errp);
+ error_setg(errp, "dump: failed to write disk dump header");
goto out;
}
@@ -784,7 +771,7 @@ static void create_header32(DumpState *s, Error **errp)
if (write_buffer(s->fd, DISKDUMP_HEADER_BLOCKS *
block_size, kh, size) < 0) {
- dump_error(s, "dump: failed to write kdump sub header", errp);
+ error_setg(errp, "dump: failed to write kdump sub header");
goto out;
}
@@ -800,7 +787,7 @@ static void create_header32(DumpState *s, Error **errp)
}
if (write_buffer(s->fd, offset_note, s->note_buf,
s->note_size) < 0) {
- dump_error(s, "dump: failed to write notes", errp);
+ error_setg(errp, "dump: failed to write notes");
goto out;
}
@@ -865,7 +852,7 @@ static void create_header64(DumpState *s, Error **errp)
dh->status = cpu_to_dump32(s, status);
if (write_buffer(s->fd, 0, dh, size) < 0) {
- dump_error(s, "dump: failed to write disk dump header", errp);
+ error_setg(errp, "dump: failed to write disk dump header");
goto out;
}
@@ -884,7 +871,7 @@ static void create_header64(DumpState *s, Error **errp)
if (write_buffer(s->fd, DISKDUMP_HEADER_BLOCKS *
block_size, kh, size) < 0) {
- dump_error(s, "dump: failed to write kdump sub header", errp);
+ error_setg(errp, "dump: failed to write kdump sub header");
goto out;
}
@@ -901,7 +888,7 @@ static void create_header64(DumpState *s, Error **errp)
if (write_buffer(s->fd, offset_note, s->note_buf,
s->note_size) < 0) {
- dump_error(s, "dump: failed to write notes", errp);
+ error_setg(errp, "dump: failed to write notes");
goto out;
}
@@ -1064,7 +1051,7 @@ static void write_dump_bitmap(DumpState *s, Error **errp)
while (get_next_page(&block_iter, &pfn, NULL, s)) {
ret = set_dump_bitmap(last_pfn, pfn, true, dump_bitmap_buf, s);
if (ret < 0) {
- dump_error(s, "dump: failed to set dump_bitmap", errp);
+ error_setg(errp, "dump: failed to set dump_bitmap");
goto out;
}
@@ -1081,7 +1068,7 @@ static void write_dump_bitmap(DumpState *s, Error **errp)
ret = set_dump_bitmap(last_pfn, last_pfn + PFN_BUFBITMAP, false,
dump_bitmap_buf, s);
if (ret < 0) {
- dump_error(s, "dump: failed to sync dump_bitmap", errp);
+ error_setg(errp, "dump: failed to sync dump_bitmap");
goto out;
}
}
@@ -1214,7 +1201,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
ret = write_cache(&page_data, buf, TARGET_PAGE_SIZE, false);
g_free(buf);
if (ret < 0) {
- dump_error(s, "dump: failed to write page data (zero page)", errp);
+ error_setg(errp, "dump: failed to write page data (zero page)");
goto out;
}
@@ -1230,7 +1217,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
ret = write_cache(&page_desc, &pd_zero, sizeof(PageDescriptor),
false);
if (ret < 0) {
- dump_error(s, "dump: failed to write page desc", errp);
+ error_setg(errp, "dump: failed to write page desc");
goto out;
}
} else {
@@ -1255,7 +1242,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
ret = write_cache(&page_data, buf_out, size_out, false);
if (ret < 0) {
- dump_error(s, "dump: failed to write page data", errp);
+ error_setg(errp, "dump: failed to write page data");
goto out;
}
#ifdef CONFIG_LZO
@@ -1268,7 +1255,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
ret = write_cache(&page_data, buf_out, size_out, false);
if (ret < 0) {
- dump_error(s, "dump: failed to write page data", errp);
+ error_setg(errp, "dump: failed to write page data");
goto out;
}
#endif
@@ -1282,7 +1269,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
ret = write_cache(&page_data, buf_out, size_out, false);
if (ret < 0) {
- dump_error(s, "dump: failed to write page data", errp);
+ error_setg(errp, "dump: failed to write page data");
goto out;
}
#endif
@@ -1297,7 +1284,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
ret = write_cache(&page_data, buf, TARGET_PAGE_SIZE, false);
if (ret < 0) {
- dump_error(s, "dump: failed to write page data", errp);
+ error_setg(errp, "dump: failed to write page data");
goto out;
}
}
@@ -1309,7 +1296,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
ret = write_cache(&page_desc, &pd, sizeof(PageDescriptor), false);
if (ret < 0) {
- dump_error(s, "dump: failed to write page desc", errp);
+ error_setg(errp, "dump: failed to write page desc");
goto out;
}
}
@@ -1317,12 +1304,12 @@ static void write_dump_pages(DumpState *s, Error **errp)
ret = write_cache(&page_desc, NULL, 0, true);
if (ret < 0) {
- dump_error(s, "dump: failed to sync cache for page_desc", errp);
+ error_setg(errp, "dump: failed to sync cache for page_desc");
goto out;
}
ret = write_cache(&page_data, NULL, 0, true);
if (ret < 0) {
- dump_error(s, "dump: failed to sync cache for page_data", errp);
+ error_setg(errp, "dump: failed to sync cache for page_data");
goto out;
}
@@ -1366,7 +1353,7 @@ static void create_kdump_vmcore(DumpState *s, Error **errp)
ret = write_start_flat_header(s->fd);
if (ret < 0) {
- dump_error(s, "dump: failed to write start flat header", errp);
+ error_setg(errp, "dump: failed to write start flat header");
return;
}
@@ -1390,11 +1377,9 @@ static void create_kdump_vmcore(DumpState *s, Error **errp)
ret = write_end_flat_header(s->fd);
if (ret < 0) {
- dump_error(s, "dump: failed to write end flat header", errp);
+ error_setg(errp, "dump: failed to write end flat header");
return;
}
-
- dump_completed(s);
}
static ram_addr_t get_start_block(DumpState *s)
@@ -1677,6 +1662,7 @@ void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
create_vmcore(s, errp);
}
+ dump_cleanup(s);
g_free(s);
}
--
2.4.3
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/8] dump-guest-memory: cleanup: removing dump_{error|cleanup}().
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 1/8] dump-guest-memory: cleanup: removing dump_{error|cleanup}() Peter Xu
@ 2015-11-27 4:28 ` Fam Zheng
2015-11-27 6:51 ` Peter Xu
0 siblings, 1 reply; 43+ messages in thread
From: Fam Zheng @ 2015-11-27 4:28 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel
On Fri, 11/27 10:48, Peter Xu wrote:
> It might be a little bit confusing to do dump_cleanup() in these two
> functions (and error prone, please see section below). A better way
> is to do dump_cleanup() before dump finish, no matter whether dump
> has succeeded or not.
>
> Meanwhile, this patch will also fix the potential memory leaks that
> might happen when dump got errors within the process (e.g., when
> write_dump_header() fails, it will skip calling dump_cleanup(),
> which will leads to memory leak for list elements in DumpState).
I think when write_dump_header() returns a failure, it does call
dump_cleanup(), in create_header{32,64}.
But the changes of code in this patch look sane to me, and the new error
handling is definitely looking better.
Fam
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> dump.c | 78 +++++++++++++++++++++++++++---------------------------------------
> 1 file changed, 32 insertions(+), 46 deletions(-)
>
> diff --git a/dump.c b/dump.c
> index 78b7d84..445e739 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -82,12 +82,6 @@ static int dump_cleanup(DumpState *s)
> return 0;
> }
>
> -static void dump_error(DumpState *s, const char *reason, Error **errp)
> -{
> - dump_cleanup(s);
> - error_setg(errp, "%s", reason);
> -}
> -
> static int fd_write_vmcore(const void *buf, size_t size, void *opaque)
> {
> DumpState *s = opaque;
> @@ -128,7 +122,7 @@ static void write_elf64_header(DumpState *s, Error **errp)
>
> ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
> if (ret < 0) {
> - dump_error(s, "dump: failed to write elf header", errp);
> + error_setg(errp, "dump: failed to write elf header");
> }
> }
>
> @@ -159,7 +153,7 @@ static void write_elf32_header(DumpState *s, Error **errp)
>
> ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s);
> if (ret < 0) {
> - dump_error(s, "dump: failed to write elf header", errp);
> + error_setg(errp, "dump: failed to write elf header");
> }
> }
>
> @@ -182,7 +176,7 @@ static void write_elf64_load(DumpState *s, MemoryMapping *memory_mapping,
>
> ret = fd_write_vmcore(&phdr, sizeof(Elf64_Phdr), s);
> if (ret < 0) {
> - dump_error(s, "dump: failed to write program header table", errp);
> + error_setg(errp, "dump: failed to write program header table");
> }
> }
>
> @@ -205,7 +199,7 @@ static void write_elf32_load(DumpState *s, MemoryMapping *memory_mapping,
>
> ret = fd_write_vmcore(&phdr, sizeof(Elf32_Phdr), s);
> if (ret < 0) {
> - dump_error(s, "dump: failed to write program header table", errp);
> + error_setg(errp, "dump: failed to write program header table");
> }
> }
>
> @@ -225,7 +219,7 @@ static void write_elf64_note(DumpState *s, Error **errp)
>
> ret = fd_write_vmcore(&phdr, sizeof(Elf64_Phdr), s);
> if (ret < 0) {
> - dump_error(s, "dump: failed to write program header table", errp);
> + error_setg(errp, "dump: failed to write program header table");
> }
> }
>
> @@ -245,7 +239,7 @@ static void write_elf64_notes(WriteCoreDumpFunction f, DumpState *s,
> id = cpu_index(cpu);
> ret = cpu_write_elf64_note(f, cpu, id, s);
> if (ret < 0) {
> - dump_error(s, "dump: failed to write elf notes", errp);
> + error_setg(errp, "dump: failed to write elf notes");
> return;
> }
> }
> @@ -253,7 +247,7 @@ static void write_elf64_notes(WriteCoreDumpFunction f, DumpState *s,
> CPU_FOREACH(cpu) {
> ret = cpu_write_elf64_qemunote(f, cpu, s);
> if (ret < 0) {
> - dump_error(s, "dump: failed to write CPU status", errp);
> + error_setg(errp, "dump: failed to write CPU status");
> return;
> }
> }
> @@ -275,7 +269,7 @@ static void write_elf32_note(DumpState *s, Error **errp)
>
> ret = fd_write_vmcore(&phdr, sizeof(Elf32_Phdr), s);
> if (ret < 0) {
> - dump_error(s, "dump: failed to write program header table", errp);
> + error_setg(errp, "dump: failed to write program header table");
> }
> }
>
> @@ -290,7 +284,7 @@ static void write_elf32_notes(WriteCoreDumpFunction f, DumpState *s,
> id = cpu_index(cpu);
> ret = cpu_write_elf32_note(f, cpu, id, s);
> if (ret < 0) {
> - dump_error(s, "dump: failed to write elf notes", errp);
> + error_setg(errp, "dump: failed to write elf notes");
> return;
> }
> }
> @@ -298,7 +292,7 @@ static void write_elf32_notes(WriteCoreDumpFunction f, DumpState *s,
> CPU_FOREACH(cpu) {
> ret = cpu_write_elf32_qemunote(f, cpu, s);
> if (ret < 0) {
> - dump_error(s, "dump: failed to write CPU status", errp);
> + error_setg(errp, "dump: failed to write CPU status");
> return;
> }
> }
> @@ -326,7 +320,7 @@ static void write_elf_section(DumpState *s, int type, Error **errp)
>
> ret = fd_write_vmcore(&shdr, shdr_size, s);
> if (ret < 0) {
> - dump_error(s, "dump: failed to write section header table", errp);
> + error_setg(errp, "dump: failed to write section header table");
> }
> }
>
> @@ -336,7 +330,7 @@ static void write_data(DumpState *s, void *buf, int length, Error **errp)
>
> ret = fd_write_vmcore(buf, length, s);
> if (ret < 0) {
> - dump_error(s, "dump: failed to save memory", errp);
> + error_setg(errp, "dump: failed to save memory");
> }
> }
>
> @@ -568,11 +562,6 @@ static void dump_begin(DumpState *s, Error **errp)
> }
> }
>
> -static void dump_completed(DumpState *s)
> -{
> - dump_cleanup(s);
> -}
> -
> static int get_next_block(DumpState *s, GuestPhysBlock *block)
> {
> while (1) {
> @@ -624,8 +613,6 @@ static void dump_iterate(DumpState *s, Error **errp)
> }
>
> } while (!get_next_block(s, block));
> -
> - dump_completed(s);
> }
>
> static void create_vmcore(DumpState *s, Error **errp)
> @@ -765,7 +752,7 @@ static void create_header32(DumpState *s, Error **errp)
> dh->status = cpu_to_dump32(s, status);
>
> if (write_buffer(s->fd, 0, dh, size) < 0) {
> - dump_error(s, "dump: failed to write disk dump header", errp);
> + error_setg(errp, "dump: failed to write disk dump header");
> goto out;
> }
>
> @@ -784,7 +771,7 @@ static void create_header32(DumpState *s, Error **errp)
>
> if (write_buffer(s->fd, DISKDUMP_HEADER_BLOCKS *
> block_size, kh, size) < 0) {
> - dump_error(s, "dump: failed to write kdump sub header", errp);
> + error_setg(errp, "dump: failed to write kdump sub header");
> goto out;
> }
>
> @@ -800,7 +787,7 @@ static void create_header32(DumpState *s, Error **errp)
> }
> if (write_buffer(s->fd, offset_note, s->note_buf,
> s->note_size) < 0) {
> - dump_error(s, "dump: failed to write notes", errp);
> + error_setg(errp, "dump: failed to write notes");
> goto out;
> }
>
> @@ -865,7 +852,7 @@ static void create_header64(DumpState *s, Error **errp)
> dh->status = cpu_to_dump32(s, status);
>
> if (write_buffer(s->fd, 0, dh, size) < 0) {
> - dump_error(s, "dump: failed to write disk dump header", errp);
> + error_setg(errp, "dump: failed to write disk dump header");
> goto out;
> }
>
> @@ -884,7 +871,7 @@ static void create_header64(DumpState *s, Error **errp)
>
> if (write_buffer(s->fd, DISKDUMP_HEADER_BLOCKS *
> block_size, kh, size) < 0) {
> - dump_error(s, "dump: failed to write kdump sub header", errp);
> + error_setg(errp, "dump: failed to write kdump sub header");
> goto out;
> }
>
> @@ -901,7 +888,7 @@ static void create_header64(DumpState *s, Error **errp)
>
> if (write_buffer(s->fd, offset_note, s->note_buf,
> s->note_size) < 0) {
> - dump_error(s, "dump: failed to write notes", errp);
> + error_setg(errp, "dump: failed to write notes");
> goto out;
> }
>
> @@ -1064,7 +1051,7 @@ static void write_dump_bitmap(DumpState *s, Error **errp)
> while (get_next_page(&block_iter, &pfn, NULL, s)) {
> ret = set_dump_bitmap(last_pfn, pfn, true, dump_bitmap_buf, s);
> if (ret < 0) {
> - dump_error(s, "dump: failed to set dump_bitmap", errp);
> + error_setg(errp, "dump: failed to set dump_bitmap");
> goto out;
> }
>
> @@ -1081,7 +1068,7 @@ static void write_dump_bitmap(DumpState *s, Error **errp)
> ret = set_dump_bitmap(last_pfn, last_pfn + PFN_BUFBITMAP, false,
> dump_bitmap_buf, s);
> if (ret < 0) {
> - dump_error(s, "dump: failed to sync dump_bitmap", errp);
> + error_setg(errp, "dump: failed to sync dump_bitmap");
> goto out;
> }
> }
> @@ -1214,7 +1201,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
> ret = write_cache(&page_data, buf, TARGET_PAGE_SIZE, false);
> g_free(buf);
> if (ret < 0) {
> - dump_error(s, "dump: failed to write page data (zero page)", errp);
> + error_setg(errp, "dump: failed to write page data (zero page)");
> goto out;
> }
>
> @@ -1230,7 +1217,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
> ret = write_cache(&page_desc, &pd_zero, sizeof(PageDescriptor),
> false);
> if (ret < 0) {
> - dump_error(s, "dump: failed to write page desc", errp);
> + error_setg(errp, "dump: failed to write page desc");
> goto out;
> }
> } else {
> @@ -1255,7 +1242,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
>
> ret = write_cache(&page_data, buf_out, size_out, false);
> if (ret < 0) {
> - dump_error(s, "dump: failed to write page data", errp);
> + error_setg(errp, "dump: failed to write page data");
> goto out;
> }
> #ifdef CONFIG_LZO
> @@ -1268,7 +1255,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
>
> ret = write_cache(&page_data, buf_out, size_out, false);
> if (ret < 0) {
> - dump_error(s, "dump: failed to write page data", errp);
> + error_setg(errp, "dump: failed to write page data");
> goto out;
> }
> #endif
> @@ -1282,7 +1269,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
>
> ret = write_cache(&page_data, buf_out, size_out, false);
> if (ret < 0) {
> - dump_error(s, "dump: failed to write page data", errp);
> + error_setg(errp, "dump: failed to write page data");
> goto out;
> }
> #endif
> @@ -1297,7 +1284,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
>
> ret = write_cache(&page_data, buf, TARGET_PAGE_SIZE, false);
> if (ret < 0) {
> - dump_error(s, "dump: failed to write page data", errp);
> + error_setg(errp, "dump: failed to write page data");
> goto out;
> }
> }
> @@ -1309,7 +1296,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
>
> ret = write_cache(&page_desc, &pd, sizeof(PageDescriptor), false);
> if (ret < 0) {
> - dump_error(s, "dump: failed to write page desc", errp);
> + error_setg(errp, "dump: failed to write page desc");
> goto out;
> }
> }
> @@ -1317,12 +1304,12 @@ static void write_dump_pages(DumpState *s, Error **errp)
>
> ret = write_cache(&page_desc, NULL, 0, true);
> if (ret < 0) {
> - dump_error(s, "dump: failed to sync cache for page_desc", errp);
> + error_setg(errp, "dump: failed to sync cache for page_desc");
> goto out;
> }
> ret = write_cache(&page_data, NULL, 0, true);
> if (ret < 0) {
> - dump_error(s, "dump: failed to sync cache for page_data", errp);
> + error_setg(errp, "dump: failed to sync cache for page_data");
> goto out;
> }
>
> @@ -1366,7 +1353,7 @@ static void create_kdump_vmcore(DumpState *s, Error **errp)
>
> ret = write_start_flat_header(s->fd);
> if (ret < 0) {
> - dump_error(s, "dump: failed to write start flat header", errp);
> + error_setg(errp, "dump: failed to write start flat header");
> return;
> }
>
> @@ -1390,11 +1377,9 @@ static void create_kdump_vmcore(DumpState *s, Error **errp)
>
> ret = write_end_flat_header(s->fd);
> if (ret < 0) {
> - dump_error(s, "dump: failed to write end flat header", errp);
> + error_setg(errp, "dump: failed to write end flat header");
> return;
> }
> -
> - dump_completed(s);
> }
>
> static ram_addr_t get_start_block(DumpState *s)
> @@ -1677,6 +1662,7 @@ void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
> create_vmcore(s, errp);
> }
>
> + dump_cleanup(s);
> g_free(s);
> }
>
> --
> 2.4.3
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/8] dump-guest-memory: cleanup: removing dump_{error|cleanup}().
2015-11-27 4:28 ` Fam Zheng
@ 2015-11-27 6:51 ` Peter Xu
0 siblings, 0 replies; 43+ messages in thread
From: Peter Xu @ 2015-11-27 6:51 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-devel
On 11/27/2015 12:28 PM, Fam Zheng wrote:
> I think when write_dump_header() returns a failure, it does call
> dump_cleanup(), in create_header{32,64}.
>
> But the changes of code in this patch look sane to me, and the new error
> handling is definitely looking better.
Yes, you are right. I will modify the commit message to correct
mentioning about memory leak, and keep the codes itself.
Thanks.
Peter
>
> Fam
^ permalink raw reply [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH v2 2/8] dump-guest-memory: add "detach" flag for QMP/HMP interfaces.
2015-11-27 2:48 [Qemu-devel] [PATCH v2 0/8] Add basic "detach" support for dump-guest-memory Peter Xu
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 1/8] dump-guest-memory: cleanup: removing dump_{error|cleanup}() Peter Xu
@ 2015-11-27 2:48 ` Peter Xu
2015-11-27 4:31 ` Fam Zheng
2015-11-30 18:21 ` Eric Blake
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support Peter Xu
` (7 subsequent siblings)
9 siblings, 2 replies; 43+ messages in thread
From: Peter Xu @ 2015-11-27 2:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Xu
This patch only adds the interfaces, but not implements them.
"detach" parameter is made optional, to make sure that all the old
dump-guest-memory requests will still be able to work.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
dump.c | 5 +++--
hmp-commands.hx | 5 +++--
hmp.c | 9 +++++++--
qapi-schema.json | 8 ++++++--
qmp-commands.hx | 6 ++++--
5 files changed, 23 insertions(+), 10 deletions(-)
diff --git a/dump.c b/dump.c
index 445e739..d79e0ed 100644
--- a/dump.c
+++ b/dump.c
@@ -1580,8 +1580,9 @@ cleanup:
dump_cleanup(s);
}
-void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
- int64_t begin, bool has_length,
+void qmp_dump_guest_memory(bool paging, const char *file,
+ bool has_detach, bool detach,
+ bool has_begin, int64_t begin, bool has_length,
int64_t length, bool has_format,
DumpGuestMemoryFormat format, Error **errp)
{
diff --git a/hmp-commands.hx b/hmp-commands.hx
index bb52e4d..664d794 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 (do 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..dccb457 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1586,8 +1586,10 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
const char *file = qdict_get_str(qdict, "filename");
bool has_begin = qdict_haskey(qdict, "begin");
bool has_length = qdict_haskey(qdict, "length");
+ bool has_detach = qdict_haskey(qdict, "detach");
int64_t begin = 0;
int64_t length = 0;
+ bool detach = false;
enum DumpGuestMemoryFormat dump_format = DUMP_GUEST_MEMORY_FORMAT_ELF;
char *prot;
@@ -1615,11 +1617,14 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
if (has_length) {
length = qdict_get_int(qdict, "length");
}
+ if (has_detach) {
+ detach = qdict_get_try_bool(qdict, "detach", false);
+ }
prot = g_strconcat("file:", file, NULL);
- qmp_dump_guest_memory(paging, prot, has_begin, begin, has_length, length,
- true, dump_format, &err);
+ qmp_dump_guest_memory(paging, prot, has_detach, detach, has_begin, begin,
+ has_length, length, true, dump_format, &err);
hmp_handle_error(mon, &err);
g_free(prot);
}
diff --git a/qapi-schema.json b/qapi-schema.json
index 8b1a423..fd81ce2 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2115,6 +2115,9 @@
# 2. fd: the protocol starts with "fd:", and the following string
# is the fd's name.
#
+# @detach: #optional if true, QMP will return immediately rather than
+# waiting dump to be finished (since 2.6).
+#
# @begin: #optional if specified, the starting physical address.
#
# @length: #optional if specified, the memory size, in bytes. If you don't
@@ -2131,8 +2134,9 @@
# Since: 1.2
##
{ 'command': 'dump-guest-memory',
- 'data': { 'paging': 'bool', 'protocol': 'str', '*begin': 'int',
- '*length': 'int', '*format': 'DumpGuestMemoryFormat' } }
+ 'data': { 'paging': 'bool', 'protocol': 'str', '*detach': 'bool',
+ '*begin': 'int', '*length': 'int',
+ '*format': 'DumpGuestMemoryFormat'} }
##
# @DumpGuestMemoryCapability:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 9d8b42f..bbb08e1 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,protocol:s,detach:b?,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,
},
@@ -857,6 +857,8 @@ Arguments:
- "paging": do paging to get guest's memory mapping (json-bool)
- "protocol": destination file(started with "file:") or destination file
descriptor (started with "fd:") (json-string)
+- "detach": if specificed, command will return immediately, without waiting
+ for dump to be finished (json-bool)
- "begin": the starting physical address. It's optional, and should be specified
with length together (json-int)
- "length": the memory size, in bytes. It's optional, and should be specified
--
2.4.3
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/8] dump-guest-memory: add "detach" flag for QMP/HMP interfaces.
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 2/8] dump-guest-memory: add "detach" flag for QMP/HMP interfaces Peter Xu
@ 2015-11-27 4:31 ` Fam Zheng
2015-11-27 6:05 ` Peter Xu
2015-11-30 18:21 ` Eric Blake
1 sibling, 1 reply; 43+ messages in thread
From: Fam Zheng @ 2015-11-27 4:31 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel
On Fri, 11/27 10:48, Peter Xu wrote:
> This patch only adds the interfaces, but not implements them.
> "detach" parameter is made optional, to make sure that all the old
> dump-guest-memory requests will still be able to work.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> dump.c | 5 +++--
> hmp-commands.hx | 5 +++--
> hmp.c | 9 +++++++--
> qapi-schema.json | 8 ++++++--
> qmp-commands.hx | 6 ++++--
> 5 files changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/dump.c b/dump.c
> index 445e739..d79e0ed 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -1580,8 +1580,9 @@ cleanup:
> dump_cleanup(s);
> }
>
> -void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
> - int64_t begin, bool has_length,
> +void qmp_dump_guest_memory(bool paging, const char *file,
> + bool has_detach, bool detach,
> + bool has_begin, int64_t begin, bool has_length,
> int64_t length, bool has_format,
> DumpGuestMemoryFormat format, Error **errp)
> {
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index bb52e4d..664d794 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 (do 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..dccb457 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1586,8 +1586,10 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
> const char *file = qdict_get_str(qdict, "filename");
> bool has_begin = qdict_haskey(qdict, "begin");
> bool has_length = qdict_haskey(qdict, "length");
> + bool has_detach = qdict_haskey(qdict, "detach");
> int64_t begin = 0;
> int64_t length = 0;
> + bool detach = false;
> enum DumpGuestMemoryFormat dump_format = DUMP_GUEST_MEMORY_FORMAT_ELF;
> char *prot;
>
> @@ -1615,11 +1617,14 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
> if (has_length) {
> length = qdict_get_int(qdict, "length");
> }
> + if (has_detach) {
> + detach = qdict_get_try_bool(qdict, "detach", false);
> + }
>
> prot = g_strconcat("file:", file, NULL);
>
> - qmp_dump_guest_memory(paging, prot, has_begin, begin, has_length, length,
> - true, dump_format, &err);
> + qmp_dump_guest_memory(paging, prot, has_detach, detach, has_begin, begin,
> + has_length, length, true, dump_format, &err);
> hmp_handle_error(mon, &err);
> g_free(prot);
> }
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 8b1a423..fd81ce2 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2115,6 +2115,9 @@
> # 2. fd: the protocol starts with "fd:", and the following string
> # is the fd's name.
> #
> +# @detach: #optional if true, QMP will return immediately rather than
> +# waiting dump to be finished (since 2.6).
> +#
Is it better to mention the related query command and events here...
> # @begin: #optional if specified, the starting physical address.
> #
> # @length: #optional if specified, the memory size, in bytes. If you don't
> @@ -2131,8 +2134,9 @@
> # Since: 1.2
> ##
> { 'command': 'dump-guest-memory',
> - 'data': { 'paging': 'bool', 'protocol': 'str', '*begin': 'int',
> - '*length': 'int', '*format': 'DumpGuestMemoryFormat' } }
> + 'data': { 'paging': 'bool', 'protocol': 'str', '*detach': 'bool',
> + '*begin': 'int', '*length': 'int',
> + '*format': 'DumpGuestMemoryFormat'} }
>
> ##
> # @DumpGuestMemoryCapability:
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 9d8b42f..bbb08e1 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,protocol:s,detach:b?,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,
> },
> @@ -857,6 +857,8 @@ Arguments:
> - "paging": do paging to get guest's memory mapping (json-bool)
> - "protocol": destination file(started with "file:") or destination file
> descriptor (started with "fd:") (json-string)
> +- "detach": if specificed, command will return immediately, without waiting
> + for dump to be finished (json-bool)
... and here?
Fam
> - "begin": the starting physical address. It's optional, and should be specified
> with length together (json-int)
> - "length": the memory size, in bytes. It's optional, and should be specified
> --
> 2.4.3
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/8] dump-guest-memory: add "detach" flag for QMP/HMP interfaces.
2015-11-27 4:31 ` Fam Zheng
@ 2015-11-27 6:05 ` Peter Xu
0 siblings, 0 replies; 43+ messages in thread
From: Peter Xu @ 2015-11-27 6:05 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-devel
On 11/27/2015 12:31 PM, Fam Zheng wrote:
> On Fri, 11/27 10:48, Peter Xu wrote:
>> This patch only adds the interfaces, but not implements them.
>> "detach" parameter is made optional, to make sure that all the old
>> dump-guest-memory requests will still be able to work.
>>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> ---
>> dump.c | 5 +++--
>> hmp-commands.hx | 5 +++--
>> hmp.c | 9 +++++++--
>> qapi-schema.json | 8 ++++++--
>> qmp-commands.hx | 6 ++++--
>> 5 files changed, 23 insertions(+), 10 deletions(-)
>>
>> diff --git a/dump.c b/dump.c
>> index 445e739..d79e0ed 100644
>> --- a/dump.c
>> +++ b/dump.c
>> @@ -1580,8 +1580,9 @@ cleanup:
>> dump_cleanup(s);
>> }
>>
>> -void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
>> - int64_t begin, bool has_length,
>> +void qmp_dump_guest_memory(bool paging, const char *file,
>> + bool has_detach, bool detach,
>> + bool has_begin, int64_t begin, bool has_length,
>> int64_t length, bool has_format,
>> DumpGuestMemoryFormat format, Error **errp)
>> {
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index bb52e4d..664d794 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 (do 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..dccb457 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -1586,8 +1586,10 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
>> const char *file = qdict_get_str(qdict, "filename");
>> bool has_begin = qdict_haskey(qdict, "begin");
>> bool has_length = qdict_haskey(qdict, "length");
>> + bool has_detach = qdict_haskey(qdict, "detach");
>> int64_t begin = 0;
>> int64_t length = 0;
>> + bool detach = false;
>> enum DumpGuestMemoryFormat dump_format = DUMP_GUEST_MEMORY_FORMAT_ELF;
>> char *prot;
>>
>> @@ -1615,11 +1617,14 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
>> if (has_length) {
>> length = qdict_get_int(qdict, "length");
>> }
>> + if (has_detach) {
>> + detach = qdict_get_try_bool(qdict, "detach", false);
>> + }
>>
>> prot = g_strconcat("file:", file, NULL);
>>
>> - qmp_dump_guest_memory(paging, prot, has_begin, begin, has_length, length,
>> - true, dump_format, &err);
>> + qmp_dump_guest_memory(paging, prot, has_detach, detach, has_begin, begin,
>> + has_length, length, true, dump_format, &err);
>> hmp_handle_error(mon, &err);
>> g_free(prot);
>> }
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 8b1a423..fd81ce2 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -2115,6 +2115,9 @@
>> # 2. fd: the protocol starts with "fd:", and the following string
>> # is the fd's name.
>> #
>> +# @detach: #optional if true, QMP will return immediately rather than
>> +# waiting dump to be finished (since 2.6).
>> +#
>
> Is it better to mention the related query command and events here...
Yes, will add more documents in v3.
>
>> # @begin: #optional if specified, the starting physical address.
>> #
>> # @length: #optional if specified, the memory size, in bytes. If you don't
>> @@ -2131,8 +2134,9 @@
>> # Since: 1.2
>> ##
>> { 'command': 'dump-guest-memory',
>> - 'data': { 'paging': 'bool', 'protocol': 'str', '*begin': 'int',
>> - '*length': 'int', '*format': 'DumpGuestMemoryFormat' } }
>> + 'data': { 'paging': 'bool', 'protocol': 'str', '*detach': 'bool',
>> + '*begin': 'int', '*length': 'int',
>> + '*format': 'DumpGuestMemoryFormat'} }
>>
>> ##
>> # @DumpGuestMemoryCapability:
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 9d8b42f..bbb08e1 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,protocol:s,detach:b?,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,
>> },
>> @@ -857,6 +857,8 @@ Arguments:
>> - "paging": do paging to get guest's memory mapping (json-bool)
>> - "protocol": destination file(started with "file:") or destination file
>> descriptor (started with "fd:") (json-string)
>> +- "detach": if specificed, command will return immediately, without waiting
>> + for dump to be finished (json-bool)
>
> ... and here?
Here too. :)
Thanks.
Peter
>
> Fam
>
>> - "begin": the starting physical address. It's optional, and should be specified
>> with length together (json-int)
>> - "length": the memory size, in bytes. It's optional, and should be specified
>> --
>> 2.4.3
>>
>>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/8] dump-guest-memory: add "detach" flag for QMP/HMP interfaces.
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 2/8] dump-guest-memory: add "detach" flag for QMP/HMP interfaces Peter Xu
2015-11-27 4:31 ` Fam Zheng
@ 2015-11-30 18:21 ` Eric Blake
2015-12-01 1:28 ` Peter Xu
1 sibling, 1 reply; 43+ messages in thread
From: Eric Blake @ 2015-11-30 18:21 UTC (permalink / raw)
To: Peter Xu, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2054 bytes --]
On 11/26/2015 07:48 PM, Peter Xu wrote:
> This patch only adds the interfaces, but not implements them.
s/not implements/does not implement/
> "detach" parameter is made optional, to make sure that all the old
> dump-guest-memory requests will still be able to work.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
In addition to Fam's comments,
> +++ b/qapi-schema.json
> @@ -2115,6 +2115,9 @@
> # 2. fd: the protocol starts with "fd:", and the following string
> # is the fd's name.
> #
> +# @detach: #optional if true, QMP will return immediately rather than
> +# waiting dump to be finished (since 2.6).
s/waiting/waiting for the/
s/be finished/finish/
> +++ 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,protocol:s,detach:b?,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,
> },
> @@ -857,6 +857,8 @@ Arguments:
> - "paging": do paging to get guest's memory mapping (json-bool)
> - "protocol": destination file(started with "file:") or destination file
> descriptor (started with "fd:") (json-string)
> +- "detach": if specificed, command will return immediately, without waiting
s/specificed/specified/
> + for dump to be finished (json-bool)
s/dump to be finished/the dump to finish/
> - "begin": the starting physical address. It's optional, and should be specified
> with length together (json-int)
> - "length": the memory size, in bytes. It's optional, and should be specified
>
--
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 --]
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/8] dump-guest-memory: add "detach" flag for QMP/HMP interfaces.
2015-11-30 18:21 ` Eric Blake
@ 2015-12-01 1:28 ` Peter Xu
0 siblings, 0 replies; 43+ messages in thread
From: Peter Xu @ 2015-12-01 1:28 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
On Mon, Nov 30, 2015 at 11:21:23AM -0700, Eric Blake wrote:
> On 11/26/2015 07:48 PM, Peter Xu wrote:
> > This patch only adds the interfaces, but not implements them.
>
> s/not implements/does not implement/
>
> > "detach" parameter is made optional, to make sure that all the old
> > dump-guest-memory requests will still be able to work.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
>
> In addition to Fam's comments,
>
> > +++ b/qapi-schema.json
> > @@ -2115,6 +2115,9 @@
> > # 2. fd: the protocol starts with "fd:", and the following string
> > # is the fd's name.
> > #
> > +# @detach: #optional if true, QMP will return immediately rather than
> > +# waiting dump to be finished (since 2.6).
>
> s/waiting/waiting for the/
> s/be finished/finish/
>
> > +++ 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,protocol:s,detach:b?,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,
> > },
> > @@ -857,6 +857,8 @@ Arguments:
> > - "paging": do paging to get guest's memory mapping (json-bool)
> > - "protocol": destination file(started with "file:") or destination file
> > descriptor (started with "fd:") (json-string)
> > +- "detach": if specificed, command will return immediately, without waiting
>
> s/specificed/specified/
>
> > + for dump to be finished (json-bool)
>
> s/dump to be finished/the dump to finish/
Thanks for the corrections. These errors exist in v3 too. Will fix
them in v4.
Peter
>
> > - "begin": the starting physical address. It's optional, and should be specified
> > with length together (json-int)
> > - "length": the memory size, in bytes. It's optional, and should be specified
> >
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.
2015-11-27 2:48 [Qemu-devel] [PATCH v2 0/8] Add basic "detach" support for dump-guest-memory Peter Xu
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 1/8] dump-guest-memory: cleanup: removing dump_{error|cleanup}() Peter Xu
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 2/8] dump-guest-memory: add "detach" flag for QMP/HMP interfaces Peter Xu
@ 2015-11-27 2:48 ` Peter Xu
2015-11-27 5:14 ` Fam Zheng
2015-11-27 10:31 ` Paolo Bonzini
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 4/8] dump-guest-memory: add qmp event DUMP_COMPLETED Peter Xu
` (6 subsequent siblings)
9 siblings, 2 replies; 43+ messages in thread
From: Peter Xu @ 2015-11-27 2:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Xu
This patch implements "detach" for "dump-guest-memory" command. When
specified, one background thread is created to do the dump work.
When there is a dump running in background, the commands "stop",
"cont" and "dump-guest-memory" will fail directly, with a hint
telling user: "there is a dump in progress".
Although it is not quite essential for now, the new codes are trying
to make sure the dump is thread safe. To do this, one list is added
into GuestPhysBlockList to track all the referenced MemoryRegions
during dump process. We should make sure each MemoryRegion would
only be referenced for once.
One global struct "GlobalDumpState" is added to store some global
informations for dump procedures. One mutex is used to protect the
global dump info.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
dump.c | 114 +++++++++++++++++++++++++++++++++++++---
include/qemu-common.h | 4 ++
include/sysemu/dump.h | 10 ++++
include/sysemu/memory_mapping.h | 8 +++
memory_mapping.c | 46 +++++++++++++++-
qmp.c | 14 +++++
6 files changed, 188 insertions(+), 8 deletions(-)
diff --git a/dump.c b/dump.c
index d79e0ed..dfd56aa 100644
--- a/dump.c
+++ b/dump.c
@@ -73,6 +73,7 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
static int dump_cleanup(DumpState *s)
{
guest_phys_blocks_free(&s->guest_phys_blocks);
+ guest_memory_region_free(&s->guest_phys_blocks);
memory_mapping_list_free(&s->list);
close(s->fd);
if (s->resume) {
@@ -1427,6 +1428,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);
@@ -1580,6 +1584,86 @@ cleanup:
dump_cleanup(s);
}
+extern GlobalDumpState global_dump_state;
+
+static GlobalDumpState *dump_state_get_global(void)
+{
+ static bool init = false;
+ static GlobalDumpState global_dump_state;
+ if (unlikely(!init)) {
+ qemu_mutex_init(&global_dump_state.gds_mutex);
+ global_dump_state.gds_cur = NULL;
+ init = true;
+ }
+ return &global_dump_state;
+}
+
+/* Returns non-zero if there is existing dump in progress, otherwise
+ * zero is returned. */
+bool dump_in_progress(void)
+{
+ GlobalDumpState *global = dump_state_get_global();
+ /* we do not need to take the mutex here if we are checking the
+ * pointer only. */
+ return (!!global->gds_cur);
+}
+
+/* Trying to create one DumpState. This will fail if there is an
+ * existing one. Return DumpState pointer if succeeded, otherwise
+ * NULL is returned. */
+static DumpState *dump_state_create(GlobalDumpState *global)
+{
+ DumpState *cur = NULL;
+ qemu_mutex_lock(&global->gds_mutex);
+
+ cur = global->gds_cur;
+ if (cur) {
+ /* one dump in progress */
+ cur = NULL;
+ } else {
+ /* we are the first! */
+ cur = g_malloc0(sizeof(*cur));
+ global->gds_cur = cur;
+ }
+
+ qemu_mutex_unlock(&global->gds_mutex);
+ return cur;
+}
+
+/* release current DumpState structure */
+static void dump_state_release(GlobalDumpState *global)
+{
+ DumpState *cur = NULL;
+ qemu_mutex_lock(&global->gds_mutex);
+
+ cur = global->gds_cur;
+ /* we should never call release before having one */
+ assert(cur);
+ global->gds_cur = NULL;
+
+ qemu_mutex_unlock(&global->gds_mutex);
+ dump_cleanup(cur);
+ g_free(cur);
+}
+
+/* this operation might be time consuming. */
+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);
+ }
+}
+
+static void *dump_thread(void *data)
+{
+ GlobalDumpState *global = (GlobalDumpState *)data;
+ dump_process(global->gds_cur, NULL);
+ dump_state_release(global);
+ return NULL;
+}
+
void qmp_dump_guest_memory(bool paging, const char *file,
bool has_detach, bool detach,
bool has_begin, int64_t begin, bool has_length,
@@ -1590,6 +1674,14 @@ void qmp_dump_guest_memory(bool paging, const char *file,
int fd = -1;
DumpState *s;
Error *local_err = NULL;
+ /* by default, we are keeping the old style, which is sync dump */
+ bool detach_p = false;
+ GlobalDumpState *global = dump_state_get_global();
+
+ if (runstate_check(RUN_STATE_INMIGRATE)) {
+ error_setg(errp, "Dump not allowed during incoming migration.");
+ return;
+ }
/*
* kdump-compressed format need the whole memory dumped, so paging or
@@ -1609,6 +1701,9 @@ void qmp_dump_guest_memory(bool paging, const char *file,
error_setg(errp, QERR_MISSING_PARAMETER, "begin");
return;
}
+ if (has_detach) {
+ detach_p = detach;
+ }
/* check whether lzo/snappy is supported */
#ifndef CONFIG_LZO
@@ -1647,7 +1742,11 @@ void qmp_dump_guest_memory(bool paging, const char *file,
return;
}
- s = g_malloc0(sizeof(DumpState));
+ s = dump_state_create(global);
+ if (!s) {
+ error_setg(errp, "One dump is in progress.");
+ return;
+ }
dump_init(s, fd, has_format, format, paging, has_begin,
begin, length, &local_err);
@@ -1657,14 +1756,15 @@ void qmp_dump_guest_memory(bool paging, const char *file,
return;
}
- if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
- create_kdump_vmcore(s, errp);
+ if (!detach_p) {
+ /* sync dump */
+ dump_process(s, errp);
+ dump_state_release(global);
} else {
- create_vmcore(s, errp);
+ qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
+ global, QEMU_THREAD_DETACHED);
+ /* DumpState is freed in dump thread */
}
-
- dump_cleanup(s);
- g_free(s);
}
DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
diff --git a/include/qemu-common.h b/include/qemu-common.h
index 405364f..7b74961 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -501,4 +501,8 @@ int parse_debug_env(const char *name, int max, int initial);
const char *qemu_ether_ntoa(const MACAddr *mac);
void page_size_init(void);
+/* returns non-zero if dump is in progress, otherwise zero is
+ * returned. */
+bool dump_in_progress(void);
+
#endif
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 7e4ec5c..13c913e 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -183,9 +183,19 @@ 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;
+typedef struct GlobalDumpState {
+ QemuMutex gds_mutex; /* protector for GlobalDumpState itself */
+ DumpState *gds_cur; /* current DumpState (might be NULL) */
+} GlobalDumpState;
+
uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
uint32_t cpu_to_dump32(DumpState *s, uint32_t val);
uint64_t cpu_to_dump64(DumpState *s, uint64_t val);
+
#endif
diff --git a/include/sysemu/memory_mapping.h b/include/sysemu/memory_mapping.h
index a75d59a..dd56fc7 100644
--- a/include/sysemu/memory_mapping.h
+++ b/include/sysemu/memory_mapping.h
@@ -30,10 +30,17 @@ typedef struct GuestPhysBlock {
QTAILQ_ENTRY(GuestPhysBlock) next;
} GuestPhysBlock;
+typedef struct GuestMemoryRegion {
+ MemoryRegion *gmr_mr;
+ QTAILQ_ENTRY(GuestMemoryRegion) next;
+} GuestMemoryRegion;
+
/* point-in-time snapshot of guest-visible physical mappings */
typedef struct GuestPhysBlockList {
unsigned num;
QTAILQ_HEAD(GuestPhysBlockHead, GuestPhysBlock) head;
+ /* housekeep all the MemoryRegion that is referenced */
+ QTAILQ_HEAD(GuestMemRegionHead, GuestMemoryRegion) head_mr;
} GuestPhysBlockList;
/* The physical and virtual address in the memory mapping are contiguous. */
@@ -65,6 +72,7 @@ void memory_mapping_list_free(MemoryMappingList *list);
void memory_mapping_list_init(MemoryMappingList *list);
void guest_phys_blocks_free(GuestPhysBlockList *list);
+void guest_memory_region_free(GuestPhysBlockList *list);
void guest_phys_blocks_init(GuestPhysBlockList *list);
void guest_phys_blocks_append(GuestPhysBlockList *list);
diff --git a/memory_mapping.c b/memory_mapping.c
index 36d6b26..a279552 100644
--- a/memory_mapping.c
+++ b/memory_mapping.c
@@ -186,6 +186,7 @@ void guest_phys_blocks_init(GuestPhysBlockList *list)
{
list->num = 0;
QTAILQ_INIT(&list->head);
+ QTAILQ_INIT(&list->head_mr);
}
typedef struct GuestPhysListener {
@@ -193,6 +194,39 @@ typedef struct GuestPhysListener {
MemoryListener listener;
} GuestPhysListener;
+static void guest_memory_region_add(GuestPhysBlockList *list,
+ MemoryRegion *mr)
+{
+ GuestMemoryRegion *gmr = NULL;
+ QTAILQ_FOREACH(gmr, &list->head_mr, next) {
+ if (gmr->gmr_mr == mr) {
+ /* we already refererenced the MemoryRegion */
+ return;
+ }
+ }
+ /* reference the MemoryRegion to make sure it's valid during dump */
+#ifdef DEBUG_DUMP_GUEST_MEMORY
+ fprintf(stderr, "DUMP: ref mem region: %s\n", mr->name);
+#endif
+ memory_region_ref(mr);
+ gmr = g_malloc0(sizeof(*gmr));
+ gmr->gmr_mr = mr;
+ QTAILQ_INSERT_TAIL(&list->head_mr, gmr, next);
+}
+
+void guest_memory_region_free(GuestPhysBlockList *list)
+{
+ GuestMemoryRegion *gmr = NULL, *q = NULL;
+ QTAILQ_FOREACH_SAFE(gmr, &list->head_mr, next, q) {
+#ifdef DEBUG_DUMP_GUEST_MEMORY
+ fprintf(stderr, "DUMP: unref mem region: %s\n", gmr->gmr_mr->name);
+#endif
+ QTAILQ_REMOVE(&list->head_mr, gmr, next);
+ memory_region_unref(gmr->gmr_mr);
+ g_free(gmr);
+ }
+}
+
static void guest_phys_blocks_region_add(MemoryListener *listener,
MemoryRegionSection *section)
{
@@ -241,6 +275,10 @@ static void guest_phys_blocks_region_add(MemoryListener *listener,
block->target_end = target_end;
block->host_addr = host_addr;
+ /* keep all the MemoryRegion that is referenced by the dump
+ * process */
+ guest_memory_region_add(g->list, section->mr);
+
QTAILQ_INSERT_TAIL(&g->list->head, block, next);
++g->list->num;
} else {
@@ -250,7 +288,7 @@ static void guest_phys_blocks_region_add(MemoryListener *listener,
predecessor->target_end = target_end;
}
-#ifdef DEBUG_GUEST_PHYS_REGION_ADD
+#ifdef DEBUG_DUMP_GUEST_MEMORY
fprintf(stderr, "%s: target_start=" TARGET_FMT_plx " target_end="
TARGET_FMT_plx ": %s (count: %u)\n", __FUNCTION__, target_start,
target_end, predecessor ? "joined" : "added", g->list->num);
@@ -263,8 +301,14 @@ void guest_phys_blocks_append(GuestPhysBlockList *list)
g.list = list;
g.listener.region_add = &guest_phys_blocks_region_add;
+ /* We should make sure all the memory objects are valid during
+ * add dump regions. Before releasing it, we should also make
+ * sure all the MemoryRegions to be used during dump is
+ * referenced. */
+ rcu_read_lock();
memory_listener_register(&g.listener, &address_space_memory);
memory_listener_unregister(&g.listener);
+ rcu_read_unlock();
}
static CPUState *find_paging_enabled_cpu(CPUState *start_cpu)
diff --git a/qmp.c b/qmp.c
index 0a1fa19..055586d 100644
--- a/qmp.c
+++ b/qmp.c
@@ -102,6 +102,13 @@ void qmp_quit(Error **errp)
void qmp_stop(Error **errp)
{
+ /* if there is a dump in background, we should wait until the dump
+ * finished */
+ if (dump_in_progress()) {
+ error_setg(errp, "There is a dump in process, please wait.");
+ return;
+ }
+
if (runstate_check(RUN_STATE_INMIGRATE)) {
autostart = 0;
} else {
@@ -174,6 +181,13 @@ void qmp_cont(Error **errp)
BlockBackend *blk;
BlockDriverState *bs;
+ /* if there is a dump in background, we should wait until the dump
+ * finished */
+ if (dump_in_progress()) {
+ 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;
--
2.4.3
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support Peter Xu
@ 2015-11-27 5:14 ` Fam Zheng
2015-11-27 5:20 ` Fam Zheng
` (2 more replies)
2015-11-27 10:31 ` Paolo Bonzini
1 sibling, 3 replies; 43+ messages in thread
From: Fam Zheng @ 2015-11-27 5:14 UTC (permalink / raw)
To: Peter Xu; +Cc: pbonzini, qemu-devel
On Fri, 11/27 10:48, Peter Xu wrote:
> This patch implements "detach" for "dump-guest-memory" command. When
> specified, one background thread is created to do the dump work.
>
> When there is a dump running in background, the commands "stop",
> "cont" and "dump-guest-memory" will fail directly, with a hint
> telling user: "there is a dump in progress".
>
> Although it is not quite essential for now, the new codes are trying
> to make sure the dump is thread safe. To do this, one list is added
> into GuestPhysBlockList to track all the referenced MemoryRegions
> during dump process. We should make sure each MemoryRegion would
> only be referenced for once.
>
> One global struct "GlobalDumpState" is added to store some global
> informations for dump procedures. One mutex is used to protect the
> global dump info.
This patch doesn't handle the incoming migration case, i.e. when QEMU is
started with "-incoming", or "-incoming defer". Dump can go wrong when incoming
migration happens at the same time.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> dump.c | 114 +++++++++++++++++++++++++++++++++++++---
> include/qemu-common.h | 4 ++
> include/sysemu/dump.h | 10 ++++
> include/sysemu/memory_mapping.h | 8 +++
> memory_mapping.c | 46 +++++++++++++++-
> qmp.c | 14 +++++
> 6 files changed, 188 insertions(+), 8 deletions(-)
>
> diff --git a/dump.c b/dump.c
> index d79e0ed..dfd56aa 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -73,6 +73,7 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
> static int dump_cleanup(DumpState *s)
> {
> guest_phys_blocks_free(&s->guest_phys_blocks);
> + guest_memory_region_free(&s->guest_phys_blocks);
> memory_mapping_list_free(&s->list);
> close(s->fd);
> if (s->resume) {
> @@ -1427,6 +1428,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);
> @@ -1580,6 +1584,86 @@ cleanup:
> dump_cleanup(s);
> }
>
> +extern GlobalDumpState global_dump_state;
dump_state_get_global() returns a pointer to the local static variable, why do
you need this?
But what I really wonder is why a single boolean is not enough: the DumpState
pointer can be passed to dump_thread, so it doesn't need to be global, then you
don't need the mutex.
> +
> +static GlobalDumpState *dump_state_get_global(void)
> +{
> + static bool init = false;
> + static GlobalDumpState global_dump_state;
> + if (unlikely(!init)) {
> + qemu_mutex_init(&global_dump_state.gds_mutex);
> + global_dump_state.gds_cur = NULL;
> + init = true;
> + }
> + return &global_dump_state;
> +}
> +
> +/* Returns non-zero if there is existing dump in progress, otherwise
> + * zero is returned. */
> +bool dump_in_progress(void)
> +{
> + GlobalDumpState *global = dump_state_get_global();
> + /* we do not need to take the mutex here if we are checking the
> + * pointer only. */
> + return (!!global->gds_cur);
> +}
> +
> +/* Trying to create one DumpState. This will fail if there is an
> + * existing one. Return DumpState pointer if succeeded, otherwise
> + * NULL is returned. */
> +static DumpState *dump_state_create(GlobalDumpState *global)
> +{
> + DumpState *cur = NULL;
> + qemu_mutex_lock(&global->gds_mutex);
> +
> + cur = global->gds_cur;
> + if (cur) {
> + /* one dump in progress */
> + cur = NULL;
> + } else {
> + /* we are the first! */
> + cur = g_malloc0(sizeof(*cur));
> + global->gds_cur = cur;
> + }
> +
> + qemu_mutex_unlock(&global->gds_mutex);
> + return cur;
> +}
> +
> +/* release current DumpState structure */
> +static void dump_state_release(GlobalDumpState *global)
> +{
> + DumpState *cur = NULL;
> + qemu_mutex_lock(&global->gds_mutex);
> +
> + cur = global->gds_cur;
> + /* we should never call release before having one */
> + assert(cur);
> + global->gds_cur = NULL;
> +
> + qemu_mutex_unlock(&global->gds_mutex);
> + dump_cleanup(cur);
> + g_free(cur);
> +}
> +
> +/* this operation might be time consuming. */
> +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);
> + }
> +}
> +
> +static void *dump_thread(void *data)
> +{
> + GlobalDumpState *global = (GlobalDumpState *)data;
> + dump_process(global->gds_cur, NULL);
> + dump_state_release(global);
> + return NULL;
> +}
> +
> void qmp_dump_guest_memory(bool paging, const char *file,
> bool has_detach, bool detach,
> bool has_begin, int64_t begin, bool has_length,
> @@ -1590,6 +1674,14 @@ void qmp_dump_guest_memory(bool paging, const char *file,
> int fd = -1;
> DumpState *s;
> Error *local_err = NULL;
> + /* by default, we are keeping the old style, which is sync dump */
> + bool detach_p = false;
> + GlobalDumpState *global = dump_state_get_global();
> +
> + if (runstate_check(RUN_STATE_INMIGRATE)) {
> + error_setg(errp, "Dump not allowed during incoming migration.");
> + return;
> + }
>
> /*
> * kdump-compressed format need the whole memory dumped, so paging or
> @@ -1609,6 +1701,9 @@ void qmp_dump_guest_memory(bool paging, const char *file,
> error_setg(errp, QERR_MISSING_PARAMETER, "begin");
> return;
> }
> + if (has_detach) {
> + detach_p = detach;
> + }
>
> /* check whether lzo/snappy is supported */
> #ifndef CONFIG_LZO
> @@ -1647,7 +1742,11 @@ void qmp_dump_guest_memory(bool paging, const char *file,
> return;
> }
>
> - s = g_malloc0(sizeof(DumpState));
> + s = dump_state_create(global);
> + if (!s) {
> + error_setg(errp, "One dump is in progress.");
> + return;
> + }
>
> dump_init(s, fd, has_format, format, paging, has_begin,
> begin, length, &local_err);
> @@ -1657,14 +1756,15 @@ void qmp_dump_guest_memory(bool paging, const char *file,
> return;
> }
>
> - if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
> - create_kdump_vmcore(s, errp);
> + if (!detach_p) {
> + /* sync dump */
> + dump_process(s, errp);
> + dump_state_release(global);
> } else {
> - create_vmcore(s, errp);
> + qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
> + global, QEMU_THREAD_DETACHED);
> + /* DumpState is freed in dump thread */
> }
> -
> - dump_cleanup(s);
> - g_free(s);
> }
>
> DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 405364f..7b74961 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -501,4 +501,8 @@ int parse_debug_env(const char *name, int max, int initial);
> const char *qemu_ether_ntoa(const MACAddr *mac);
> void page_size_init(void);
>
> +/* returns non-zero if dump is in progress, otherwise zero is
> + * returned. */
> +bool dump_in_progress(void);
> +
> #endif
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index 7e4ec5c..13c913e 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -183,9 +183,19 @@ 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;
>
> +typedef struct GlobalDumpState {
> + QemuMutex gds_mutex; /* protector for GlobalDumpState itself */
> + DumpState *gds_cur; /* current DumpState (might be NULL) */
> +} GlobalDumpState;
> +
> uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
> uint32_t cpu_to_dump32(DumpState *s, uint32_t val);
> uint64_t cpu_to_dump64(DumpState *s, uint64_t val);
> +
> #endif
> diff --git a/include/sysemu/memory_mapping.h b/include/sysemu/memory_mapping.h
> index a75d59a..dd56fc7 100644
> --- a/include/sysemu/memory_mapping.h
> +++ b/include/sysemu/memory_mapping.h
> @@ -30,10 +30,17 @@ typedef struct GuestPhysBlock {
> QTAILQ_ENTRY(GuestPhysBlock) next;
> } GuestPhysBlock;
>
> +typedef struct GuestMemoryRegion {
> + MemoryRegion *gmr_mr;
> + QTAILQ_ENTRY(GuestMemoryRegion) next;
> +} GuestMemoryRegion;
> +
> /* point-in-time snapshot of guest-visible physical mappings */
> typedef struct GuestPhysBlockList {
> unsigned num;
> QTAILQ_HEAD(GuestPhysBlockHead, GuestPhysBlock) head;
> + /* housekeep all the MemoryRegion that is referenced */
> + QTAILQ_HEAD(GuestMemRegionHead, GuestMemoryRegion) head_mr;
> } GuestPhysBlockList;
>
> /* The physical and virtual address in the memory mapping are contiguous. */
> @@ -65,6 +72,7 @@ void memory_mapping_list_free(MemoryMappingList *list);
> void memory_mapping_list_init(MemoryMappingList *list);
>
> void guest_phys_blocks_free(GuestPhysBlockList *list);
> +void guest_memory_region_free(GuestPhysBlockList *list);
> void guest_phys_blocks_init(GuestPhysBlockList *list);
> void guest_phys_blocks_append(GuestPhysBlockList *list);
>
> diff --git a/memory_mapping.c b/memory_mapping.c
> index 36d6b26..a279552 100644
> --- a/memory_mapping.c
> +++ b/memory_mapping.c
> @@ -186,6 +186,7 @@ void guest_phys_blocks_init(GuestPhysBlockList *list)
> {
> list->num = 0;
> QTAILQ_INIT(&list->head);
> + QTAILQ_INIT(&list->head_mr);
> }
>
> typedef struct GuestPhysListener {
> @@ -193,6 +194,39 @@ typedef struct GuestPhysListener {
> MemoryListener listener;
> } GuestPhysListener;
>
> +static void guest_memory_region_add(GuestPhysBlockList *list,
> + MemoryRegion *mr)
> +{
> + GuestMemoryRegion *gmr = NULL;
> + QTAILQ_FOREACH(gmr, &list->head_mr, next) {
> + if (gmr->gmr_mr == mr) {
> + /* we already refererenced the MemoryRegion */
> + return;
> + }
This is O(N^2). I think it should be fine to skip this check and just grab the
reference. If that is the case, we can probably embed the mr pointer in
GuestPhysBlock. Paolo?
> + }
> + /* reference the MemoryRegion to make sure it's valid during dump */
> +#ifdef DEBUG_DUMP_GUEST_MEMORY
> + fprintf(stderr, "DUMP: ref mem region: %s\n", mr->name);
> +#endif
> + memory_region_ref(mr);
> + gmr = g_malloc0(sizeof(*gmr));
> + gmr->gmr_mr = mr;
> + QTAILQ_INSERT_TAIL(&list->head_mr, gmr, next);
> +}
> +
> +void guest_memory_region_free(GuestPhysBlockList *list)
> +{
> + GuestMemoryRegion *gmr = NULL, *q = NULL;
> + QTAILQ_FOREACH_SAFE(gmr, &list->head_mr, next, q) {
> +#ifdef DEBUG_DUMP_GUEST_MEMORY
> + fprintf(stderr, "DUMP: unref mem region: %s\n", gmr->gmr_mr->name);
> +#endif
> + QTAILQ_REMOVE(&list->head_mr, gmr, next);
> + memory_region_unref(gmr->gmr_mr);
> + g_free(gmr);
> + }
> +}
> +
> static void guest_phys_blocks_region_add(MemoryListener *listener,
> MemoryRegionSection *section)
> {
> @@ -241,6 +275,10 @@ static void guest_phys_blocks_region_add(MemoryListener *listener,
> block->target_end = target_end;
> block->host_addr = host_addr;
>
> + /* keep all the MemoryRegion that is referenced by the dump
> + * process */
> + guest_memory_region_add(g->list, section->mr);
> +
> QTAILQ_INSERT_TAIL(&g->list->head, block, next);
> ++g->list->num;
> } else {
> @@ -250,7 +288,7 @@ static void guest_phys_blocks_region_add(MemoryListener *listener,
> predecessor->target_end = target_end;
> }
>
> -#ifdef DEBUG_GUEST_PHYS_REGION_ADD
> +#ifdef DEBUG_DUMP_GUEST_MEMORY
> fprintf(stderr, "%s: target_start=" TARGET_FMT_plx " target_end="
> TARGET_FMT_plx ": %s (count: %u)\n", __FUNCTION__, target_start,
> target_end, predecessor ? "joined" : "added", g->list->num);
> @@ -263,8 +301,14 @@ void guest_phys_blocks_append(GuestPhysBlockList *list)
>
> g.list = list;
> g.listener.region_add = &guest_phys_blocks_region_add;
> + /* We should make sure all the memory objects are valid during
> + * add dump regions. Before releasing it, we should also make
> + * sure all the MemoryRegions to be used during dump is
> + * referenced. */
> + rcu_read_lock();
> memory_listener_register(&g.listener, &address_space_memory);
> memory_listener_unregister(&g.listener);
> + rcu_read_unlock();
I don't think this is necessary if VM is stopped and incoming migration is
not running - address_space_memory will be safe to access as usual. Otherwise
this is a separate problem.
> }
>
> static CPUState *find_paging_enabled_cpu(CPUState *start_cpu)
> diff --git a/qmp.c b/qmp.c
> index 0a1fa19..055586d 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -102,6 +102,13 @@ void qmp_quit(Error **errp)
>
> void qmp_stop(Error **errp)
> {
> + /* if there is a dump in background, we should wait until the dump
> + * finished */
> + if (dump_in_progress()) {
> + error_setg(errp, "There is a dump in process, please wait.");
> + return;
> + }
> +
> if (runstate_check(RUN_STATE_INMIGRATE)) {
> autostart = 0;
> } else {
> @@ -174,6 +181,13 @@ void qmp_cont(Error **errp)
> BlockBackend *blk;
> BlockDriverState *bs;
>
> + /* if there is a dump in background, we should wait until the dump
> + * finished */
> + if (dump_in_progress()) {
> + 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;
> --
> 2.4.3
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.
2015-11-27 5:14 ` Fam Zheng
@ 2015-11-27 5:20 ` Fam Zheng
2015-11-27 6:27 ` Peter Xu
2015-11-28 5:51 ` Peter Xu
2 siblings, 0 replies; 43+ messages in thread
From: Fam Zheng @ 2015-11-27 5:20 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel
On Fri, 11/27 13:14, Fam Zheng wrote:
> But what I really wonder is why a single boolean is not enough: the DumpState
> pointer can be passed to dump_thread, so it doesn't need to be global, then you
> don't need the mutex.
Never mind, it's useful in later patches.
Fam
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.
2015-11-27 5:14 ` Fam Zheng
2015-11-27 5:20 ` Fam Zheng
@ 2015-11-27 6:27 ` Peter Xu
2015-11-27 10:14 ` Paolo Bonzini
2015-11-28 5:51 ` Peter Xu
2 siblings, 1 reply; 43+ messages in thread
From: Peter Xu @ 2015-11-27 6:27 UTC (permalink / raw)
To: Fam Zheng; +Cc: pbonzini, qemu-devel
On 11/27/2015 01:14 PM, Fam Zheng wrote:
> On Fri, 11/27 10:48, Peter Xu wrote:
>> This patch implements "detach" for "dump-guest-memory" command. When
>> specified, one background thread is created to do the dump work.
>>
>> When there is a dump running in background, the commands "stop",
>> "cont" and "dump-guest-memory" will fail directly, with a hint
>> telling user: "there is a dump in progress".
>>
>> Although it is not quite essential for now, the new codes are trying
>> to make sure the dump is thread safe. To do this, one list is added
>> into GuestPhysBlockList to track all the referenced MemoryRegions
>> during dump process. We should make sure each MemoryRegion would
>> only be referenced for once.
>>
>> One global struct "GlobalDumpState" is added to store some global
>> informations for dump procedures. One mutex is used to protect the
>> global dump info.
>
> This patch doesn't handle the incoming migration case, i.e. when QEMU is
> started with "-incoming", or "-incoming defer". Dump can go wrong when incoming
> migration happens at the same time.
>
>>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> ---
>> dump.c | 114 +++++++++++++++++++++++++++++++++++++---
>> include/qemu-common.h | 4 ++
>> include/sysemu/dump.h | 10 ++++
>> include/sysemu/memory_mapping.h | 8 +++
>> memory_mapping.c | 46 +++++++++++++++-
>> qmp.c | 14 +++++
>> 6 files changed, 188 insertions(+), 8 deletions(-)
>>
>> diff --git a/dump.c b/dump.c
>> index d79e0ed..dfd56aa 100644
>> --- a/dump.c
>> +++ b/dump.c
>> @@ -73,6 +73,7 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
>> static int dump_cleanup(DumpState *s)
>> {
>> guest_phys_blocks_free(&s->guest_phys_blocks);
>> + guest_memory_region_free(&s->guest_phys_blocks);
>> memory_mapping_list_free(&s->list);
>> close(s->fd);
>> if (s->resume) {
>> @@ -1427,6 +1428,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);
>> @@ -1580,6 +1584,86 @@ cleanup:
>> dump_cleanup(s);
>> }
>>
>> +extern GlobalDumpState global_dump_state;
>
> dump_state_get_global() returns a pointer to the local static variable, why do
> you need this?
>
> But what I really wonder is why a single boolean is not enough: the DumpState
> pointer can be passed to dump_thread, so it doesn't need to be global, then you
> don't need the mutex.
>
>> +
>> +static GlobalDumpState *dump_state_get_global(void)
>> +{
>> + static bool init = false;
>> + static GlobalDumpState global_dump_state;
>> + if (unlikely(!init)) {
>> + qemu_mutex_init(&global_dump_state.gds_mutex);
>> + global_dump_state.gds_cur = NULL;
>> + init = true;
>> + }
>> + return &global_dump_state;
>> +}
>> +
>> +/* Returns non-zero if there is existing dump in progress, otherwise
>> + * zero is returned. */
>> +bool dump_in_progress(void)
>> +{
>> + GlobalDumpState *global = dump_state_get_global();
>> + /* we do not need to take the mutex here if we are checking the
>> + * pointer only. */
>> + return (!!global->gds_cur);
>> +}
>> +
>> +/* Trying to create one DumpState. This will fail if there is an
>> + * existing one. Return DumpState pointer if succeeded, otherwise
>> + * NULL is returned. */
>> +static DumpState *dump_state_create(GlobalDumpState *global)
>> +{
>> + DumpState *cur = NULL;
>> + qemu_mutex_lock(&global->gds_mutex);
>> +
>> + cur = global->gds_cur;
>> + if (cur) {
>> + /* one dump in progress */
>> + cur = NULL;
>> + } else {
>> + /* we are the first! */
>> + cur = g_malloc0(sizeof(*cur));
>> + global->gds_cur = cur;
>> + }
>> +
>> + qemu_mutex_unlock(&global->gds_mutex);
>> + return cur;
>> +}
>> +
>> +/* release current DumpState structure */
>> +static void dump_state_release(GlobalDumpState *global)
>> +{
>> + DumpState *cur = NULL;
>> + qemu_mutex_lock(&global->gds_mutex);
>> +
>> + cur = global->gds_cur;
>> + /* we should never call release before having one */
>> + assert(cur);
>> + global->gds_cur = NULL;
>> +
>> + qemu_mutex_unlock(&global->gds_mutex);
>> + dump_cleanup(cur);
>> + g_free(cur);
>> +}
>> +
>> +/* this operation might be time consuming. */
>> +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);
>> + }
>> +}
>> +
>> +static void *dump_thread(void *data)
>> +{
>> + GlobalDumpState *global = (GlobalDumpState *)data;
>> + dump_process(global->gds_cur, NULL);
>> + dump_state_release(global);
>> + return NULL;
>> +}
>> +
>> void qmp_dump_guest_memory(bool paging, const char *file,
>> bool has_detach, bool detach,
>> bool has_begin, int64_t begin, bool has_length,
>> @@ -1590,6 +1674,14 @@ void qmp_dump_guest_memory(bool paging, const char *file,
>> int fd = -1;
>> DumpState *s;
>> Error *local_err = NULL;
>> + /* by default, we are keeping the old style, which is sync dump */
>> + bool detach_p = false;
>> + GlobalDumpState *global = dump_state_get_global();
>> +
>> + if (runstate_check(RUN_STATE_INMIGRATE)) {
>> + error_setg(errp, "Dump not allowed during incoming migration.");
>> + return;
>> + }
>>
>> /*
>> * kdump-compressed format need the whole memory dumped, so paging or
>> @@ -1609,6 +1701,9 @@ void qmp_dump_guest_memory(bool paging, const char *file,
>> error_setg(errp, QERR_MISSING_PARAMETER, "begin");
>> return;
>> }
>> + if (has_detach) {
>> + detach_p = detach;
>> + }
>>
>> /* check whether lzo/snappy is supported */
>> #ifndef CONFIG_LZO
>> @@ -1647,7 +1742,11 @@ void qmp_dump_guest_memory(bool paging, const char *file,
>> return;
>> }
>>
>> - s = g_malloc0(sizeof(DumpState));
>> + s = dump_state_create(global);
>> + if (!s) {
>> + error_setg(errp, "One dump is in progress.");
>> + return;
>> + }
>>
>> dump_init(s, fd, has_format, format, paging, has_begin,
>> begin, length, &local_err);
>> @@ -1657,14 +1756,15 @@ void qmp_dump_guest_memory(bool paging, const char *file,
>> return;
>> }
>>
>> - if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
>> - create_kdump_vmcore(s, errp);
>> + if (!detach_p) {
>> + /* sync dump */
>> + dump_process(s, errp);
>> + dump_state_release(global);
>> } else {
>> - create_vmcore(s, errp);
>> + qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
>> + global, QEMU_THREAD_DETACHED);
>> + /* DumpState is freed in dump thread */
>> }
>> -
>> - dump_cleanup(s);
>> - g_free(s);
>> }
>>
>> DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
>> diff --git a/include/qemu-common.h b/include/qemu-common.h
>> index 405364f..7b74961 100644
>> --- a/include/qemu-common.h
>> +++ b/include/qemu-common.h
>> @@ -501,4 +501,8 @@ int parse_debug_env(const char *name, int max, int initial);
>> const char *qemu_ether_ntoa(const MACAddr *mac);
>> void page_size_init(void);
>>
>> +/* returns non-zero if dump is in progress, otherwise zero is
>> + * returned. */
>> +bool dump_in_progress(void);
>> +
>> #endif
>> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
>> index 7e4ec5c..13c913e 100644
>> --- a/include/sysemu/dump.h
>> +++ b/include/sysemu/dump.h
>> @@ -183,9 +183,19 @@ 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;
>>
>> +typedef struct GlobalDumpState {
>> + QemuMutex gds_mutex; /* protector for GlobalDumpState itself */
>> + DumpState *gds_cur; /* current DumpState (might be NULL) */
>> +} GlobalDumpState;
>> +
>> uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
>> uint32_t cpu_to_dump32(DumpState *s, uint32_t val);
>> uint64_t cpu_to_dump64(DumpState *s, uint64_t val);
>> +
>> #endif
>> diff --git a/include/sysemu/memory_mapping.h b/include/sysemu/memory_mapping.h
>> index a75d59a..dd56fc7 100644
>> --- a/include/sysemu/memory_mapping.h
>> +++ b/include/sysemu/memory_mapping.h
>> @@ -30,10 +30,17 @@ typedef struct GuestPhysBlock {
>> QTAILQ_ENTRY(GuestPhysBlock) next;
>> } GuestPhysBlock;
>>
>> +typedef struct GuestMemoryRegion {
>> + MemoryRegion *gmr_mr;
>> + QTAILQ_ENTRY(GuestMemoryRegion) next;
>> +} GuestMemoryRegion;
>> +
>> /* point-in-time snapshot of guest-visible physical mappings */
>> typedef struct GuestPhysBlockList {
>> unsigned num;
>> QTAILQ_HEAD(GuestPhysBlockHead, GuestPhysBlock) head;
>> + /* housekeep all the MemoryRegion that is referenced */
>> + QTAILQ_HEAD(GuestMemRegionHead, GuestMemoryRegion) head_mr;
>> } GuestPhysBlockList;
>>
>> /* The physical and virtual address in the memory mapping are contiguous. */
>> @@ -65,6 +72,7 @@ void memory_mapping_list_free(MemoryMappingList *list);
>> void memory_mapping_list_init(MemoryMappingList *list);
>>
>> void guest_phys_blocks_free(GuestPhysBlockList *list);
>> +void guest_memory_region_free(GuestPhysBlockList *list);
>> void guest_phys_blocks_init(GuestPhysBlockList *list);
>> void guest_phys_blocks_append(GuestPhysBlockList *list);
>>
>> diff --git a/memory_mapping.c b/memory_mapping.c
>> index 36d6b26..a279552 100644
>> --- a/memory_mapping.c
>> +++ b/memory_mapping.c
>> @@ -186,6 +186,7 @@ void guest_phys_blocks_init(GuestPhysBlockList *list)
>> {
>> list->num = 0;
>> QTAILQ_INIT(&list->head);
>> + QTAILQ_INIT(&list->head_mr);
>> }
>>
>> typedef struct GuestPhysListener {
>> @@ -193,6 +194,39 @@ typedef struct GuestPhysListener {
>> MemoryListener listener;
>> } GuestPhysListener;
>>
>> +static void guest_memory_region_add(GuestPhysBlockList *list,
>> + MemoryRegion *mr)
>> +{
>> + GuestMemoryRegion *gmr = NULL;
>> + QTAILQ_FOREACH(gmr, &list->head_mr, next) {
>> + if (gmr->gmr_mr == mr) {
>> + /* we already refererenced the MemoryRegion */
>> + return;
>> + }
>
> This is O(N^2). I think it should be fine to skip this check and just grab the
> reference. If that is the case, we can probably embed the mr pointer in
> GuestPhysBlock. Paolo?
If embed the mr pointer into GuestPhyBlock, then we might need to
ref/unref one MemoryRegion for multiple times (and I am not sure how
many, maybe it could be a very big number especially the
MemoryRegionSections are scattered?). Not sure whether it is more
efficient.
For what I see, the number of MemoryRegions should be not big (<5 in
my case). So even with O(N^2), it should merely like O(N). Not sure
about this too.
Would like to hear more review comments from Paolo and others.
>
>> + }
>> + /* reference the MemoryRegion to make sure it's valid during dump */
>> +#ifdef DEBUG_DUMP_GUEST_MEMORY
>> + fprintf(stderr, "DUMP: ref mem region: %s\n", mr->name);
>> +#endif
>> + memory_region_ref(mr);
>> + gmr = g_malloc0(sizeof(*gmr));
>> + gmr->gmr_mr = mr;
>> + QTAILQ_INSERT_TAIL(&list->head_mr, gmr, next);
>> +}
>> +
>> +void guest_memory_region_free(GuestPhysBlockList *list)
>> +{
>> + GuestMemoryRegion *gmr = NULL, *q = NULL;
>> + QTAILQ_FOREACH_SAFE(gmr, &list->head_mr, next, q) {
>> +#ifdef DEBUG_DUMP_GUEST_MEMORY
>> + fprintf(stderr, "DUMP: unref mem region: %s\n", gmr->gmr_mr->name);
>> +#endif
>> + QTAILQ_REMOVE(&list->head_mr, gmr, next);
>> + memory_region_unref(gmr->gmr_mr);
>> + g_free(gmr);
>> + }
>> +}
>> +
>> static void guest_phys_blocks_region_add(MemoryListener *listener,
>> MemoryRegionSection *section)
>> {
>> @@ -241,6 +275,10 @@ static void guest_phys_blocks_region_add(MemoryListener *listener,
>> block->target_end = target_end;
>> block->host_addr = host_addr;
>>
>> + /* keep all the MemoryRegion that is referenced by the dump
>> + * process */
>> + guest_memory_region_add(g->list, section->mr);
>> +
>> QTAILQ_INSERT_TAIL(&g->list->head, block, next);
>> ++g->list->num;
>> } else {
>> @@ -250,7 +288,7 @@ static void guest_phys_blocks_region_add(MemoryListener *listener,
>> predecessor->target_end = target_end;
>> }
>>
>> -#ifdef DEBUG_GUEST_PHYS_REGION_ADD
>> +#ifdef DEBUG_DUMP_GUEST_MEMORY
>> fprintf(stderr, "%s: target_start=" TARGET_FMT_plx " target_end="
>> TARGET_FMT_plx ": %s (count: %u)\n", __FUNCTION__, target_start,
>> target_end, predecessor ? "joined" : "added", g->list->num);
>> @@ -263,8 +301,14 @@ void guest_phys_blocks_append(GuestPhysBlockList *list)
>>
>> g.list = list;
>> g.listener.region_add = &guest_phys_blocks_region_add;
>> + /* We should make sure all the memory objects are valid during
>> + * add dump regions. Before releasing it, we should also make
>> + * sure all the MemoryRegions to be used during dump is
>> + * referenced. */
>> + rcu_read_lock();
>> memory_listener_register(&g.listener, &address_space_memory);
>> memory_listener_unregister(&g.listener);
>> + rcu_read_unlock();
>
> I don't think this is necessary if VM is stopped and incoming migration is
> not running - address_space_memory will be safe to access as usual. Otherwise
> this is a separate problem.
Yes, I think it should work too without the two lines. I am just
taking Paolo's suggestions. It should work too if we do not take the
reference counts for MemoryRegion. However, without those things
(ref counts, rcu locks), it's not thread safe even it could work.
Not sure whether I am understanding it correctly.
Thanks!
Peter
>
>> }
>>
>> static CPUState *find_paging_enabled_cpu(CPUState *start_cpu)
>> diff --git a/qmp.c b/qmp.c
>> index 0a1fa19..055586d 100644
>> --- a/qmp.c
>> +++ b/qmp.c
>> @@ -102,6 +102,13 @@ void qmp_quit(Error **errp)
>>
>> void qmp_stop(Error **errp)
>> {
>> + /* if there is a dump in background, we should wait until the dump
>> + * finished */
>> + if (dump_in_progress()) {
>> + error_setg(errp, "There is a dump in process, please wait.");
>> + return;
>> + }
>> +
>> if (runstate_check(RUN_STATE_INMIGRATE)) {
>> autostart = 0;
>> } else {
>> @@ -174,6 +181,13 @@ void qmp_cont(Error **errp)
>> BlockBackend *blk;
>> BlockDriverState *bs;
>>
>> + /* if there is a dump in background, we should wait until the dump
>> + * finished */
>> + if (dump_in_progress()) {
>> + 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;
>> --
>> 2.4.3
>>
>>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.
2015-11-27 6:27 ` Peter Xu
@ 2015-11-27 10:14 ` Paolo Bonzini
2015-11-27 11:03 ` Peter Xu
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2015-11-27 10:14 UTC (permalink / raw)
To: Peter Xu, Fam Zheng; +Cc: qemu-devel
On 27/11/2015 07:27, Peter Xu wrote:
> If embed the mr pointer into GuestPhyBlock, then we might need to
> ref/unref one MemoryRegion for multiple times (and I am not sure how
> many, maybe it could be a very big number especially the
> MemoryRegionSections are scattered?). Not sure whether it is more
> efficient.
>
> For what I see, the number of MemoryRegions should be not big (<5 in
> my case). So even with O(N^2), it should merely like O(N). Not sure
> about this too.
>
> Would like to hear more review comments from Paolo and others.
>
Fam suggestion is a good one, ref'ing one MemoryRegion many times is not
a problem.
Also I noticed now that you do the dump_init in the main thread (using a
listener), so the RCU lock/unlock is not needed. I don't know this code
very well.
It's worth adding a comment at the top of functions that are called from
a separate thread.
Thanks,
Paolo
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.
2015-11-27 10:14 ` Paolo Bonzini
@ 2015-11-27 11:03 ` Peter Xu
0 siblings, 0 replies; 43+ messages in thread
From: Peter Xu @ 2015-11-27 11:03 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Fam Zheng, qemu-devel
On Fri, Nov 27, 2015 at 11:14:17AM +0100, Paolo Bonzini wrote:
>
>
> On 27/11/2015 07:27, Peter Xu wrote:
> > If embed the mr pointer into GuestPhyBlock, then we might need to
> > ref/unref one MemoryRegion for multiple times (and I am not sure how
> > many, maybe it could be a very big number especially the
> > MemoryRegionSections are scattered?). Not sure whether it is more
> > efficient.
> >
> > For what I see, the number of MemoryRegions should be not big (<5 in
> > my case). So even with O(N^2), it should merely like O(N). Not sure
> > about this too.
> >
> > Would like to hear more review comments from Paolo and others.
> >
>
> Fam suggestion is a good one, ref'ing one MemoryRegion many times is not
> a problem.
Ok, then I will embed the MemoryRegion pointer into GuestPhysBlocks
in v3.
>
> Also I noticed now that you do the dump_init in the main thread (using a
> listener), so the RCU lock/unlock is not needed. I don't know this code
> very well.
Yes, it's in main thread too in v1 patch, since I think that should
not be the time consuming part. If so, I will remove the RCU
lock/unlock too.
>
> It's worth adding a comment at the top of functions that are called from
> a separate thread.
Ok. Will add that.
Thanks!
Peter
>
> Thanks,
>
> Paolo
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.
2015-11-27 5:14 ` Fam Zheng
2015-11-27 5:20 ` Fam Zheng
2015-11-27 6:27 ` Peter Xu
@ 2015-11-28 5:51 ` Peter Xu
2015-11-30 1:48 ` Fam Zheng
2 siblings, 1 reply; 43+ messages in thread
From: Peter Xu @ 2015-11-28 5:51 UTC (permalink / raw)
To: Fam Zheng; +Cc: pbonzini, qemu-devel
On Fri, Nov 27, 2015 at 01:14:25PM +0800, Fam Zheng wrote:
> On Fri, 11/27 10:48, Peter Xu wrote:
[snip]
>
> This patch doesn't handle the incoming migration case, i.e. when QEMU is
> started with "-incoming", or "-incoming defer". Dump can go wrong when incoming
> migration happens at the same time.
Sorry to missed these lines. Still not understand why this patch
cannot handle this if with [1] (Please check below for [1])?
[snip]
> >
> > +extern GlobalDumpState global_dump_state;
>
> dump_state_get_global() returns a pointer to the local static variable, why do
> you need this?
Yes, I should remove that. Thanks!
[snip]
> > +
> > @@ -1590,6 +1674,14 @@ void qmp_dump_guest_memory(bool paging, const char *file,
> > int fd = -1;
> > DumpState *s;
> > Error *local_err = NULL;
> > + /* by default, we are keeping the old style, which is sync dump */
> > + bool detach_p = false;
> > + GlobalDumpState *global = dump_state_get_global();
> > +
[1]
> > + if (runstate_check(RUN_STATE_INMIGRATE)) {
> > + error_setg(errp, "Dump not allowed during incoming migration.");
> > + return;
> > + }
Thanks.
Peter
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.
2015-11-28 5:51 ` Peter Xu
@ 2015-11-30 1:48 ` Fam Zheng
0 siblings, 0 replies; 43+ messages in thread
From: Fam Zheng @ 2015-11-30 1:48 UTC (permalink / raw)
To: Peter Xu; +Cc: pbonzini, qemu-devel
On Sat, 11/28 13:51, Peter Xu wrote:
> On Fri, Nov 27, 2015 at 01:14:25PM +0800, Fam Zheng wrote:
> > On Fri, 11/27 10:48, Peter Xu wrote:
>
> [snip]
>
> >
> > This patch doesn't handle the incoming migration case, i.e. when QEMU is
> > started with "-incoming", or "-incoming defer". Dump can go wrong when incoming
> > migration happens at the same time.
>
> Sorry to missed these lines. Still not understand why this patch
> cannot handle this if with [1] (Please check below for [1])?
You're right, that does work. I missed that "defer" also sets
RUN_STATE_INMIGRATE.
Fam
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support Peter Xu
2015-11-27 5:14 ` Fam Zheng
@ 2015-11-27 10:31 ` Paolo Bonzini
2015-11-27 11:26 ` Peter Xu
1 sibling, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2015-11-27 10:31 UTC (permalink / raw)
To: Peter Xu, qemu-devel
On 27/11/2015 03:48, Peter Xu wrote:
> This patch implements "detach" for "dump-guest-memory" command. When
> specified, one background thread is created to do the dump work.
>
> When there is a dump running in background, the commands "stop",
> "cont" and "dump-guest-memory" will fail directly, with a hint
> telling user: "there is a dump in progress".
>
> Although it is not quite essential for now, the new codes are trying
> to make sure the dump is thread safe. To do this, one list is added
> into GuestPhysBlockList to track all the referenced MemoryRegions
> during dump process. We should make sure each MemoryRegion would
> only be referenced for once.
>
> One global struct "GlobalDumpState" is added to store some global
> informations for dump procedures. One mutex is used to protect the
> global dump info.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> dump.c | 114 +++++++++++++++++++++++++++++++++++++---
> include/qemu-common.h | 4 ++
> include/sysemu/dump.h | 10 ++++
> include/sysemu/memory_mapping.h | 8 +++
> memory_mapping.c | 46 +++++++++++++++-
> qmp.c | 14 +++++
> 6 files changed, 188 insertions(+), 8 deletions(-)
>
> diff --git a/dump.c b/dump.c
> index d79e0ed..dfd56aa 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -73,6 +73,7 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
> static int dump_cleanup(DumpState *s)
> {
> guest_phys_blocks_free(&s->guest_phys_blocks);
> + guest_memory_region_free(&s->guest_phys_blocks);
> memory_mapping_list_free(&s->list);
> close(s->fd);
> if (s->resume) {
> @@ -1427,6 +1428,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);
> @@ -1580,6 +1584,86 @@ cleanup:
> dump_cleanup(s);
> }
>
> +extern GlobalDumpState global_dump_state;
> +
> +static GlobalDumpState *dump_state_get_global(void)
> +{
> + static bool init = false;
> + static GlobalDumpState global_dump_state;
> + if (unlikely(!init)) {
> + qemu_mutex_init(&global_dump_state.gds_mutex);
The mutex is not necessary. The structure is always created in the main
thread and released by the dump thread (of which there is just one).
You can then just make a global DumpState (not a pointer!), and separate
the fields between:
- those that are protected by a mutex (most likely the DumpResult and
written_size, possibly others)
- those that are only written before the thread is started, and thus do
not need to be protected by a mutex
- those that are only accessed by the thread, and thus do not need to be
protected by a mutex either.
See for example this extract from migration/block.c:
typedef struct BlkMigState {
/* Written during setup phase. Can be read without a lock. */
int blk_enable;
int shared_base;
QSIMPLEQ_HEAD(bmds_list, BlkMigDevState) bmds_list;
int64_t total_sector_sum;
bool zero_blocks;
/* Protected by lock. */
QSIMPLEQ_HEAD(blk_list, BlkMigBlock) blk_list;
int submitted;
int read_done;
/* Only used by migration thread. Does not need a lock. */
int transferred;
int prev_progress;
int bulk_completed;
/* The lock. */
QemuMutex lock;
} BlkMigState;
static BlkMigState block_mig_state;
Paolo
> + global_dump_state.gds_cur = NULL;
> + init = true;
> + }
> + return &global_dump_state;
> +}
> +
> +/* Returns non-zero if there is existing dump in progress, otherwise
> + * zero is returned. */
> +bool dump_in_progress(void)
> +{
> + GlobalDumpState *global = dump_state_get_global();
> + /* we do not need to take the mutex here if we are checking the
> + * pointer only. */
> + return (!!global->gds_cur);
> +}
> +
> +/* Trying to create one DumpState. This will fail if there is an
> + * existing one. Return DumpState pointer if succeeded, otherwise
> + * NULL is returned. */
> +static DumpState *dump_state_create(GlobalDumpState *global)
> +{
> + DumpState *cur = NULL;
> + qemu_mutex_lock(&global->gds_mutex);
> +
> + cur = global->gds_cur;
> + if (cur) {
> + /* one dump in progress */
> + cur = NULL;
> + } else {
> + /* we are the first! */
> + cur = g_malloc0(sizeof(*cur));
> + global->gds_cur = cur;
> + }
> +
> + qemu_mutex_unlock(&global->gds_mutex);
> + return cur;
> +}
> +
> +/* release current DumpState structure */
> +static void dump_state_release(GlobalDumpState *global)
> +{
> + DumpState *cur = NULL;
> + qemu_mutex_lock(&global->gds_mutex);
> +
> + cur = global->gds_cur;
> + /* we should never call release before having one */
> + assert(cur);
> + global->gds_cur = NULL;
> +
> + qemu_mutex_unlock(&global->gds_mutex);
> + dump_cleanup(cur);
> + g_free(cur);
> +}
> +
> +/* this operation might be time consuming. */
> +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);
> + }
> +}
> +
> +static void *dump_thread(void *data)
> +{
> + GlobalDumpState *global = (GlobalDumpState *)data;
> + dump_process(global->gds_cur, NULL);
> + dump_state_release(global);
> + return NULL;
> +}
> +
> void qmp_dump_guest_memory(bool paging, const char *file,
> bool has_detach, bool detach,
> bool has_begin, int64_t begin, bool has_length,
> @@ -1590,6 +1674,14 @@ void qmp_dump_guest_memory(bool paging, const char *file,
> int fd = -1;
> DumpState *s;
> Error *local_err = NULL;
> + /* by default, we are keeping the old style, which is sync dump */
> + bool detach_p = false;
> + GlobalDumpState *global = dump_state_get_global();
> +
> + if (runstate_check(RUN_STATE_INMIGRATE)) {
> + error_setg(errp, "Dump not allowed during incoming migration.");
> + return;
> + }
>
> /*
> * kdump-compressed format need the whole memory dumped, so paging or
> @@ -1609,6 +1701,9 @@ void qmp_dump_guest_memory(bool paging, const char *file,
> error_setg(errp, QERR_MISSING_PARAMETER, "begin");
> return;
> }
> + if (has_detach) {
> + detach_p = detach;
> + }
>
> /* check whether lzo/snappy is supported */
> #ifndef CONFIG_LZO
> @@ -1647,7 +1742,11 @@ void qmp_dump_guest_memory(bool paging, const char *file,
> return;
> }
>
> - s = g_malloc0(sizeof(DumpState));
> + s = dump_state_create(global);
> + if (!s) {
> + error_setg(errp, "One dump is in progress.");
> + return;
> + }
>
> dump_init(s, fd, has_format, format, paging, has_begin,
> begin, length, &local_err);
> @@ -1657,14 +1756,15 @@ void qmp_dump_guest_memory(bool paging, const char *file,
> return;
> }
>
> - if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
> - create_kdump_vmcore(s, errp);
> + if (!detach_p) {
> + /* sync dump */
> + dump_process(s, errp);
> + dump_state_release(global);
> } else {
> - create_vmcore(s, errp);
> + qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
> + global, QEMU_THREAD_DETACHED);
> + /* DumpState is freed in dump thread */
> }
> -
> - dump_cleanup(s);
> - g_free(s);
> }
>
> DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 405364f..7b74961 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -501,4 +501,8 @@ int parse_debug_env(const char *name, int max, int initial);
> const char *qemu_ether_ntoa(const MACAddr *mac);
> void page_size_init(void);
>
> +/* returns non-zero if dump is in progress, otherwise zero is
> + * returned. */
> +bool dump_in_progress(void);
> +
> #endif
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index 7e4ec5c..13c913e 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -183,9 +183,19 @@ 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;
>
> +typedef struct GlobalDumpState {
> + QemuMutex gds_mutex; /* protector for GlobalDumpState itself */
> + DumpState *gds_cur; /* current DumpState (might be NULL) */
> +} GlobalDumpState;
> +
> uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
> uint32_t cpu_to_dump32(DumpState *s, uint32_t val);
> uint64_t cpu_to_dump64(DumpState *s, uint64_t val);
> +
> #endif
> diff --git a/include/sysemu/memory_mapping.h b/include/sysemu/memory_mapping.h
> index a75d59a..dd56fc7 100644
> --- a/include/sysemu/memory_mapping.h
> +++ b/include/sysemu/memory_mapping.h
> @@ -30,10 +30,17 @@ typedef struct GuestPhysBlock {
> QTAILQ_ENTRY(GuestPhysBlock) next;
> } GuestPhysBlock;
>
> +typedef struct GuestMemoryRegion {
> + MemoryRegion *gmr_mr;
> + QTAILQ_ENTRY(GuestMemoryRegion) next;
> +} GuestMemoryRegion;
> +
> /* point-in-time snapshot of guest-visible physical mappings */
> typedef struct GuestPhysBlockList {
> unsigned num;
> QTAILQ_HEAD(GuestPhysBlockHead, GuestPhysBlock) head;
> + /* housekeep all the MemoryRegion that is referenced */
> + QTAILQ_HEAD(GuestMemRegionHead, GuestMemoryRegion) head_mr;
> } GuestPhysBlockList;
>
> /* The physical and virtual address in the memory mapping are contiguous. */
> @@ -65,6 +72,7 @@ void memory_mapping_list_free(MemoryMappingList *list);
> void memory_mapping_list_init(MemoryMappingList *list);
>
> void guest_phys_blocks_free(GuestPhysBlockList *list);
> +void guest_memory_region_free(GuestPhysBlockList *list);
> void guest_phys_blocks_init(GuestPhysBlockList *list);
> void guest_phys_blocks_append(GuestPhysBlockList *list);
>
> diff --git a/memory_mapping.c b/memory_mapping.c
> index 36d6b26..a279552 100644
> --- a/memory_mapping.c
> +++ b/memory_mapping.c
> @@ -186,6 +186,7 @@ void guest_phys_blocks_init(GuestPhysBlockList *list)
> {
> list->num = 0;
> QTAILQ_INIT(&list->head);
> + QTAILQ_INIT(&list->head_mr);
> }
>
> typedef struct GuestPhysListener {
> @@ -193,6 +194,39 @@ typedef struct GuestPhysListener {
> MemoryListener listener;
> } GuestPhysListener;
>
> +static void guest_memory_region_add(GuestPhysBlockList *list,
> + MemoryRegion *mr)
> +{
> + GuestMemoryRegion *gmr = NULL;
> + QTAILQ_FOREACH(gmr, &list->head_mr, next) {
> + if (gmr->gmr_mr == mr) {
> + /* we already refererenced the MemoryRegion */
> + return;
> + }
> + }
> + /* reference the MemoryRegion to make sure it's valid during dump */
> +#ifdef DEBUG_DUMP_GUEST_MEMORY
> + fprintf(stderr, "DUMP: ref mem region: %s\n", mr->name);
> +#endif
> + memory_region_ref(mr);
> + gmr = g_malloc0(sizeof(*gmr));
> + gmr->gmr_mr = mr;
> + QTAILQ_INSERT_TAIL(&list->head_mr, gmr, next);
> +}
> +
> +void guest_memory_region_free(GuestPhysBlockList *list)
> +{
> + GuestMemoryRegion *gmr = NULL, *q = NULL;
> + QTAILQ_FOREACH_SAFE(gmr, &list->head_mr, next, q) {
> +#ifdef DEBUG_DUMP_GUEST_MEMORY
> + fprintf(stderr, "DUMP: unref mem region: %s\n", gmr->gmr_mr->name);
> +#endif
> + QTAILQ_REMOVE(&list->head_mr, gmr, next);
> + memory_region_unref(gmr->gmr_mr);
> + g_free(gmr);
> + }
> +}
> +
> static void guest_phys_blocks_region_add(MemoryListener *listener,
> MemoryRegionSection *section)
> {
> @@ -241,6 +275,10 @@ static void guest_phys_blocks_region_add(MemoryListener *listener,
> block->target_end = target_end;
> block->host_addr = host_addr;
>
> + /* keep all the MemoryRegion that is referenced by the dump
> + * process */
> + guest_memory_region_add(g->list, section->mr);
> +
> QTAILQ_INSERT_TAIL(&g->list->head, block, next);
> ++g->list->num;
> } else {
> @@ -250,7 +288,7 @@ static void guest_phys_blocks_region_add(MemoryListener *listener,
> predecessor->target_end = target_end;
> }
>
> -#ifdef DEBUG_GUEST_PHYS_REGION_ADD
> +#ifdef DEBUG_DUMP_GUEST_MEMORY
> fprintf(stderr, "%s: target_start=" TARGET_FMT_plx " target_end="
> TARGET_FMT_plx ": %s (count: %u)\n", __FUNCTION__, target_start,
> target_end, predecessor ? "joined" : "added", g->list->num);
> @@ -263,8 +301,14 @@ void guest_phys_blocks_append(GuestPhysBlockList *list)
>
> g.list = list;
> g.listener.region_add = &guest_phys_blocks_region_add;
> + /* We should make sure all the memory objects are valid during
> + * add dump regions. Before releasing it, we should also make
> + * sure all the MemoryRegions to be used during dump is
> + * referenced. */
> + rcu_read_lock();
> memory_listener_register(&g.listener, &address_space_memory);
> memory_listener_unregister(&g.listener);
> + rcu_read_unlock();
> }
>
> static CPUState *find_paging_enabled_cpu(CPUState *start_cpu)
> diff --git a/qmp.c b/qmp.c
> index 0a1fa19..055586d 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -102,6 +102,13 @@ void qmp_quit(Error **errp)
>
> void qmp_stop(Error **errp)
> {
> + /* if there is a dump in background, we should wait until the dump
> + * finished */
> + if (dump_in_progress()) {
> + error_setg(errp, "There is a dump in process, please wait.");
> + return;
> + }
> +
> if (runstate_check(RUN_STATE_INMIGRATE)) {
> autostart = 0;
> } else {
> @@ -174,6 +181,13 @@ void qmp_cont(Error **errp)
> BlockBackend *blk;
> BlockDriverState *bs;
>
> + /* if there is a dump in background, we should wait until the dump
> + * finished */
> + if (dump_in_progress()) {
> + 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;
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.
2015-11-27 10:31 ` Paolo Bonzini
@ 2015-11-27 11:26 ` Peter Xu
2015-11-27 11:52 ` Paolo Bonzini
0 siblings, 1 reply; 43+ messages in thread
From: Peter Xu @ 2015-11-27 11:26 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On Fri, Nov 27, 2015 at 11:31:00AM +0100, Paolo Bonzini wrote:
>
[snip]
> > +
> > +static GlobalDumpState *dump_state_get_global(void)
> > +{
> > + static bool init = false;
> > + static GlobalDumpState global_dump_state;
> > + if (unlikely(!init)) {
> > + qemu_mutex_init(&global_dump_state.gds_mutex);
>
> The mutex is not necessary. The structure is always created in the main
> thread and released by the dump thread (of which there is just one).
[1]
>
> You can then just make a global DumpState (not a pointer!), and separate
> the fields between:
>
> - those that are protected by a mutex (most likely the DumpResult and
> written_size, possibly others)
Hi, Paolo,
So mutex is still necessary, right? (refer to [1]) Since
"dump-query" will read several fields of it, while the dump thread
might be modifying them as well?
>
> - those that are only written before the thread is started, and thus do
> not need to be protected by a mutex
>
> - those that are only accessed by the thread, and thus do not need to be
> protected by a mutex either.
>
> See for example this extract from migration/block.c:
>
> typedef struct BlkMigState {
> /* Written during setup phase. Can be read without a lock. */
> int blk_enable;
> int shared_base;
> QSIMPLEQ_HEAD(bmds_list, BlkMigDevState) bmds_list;
> int64_t total_sector_sum;
> bool zero_blocks;
>
> /* Protected by lock. */
> QSIMPLEQ_HEAD(blk_list, BlkMigBlock) blk_list;
> int submitted;
> int read_done;
>
> /* Only used by migration thread. Does not need a lock. */
> int transferred;
> int prev_progress;
> int bulk_completed;
>
> /* The lock. */
> QemuMutex lock;
> } BlkMigState;
>
> static BlkMigState block_mig_state;
Ok, I think I can remove the global state and make a static
DumpState. When I was drafting the patch, I just tried to keep the
old logic (malloc/free) and avoid introducing bugs. Maybe I was
wrong. I should better not introduce new struct if not necessary.
Will try to follow this example in v3.
Thanks.
Peter
>
> Paolo
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.
2015-11-27 11:26 ` Peter Xu
@ 2015-11-27 11:52 ` Paolo Bonzini
0 siblings, 0 replies; 43+ messages in thread
From: Paolo Bonzini @ 2015-11-27 11:52 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel
On 27/11/2015 12:26, Peter Xu wrote:
> On Fri, Nov 27, 2015 at 11:31:00AM +0100, Paolo Bonzini wrote:
>>
>
> [snip]
>
>>> +
>>> +static GlobalDumpState *dump_state_get_global(void)
>>> +{
>>> + static bool init = false;
>>> + static GlobalDumpState global_dump_state;
>>> + if (unlikely(!init)) {
>>> + qemu_mutex_init(&global_dump_state.gds_mutex);
>>
>> The mutex is not necessary. The structure is always created in the main
>> thread and released by the dump thread (of which there is just one).
>
> [1]
>
>>
>> You can then just make a global DumpState (not a pointer!), and separate
>> the fields between:
>>
>> - those that are protected by a mutex (most likely the DumpResult and
>> written_size, possibly others)
>
> Hi, Paolo,
>
> So mutex is still necessary, right? (refer to [1]) Since
> "dump-query" will read several fields of it, while the dump thread
> might be modifying them as well?
Right, initially I meant a mutex around the allocation. But then I read
the other patches where you add more fields, and came back to add more
content.
Strictly speaking it's likely that you can do everything without a
mutex, but it's easier to use one.
> Ok, I think I can remove the global state and make a static
> DumpState. When I was drafting the patch, I just tried to keep the
> old logic (malloc/free) and avoid introducing bugs. Maybe I was
> wrong. I should better not introduce new struct if not necessary.
It's okay. The only confusing bit was that you had to add more state to
GlobalDumpState, and in the end it was split between DumpState and
GlobalDumpState. As far as this patch is concerned, using malloc/free
would have been okay, but then the subsequent changes suggest a
different approach.
Thanks!
Paolo
> Will try to follow this example in v3.
>
> Thanks.
> Peter
>
>>
>> Paolo
>>
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH v2 4/8] dump-guest-memory: add qmp event DUMP_COMPLETED
2015-11-27 2:48 [Qemu-devel] [PATCH v2 0/8] Add basic "detach" support for dump-guest-memory Peter Xu
` (2 preceding siblings ...)
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support Peter Xu
@ 2015-11-27 2:48 ` Peter Xu
2015-11-27 5:19 ` Fam Zheng
` (2 more replies)
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 5/8] dump-query: add "dump-query" command to query dump status Peter Xu
` (5 subsequent siblings)
9 siblings, 3 replies; 43+ messages in thread
From: Peter Xu @ 2015-11-27 2:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Xu
To get aligned with QMP interface, one new QMP event DUMP_COMPLETED
is added. It is used when user specified "detach" in dump, and
triggered when the dump finishes. Error message will be appended to
this event if the dump has failed.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
docs/qmp-events.txt | 12 ++++++++++++
dump.c | 12 +++++++++++-
qapi/event.json | 10 ++++++++++
3 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
index d2f1ce4..fe494f9 100644
--- a/docs/qmp-events.txt
+++ b/docs/qmp-events.txt
@@ -674,3 +674,15 @@ Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is
followed respectively by the RESET, SHUTDOWN, or STOP events.
Note: this event is rate-limited.
+
+DUMP_COMPLETED
+--------------
+
+Emitted when the guest has finished one memory dump.
+
+Data: None.
+
+Example:
+
+{ "event": "DUMP_COMPLETED",
+ "data": { "msg": "Dump completed successfully" } }
diff --git a/dump.c b/dump.c
index dfd56aa..0d321d4 100644
--- a/dump.c
+++ b/dump.c
@@ -25,6 +25,7 @@
#include "qapi/error.h"
#include "qapi/qmp/qerror.h"
#include "qmp-commands.h"
+#include "qapi-event.h"
#include <zlib.h>
#ifdef CONFIG_LZO
@@ -1659,8 +1660,17 @@ static void dump_process(DumpState *s, Error **errp)
static void *dump_thread(void *data)
{
GlobalDumpState *global = (GlobalDumpState *)data;
- dump_process(global->gds_cur, NULL);
+ Error *local_err = NULL;
+ const char *msg = "Dump completed successfully";
+
+ dump_process(global->gds_cur, &local_err);
dump_state_release(global);
+
+ /* if detach is used, notify user that dump has finished */
+ if (local_err) {
+ msg = error_get_pretty(local_err);
+ }
+ qapi_event_send_dump_completed(msg, &error_abort);
return NULL;
}
diff --git a/qapi/event.json b/qapi/event.json
index f0cef01..de24c27 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -356,3 +356,13 @@
##
{ 'event': 'MEM_UNPLUG_ERROR',
'data': { 'device': 'str', 'msg': 'str' } }
+
+##
+# @DUMP_COMPLETED
+#
+# Emitted when background dump has completed
+#
+# Since: 2.6
+##
+{ 'event': 'DUMP_COMPLETED' ,
+ 'data': { 'msg': 'str' } }
--
2.4.3
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 4/8] dump-guest-memory: add qmp event DUMP_COMPLETED
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 4/8] dump-guest-memory: add qmp event DUMP_COMPLETED Peter Xu
@ 2015-11-27 5:19 ` Fam Zheng
2015-11-27 6:43 ` Peter Xu
2015-11-27 10:15 ` Paolo Bonzini
2015-11-30 18:18 ` Eric Blake
2 siblings, 1 reply; 43+ messages in thread
From: Fam Zheng @ 2015-11-27 5:19 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel
On Fri, 11/27 10:48, Peter Xu wrote:
> @@ -1659,8 +1660,17 @@ static void dump_process(DumpState *s, Error **errp)
> static void *dump_thread(void *data)
> {
> GlobalDumpState *global = (GlobalDumpState *)data;
> - dump_process(global->gds_cur, NULL);
> + Error *local_err = NULL;
> + const char *msg = "Dump completed successfully";
> +
> + dump_process(global->gds_cur, &local_err);
> dump_state_release(global);
> +
> + /* if detach is used, notify user that dump has finished */
> + if (local_err) {
> + msg = error_get_pretty(local_err);
error_free?
> + }
> + qapi_event_send_dump_completed(msg, &error_abort);
> return NULL;
> }
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 4/8] dump-guest-memory: add qmp event DUMP_COMPLETED
2015-11-27 5:19 ` Fam Zheng
@ 2015-11-27 6:43 ` Peter Xu
0 siblings, 0 replies; 43+ messages in thread
From: Peter Xu @ 2015-11-27 6:43 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-devel
On 11/27/2015 01:19 PM, Fam Zheng wrote:
> On Fri, 11/27 10:48, Peter Xu wrote:
>> @@ -1659,8 +1660,17 @@ static void dump_process(DumpState *s, Error **errp)
>> static void *dump_thread(void *data)
>> {
>> GlobalDumpState *global = (GlobalDumpState *)data;
>> - dump_process(global->gds_cur, NULL);
>> + Error *local_err = NULL;
>> + const char *msg = "Dump completed successfully";
>> +
>> + dump_process(global->gds_cur, &local_err);
>> dump_state_release(global);
>> +
>> + /* if detach is used, notify user that dump has finished */
>> + if (local_err) {
>> + msg = error_get_pretty(local_err);
>
> error_free?
Yes, thanks!
Peter
>
>
>> + }
>> + qapi_event_send_dump_completed(msg, &error_abort);
>> return NULL;
>> }
>>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/8] dump-guest-memory: add qmp event DUMP_COMPLETED
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 4/8] dump-guest-memory: add qmp event DUMP_COMPLETED Peter Xu
2015-11-27 5:19 ` Fam Zheng
@ 2015-11-27 10:15 ` Paolo Bonzini
2015-11-27 11:29 ` Peter Xu
2015-11-30 18:18 ` Eric Blake
2 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2015-11-27 10:15 UTC (permalink / raw)
To: Peter Xu, qemu-devel
On 27/11/2015 03:48, Peter Xu wrote:
> +##
> +# @DUMP_COMPLETED
> +#
> +# Emitted when background dump has completed
> +#
> +# Since: 2.6
> +##
> +{ 'event': 'DUMP_COMPLETED' ,
> + 'data': { 'msg': 'str' } }
This makes it hard to understand whether there was an error or not. I
suggest using '*error': 'str' instead, and omitting the error if the
dump is successful.
Paolo
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 4/8] dump-guest-memory: add qmp event DUMP_COMPLETED
2015-11-27 10:15 ` Paolo Bonzini
@ 2015-11-27 11:29 ` Peter Xu
0 siblings, 0 replies; 43+ messages in thread
From: Peter Xu @ 2015-11-27 11:29 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On Fri, Nov 27, 2015 at 11:15:35AM +0100, Paolo Bonzini wrote:
>
>
> On 27/11/2015 03:48, Peter Xu wrote:
> > +##
> > +# @DUMP_COMPLETED
> > +#
> > +# Emitted when background dump has completed
> > +#
> > +# Since: 2.6
> > +##
> > +{ 'event': 'DUMP_COMPLETED' ,
> > + 'data': { 'msg': 'str' } }
>
> This makes it hard to understand whether there was an error or not. I
> suggest using '*error': 'str' instead, and omitting the error if the
> dump is successful.
Will adopt it. Thanks!
Peter
>
> Paolo
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/8] dump-guest-memory: add qmp event DUMP_COMPLETED
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 4/8] dump-guest-memory: add qmp event DUMP_COMPLETED Peter Xu
2015-11-27 5:19 ` Fam Zheng
2015-11-27 10:15 ` Paolo Bonzini
@ 2015-11-30 18:18 ` Eric Blake
2015-12-01 1:52 ` Peter Xu
2 siblings, 1 reply; 43+ messages in thread
From: Eric Blake @ 2015-11-30 18:18 UTC (permalink / raw)
To: Peter Xu, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2062 bytes --]
On 11/26/2015 07:48 PM, Peter Xu wrote:
> To get aligned with QMP interface, one new QMP event DUMP_COMPLETED
> is added. It is used when user specified "detach" in dump, and
> triggered when the dump finishes. Error message will be appended to
> this event if the dump has failed.
Why not emit the new event unconditionally, instead of only when detach
was specified?
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> docs/qmp-events.txt | 12 ++++++++++++
> dump.c | 12 +++++++++++-
> qapi/event.json | 10 ++++++++++
> 3 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
> index d2f1ce4..fe494f9 100644
> --- a/docs/qmp-events.txt
> +++ b/docs/qmp-events.txt
> @@ -674,3 +674,15 @@ Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is
> followed respectively by the RESET, SHUTDOWN, or STOP events.
>
> Note: this event is rate-limited.
> +
> +DUMP_COMPLETED
> +--------------
> +
> +Emitted when the guest has finished one memory dump.
> +
> +Data: None.
Wrong - you have 'msg' as data. Except that Paolo is right, 'msg'
should be optional, and only present on error. You should also document
that the contents of 'msg' are for human consumption and should not be
machine-parsed (basically, only the presence of absence of 'msg' is
useful for machines).
> +
> +Example:
> +
> +{ "event": "DUMP_COMPLETED",
> + "data": { "msg": "Dump completed successfully" } }
> diff --git a/dump.c b/dump.c
> +++ b/qapi/event.json
> @@ -356,3 +356,13 @@
> ##
> { 'event': 'MEM_UNPLUG_ERROR',
> 'data': { 'device': 'str', 'msg': 'str' } }
> +
> +##
> +# @DUMP_COMPLETED
> +#
> +# Emitted when background dump has completed
> +#
Missing documentation of 'msg', which should be optional.
> +# Since: 2.6
> +##
> +{ 'event': 'DUMP_COMPLETED' ,
> + 'data': { 'msg': 'str' } }
>
--
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 --]
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 4/8] dump-guest-memory: add qmp event DUMP_COMPLETED
2015-11-30 18:18 ` Eric Blake
@ 2015-12-01 1:52 ` Peter Xu
0 siblings, 0 replies; 43+ messages in thread
From: Peter Xu @ 2015-12-01 1:52 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
On Mon, Nov 30, 2015 at 11:18:48AM -0700, Eric Blake wrote:
> On 11/26/2015 07:48 PM, Peter Xu wrote:
> > To get aligned with QMP interface, one new QMP event DUMP_COMPLETED
> > is added. It is used when user specified "detach" in dump, and
> > triggered when the dump finishes. Error message will be appended to
> > this event if the dump has failed.
>
> Why not emit the new event unconditionally, instead of only when detach
> was specified?
I was just trying to make sure that dump will work exactly as it is
when detach is not used. Yes, I think I could emit it
unconditionally if libvirt does not complain about it. :)
>
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > docs/qmp-events.txt | 12 ++++++++++++
> > dump.c | 12 +++++++++++-
> > qapi/event.json | 10 ++++++++++
> > 3 files changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
> > index d2f1ce4..fe494f9 100644
> > --- a/docs/qmp-events.txt
> > +++ b/docs/qmp-events.txt
> > @@ -674,3 +674,15 @@ Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is
> > followed respectively by the RESET, SHUTDOWN, or STOP events.
> >
> > Note: this event is rate-limited.
> > +
> > +DUMP_COMPLETED
> > +--------------
> > +
> > +Emitted when the guest has finished one memory dump.
> > +
> > +Data: None.
>
> Wrong - you have 'msg' as data. Except that Paolo is right, 'msg'
> should be optional, and only present on error. You should also document
> that the contents of 'msg' are for human consumption and should not be
> machine-parsed (basically, only the presence of absence of 'msg' is
> useful for machines).
I will make sure v4 contains these suggestions. Thanks.
>
> > +
> > +Example:
> > +
> > +{ "event": "DUMP_COMPLETED",
> > + "data": { "msg": "Dump completed successfully" } }
> > diff --git a/dump.c b/dump.c
>
> > +++ b/qapi/event.json
> > @@ -356,3 +356,13 @@
> > ##
> > { 'event': 'MEM_UNPLUG_ERROR',
> > 'data': { 'device': 'str', 'msg': 'str' } }
> > +
> > +##
> > +# @DUMP_COMPLETED
> > +#
> > +# Emitted when background dump has completed
> > +#
>
> Missing documentation of 'msg', which should be optional.
Sorry I missed that. Thanks to point out!
Peter
>
> > +# Since: 2.6
> > +##
> > +{ 'event': 'DUMP_COMPLETED' ,
> > + 'data': { 'msg': 'str' } }
> >
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH v2 5/8] dump-query: add "dump-query" command to query dump status
2015-11-27 2:48 [Qemu-devel] [PATCH v2 0/8] Add basic "detach" support for dump-guest-memory Peter Xu
` (3 preceding siblings ...)
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 4/8] dump-guest-memory: add qmp event DUMP_COMPLETED Peter Xu
@ 2015-11-27 2:48 ` Peter Xu
2015-11-27 5:25 ` Fam Zheng
2015-11-30 18:27 ` Eric Blake
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 6/8] dump-query: implement "status" of "dump-query" command Peter Xu
` (4 subsequent siblings)
9 siblings, 2 replies; 43+ messages in thread
From: Peter Xu @ 2015-11-27 2:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Xu
This patch is only adding the QMP/HMP interface for "dump-query"
command, but not implementing them. This command could be used to
query background dump status. Please refer to the next patch to see
how dump status are defined.
Currently, only fake results are returned.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
dump.c | 9 +++++++++
hmp-commands.hx | 15 +++++++++++++++
hmp.c | 6 ++++++
hmp.h | 1 +
qapi-schema.json | 21 +++++++++++++++++++++
qmp-commands.hx | 29 ++++++++++++++++++++++++++++-
6 files changed, 80 insertions(+), 1 deletion(-)
diff --git a/dump.c b/dump.c
index 0d321d4..446a991 100644
--- a/dump.c
+++ b/dump.c
@@ -1777,6 +1777,15 @@ void qmp_dump_guest_memory(bool paging, const char *file,
}
}
+DumpStatus *qmp_dump_query(Error **errp)
+{
+ DumpStatus *status = g_malloc0(sizeof(*status));
+ /* TBD */
+ status->status = g_strdup("WORKING");
+ status->percentage = g_strdup("50%");
+ return status;
+}
+
DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
{
DumpGuestMemoryFormatList *item;
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 664d794..4ce7721 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1087,6 +1087,21 @@ gdb. Without -z|-l|-s, the dump format is ELF.
together with begin.
ETEXI
+ {
+ .name = "dump-query",
+ .args_type = "",
+ .params = "",
+ .help = "query last guest memory dump status.\n\t\t\t",
+ .mhandler.cmd = hmp_dump_query,
+ },
+
+
+STEXI
+@item dump-query
+@findex dump-query
+Query latest dump status.
+ETEXI
+
#if defined(TARGET_S390X)
{
.name = "dump-skeys",
diff --git a/hmp.c b/hmp.c
index dccb457..6d9c127 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1576,6 +1576,12 @@ void hmp_device_del(Monitor *mon, const QDict *qdict)
hmp_handle_error(mon, &err);
}
+void hmp_dump_query(Monitor *mon, const QDict *qdict)
+{
+ /* TBD */
+ monitor_printf(mon, "QUERY DUMP STATUS\n");
+}
+
void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
{
Error *err = NULL;
diff --git a/hmp.h b/hmp.h
index a8c5b5a..fdde4a3 100644
--- a/hmp.h
+++ b/hmp.h
@@ -85,6 +85,7 @@ void hmp_migrate(Monitor *mon, const QDict *qdict);
void hmp_device_add(Monitor *mon, const QDict *qdict);
void hmp_device_del(Monitor *mon, const QDict *qdict);
void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
+void hmp_dump_query(Monitor *mon, const QDict *qdict);
void hmp_netdev_add(Monitor *mon, const QDict *qdict);
void hmp_netdev_del(Monitor *mon, const QDict *qdict);
void hmp_getfd(Monitor *mon, const QDict *qdict);
diff --git a/qapi-schema.json b/qapi-schema.json
index fd81ce2..5db615d 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2139,6 +2139,27 @@
'*format': 'DumpGuestMemoryFormat'} }
##
+# @DumpStatus
+#
+# Status for the last guest memory dump.
+#
+# Since: 2.6
+##
+{ 'struct': 'DumpStatus',
+ 'data': { 'status': 'str', 'percentage': 'str' } }
+
+##
+# @dump-query
+#
+# Query latest dump status.
+#
+# Returns: A @DumpStatus object showing the dump status.
+#
+# Since: 2.6
+##
+{ 'command': 'dump-query', 'returns': 'DumpStatus' }
+
+##
# @DumpGuestMemoryCapability:
#
# A list of the available formats for dump-guest-memory
diff --git a/qmp-commands.hx b/qmp-commands.hx
index bbb08e1..6d13778 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -879,9 +879,36 @@ Notes:
EQMP
{
+ .name = "dump-query",
+ .args_type = "",
+ .params = "",
+ .help = "query background dump status",
+ .mhandler.cmd_new = qmp_marshal_dump_query,
+ },
+
+SQMP
+dump-query
+----------
+
+Query background dump status.
+
+Arguments: None.
+
+Example:
+
+-> { "execute": "dump-query" }
+<- { "return": {"status": "IN_PROGRESS", "percentage": "85%" } }
+
+Notes:
+
+(1) All boolean arguments default to false
+
+EQMP
+
+ {
.name = "query-dump-guest-memory-capability",
.args_type = "",
- .mhandler.cmd_new = qmp_marshal_query_dump_guest_memory_capability,
+ .mhandler.cmd_new = qmp_marshal_query_dump_guest_memory_capability,
},
SQMP
--
2.4.3
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 5/8] dump-query: add "dump-query" command to query dump status
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 5/8] dump-query: add "dump-query" command to query dump status Peter Xu
@ 2015-11-27 5:25 ` Fam Zheng
2015-11-27 7:03 ` Peter Xu
2015-11-30 18:27 ` Eric Blake
1 sibling, 1 reply; 43+ messages in thread
From: Fam Zheng @ 2015-11-27 5:25 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel
On Fri, 11/27 10:48, Peter Xu wrote:
> This patch is only adding the QMP/HMP interface for "dump-query"
> command, but not implementing them. This command could be used to
> query background dump status. Please refer to the next patch to see
> how dump status are defined.
>
> Currently, only fake results are returned.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> dump.c | 9 +++++++++
> hmp-commands.hx | 15 +++++++++++++++
> hmp.c | 6 ++++++
> hmp.h | 1 +
> qapi-schema.json | 21 +++++++++++++++++++++
> qmp-commands.hx | 29 ++++++++++++++++++++++++++++-
> 6 files changed, 80 insertions(+), 1 deletion(-)
>
> diff --git a/dump.c b/dump.c
> index 0d321d4..446a991 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -1777,6 +1777,15 @@ void qmp_dump_guest_memory(bool paging, const char *file,
> }
> }
>
> +DumpStatus *qmp_dump_query(Error **errp)
> +{
> + DumpStatus *status = g_malloc0(sizeof(*status));
> + /* TBD */
> + status->status = g_strdup("WORKING");
> + status->percentage = g_strdup("50%");
> + return status;
> +}
> +
> DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
> {
> DumpGuestMemoryFormatList *item;
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 664d794..4ce7721 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1087,6 +1087,21 @@ gdb. Without -z|-l|-s, the dump format is ELF.
> together with begin.
> ETEXI
>
> + {
> + .name = "dump-query",
> + .args_type = "",
> + .params = "",
> + .help = "query last guest memory dump status.\n\t\t\t",
What are "\n\t\t\t" doing here?
> + .mhandler.cmd = hmp_dump_query,
> + },
> +
> +
> +STEXI
> +@item dump-query
> +@findex dump-query
> +Query latest dump status.
> +ETEXI
> +
> #if defined(TARGET_S390X)
> {
> .name = "dump-skeys",
> diff --git a/hmp.c b/hmp.c
> index dccb457..6d9c127 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1576,6 +1576,12 @@ void hmp_device_del(Monitor *mon, const QDict *qdict)
> hmp_handle_error(mon, &err);
> }
>
> +void hmp_dump_query(Monitor *mon, const QDict *qdict)
> +{
> + /* TBD */
> + monitor_printf(mon, "QUERY DUMP STATUS\n");
> +}
> +
> void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
> {
> Error *err = NULL;
> diff --git a/hmp.h b/hmp.h
> index a8c5b5a..fdde4a3 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -85,6 +85,7 @@ void hmp_migrate(Monitor *mon, const QDict *qdict);
> void hmp_device_add(Monitor *mon, const QDict *qdict);
> void hmp_device_del(Monitor *mon, const QDict *qdict);
> void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
> +void hmp_dump_query(Monitor *mon, const QDict *qdict);
> void hmp_netdev_add(Monitor *mon, const QDict *qdict);
> void hmp_netdev_del(Monitor *mon, const QDict *qdict);
> void hmp_getfd(Monitor *mon, const QDict *qdict);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index fd81ce2..5db615d 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2139,6 +2139,27 @@
> '*format': 'DumpGuestMemoryFormat'} }
>
> ##
> +# @DumpStatus
> +#
> +# Status for the last guest memory dump.
> +#
> +# Since: 2.6
> +##
> +{ 'struct': 'DumpStatus',
> + 'data': { 'status': 'str', 'percentage': 'str' } }
I suggest using enum for "status" and int for "percentage" (or two ints for
"total" and "current").
> +
> +##
> +# @dump-query
> +#
> +# Query latest dump status.
> +#
> +# Returns: A @DumpStatus object showing the dump status.
> +#
> +# Since: 2.6
> +##
> +{ 'command': 'dump-query', 'returns': 'DumpStatus' }
> +
> +##
> # @DumpGuestMemoryCapability:
> #
> # A list of the available formats for dump-guest-memory
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index bbb08e1..6d13778 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -879,9 +879,36 @@ Notes:
> EQMP
>
> {
> + .name = "dump-query",
> + .args_type = "",
> + .params = "",
> + .help = "query background dump status",
> + .mhandler.cmd_new = qmp_marshal_dump_query,
> + },
> +
> +SQMP
> +dump-query
> +----------
> +
> +Query background dump status.
> +
> +Arguments: None.
> +
> +Example:
> +
> +-> { "execute": "dump-query" }
> +<- { "return": {"status": "IN_PROGRESS", "percentage": "85%" } }
> +
> +Notes:
> +
> +(1) All boolean arguments default to false
Which "boolean arguments"?
> +
> +EQMP
> +
> + {
> .name = "query-dump-guest-memory-capability",
> .args_type = "",
> - .mhandler.cmd_new = qmp_marshal_query_dump_guest_memory_capability,
> + .mhandler.cmd_new = qmp_marshal_query_dump_guest_memory_capability,
> },
>
> SQMP
> --
> 2.4.3
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 5/8] dump-query: add "dump-query" command to query dump status
2015-11-27 5:25 ` Fam Zheng
@ 2015-11-27 7:03 ` Peter Xu
2015-11-27 10:17 ` Paolo Bonzini
0 siblings, 1 reply; 43+ messages in thread
From: Peter Xu @ 2015-11-27 7:03 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-devel
On 11/27/2015 01:25 PM, Fam Zheng wrote:
> On Fri, 11/27 10:48, Peter Xu wrote:
>> This patch is only adding the QMP/HMP interface for "dump-query"
>> command, but not implementing them. This command could be used to
>> query background dump status. Please refer to the next patch to see
>> how dump status are defined.
>>
>> Currently, only fake results are returned.
>>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> ---
>> dump.c | 9 +++++++++
>> hmp-commands.hx | 15 +++++++++++++++
>> hmp.c | 6 ++++++
>> hmp.h | 1 +
>> qapi-schema.json | 21 +++++++++++++++++++++
>> qmp-commands.hx | 29 ++++++++++++++++++++++++++++-
>> 6 files changed, 80 insertions(+), 1 deletion(-)
>>
>> diff --git a/dump.c b/dump.c
>> index 0d321d4..446a991 100644
>> --- a/dump.c
>> +++ b/dump.c
>> @@ -1777,6 +1777,15 @@ void qmp_dump_guest_memory(bool paging, const char *file,
>> }
>> }
>>
>> +DumpStatus *qmp_dump_query(Error **errp)
>> +{
>> + DumpStatus *status = g_malloc0(sizeof(*status));
>> + /* TBD */
>> + status->status = g_strdup("WORKING");
>> + status->percentage = g_strdup("50%");
>> + return status;
>> +}
>> +
>> DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
>> {
>> DumpGuestMemoryFormatList *item;
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 664d794..4ce7721 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -1087,6 +1087,21 @@ gdb. Without -z|-l|-s, the dump format is ELF.
>> together with begin.
>> ETEXI
>>
>> + {
>> + .name = "dump-query",
>> + .args_type = "",
>> + .params = "",
>> + .help = "query last guest memory dump status.\n\t\t\t",
>
> What are "\n\t\t\t" doing here?
Sorry I should remove that.
>
>> + .mhandler.cmd = hmp_dump_query,
>> + },
>> +
>> +
>> +STEXI
>> +@item dump-query
>> +@findex dump-query
>> +Query latest dump status.
>> +ETEXI
>> +
>> #if defined(TARGET_S390X)
>> {
>> .name = "dump-skeys",
>> diff --git a/hmp.c b/hmp.c
>> index dccb457..6d9c127 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -1576,6 +1576,12 @@ void hmp_device_del(Monitor *mon, const QDict *qdict)
>> hmp_handle_error(mon, &err);
>> }
>>
>> +void hmp_dump_query(Monitor *mon, const QDict *qdict)
>> +{
>> + /* TBD */
>> + monitor_printf(mon, "QUERY DUMP STATUS\n");
>> +}
>> +
>> void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
>> {
>> Error *err = NULL;
>> diff --git a/hmp.h b/hmp.h
>> index a8c5b5a..fdde4a3 100644
>> --- a/hmp.h
>> +++ b/hmp.h
>> @@ -85,6 +85,7 @@ void hmp_migrate(Monitor *mon, const QDict *qdict);
>> void hmp_device_add(Monitor *mon, const QDict *qdict);
>> void hmp_device_del(Monitor *mon, const QDict *qdict);
>> void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
>> +void hmp_dump_query(Monitor *mon, const QDict *qdict);
>> void hmp_netdev_add(Monitor *mon, const QDict *qdict);
>> void hmp_netdev_del(Monitor *mon, const QDict *qdict);
>> void hmp_getfd(Monitor *mon, const QDict *qdict);
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index fd81ce2..5db615d 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -2139,6 +2139,27 @@
>> '*format': 'DumpGuestMemoryFormat'} }
>>
>> ##
>> +# @DumpStatus
>> +#
>> +# Status for the last guest memory dump.
>> +#
>> +# Since: 2.6
>> +##
>> +{ 'struct': 'DumpStatus',
>> + 'data': { 'status': 'str', 'percentage': 'str' } }
>
> I suggest using enum for "status" and int for "percentage" (or two ints for
> "total" and "current").
Yes, I picked string just for flexibility. Regarding to this
interface issue, I'd like to wait for others' comments too before
modification.
>
>> +
>> +##
>> +# @dump-query
>> +#
>> +# Query latest dump status.
>> +#
>> +# Returns: A @DumpStatus object showing the dump status.
>> +#
>> +# Since: 2.6
>> +##
>> +{ 'command': 'dump-query', 'returns': 'DumpStatus' }
>> +
>> +##
>> # @DumpGuestMemoryCapability:
>> #
>> # A list of the available formats for dump-guest-memory
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index bbb08e1..6d13778 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -879,9 +879,36 @@ Notes:
>> EQMP
>>
>> {
>> + .name = "dump-query",
>> + .args_type = "",
>> + .params = "",
>> + .help = "query background dump status",
>> + .mhandler.cmd_new = qmp_marshal_dump_query,
>> + },
>> +
>> +SQMP
>> +dump-query
>> +----------
>> +
>> +Query background dump status.
>> +
>> +Arguments: None.
>> +
>> +Example:
>> +
>> +-> { "execute": "dump-query" }
>> +<- { "return": {"status": "IN_PROGRESS", "percentage": "85%" } }
>> +
>> +Notes:
>> +
>> +(1) All boolean arguments default to false
>
> Which "boolean arguments"?
Should remove. Sorry.
Peter
>
>
>> +
>> +EQMP
>> +
>> + {
>> .name = "query-dump-guest-memory-capability",
>> .args_type = "",
>> - .mhandler.cmd_new = qmp_marshal_query_dump_guest_memory_capability,
>> + .mhandler.cmd_new = qmp_marshal_query_dump_guest_memory_capability,
>> },
>>
>> SQMP
>> --
>> 2.4.3
>>
>>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 5/8] dump-query: add "dump-query" command to query dump status
2015-11-27 7:03 ` Peter Xu
@ 2015-11-27 10:17 ` Paolo Bonzini
2015-11-27 11:33 ` Peter Xu
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2015-11-27 10:17 UTC (permalink / raw)
To: Peter Xu, Fam Zheng; +Cc: qemu-devel
On 27/11/2015 08:03, Peter Xu wrote:
> > > +{ 'struct': 'DumpStatus',
> > > + 'data': { 'status': 'str', 'percentage': 'str' } }
> >
> > I suggest using enum for "status" and int for "percentage" (or two ints for
> > "total" and "current").
>
> Yes, I picked string just for flexibility. Regarding to this
> interface issue, I'd like to wait for others' comments too before
> modification.
I like Fam's idea of using an enum and two ints.
Also, the command should be named query-dump and the corresponding HMP
command should be "info dump" (by defining "dump" in hmp-commands-info.h".
Paolo
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 5/8] dump-query: add "dump-query" command to query dump status
2015-11-27 10:17 ` Paolo Bonzini
@ 2015-11-27 11:33 ` Peter Xu
0 siblings, 0 replies; 43+ messages in thread
From: Peter Xu @ 2015-11-27 11:33 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Fam Zheng, qemu-devel
On Fri, Nov 27, 2015 at 11:17:52AM +0100, Paolo Bonzini wrote:
>
>
> On 27/11/2015 08:03, Peter Xu wrote:
> > > > +{ 'struct': 'DumpStatus',
> > > > + 'data': { 'status': 'str', 'percentage': 'str' } }
> > >
> > > I suggest using enum for "status" and int for "percentage" (or two ints for
> > > "total" and "current").
> >
> > Yes, I picked string just for flexibility. Regarding to this
> > interface issue, I'd like to wait for others' comments too before
> > modification.
>
> I like Fam's idea of using an enum and two ints.
>
> Also, the command should be named query-dump and the corresponding HMP
> command should be "info dump" (by defining "dump" in hmp-commands-info.h".
Ok, will change in v3.
Thanks.
Peter
>
> Paolo
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/8] dump-query: add "dump-query" command to query dump status
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 5/8] dump-query: add "dump-query" command to query dump status Peter Xu
2015-11-27 5:25 ` Fam Zheng
@ 2015-11-30 18:27 ` Eric Blake
2015-12-01 2:03 ` Peter Xu
1 sibling, 1 reply; 43+ messages in thread
From: Eric Blake @ 2015-11-30 18:27 UTC (permalink / raw)
To: Peter Xu, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2441 bytes --]
On 11/26/2015 07:48 PM, Peter Xu wrote:
> This patch is only adding the QMP/HMP interface for "dump-query"
> command, but not implementing them. This command could be used to
> query background dump status. Please refer to the next patch to see
> how dump status are defined.
>
> Currently, only fake results are returned.
Feels a bit awkward to return fake results; maybe you should squash some
patches together or fail with an error instead of returning fake results.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> dump.c | 9 +++++++++
> hmp-commands.hx | 15 +++++++++++++++
> hmp.c | 6 ++++++
> hmp.h | 1 +
> qapi-schema.json | 21 +++++++++++++++++++++
> qmp-commands.hx | 29 ++++++++++++++++++++++++++++-
> 6 files changed, 80 insertions(+), 1 deletion(-)
>
> +++ b/qapi-schema.json
> @@ -2139,6 +2139,27 @@
> '*format': 'DumpGuestMemoryFormat'} }
>
> ##
> +# @DumpStatus
> +#
> +# Status for the last guest memory dump.
> +#
Missing documentation for 'status' and 'percentage' fields.
> +# Since: 2.6
> +##
> +{ 'struct': 'DumpStatus',
> + 'data': { 'status': 'str', 'percentage': 'str' } }
What values will 'status' contain? If it is a finite set of status
strings, then it should be an enum type.
'percentage' should NOT be a string. It should probably be numeric;
'number' if you intend to return a floating point value between 0 and 1.
Or, like other interfaces, you should probably return two numbers
(current and total, both 'int'), and let the caller compute percentage
themselves.
> +
> +##
> +# @dump-query
Most query commands are named 'query-FOO', not 'FOO-query'. Unless you
have a compelling reason otherwise, this should be 'query-dump'.
> +#
> +# Query latest dump status.
> +#
> +# Returns: A @DumpStatus object showing the dump status.
> +#
> +# Since: 2.6
> +##
> +{ 'command': 'dump-query', 'returns': 'DumpStatus' }
> +
> +SQMP
> +dump-query
> +----------
> +
> +Query background dump status.
> +
> +Arguments: None.
> +
> +Example:
> +
> +-> { "execute": "dump-query" }
> +<- { "return": {"status": "IN_PROGRESS", "percentage": "85%" } }
ALL_CAPS status is annoying to read; if you add an enum type, it should
be 'lower-case' values.
--
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 --]
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 5/8] dump-query: add "dump-query" command to query dump status
2015-11-30 18:27 ` Eric Blake
@ 2015-12-01 2:03 ` Peter Xu
0 siblings, 0 replies; 43+ messages in thread
From: Peter Xu @ 2015-12-01 2:03 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
On Mon, Nov 30, 2015 at 11:27:35AM -0700, Eric Blake wrote:
> On 11/26/2015 07:48 PM, Peter Xu wrote:
> > This patch is only adding the QMP/HMP interface for "dump-query"
> > command, but not implementing them. This command could be used to
> > query background dump status. Please refer to the next patch to see
> > how dump status are defined.
> >
> > Currently, only fake results are returned.
>
> Feels a bit awkward to return fake results; maybe you should squash some
> patches together or fail with an error instead of returning fake results.
Yes, sorry for that. This was pointed out by Paolo and it's fixed in v3.
>
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > dump.c | 9 +++++++++
> > hmp-commands.hx | 15 +++++++++++++++
> > hmp.c | 6 ++++++
> > hmp.h | 1 +
> > qapi-schema.json | 21 +++++++++++++++++++++
> > qmp-commands.hx | 29 ++++++++++++++++++++++++++++-
> > 6 files changed, 80 insertions(+), 1 deletion(-)
> >
>
> > +++ b/qapi-schema.json
> > @@ -2139,6 +2139,27 @@
> > '*format': 'DumpGuestMemoryFormat'} }
> >
> > ##
> > +# @DumpStatus
> > +#
> > +# Status for the last guest memory dump.
> > +#
>
> Missing documentation for 'status' and 'percentage' fields.
Yes. Fixed in v3. Thanks!
>
> > +# Since: 2.6
> > +##
> > +{ 'struct': 'DumpStatus',
> > + 'data': { 'status': 'str', 'percentage': 'str' } }
>
> What values will 'status' contain? If it is a finite set of status
> strings, then it should be an enum type.
>
> 'percentage' should NOT be a string. It should probably be numeric;
> 'number' if you intend to return a floating point value between 0 and 1.
> Or, like other interfaces, you should probably return two numbers
> (current and total, both 'int'), and let the caller compute percentage
> themselves.
Sorry for the old unclear API. Now it contains:
- status: enum
- written_bytes: int
- total_bytes: int
These were modified in v3 (as Fam & Paolo suggested). Thanks!
>
> > +
> > +##
> > +# @dump-query
>
> Most query commands are named 'query-FOO', not 'FOO-query'. Unless you
> have a compelling reason otherwise, this should be 'query-dump'.
Changed in v3.
>
> > +#
> > +# Query latest dump status.
> > +#
> > +# Returns: A @DumpStatus object showing the dump status.
> > +#
> > +# Since: 2.6
> > +##
> > +{ 'command': 'dump-query', 'returns': 'DumpStatus' }
> > +
>
> > +SQMP
> > +dump-query
> > +----------
> > +
> > +Query background dump status.
> > +
> > +Arguments: None.
> > +
> > +Example:
> > +
> > +-> { "execute": "dump-query" }
> > +<- { "return": {"status": "IN_PROGRESS", "percentage": "85%" } }
>
> ALL_CAPS status is annoying to read; if you add an enum type, it should
> be 'lower-case' values.
Changed in v3.
Thanks!
Peter
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH v2 6/8] dump-query: implement "status" of "dump-query" command.
2015-11-27 2:48 [Qemu-devel] [PATCH v2 0/8] Add basic "detach" support for dump-guest-memory Peter Xu
` (4 preceding siblings ...)
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 5/8] dump-query: add "dump-query" command to query dump status Peter Xu
@ 2015-11-27 2:48 ` Peter Xu
2015-11-27 10:22 ` Paolo Bonzini
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 7/8] DumpState: adding total_size and written_size fields Peter Xu
` (3 subsequent siblings)
9 siblings, 1 reply; 43+ messages in thread
From: Peter Xu @ 2015-11-27 2:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Xu
This patch implements the status changes inside dump process. When
query dump status, three possible results could be:
1. never started dump:
{ "status": "NOT_STARTED", "percentage": "N/A" }
2. one dump is running in background:
{ "status": "IN_PROGRESS", "percentage": "{0..100}%" }
3. no dump is running, and the last dump has finished:
{ "status": "SUCCEEDED|FAILED", "percentage": "N/A" }
It's mostly done. Except that we might need more accurate
"percentage" results (which is now 50% all the time!).
Signed-off-by: Peter Xu <peterx@redhat.com>
---
dump.c | 57 +++++++++++++++++++++++++++++++++++++++++++--------
hmp.c | 7 +++++--
include/qemu-common.h | 4 ++++
include/sysemu/dump.h | 13 ++++++++++--
4 files changed, 68 insertions(+), 13 deletions(-)
diff --git a/dump.c b/dump.c
index 446a991..25298b7 100644
--- a/dump.c
+++ b/dump.c
@@ -1594,11 +1594,44 @@ static GlobalDumpState *dump_state_get_global(void)
if (unlikely(!init)) {
qemu_mutex_init(&global_dump_state.gds_mutex);
global_dump_state.gds_cur = NULL;
+ global_dump_state.gds_result = DUMP_RES_NOT_STARTED;
init = true;
}
return &global_dump_state;
}
+static const char *const dump_result_table[] = {
+ [DUMP_RES_NOT_STARTED] = "NOT_STARTED",
+ [DUMP_RES_IN_PROGRESS] = "IN_PROGRESS",
+ [DUMP_RES_SUCCEEDED] = "SUCCEEDED",
+ [DUMP_RES_FAILED] = "FAILED",
+};
+
+/* Returns DumpStatus. Caller should be responsible to free the
+ * resources using qapi_free_DumpStatus(). */
+DumpStatus *dump_status_query(void)
+{
+ DumpStatus *status = g_malloc0(sizeof(*status));
+ int percentage = 50;
+ char buf[64] = {0};
+
+ GlobalDumpState *global = dump_state_get_global();
+ qemu_mutex_lock(&global->gds_mutex);
+
+ /* TBD: get correct percentage */
+ status->status = g_strdup(dump_result_table[global->gds_result]);
+ if (global->gds_result == DUMP_RES_IN_PROGRESS) {
+ snprintf(buf, sizeof(buf) - 1, "%d%%", percentage);
+ status->percentage = g_strdup(buf);
+ } else {
+ status->percentage = g_strdup("N/A");
+ }
+
+ qemu_mutex_unlock(&global->gds_mutex);
+
+ return status;
+}
+
/* Returns non-zero if there is existing dump in progress, otherwise
* zero is returned. */
bool dump_in_progress(void)
@@ -1620,27 +1653,37 @@ static DumpState *dump_state_create(GlobalDumpState *global)
cur = global->gds_cur;
if (cur) {
/* one dump in progress */
+ assert(global->gds_result == DUMP_RES_IN_PROGRESS);
cur = NULL;
} else {
/* we are the first! */
+ assert(global->gds_result != DUMP_RES_IN_PROGRESS);
cur = g_malloc0(sizeof(*cur));
global->gds_cur = cur;
}
+ global->gds_result = DUMP_RES_IN_PROGRESS;
qemu_mutex_unlock(&global->gds_mutex);
return cur;
}
-/* release current DumpState structure */
-static void dump_state_release(GlobalDumpState *global)
+/* release current DumpState structure. "done" is hint to show
+ * whether the dump is succeeded or not. */
+static void dump_state_release(GlobalDumpState *global, bool done)
{
DumpState *cur = NULL;
qemu_mutex_lock(&global->gds_mutex);
+ assert(global->gds_result == DUMP_RES_IN_PROGRESS);
cur = global->gds_cur;
/* we should never call release before having one */
assert(cur);
global->gds_cur = NULL;
+ if (done) {
+ global->gds_result = DUMP_RES_SUCCEEDED;
+ } else {
+ global->gds_result = DUMP_RES_FAILED;
+ }
qemu_mutex_unlock(&global->gds_mutex);
dump_cleanup(cur);
@@ -1664,7 +1707,7 @@ static void *dump_thread(void *data)
const char *msg = "Dump completed successfully";
dump_process(global->gds_cur, &local_err);
- dump_state_release(global);
+ dump_state_release(global, (local_err == NULL));
/* if detach is used, notify user that dump has finished */
if (local_err) {
@@ -1769,7 +1812,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
if (!detach_p) {
/* sync dump */
dump_process(s, errp);
- dump_state_release(global);
+ dump_state_release(global, (*errp == NULL));
} else {
qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
global, QEMU_THREAD_DETACHED);
@@ -1779,11 +1822,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
DumpStatus *qmp_dump_query(Error **errp)
{
- DumpStatus *status = g_malloc0(sizeof(*status));
- /* TBD */
- status->status = g_strdup("WORKING");
- status->percentage = g_strdup("50%");
- return status;
+ return dump_status_query();
}
DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
diff --git a/hmp.c b/hmp.c
index 6d9c127..049ac4b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1578,8 +1578,11 @@ void hmp_device_del(Monitor *mon, const QDict *qdict)
void hmp_dump_query(Monitor *mon, const QDict *qdict)
{
- /* TBD */
- monitor_printf(mon, "QUERY DUMP STATUS\n");
+ DumpStatus *status = dump_status_query();
+ assert(status);
+ monitor_printf(mon, "STATUS: %s\n", status->status);
+ monitor_printf(mon, "PERCENTAGE: %s\n", status->percentage);
+ qapi_free_DumpStatus(status);
}
void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
diff --git a/include/qemu-common.h b/include/qemu-common.h
index 7b74961..fbfa852 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -505,4 +505,8 @@ void page_size_init(void);
* returned. */
bool dump_in_progress(void);
+/* Returns DumpStatus. Caller should be responsible to free the
+ * resources using qapi_free_DumpStatus(). */
+DumpStatus *dump_status_query(void);
+
#endif
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 13c913e..0f0a463 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -189,9 +189,18 @@ typedef struct DumpState {
DumpGuestMemoryFormat format; /* valid only if has_format == true */
} DumpState;
+typedef enum DumpResult {
+ DUMP_RES_NOT_STARTED = 0,
+ DUMP_RES_IN_PROGRESS,
+ DUMP_RES_SUCCEEDED,
+ DUMP_RES_FAILED,
+ DUMP_RES_MAX,
+} DumpResult;
+
typedef struct GlobalDumpState {
- QemuMutex gds_mutex; /* protector for GlobalDumpState itself */
- DumpState *gds_cur; /* current DumpState (might be NULL) */
+ QemuMutex gds_mutex; /* protector for GlobalDumpState itself */
+ DumpState *gds_cur; /* current DumpState (might be NULL) */
+ DumpResult gds_result; /* current dump result */
} GlobalDumpState;
uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
--
2.4.3
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 6/8] dump-query: implement "status" of "dump-query" command.
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 6/8] dump-query: implement "status" of "dump-query" command Peter Xu
@ 2015-11-27 10:22 ` Paolo Bonzini
2015-11-27 11:42 ` Peter Xu
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2015-11-27 10:22 UTC (permalink / raw)
To: Peter Xu, qemu-devel
On 27/11/2015 03:48, Peter Xu wrote:
> This patch implements the status changes inside dump process. When
> query dump status, three possible results could be:
>
> 1. never started dump:
> { "status": "NOT_STARTED", "percentage": "N/A" }
>
> 2. one dump is running in background:
> { "status": "IN_PROGRESS", "percentage": "{0..100}%" }
>
> 3. no dump is running, and the last dump has finished:
> { "status": "SUCCEEDED|FAILED", "percentage": "N/A" }
>
> It's mostly done. Except that we might need more accurate
> "percentage" results (which is now 50% all the time!).
As mentioned early, you can use an enum. QEMU usually prints enums in
lowercase and separates words with "-". Similar to MigrationStatus you
can use none, active, completed, failed. In fact you might even reuse
MigrationStatus directly, it's simpler.
The percentage is not necessary as part of the QMP return value. You
can compute it in hmp_info_dump however and print it to HMP only.
I think you can structure the patches like this:
add basic "detach" support
add qmp event DUMP_COMPLETED
add total_size and written_size to DumpState
add "query-dump" QMP command
add "info dump" HMP command
where the fourth patch already sets all fields correctly.
Thanks,
Paolo
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> dump.c | 57 +++++++++++++++++++++++++++++++++++++++++++--------
> hmp.c | 7 +++++--
> include/qemu-common.h | 4 ++++
> include/sysemu/dump.h | 13 ++++++++++--
> 4 files changed, 68 insertions(+), 13 deletions(-)
>
> diff --git a/dump.c b/dump.c
> index 446a991..25298b7 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -1594,11 +1594,44 @@ static GlobalDumpState *dump_state_get_global(void)
> if (unlikely(!init)) {
> qemu_mutex_init(&global_dump_state.gds_mutex);
> global_dump_state.gds_cur = NULL;
> + global_dump_state.gds_result = DUMP_RES_NOT_STARTED;
> init = true;
> }
> return &global_dump_state;
> }
>
> +static const char *const dump_result_table[] = {
> + [DUMP_RES_NOT_STARTED] = "NOT_STARTED",
> + [DUMP_RES_IN_PROGRESS] = "IN_PROGRESS",
> + [DUMP_RES_SUCCEEDED] = "SUCCEEDED",
> + [DUMP_RES_FAILED] = "FAILED",
> +};
> +
> +/* Returns DumpStatus. Caller should be responsible to free the
> + * resources using qapi_free_DumpStatus(). */
> +DumpStatus *dump_status_query(void)
> +{
> + DumpStatus *status = g_malloc0(sizeof(*status));
> + int percentage = 50;
> + char buf[64] = {0};
> +
> + GlobalDumpState *global = dump_state_get_global();
> + qemu_mutex_lock(&global->gds_mutex);
> +
> + /* TBD: get correct percentage */
> + status->status = g_strdup(dump_result_table[global->gds_result]);
> + if (global->gds_result == DUMP_RES_IN_PROGRESS) {
> + snprintf(buf, sizeof(buf) - 1, "%d%%", percentage);
> + status->percentage = g_strdup(buf);
> + } else {
> + status->percentage = g_strdup("N/A");
> + }
> +
> + qemu_mutex_unlock(&global->gds_mutex);
> +
> + return status;
> +}
> +
> /* Returns non-zero if there is existing dump in progress, otherwise
> * zero is returned. */
> bool dump_in_progress(void)
> @@ -1620,27 +1653,37 @@ static DumpState *dump_state_create(GlobalDumpState *global)
> cur = global->gds_cur;
> if (cur) {
> /* one dump in progress */
> + assert(global->gds_result == DUMP_RES_IN_PROGRESS);
> cur = NULL;
> } else {
> /* we are the first! */
> + assert(global->gds_result != DUMP_RES_IN_PROGRESS);
> cur = g_malloc0(sizeof(*cur));
> global->gds_cur = cur;
> }
> + global->gds_result = DUMP_RES_IN_PROGRESS;
>
> qemu_mutex_unlock(&global->gds_mutex);
> return cur;
> }
>
> -/* release current DumpState structure */
> -static void dump_state_release(GlobalDumpState *global)
> +/* release current DumpState structure. "done" is hint to show
> + * whether the dump is succeeded or not. */
> +static void dump_state_release(GlobalDumpState *global, bool done)
> {
> DumpState *cur = NULL;
> qemu_mutex_lock(&global->gds_mutex);
>
> + assert(global->gds_result == DUMP_RES_IN_PROGRESS);
> cur = global->gds_cur;
> /* we should never call release before having one */
> assert(cur);
> global->gds_cur = NULL;
> + if (done) {
> + global->gds_result = DUMP_RES_SUCCEEDED;
> + } else {
> + global->gds_result = DUMP_RES_FAILED;
> + }
>
> qemu_mutex_unlock(&global->gds_mutex);
> dump_cleanup(cur);
> @@ -1664,7 +1707,7 @@ static void *dump_thread(void *data)
> const char *msg = "Dump completed successfully";
>
> dump_process(global->gds_cur, &local_err);
> - dump_state_release(global);
> + dump_state_release(global, (local_err == NULL));
>
> /* if detach is used, notify user that dump has finished */
> if (local_err) {
> @@ -1769,7 +1812,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
> if (!detach_p) {
> /* sync dump */
> dump_process(s, errp);
> - dump_state_release(global);
> + dump_state_release(global, (*errp == NULL));
> } else {
> qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
> global, QEMU_THREAD_DETACHED);
> @@ -1779,11 +1822,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
>
> DumpStatus *qmp_dump_query(Error **errp)
> {
> - DumpStatus *status = g_malloc0(sizeof(*status));
> - /* TBD */
> - status->status = g_strdup("WORKING");
> - status->percentage = g_strdup("50%");
> - return status;
> + return dump_status_query();
> }
>
> DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
> diff --git a/hmp.c b/hmp.c
> index 6d9c127..049ac4b 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1578,8 +1578,11 @@ void hmp_device_del(Monitor *mon, const QDict *qdict)
>
> void hmp_dump_query(Monitor *mon, const QDict *qdict)
> {
> - /* TBD */
> - monitor_printf(mon, "QUERY DUMP STATUS\n");
> + DumpStatus *status = dump_status_query();
> + assert(status);
> + monitor_printf(mon, "STATUS: %s\n", status->status);
> + monitor_printf(mon, "PERCENTAGE: %s\n", status->percentage);
> + qapi_free_DumpStatus(status);
> }
>
> void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 7b74961..fbfa852 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -505,4 +505,8 @@ void page_size_init(void);
> * returned. */
> bool dump_in_progress(void);
>
> +/* Returns DumpStatus. Caller should be responsible to free the
> + * resources using qapi_free_DumpStatus(). */
> +DumpStatus *dump_status_query(void);
> +
> #endif
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index 13c913e..0f0a463 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -189,9 +189,18 @@ typedef struct DumpState {
> DumpGuestMemoryFormat format; /* valid only if has_format == true */
> } DumpState;
>
> +typedef enum DumpResult {
> + DUMP_RES_NOT_STARTED = 0,
> + DUMP_RES_IN_PROGRESS,
> + DUMP_RES_SUCCEEDED,
> + DUMP_RES_FAILED,
> + DUMP_RES_MAX,
> +} DumpResult;
> +
> typedef struct GlobalDumpState {
> - QemuMutex gds_mutex; /* protector for GlobalDumpState itself */
> - DumpState *gds_cur; /* current DumpState (might be NULL) */
> + QemuMutex gds_mutex; /* protector for GlobalDumpState itself */
> + DumpState *gds_cur; /* current DumpState (might be NULL) */
> + DumpResult gds_result; /* current dump result */
> } GlobalDumpState;
>
> uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 6/8] dump-query: implement "status" of "dump-query" command.
2015-11-27 10:22 ` Paolo Bonzini
@ 2015-11-27 11:42 ` Peter Xu
2015-11-27 11:53 ` Paolo Bonzini
0 siblings, 1 reply; 43+ messages in thread
From: Peter Xu @ 2015-11-27 11:42 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On Fri, Nov 27, 2015 at 11:22:48AM +0100, Paolo Bonzini wrote:
>
>
> On 27/11/2015 03:48, Peter Xu wrote:
> > This patch implements the status changes inside dump process. When
> > query dump status, three possible results could be:
> >
> > 1. never started dump:
> > { "status": "NOT_STARTED", "percentage": "N/A" }
> >
> > 2. one dump is running in background:
> > { "status": "IN_PROGRESS", "percentage": "{0..100}%" }
> >
> > 3. no dump is running, and the last dump has finished:
> > { "status": "SUCCEEDED|FAILED", "percentage": "N/A" }
> >
> > It's mostly done. Except that we might need more accurate
> > "percentage" results (which is now 50% all the time!).
>
> As mentioned early, you can use an enum. QEMU usually prints enums in
> lowercase and separates words with "-". Similar to MigrationStatus you
> can use none, active, completed, failed. In fact you might even reuse
> MigrationStatus directly, it's simpler.
I think it might be a little big confusing if I use MigrationStatus
directly. I can define a enum.
>
> The percentage is not necessary as part of the QMP return value. You
> can compute it in hmp_info_dump however and print it to HMP only.
Ok, Then I will drop percentage in QMP query.
>
> I think you can structure the patches like this:
>
> add basic "detach" support
> add qmp event DUMP_COMPLETED
> add total_size and written_size to DumpState
> add "query-dump" QMP command
> add "info dump" HMP command
>
> where the fourth patch already sets all fields correctly.
Ok. Will reorder the patches and make sure there are no fake values
any more.
Thanks!
Peter
>
> Thanks,
>
> Paolo
>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > dump.c | 57 +++++++++++++++++++++++++++++++++++++++++++--------
> > hmp.c | 7 +++++--
> > include/qemu-common.h | 4 ++++
> > include/sysemu/dump.h | 13 ++++++++++--
> > 4 files changed, 68 insertions(+), 13 deletions(-)
> >
> > diff --git a/dump.c b/dump.c
> > index 446a991..25298b7 100644
> > --- a/dump.c
> > +++ b/dump.c
> > @@ -1594,11 +1594,44 @@ static GlobalDumpState *dump_state_get_global(void)
> > if (unlikely(!init)) {
> > qemu_mutex_init(&global_dump_state.gds_mutex);
> > global_dump_state.gds_cur = NULL;
> > + global_dump_state.gds_result = DUMP_RES_NOT_STARTED;
> > init = true;
> > }
> > return &global_dump_state;
> > }
> >
> > +static const char *const dump_result_table[] = {
> > + [DUMP_RES_NOT_STARTED] = "NOT_STARTED",
> > + [DUMP_RES_IN_PROGRESS] = "IN_PROGRESS",
> > + [DUMP_RES_SUCCEEDED] = "SUCCEEDED",
> > + [DUMP_RES_FAILED] = "FAILED",
> > +};
> > +
> > +/* Returns DumpStatus. Caller should be responsible to free the
> > + * resources using qapi_free_DumpStatus(). */
> > +DumpStatus *dump_status_query(void)
> > +{
> > + DumpStatus *status = g_malloc0(sizeof(*status));
> > + int percentage = 50;
> > + char buf[64] = {0};
> > +
> > + GlobalDumpState *global = dump_state_get_global();
> > + qemu_mutex_lock(&global->gds_mutex);
> > +
> > + /* TBD: get correct percentage */
> > + status->status = g_strdup(dump_result_table[global->gds_result]);
> > + if (global->gds_result == DUMP_RES_IN_PROGRESS) {
> > + snprintf(buf, sizeof(buf) - 1, "%d%%", percentage);
> > + status->percentage = g_strdup(buf);
> > + } else {
> > + status->percentage = g_strdup("N/A");
> > + }
> > +
> > + qemu_mutex_unlock(&global->gds_mutex);
> > +
> > + return status;
> > +}
> > +
> > /* Returns non-zero if there is existing dump in progress, otherwise
> > * zero is returned. */
> > bool dump_in_progress(void)
> > @@ -1620,27 +1653,37 @@ static DumpState *dump_state_create(GlobalDumpState *global)
> > cur = global->gds_cur;
> > if (cur) {
> > /* one dump in progress */
> > + assert(global->gds_result == DUMP_RES_IN_PROGRESS);
> > cur = NULL;
> > } else {
> > /* we are the first! */
> > + assert(global->gds_result != DUMP_RES_IN_PROGRESS);
> > cur = g_malloc0(sizeof(*cur));
> > global->gds_cur = cur;
> > }
> > + global->gds_result = DUMP_RES_IN_PROGRESS;
> >
> > qemu_mutex_unlock(&global->gds_mutex);
> > return cur;
> > }
> >
> > -/* release current DumpState structure */
> > -static void dump_state_release(GlobalDumpState *global)
> > +/* release current DumpState structure. "done" is hint to show
> > + * whether the dump is succeeded or not. */
> > +static void dump_state_release(GlobalDumpState *global, bool done)
> > {
> > DumpState *cur = NULL;
> > qemu_mutex_lock(&global->gds_mutex);
> >
> > + assert(global->gds_result == DUMP_RES_IN_PROGRESS);
> > cur = global->gds_cur;
> > /* we should never call release before having one */
> > assert(cur);
> > global->gds_cur = NULL;
> > + if (done) {
> > + global->gds_result = DUMP_RES_SUCCEEDED;
> > + } else {
> > + global->gds_result = DUMP_RES_FAILED;
> > + }
> >
> > qemu_mutex_unlock(&global->gds_mutex);
> > dump_cleanup(cur);
> > @@ -1664,7 +1707,7 @@ static void *dump_thread(void *data)
> > const char *msg = "Dump completed successfully";
> >
> > dump_process(global->gds_cur, &local_err);
> > - dump_state_release(global);
> > + dump_state_release(global, (local_err == NULL));
> >
> > /* if detach is used, notify user that dump has finished */
> > if (local_err) {
> > @@ -1769,7 +1812,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
> > if (!detach_p) {
> > /* sync dump */
> > dump_process(s, errp);
> > - dump_state_release(global);
> > + dump_state_release(global, (*errp == NULL));
> > } else {
> > qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
> > global, QEMU_THREAD_DETACHED);
> > @@ -1779,11 +1822,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
> >
> > DumpStatus *qmp_dump_query(Error **errp)
> > {
> > - DumpStatus *status = g_malloc0(sizeof(*status));
> > - /* TBD */
> > - status->status = g_strdup("WORKING");
> > - status->percentage = g_strdup("50%");
> > - return status;
> > + return dump_status_query();
> > }
> >
> > DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
> > diff --git a/hmp.c b/hmp.c
> > index 6d9c127..049ac4b 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -1578,8 +1578,11 @@ void hmp_device_del(Monitor *mon, const QDict *qdict)
> >
> > void hmp_dump_query(Monitor *mon, const QDict *qdict)
> > {
> > - /* TBD */
> > - monitor_printf(mon, "QUERY DUMP STATUS\n");
> > + DumpStatus *status = dump_status_query();
> > + assert(status);
> > + monitor_printf(mon, "STATUS: %s\n", status->status);
> > + monitor_printf(mon, "PERCENTAGE: %s\n", status->percentage);
> > + qapi_free_DumpStatus(status);
> > }
> >
> > void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
> > diff --git a/include/qemu-common.h b/include/qemu-common.h
> > index 7b74961..fbfa852 100644
> > --- a/include/qemu-common.h
> > +++ b/include/qemu-common.h
> > @@ -505,4 +505,8 @@ void page_size_init(void);
> > * returned. */
> > bool dump_in_progress(void);
> >
> > +/* Returns DumpStatus. Caller should be responsible to free the
> > + * resources using qapi_free_DumpStatus(). */
> > +DumpStatus *dump_status_query(void);
> > +
> > #endif
> > diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> > index 13c913e..0f0a463 100644
> > --- a/include/sysemu/dump.h
> > +++ b/include/sysemu/dump.h
> > @@ -189,9 +189,18 @@ typedef struct DumpState {
> > DumpGuestMemoryFormat format; /* valid only if has_format == true */
> > } DumpState;
> >
> > +typedef enum DumpResult {
> > + DUMP_RES_NOT_STARTED = 0,
> > + DUMP_RES_IN_PROGRESS,
> > + DUMP_RES_SUCCEEDED,
> > + DUMP_RES_FAILED,
> > + DUMP_RES_MAX,
> > +} DumpResult;
> > +
> > typedef struct GlobalDumpState {
> > - QemuMutex gds_mutex; /* protector for GlobalDumpState itself */
> > - DumpState *gds_cur; /* current DumpState (might be NULL) */
> > + QemuMutex gds_mutex; /* protector for GlobalDumpState itself */
> > + DumpState *gds_cur; /* current DumpState (might be NULL) */
> > + DumpResult gds_result; /* current dump result */
> > } GlobalDumpState;
> >
> > uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
> >
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 6/8] dump-query: implement "status" of "dump-query" command.
2015-11-27 11:42 ` Peter Xu
@ 2015-11-27 11:53 ` Paolo Bonzini
0 siblings, 0 replies; 43+ messages in thread
From: Paolo Bonzini @ 2015-11-27 11:53 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel
On 27/11/2015 12:42, Peter Xu wrote:
> > As mentioned early, you can use an enum. QEMU usually prints enums in
> > lowercase and separates words with "-". Similar to MigrationStatus you
> > can use none, active, completed, failed. In fact you might even reuse
> > MigrationStatus directly, it's simpler.
>
> I think it might be a little big confusing if I use MigrationStatus
> directly. I can define a enum.
Sure, that's fine. Though there is more than a little similarity
between migration and dump... (essentially, migration is a dump with a
custom format and with additional support for storing device state).
Paolo
^ permalink raw reply [flat|nested] 43+ messages in thread
* [Qemu-devel] [PATCH v2 7/8] DumpState: adding total_size and written_size fields
2015-11-27 2:48 [Qemu-devel] [PATCH v2 0/8] Add basic "detach" support for dump-guest-memory Peter Xu
` (5 preceding siblings ...)
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 6/8] dump-query: implement "status" of "dump-query" command Peter Xu
@ 2015-11-27 2:48 ` Peter Xu
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 8/8] dump-query: make the percentage accurate Peter Xu
` (2 subsequent siblings)
9 siblings, 0 replies; 43+ messages in thread
From: Peter Xu @ 2015-11-27 2:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Xu
Here, total_size is the size in bytes to be dumped (raw data, which
means before compression), while written_size are bytes handled (raw
size too).
These two fields could be used when do "dump-query". With these
information, we could get a very accurate percentage of finished
work.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
dump.c | 36 +++++++++++++++++++++++++++++++++---
include/sysemu/dump.h | 9 +++++++++
2 files changed, 42 insertions(+), 3 deletions(-)
diff --git a/dump.c b/dump.c
index 25298b7..65bd5fb 100644
--- a/dump.c
+++ b/dump.c
@@ -334,6 +334,8 @@ static void write_data(DumpState *s, void *buf, int length, Error **errp)
if (ret < 0) {
error_setg(errp, "dump: failed to save memory");
}
+
+ s->written_size += length;
}
/* write the memory to vmcore. 1 page per I/O. */
@@ -1302,6 +1304,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
goto out;
}
}
+ s->written_size += TARGET_PAGE_SIZE;
}
ret = write_cache(&page_desc, NULL, 0, true);
@@ -1420,6 +1423,30 @@ static void get_max_mapnr(DumpState *s)
s->max_mapnr = paddr_to_pfn(last_block->target_end);
}
+/* calculate total size of memory to be dumped (taking filter into
+ * acoount.) */
+static size_t dump_calculate_size(DumpState *s)
+{
+ GuestPhysBlock *block;
+ int64_t size = 0, total = 0, left = 0, right = 0;
+
+ QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
+ if (s->has_filter) {
+ /* calculate the overlapped region. */
+ left = MAX(s->begin, block->target_start);
+ right = MIN(s->begin + s->length, block->target_end);
+ size = right - left;
+ size = size > 0 ? size : 0;
+ } else {
+ /* count the whole region in */
+ size = (block->target_end - block->target_start);
+ }
+ total += size;
+ }
+
+ return total;
+}
+
static void dump_init(DumpState *s, int fd, bool has_format,
DumpGuestMemoryFormat format, bool paging, bool has_filter,
int64_t begin, int64_t length, Error **errp)
@@ -1431,6 +1458,7 @@ static void dump_init(DumpState *s, int fd, bool has_format,
s->has_format = has_format;
s->format = format;
+ s->written_size = 0;
/* kdump-compressed is conflict with paging and filter */
if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
@@ -1462,6 +1490,10 @@ static void dump_init(DumpState *s, int fd, bool has_format,
guest_phys_blocks_init(&s->guest_phys_blocks);
guest_phys_blocks_append(&s->guest_phys_blocks);
+ s->total_size = dump_calculate_size(s);
+#ifdef DEBUG_DUMP_GUEST_MEMORY
+ fprintf(stderr, "DUMP: total memory to dump: %lu\n", s->total_size);
+#endif
s->start = get_start_block(s);
if (s->start == -1) {
@@ -1680,6 +1712,7 @@ static void dump_state_release(GlobalDumpState *global, bool done)
assert(cur);
global->gds_cur = NULL;
if (done) {
+ assert(cur->total_size == cur->written_size);
global->gds_result = DUMP_RES_SUCCEEDED;
} else {
global->gds_result = DUMP_RES_FAILED;
@@ -1710,9 +1743,6 @@ static void *dump_thread(void *data)
dump_state_release(global, (local_err == NULL));
/* if detach is used, notify user that dump has finished */
- if (local_err) {
- msg = error_get_pretty(local_err);
- }
qapi_event_send_dump_completed(msg, &error_abort);
return NULL;
}
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 0f0a463..d7047ae 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -187,6 +187,15 @@ typedef struct DumpState {
QemuThread dump_thread; /* only used when do async dump */
bool has_format; /* whether format is provided */
DumpGuestMemoryFormat format; /* valid only if has_format == true */
+
+ size_t total_size; /* total memory size (in bytes) to
+ * be dumped. When filter is
+ * enabled, this will only count
+ * those to be written. */
+ size_t written_size; /* written memory size (in bytes),
+ * this could be used to calculate
+ * how many work we have
+ * finished. */
} DumpState;
typedef enum DumpResult {
--
2.4.3
^ permalink raw reply related [flat|nested] 43+ messages in thread* [Qemu-devel] [PATCH v2 8/8] dump-query: make the percentage accurate.
2015-11-27 2:48 [Qemu-devel] [PATCH v2 0/8] Add basic "detach" support for dump-guest-memory Peter Xu
` (6 preceding siblings ...)
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 7/8] DumpState: adding total_size and written_size fields Peter Xu
@ 2015-11-27 2:48 ` Peter Xu
2015-11-27 2:59 ` [Qemu-devel] [PATCH v2 0/8] Add basic "detach" support for dump-guest-memory Peter Xu
2015-11-27 5:28 ` Fam Zheng
9 siblings, 0 replies; 43+ messages in thread
From: Peter Xu @ 2015-11-27 2:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Xu
By calculating the total_size and written_size of memory, we could
get relatively accurate percentage of finished dump.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
dump.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/dump.c b/dump.c
index 65bd5fb..0fcad28 100644
--- a/dump.c
+++ b/dump.c
@@ -1643,8 +1643,9 @@ static const char *const dump_result_table[] = {
* resources using qapi_free_DumpStatus(). */
DumpStatus *dump_status_query(void)
{
+ DumpState *state = NULL;
DumpStatus *status = g_malloc0(sizeof(*status));
- int percentage = 50;
+ float percentage = 0;
char buf[64] = {0};
GlobalDumpState *global = dump_state_get_global();
@@ -1653,7 +1654,9 @@ DumpStatus *dump_status_query(void)
/* TBD: get correct percentage */
status->status = g_strdup(dump_result_table[global->gds_result]);
if (global->gds_result == DUMP_RES_IN_PROGRESS) {
- snprintf(buf, sizeof(buf) - 1, "%d%%", percentage);
+ state = global->gds_cur;
+ percentage = 100.0 * state->written_size / state->total_size;
+ snprintf(buf, sizeof(buf) - 1, "%.2f%%", percentage);
status->percentage = g_strdup(buf);
} else {
status->percentage = g_strdup("N/A");
--
2.4.3
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 0/8] Add basic "detach" support for dump-guest-memory
2015-11-27 2:48 [Qemu-devel] [PATCH v2 0/8] Add basic "detach" support for dump-guest-memory Peter Xu
` (7 preceding siblings ...)
2015-11-27 2:48 ` [Qemu-devel] [PATCH v2 8/8] dump-query: make the percentage accurate Peter Xu
@ 2015-11-27 2:59 ` Peter Xu
2015-11-27 5:28 ` Fam Zheng
9 siblings, 0 replies; 43+ messages in thread
From: Peter Xu @ 2015-11-27 2:59 UTC (permalink / raw)
To: qemu-devel
Cc: Andrew Jones, Laszlo Ersek, Markus Armbruster, Peter Xu,
Luiz Capitulino, Fam Zheng, Paolo Bonzini
Hi, all,
Sorry that I forgot to CC reviewers and maintainers when posting the
patch set. :(
Please review!
Thanks.
Peter
On 11/27/2015 10:48 AM, Peter Xu wrote:
> Sorry that this v2 series cannot be aligned with the v1 series. One
> patch is added at the begining of the series to do some code
> cleanups (also fix potential memory leak). Meanwhile, several new
> patches are appended to the v1 series. Please see the change log for
> more info.
>
> v2 changes:
> - fixed English errors [Drew]
> - reordered the "detach" field, first make it optional, then make sure
> it's order is consistent [Drew, Fam]
> - added doc for new detach flag [Eric]
> - collected error msg even detached [Drew]
> - added qmp event DUMP_COMPLETED to notify user [Eric, Fam]
> - added "dump-query" QMP & HMP commands to query dump status [Eric]
> - "stop" is not allowed when dump in background (also include
> "cont" and "dump-guest-memory") [Fam]
> - added codes to calculate how many dump work finished, which could
> be queried from "dump-query" [Laszlo]
> - added list to track all used MemoryRegion objects, also ref before
> use [Paolo]
> - dump-guest-memory will be forbidden during incoming migrate [Paolo]
> - taking rcu lock when collecting memory info [Paolo]
>
> Test Done:
> - QMP & HMP
> - test default dump (sync), work as usual
> - test detached dump, command return immediately. When dump
> finished, will receive event DUMP_COMPLETED with a message.
> - test dump-query before/during/after dump
> - test kdump with zlib compression, w/ and w/o detach
> - libvirt
> - test "virsh dump --memory-only" with default format and
> kdump-zlib format, work as usual
>
> Peter Xu (8):
> dump-guest-memory: cleanup: removing dump_{error|cleanup}().
> dump-guest-memory: add "detach" flag for QMP/HMP interfaces.
> dump-guest-memory: add basic "detach" support.
> dump-guest-memory: add qmp event DUMP_COMPLETED
> dump-query: add "dump-query" command to query dump status
> dump-query: implement "status" of "dump-query" command.
> DumpState: adding total_size and written_size fields
> dump-query: make the percentage accurate.
>
> docs/qmp-events.txt | 12 ++
> dump.c | 284 ++++++++++++++++++++++++++++++++--------
> hmp-commands.hx | 20 ++-
> hmp.c | 18 ++-
> hmp.h | 1 +
> include/qemu-common.h | 8 ++
> include/sysemu/dump.h | 28 ++++
> include/sysemu/memory_mapping.h | 8 ++
> memory_mapping.c | 46 ++++++-
> qapi-schema.json | 29 +++-
> qapi/event.json | 10 ++
> qmp-commands.hx | 35 ++++-
> qmp.c | 14 ++
> 13 files changed, 450 insertions(+), 63 deletions(-)
>
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [Qemu-devel] [PATCH v2 0/8] Add basic "detach" support for dump-guest-memory
2015-11-27 2:48 [Qemu-devel] [PATCH v2 0/8] Add basic "detach" support for dump-guest-memory Peter Xu
` (8 preceding siblings ...)
2015-11-27 2:59 ` [Qemu-devel] [PATCH v2 0/8] Add basic "detach" support for dump-guest-memory Peter Xu
@ 2015-11-27 5:28 ` Fam Zheng
2015-11-27 6:53 ` Peter Xu
9 siblings, 1 reply; 43+ messages in thread
From: Fam Zheng @ 2015-11-27 5:28 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel
On Fri, 11/27 10:48, Peter Xu wrote:
> Sorry that this v2 series cannot be aligned with the v1 series. One
> patch is added at the begining of the series to do some code
> cleanups (also fix potential memory leak). Meanwhile, several new
> patches are appended to the v1 series. Please see the change log for
> more info.
Looks good overall! I've replied patches with a few questions. Thanks!
Fam
>
> v2 changes:
> - fixed English errors [Drew]
> - reordered the "detach" field, first make it optional, then make sure
> it's order is consistent [Drew, Fam]
> - added doc for new detach flag [Eric]
> - collected error msg even detached [Drew]
> - added qmp event DUMP_COMPLETED to notify user [Eric, Fam]
> - added "dump-query" QMP & HMP commands to query dump status [Eric]
> - "stop" is not allowed when dump in background (also include
> "cont" and "dump-guest-memory") [Fam]
> - added codes to calculate how many dump work finished, which could
> be queried from "dump-query" [Laszlo]
> - added list to track all used MemoryRegion objects, also ref before
> use [Paolo]
> - dump-guest-memory will be forbidden during incoming migrate [Paolo]
> - taking rcu lock when collecting memory info [Paolo]
>
> Test Done:
> - QMP & HMP
> - test default dump (sync), work as usual
> - test detached dump, command return immediately. When dump
> finished, will receive event DUMP_COMPLETED with a message.
> - test dump-query before/during/after dump
> - test kdump with zlib compression, w/ and w/o detach
> - libvirt
> - test "virsh dump --memory-only" with default format and
> kdump-zlib format, work as usual
>
> Peter Xu (8):
> dump-guest-memory: cleanup: removing dump_{error|cleanup}().
> dump-guest-memory: add "detach" flag for QMP/HMP interfaces.
> dump-guest-memory: add basic "detach" support.
> dump-guest-memory: add qmp event DUMP_COMPLETED
> dump-query: add "dump-query" command to query dump status
> dump-query: implement "status" of "dump-query" command.
> DumpState: adding total_size and written_size fields
> dump-query: make the percentage accurate.
>
> docs/qmp-events.txt | 12 ++
> dump.c | 284 ++++++++++++++++++++++++++++++++--------
> hmp-commands.hx | 20 ++-
> hmp.c | 18 ++-
> hmp.h | 1 +
> include/qemu-common.h | 8 ++
> include/sysemu/dump.h | 28 ++++
> include/sysemu/memory_mapping.h | 8 ++
> memory_mapping.c | 46 ++++++-
> qapi-schema.json | 29 +++-
> qapi/event.json | 10 ++
> qmp-commands.hx | 35 ++++-
> qmp.c | 14 ++
> 13 files changed, 450 insertions(+), 63 deletions(-)
>
> --
> 2.4.3
>
>
^ permalink raw reply [flat|nested] 43+ messages in thread