RE: [PATCH] migration: Move bitmap_mutex out of migration_bitmap_clear_dirty()

[Wang, Wei W]

On Friday, July 2, 2021 3:07 PM, David Hildenbrand wrote:
> On 02.07.21 04:48, Wang, Wei W wrote:
> > On Thursday, July 1, 2021 10:22 PM, David Hildenbrand wrote:
> >> On 01.07.21 14:51, Peter Xu wrote:
> 
> I think that clearly shows the issue.
> 
> My theory I did not verify yet: Assume we have 1GB chunks in the clear bmap.
> Assume the VM reports all pages within a 1GB chunk as free (easy with a fresh
> VM). While processing hints, we will clear the bits from the dirty bmap in the
> RAMBlock. As we will never migrate any page of that 1GB chunk, we will not
> actually clear the dirty bitmap of the memory region. When re-syncing, we will
> set all bits bits in the dirty bmap again from the dirty bitmap in the memory
> region. Thus, many of our hints end up being mostly ignored. The smaller the
> clear bmap chunk, the more extreme the issue.

OK, that looks possible. We need to clear the related bits from the memory 
region
bitmap before skipping the free pages. Could you try with below patch:

diff --git a/migration/ram.c b/migration/ram.c
index ace8ad431c..a1f6df3e6c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -811,6 +811,26 @@ unsigned long migration_bitmap_find_dirty(RAMState *rs, 
RAMBlock *rb,
     return next;
 }

+
+static void migration_clear_memory_region_dirty_bitmap(RAMState *rs,
+                                                       RAMBlock *rb,
+                                                      unsigned long page)
+{
+    uint8_t shift;
+    hwaddr size, start;
+
+    if (!rb->clear_bmap || !clear_bmap_test_and_clear(rb, page))
+        return;
+
+    shift = rb->clear_bmap_shift;
+    assert(shift >= 6);
+
+    size = 1ULL << (TARGET_PAGE_BITS + shift);
+    start = (((ram_addr_t)page) << TARGET_PAGE_BITS) & (-size);
+    trace_migration_bitmap_clear_dirty(rb->idstr, start, size, page);
+    memory_region_clear_dirty_bitmap(rb->mr, start, size);
+}
+
 static inline bool migration_bitmap_clear_dirty(RAMState *rs,
                                                 RAMBlock *rb,
                                                 unsigned long page)
@@ -827,26 +847,9 @@ static inline bool migration_bitmap_clear_dirty(RAMState 
*rs,
      * the page in the chunk we clear the remote dirty bitmap for all.
      * Clearing it earlier won't be a problem, but too late will.
      */
-    if (rb->clear_bmap && clear_bmap_test_and_clear(rb, page)) {
-        uint8_t shift = rb->clear_bmap_shift;
-        hwaddr size = 1ULL << (TARGET_PAGE_BITS + shift);
-        hwaddr start = (((ram_addr_t)page) << TARGET_PAGE_BITS) & (-size);
-
-        /*
-         * CLEAR_BITMAP_SHIFT_MIN should always guarantee this... this
-         * can make things easier sometimes since then start address
-         * of the small chunk will always be 64 pages aligned so the
-         * bitmap will always be aligned to unsigned long.  We should
-         * even be able to remove this restriction but I'm simply
-         * keeping it.
-         */
-        assert(shift >= 6);
-        trace_migration_bitmap_clear_dirty(rb->idstr, start, size, page);
-        memory_region_clear_dirty_bitmap(rb->mr, start, size);
-    }
+    migration_clear_memory_region_dirty_bitmap(rs, rb, page);

     ret = test_and_clear_bit(page, rb->bmap);
-
     if (ret) {
         rs->migration_dirty_pages--;
     }
@@ -2746,7 +2749,7 @@ void qemu_guest_free_page_hint(void *addr, size_t len)
 {
     RAMBlock *block;
     ram_addr_t offset;
-    size_t used_len, start, npages;
+    size_t used_len, start, npages, page_to_clear, i = 0;
     MigrationState *s = migrate_get_current();

     /* This function is currently expected to be used during live migration */
@@ -2775,6 +2778,19 @@ void qemu_guest_free_page_hint(void *addr, size_t len)
         start = offset >> TARGET_PAGE_BITS;
         npages = used_len >> TARGET_PAGE_BITS;

+        /*
+         * The skipped free pages are equavelent to be sent from clear_bmap's
+        * perspective, so clear the bits from the memory region bitmap which
+        * are initially set. Otherwise those skipped pages will be sent in
+        * the next round after syncing from the memory region bitmap.
+        */
+        /*
+         * The skipped free pages are equavelent to be sent from clear_bmap's
+        * perspective, so clear the bits from the memory region bitmap which
+        * are initially set. Otherwise those skipped pages will be sent in
+        * the next round after syncing from the memory region bitmap.
+        */
+       do {
+            page_to_clear = start + (i++ << block->clear_bmap_shift);
+            migration_clear_memory_region_dirty_bitmap(ram_state,
+                                                       block,
+                                                       page_to_clear);
+       } while (i <= npages >> block->clear_bmap_shift);
+
         qemu_mutex_lock(&ram_state->bitmap_mutex);
         ram_state->migration_dirty_pages -=
                       bitmap_count_one_with_offset(block->bmap, start, npages);

Best,
Wei