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

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> On Wed, Jan 19, 2022 at 04:36:50PM +0000, Dr. David Alan Gilbert wrote:
> > * 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.
> 
> Ah, that makes sense.  How about I keep the test_bit but remove the loop?  I
> can make both a) and b) into one tracepoint:

Yes, i think that's fine.


> ========
> diff --git a/migration/ram.c b/migration/ram.c
> index 0df15ff663..02f36fa6d5 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1572,6 +1572,9 @@ static RAMBlock *unqueue_page(RAMState *rs, ram_addr_t 
> *offset)
>          migration_consume_urgent_request();
>      }
>  
> +    trace_unqueue_page(block->idstr, *offset,
> +                       test_bit((*offset >> TARGET_PAGE_BITS), block->bmap));
> +
>      return block;
>  }
>  
> diff --git a/migration/trace-events b/migration/trace-events
> index 3a9b3567ae..efa3a95f81 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -110,6 +110,7 @@ ram_save_iterate_big_wait(uint64_t milliconds, int 
> iterations) "big wait: %" PRI
>  ram_load_complete(int ret, uint64_t seq_iter) "exit_code %d seq iteration %" 
> PRIu64
>  ram_write_tracking_ramblock_start(const char *block_id, size_t page_size, 
> void *addr, size_t length) "%s: page_size: %zu addr: %p length: %zu"
>  ram_write_tracking_ramblock_stop(const char *block_id, size_t page_size, 
> void *addr, size_t length) "%s: page_size: %zu addr: %p length: %zu"
> +unqueue_page(char *block, uint64_t offset, bool dirty) "ramblock '%s' offset 
> 0x%"PRIx64" dirty %d"
>  
>  # multifd.c
>  multifd_new_send_channel_async(uint8_t id) "channel %d"
> ========
> 
> Thanks,
> 
> -- 
> Peter Xu
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK