From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [RFC PATCH v2 1/3] separate thread for VM migration Date: Mon, 01 Aug 2011 11:37:39 +0200 Message-ID: <4E3673E3.1000201@redhat.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, qemu-devel@nongnu.org, Juan Quintela , mtosatti@redhat.com To: Umesh Deshpande Return-path: Received: from mx1.redhat.com ([209.132.183.28]:14600 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753282Ab1HAJho (ORCPT ); Mon, 1 Aug 2011 05:37:44 -0400 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: 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 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