From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:39353) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QnowT-0000Ht-1L for qemu-devel@nongnu.org; Mon, 01 Aug 2011 05:37:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QnowS-0002WV-3S for qemu-devel@nongnu.org; Mon, 01 Aug 2011 05:37:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53002) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QnowR-0002W6-S6 for qemu-devel@nongnu.org; Mon, 01 Aug 2011 05:37:44 -0400 Message-ID: <4E3673E3.1000201@redhat.com> Date: Mon, 01 Aug 2011 11:37:39 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH v2 1/3] separate thread for VM migration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Umesh Deshpande Cc: mtosatti@redhat.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, Juan Quintela 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