* Re: [Qemu-devel] [RFC PATCH v4 3/5] separate migration bitmap @ 2011-08-19 12:51 Paolo Bonzini 2011-08-21 1:41 ` Umesh Deshpande 0 siblings, 1 reply; 4+ messages in thread From: Paolo Bonzini @ 2011-08-19 12:51 UTC (permalink / raw) To: Umesh Deshpande, qemu-devel On 08/16/2011 08:56 PM, Umesh Deshpande wrote: > @@ -2128,8 +2132,61 @@ void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t end, > start1, length); > } > } > + > } > > +void migration_bitmap_reset_dirty(ram_addr_t start, ram_addr_t end, > + int dirty_flags) > +{ > + unsigned long length, start1; > + > + start &= TARGET_PAGE_MASK; > + end = TARGET_PAGE_ALIGN(end); > + > + length = end - start; > + if (length == 0) { > + return; > + } > + > + migration_bitmap_mask_dirty_range(start, length, dirty_flags); > + > + /* we modify the TLB cache so that the dirty bit will be set again > + when accessing the range */ The comment does not apply here, and the code below can also be safely deleted. > + start1 = (unsigned long)qemu_safe_ram_ptr(start); > + /* Check that we don't span multiple blocks - this breaks the > + address comparisons below. */ > + if ((unsigned long)qemu_safe_ram_ptr(end - 1) - start1 > + != (end - 1) - start) { > + abort(); > + } > +} > + > +void sync_migration_bitmap(ram_addr_t start, ram_addr_t end) > +{ > + unsigned long length, len, i; > + ram_addr_t addr; > + start &= TARGET_PAGE_MASK; > + end = TARGET_PAGE_ALIGN(end); > + > + length = end - start; > + if (length == 0) { > + return; > + } > + > + len = length >> TARGET_PAGE_BITS; > + for (i = 0; i < len; i++) { > + addr = i << TARGET_PAGE_BITS; > + if (cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) { > + migration_bitmap_set_dirty(addr); > + cpu_physical_memory_reset_dirty(addr, addr + TARGET_PAGE_SIZE, > + MIGRATION_DIRTY_FLAG); This should be run under the iothread lock. Pay attention to avoiding lock inversion: the I/O thread always takes the iothread lock outside and the ramlist lock within, so the migration thread must do the same. BTW, I think this code in the migration thread patch also needs the iothread lock: > if (stage < 0) { > cpu_physical_memory_set_dirty_tracking(0); > return 0; > } > > if (cpu_physical_sync_dirty_bitmap(0, TARGET_PHYS_ADDR_MAX) != 0) { > qemu_file_set_error(f); > return 0; > } > Finally, here: > /* Make sure all dirty bits are set */ > QLIST_FOREACH(block, &ram_list.blocks, next) { > for (addr = block->offset; addr < block->offset + block->length; > addr += TARGET_PAGE_SIZE) { > if (!migration_bitmap_get_dirty(addr, > MIGRATION_DIRTY_FLAG)) { > migration_bitmap_set_dirty(addr); > } > } > } > ... you can skip the get_dirty operation since we are not interested in other flags than the migration flag for the migration-specific bitmap. Paolo ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v4 3/5] separate migration bitmap 2011-08-19 12:51 [Qemu-devel] [RFC PATCH v4 3/5] separate migration bitmap Paolo Bonzini @ 2011-08-21 1:41 ` Umesh Deshpande 2011-08-22 8:30 ` Paolo Bonzini 0 siblings, 1 reply; 4+ messages in thread From: Umesh Deshpande @ 2011-08-21 1:41 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-devel On 08/19/2011 08:51 AM, Paolo Bonzini wrote: > On 08/16/2011 08:56 PM, Umesh Deshpande wrote: >> @@ -2128,8 +2132,61 @@ void >> cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t end, >> start1, length); >> } >> } >> + >> } >> >> +void migration_bitmap_reset_dirty(ram_addr_t start, ram_addr_t end, >> + int dirty_flags) >> +{ >> + unsigned long length, start1; >> + >> + start &= TARGET_PAGE_MASK; >> + end = TARGET_PAGE_ALIGN(end); >> + >> + length = end - start; >> + if (length == 0) { >> + return; >> + } >> + >> + migration_bitmap_mask_dirty_range(start, length, dirty_flags); >> + >> + /* we modify the TLB cache so that the dirty bit will be set again >> + when accessing the range */ > > The comment does not apply here, and the code below can also be safely > deleted. > >> + start1 = (unsigned long)qemu_safe_ram_ptr(start); >> + /* Check that we don't span multiple blocks - this breaks the >> + address comparisons below. */ >> + if ((unsigned long)qemu_safe_ram_ptr(end - 1) - start1 >> + != (end - 1) - start) { >> + abort(); >> + } >> +} >> + >> +void sync_migration_bitmap(ram_addr_t start, ram_addr_t end) >> +{ >> + unsigned long length, len, i; >> + ram_addr_t addr; >> + start &= TARGET_PAGE_MASK; >> + end = TARGET_PAGE_ALIGN(end); >> + >> + length = end - start; >> + if (length == 0) { >> + return; >> + } >> + >> + len = length >> TARGET_PAGE_BITS; >> + for (i = 0; i < len; i++) { >> + addr = i << TARGET_PAGE_BITS; >> + if (cpu_physical_memory_get_dirty(addr, >> MIGRATION_DIRTY_FLAG)) { >> + migration_bitmap_set_dirty(addr); >> + cpu_physical_memory_reset_dirty(addr, addr + >> TARGET_PAGE_SIZE, >> + MIGRATION_DIRTY_FLAG); > > This should be run under the iothread lock. Pay attention to avoiding > lock inversion: the I/O thread always takes the iothread lock outside > and the ramlist lock within, so the migration thread must do the same. > > BTW, I think this code in the migration thread patch also needs the > iothread lock: > >> if (stage < 0) { >> cpu_physical_memory_set_dirty_tracking(0); >> return 0; >> } >> >> if (cpu_physical_sync_dirty_bitmap(0, TARGET_PHYS_ADDR_MAX) != 0) { >> qemu_file_set_error(f); >> return 0; >> } >> Callers of above code snippets (sync_migration_bitmap etc.) are holding the iothread mutex. It has been made sure that the original qemu dirty bitmap is only accessed when holding the mutex. > > Finally, here: > >> /* Make sure all dirty bits are set */ >> QLIST_FOREACH(block, &ram_list.blocks, next) { >> for (addr = block->offset; addr < block->offset + >> block->length; >> addr += TARGET_PAGE_SIZE) { >> if (!migration_bitmap_get_dirty(addr, >> >> MIGRATION_DIRTY_FLAG)) { >> migration_bitmap_set_dirty(addr); >> } >> } >> } >> > > ... you can skip the get_dirty operation since we are not interested > in other flags than the migration flag for the migration-specific bitmap. okay Thanks Umesh ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v4 3/5] separate migration bitmap 2011-08-21 1:41 ` Umesh Deshpande @ 2011-08-22 8:30 ` Paolo Bonzini 0 siblings, 0 replies; 4+ messages in thread From: Paolo Bonzini @ 2011-08-22 8:30 UTC (permalink / raw) To: Umesh Deshpande; +Cc: qemu-devel On 08/21/2011 03:41 AM, Umesh Deshpande wrote: >> >> This should be run under the iothread lock. Pay attention to avoiding >> lock inversion: the I/O thread always takes the iothread lock outside >> and the ramlist lock within, so the migration thread must do the same. >> >> BTW, I think this code in the migration thread patch also needs the >> iothread lock: >> >>> if (stage < 0) { >>> cpu_physical_memory_set_dirty_tracking(0); >>> return 0; >>> } >>> >>> if (cpu_physical_sync_dirty_bitmap(0, TARGET_PHYS_ADDR_MAX) != 0) { >>> qemu_file_set_error(f); >>> return 0; >>> } >>> > Callers of above code snippets (sync_migration_bitmap etc.) are holding > the iothread mutex. It has been made sure that the original qemu dirty > bitmap is only accessed when holding the mutex. But you cannot do it like in this patch, because here you have a deadlock: > + if (stage != 3) { > + qemu_mutex_lock_ramlist(); > + qemu_mutex_unlock_iothread(); > + } > + > while (!qemu_file_rate_limit(f)) { > int bytes_sent; > > @@ -308,6 +315,11 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque) > } > } > > + if (stage != 3) { > + qemu_mutex_lock_iothread(); Lock order: ramlist, iothread. The I/O thread instead takes the iothread lock outside and the ramlist lock inside. All this makes me even more convinced that you're locking is both too coarse and too complicated (perhaps it's not complicated, it's just under-documented; but the coarseness problem is there and it's what causes these lock inversions). > + qemu_mutex_unlock_ramlist(); > + } > + Paolo ^ permalink raw reply [flat|nested] 4+ messages in thread
* [Qemu-devel] [RFC PATCH v4 0/5] Separate thread for VM migration @ 2011-08-17 3:56 Umesh Deshpande 2011-08-17 3:56 ` [Qemu-devel] [RFC PATCH v4 3/5] separate migration bitmap Umesh Deshpande 0 siblings, 1 reply; 4+ messages in thread From: Umesh Deshpande @ 2011-08-17 3:56 UTC (permalink / raw) To: kvm, qemu-devel; +Cc: pbonzini, mtosatti, Umesh Deshpande, quintela Following patch series deals with VCPU and iothread starvation during the migration of a guest. Currently the iothread is responsible for performing the guest migration. It holds qemu_mutex during the migration and doesn't allow VCPU to enter the qemu mode and delays its return to the guest. The guest migration, executed as an iohandler also delays the execution of other iohandlers. In the following patch series, The migration has been moved to a separate thread to reduce the qemu_mutex contention and iohandler starvation. Umesh Deshpande (5): MRU ram list ramlist lock separate migration bitmap separate thread for VM migration synchronous migrate_cancel arch_init.c | 26 +++++++++--- buffered_file.c | 104 ++++++++++++++++++++++++++++++++++----------------- buffered_file.h | 3 + cpu-all.h | 41 ++++++++++++++++++++ exec.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++-- hw/hw.h | 5 ++- migration.c | 78 ++++++++++++++++++++------------------ migration.h | 1 + qemu-common.h | 2 + qemu-thread-posix.c | 10 +++++ qemu-thread.h | 1 + savevm.c | 30 +++++++++------ 12 files changed, 304 insertions(+), 97 deletions(-) -- 1.7.4.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [Qemu-devel] [RFC PATCH v4 3/5] separate migration bitmap 2011-08-17 3:56 [Qemu-devel] [RFC PATCH v4 0/5] Separate thread for VM migration Umesh Deshpande @ 2011-08-17 3:56 ` Umesh Deshpande 0 siblings, 0 replies; 4+ messages in thread From: Umesh Deshpande @ 2011-08-17 3:56 UTC (permalink / raw) To: kvm, qemu-devel; +Cc: pbonzini, mtosatti, Umesh Deshpande, quintela This patch creates a migration bitmap, which is periodically kept in sync with the qemu bitmap. A separate copy of the dirty bitmap for the migration avoids concurrent access to the qemu bitmap from iothread and migration thread. Signed-off-by: Umesh Deshpande <udeshpan@redhat.com> --- arch_init.c | 26 +++++++++++++++++------ cpu-all.h | 37 ++++++++++++++++++++++++++++++++++ exec.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 7 deletions(-) diff --git a/arch_init.c b/arch_init.c index 484b39d..296b7d6 100644 --- a/arch_init.c +++ b/arch_init.c @@ -123,13 +123,13 @@ static int ram_save_block(QEMUFile *f) current_addr = block->offset + offset; do { - if (cpu_physical_memory_get_dirty(current_addr, MIGRATION_DIRTY_FLAG)) { + if (migration_bitmap_get_dirty(current_addr, MIGRATION_DIRTY_FLAG)) { uint8_t *p; int cont = (block == last_block) ? RAM_SAVE_FLAG_CONTINUE : 0; - cpu_physical_memory_reset_dirty(current_addr, - current_addr + TARGET_PAGE_SIZE, - MIGRATION_DIRTY_FLAG); + migration_bitmap_reset_dirty(current_addr, + current_addr + TARGET_PAGE_SIZE, + MIGRATION_DIRTY_FLAG); p = block->host + offset; @@ -185,7 +185,7 @@ static ram_addr_t ram_save_remaining(void) ram_addr_t addr; for (addr = block->offset; addr < block->offset + block->length; addr += TARGET_PAGE_SIZE) { - if (cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) { + if (migration_bitmap_get_dirty(addr, MIGRATION_DIRTY_FLAG)) { count++; } } @@ -265,6 +265,8 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque) return 0; } + sync_migration_bitmap(0, TARGET_PHYS_ADDR_MAX); + if (stage == 1) { RAMBlock *block; bytes_transferred = 0; @@ -276,9 +278,9 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque) QLIST_FOREACH(block, &ram_list.blocks, next) { for (addr = block->offset; addr < block->offset + block->length; addr += TARGET_PAGE_SIZE) { - if (!cpu_physical_memory_get_dirty(addr, + if (!migration_bitmap_get_dirty(addr, MIGRATION_DIRTY_FLAG)) { - cpu_physical_memory_set_dirty(addr); + migration_bitmap_set_dirty(addr); } } } @@ -298,6 +300,11 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque) bytes_transferred_last = bytes_transferred; bwidth = qemu_get_clock_ns(rt_clock); + if (stage != 3) { + qemu_mutex_lock_ramlist(); + qemu_mutex_unlock_iothread(); + } + while (!qemu_file_rate_limit(f)) { int bytes_sent; @@ -308,6 +315,11 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque) } } + if (stage != 3) { + qemu_mutex_lock_iothread(); + qemu_mutex_unlock_ramlist(); + } + bwidth = qemu_get_clock_ns(rt_clock) - bwidth; bwidth = (bytes_transferred - bytes_transferred_last) / bwidth; diff --git a/cpu-all.h b/cpu-all.h index eab9803..e709277 100644 --- a/cpu-all.h +++ b/cpu-all.h @@ -935,6 +935,7 @@ typedef struct RAMBlock { typedef struct RAMList { QemuMutex mutex; uint8_t *phys_dirty; + uint8_t *migration_bitmap; QLIST_HEAD(ram, RAMBlock) blocks; QLIST_HEAD(, RAMBlock) blocks_mru; } RAMList; @@ -1008,8 +1009,44 @@ static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t start, } } + + void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t end, int dirty_flags); + +static inline int migration_bitmap_get_dirty(ram_addr_t addr, + int dirty_flags) +{ + return ram_list.migration_bitmap[addr >> TARGET_PAGE_BITS] & dirty_flags; +} + +static inline void migration_bitmap_set_dirty(ram_addr_t addr) +{ + ram_list.migration_bitmap[addr >> TARGET_PAGE_BITS] = 0xff; +} + +static inline void migration_bitmap_mask_dirty_range(ram_addr_t start, + int length, + int dirty_flags) +{ + int i, mask, len; + uint8_t *p; + + len = length >> TARGET_PAGE_BITS; + mask = ~dirty_flags; + p = ram_list.migration_bitmap + (start >> TARGET_PAGE_BITS); + for (i = 0; i < len; i++) { + p[i] &= mask; + } +} + + +void migration_bitmap_reset_dirty(ram_addr_t start, + ram_addr_t end, + int dirty_flags); + +void sync_migration_bitmap(ram_addr_t start, ram_addr_t end); + void cpu_tlb_update_dirty(CPUState *env); int cpu_physical_memory_set_dirty_tracking(int enable); diff --git a/exec.c b/exec.c index 404d8ea..36a44c7 100644 --- a/exec.c +++ b/exec.c @@ -2111,6 +2111,10 @@ void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t end, abort(); } + if (kvm_enabled()) { + return; + } + for(env = first_cpu; env != NULL; env = env->next_cpu) { int mmu_idx; for (mmu_idx = 0; mmu_idx < NB_MMU_MODES; mmu_idx++) { @@ -2119,8 +2123,61 @@ void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t end, start1, length); } } + +} + +void migration_bitmap_reset_dirty(ram_addr_t start, ram_addr_t end, + int dirty_flags) +{ + unsigned long length, start1; + + start &= TARGET_PAGE_MASK; + end = TARGET_PAGE_ALIGN(end); + + length = end - start; + if (length == 0) { + return; + } + + migration_bitmap_mask_dirty_range(start, length, dirty_flags); + + /* we modify the TLB cache so that the dirty bit will be set again + when accessing the range */ + start1 = (unsigned long)qemu_safe_ram_ptr(start); + /* Check that we don't span multiple blocks - this breaks the + address comparisons below. */ + if ((unsigned long)qemu_safe_ram_ptr(end - 1) - start1 + != (end - 1) - start) { + abort(); + } +} + +void sync_migration_bitmap(ram_addr_t start, ram_addr_t end) +{ + unsigned long length, len, i; + ram_addr_t addr; + start &= TARGET_PAGE_MASK; + end = TARGET_PAGE_ALIGN(end); + + length = end - start; + if (length == 0) { + return; + } + + len = length >> TARGET_PAGE_BITS; + for (i = 0; i < len; i++) { + addr = i << TARGET_PAGE_BITS; + if (cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) { + migration_bitmap_set_dirty(addr); + cpu_physical_memory_reset_dirty(addr, addr + TARGET_PAGE_SIZE, + MIGRATION_DIRTY_FLAG); + } + } + } + + int cpu_physical_memory_set_dirty_tracking(int enable) { int ret = 0; @@ -2997,6 +3054,13 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name, memset(ram_list.phys_dirty + (new_block->offset >> TARGET_PAGE_BITS), 0xff, size >> TARGET_PAGE_BITS); + ram_list.migration_bitmap = qemu_realloc(ram_list.phys_dirty, + last_ram_offset() >> + TARGET_PAGE_BITS); + + memset(ram_list.migration_bitmap + (new_block->offset >> TARGET_PAGE_BITS), + 0xff, size >> TARGET_PAGE_BITS); + qemu_mutex_unlock_ramlist(); if (kvm_enabled()) -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-08-22 8:31 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-08-19 12:51 [Qemu-devel] [RFC PATCH v4 3/5] separate migration bitmap Paolo Bonzini 2011-08-21 1:41 ` Umesh Deshpande 2011-08-22 8:30 ` Paolo Bonzini -- strict thread matches above, loose matches on Subject: below -- 2011-08-17 3:56 [Qemu-devel] [RFC PATCH v4 0/5] Separate thread for VM migration Umesh Deshpande 2011-08-17 3:56 ` [Qemu-devel] [RFC PATCH v4 3/5] separate migration bitmap Umesh Deshpande
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).