From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45423) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtfRN-0003ZZ-QX for qemu-devel@nongnu.org; Tue, 03 Nov 2015 12:32:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZtfRI-0000HW-LU for qemu-devel@nongnu.org; Tue, 03 Nov 2015 12:32:13 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51852) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtfRI-0000HN-Bq for qemu-devel@nongnu.org; Tue, 03 Nov 2015 12:32:08 -0500 Date: Tue, 3 Nov 2015 17:32:01 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20151103173200.GA31958@work-vm> References: <1443515898-3594-1-git-send-email-dgilbert@redhat.com> <1443515898-3594-46-git-send-email-dgilbert@redhat.com> <87wpu78cco.fsf@neno.neno> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87wpu78cco.fsf@neno.neno> Subject: Re: [Qemu-devel] [PATCH v8 45/54] Host page!=target page: Cleanup bitmaps List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: aarcange@redhat.com, liang.z.li@intel.com, qemu-devel@nongnu.org, luis@cs.umu.se, bharata@linux.vnet.ibm.com, amit.shah@redhat.com, pbonzini@redhat.com * Juan Quintela (quintela@redhat.com) wrote: > "Dr. David Alan Gilbert (git)" wrote: > > From: "Dr. David Alan Gilbert" > > > > 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 > > + 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 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 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