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

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

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > 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 <dgilbert@redhat.com>
> 
> 
> > +static bool last_was_from_queue;
> 
> Are we using this variable later in the series?

That was a straggler; I've killed it off.

> >  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.

I've added an unref to the 'flush_page_queue' routine that's
called during cleanup; thus we take a ref whenever anything is added to
the queue, and release it either when we remove it to use it, or during
cleanup.

> > +            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?

I've just posted a pair of patches separately that do this; please let
me know if they're on the right lines; they can be applied without postcopy.

> 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.

Yes we could do that; at the moment I'm working in host page sized chunks.

> > +
> > +        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()

Renamed to the shorter get_queued_page (it's static anyway).

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK