Re: [Qemu-devel] [PATCH V7 RESEND 09/17] COLO: Flush memory data from ram cache

[Zhang Chen]

On Tue, May 15, 2018 at 10:44 PM, Dr. David Alan Gilbert <
address@hidden> wrote:

> * Zhang Chen (address@hidden) wrote:
> > During the time of VM's running, PVM may dirty some pages, we will
> transfer
> > PVM's dirty pages to SVM and store them into SVM's RAM cache at next
> checkpoint
> > time. So, the content of SVM's RAM cache will always be same with PVM's
> memory
> > after checkpoint.
> >
> > Instead of flushing all content of PVM's RAM cache into SVM's MEMORY,
> > we do this in a more efficient way:
> > Only flush any page that dirtied by PVM since last checkpoint.
> > In this way, we can ensure SVM's memory same with PVM's.
> >
> > Besides, we must ensure flush RAM cache before load device state.
> >
> > Signed-off-by: zhanghailiang <address@hidden>
> > Signed-off-by: Li Zhijian <address@hidden>
> > Reviewed-by: Dr. David Alan Gilbert <address@hidden>
> > ---
> >  migration/ram.c        | 39 +++++++++++++++++++++++++++++++++++++++
> >  migration/trace-events |  2 ++
> >  2 files changed, 41 insertions(+)
> >
> > diff --git a/migration/ram.c b/migration/ram.c
> > index e35dfee06e..4235a8f24d 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -3031,6 +3031,40 @@ static bool postcopy_is_running(void)
> >      return ps >= POSTCOPY_INCOMING_LISTENING && ps <
> POSTCOPY_INCOMING_END;
> >  }
> >
> > +/*
> > + * Flush content of RAM cache into SVM's memory.
> > + * Only flush the pages that be dirtied by PVM or SVM or both.
> > + */
> > +static void colo_flush_ram_cache(void)
> > +{
> > +    RAMBlock *block = NULL;
> > +    void *dst_host;
> > +    void *src_host;
> > +    unsigned long offset = 0;
> > +
> > +    trace_colo_flush_ram_cache_begin(ram_state->migration_dirty_pages);
> > +    rcu_read_lock();
> > +    block = QLIST_FIRST_RCU(&ram_list.blocks);
> > +
> > +    while (block) {
> > +        offset = migration_bitmap_find_dirty(ram_state, block, offset);
> > +        migration_bitmap_clear_dirty(ram_state, block, offset);
>
> That looks suspicious to me; shouldn't that be inside the else block
> below?   If the find_dirty returns a bit after or equal to used_length
> (i.e. there's nothing dirty in this block), then you don't want to clear
> that bit yet because it really means there's a dirty page at the start
> of the next block?
>
>
Make sense. I will move it to the 'else block below'.

Thanks
Zhang Chen



> Dave
>
> > +        if (offset << TARGET_PAGE_BITS >= block->used_length) {
> > +            offset = 0;
> > +            block = QLIST_NEXT_RCU(block, next);
> > +        } else {
> > +            dst_host = block->host + (offset << TARGET_PAGE_BITS);
> > +            src_host = block->colo_cache + (offset << TARGET_PAGE_BITS);
> > +            memcpy(dst_host, src_host, TARGET_PAGE_SIZE);
> > +        }
> > +    }
> > +
> > +    rcu_read_unlock();
> > +    trace_colo_flush_ram_cache_end();
> > +    assert(ram_state->migration_dirty_pages == 0);
> > +}
> > +
> >  static int ram_load(QEMUFile *f, void *opaque, int version_id)
> >  {
> >      int flags = 0, ret = 0, invalid_flags = 0;
> > @@ -3043,6 +3077,7 @@ static int ram_load(QEMUFile *f, void *opaque, int
> version_id)
> >      bool postcopy_running = postcopy_is_running();
> >      /* ADVISE is earlier, it shows the source has the postcopy
> capability on */
> >      bool postcopy_advised = postcopy_is_advised();
> > +    bool need_flush = false;
> >
> >      seq_iter++;
> >
> > @@ -3218,6 +3253,10 @@ static int ram_load(QEMUFile *f, void *opaque,
> int version_id)
> >      ret |= wait_for_decompress_done();
> >      rcu_read_unlock();
> >      trace_ram_load_complete(ret, seq_iter);
> > +
> > +    if (!ret  && migration_incoming_in_colo_state() && need_flush) {
> > +        colo_flush_ram_cache();
> > +    }
> >      return ret;
> >  }
> >
> > diff --git a/migration/trace-events b/migration/trace-events
> > index 9295b4cf40..8e2f9749e0 100644
> > --- a/migration/trace-events
> > +++ b/migration/trace-events
> > @@ -78,6 +78,8 @@ ram_load_postcopy_loop(uint64_t addr, int flags) "@%"
> PRIx64 " %x"
> >  ram_postcopy_send_discard_bitmap(void) ""
> >  ram_save_page(const char *rbname, uint64_t offset, void *host) "%s:
> offset: 0x%" PRIx64 " host: %p"
> >  ram_save_queue_pages(const char *rbname, size_t start, size_t len) "%s:
> start: 0x%zx len: 0x%zx"
> > +colo_flush_ram_cache_begin(uint64_t dirty_pages) "dirty_pages %" PRIu64
> > +colo_flush_ram_cache_end(void) ""
> >
> >  # migration/migration.c
> >  await_return_path_close_on_source_close(void) ""
> > --
> > 2.17.0
> >
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK
>