Re: [Qemu-devel] [PATCH v5 3/3] migration: add bitmap for received page

[Peter Xu]

On Tue, Jun 27, 2017 at 01:40:58PM +0300, Alexey wrote:
> On Tue, Jun 27, 2017 at 06:17:40PM +0800, Peter Xu wrote:
> > On Tue, Jun 27, 2017 at 05:50:27AM -0400, Alexey Perevalov wrote:
> > 
> > [...]
> > 
> > > @@ -60,6 +62,14 @@ static inline void *ramblock_ptr(RAMBlock *block, 
> > > ram_addr_t offset)
> > >      return (char *)block->host + offset;
> > >  }
> > >  
> > > +static inline unsigned long int ramblock_recv_bitmap_offset(void 
> > > *host_addr,
> > > +                                                            RAMBlock *rb)
> > > +{
> > > +    uint64_t host_addr_offset =
> > > +            (uint64_t)(uintptr_t)(host_addr - (void *)rb->host);
> > > +    return host_addr_offset >> TARGET_PAGE_BITS;
> > > +}
> > > +
> > >  long qemu_getrampagesize(void);
> > >  unsigned long last_ram_page(void);
> > >  RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
> > > diff --git a/migration/migration.c b/migration/migration.c
> > > index 71e38bc..53fbd41 100644
> > > --- a/migration/migration.c
> > > +++ b/migration/migration.c
> > > @@ -143,6 +143,7 @@ MigrationIncomingState 
> > > *migration_incoming_get_current(void)
> > >          qemu_mutex_init(&mis_current.rp_mutex);
> > >          qemu_event_init(&mis_current.main_thread_load_event, false);
> > >          once = true;
> > > +        ramblock_recv_map_init();
> > 
> > One tiny more comment: shall we init this at the beginning of incoming
> > migration? Maybe into migration_fd_process_incoming(), before entering
> > the coroutine?
> maybe this function (migration_incoming_get_current) is not best place
> to initialize something in ramblock list from point of
> view maintainability.

Yes, the point is, it is only inited once per QEMU instance, while
actually it should be inited for each incoming migration procedure
(though I think yes we will normally have one incoming migration per
QEMU instance...).

> > 
> > Then, for the destruction of it below...
> > 
> > [...]
> > 
> > > @@ -2324,8 +2352,14 @@ static int ram_load_setup(QEMUFile *f, void 
> > > *opaque)
> > >  
> > >  static int ram_load_cleanup(void *opaque)
> > >  {
> > > +    RAMBlock *rb;
> > >      xbzrle_load_cleanup();
> > >      compress_threads_load_cleanup();
> > > +
> > > +    RAMBLOCK_FOREACH(rb) {
> > > +        g_free(rb->receivedmap);
> > > +        rb->receivedmap = NULL;
> > > +    }
> > 
> > ... maybe move to migration_incoming_state_destroy()?
> I'll think about it, because ram_load_cleanup in current Juan's
> patch set is not calling in postcopy scenario.

Sure.

> 
> > 
> > And, I didn't really find ram_load_cleanup() in my repo. Am I missing
> > something?
> you need Juan's [PATCH v2 0/5] Create setup/cleanup methods for
> migration incoming side

Ok. Then no problem. Thanks,

> 
> > 
> > Other than above, this patch looks good to me.  Thanks,
> > 
> > -- 
> > Peter Xu
> > 
> 
> -- 
> 
> BR
> Alexey

-- 
Peter Xu