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

[Peter Xu <peterx@redhat.com>]

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 <a.perevalov@samsung.com>
> ---
>  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?

> +}
> +
>  /*
>   * 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.)

Besides:

Reviewed-by: Peter Xu <peterx@redhat.com>

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