Re: [Qemu-devel] [PATCH v2 09/16] postcopy: Use temporary for placing zero huge pages

[Dr. David Alan Gilbert]

* Laurent Vivier (address@hidden) wrote:
> On 06/02/2017 18:32, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> > 
> > The kernel can't do UFFDIO_ZEROPAGE for huge pages, so we have
> > to allocate a temporary (always zero) page and use UFFDIO_COPYPAGE
> > on it.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > Reviewed-by: Juan Quintela <address@hidden>
> > ---
> >  include/migration/migration.h |  1 +
> >  migration/postcopy-ram.c      | 23 +++++++++++++++++++++--
> >  2 files changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/migration/migration.h b/include/migration/migration.h
> > index c9c1d5f..bd399fc 100644
> > --- a/include/migration/migration.h
> > +++ b/include/migration/migration.h
> > @@ -108,6 +108,7 @@ struct MigrationIncomingState {
> >      QEMUFile *to_src_file;
> >      QemuMutex rp_mutex;    /* We send replies from multiple threads */
> >      void     *postcopy_tmp_page;
> > +    void     *postcopy_tmp_zero_page;
> >  
> >      QEMUBH *bh;
> >  
> > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > index a8b7fed..4c736d2 100644
> > --- a/migration/postcopy-ram.c
> > +++ b/migration/postcopy-ram.c
> > @@ -324,6 +324,10 @@ int 
> > postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
> >          munmap(mis->postcopy_tmp_page, mis->largest_page_size);
> >          mis->postcopy_tmp_page = NULL;
> >      }
> > +    if (mis->postcopy_tmp_zero_page) {
> > +        munmap(mis->postcopy_tmp_zero_page, mis->largest_page_size);
> > +        mis->postcopy_tmp_zero_page = NULL;
> > +    }
> >      trace_postcopy_ram_incoming_cleanup_exit();
> >      return 0;
> >  }
> > @@ -593,8 +597,23 @@ int postcopy_place_page_zero(MigrationIncomingState 
> > *mis, void *host,
> >              return -e;
> >          }
> >      } else {
> > -        /* TODO: The kernel can't use UFFDIO_ZEROPAGE for hugepages */
> > -        assert(0);
> > +        /* The kernel can't use UFFDIO_ZEROPAGE for hugepages */
> > +        if (!mis->postcopy_tmp_zero_page) {
> > +            mis->postcopy_tmp_zero_page = mmap(NULL, 
> > mis->largest_page_size,
> > +                                               PROT_READ | PROT_WRITE,
> > +                                               MAP_PRIVATE | MAP_ANONYMOUS,
> > +                                               -1, 0);
> > +            if (mis->postcopy_tmp_zero_page == MAP_FAILED) {
> > +                int e = errno;
> > +                mis->postcopy_tmp_zero_page = NULL;
> > +                error_report("%s: %s mapping large zero page",
> > +                             __func__, strerror(e));
> > +                return -e;
> > +            }
> > +            memset(mis->postcopy_tmp_zero_page, '\0', 
> > mis->largest_page_size);
> > +        }
> > +        return postcopy_place_page(mis, host, mis->postcopy_tmp_zero_page,
> > +                                   pagesize);
> >      }
> 
> It's sad to have to allocate 1 huge page just to zero them.
> 
> Are you sure the kernel doesn't support UFFDIO_ZEROPAGE for huge page.
> It seems __mcopy_atomic() manages HUGETLB vma (it is called by
> mfill_zeropage(), called by userfaultfd_zeropage())?

That's as I understand it from Andrea; and I think it does fail if you try it.

> Anyway, the code looks good:
> Reviewed-by: Laurent Vivier <address@hidden>

Thanks.

Dave

> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK