Re: [Qemu-devel] [PATCH] migration: optimize the downtime

[Dr. David Alan Gilbert]

* Jay Zhou (address@hidden) wrote:
> Qemu_savevm_state_cleanup() takes about 300ms in my ram migration tests
> with a 8U24G vm(20G is really occupied), the main cost comes from
> KVM_SET_USER_MEMORY_REGION ioctl when mem.memory_size = 0 in
> kvm_set_user_memory_region(). In kmod, the main cost is
> kvm_zap_obsolete_pages(), which traverses the active_mmu_pages list to
> zap the unsync sptes.

Hi Jay,
  Is this actually increasing the real downtime when the guest isn't
running, or is it just the reported time? I see that the s->downtime
value is calculated right after where we currently call
qemu_savevm_state_cleanup.

  I guess the biggest problem is that 300ms happens before we restart
the guest on the source if a migration fails.

> I think it can be optimized:
> (1) source vm will be destroyed if the migration is successfully done,
>     so the resources will be cleanuped automatically by the system
> (2) delay the cleanup if the migration failed

I don't like putting it in qmp_cont; that shouldn't have migration magic
in it.
I guess we could put it in migrate_fd_cleanup perhaps? It gets called on
a bh near the end -  or could we just move it closer to the end of 
migration_thread?

However, we would need to be a bit careful of anything that needs
cleaning up before the source restarts on failure; I'm not sure of
the semantics of all the current things wired into save_cleanup.

Dave


> Signed-off-by: Jay Zhou <address@hidden>
> ---
>  migration/migration.c | 16 +++++++++-------
>  qmp.c                 | 10 ++++++++++
>  2 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index a0db40d..72832be 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1877,6 +1877,15 @@ static void *migration_thread(void *opaque)
>          if (qemu_file_get_error(s->to_dst_file)) {
>              migrate_set_state(&s->state, current_active_state,
>                                MIGRATION_STATUS_FAILED);
> +            /*
> +             * The resource has been allocated by migration will be reused in
> +             * COLO process, so don't release them.
> +             */
> +            if (!enable_colo) {
> +                qemu_mutex_lock_iothread();
> +                qemu_savevm_state_cleanup();
> +                qemu_mutex_unlock_iothread();
> +            }
>              trace_migration_thread_file_err();
>              break;
>          }
> @@ -1916,13 +1925,6 @@ static void *migration_thread(void *opaque)
>      end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>  
>      qemu_mutex_lock_iothread();
> -    /*
> -     * The resource has been allocated by migration will be reused in COLO
> -     * process, so don't release them.
> -     */
> -    if (!enable_colo) {
> -        qemu_savevm_state_cleanup();
> -    }
>      if (s->state == MIGRATION_STATUS_COMPLETED) {
>          uint64_t transferred_bytes = qemu_ftell(s->to_dst_file);
>          s->total_time = end_time - s->total_time;
> diff --git a/qmp.c b/qmp.c
> index b86201e..0e68eaa 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -37,6 +37,8 @@
>  #include "qom/object_interfaces.h"
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/acpi/acpi_dev_interface.h"
> +#include "migration/migration.h"
> +#include "migration/savevm.h"
>  
>  NameInfo *qmp_query_name(Error **errp)
>  {
> @@ -200,6 +202,14 @@ void qmp_cont(Error **errp)
>      if (runstate_check(RUN_STATE_INMIGRATE)) {
>          autostart = 1;
>      } else {
> +        /*
> +         * Delay the cleanup to reduce the downtime of migration.
> +         * The resource has been allocated by migration will be reused
> +         * in COLO process, so don't release them.
> +         */
> +        if (runstate_check(RUN_STATE_POSTMIGRATE) && 
> !migrate_colo_enabled()) {
> +            qemu_savevm_state_cleanup();
> +        }
>          vm_start();
>      }
>  }
> -- 
> 1.8.3.1
> 
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK