From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:40594) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qnzbe-0002TU-F8 for qemu-devel@nongnu.org; Mon, 01 Aug 2011 17:00:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qnzbd-0002B1-FE for qemu-devel@nongnu.org; Mon, 01 Aug 2011 17:00:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:16907) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qnzbd-0002An-86 for qemu-devel@nongnu.org; Mon, 01 Aug 2011 17:00:57 -0400 Message-ID: <4E371406.7040109@redhat.com> Date: Mon, 01 Aug 2011 17:00:54 -0400 From: Umesh Deshpande MIME-Version: 1.0 References: <4E3673E3.1000201@redhat.com> In-Reply-To: <4E3673E3.1000201@redhat.com> 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: Paolo Bonzini Cc: mtosatti@redhat.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, Juan Quintela On 08/01/2011 05:37 AM, Paolo Bonzini wrote: > 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? > I kept this in migration.c to call qemu_savevm_state_begin. (The way it is done currently. i.e. to keep access to FdMigrationState in migration.c) Calling it from buffered_file.c would be inconsistent in that sense. or we will have to call it from the iothread before spawning the migration thread. Also why is the separation between FdMigrationState and QEMUFileBuffered is required. Is QEMUFileBuffered designed to use also for things other than migration? Thanks Umesh > > Paolo