Re: [PATCH v2] migration: clear the memory region dirty bitmap when skipping free pages

[David Hildenbrand <david@redhat.com>]

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)

-- 
Thanks,

David / dhildenb