Re: [PATCH RFC 14/21] migration: Map hugetlbfs ramblocks twice, and pre-allocate

[Peter Xu]

On Mon, Jan 30, 2023 at 06:24:20AM +0100, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > Add a RAMBlock.host_mirror for all the hugetlbfs backed guest memories.
> > It'll be used to remap the same region twice and it'll be used to service
> > page faults using UFFDIO_CONTINUE.
> >
> > To make sure all accesses to these ranges will generate minor page faults
> > not missing page faults, we need to pre-allocate the files to make sure
> > page cache exist start from the beginning.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> but what about this change
> 
> > ---
> >  include/exec/ramblock.h |  7 +++++
> >  migration/ram.c         | 59 +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 66 insertions(+)
> >
> > diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
> > index 3f31ce1591..c76683c3c8 100644
> > --- a/include/exec/ramblock.h
> > +++ b/include/exec/ramblock.h
> > @@ -28,6 +28,13 @@ struct RAMBlock {
> >      struct rcu_head rcu;
> >      struct MemoryRegion *mr;
> >      uint8_t *host;
> > +    /*
> > +     * This is only used for hugetlbfs ramblocks where doublemap is
> > +     * enabled.  The pointer is managed by dest host migration code, and
> > +     * should be NULL when migration is finished.  On src host, it should
> > +     * always be NULL.
> > +     */
> > +    uint8_t *host_mirror;
> 
> I would consider here:
> 
>     uint8_t *host_doublemap;
> 
> as I have not a small name that means
>     uint8_t *host_map_smaller_size_pages;
> 
> That explains why we need it.

Sure, I can rename this one if it helps.

One thing worth mention is that, it's not mapping things in small page size
here with host_doublemap but in huge page size only.

It's just that UFFDIO_CONTINUE needs another mapping to resolve the page
faults. It'll be the guest hugetlb ramblocks that will be mapped in small
pages during postcopy.

> 
> 
> >      uint8_t *colo_cache; /* For colo, VM's ram cache */
> >      ram_addr_t offset;
> >      ram_addr_t used_length;
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 2ebf414f5f..37d7b3553a 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -3879,6 +3879,57 @@ void colo_release_ram_cache(void)
> >      ram_state_cleanup(&ram_state);
> >  }
> >  
> > +static int migrate_hugetlb_doublemap_init(void)
> > +{
> > +    RAMBlock *rb;
> > +    void *addr;
> > +    int ret;
> 
> Not initialized variables, remove the last two.

I can do this.

> 
> > +    if (!migrate_hugetlb_doublemap()) {
> > +        return 0;
> > +    }
> > +
> 
> I would move the declaration of the RAMBlock here.

But isn't QEMU in most cases declaring variables at the start of any code
block, rather than after or in the middle of any code segments?  IIRC some
compiler should start to fail with it, even though not on the modern ones.

Thanks,

-- 
Peter Xu