Separate thread for VM migration
v4 cover Separate thread for VM migration
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(-)
Signature · 1 line
1.7.4.1
RFC PATCH v4 1/5 MRU ram list
This patch creates a new list of RAM blocks in MRU order. So that separate locking rules can be applied to the regular RAM block list and the MRU list.
--- cpu-all.h | 2 ++ exec.c | 17 ++++++++++++----- 2 files changed, 14 insertions(+), 5 deletions(-)
cpu-all.h +2 -0
@@ -925,6 +925,7 @@ typedef struct RAMBlock {uint32_t flags;char idstr[256 ];QLIST_ENTRY (RAMBlock) next; +QLIST_ENTRY (RAMBlock) next_mru;#if defined(__linux__) && !defined(TARGET_S390X )int fd;#endif
@@ -933,6 +934,7 @@ typedef struct RAMBlock {typedef struct RAMList {uint8_t *phys_dirty; QLIST_HEAD(ram, RAMBlock) blocks; + QLIST_HEAD(, RAMBlock) blocks_mru; }RAMList ;extern RAMList ram_list;
exec.c +12 -5
@@ -113,7 +113,11 @@ static uint8_t *code_gen_ptr;int phys_ram_fd;static int in_migration; -RAMList ram_list = { .blocks =QLIST_HEAD_INITIALIZER (ram_list) }; +RAMList ram_list = { + .blocks =QLIST_HEAD_INITIALIZER (ram_list), + .blocks_mru =QLIST_HEAD_INITIALIZER (ram_list.blocks_mru) +}; +#endif CPUState *first_cpu;
@@ -2973,6 +2977,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name, new_block->length = size;QLIST_INSERT_HEAD (&ram_list.blocks, new_block, next); +QLIST_INSERT_HEAD (&ram_list.blocks_mru, new_block, next_mru); ram_list.phys_dirty =qemu_realloc (ram_list.phys_dirty,last_ram_offset () >>TARGET_PAGE_BITS );
@@ -2997,6 +3002,7 @@ void qemu_ram_free_from_ptr(ram_addr_t addr) QLIST_FOREACH(block, &ram_list.blocks, next) {if (addr == block->offset) {QLIST_REMOVE (block, next); +QLIST_REMOVE (block, next_mru);qemu_free (block);return ; }
@@ -3010,6 +3016,7 @@ void qemu_ram_free(ram_addr_t addr) QLIST_FOREACH(block, &ram_list.blocks, next) {if (addr == block->offset) {QLIST_REMOVE (block, next); +QLIST_REMOVE (block, next_mru);if (block->flags &RAM_PREALLOC_MASK ) { ; }else if (mem_path) {
@@ -3113,12 +3120,12 @@ void *qemu_get_ram_ptr(ram_addr_t addr) {RAMBlock *block; - QLIST_FOREACH(block, &ram_list.blocks, next) { + QLIST_FOREACH(block, &ram_list.blocks_mru, next_mru) {if (addr - block->offset < block->length) {/* Move this entry to to start of the list. */ if (block !=QLIST_FIRST (&ram_list.blocks)) { -QLIST_REMOVE (block, next); -QLIST_INSERT_HEAD (&ram_list.blocks, block, next); +QLIST_REMOVE (block, next_mru); +QLIST_INSERT_HEAD (&ram_list.blocks_mru, block, next_mru); }if (xen_mapcache_enabled ()) {/* We need to check if the requested address is in the RAM
@@ -3211,7 +3218,7 @@ int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)return 0 ; } - QLIST_FOREACH(block, &ram_list.blocks, next) { + QLIST_FOREACH(block, &ram_list.blocks_mru, next_mru) {/* This case append when the block is not mapped. */ if (block->host ==NULL ) {continue ;
Signature · 1 line
1.7.4.1
RFC PATCH v4 2/5 ramlist mutex
In reply to Umesh Deshpanderamlist mutex is implemented to protect the RAMBlock list traversal in the migration thread from their addition/removal from the iothread.
--- cpu-all.h | 2 ++ exec.c | 19 +++++++++++++++++++ qemu-common.h | 2 ++ 3 files changed, 23 insertions(+), 0 deletions(-)
cpu-all.h +2 -0
@@ -21,6 +21,7 @@#include "qemu-common.h" #include "cpu-common.h" +#include "qemu-thread.h" /* some important defines: *
@@ -932,6 +933,7 @@ typedef struct RAMBlock { } RAMBlock; typedef struct RAMList { + QemuMutex mutex; uint8_t *phys_dirty; QLIST_HEAD(ram, RAMBlock) blocks; QLIST_HEAD(, RAMBlock) blocks_mru;
exec.c +19 -0
@@ -582,6 +582,7 @@ void cpu_exec_init_all(unsigned long tb_size)code_gen_alloc (tb_size); code_gen_ptr = code_gen_buffer;page_init (); +qemu_mutex_init (&ram_list.mutex);#if !defined(CONFIG_USER_ONLY )io_mem_init ();#endif
@@ -2802,6 +2803,16 @@ static long gethugepagesize(const char *path)return fs.f_bsize; } +void qemu_mutex_lock_ramlist (void ) +{ +qemu_mutex_lock (&ram_list.mutex); +} + +void qemu_mutex_unlock_ramlist (void ) +{ +qemu_mutex_unlock (&ram_list.mutex); +} +static void *file_ram_alloc (RAMBlock *block,ram_addr_t memory,const char *path)
@@ -2976,6 +2987,8 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name, } new_block->length = size; +qemu_mutex_lock_ramlist (); +QLIST_INSERT_HEAD (&ram_list.blocks, new_block, next);QLIST_INSERT_HEAD (&ram_list.blocks_mru, new_block, next_mru);
@@ -2984,6 +2997,8 @@ 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 ); +qemu_mutex_unlock_ramlist (); +if (kvm_enabled ())kvm_setup_guest_memory (new_block->host, size);
@@ -3001,8 +3016,10 @@ void qemu_ram_free_from_ptr(ram_addr_t addr) QLIST_FOREACH(block, &ram_list.blocks, next) {if (addr == block->offset) { +qemu_mutex_lock_ramlist ();QLIST_REMOVE (block, next);QLIST_REMOVE (block, next_mru); +qemu_mutex_unlock_ramlist ();qemu_free (block);return ; }
@@ -3015,8 +3032,10 @@ void qemu_ram_free(ram_addr_t addr) QLIST_FOREACH(block, &ram_list.blocks, next) {if (addr == block->offset) { +qemu_mutex_lock_ramlist ();QLIST_REMOVE (block, next);QLIST_REMOVE (block, next_mru); +qemu_mutex_unlock_ramlist ();if (block->flags &RAM_PREALLOC_MASK ) { ; }else if (mem_path) {
qemu-common.h +2 -0
@@ -212,6 +212,8 @@ char *qemu_strndup(const char *str, size_t size);void qemu_mutex_lock_iothread (void );void qemu_mutex_unlock_iothread (void ); +void qemu_mutex_lock_ramlist (void ); +void qemu_mutex_unlock_ramlist (void );int qemu_open (const char *name,int flags, ...);ssize_t qemu_write_full (int fd,const void *buf,size_t count)
Signature · 1 line
1.7.4.1
Paolo Bonzini
maintainerRe: [RFC PATCH v4 2/5] ramlist mutex
Quoted 12 lines · Umesh Deshpande
exec.c +2
-0@@ -3001,8 +3016,10 @@ void qemu_ram_free_from_ptr(ram_addr_t addr) QLIST_FOREACH(block,&ram_list.blocks, next) {if (addr == block->offset) { +qemu_mutex_lock_ramlist ();QLIST_REMOVE (block, next);QLIST_REMOVE (block, next_mru); +qemu_mutex_unlock_ramlist ();qemu_free (block);return ; }
exec.c void qemu_ram_free(ram_addr_t addr) · line 3041
Quoted 10 lines
exec.c +2
-0@@ -3015,8 +3032,10 @@ void qemu_ram_free(ram_addr_t addr) QLIST_FOREACH(block,&ram_list.blocks, next) {if (addr == block->offset) { +qemu_mutex_lock_ramlist ();QLIST_REMOVE (block, next);QLIST_REMOVE (block, next_mru); +qemu_mutex_unlock_ramlist ();if (block->flags&RAM_PREALLOC_MASK ) { ; }else if (mem_path) {
You must protect the whole QLIST_FOREACH. Otherwise looks good.
Paolo
Re: [RFC PATCH v4 2/5] ramlist mutex
Quoted 14 lines · Paolo Bonzini
Quoted 13 lines · Umesh Deshpande
exec.c +2
-0@@ -3001,8 +3016,10 @@ void qemu_ram_free_from_ptr(ram_addr_t addr) QLIST_FOREACH(block,&ram_list.blocks, next) {if (addr == block->offset) { +qemu_mutex_lock_ramlist ();QLIST_REMOVE (block, next);QLIST_REMOVE (block, next_mru); +qemu_mutex_unlock_ramlist ();qemu_free (block);return ; }@@ -3015,8 +3032,10 @@ void qemu_ram_free(ram_addr_t addr)
Quoted 11 lines
exec.c void qemu_ram_free(ram_addr_t addr) · line 3041
Quoted 9 lines
exec.c +2
-0QLIST_FOREACH(block,&ram_list.blocks, next) {if (addr == block->offset) { +qemu_mutex_lock_ramlist ();QLIST_REMOVE (block, next);QLIST_REMOVE (block, next_mru); +qemu_mutex_unlock_ramlist ();if (block->flags&RAM_PREALLOC_MASK ) { ; }else if (mem_path) {You must protect the whole
QLIST_FOREACH. Otherwise looks good.
Or, is it okay to convert all the ramblock list traversals in exec.c (under iothread) to mru traversals, and probably it makes sense as the original list was also maintained in the mru order, whereas the sequence of blocks doesn't matter for the migration code. This way we don't have to acquire the mutex for block list traversals.
- Umesh
Paolo Bonzini
maintainerRe: [RFC PATCH v4 2/5] ramlist mutex
Quoted 5 lines · Umesh Deshpande
Or, is it okay to convert all the ramblock list traversals in
exec.c(under iothread) to mru traversals, and probably it makes sense as the original list was also maintained in the mru order, whereas the sequence of blocks doesn't matter for the migration code. This way we don't have to acquire the mutex for block list traversals.
I'm not sure... as I said, the MRU list is on a fast path and restricting it to that fast path keeps us honest. Also, the non-MRU list is almost never accessed outside the migration thread, so the mutex shouldn't be heavily contended anyway. You can also think about (not too clever) ways to keep the mutex unlocked while not doing ram_save_live.
BTW, actually the migration code tries to migrate the largest blocks first (because usually all the blocks after the first are small and easily migrated during the _complete pass), so the order does somewhat matter for migration.
Paolo
Re: [RFC PATCH v4 2/5] ramlist mutex
In reply to Umesh DeshpandeQuoted 23 lines · Umesh Deshpande
ramlist mutex is implemented to protect the RAMBlock list traversal in the migration thread from their addition/removal from the iothread.
Signed-off-by Umesh Deshpande <[email protected]>--- cpu-all.h | 2 ++ exec.c | 19 +++++++++++++++++++ qemu-common.h | 2 ++ 3 files changed, 23 insertions(+), 0 deletions(-)cpu-all.h +1
-0@@ -21,6 +21,7 @@#include "qemu-common.h" #include "cpu-common.h" +#include "qemu-thread.h" /* some important defines: *
cpu-all.h typedef struct RAMBlock { · line 939
Quoted 7 lines
cpu-all.h +1
-0@@ -932,6 +933,7 @@ typedef struct RAMBlock { } RAMBlock; typedef struct RAMList { + QemuMutex mutex; uint8_t *phys_dirty; QLIST_HEAD(ram, RAMBlock) blocks; QLIST_HEAD(, RAMBlock) blocks_mru;
A comment on what the mutex protects would be good.
Re: [RFC PATCH v4 2/5] ramlist mutex
Quoted 26 lines · Marcelo Tosatti
Quoted 25 lines · Umesh Deshpande
ramlist mutex is implemented to protect the RAMBlock list traversal in the migration thread from their addition/removal from the iothread.
Signed-off-by Umesh Deshpande <[email protected]>--- cpu-all.h | 2 ++ exec.c | 19 +++++++++++++++++++ qemu-common.h | 2 ++ 3 files changed, 23 insertions(+), 0 deletions(-)cpu-all.h +1
-0@@ -21,6 +21,7 @@#include "qemu-common.h" #include "cpu-common.h" +#include "qemu-thread.h" /* some important defines: * @@ -932,6 +933,7 @@ typedef struct RAMBlock { } RAMBlock;
Quoted 7 lines
cpu-all.h typedef struct RAMBlock { · line 939
Quoted 5 lines
cpu-all.h +1
-0typedef struct RAMList { + QemuMutex mutex; uint8_t *phys_dirty; QLIST_HEAD(ram, RAMBlock) blocks; QLIST_HEAD(, RAMBlock) blocks_mru;A comment on what the mutex protects would be good.
And on the lock ordering.
Paolo Bonzini
maintainerRe: [RFC PATCH v4 2/5] ramlist mutex
Quoted 7 lines · Marcelo Tosatti
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
typedef struct RAMList { + QemuMutex mutex; uint8_t *phys_dirty; QLIST_HEAD(ram, RAMBlock) blocks; QLIST_HEAD(, RAMBlock) blocks_mru;A comment on what the mutex protects would be good.
Indeed, especially because Umesh wanted to use the ramlist+iothread combo as a rw-lock: iothread = read-lock for the I/O thread, ramlist = read-lock for the migration thread, together = exclusive (write) lock. But I think I talked him out of this. :) It's not a bad idea in general, it just sounds like overkill in this case.
Quoted 1 line
And on the lock ordering.
I think when only two locks are involved, we can always assume iothread is outer and the other is inner. Do you agree?
Paolo
Re: [RFC PATCH v4 2/5] ramlist mutex
Quoted 15 lines · Paolo Bonzini
Quoted 7 lines · Marcelo Tosatti
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
typedef struct RAMList { + QemuMutex mutex; uint8_t *phys_dirty; QLIST_HEAD(ram, RAMBlock) blocks; QLIST_HEAD(, RAMBlock) blocks_mru;A comment on what the mutex protects would be good.
Indeed, especially because Umesh wanted to use the ramlist+iothread combo as a rw-lock: iothread = read-lock for the I/O thread, ramlist = read-lock for the migration thread, together = exclusive (write) lock. But I think I talked him out of this. :) It's not a bad idea in general, it just sounds like overkill in this case.
Quoted 6 lines
Quoted 1 line
And on the lock ordering.
I think when only two locks are involved, we can always assume iothread is outer and the other is inner. Do you agree?
Paolo
Yep.
RFC PATCH v4 3/5 separate migration bitmap
In reply to Umesh DeshpandeThis 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.
--- arch_init.c | 26 +++++++++++++++++------ cpu-all.h | 37 ++++++++++++++++++++++++++++++++++ exec.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 7 deletions(-)
arch_init.c +19 -7
@@ -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;
cpu-all.h +37 -0
@@ -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);
exec.c +64 -0
84 lines not on this page · raw message
Signature · 1 line
1.7.4.1
RFC PATCH v4 4/5 separate thread for VM migration
In reply to Umesh DeshpandeThis patch creates a separate thread for the guest migration on the source side. migrate_cancel request from the iothread is handled asynchronously. That is, iothread submits migrate_cancel to the migration thread and returns, while the migration thread attends this request at the next iteration to terminate its execution.
--- buffered_file.c | 93 ++++++++++++++++++++++++++++++++++-------------------- migration.c | 77 +++++++++++++++++++++++---------------------- migration.h | 1 + savevm.c | 5 --- 4 files changed, 99 insertions(+), 77 deletions(-)
buffered_file.c +58 -35
@@ -16,6 +16,8 @@#include "qemu-timer.h" #include "qemu-char.h" #include "buffered_file.h" +#include "migration.h" +#include "qemu-thread.h" //#define DEBUG_BUFFERED_FILE
@@ -28,13 +30,14 @@ typedef struct QEMUFileBufferedvoid *opaque;QEMUFile *file;int has_error; +int closed;int freeze_output;size_t bytes_xfer;size_t xfer_limit;uint8_t *buffer;size_t buffer_size;size_t buffer_capacity; -QEMUTimer *timer; +QemuThread thread; } QEMUFileBuffered;#ifdef DEBUG_BUFFERED_FILE
@@ -155,14 +158,6 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in offset = size; } -if (pos ==0 && size ==0 ) { -DPRINTF ("file is ready\n" ); -if (s->bytes_xfer <= s->xfer_limit) { -DPRINTF ("notifying client\n" ); - s->put_ready (s->opaque); - } - } -return offset; }
@@ -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); + } } - 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);return ret; } +static int buffered_rate_limit (void *opaque) {QEMUFileBuffered *s = opaque;
@@ -228,34 +223,63 @@ 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) { +if (s->freeze_output) { +qemu_mutex_lock_ramlist (); +qemu_mutex_unlock_iothread (); + s->wait_for_unfreeze (s); +qemu_mutex_lock_iothread (); +qemu_mutex_unlock_ramlist (); + s->freeze_output =0 ; +continue ; + } -if (s->freeze_output) -return ; +if (s->has_error) { +break ; + } + + current_time =qemu_get_clock_ms (rt_clock); +if (!s->closed && (expire_time > current_time)) { + tv.tv_usec =1000 * (expire_time - current_time); +qemu_mutex_lock_ramlist (); +qemu_mutex_unlock_iothread (); +select (0 ,NULL ,NULL ,NULL , &tv); +qemu_mutex_lock_iothread (); +qemu_mutex_unlock_ramlist (); +continue ; + } - s->bytes_xfer =0 ; + s->bytes_xfer =0 ; +buffered_flush (s); -buffered_flush (s); + expire_time =qemu_get_clock_ms (rt_clock) +100 ; + s->put_ready (s->opaque); + } -/* Add some checks around this */ - s->put_ready (s->opaque); +if (s->has_error) { +buffered_close (s); + } +qemu_free (s); + +qemu_mutex_unlock_iothread (); + +return NULL ; }QEMUFile *qemu_fopen_ops_buffered (void *opaque, -size_t bytes_per_sec, -BufferedPutFunc *put_buffer, -BufferedPutReadyFunc *put_ready, -BufferedWaitForUnfreezeFunc *wait_for_unfreeze, -BufferedCloseFunc *close) +size_t bytes_per_sec, +BufferedPutFunc *put_buffer, +BufferedPutReadyFunc *put_ready, +BufferedWaitForUnfreezeFunc *wait_for_unfreeze, +BufferedCloseFunc *close) {QEMUFileBuffered *s;
@@ -267,15 +291,14 @@ QEMUFile *qemu_fopen_ops_buffered(void *opaque, s->put_ready = put_ready; s->wait_for_unfreeze = wait_for_unfreeze; s->close = close; + s->closed =0 ; s->file =qemu_fopen_ops (s, buffered_put_buffer,NULL , buffered_close, buffered_rate_limit, buffered_set_rate_limit, - buffered_get_rate_limit); - - s->timer =qemu_new_timer_ms (rt_clock, buffered_rate_tick, s); + buffered_get_rate_limit); -qemu_mod_timer (s->timer,qemu_get_clock_ms (rt_clock) +100 ); +qemu_thread_create (&s->thread, migrate_vm, s);return s->file; }
migration.c +40 -37
148 lines not on this page · raw message
migration.h +1 -0
@@ -45,6 +45,7 @@ struct FdMigrationStateint fd;Monitor *mon;int state; +int begin;int (*get_error)(struct FdMigrationState *);int (*close)(struct FdMigrationState *);int (*write)(struct FdMigrationState *,const void *,size_t );
savevm.c +0 -5
@@ -481,11 +481,6 @@ int qemu_fclose(QEMUFile *f)return ret; } -void qemu_file_put_notify (QEMUFile *f) -{ - f->put_buffer (f->opaque,NULL ,0 ,0 ); -} -void qemu_put_buffer (QEMUFile *f,const uint8_t *buf,int size) {int l;
Signature · 1 line
1.7.4.1
Paolo Bonzini
maintainerRe: [RFC PATCH v4 4/5] separate thread for VM migration
Quoted 1 line · Umesh Deshpande
+
qemu_mutex_lock_ramlist();
Taken locks: iothread, ramlist
Quoted 1 line
+
qemu_mutex_unlock_iothread();
Taken locks: ramlist
Review
Quoted 2 lines
(quoted diff) +2
-0+ s->wait_for_unfreeze(s); + qemu_mutex_lock_iothread();
Taken locks: ramlist, iothread
You'd have a deadlock here if hypothetically you had two migrations at the same time.
Quoted 1 line
+
qemu_mutex_unlock_ramlist();
But in general, why this locking? The buffered file need not know anything about the ram list and its mutex. Only ram_save_live needs to hold the ramlist lock---starting just before sort_ram_list and ending just after the end of stage 3. That should be part of patch 2.
Actually buffered_file.c should ideally not even take the iothread lock!
The code there is only copying data from a private buffer to a file
descriptor; neither is shared. It's migrate_fd_put_buffer that should take care of locking. This is an example of why keeping the separation of QEMUBufferedFile is a good idea at least for now.
I am still kind of unconvinced about calling qemu_fclose from the migration thread. You still have one instance of cancellation in the iothread when migrate_fd_release is called. Ideally, as soon as migration finishes or has an error you could trigger a bottom half that closes the file (which in turn joins the thread). Migration state notifiers should also be run only from the iothread. Failure to do so (or in general lack of a policy of what runs where) can lead to very difficult bugs. Not so much hard to debug in this case (we have a global lock, so things cannot go _that_ bad), but hard to fix without redoing everything.
However, this patch is a good start (with locking fixed). It should takes several incremental steps before getting there, including incredible simplification if you take into account that migration can block and wait_for_unfreeze can disappear. In the end it probably should be committed as a single patch, but I'm liking the patches more and more.
Paolo
RFC PATCH v3 5/5 Making iothread block for migrate_cancel
In reply to Umesh DeshpandeFollowing patch makes iothread wait until the migration thread responds to the migrate_cancel request and terminates its execution.
--- buffered_file.c | 13 ++++++++++++- buffered_file.h | 3 +++ hw/hw.h | 5 ++++- migration.c | 1 + qemu-thread-posix.c | 10 ++++++++++ qemu-thread.h | 1 + savevm.c | 31 +++++++++++++++++++++---------- 7 files changed, 52 insertions(+), 12 deletions(-)
buffered_file.c +12 -1
@@ -223,6 +223,16 @@ static int64_t buffered_get_rate_limit(void *opaque)return s->xfer_limit; } +static void buffered_wait_for_cancel (void *opaque) +{ +QEMUFileBuffered *s = opaque; +QemuThread thread = s->thread; + +qemu_mutex_unlock_iothread (); +qemu_thread_join (thread); +qemu_mutex_lock_iothread (); +} +static void *migrate_vm (void *opaque) {QEMUFileBuffered *s = opaque;
@@ -296,7 +306,8 @@ QEMUFile *qemu_fopen_ops_buffered(void *opaque, s->file =qemu_fopen_ops (s, buffered_put_buffer,NULL , buffered_close, buffered_rate_limit, buffered_set_rate_limit, - buffered_get_rate_limit); + buffered_get_rate_limit, + buffered_wait_for_cancel);qemu_thread_create (&s->thread, migrate_vm, s);
buffered_file.h +3 -0
@@ -20,6 +20,9 @@ typedef ssize_t (BufferedPutFunc)(void *opaque, const void *data, size_t size);typedef void (BufferedPutReadyFunc )(void *opaque);typedef void (BufferedWaitForUnfreezeFunc )(void *opaque);typedef int (BufferedCloseFunc )(void *opaque); +typedef void (BufferedWaitForCancelFunc )(void *opaque); + +void wait_for_cancel (void *opaque); QEMUFile *qemu_fopen_ops_buffered(void *opaque, size_t xfer_limit, BufferedPutFunc *put_buffer,
hw/hw.h +4 -1
@@ -41,13 +41,15 @@ typedef int (QEMUFileRateLimit)(void *opaque);*/ typedef int64_t (QEMUFileSetRateLimit )(void *opaque,int64_t new_rate);typedef int64_t (QEMUFileGetRateLimit )(void *opaque); +typedef void (QEMUFileWaitForCancel )(void *opaque);QEMUFile *qemu_fopen_ops (void *opaque,QEMUFilePutBufferFunc *put_buffer,QEMUFileGetBufferFunc *get_buffer,QEMUFileCloseFunc *close,QEMUFileRateLimit *rate_limit,QEMUFileSetRateLimit *set_rate_limit, -QEMUFileGetRateLimit *get_rate_limit); +QEMUFileGetRateLimit *get_rate_limit, +QEMUFileWaitForCancel *wait_for_cancel);QEMUFile *qemu_fopen (const char *filename,const char *mode);QEMUFile *qemu_fdopen (int fd,const char *mode);QEMUFile *qemu_fopen_socket (int fd);
@@ -56,6 +58,7 @@ QEMUFile *qemu_popen_cmd(const char *command, const char *mode);int qemu_stdio_fd (QEMUFile *f);void qemu_fflush (QEMUFile *f);int qemu_fclose (QEMUFile *f); +void qemu_wait_for_cancel (QEMUFile *f);void qemu_put_buffer (QEMUFile *f,const uint8_t *buf,int size);void qemu_put_byte (QEMUFile *f,int v);
migration.c +1 -0
@@ -423,6 +423,7 @@ void migrate_fd_cancel(MigrationState *mig_state)DPRINTF ("cancelling migration\n" ); s->state =MIG_STATE_CANCELLED ; +qemu_wait_for_cancel (s->file); }void migrate_fd_release (MigrationState *mig_state)
qemu-thread-posix.c +10 -0
@@ -115,6 +115,16 @@ void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)error_exit (err, __func__); } +void qemu_thread_join (QemuThread thread) +{ +int err; + + err =pthread_join (thread.thread,NULL ); +if (err) { +error_exit (err, __func__); + } +} +void qemu_thread_create (QemuThread *thread,void *(*start_routine)(void *),void *arg)
qemu-thread.h +1 -0
@@ -30,6 +30,7 @@ void qemu_cond_destroy(QemuCond *cond);void qemu_cond_signal (QemuCond *cond);void qemu_cond_broadcast (QemuCond *cond);void qemu_cond_wait (QemuCond *cond,QemuMutex *mutex); +void qemu_thread_join (QemuThread thread); void qemu_thread_create(QemuThread *thread, void *(*start_routine)(void*),
savevm.c +21 -10
@@ -164,6 +164,7 @@ struct QEMUFile {QEMUFileRateLimit *rate_limit;QEMUFileSetRateLimit *set_rate_limit;QEMUFileGetRateLimit *get_rate_limit; +QEMUFileWaitForCancel *wait_for_cancel;void *opaque;int is_write;
@@ -261,10 +262,10 @@ QEMUFile *qemu_popen(FILE *stdio_file, const char *mode)if (mode[0 ] =='r' ) { s->file =qemu_fopen_ops (s,NULL , stdio_get_buffer, stdio_pclose, -NULL ,NULL ,NULL ); +NULL ,NULL ,NULL ,NULL ); }else { s->file =qemu_fopen_ops (s, stdio_put_buffer,NULL , stdio_pclose, -NULL ,NULL ,NULL ); +NULL ,NULL ,NULL ,NULL ); }return s->file; }
@@ -310,10 +311,10 @@ QEMUFile *qemu_fdopen(int fd, const char *mode)if (mode[0 ] =='r' ) { s->file =qemu_fopen_ops (s,NULL , stdio_get_buffer, stdio_fclose, -NULL ,NULL ,NULL ); +NULL ,NULL ,NULL ,NULL ); }else { s->file =qemu_fopen_ops (s, stdio_put_buffer,NULL , stdio_fclose, -NULL ,NULL ,NULL ); +NULL ,NULL ,NULL ,NULL ); }return s->file;
@@ -328,7 +329,7 @@ QEMUFile *qemu_fopen_socket(int fd) s->fd = fd; s->file =qemu_fopen_ops (s,NULL , socket_get_buffer, socket_close, -NULL ,NULL ,NULL ); +NULL ,NULL ,NULL ,NULL );return s->file; }
@@ -366,10 +367,10 @@ QEMUFile *qemu_fopen(const char *filename, const char *mode)if (mode[0 ] =='w' ) { s->file =qemu_fopen_ops (s, file_put_buffer,NULL , stdio_fclose, -NULL ,NULL ,NULL ); +NULL ,NULL ,NULL ,NULL ); }else { s->file =qemu_fopen_ops (s,NULL , file_get_buffer, stdio_fclose, -NULL ,NULL ,NULL ); +NULL ,NULL ,NULL ,NULL ); }return s->file; fail:
@@ -398,8 +399,9 @@ static QEMUFile *qemu_fopen_bdrv(BlockDriverState *bs, int is_writable) {if (is_writable)return qemu_fopen_ops (bs, block_put_buffer,NULL , bdrv_fclose, -NULL ,NULL ,NULL ); -return qemu_fopen_ops (bs,NULL , block_get_buffer, bdrv_fclose,NULL ,NULL ,NULL ); +NULL ,NULL ,NULL ,NULL ); +return qemu_fopen_ops (bs,NULL , block_get_buffer, bdrv_fclose,NULL ,NULL , +NULL ,NULL ); } QEMUFile *qemu_fopen_ops(void *opaque, QEMUFilePutBufferFunc *put_buffer,
@@ -407,7 +409,8 @@ QEMUFile *qemu_fopen_ops(void *opaque, QEMUFilePutBufferFunc *put_buffer, QEMUFileCloseFunc *close, QEMUFileRateLimit *rate_limit, QEMUFileSetRateLimit *set_rate_limit, - QEMUFileGetRateLimit *get_rate_limit) + QEMUFileGetRateLimit *get_rate_limit, + QEMUFileWaitForCancel *wait_for_cancel) {QEMUFile *f;
@@ -420,6 +423,7 @@ QEMUFile *qemu_fopen_ops(void *opaque, QEMUFilePutBufferFunc *put_buffer, f->rate_limit = rate_limit; f->set_rate_limit = set_rate_limit; f->get_rate_limit = get_rate_limit; + f->wait_for_cancel = wait_for_cancel; f->is_write =0 ;return f;
@@ -481,6 +485,13 @@ int qemu_fclose(QEMUFile *f)return ret; } +void qemu_wait_for_cancel (QEMUFile *f) +{ +if (f->wait_for_cancel) { + f->wait_for_cancel (f->opaque); + } +} +void qemu_put_buffer (QEMUFile *f,const uint8_t *buf,int size) {int l;
Signature · 1 line
1.7.4.1
Paolo Bonzini
maintainerRe: [RFC PATCH v3 5/5] Making iothread block for migrate_cancel
qemu-thread-posix.c void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex) · line 118
Quoted 9 lines · Umesh Deshpande
qemu-thread-posix.c +1
-0@@ -115,6 +115,16 @@ void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)error_exit (err, __func__); } +void qemu_thread_join (QemuThread thread)
Should take a pointer.
Paolo