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

[Hyman Huang]

在 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(黄勇)