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@xxxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
