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

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

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

-- 
Peter Xu