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

Paolo Bonzini
2011-07-26T11:13:42Z

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