Re: [PATCH v7 3/7] memory: make global_dirty_tracking a bitmask

From: Hyman Huang <huangy81@chinatelecom.cn>
To: Peter Xu <peterx@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>,
	Juan Quintela <quintela@redhat.com>,
	qemu-devel@nongnu.org,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
	Chuan Zheng <zhengchuan@huawei.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v7 3/7] memory: make global_dirty_tracking a bitmask
Date: Fri, 18 Jun 2021 08:36:55 +0800	[thread overview]
Message-ID: <167c1a3e-5c6f-6879-67b9-4b66e93c07ba@chinatelecom.cn> (raw)
In-Reply-To: <YMtqQgQ/Od4VoQs4@t490s>


在 2021/6/17 23:29, Peter Xu 写道:
> On Thu, Jun 17, 2021 at 10:12:04PM +0800, huangy81@chinatelecom.cn wrote:
>> From: Hyman Huang(黄勇) <huangy81@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(黄勇) <huangy81@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.
> 
OK
>> +    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.
> 
OK
> 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);
>>   }
> 

-- 
Best regard

Hyman Huang(黄勇)