Re: [Qemu-devel] [PATCH v8 45/54] Host page!=target page: Cleanup bitmaps

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Prior to the start of postcopy, ensure that everything that will
> > be transferred later is a whole host-page in size.
> >
> > This is accomplished by discarding partially transferred host pages
> > and marking any that are partially dirty as fully dirty.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > +    struct RAMBlock *block;
> > +    unsigned int host_ratio = qemu_host_page_size / TARGET_PAGE_SIZE;
> > +
> > +    if (qemu_host_page_size == TARGET_PAGE_SIZE) {
> > +        /* Easy case - TPS==HPS - nothing to be done */
> > +        return 0;
> > +    }
> > +
> > +    /* Easiest way to make sure we don't resume in the middle of a host-page */
> > +    last_seen_block = NULL;
> > +    last_sent_block = NULL;
> > +    last_offset     = 0;
> 
> 
> It should be enough with the last one, right?  if you put
> last_seen/sent_block to NULL, you will return from the beggining each
> time that you do a migration bitmap sync, penalizing the pages on the
> begining of the cycle.  Even better than:
> 
> last_offset = 0 is doing a:
> 
> last_offset &= HOST_PAGE_MASK
> 
> or whatever is the constant, no?

These only happen once at the transition to postcopy; so I just
make sure by resetting the 3 associated variables; you're probably
right you could just do it by setting last_offset or last_seen_block;
but resetting all 3 gives you something that's obviously consistent.

> > +
> > +    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
> > +        unsigned long first = block->offset >> TARGET_PAGE_BITS;
> > +        unsigned long len = block->used_length >> TARGET_PAGE_BITS;
> > +        unsigned long last = first + (len - 1);
> > +        unsigned long found_set;
> > +        unsigned long search_start;
> 
> next_search?  search_next?

Based on your comments below, it's gone.

> > +
> > +        PostcopyDiscardState *pds =
> > +                         postcopy_discard_send_init(ms, first, block->idstr);
> > +
> > +        /* First pass: Discard all partially sent host pages */
> > +        found_set = find_next_bit(ms->sentmap, last + 1, first);
> > +        while (found_set <= last) {
> > +            bool do_discard = false;
> > +            unsigned long discard_start_addr;
> > +            /*
> > +             * If the start of this run of pages is in the middle of a host
> > +             * page, then we need to discard this host page.
> > +             */
> > +            if (found_set % host_ratio) {
> > +                do_discard = true;
> > +                found_set -= found_set % host_ratio;
> 
> please, create a PAGE_HOST_ALIGN() macro, or whatever you want to call it?

Note this is aligning by bit rather than page; so I think the
uses in here are the only case; however I've redone it as:
  host_offset = found_set % host_ratio;
  if (host_offset) {
      do_discard = true;
      found_set -= host_offset;
  }

so no repetition.

> > +                     * next 1 bit
> > +                     */
> > +                    search_start = found_zero + 1;
> 
> change for this
> 
> found_set = found_zero + 1;
> 
> > +                }
> > +            }
> > +            /* Find the next 1 for the next iteration */
> > +            found_set = find_next_bit(ms->sentmap, last + 1, search_start);
> 
> 
> and move previous line to:
> 
> > +            if (do_discard) {
> > +                unsigned long page;
> > +
> > +                /* Tell the destination to discard this page */
> > +                postcopy_discard_send_range(ms, pds, discard_start_addr,
> > +                         discard_start_addr + host_ratio - 1);
> > +                /* Clean up the bitmap */
> > +                for (page = discard_start_addr;
> > +                     page < discard_start_addr + host_ratio; page++) {
> > +                    /* All pages in this host page are now not sent */
> > +                    clear_bit(page, ms->sentmap);
> > +
> > +                    /*
> > +                     * Remark them as dirty, updating the count for any pages
> > +                     * that weren't previously dirty.
> > +                     */
> > +                    migration_dirty_pages += !test_and_set_bit(page,
> > +                                                             migration_bitmap);
> > +                }
> 
> 
> to here
>                    /* Find the next 1 for the next iteration */
>                    found_set = find_next_bit(ms->sentmap, last + 1, search_start);
>                }

OK, see the version below; I think I've done it as suggested; I've got rid of the
'search_start' and just reused the found_set (that's now run_start).

> > +        }
> 
> ?
> 
> 
> > +
> > +        /*
> > +         * Second pass: Ensure that all partially dirty host pages are made
> > +         * fully dirty.
> > +         */
> > +        found_set = find_next_bit(migration_bitmap, last + 1, first);
> > +        while (found_set <= last) {
> > +            bool do_dirty = false;
> > +            unsigned long dirty_start_addr;
> > +            /*
> > +             * If the start of this run of pages is in the middle of a host
> > +             * page, then we need to mark the whole of this host page dirty
> > +             */
> > +            if (found_set % host_ratio) {
> > +                do_dirty = true;
> > +                found_set -= found_set % host_ratio;
> > +                dirty_start_addr = found_set;
> > +                search_start = found_set + host_ratio;
> > +            } else {
> > +                /* Find the end of this run */
> > +                unsigned long found_zero;
> > +                found_zero = find_next_zero_bit(migration_bitmap, last + 1,
> > +                                                found_set + 1);
> > +                /*
> > +                 * If the 0 isn't at the start of a host page, then the
> > +                 * run of 1's doesn't finish at the end of a host page
> > +                 * and we need to discard.
> > +                 */
> > +                if (found_zero % host_ratio) {
> > +                    do_dirty = true;
> > +                    dirty_start_addr = found_zero - (found_zero % host_ratio);
> > +                    /*
> > +                     * This host page has gone, the next loop iteration starts
> > +                     * from the next page with a 1 bit
> > +                     */
> > +                    search_start = dirty_start_addr + host_ratio;
> > +                } else {
> > +                    /*
> > +                     * No discards on this iteration, next loop starts from
> > +                     * next 1 bit
> > +                     */
> > +                    search_start = found_zero + 1;
> > +                }
> > +            }
> > +
> > +            /* Find the next 1 for the next iteration */
> > +            found_set = find_next_bit(migration_bitmap, last + 1, search_start);
> > +
> > +            if (do_dirty) {
> > +                unsigned long page;
> > +
> > +                if (test_bit(dirty_start_addr, ms->sentmap)) {
> > +                    /*
> > +                     * If the page being redirtied is marked as sent, then it
> > +                     * must have been fully sent (otherwise it would have been
> > +                     * discarded by the previous pass.)
> > +                     * Discard it now.
> > +                     */
> > +                    postcopy_discard_send_range(ms, pds, dirty_start_addr,
> > +                                                dirty_start_addr +
> > +                                                host_ratio - 1);
> > +                }
> > +
> > +                /* Clean up the bitmap */
> > +                for (page = dirty_start_addr;
> > +                     page < dirty_start_addr + host_ratio; page++) {
> > +
> > +                    /* Clear the sentmap bits for the discard case above */
> > +                    clear_bit(page, ms->sentmap);
> > +
> > +                    /*
> > +                     * Mark them as dirty, updating the count for any pages
> > +                     * that weren't previously dirty.
> > +                     */
> > +                    migration_dirty_pages += !test_and_set_bit(page,
> > +                                                             migration_bitmap);
> > +                }
> > +            }
> > +        }
> 
> 
> This is exactly the same code than the previous half of the function,
> you just need to factor out in a function?
> 
> walk_btimap_host_page_chunks or whatever, and pass the two bits that
> change?  the bitmap, and what to do with the ranges that are not there?

Split out; see below - it gets a little bit more hairy since sentmap is
now unsentmap, so we need a few if's; but it's still lost the duplication:
(build tested only so far):

Dave

commit 15003123520ee5c358b2233c0bc30635aa90eb75
Author: Dr. David Alan Gilbert <dgilbert@redhat.com>
Date:   Fri Sep 26 15:15:14 2014 +0100

    Host page!=target page: Cleanup bitmaps
    
    Prior to the start of postcopy, ensure that everything that will
    be transferred later is a whole host-page in size.
    
    This is accomplished by discarding partially transferred host pages
    and marking any that are partially dirty as fully dirty.
    
    Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

diff --git a/migration/ram.c b/migration/ram.c
index fe782e7..e30ed2b 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1576,6 +1576,167 @@ static int postcopy_each_ram_send_discard(MigrationState *ms)
 }
 
 /*
+ * Helper for postcopy_chunk_hostpages; it's called twice to cleanup
+ *   the two bitmaps, that are similar, but one is inverted.
+ *
+ * We search for runs of target-pages that don't start or end on a
+ * host page boundary;
+ * unsent_pass=true: Cleans up partially unsent host pages by searching
+ *                 the unsentmap
+ * unsent_pass=false: Cleans up partially dirty host pages by searching
+ *                 the main migration bitmap
+ *
+ */
+static void postcopy_chunk_hostpages_pass(MigrationState *ms, bool unsent_pass,
+                                          RAMBlock *block,
+                                          PostcopyDiscardState *pds)
+{
+    unsigned long *bitmap = atomic_rcu_read(&migration_bitmap_rcu)->bmap;
+    unsigned int host_ratio = qemu_host_page_size / TARGET_PAGE_SIZE;
+    unsigned long first = block->offset >> TARGET_PAGE_BITS;
+    unsigned long len = block->used_length >> TARGET_PAGE_BITS;
+    unsigned long last = first + (len - 1);
+    unsigned long run_start;
+
+    if (unsent_pass) {
+        /* Find a sent page */
+        run_start = find_next_zero_bit(ms->unsentmap, last + 1, first);
+    } else {
+        /* Find a dirty page */
+        run_start = find_next_bit(bitmap, last + 1, first);
+    }
+
+    while (run_start <= last) {
+        bool do_fixup = false;
+        unsigned long fixup_start_addr;
+        unsigned long host_offset;
+
+        /*
+         * If the start of this run of pages is in the middle of a host
+         * page, then we need to fixup this host page.
+         */
+        host_offset = run_start % host_ratio;
+        if (host_offset) {
+            do_fixup = true;
+            run_start -= host_offset;
+            fixup_start_addr = run_start;
+            /* For the next pass */
+            run_start = run_start + host_ratio;
+        } else {
+            /* Find the end of this run */
+            unsigned long run_end;
+            if (unsent_pass) {
+                run_end = find_next_bit(ms->unsentmap, last + 1, run_start + 1);
+            } else {
+                run_end = find_next_zero_bit(bitmap, last + 1, run_start + 1);
+            }
+            /*
+             * If the end isn't at the start of a host page, then the
+             * run doesn't finish at the end of a host page
+             * and we need to discard.
+             */
+            host_offset = run_end % host_ratio;
+            if (host_offset) {
+                do_fixup = true;
+                fixup_start_addr = run_end - host_offset;
+                /*
+                 * This host page has gone, the next loop iteration starts
+                 * from after the fixup
+                 */
+                run_start = fixup_start_addr + host_ratio;
+            } else {
+                /*
+                 * No discards on this iteration, next loop starts from
+                 * next sent/dirty page
+                 */
+                run_start = run_end + 1;
+            }
+        }
+
+        if (do_fixup) {
+            unsigned long page;
+
+            /* Tell the destination to discard this page */
+            if (unsent_pass || !test_bit(fixup_start_addr, ms->unsentmap)) {
+                /* For the unsent_pass we:
+                 *     discard partially sent pages
+                 * For the !unsent_pass (dirty) we:
+                 *     discard partially dirty pages that were sent
+                 *     (any partially sent pages were already discarded
+                 *     by the previous unsent_pass)
+                 */
+                postcopy_discard_send_range(ms, pds, fixup_start_addr,
+                                            host_ratio);
+            }
+
+            /* Clean up the bitmap */
+            for (page = fixup_start_addr;
+                 page < fixup_start_addr + host_ratio; page++) {
+                /* All pages in this host page are now not sent */
+                set_bit(page, ms->unsentmap);
+
+                /*
+                 * Remark them as dirty, updating the count for any pages
+                 * that weren't previously dirty.
+                 */
+                migration_dirty_pages += !test_and_set_bit(page, bitmap);
+            }
+        }
+
+        if (unsent_pass) {
+            /* Find the next sent page for the next iteration */
+            run_start = find_next_zero_bit(ms->unsentmap, last + 1,
+                                           run_start);
+        } else {
+            /* Find the next dirty page for the next iteration */
+            run_start = find_next_bit(bitmap, last + 1, run_start);
+        }
+    }
+}
+
+/*
+ * Utility for the outgoing postcopy code.
+ *
+ * Discard any partially sent host-page size chunks, mark any partially
+ * dirty host-page size chunks as all dirty.
+ *
+ * Returns: 0 on success
+ */
+static int postcopy_chunk_hostpages(MigrationState *ms)
+{
+    struct RAMBlock *block;
+
+    if (qemu_host_page_size == TARGET_PAGE_SIZE) {
+        /* Easy case - TPS==HPS - nothing to be done */
+        return 0;
+    }
+
+    /* Easiest way to make sure we don't resume in the middle of a host-page */
+    last_seen_block = NULL;
+    last_sent_block = NULL;
+    last_offset     = 0;
+
+    QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
+        unsigned long first = block->offset >> TARGET_PAGE_BITS;
+
+        PostcopyDiscardState *pds =
+                         postcopy_discard_send_init(ms, first, block->idstr);
+
+        /* First pass: Discard all partially sent host pages */
+        postcopy_chunk_hostpages_pass(ms, true, block, pds);
+        /*
+         * Second pass: Ensure that all partially dirty host pages are made
+         * fully dirty.
+         */
+        postcopy_chunk_hostpages_pass(ms, false, block, pds);
+
+        postcopy_discard_send_finish(ms, pds);
+    } /* ram_list loop */
+
+    return 0;
+}
+
+/*
  * Transmit the set of pages to be discarded after precopy to the target
  * these are pages that:
  *     a) Have been previously transmitted but are now dirty again
@@ -1594,6 +1755,13 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms)
     /* This should be our last sync, the src is now paused */
     migration_bitmap_sync();
 
+    /* Deal with TPS != HPS */
+    ret = postcopy_chunk_hostpages(ms);
+    if (ret) {
+        rcu_read_unlock();
+        return ret;
+    }
+
     /*
      * Update the unsentmap to be unsentmap = unsentmap | dirty
      */
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK