| Message ID | 20210715075326.421977-1-wei.w.wang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] migration: clear the memory region dirty bitmap when skipping free pages | expand |
On 15.07.21 09:53, Wei Wang wrote: > When skipping free pages to send, their corresponding dirty bits in the > memory region dirty bitmap need to be cleared. Otherwise the skipped > pages will be sent in the next round after the migration thread syncs > dirty bits from the memory region dirty bitmap. > > Cc: David Hildenbrand <david@redhat.com> > Cc: Peter Xu <peterx@redhat.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Reported-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Wei Wang <wei.w.wang@intel.com> > --- > migration/ram.c | 72 ++++++++++++++++++++++++++++++++++++------------- > 1 file changed, 54 insertions(+), 18 deletions(-) > > v1->v2 changelog: > - move migration_clear_memory_region_dirty_bitmap under bitmap_mutex as > we lack confidence to have it outside the lock for now. > - clean the unnecessary subproject commit. > > diff --git a/migration/ram.c b/migration/ram.c > index b5fc454b2f..69e06b55ec 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -789,6 +789,51 @@ unsigned long migration_bitmap_find_dirty(RAMState *rs, RAMBlock *rb, > return find_next_bit(bitmap, size, start); > } > > +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; You could initialize this right at the beginning of the function without doing any harm. > + /* > + * 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); > + > + size = 1ULL << (TARGET_PAGE_BITS + shift); > + start = (((ram_addr_t)page) << TARGET_PAGE_BITS) & (-size); these as well as. > + trace_migration_bitmap_clear_dirty(rb->idstr, start, size, page); > + memory_region_clear_dirty_bitmap(rb->mr, start, size); > +} > + > +static void > +migration_clear_memory_region_dirty_bitmap_range(RAMState *rs, > + RAMBlock *rb, > + unsigned long start, > + unsigned long npages) > +{ > + unsigned long page_to_clear, i, nchunks; > + unsigned long chunk_pages = 1UL << rb->clear_bmap_shift; > + > + nchunks = (start + npages) / chunk_pages - start / chunk_pages + 1; Wouldn't you have to align the start and the end range up/down to properly calculate the number of chunks? The following might be better and a little easier to grasp: unsigned long chunk_pages = 1ULL << rb->clear_bmap_shift; unsigned long aligned_start = QEMU_ALIGN_DOWN(start, chunk_pages); unsigned long aligned_end = QEMU_ALIGN_UP(start + npages, chunk_pages) /* * Clear the clar_bmap of all covered chunks. It's sufficient to call it for * one page within a chunk. */ for (start = aligned_start, start != aligned_end, start += chunk_pages) { migration_clear_memory_region_dirty_bitmap(rs, rb, start); } > + > + for (i = 0; i < nchunks; i++) { > + page_to_clear = start + i * chunk_pages; > + migration_clear_memory_region_dirty_bitmap(rs, rb, page_to_clear); > + } > +} > + > static inline bool migration_bitmap_clear_dirty(RAMState *rs, > RAMBlock *rb, > unsigned long page) > @@ -803,26 +848,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); > - unrelated change but ok for me. > if (ret) { > rs->migration_dirty_pages--; > } > @@ -2741,6 +2769,14 @@ void qemu_guest_free_page_hint(void *addr, size_t len) > npages = used_len >> TARGET_PAGE_BITS; > > qemu_mutex_lock(&ram_state->bitmap_mutex); > + /* > + * The skipped free pages are equavelent to be sent from clear_bmap's s/equavelent/equivalent/ > + * 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. > + */ > + migration_clear_memory_region_dirty_bitmap_range(ram_state, block, > + start, npages); > ram_state->migration_dirty_pages -= > bitmap_count_one_with_offset(block->bmap, start, npages); > bitmap_clear(block->bmap, start, npages); > Apart from that, lgtm. (although I find the use of "start" to describe a PFN and not an address very confusing, but it's already in the current code ... start_pfn or just pfn as used in the kernel would be much clearer)
On Thursday, July 15, 2021 5:29 PM, David Hildenbrand wrote: > On 15.07.21 09:53, Wei Wang wrote: > > When skipping free pages to send, their corresponding dirty bits in > > the memory region dirty bitmap need to be cleared. Otherwise the > > skipped pages will be sent in the next round after the migration > > thread syncs dirty bits from the memory region dirty bitmap. > > > > Cc: David Hildenbrand <david@redhat.com> > > Cc: Peter Xu <peterx@redhat.com> > > Cc: Michael S. Tsirkin <mst@redhat.com> > > Reported-by: David Hildenbrand <david@redhat.com> > > Signed-off-by: Wei Wang <wei.w.wang@intel.com> > > --- > > migration/ram.c | 72 > ++++++++++++++++++++++++++++++++++++------------- > > 1 file changed, 54 insertions(+), 18 deletions(-) > > > > v1->v2 changelog: > > - move migration_clear_memory_region_dirty_bitmap under bitmap_mutex > as > > we lack confidence to have it outside the lock for now. > > - clean the unnecessary subproject commit. > > > > diff --git a/migration/ram.c b/migration/ram.c index > > b5fc454b2f..69e06b55ec 100644 > > --- a/migration/ram.c > > +++ b/migration/ram.c > > @@ -789,6 +789,51 @@ unsigned long > migration_bitmap_find_dirty(RAMState *rs, RAMBlock *rb, > > return find_next_bit(bitmap, size, start); > > } > > > > +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; > > You could initialize this right at the beginning of the function without doing any > harm. > > > + /* > > + * 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); > > + > > + size = 1ULL << (TARGET_PAGE_BITS + shift); > > + start = (((ram_addr_t)page) << TARGET_PAGE_BITS) & (-size); > > these as well as. Is there any coding style requirement for this? My thought was that those operations could mostly be avoided if they don't pass the above if condition (e.g. just once per 1GB chunk). > > > + trace_migration_bitmap_clear_dirty(rb->idstr, start, size, page); > > + memory_region_clear_dirty_bitmap(rb->mr, start, size); } > > + > > +static void > > +migration_clear_memory_region_dirty_bitmap_range(RAMState *rs, > > + RAMBlock *rb, > > + unsigned long > start, > > + unsigned long > > +npages) { > > + unsigned long page_to_clear, i, nchunks; > > + unsigned long chunk_pages = 1UL << rb->clear_bmap_shift; > > + > > + nchunks = (start + npages) / chunk_pages - start / chunk_pages + > > + 1; > > Wouldn't you have to align the start and the end range up/down to properly > calculate the number of chunks? No, divide will round it to the integer (beginning of the chunk to clear). > > The following might be better and a little easier to grasp: > > unsigned long chunk_pages = 1ULL << rb->clear_bmap_shift; unsigned long > aligned_start = QEMU_ALIGN_DOWN(start, chunk_pages); unsigned long > aligned_end = QEMU_ALIGN_UP(start + npages, chunk_pages) > > /* > * Clear the clar_bmap of all covered chunks. It's sufficient to call it for > * one page within a chunk. > */ > for (start = aligned_start, start != aligned_end, start += chunk_pages) { What if "aligned_end == start + npages"? i.e the above start + npages is aligned by itself without QEMU_ALIGN_UP(). For example, chunk size is 1GB, and start+npages=2GB, which is right at the beginning of [2GB,3GB) chunk. Then aligned_end is also 2GB, but we need to clear the [2GB, 3GB) chunk, right? Best, Wei
>>> + /* >>> + * 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); >>> + >>> + size = 1ULL << (TARGET_PAGE_BITS + shift); >>> + start = (((ram_addr_t)page) << TARGET_PAGE_BITS) & (-size); >> >> these as well as. > > Is there any coding style requirement for this? Don't think so. It simply results in less LOC and less occurrences of variables. > My thought was that those operations could mostly be avoided if they don't pass the > above if condition (e.g. just once per 1GB chunk). Usually the compiler will reshuffle as possible to optimize. But in this case, due to clear_bmap_test_and_clear(), it might not be able to move the computations behind that call. So the final code might actually differ. Not that we really care about this micro-optimization, though. > >> >>> + trace_migration_bitmap_clear_dirty(rb->idstr, start, size, page); >>> + memory_region_clear_dirty_bitmap(rb->mr, start, size); } >>> + >>> +static void >>> +migration_clear_memory_region_dirty_bitmap_range(RAMState *rs, >>> + RAMBlock *rb, >>> + unsigned long >> start, >>> + unsigned long >>> +npages) { >>> + unsigned long page_to_clear, i, nchunks; >>> + unsigned long chunk_pages = 1UL << rb->clear_bmap_shift; >>> + >>> + nchunks = (start + npages) / chunk_pages - start / chunk_pages + >>> + 1; >> >> Wouldn't you have to align the start and the end range up/down to properly >> calculate the number of chunks? > > No, divide will round it to the integer (beginning of the chunk to clear). nchunks = (start + npages) / chunk_pages - start / chunk_pages + 1; For simplicity: nchunks = (addr + size) / chunk_size - addr / chunk_size + 1; addr=1GB size=3GB chunk_size=2GB So for that range [1GB, 3GB), we'd have to clear [0GB,2GB), [2GB,4GB) Range: [ ] Chunks: [ - ][ - ][ - ][ - ] ... ^0 ^2 ^4 ^6 nchunks = (1 + 3) / 2 - 1 / 2 + 1 = 4 / 2 - 0 + 1 = 2 + 1 = 3 Which is wrong. While my variant will give you aligned_start = 0GB aligned_end = 4GB And consequently clear [0GB,2GB) and [2GB,4GB). Am I making a stupid mistake and should rather get another cup of coffee? :) > >> >> The following might be better and a little easier to grasp: >> >> unsigned long chunk_pages = 1ULL << rb->clear_bmap_shift; unsigned long >> aligned_start = QEMU_ALIGN_DOWN(start, chunk_pages); unsigned long >> aligned_end = QEMU_ALIGN_UP(start + npages, chunk_pages) >> >> /* >> * Clear the clar_bmap of all covered chunks. It's sufficient to call it for >> * one page within a chunk. >> */ >> for (start = aligned_start, start != aligned_end, start += chunk_pages) { > > What if "aligned_end == start + npages"? > i.e the above start + npages is aligned by itself without QEMU_ALIGN_UP(). > For example, chunk size is 1GB, and start+npages=2GB, which is right at the beginning of [2GB,3GB) chunk. > Then aligned_end is also 2GB, but we need to clear the [2GB, 3GB) chunk, right? Again, let's work with sizes instead of PFNs: addr=1GB size=1GB chunk_size=1GB Range: [ ] Chunks: [ - ][ - ][ - ][ - ] ... ^0 ^1 ^2 ^3 aligned_start = 1GB aligned_end = 2GB As you say, we'd clear the [1GB,2GB) chunk, but not the [2GB,3GB) chunk. But that's correct, as our range to hint is actually [start, start+npages) == [1GB,2GB). > > Best, > Wei >
On Friday, July 16, 2021 4:26 PM, David Hildenbrand wrote: > >>> + /* > >>> + * 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); > >>> + > >>> + size = 1ULL << (TARGET_PAGE_BITS + shift); > >>> + start = (((ram_addr_t)page) << TARGET_PAGE_BITS) & (-size); > >> > >> these as well as. > > > > Is there any coding style requirement for this? > > Don't think so. It simply results in less LOC and less occurrences of variables. > > > My thought was that those operations could mostly be avoided if they > > don't pass the above if condition (e.g. just once per 1GB chunk). > > Usually the compiler will reshuffle as possible to optimize. But in this case, due > to clear_bmap_test_and_clear(), it might not be able to move the > computations behind that call. So the final code might actually differ. > > Not that we really care about this micro-optimization, though. OK, looks that's just a personal favor. I'm inclined to keeping the micro-optimization. > > > > >> > >>> + trace_migration_bitmap_clear_dirty(rb->idstr, start, size, page); > >>> + memory_region_clear_dirty_bitmap(rb->mr, start, size); } > >>> + > >>> +static void > >>> +migration_clear_memory_region_dirty_bitmap_range(RAMState *rs, > >>> + RAMBlock > *rb, > >>> + unsigned > long > >> start, > >>> + unsigned > long > >>> +npages) { > >>> + unsigned long page_to_clear, i, nchunks; > >>> + unsigned long chunk_pages = 1UL << rb->clear_bmap_shift; > >>> + > >>> + nchunks = (start + npages) / chunk_pages - start / chunk_pages > >>> + + 1; > >> > >> Wouldn't you have to align the start and the end range up/down to > >> properly calculate the number of chunks? > > > > No, divide will round it to the integer (beginning of the chunk to clear). > > > nchunks = (start + npages) / chunk_pages - start / chunk_pages + 1; I had a mistake on the right boundary, it should be [start, start + npages), instead of [start, start + npages]. i.e. nchunks = (start + npages - 1) / chunk_pages - start / chunk_pages + 1 But I can take your approach here, thanks. Best, Wei
diff --git a/migration/ram.c b/migration/ram.c index b5fc454b2f..69e06b55ec 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -789,6 +789,51 @@ unsigned long migration_bitmap_find_dirty(RAMState *rs, RAMBlock *rb, return find_next_bit(bitmap, size, start); } +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; + /* + * 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); + + 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 void +migration_clear_memory_region_dirty_bitmap_range(RAMState *rs, + RAMBlock *rb, + unsigned long start, + unsigned long npages) +{ + unsigned long page_to_clear, i, nchunks; + unsigned long chunk_pages = 1UL << rb->clear_bmap_shift; + + nchunks = (start + npages) / chunk_pages - start / chunk_pages + 1; + + for (i = 0; i < nchunks; i++) { + page_to_clear = start + i * chunk_pages; + migration_clear_memory_region_dirty_bitmap(rs, rb, page_to_clear); + } +} + static inline bool migration_bitmap_clear_dirty(RAMState *rs, RAMBlock *rb, unsigned long page) @@ -803,26 +848,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--; } @@ -2741,6 +2769,14 @@ void qemu_guest_free_page_hint(void *addr, size_t len) npages = used_len >> TARGET_PAGE_BITS; qemu_mutex_lock(&ram_state->bitmap_mutex); + /* + * 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. + */ + migration_clear_memory_region_dirty_bitmap_range(ram_state, block, + start, npages); ram_state->migration_dirty_pages -= bitmap_count_one_with_offset(block->bmap, start, npages); bitmap_clear(block->bmap, start, npages);
When skipping free pages to send, their corresponding dirty bits in the memory region dirty bitmap need to be cleared. Otherwise the skipped pages will be sent in the next round after the migration thread syncs dirty bits from the memory region dirty bitmap. Cc: David Hildenbrand <david@redhat.com> Cc: Peter Xu <peterx@redhat.com> Cc: Michael S. Tsirkin <mst@redhat.com> Reported-by: David Hildenbrand <david@redhat.com> Signed-off-by: Wei Wang <wei.w.wang@intel.com> --- migration/ram.c | 72 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 54 insertions(+), 18 deletions(-) v1->v2 changelog: - move migration_clear_memory_region_dirty_bitmap under bitmap_mutex as we lack confidence to have it outside the lock for now. - clean the unnecessary subproject commit.