Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.

[Fam Zheng]

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 <address@hidden>
> ---
>  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
> 
>