Re: [PATCH v4 2/6] memory: make global_dirty_log a bitmask

[Hyman Huang]

在 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.
>

--
Best regard

Hyman Huang(黄勇)