Re: [Qemu-devel] [PATCH v2 2/3] migration: introduce qemu_ufd_copy_ioctl helper

[Dr. David Alan Gilbert]

* Peter Xu (address@hidden) wrote:
> On Thu, Jun 15, 2017 at 07:36:14PM +0300, Alexey Perevalov wrote:
> > Just for placing auxilary operations inside helper,
> > auxilary operations like: track received pages,
> > notify about copying operation in futher patches.
> > 
> > Signed-off-by: Alexey Perevalov <address@hidden>
> > ---
> >  migration/postcopy-ram.c | 35 ++++++++++++++++++++++-------------
> >  1 file changed, 22 insertions(+), 13 deletions(-)
> > 
> > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > index 743ca6c..38a429a 100644
> > --- a/migration/postcopy-ram.c
> > +++ b/migration/postcopy-ram.c
> > @@ -561,6 +561,26 @@ int postcopy_ram_enable_notify(MigrationIncomingState 
> > *mis)
> >      return 0;
> >  }
> >  
> > +static int qemu_ufd_copy_ioctl(int userfault_fd, void *host_addr,
> > +        void *from_addr, uint64_t pagesize)
> > +{
> > +    if (from_addr) {
> > +        struct uffdio_copy copy_struct;
> > +        copy_struct.dst = (uint64_t)(uintptr_t)host_addr;
> > +        copy_struct.src = (uint64_t)(uintptr_t)from_addr;
> > +        copy_struct.len = pagesize;
> > +        copy_struct.mode = 0;
> > +        return ioctl(userfault_fd, UFFDIO_COPY, &copy_struct);
> > +    } else {
> > +        struct uffdio_zeropage zero_struct;
> > +        zero_struct.range.start = (uint64_t)(uintptr_t)host_addr;
> > +        zero_struct.range.len = pagesize;
> > +        zero_struct.mode = 0;
> > +        return ioctl(userfault_fd, UFFDIO_ZEROPAGE, &zero_struct);
> > +    }
> > +    return 0;
> 
> Remove this line?

Yes, probably should; only minor.

> > +}
> > +
> >  /*
> >   * Place a host page (from) at (host) atomically
> >   * returns 0 on success
> > @@ -568,20 +588,14 @@ int postcopy_ram_enable_notify(MigrationIncomingState 
> > *mis)
> >  int postcopy_place_page(MigrationIncomingState *mis, void *host, void 
> > *from,
> >                          RAMBlock *rb)
> >  {
> > -    struct uffdio_copy copy_struct;
> >      size_t pagesize = qemu_ram_pagesize(rb);
> >  
> > -    copy_struct.dst = (uint64_t)(uintptr_t)host;
> > -    copy_struct.src = (uint64_t)(uintptr_t)from;
> > -    copy_struct.len = pagesize;
> > -    copy_struct.mode = 0;
> > -
> >      /* copy also acks to the kernel waking the stalled thread up
> >       * TODO: We can inhibit that ack and only do it if it was requested
> >       * which would be slightly cheaper, but we'd have to be careful
> >       * of the order of updating our page state.
> >       */
> > -    if (ioctl(mis->userfault_fd, UFFDIO_COPY, &copy_struct)) {
> > +    if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, from, pagesize)) {
> 
> (hmm I know "from" won't be zero, but naturally if without the context
>  I would doubt it. Shall we add an assert here? You call.)

Doesn't seem worth it.

> Besides:
> 
> Reviewed-by: Peter Xu <address@hidden>


Reviewed-by: Dr. David Alan Gilbert <address@hidden>

> 
> Thanks,
> 
> >          int e = errno;
> >          error_report("%s: %s copy host: %p from: %p (size: %zd)",
> >                       __func__, strerror(e), host, from, pagesize);
> > @@ -605,12 +619,7 @@ int postcopy_place_page_zero(MigrationIncomingState 
> > *mis, void *host,
> >  
> >      pagesize = qemu_ram_pagesize(rb);
> >      if (pagesize == getpagesize()) {
> > -        struct uffdio_zeropage zero_struct;
> > -        zero_struct.range.start = (uint64_t)(uintptr_t)host;
> > -        zero_struct.range.len = getpagesize();
> > -        zero_struct.mode = 0;
> > -
> > -        if (ioctl(mis->userfault_fd, UFFDIO_ZEROPAGE, &zero_struct)) {
> > +        if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, 0, 
> > getpagesize())) {
> >              int e = errno;
> >              error_report("%s: %s zero host: %p",
> >                           __func__, strerror(e), host);
> > -- 
> > 1.9.1
> > 
> 
> -- 
> Peter Xu
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK