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

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