* Peter Xu (pet...@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 <pet...@redhat.com>
Yes, OK
Reviewed-by: Dr. David Alan Gilbert <dgilb...@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 / dgilb...@redhat.com / Manchester, UK
