Re: [Qemu-devel] [PATCH] Add warmup phase for live migration of large memory apps

[Juan Quintela]

"Shribman, Aidan" <address@hidden> wrote:
> From: Aidan Shribman <address@hidden>
>
> [PATCH] Add warmup phase for live migration of large memory apps
>
> By invoking "migrate -w <url>" we initiate a background live-migration
> transferring of dirty pages continuously until invocation of "migrate_end"
> which attempts to complete the live migration operation.
>
> Qemu host: Ubuntu 10.10

Codding style comments already commented by Stefan.  Please use checkpatch.



> +static int is_migrate_warmup = 0;

once here, this can be "bool" O:-)
And this should be part of FdMigrationState, not a global variable.

> +>      DPRINTF("iterate\n");
> -    if (qemu_savevm_state_iterate(s->mon, s->file) == 1) {
> +    if (is_migrate_warmup) {
> +            qemu_savevm_state_warmup(s->mon, s->file);
> +     } else if (qemu_savevm_state_iterate(s->mon, s->file) == 1) {

Refactor this out?  qemu_savevm_state_warmup is just calling
qemu_savevm_state_iterate() and you are just ignoring error return (what
is wrong).

Why not just do something like.

    ret = qemu_savevm_state_iterate(s->mon, s->file);
    if (ret < 0) {
        s->state = state;
        notifier_list_notify(&migration_state_notifiers);
        return;
    }

    if (s->is_migrate_warmup) {
       return;
    }
    if (ret == 1) {
        int state;
        int old_vm_running = vm_running;

        /* and rest of old normal code */
    }

> +void do_migrate_end(Monitor *mon, const QDict *qdict)
> +{
> +    if (!vm_running) {
> +        return;
> +     }
> +    if (!is_migrate_warmup) {
> +        return;
> +     }
> +    is_migrate_warmup = 0;
> +}

If we add this, we should generalize it to always work.  i.e. just
convert the current migration in non-live, or something like that.

> diff --git a/savevm.c b/savevm.c
> index 4e49765..521589c 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1471,6 +1471,18 @@ int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, 
> int blk_enable,
>      return 0;
>  }
>  
> +int qemu_savevm_state_warmup(Monitor *mon, QEMUFile *f) {
> +    int ret = 1;
> +
> +    ret = qemu_savevm_state_iterate(mon, f);
> +
> +    if (ret == -EIO) {
> +        return ret;
> +     }
> +
> +    return 0;
> +}

You can see here why I don't like this function, it is just a wrapper to
qemu_savevm_state_iterate() that does nothing, just "detects" the return
value equal to 1.

As stated by Anthony, I think that we can "simulate" this functionality
playing with max_downtime value.  Why is that solution not enough for you?

Later, Juan.