From mboxrd@z Thu Jan 1 00:00:00 1970 From: Umesh Deshpande Subject: Re: [RFC PATCH v3 1/4] separate thread for VM migration Date: Thu, 11 Aug 2011 13:36:02 -0400 Message-ID: <4E441302.5090102@redhat.com> References: <6ac256e1f481ea28678bae846a13714302f258db.1313076455.git.udeshpan@redhat.com> <4E4400EE.1030905@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, quintela@redhat.com, mtosatti@redhat.com To: Paolo Bonzini Return-path: Received: from mx1.redhat.com ([209.132.183.28]:10203 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751260Ab1HKRgE (ORCPT ); Thu, 11 Aug 2011 13:36:04 -0400 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p7BHa4C1020881 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 11 Aug 2011 13:36:04 -0400 In-Reply-To: <4E4400EE.1030905@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 08/11/2011 12:18 PM, Paolo Bonzini wrote: >> @@ -175,20 +170,20 @@ static int buffered_close(void *opaque) >> >> while (!s->has_error&& s->buffer_size) { >> buffered_flush(s); >> - if (s->freeze_output) >> + if (s->freeze_output) { >> s->wait_for_unfreeze(s); >> + } >> } > > This is racy; you might end up calling buffered_put_buffer twice from > two different threads. Now, migrate_fd_cleanup, buffured_close is just executed by the migration thread. I am not letting iothread call any migration cancellation related functions. In stead it just submits the request and waits for the migration thread to terminate itself in the next iteration. The reason is to avoid the call to qemu_fflush, qemu_savevm_state_cancel (to carry out migrate_cancel) from iothread while migration thread is transferring data without holding the locks. > >> - ret = s->close(s->opaque); >> + s->closed = 1; >> >> - qemu_del_timer(s->timer); >> - qemu_free_timer(s->timer); >> + ret = s->close(s->opaque); >> qemu_free(s->buffer); >> - qemu_free(s); > > ... similarly, here the migration thread might end up using the > buffer. Just set s->closed here and wait for thread completion; the > migration thread can handle the flushes free the buffer etc. Let the > migration thread do as much as possible, it will simplify your life. > >> return ret; >> } >> >> + >> static int buffered_rate_limit(void *opaque) >> { >> QEMUFileBuffered *s = opaque; >> @@ -228,34 +223,55 @@ static int64_t buffered_get_rate_limit(void >> *opaque) >> return s->xfer_limit; >> } >> >> -static void buffered_rate_tick(void *opaque) >> +static void *migrate_vm(void *opaque) >> { >> QEMUFileBuffered *s = opaque; >> + int64_t current_time, expire_time = qemu_get_clock_ms(rt_clock) >> + 100; >> + struct timeval tv = { .tv_sec = 0, .tv_usec = 100000}; >> >> - if (s->has_error) { >> - buffered_close(s); >> - return; >> - } >> + qemu_mutex_lock_iothread(); >> >> - qemu_mod_timer(s->timer, qemu_get_clock_ms(rt_clock) + 100); >> + while (!s->closed) { > > ... This can be in fact > > while (!s->closed || s->buffered_size) > > and that alone will subsume the loop in buffered_close, no? s->fd is closed in migrate_fd_cleanup (which calls buffered_close). So I flush the buffer in buffered_close before closing the descriptor, and then migration thread simply exits because s->closed is set. - Umesh