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

[Jay Zhou]

Hi Dave,

On 2017/7/21 17:49, Dr. David Alan Gilbert wrote:
> * 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.

It actually increased the real downtime, I used the "ping" command to
test. Reason is that the source side libvirt sends qmp to qemu to query
the status of migration, which needs the BQL. qemu_savevm_state_cleanup
is done with BQL, qemu can not handle the qmp if qemu_savevm_state_cleanup
has not finished. And the source side libvirt delays about 300ms to notify
the destination side libvirt to send the "cont" command to start the vm.

I think the value of s->downtime is not accurate enough, maybe we could
move the calculation of end_time after qemu_savevm_state_cleanup has done.

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

300ms happens even if a migration succeeds.

> > 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.

Yes, it is not a ideal place. :(

> 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?

I have tested putting it in migrate_fd_cleanup, but the downtime is not
optimized. So I think it is the same to move it closer to the end of
migration_thread if it holds the BQL.
Could we put it in migrate_init?

Thanks,
Jay

> 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
>
> .