| Message ID | 1455609061-16279-4-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Feb 16, 2016 at 03:50:53PM +0800, Peter Xu wrote: > Instead of malloc/free each time for DumpState, make it > static. Added DumpStatus to show status for dump. I see that the motivation for making DumpState static is for dump_in_progress(). DumpState isn't massive, but it isn't tiny either, so maybe we should just have a global pointer instead? dump_in_progress() can report DUMP_STATUS_NONE when the pointer is NULL, or whatever the status is, when it's not. drew > > This is to be used for detached dump. > > Signed-off-by: Peter Xu <peterx@redhat.com> > Reviewed-by: Fam Zheng <famz@redhat.com> > --- > dump.c | 21 ++++++++++++++++----- > include/sysemu/dump.h | 2 ++ > qapi-schema.json | 18 ++++++++++++++++++ > 3 files changed, 36 insertions(+), 5 deletions(-) > > diff --git a/dump.c b/dump.c > index c4a62d9..434bc60 100644 > --- a/dump.c > +++ b/dump.c > @@ -1442,6 +1442,14 @@ static void get_max_mapnr(DumpState *s) > s->max_mapnr = dump_paddr_to_pfn(s, last_block->target_end); > } > > +static DumpState dump_state_global = { .status = DUMP_STATUS_NONE }; > + > +static void dump_state_prepare(DumpState *s) > +{ > + /* zero the struct, setting status to active */ > + *s = (DumpState) { .status = DUMP_STATUS_ACTIVE }; > +} > + > 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) > @@ -1676,24 +1684,27 @@ void qmp_dump_guest_memory(bool paging, const char *file, > return; > } > > - s = g_malloc0(sizeof(DumpState)); > + s = &dump_state_global; > + dump_state_prepare(s); > > dump_init(s, fd, has_format, format, paging, has_begin, > begin, length, &local_err); > if (local_err) { > - g_free(s); > error_propagate(errp, local_err); > + s->status = DUMP_STATUS_FAILED; > return; > } > > if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) { > - create_kdump_vmcore(s, errp); > + create_kdump_vmcore(s, &local_err); > } else { > - create_vmcore(s, errp); > + create_vmcore(s, &local_err); > } > > + s->status = (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED); > + error_propagate(errp, local_err); > + > dump_cleanup(s); > - g_free(s); > } > > DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp) > diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h > index 2f04b24..21fc02d 100644 > --- a/include/sysemu/dump.h > +++ b/include/sysemu/dump.h > @@ -38,6 +38,7 @@ > > #include "sysemu/dump-arch.h" > #include "sysemu/memory_mapping.h" > +#include "qapi-types.h" > > typedef struct QEMU_PACKED MakedumpfileHeader { > char signature[16]; /* = "makedumpfile" */ > @@ -176,6 +177,7 @@ 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 */ > + DumpStatus status; /* current dump status */ > } DumpState; > > uint16_t cpu_to_dump16(DumpState *s, uint16_t val); > diff --git a/qapi-schema.json b/qapi-schema.json > index caff580..ccd30c8 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -2219,6 +2219,24 @@ > '*format': 'DumpGuestMemoryFormat'} } > > ## > +# @DumpStatus > +# > +# Describe the status of a long-running background guest memory dump. > +# > +# @none: no dump-guest-memory has started yet. > +# > +# @active: there is one dump running in background. > +# > +# @completed: the last dump has finished successfully. > +# > +# @failed: the last dump has failed. > +# > +# Since 2.6 > +## > +{ 'enum': 'DumpStatus', > + 'data': [ 'none', 'active', 'completed', 'failed' ] } > + > +## > # @DumpGuestMemoryCapability: > # > # A list of the available formats for dump-guest-memory > -- > 2.4.3 > >
On Tue, Feb 16, 2016 at 01:32:00PM +0100, Andrew Jones wrote: > On Tue, Feb 16, 2016 at 03:50:53PM +0800, Peter Xu wrote: > > Instead of malloc/free each time for DumpState, make it > > static. Added DumpStatus to show status for dump. > > I see that the motivation for making DumpState static is for > dump_in_progress(). DumpState isn't massive, but it isn't tiny > either, so maybe we should just have a global pointer instead? > dump_in_progress() can report DUMP_STATUS_NONE when the pointer > is NULL, or whatever the status is, when it's not. I chose to use static dump state mostly for the simplicity of implementation. If I use a dynamic one, I can save some memory spaces when there is no dump task, however since it's async dump with dedicated thread, I need at least a lock to protect the global pointer when to use/malloc/free it, since there can be concurrent operations (e.g., when dump thread finished and trying to free the dump state, one user might be querying dump status too). So I just chose not to save the 200+ bytes. One more thing a static dump state could bring is that, instead of letting the user know "there is/is't a dump running", we can let him know something more, like: "no dump started" or "latest dump finished, with total written bytes XXX". Also, user can always check for all the information with the last time he/she did the dump (which I take it as a tiny small enhancement too). Thanks. Peter > > drew > > > > > This is to be used for detached dump. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > Reviewed-by: Fam Zheng <famz@redhat.com> > > --- > > dump.c | 21 ++++++++++++++++----- > > include/sysemu/dump.h | 2 ++ > > qapi-schema.json | 18 ++++++++++++++++++ > > 3 files changed, 36 insertions(+), 5 deletions(-) > > > > diff --git a/dump.c b/dump.c > > index c4a62d9..434bc60 100644 > > --- a/dump.c > > +++ b/dump.c > > @@ -1442,6 +1442,14 @@ static void get_max_mapnr(DumpState *s) > > s->max_mapnr = dump_paddr_to_pfn(s, last_block->target_end); > > } > > > > +static DumpState dump_state_global = { .status = DUMP_STATUS_NONE }; > > + > > +static void dump_state_prepare(DumpState *s) > > +{ > > + /* zero the struct, setting status to active */ > > + *s = (DumpState) { .status = DUMP_STATUS_ACTIVE }; > > +} > > + > > 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) > > @@ -1676,24 +1684,27 @@ void qmp_dump_guest_memory(bool paging, const char *file, > > return; > > } > > > > - s = g_malloc0(sizeof(DumpState)); > > + s = &dump_state_global; > > + dump_state_prepare(s); > > > > dump_init(s, fd, has_format, format, paging, has_begin, > > begin, length, &local_err); > > if (local_err) { > > - g_free(s); > > error_propagate(errp, local_err); > > + s->status = DUMP_STATUS_FAILED; > > return; > > } > > > > if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) { > > - create_kdump_vmcore(s, errp); > > + create_kdump_vmcore(s, &local_err); > > } else { > > - create_vmcore(s, errp); > > + create_vmcore(s, &local_err); > > } > > > > + s->status = (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED); > > + error_propagate(errp, local_err); > > + > > dump_cleanup(s); > > - g_free(s); > > } > > > > DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp) > > diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h > > index 2f04b24..21fc02d 100644 > > --- a/include/sysemu/dump.h > > +++ b/include/sysemu/dump.h > > @@ -38,6 +38,7 @@ > > > > #include "sysemu/dump-arch.h" > > #include "sysemu/memory_mapping.h" > > +#include "qapi-types.h" > > > > typedef struct QEMU_PACKED MakedumpfileHeader { > > char signature[16]; /* = "makedumpfile" */ > > @@ -176,6 +177,7 @@ 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 */ > > + DumpStatus status; /* current dump status */ > > } DumpState; > > > > uint16_t cpu_to_dump16(DumpState *s, uint16_t val); > > diff --git a/qapi-schema.json b/qapi-schema.json > > index caff580..ccd30c8 100644 > > --- a/qapi-schema.json > > +++ b/qapi-schema.json > > @@ -2219,6 +2219,24 @@ > > '*format': 'DumpGuestMemoryFormat'} } > > > > ## > > +# @DumpStatus > > +# > > +# Describe the status of a long-running background guest memory dump. > > +# > > +# @none: no dump-guest-memory has started yet. > > +# > > +# @active: there is one dump running in background. > > +# > > +# @completed: the last dump has finished successfully. > > +# > > +# @failed: the last dump has failed. > > +# > > +# Since 2.6 > > +## > > +{ 'enum': 'DumpStatus', > > + 'data': [ 'none', 'active', 'completed', 'failed' ] } > > + > > +## > > # @DumpGuestMemoryCapability: > > # > > # A list of the available formats for dump-guest-memory > > -- > > 2.4.3 > > > >
diff --git a/dump.c b/dump.c index c4a62d9..434bc60 100644 --- a/dump.c +++ b/dump.c @@ -1442,6 +1442,14 @@ static void get_max_mapnr(DumpState *s) s->max_mapnr = dump_paddr_to_pfn(s, last_block->target_end); } +static DumpState dump_state_global = { .status = DUMP_STATUS_NONE }; + +static void dump_state_prepare(DumpState *s) +{ + /* zero the struct, setting status to active */ + *s = (DumpState) { .status = DUMP_STATUS_ACTIVE }; +} + 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) @@ -1676,24 +1684,27 @@ void qmp_dump_guest_memory(bool paging, const char *file, return; } - s = g_malloc0(sizeof(DumpState)); + s = &dump_state_global; + dump_state_prepare(s); dump_init(s, fd, has_format, format, paging, has_begin, begin, length, &local_err); if (local_err) { - g_free(s); error_propagate(errp, local_err); + s->status = DUMP_STATUS_FAILED; return; } if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) { - create_kdump_vmcore(s, errp); + create_kdump_vmcore(s, &local_err); } else { - create_vmcore(s, errp); + create_vmcore(s, &local_err); } + s->status = (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED); + error_propagate(errp, local_err); + dump_cleanup(s); - g_free(s); } DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp) diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h index 2f04b24..21fc02d 100644 --- a/include/sysemu/dump.h +++ b/include/sysemu/dump.h @@ -38,6 +38,7 @@ #include "sysemu/dump-arch.h" #include "sysemu/memory_mapping.h" +#include "qapi-types.h" typedef struct QEMU_PACKED MakedumpfileHeader { char signature[16]; /* = "makedumpfile" */ @@ -176,6 +177,7 @@ 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 */ + DumpStatus status; /* current dump status */ } DumpState; uint16_t cpu_to_dump16(DumpState *s, uint16_t val); diff --git a/qapi-schema.json b/qapi-schema.json index caff580..ccd30c8 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2219,6 +2219,24 @@ '*format': 'DumpGuestMemoryFormat'} } ## +# @DumpStatus +# +# Describe the status of a long-running background guest memory dump. +# +# @none: no dump-guest-memory has started yet. +# +# @active: there is one dump running in background. +# +# @completed: the last dump has finished successfully. +# +# @failed: the last dump has failed. +# +# Since 2.6 +## +{ 'enum': 'DumpStatus', + 'data': [ 'none', 'active', 'completed', 'failed' ] } + +## # @DumpGuestMemoryCapability: # # A list of the available formats for dump-guest-memory