From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:54569) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qlfa3-0006yQ-PE for qemu-devel@nongnu.org; Tue, 26 Jul 2011 07:13:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qlfa2-00060x-Jr for qemu-devel@nongnu.org; Tue, 26 Jul 2011 07:13:43 -0400 Received: from mail-yx0-f173.google.com ([209.85.213.173]:39236) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qlfa2-00060o-Ev for qemu-devel@nongnu.org; Tue, 26 Jul 2011 07:13:42 -0400 Received: by yxt3 with SMTP id 3so205122yxt.4 for ; Tue, 26 Jul 2011 04:13:42 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <4E2EA161.2040606@redhat.com> Date: Tue, 26 Jul 2011 13:13:37 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <6fef8cdde0cfabfd664b00c27d39613ba4e75737.1311363171.git.udeshpan@redhat.com> In-Reply-To: <6fef8cdde0cfabfd664b00c27d39613ba4e75737.1311363171.git.udeshpan@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH 2/2] separate thread for VM migration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Umesh Deshapnde Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org On 07/22/2011 09:58 PM, Umesh Deshapnde wrote: > - qemu_mod_timer(s->timer, qemu_get_clock_ms(rt_clock) + 100); > + qemu_mod_timer(s->timer, qemu_get_clock_ms(migration_clock) + 100); > > if (s->freeze_output) > return; > @@ -246,8 +246,10 @@ static void buffered_rate_tick(void *opaque) > > buffered_flush(s); > > - /* Add some checks around this */ > s->put_ready(s->opaque); > + qemu_mutex_unlock_iothread(); > + usleep(qemu_timer_difference(s->timer, migration_clock) * 1000); > + qemu_mutex_lock_iothread(); > } Do you really need a timer? The timer will only run in the migration thread, where you should have full control on when to wait. The BufferedFile code is more general, but you can create one thread per BufferedFile (you will only have one). Then, since you only have one timer, calling buffered_rate_tick repeatedly is really all that is done in the body of the thread: while (s->state == MIG_STATE_ACTIVE) start_time = qemu_get_clock_ms(rt_clock); buffered_rate_tick (s->file); qemu_mutex_unlock_iothread(); usleep ((qemu_get_clock_ms(rt_clock) + 100 - start_time) * 1000); qemu_mutex_lock_iothread(); } Perhaps the whole QEMUFile abstraction should be abandoned as the basis of QEMUBufferedFile. It is simply too heavy when you're working in a separate thread and you can afford blocking operation. I also am not sure about the correctness. Here you removed the delayed call to migrate_fd_put_notify, which is what resets freeze_output to 0: > @@ -327,9 +320,7 @@ ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size) > if (ret == -1) > ret = -(s->get_error(s)); > > - if (ret == -EAGAIN) { > - qemu_set_fd_handler2(s->fd, NULL, NULL, migrate_fd_put_notify, s); > - } else if (ret< 0) { > + if (ret< 0&& ret != -EAGAIN) { > if (s->mon) { > monitor_resume(s->mon); > } The call to migrate_fd_put_notify is here: > @@ -396,6 +401,9 @@ void migrate_fd_put_ready(void *opaque) > } > s->state = state; > notifier_list_notify(&migration_state_notifiers); > + } else { > + migrate_fd_wait_for_unfreeze(s); > + qemu_file_put_notify(s->file); > } > } > But it is not clear to me what calls migrate_fd_put_ready when the output file is frozen. Finally, here you are busy waiting: > @@ -340,10 +331,34 @@ ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size) > return ret; > } > > -void migrate_fd_connect(FdMigrationState *s) > +void *migrate_run_timers(void *arg) > { > + FdMigrationState *s = arg; > int ret; > > + qemu_mutex_lock_iothread(); > + ret = qemu_savevm_state_begin(s->mon, s->file, s->mig_state.blk, > + s->mig_state.shared); > + if (ret< 0) { > + DPRINTF("failed, %d\n", ret); > + migrate_fd_error(s); > + return NULL; > + } > + > + migrate_fd_put_ready(s); > + > + while (s->state == MIG_STATE_ACTIVE) { > + qemu_run_timers(migration_clock); > + } which also convinces me that if you make rate limiting the main purpose of the migration thread's main loop, most problems will go away. You can also use select() instead of usleep, so that you fix the problem with qemu_file_put_notify. Paolo