Re: [RFC PATCH 2/2] separate thread for VM migration

[Paolo Bonzini <pbonzini@redhat.com>]

On 07/22/2011 09:58 PM, Umesh Deshapnde wrote:
> -    qemu_mod_timer(s->timer, qemu_get_clock_ms(rt_clock) + 100);
> +    qemu_mod_timer(s->timer, qemu_get_clock_ms(migration_clock) + 100);
>
>       if (s->freeze_output)
>           return;
> @@ -246,8 +246,10 @@ static void buffered_rate_tick(void *opaque)
>
>       buffered_flush(s);
>
> -    /* Add some checks around this */
>       s->put_ready(s->opaque);
> +    qemu_mutex_unlock_iothread();
> +    usleep(qemu_timer_difference(s->timer, migration_clock) * 1000);
> +    qemu_mutex_lock_iothread();
>   }

Do you really need a timer?  The timer will only run in the migration 
thread, where you should have full control on when to wait.

The BufferedFile code is more general, but you can create one thread per 
BufferedFile (you will only have one).  Then, since you only have one 
timer, calling buffered_rate_tick repeatedly is really all that is done 
in the body of the thread:

while (s->state == MIG_STATE_ACTIVE)
     start_time = qemu_get_clock_ms(rt_clock);
     buffered_rate_tick (s->file);

     qemu_mutex_unlock_iothread();
     usleep ((qemu_get_clock_ms(rt_clock) + 100 - start_time) * 1000);
     qemu_mutex_lock_iothread();
}


Perhaps the whole QEMUFile abstraction should be abandoned as the basis 
of QEMUBufferedFile.  It is simply too heavy when you're working in a 
separate thread and you can afford blocking operation.

I also am not sure about the correctness.  Here you removed the delayed 
call to migrate_fd_put_notify, which is what resets freeze_output to 0:

> @@ -327,9 +320,7 @@ ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size)
>       if (ret == -1)
>           ret = -(s->get_error(s));
>
> -    if (ret == -EAGAIN) {
> -        qemu_set_fd_handler2(s->fd, NULL, NULL, migrate_fd_put_notify, s);
> -    } else if (ret<  0) {
> +    if (ret<  0&&  ret != -EAGAIN) {
>           if (s->mon) {
>               monitor_resume(s->mon);
>           }

The call to migrate_fd_put_notify is here:

> @@ -396,6 +401,9 @@ void migrate_fd_put_ready(void *opaque)
>          }
>          s->state = state;
>          notifier_list_notify(&migration_state_notifiers);
> +    } else {
> +        migrate_fd_wait_for_unfreeze(s);
> +        qemu_file_put_notify(s->file);
>      }
>  }
>

But it is not clear to me what calls migrate_fd_put_ready when the 
output file is frozen.

Finally, here you are busy waiting:

> @@ -340,10 +331,34 @@ ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size)
>       return ret;
>   }
>
> -void migrate_fd_connect(FdMigrationState *s)
> +void *migrate_run_timers(void *arg)
>   {
> +    FdMigrationState *s = arg;
>       int ret;
>
> +    qemu_mutex_lock_iothread();
> +    ret = qemu_savevm_state_begin(s->mon, s->file, s->mig_state.blk,
> +            s->mig_state.shared);
> +    if (ret<  0) {
> +        DPRINTF("failed, %d\n", ret);
> +        migrate_fd_error(s);
> +        return NULL;
> +    }
> +
> +    migrate_fd_put_ready(s);
> +
> +    while (s->state == MIG_STATE_ACTIVE) {
> +        qemu_run_timers(migration_clock);
> +    }

which also convinces me that if you make rate limiting the main purpose 
of the migration thread's main loop, most problems will go away.  You 
can also use select() instead of usleep, so that you fix the problem 
with qemu_file_put_notify.

Paolo