diff mbox series

[v2,6/7] migration/postcopy: Use uffd helpers

Message ID 20240919134626.166183-7-dave@treblig.org (mailing list archive)
State New, archived
Headers show
Series Migration deadcode removal | expand

Commit Message

Dr. David Alan Gilbert Sept. 19, 2024, 1:46 p.m. UTC 
From: "Dr. David Alan Gilbert" <dave@treblig.org>

Use the uffd_copy_page, uffd_zero_page and uffd_wakeup helpers
rather than calling ioctl ourselves.

They return -errno on error, and print an error_report themselves.
I think this actually makes postcopy_place_page actually more
consistent in it's callers.

Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org>
---
 migration/postcopy-ram.c | 47 +++++++++++-----------------------------
 1 file changed, 13 insertions(+), 34 deletions(-)

Comments

Peter Xu Sept. 19, 2024, 5:44 p.m. UTC | #1 
On Thu, Sep 19, 2024 at 02:46:25PM +0100, dave@treblig.org wrote:
> From: "Dr. David Alan Gilbert" <dave@treblig.org>
> 
> Use the uffd_copy_page, uffd_zero_page and uffd_wakeup helpers
> rather than calling ioctl ourselves.
> 
> They return -errno on error, and print an error_report themselves.
> I think this actually makes postcopy_place_page actually more
> consistent in it's callers.
> 
> Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org>

Reviewed-by: Peter Xu <peterx@redhat.com>
Peter Xu Sept. 30, 2024, 7:51 p.m. UTC | #2 
On Thu, Sep 19, 2024 at 02:46:25PM +0100, dave@treblig.org wrote:
> From: "Dr. David Alan Gilbert" <dave@treblig.org>
> 
> Use the uffd_copy_page, uffd_zero_page and uffd_wakeup helpers
> rather than calling ioctl ourselves.
> 
> They return -errno on error, and print an error_report themselves.
> I think this actually makes postcopy_place_page actually more
> consistent in it's callers.
> 
> Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org>
> ---
>  migration/postcopy-ram.c | 47 +++++++++++-----------------------------
>  1 file changed, 13 insertions(+), 34 deletions(-)
> 
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 1c374b7ea1..e2b318d3da 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -746,18 +746,9 @@ int postcopy_wake_shared(struct PostCopyFD *pcfd,
>                           RAMBlock *rb)
>  {
>      size_t pagesize = qemu_ram_pagesize(rb);
> -    struct uffdio_range range;
> -    int ret;
>      trace_postcopy_wake_shared(client_addr, qemu_ram_get_idstr(rb));
> -    range.start = ROUND_DOWN(client_addr, pagesize);
> -    range.len = pagesize;
> -    ret = ioctl(pcfd->fd, UFFDIO_WAKE, &range);
> -    if (ret) {
> -        error_report("%s: Failed to wake: %zx in %s (%s)",
> -                     __func__, (size_t)client_addr, qemu_ram_get_idstr(rb),
> -                     strerror(errno));
> -    }
> -    return ret;
> +    return uffd_wakeup(pcfd->fd, (void *)ROUND_DOWN(client_addr, pagesize),
> +                       pagesize);
>  }

There's a build issue on i386:

../migration/postcopy-ram.c: In function ‘postcopy_wake_shared’:
../migration/postcopy-ram.c:750:34: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
  750 |     return uffd_wakeup(pcfd->fd, (void *)ROUND_DOWN(client_addr, pagesize),
      |                                  ^

The plan is to squash below fix:

=========8<===========
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 03a63ef5cd..83f6160a36 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
-@@ -747,7 +747,8 @@ int postcopy_wake_shared(struct PostCopyFD *pcfd,
 {
     size_t pagesize = qemu_ram_pagesize(rb);
     trace_postcopy_wake_shared(client_addr, qemu_ram_get_idstr(rb));
-    return uffd_wakeup(pcfd->fd, (void *)ROUND_DOWN(client_addr, pagesize),
+    return uffd_wakeup(pcfd->fd,
+                       (void *)(uintptr_t)ROUND_DOWN(client_addr, pagesize),
                        pagesize);
 }
=========8<===========

Thanks,
Dr. David Alan Gilbert Sept. 30, 2024, 8:23 p.m. UTC | #3 
* Peter Xu (peterx@redhat.com) wrote:
> On Thu, Sep 19, 2024 at 02:46:25PM +0100, dave@treblig.org wrote:
> > From: "Dr. David Alan Gilbert" <dave@treblig.org>
> > 
> > Use the uffd_copy_page, uffd_zero_page and uffd_wakeup helpers
> > rather than calling ioctl ourselves.
> > 
> > They return -errno on error, and print an error_report themselves.
> > I think this actually makes postcopy_place_page actually more
> > consistent in it's callers.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org>
> > ---
> >  migration/postcopy-ram.c | 47 +++++++++++-----------------------------
> >  1 file changed, 13 insertions(+), 34 deletions(-)
> > 
> > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > index 1c374b7ea1..e2b318d3da 100644
> > --- a/migration/postcopy-ram.c
> > +++ b/migration/postcopy-ram.c
> > @@ -746,18 +746,9 @@ int postcopy_wake_shared(struct PostCopyFD *pcfd,
> >                           RAMBlock *rb)
> >  {
> >      size_t pagesize = qemu_ram_pagesize(rb);
> > -    struct uffdio_range range;
> > -    int ret;
> >      trace_postcopy_wake_shared(client_addr, qemu_ram_get_idstr(rb));
> > -    range.start = ROUND_DOWN(client_addr, pagesize);
> > -    range.len = pagesize;
> > -    ret = ioctl(pcfd->fd, UFFDIO_WAKE, &range);
> > -    if (ret) {
> > -        error_report("%s: Failed to wake: %zx in %s (%s)",
> > -                     __func__, (size_t)client_addr, qemu_ram_get_idstr(rb),
> > -                     strerror(errno));
> > -    }
> > -    return ret;
> > +    return uffd_wakeup(pcfd->fd, (void *)ROUND_DOWN(client_addr, pagesize),
> > +                       pagesize);
> >  }
> 
> There's a build issue on i386:
> 
> ../migration/postcopy-ram.c: In function ‘postcopy_wake_shared’:
> ../migration/postcopy-ram.c:750:34: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>   750 |     return uffd_wakeup(pcfd->fd, (void *)ROUND_DOWN(client_addr, pagesize),
>       |                                  ^
> 
> The plan is to squash below fix:

Thanks!

Dave

> =========8<===========
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 03a63ef5cd..83f6160a36 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> -@@ -747,7 +747,8 @@ int postcopy_wake_shared(struct PostCopyFD *pcfd,
>  {
>      size_t pagesize = qemu_ram_pagesize(rb);
>      trace_postcopy_wake_shared(client_addr, qemu_ram_get_idstr(rb));
> -    return uffd_wakeup(pcfd->fd, (void *)ROUND_DOWN(client_addr, pagesize),
> +    return uffd_wakeup(pcfd->fd,
> +                       (void *)(uintptr_t)ROUND_DOWN(client_addr, pagesize),
>                         pagesize);
>  }
> =========8<===========
> 
> Thanks,
> 
> -- 
> Peter Xu
>
diff mbox series

Patch

diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 1c374b7ea1..e2b318d3da 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -746,18 +746,9 @@  int postcopy_wake_shared(struct PostCopyFD *pcfd,
                          RAMBlock *rb)
 {
     size_t pagesize = qemu_ram_pagesize(rb);
-    struct uffdio_range range;
-    int ret;
     trace_postcopy_wake_shared(client_addr, qemu_ram_get_idstr(rb));
-    range.start = ROUND_DOWN(client_addr, pagesize);
-    range.len = pagesize;
-    ret = ioctl(pcfd->fd, UFFDIO_WAKE, &range);
-    if (ret) {
-        error_report("%s: Failed to wake: %zx in %s (%s)",
-                     __func__, (size_t)client_addr, qemu_ram_get_idstr(rb),
-                     strerror(errno));
-    }
-    return ret;
+    return uffd_wakeup(pcfd->fd, (void *)ROUND_DOWN(client_addr, pagesize),
+                       pagesize);
 }
 
 static int postcopy_request_page(MigrationIncomingState *mis, RAMBlock *rb,
@@ -1275,18 +1266,10 @@  static int qemu_ufd_copy_ioctl(MigrationIncomingState *mis, void *host_addr,
     int ret;
 
     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;
-        ret = ioctl(userfault_fd, UFFDIO_COPY, &copy_struct);
+        ret = uffd_copy_page(userfault_fd, host_addr, from_addr, pagesize,
+                             false);
     } 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;
-        ret = ioctl(userfault_fd, UFFDIO_ZEROPAGE, &zero_struct);
+        ret = uffd_zero_page(userfault_fd, host_addr, pagesize, false);
     }
     if (!ret) {
         qemu_mutex_lock(&mis->page_request_mutex);
@@ -1343,18 +1326,16 @@  int postcopy_place_page(MigrationIncomingState *mis, void *host, void *from,
                         RAMBlock *rb)
 {
     size_t pagesize = qemu_ram_pagesize(rb);
+    int e;
 
     /* 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 (qemu_ufd_copy_ioctl(mis, host, from, pagesize, rb)) {
-        int e = errno;
-        error_report("%s: %s copy host: %p from: %p (size: %zd)",
-                     __func__, strerror(e), host, from, pagesize);
-
-        return -e;
+    e = qemu_ufd_copy_ioctl(mis, host, from, pagesize, rb);
+    if (e) {
+        return e;
     }
 
     trace_postcopy_place_page(host);
@@ -1376,12 +1357,10 @@  int postcopy_place_page_zero(MigrationIncomingState *mis, void *host,
      * but it's not available for everything (e.g. hugetlbpages)
      */
     if (qemu_ram_is_uf_zeroable(rb)) {
-        if (qemu_ufd_copy_ioctl(mis, host, NULL, pagesize, rb)) {
-            int e = errno;
-            error_report("%s: %s zero host: %p",
-                         __func__, strerror(e), host);
-
-            return -e;
+        int e;
+        e = qemu_ufd_copy_ioctl(mis, host, NULL, pagesize, rb);
+        if (e) {
+            return e;
         }
         return postcopy_notify_shared_wake(rb,
                                            qemu_ram_block_host_offset(rb,