Re: [Qemu-devel] [PATCH v7 32/42] Page request: Consume pages off the post-copy queue

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <address@hidden> wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
>
> When transmitting RAM pages, consume pages that have been queued by
> MIG_RPCOMM_REQPAGE commands and send them ahead of normal page scanning.
>
> Note:
>   a) After a queued page the linear walk carries on from after the
> unqueued page; there is a reasonable chance that the destination
> was about to ask for other closeby pages anyway.
>
>   b) We have to be careful of any assumptions that the page walking
> code makes, in particular it does some short cuts on its first linear
> walk that break as soon as we do a queued page.
>
>   c) We have to be careful to not break up host-page size chunks, since
> this makes it harder to place the pages on the destination.
>
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>


> +static bool last_was_from_queue;

Are we using this variable later in the series?

>  static void migration_bitmap_sync_range(ram_addr_t start, ram_addr_t length)
>  {
>      migration_dirty_pages +=
> @@ -923,6 +933,41 @@ static int ram_save_compressed_page(QEMUFile *f, 
> RAMBlock *block,
>      return pages;
>  }
>  
> +/*
> + * Unqueue a page from the queue fed by postcopy page requests
> + *
> + * Returns:      The RAMBlock* to transmit from (or NULL if the queue is 
> empty)
> + *      ms:      MigrationState in
> + *  offset:      the byte offset within the RAMBlock for the start of the 
> page
> + * ram_addr_abs: global offset in the dirty/sent bitmaps
> + */
> +static RAMBlock *ram_save_unqueue_page(MigrationState *ms, ram_addr_t 
> *offset,
> +                                       ram_addr_t *ram_addr_abs)
> +{
> +    RAMBlock *result = NULL;
> +    qemu_mutex_lock(&ms->src_page_req_mutex);
> +    if (!QSIMPLEQ_EMPTY(&ms->src_page_requests)) {
> +        struct MigrationSrcPageRequest *entry =
> +                                    QSIMPLEQ_FIRST(&ms->src_page_requests);
> +        result = entry->rb;
> +        *offset = entry->offset;
> +        *ram_addr_abs = (entry->offset + entry->rb->offset) & 
> TARGET_PAGE_MASK;
> +
> +        if (entry->len > TARGET_PAGE_SIZE) {
> +            entry->len -= TARGET_PAGE_SIZE;
> +            entry->offset += TARGET_PAGE_SIZE;
> +        } else {
> +            memory_region_unref(result->mr);

Here it is the unref, but I still don't understand why we don't need to
undo that on the error case on previous patch.

> +            QSIMPLEQ_REMOVE_HEAD(&ms->src_page_requests, next_req);
> +            g_free(entry);
> +        }
> +    }
> +    qemu_mutex_unlock(&ms->src_page_req_mutex);
> +
> +    return result;
> +}
> +
> +
>  /**
>   * Queue the pages for transmission, e.g. a request from postcopy destination
>   *   ms: MigrationStatus in which the queue is held
> @@ -987,6 +1032,58 @@ err:
>  

> @@ -997,65 +1094,102 @@ err:
>   * @f: QEMUFile where to send the data
>   * @last_stage: if we are at the completion stage
>   * @bytes_transferred: increase it with the number of transferred bytes
> + *
> + * On systems where host-page-size > target-page-size it will send all the
> + * pages in a host page that are dirty.
>   */
>  
>  static int ram_find_and_save_block(QEMUFile *f, bool last_stage,
>                                     uint64_t *bytes_transferred)
>  {
> +    MigrationState *ms = migrate_get_current();
>      RAMBlock *block = last_seen_block;
> +    RAMBlock *tmpblock;
>      ram_addr_t offset = last_offset;
> +    ram_addr_t tmpoffset;
>      bool complete_round = false;
>      int pages = 0;
> -    MemoryRegion *mr;
>      ram_addr_t dirty_ram_abs; /* Address of the start of the dirty page in
>                                   ram_addr_t space */
>  
> -    if (!block)
> +    if (!block) {
>          block = QLIST_FIRST_RCU(&ram_list.blocks);
> +        last_was_from_queue = false;
> +    }
>  
> -    while (true) {
> -        mr = block->mr;
> -        offset = migration_bitmap_find_and_reset_dirty(mr, offset,
> -                                                       &dirty_ram_abs);
> -        if (complete_round && block == last_seen_block &&
> -            offset >= last_offset) {
> -            break;
> -        }
> -        if (offset >= block->used_length) {
> -            offset = 0;
> -            block = QLIST_NEXT_RCU(block, next);
> -            if (!block) {
> -                block = QLIST_FIRST_RCU(&ram_list.blocks);
> -                complete_round = true;
> -                ram_bulk_stage = false;
> -                if (migrate_use_xbzrle()) {
> -                    /* If xbzrle is on, stop using the data compression at 
> this
> -                     * point. In theory, xbzrle can do better than 
> compression.
> -                     */
> -                    flush_compressed_data(f);
> -                    compression_switch = false;
> -                }
> +    while (true) { /* Until we send a block or run out of stuff to send */
> +        tmpblock = ram_save_unqueue_page(ms, &tmpoffset, &dirty_ram_abs);

This function was ugly.  You already split it in the past.  This patch
makes it even more complicated.  Can we try something like add a

ram_find_next_page() and try to put some of the code inside the while
there?

Once here, can we agree to send the next N pages (if they are contiguos)
if we receive a queued request?  Yeap, deciding N means testing and measuring.
And can wait for this to be integrated.

> +
> +        if (tmpblock) {
> +            /* We've got a block from the postcopy queue */
> +            trace_ram_find_and_save_block_postcopy(tmpblock->idstr,
> +                                                   (uint64_t)tmpoffset,
> +                                                   (uint64_t)dirty_ram_abs);
> +            /*
> +             * 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 (!test_bit(dirty_ram_abs >> TARGET_PAGE_BITS,
> +                          migration_bitmap)) {

I think this test can be inside ram_save_unqueue_page()

I.e. rename to:

ram_save_get_next_queued_page()