| Message ID | f4440591ca4843adff3a588864199ace81e5f11c.1311971938.git.udeshpan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/29/2011 10:57 PM, Umesh Deshpande wrote: > + qemu_mutex_unlock_iothread(); > > while (s->state == MIG_STATE_ACTIVE) { > if (migrate_fd_check_expire()) { > + qemu_mutex_lock_iothread(); > buffered_rate_tick(s->file); > + qemu_mutex_unlock_iothread(); > } > > if (s->state != MIG_STATE_ACTIVE) { > @@ -392,12 +396,11 @@ void migrate_fd_begin(void *arg) > > if (s->callback) { > migrate_fd_wait_for_unfreeze(s); > + qemu_mutex_lock_iothread(); > s->callback(s); > + qemu_mutex_unlock_iothread(); > } > } > - > -out: > - qemu_mutex_unlock_iothread(); I think it's clearer to unlock explicitly around the waiting points (see review of 1/3). In fact, I think you're working around the busy wait by accessing s->state outside the lock, right? I don't think this is provably safe; moving the knowledge of the thread entirely within buffered_file.c also fixes this, because then the lifetimes of the thread and the QEMUFile are much clearer. Thanks, Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 29, 2011 at 04:57:25PM -0400, Umesh Deshpande wrote: > In the migration thread, qemu_mutex is released during the most time consuming > part. i.e. during is_dup_page which identifies the uniform data pages and during > the put_buffer. qemu_mutex is also released while blocking on select to wait for > the descriptor to become ready for writes. > > Signed-off-by: Umesh Deshpande <udeshpan@redhat.com> > --- > arch_init.c | 14 +++++++++++--- > migration.c | 11 +++++++---- > 2 files changed, 18 insertions(+), 7 deletions(-) > > diff --git a/arch_init.c b/arch_init.c > index 484b39d..cd545bc 100644 > --- a/arch_init.c > +++ b/arch_init.c > @@ -110,7 +110,7 @@ static int is_dup_page(uint8_t *page, uint8_t ch) > static RAMBlock *last_block; > static ram_addr_t last_offset; > > -static int ram_save_block(QEMUFile *f) > +static int ram_save_block(QEMUFile *f, int stage) > { > RAMBlock *block = last_block; > ram_addr_t offset = last_offset; > @@ -131,6 +131,10 @@ static int ram_save_block(QEMUFile *f) > current_addr + TARGET_PAGE_SIZE, > MIGRATION_DIRTY_FLAG); > > + if (stage != 3) { > + qemu_mutex_unlock_iothread(); > + } > + > p = block->host + offset; > > if (is_dup_page(p, *p)) { > @@ -153,6 +157,10 @@ static int ram_save_block(QEMUFile *f) > bytes_sent = TARGET_PAGE_SIZE; > } > > + if (stage != 3) { > + qemu_mutex_lock_iothread(); > + } > + Batching multiple pages (instead of a single page per lock/unlock cycle) is probably worthwhile. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch_init.c b/arch_init.c index 484b39d..cd545bc 100644 --- a/arch_init.c +++ b/arch_init.c @@ -110,7 +110,7 @@ static int is_dup_page(uint8_t *page, uint8_t ch) static RAMBlock *last_block; static ram_addr_t last_offset; -static int ram_save_block(QEMUFile *f) +static int ram_save_block(QEMUFile *f, int stage) { RAMBlock *block = last_block; ram_addr_t offset = last_offset; @@ -131,6 +131,10 @@ static int ram_save_block(QEMUFile *f) current_addr + TARGET_PAGE_SIZE, MIGRATION_DIRTY_FLAG); + if (stage != 3) { + qemu_mutex_unlock_iothread(); + } + p = block->host + offset; if (is_dup_page(p, *p)) { @@ -153,6 +157,10 @@ static int ram_save_block(QEMUFile *f) bytes_sent = TARGET_PAGE_SIZE; } + if (stage != 3) { + qemu_mutex_lock_iothread(); + } + break; } @@ -301,7 +309,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque) while (!qemu_file_rate_limit(f)) { int bytes_sent; - bytes_sent = ram_save_block(f); + bytes_sent = ram_save_block(f, stage); bytes_transferred += bytes_sent; if (bytes_sent == 0) { /* no more blocks */ break; @@ -322,7 +330,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque) int bytes_sent; /* flush all remaining blocks regardless of rate limiting */ - while ((bytes_sent = ram_save_block(f)) != 0) { + while ((bytes_sent = ram_save_block(f, stage)) != 0) { bytes_transferred += bytes_sent; } cpu_physical_memory_set_dirty_tracking(0); diff --git a/migration.c b/migration.c index bf86067..992fef5 100644 --- a/migration.c +++ b/migration.c @@ -375,15 +375,19 @@ void migrate_fd_begin(void *arg) if (ret < 0) { DPRINTF("failed, %d\n", ret); migrate_fd_error(s); - goto out; + qemu_mutex_unlock_iothread(); + return; } expire_time = qemu_get_clock_ms(rt_clock) + 100; migrate_fd_put_ready(s); + qemu_mutex_unlock_iothread(); while (s->state == MIG_STATE_ACTIVE) { if (migrate_fd_check_expire()) { + qemu_mutex_lock_iothread(); buffered_rate_tick(s->file); + qemu_mutex_unlock_iothread(); } if (s->state != MIG_STATE_ACTIVE) { @@ -392,12 +396,11 @@ void migrate_fd_begin(void *arg) if (s->callback) { migrate_fd_wait_for_unfreeze(s); + qemu_mutex_lock_iothread(); s->callback(s); + qemu_mutex_unlock_iothread(); } } - -out: - qemu_mutex_unlock_iothread(); }
In the migration thread, qemu_mutex is released during the most time consuming part. i.e. during is_dup_page which identifies the uniform data pages and during the put_buffer. qemu_mutex is also released while blocking on select to wait for the descriptor to become ready for writes. Signed-off-by: Umesh Deshpande <udeshpan@redhat.com> --- arch_init.c | 14 +++++++++++--- migration.c | 11 +++++++---- 2 files changed, 18 insertions(+), 7 deletions(-)