Re: [PATCH RFC 05/15] migration: Simplify unqueue_page()

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

* Peter Xu (peterx@redhat.com) wrote:
> This patch simplifies unqueue_page() on both sides of it (itself, and caller).
> 
> Firstly, due to the fact that right after unqueue_page() returned true, we'll
> definitely send a huge page (see ram_save_huge_page() call - it will _never_
> exit before finish sending that huge page), so unqueue_page() does not need to
> jump in small page size if huge page is enabled on the ramblock.  IOW, it's
> destined that only the 1st 4K page will be valid, when unqueue the 2nd+ time
> we'll notice the whole huge page has already been sent anyway.  Switching to
> operating on huge page reduces a lot of the loops of redundant unqueue_page().
> 
> Meanwhile, drop the dirty check.  It's not helpful to call test_bit() every
> time to jump over clean pages, as ram_save_host_page() has already done so,
> while in a faster way (see commit ba1b7c812c ("migration/ram: Optimize
> ram_save_host_page()", 2021-05-13)).  So that's not necessary too.
> 
> Drop the two tracepoints along the way - based on above analysis it's very
> possible that no one is really using it..
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Yes, OK

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Although:
  a) You might like to keep a trace in get_queued_page just to see
what's getting unqueued
  b) I think originally it was a useful diagnostic to find out when we
were getting a lot of queue requests for pages that were already sent.

Dave


> ---
>  migration/ram.c        | 34 ++++++++--------------------------
>  migration/trace-events |  2 --
>  2 files changed, 8 insertions(+), 28 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index dc6ba041fa..0df15ff663 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1541,6 +1541,7 @@ static RAMBlock *unqueue_page(RAMState *rs, ram_addr_t *offset)
>  {
>      struct RAMSrcPageRequest *entry;
>      RAMBlock *block = NULL;
> +    size_t page_size;
>  
>      if (!postcopy_has_request(rs)) {
>          return NULL;
> @@ -1557,10 +1558,13 @@ static RAMBlock *unqueue_page(RAMState *rs, ram_addr_t *offset)
>      entry = QSIMPLEQ_FIRST(&rs->src_page_requests);
>      block = entry->rb;
>      *offset = entry->offset;
> +    page_size = qemu_ram_pagesize(block);
> +    /* Each page request should only be multiple page size of the ramblock */
> +    assert((entry->len % page_size) == 0);
>  
> -    if (entry->len > TARGET_PAGE_SIZE) {
> -        entry->len -= TARGET_PAGE_SIZE;
> -        entry->offset += TARGET_PAGE_SIZE;
> +    if (entry->len > page_size) {
> +        entry->len -= page_size;
> +        entry->offset += page_size;
>      } else {
>          memory_region_unref(block->mr);
>          QSIMPLEQ_REMOVE_HEAD(&rs->src_page_requests, next_req);
> @@ -1942,30 +1946,8 @@ static bool get_queued_page(RAMState *rs, PageSearchStatus *pss)
>  {
>      RAMBlock  *block;
>      ram_addr_t offset;
> -    bool dirty;
>  
> -    do {
> -        block = unqueue_page(rs, &offset);
> -        /*
> -         * We're sending this page, and since it's postcopy nothing else
> -         * will dirty it, and we must make sure it doesn't get sent again
> -         * even if this queue request was received after the background
> -         * search already sent it.
> -         */
> -        if (block) {
> -            unsigned long page;
> -
> -            page = offset >> TARGET_PAGE_BITS;
> -            dirty = test_bit(page, block->bmap);
> -            if (!dirty) {
> -                trace_get_queued_page_not_dirty(block->idstr, (uint64_t)offset,
> -                                                page);
> -            } else {
> -                trace_get_queued_page(block->idstr, (uint64_t)offset, page);
> -            }
> -        }
> -
> -    } while (block && !dirty);
> +    block = unqueue_page(rs, &offset);
>  
>      if (!block) {
>          /*
> diff --git a/migration/trace-events b/migration/trace-events
> index e165687af2..3a9b3567ae 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -85,8 +85,6 @@ put_qlist_end(const char *field_name, const char *vmsd_name) "%s(%s)"
>  qemu_file_fclose(void) ""
>  
>  # ram.c
> -get_queued_page(const char *block_name, uint64_t tmp_offset, unsigned long page_abs) "%s/0x%" PRIx64 " page_abs=0x%lx"
> -get_queued_page_not_dirty(const char *block_name, uint64_t tmp_offset, unsigned long page_abs) "%s/0x%" PRIx64 " page_abs=0x%lx"
>  migration_bitmap_sync_start(void) ""
>  migration_bitmap_sync_end(uint64_t dirty_pages) "dirty_pages %" PRIu64
>  migration_bitmap_clear_dirty(char *str, uint64_t start, uint64_t size, unsigned long page) "rb %s start 0x%"PRIx64" size 0x%"PRIx64" page 0x%lx"
> -- 
> 2.32.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK