Message ID | 3848028c3029e2e870286c84c2b3e9a93967567e.1623804189.git.huangy81@chinatelecom.cn (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | support dirtyrate at the granualrity of vcpu | expand |
On Wed, Jun 16, 2021 at 09:12:28AM +0800, huangy81@chinatelecom.cn wrote: > diff --git a/include/exec/memory.h b/include/exec/memory.h > index b114f54..e31eef2 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -55,7 +55,11 @@ static inline void fuzz_dma_read_cb(size_t addr, > } > #endif > > -extern bool global_dirty_log; > +/* what is the purpose of current dirty log, migration or dirty rate ? */ Nitpick: I'll make it: /* Possible bits for global_dirty_log */ /* Dirty tracking enabled because migration is running */ #define GLOBAL_DIRTY_MIGRATION (1U << 0) /* Dirty tracking enabled because measuring dirty rate */ #define GLOBAL_DIRTY_DIRTY_RATE (1U << 1) > +#define GLOBAL_DIRTY_MIGRATION (1U << 0) > +#define GLOBAL_DIRTY_DIRTY_RATE (1U << 1) > + > +extern unsigned int global_dirty_log; > > typedef struct MemoryRegionOps MemoryRegionOps; > [...] > @@ -39,7 +39,7 @@ > static unsigned memory_region_transaction_depth; > static bool memory_region_update_pending; > static bool ioeventfd_update_pending; > -bool global_dirty_log; > +unsigned int global_dirty_log; I'm wondering whether it's a good chance to rename it to global_dirty_tracking, because "logging" has a hint on the method while it's not the only one now. > > static QTAILQ_HEAD(, MemoryListener) memory_listeners > = QTAILQ_HEAD_INITIALIZER(memory_listeners); > @@ -2659,14 +2659,19 @@ void memory_global_after_dirty_log_sync(void) > > static VMChangeStateEntry *vmstate_change; > > -void memory_global_dirty_log_start(void) > +void memory_global_dirty_log_start(unsigned int flags) > { > if (vmstate_change) { > qemu_del_vm_change_state_handler(vmstate_change); > vmstate_change = NULL; > } > > - global_dirty_log = true; > +#define GLOBAL_DIRTY_MASK (0x3) > + assert(flags && !(flags & (~GLOBAL_DIRTY_MASK))); > + assert(global_dirty_log ^ flags); Heh, this is probably my fault... I think what I wanted to suggest is actually: assert(!(global_dirty_log & flags)); Then for stop() below... > + global_dirty_log |= flags; > + > + trace_global_dirty_changed(global_dirty_log); > > MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward); > > @@ -2676,9 +2681,12 @@ void memory_global_dirty_log_start(void) > memory_region_transaction_commit(); > } > > -static void memory_global_dirty_log_do_stop(void) > +static void memory_global_dirty_log_do_stop(unsigned int flags) > { > - global_dirty_log = false; > + assert(flags && !(flags & (~GLOBAL_DIRTY_MASK))); ... it should probably be: assert((global_dirty_log & flags) == flags); Sorry about the confusion.
在 2021/6/16 23:22, Peter Xu 写道: > On Wed, Jun 16, 2021 at 09:12:28AM +0800, huangy81@chinatelecom.cn wrote: >> diff --git a/include/exec/memory.h b/include/exec/memory.h >> index b114f54..e31eef2 100644 >> --- a/include/exec/memory.h >> +++ b/include/exec/memory.h >> @@ -55,7 +55,11 @@ static inline void fuzz_dma_read_cb(size_t addr, >> } >> #endif >> >> -extern bool global_dirty_log; >> +/* what is the purpose of current dirty log, migration or dirty rate ? */ > > Nitpick: I'll make it: > > /* Possible bits for global_dirty_log */ > > /* Dirty tracking enabled because migration is running */ > #define GLOBAL_DIRTY_MIGRATION (1U << 0) > > /* Dirty tracking enabled because measuring dirty rate */ > #define GLOBAL_DIRTY_DIRTY_RATE (1U << 1) > >> +#define GLOBAL_DIRTY_MIGRATION (1U << 0) >> +#define GLOBAL_DIRTY_DIRTY_RATE (1U << 1) >> + >> +extern unsigned int global_dirty_log; >> >> typedef struct MemoryRegionOps MemoryRegionOps; >> > > [...] > >> @@ -39,7 +39,7 @@ >> static unsigned memory_region_transaction_depth; >> static bool memory_region_update_pending; >> static bool ioeventfd_update_pending; >> -bool global_dirty_log; >> +unsigned int global_dirty_log; > > I'm wondering whether it's a good chance to rename it to global_dirty_tracking, > because "logging" has a hint on the method while it's not the only one now. yeah, all references to global_dirty_log should be modified and can this be done in a single patch before this patchset? > >> >> static QTAILQ_HEAD(, MemoryListener) memory_listeners >> = QTAILQ_HEAD_INITIALIZER(memory_listeners); >> @@ -2659,14 +2659,19 @@ void memory_global_after_dirty_log_sync(void) >> >> static VMChangeStateEntry *vmstate_change; >> >> -void memory_global_dirty_log_start(void) >> +void memory_global_dirty_log_start(unsigned int flags) >> { >> if (vmstate_change) { >> qemu_del_vm_change_state_handler(vmstate_change); >> vmstate_change = NULL; >> } >> >> - global_dirty_log = true; >> +#define GLOBAL_DIRTY_MASK (0x3) >> + assert(flags && !(flags & (~GLOBAL_DIRTY_MASK))); >> + assert(global_dirty_log ^ flags); > > Heh, this is probably my fault... I think what I wanted to suggest is actually: > > assert(!(global_dirty_log & flags)); this is more graceful if concerning about only one of the reason can start dirty tracking at once. I'll pick up it in the next version. > > Then for stop() below... > >> + global_dirty_log |= flags; >> + >> + trace_global_dirty_changed(global_dirty_log); >> >> MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward); >> >> @@ -2676,9 +2681,12 @@ void memory_global_dirty_log_start(void) >> memory_region_transaction_commit(); >> } >> >> -static void memory_global_dirty_log_do_stop(void) >> +static void memory_global_dirty_log_do_stop(unsigned int flags) >> { >> - global_dirty_log = false; >> + assert(flags && !(flags & (~GLOBAL_DIRTY_MASK))); > > ... it should probably be: > > assert((global_dirty_log & flags) == flags); > > Sorry about the confusion. not at all, since i'm not figure out how this bitmask works clearly, thanks a lot for your guidance. >
diff --git a/include/exec/memory.h b/include/exec/memory.h index b114f54..e31eef2 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -55,7 +55,11 @@ static inline void fuzz_dma_read_cb(size_t addr, } #endif -extern bool global_dirty_log; +/* what is the purpose of current dirty log, migration or dirty rate ? */ +#define GLOBAL_DIRTY_MIGRATION (1U << 0) +#define GLOBAL_DIRTY_DIRTY_RATE (1U << 1) + +extern unsigned int global_dirty_log; typedef struct MemoryRegionOps MemoryRegionOps; @@ -2099,13 +2103,17 @@ void memory_listener_unregister(MemoryListener *listener); /** * memory_global_dirty_log_start: begin dirty logging for all regions + * + * @flags: purpose of starting dirty log, migration or dirty rate */ -void memory_global_dirty_log_start(void); +void memory_global_dirty_log_start(unsigned int flags); /** * memory_global_dirty_log_stop: end dirty logging for all regions + * + * @flags: purpose of stopping dirty log, migration or dirty rate */ -void memory_global_dirty_log_stop(void); +void memory_global_dirty_log_stop(unsigned int flags); void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled); diff --git a/migration/ram.c b/migration/ram.c index 60ea913..9ce31af 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -2190,7 +2190,7 @@ static void ram_save_cleanup(void *opaque) /* caller have hold iothread lock or is in a bh, so there is * no writing race against the migration bitmap */ - memory_global_dirty_log_stop(); + memory_global_dirty_log_stop(GLOBAL_DIRTY_MIGRATION); } RAMBLOCK_FOREACH_NOT_IGNORED(block) { @@ -2652,7 +2652,7 @@ static void ram_init_bitmaps(RAMState *rs) ram_list_init_bitmaps(); /* We don't use dirty log with background snapshots */ if (!migrate_background_snapshot()) { - memory_global_dirty_log_start(); + memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION); migration_bitmap_sync_precopy(rs); } } @@ -3393,7 +3393,7 @@ void colo_incoming_start_dirty_log(void) /* Discard this dirty bitmap record */ bitmap_zero(block->bmap, block->max_length >> TARGET_PAGE_BITS); } - memory_global_dirty_log_start(); + memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION); } ram_state->migration_dirty_pages = 0; qemu_mutex_unlock_ramlist(); @@ -3405,7 +3405,7 @@ void colo_release_ram_cache(void) { RAMBlock *block; - memory_global_dirty_log_stop(); + memory_global_dirty_log_stop(GLOBAL_DIRTY_MIGRATION); RAMBLOCK_FOREACH_NOT_IGNORED(block) { g_free(block->bmap); block->bmap = NULL; diff --git a/softmmu/memory.c b/softmmu/memory.c index c19b0be..d74172f 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -39,7 +39,7 @@ static unsigned memory_region_transaction_depth; static bool memory_region_update_pending; static bool ioeventfd_update_pending; -bool global_dirty_log; +unsigned int global_dirty_log; static QTAILQ_HEAD(, MemoryListener) memory_listeners = QTAILQ_HEAD_INITIALIZER(memory_listeners); @@ -2659,14 +2659,19 @@ void memory_global_after_dirty_log_sync(void) static VMChangeStateEntry *vmstate_change; -void memory_global_dirty_log_start(void) +void memory_global_dirty_log_start(unsigned int flags) { if (vmstate_change) { qemu_del_vm_change_state_handler(vmstate_change); vmstate_change = NULL; } - global_dirty_log = true; +#define GLOBAL_DIRTY_MASK (0x3) + assert(flags && !(flags & (~GLOBAL_DIRTY_MASK))); + assert(global_dirty_log ^ flags); + global_dirty_log |= flags; + + trace_global_dirty_changed(global_dirty_log); MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward); @@ -2676,9 +2681,12 @@ void memory_global_dirty_log_start(void) memory_region_transaction_commit(); } -static void memory_global_dirty_log_do_stop(void) +static void memory_global_dirty_log_do_stop(unsigned int flags) { - global_dirty_log = false; + assert(flags && !(flags & (~GLOBAL_DIRTY_MASK))); + global_dirty_log &= ~flags; + + trace_global_dirty_changed(global_dirty_log); /* Refresh DIRTY_MEMORY_MIGRATION bit. */ memory_region_transaction_begin(); @@ -2691,8 +2699,10 @@ static void memory_global_dirty_log_do_stop(void) static void memory_vm_change_state_handler(void *opaque, bool running, RunState state) { + int *flags = opaque; if (running) { - memory_global_dirty_log_do_stop(); + memory_global_dirty_log_do_stop(*flags); + g_free(opaque); if (vmstate_change) { qemu_del_vm_change_state_handler(vmstate_change); @@ -2701,18 +2711,21 @@ static void memory_vm_change_state_handler(void *opaque, bool running, } } -void memory_global_dirty_log_stop(void) +void memory_global_dirty_log_stop(unsigned int flags) { + int *opaque = NULL; if (!runstate_is_running()) { if (vmstate_change) { return; } + opaque = g_malloc0(sizeof(opaque)); + *opaque = flags; vmstate_change = qemu_add_vm_change_state_handler( - memory_vm_change_state_handler, NULL); + memory_vm_change_state_handler, opaque); return; } - memory_global_dirty_log_do_stop(); + memory_global_dirty_log_do_stop(flags); } static void listener_add_address_space(MemoryListener *listener, diff --git a/softmmu/trace-events b/softmmu/trace-events index 5262828..4431f7f 100644 --- a/softmmu/trace-events +++ b/softmmu/trace-events @@ -18,6 +18,7 @@ memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr, uint64_t flatview_new(void *view, void *root) "%p (root %p)" flatview_destroy(void *view, void *root) "%p (root %p)" flatview_destroy_rcu(void *view, void *root) "%p (root %p)" +global_dirty_changed(unsigned int bitmask) "bitmask 0x%"PRIx32 # vl.c vm_state_notify(int running, int reason, const char *reason_str) "running %d reason %d (%s)"