From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [RFC PATCH 2/2] separate thread for VM migration Date: Tue, 26 Jul 2011 13:13:37 +0200 Message-ID: <4E2EA161.2040606@redhat.com> References: <6fef8cdde0cfabfd664b00c27d39613ba4e75737.1311363171.git.udeshpan@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, qemu-devel@nongnu.org To: Umesh Deshapnde Return-path: Received: from mail-yx0-f174.google.com ([209.85.213.174]:60512 "EHLO mail-yx0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751211Ab1GZLNm (ORCPT ); Tue, 26 Jul 2011 07:13:42 -0400 Received: by yxi11 with SMTP id 11so152533yxi.19 for ; Tue, 26 Jul 2011 04:13:42 -0700 (PDT) In-Reply-To: <6fef8cdde0cfabfd664b00c27d39613ba4e75737.1311363171.git.udeshpan@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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