Re: [RFC PATCH v2 1/3] separate thread for VM migration

Paolo Bonzini
2011-08-01T09:37:44Z

On 07/29/2011 10:57 PM, Umesh Deshpande wrote:
> This patch creates a separate thread for the guest migration on the source side.
>
> Signed-off-by: Umesh Deshpande<udeshpan@redhat.com>

Looks pretty good!

One thing that shows, is that the interface separation between
buffered_file.c is migration.c is pretty weird. Your patch makes it
somewhat worse, but it was like this before so it's not your fault. The
good thing is that if buffered_file.c uses threads, you can fix a large
part of this and get even simpler code:

1) there is really just one way to implement migrate_fd_put_notify, and
with your simplifications it does not belong anymore in migration.c.

2) s->callback is actually not NULL exactly if s->file->frozen_output is
true, you can remove it as well;

3) buffered_close is messy because it can be called from both the
iothread (monitor->migrate_fd_cancel->migrate_fd_cleanup->qemu_fclose)
or the migration thread (after qemu_savevm_state_complete). But
buffered_close is actually very similar to your thread function (it does
flush+wait_for_unfreeze, basically)! So buffered_close can be simply:

s->closed = 1;
ret = qemu_thread_join(s->thread); /* doesn't exist yet :) */
qemu_free(...);
return ret;

Another nit is that here:

> + if (migrate_fd_check_expire()) {
> + buffered_rate_tick(s->file);
> + }
> +
> + if (s->state != MIG_STATE_ACTIVE) {
> + break;
> + }
> +
> + if (s->callback) {
> + migrate_fd_wait_for_unfreeze(s);
> + s->callback(s);
> + }

you can still have a busy wait.

Putting it all together, you can move the thread function back to
buffered_file.c like:

while (!s->closed || (!s->has_error && s->buffer_size)) {
if (s->freeze_output) {
qemu_mutex_unlock_iothread();
s->wait_for_unfreeze(s);
qemu_mutex_lock_iothread();
/* This comes from qemu_file_put_notify (via
buffered_put_buffer---can be simplified a lot too?).
s->freeze_output = 0;
/* Test again for cancellation. */
continue;
}

int64_t current_time = qemu_get_clock_ms(rt_clock);
if (s->expire_time > current_time) {
struct timeval tv = { .tv_sec = 0, .tv_usec = ... };
qemu_mutex_unlock_iothread();
select (0, NULL, NULL, NULL, &tv);
qemu_mutex_lock_iothread();
s->expire_time = qemu_get_clock_ms(rt_clock) + 100;
continue;
}

/* This comes from buffered_rate_tick. */
s->bytes_xfer = 0;
buffered_flush(s);
if (!s->closed) {
s->put_ready(s->opaque);
}
}

ret = s->close(s->opaque);
...

Does it look sane?

Paolo