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<address@hidden>
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?