On Thu, Jun 17, 2021 at 10:12:04PM +0800, huang...@chinatelecom.cn wrote:
> From: Hyman Huang(黄勇) <huang...@chinatelecom.cn>
>
> dirty rate measurement may start or stop dirty tracking during
> calculation. this conflict with migration because stop dirty
> tracking make migration leave dirty pages out then that'll be
> a problem.
>
> make global_dirty_tracking a bitmask can let both migration and
> dirty rate measurement work fine. introduce GLOBAL_DIRTY_MIGRATION
> and GLOBAL_DIRTY_DIRTY_RATE to distinguish what current dirty
> tracking aims for, migration or dirty rate.
>
> Signed-off-by: Hyman Huang(黄勇) <huang...@chinatelecom.cn>
> ---
> include/exec/memory.h | 18 +++++++++++++++---
> migration/ram.c | 8 ++++----
> softmmu/memory.c | 32 +++++++++++++++++++++++---------
> softmmu/trace-events | 1 +
> 4 files changed, 43 insertions(+), 16 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index cc0e549..63694dc 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -55,7 +55,15 @@ static inline void fuzz_dma_read_cb(size_t addr,
> }
> #endif
>
> -extern bool global_dirty_tracking;
> +/* 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)
> +
> +extern unsigned int global_dirty_tracking;
>
> typedef struct MemoryRegionOps MemoryRegionOps;
>
> @@ -2099,13 +2107,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 5682053..432cec8 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_tracking;
> +unsigned int global_dirty_tracking;
>
> 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_tracking = true;
> +#define GLOBAL_DIRTY_MASK (0x3)
I should raised this earlier... but I think better move this macro to the
defines of the bits.
> + assert(flags && !(flags & (~GLOBAL_DIRTY_MASK)));
> + assert(!(global_dirty_tracking & flags));
> + global_dirty_tracking |= flags;
> +
> + trace_global_dirty_changed(global_dirty_tracking);
>
> MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward);
>
> @@ -2676,9 +2681,13 @@ 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_tracking = false;
> + assert(flags && !(flags & (~GLOBAL_DIRTY_MASK)));
> + assert((global_dirty_tracking & flags) == flags);
> + global_dirty_tracking &= ~flags;
> +
> + trace_global_dirty_changed(global_dirty_tracking);
>
> /* Refresh DIRTY_MEMORY_MIGRATION bit. */
> memory_region_transaction_begin();
> @@ -2691,8 +2700,10 @@ static void memory_global_dirty_log_do_stop(void)
> static void memory_vm_change_state_handler(void *opaque, bool running,
> RunState state)
> {
> + unsigned 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 +2712,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)
> {
> + unsigned int *opaque = NULL;
> if (!runstate_is_running()) {
> if (vmstate_change) {
> return;
> }
> + opaque = g_malloc0(sizeof(opaque));
> + *opaque = flags;
Here the flags can be directly casted into a "void *" so we could avoid
malloc/free. You call..
I also still think it's easier to squash previous renaming patch into this one.
Thanks,
> 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);
> }
--
Peter Xu
