diff mbox series

[v4,06/10] Refactor out libvhost-user fault generation logic

Message ID 1588533678-23450-7-git-send-email-raphael.norwitz@nutanix.com (mailing list archive)
State New, archived
Headers show
Series vhost-user: Lift Max Ram Slots Limitation | expand

Commit Message

Raphael Norwitz May 21, 2020, 5 a.m. UTC 
In libvhost-user, the incoming postcopy migration path for setting the
backend's memory tables has become convolued. In particular, moving the
logic which starts generating faults, having received the final ACK from
qemu can be moved to a separate function. This simplifies the code
substantially.

This logic will also be needed by the postcopy path once the
VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS feature is supported.

Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
---
 contrib/libvhost-user/libvhost-user.c | 147 ++++++++++++++++++----------------
 1 file changed, 79 insertions(+), 68 deletions(-)

Comments

Marc-André Lureau June 4, 2020, 2:48 p.m. UTC | #1 
Hi

On Thu, May 21, 2020 at 7:00 AM Raphael Norwitz <raphael.norwitz@nutanix.com>
wrote:

> In libvhost-user, the incoming postcopy migration path for setting the
> backend's memory tables has become convolued. In particular, moving the
> logic which starts generating faults, having received the final ACK from
> qemu can be moved to a separate function. This simplifies the code
> substantially.
>
> This logic will also be needed by the postcopy path once the
> VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS feature is supported.
>
> Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
>  contrib/libvhost-user/libvhost-user.c | 147
> ++++++++++++++++++----------------
>  1 file changed, 79 insertions(+), 68 deletions(-)
>
> diff --git a/contrib/libvhost-user/libvhost-user.c
> b/contrib/libvhost-user/libvhost-user.c
> index 3bca996..cccfa22 100644
> --- a/contrib/libvhost-user/libvhost-user.c
> +++ b/contrib/libvhost-user/libvhost-user.c
> @@ -584,6 +584,84 @@ map_ring(VuDev *dev, VuVirtq *vq)
>  }
>
>  static bool
> +generate_faults(VuDev *dev) {
> +    int i;
> +    for (i = 0; i < dev->nregions; i++) {
> +        VuDevRegion *dev_region = &dev->regions[i];
> +        int ret;
> +#ifdef UFFDIO_REGISTER
> +        /*
> +         * We should already have an open ufd. Mark each memory
> +         * range as ufd.
> +         * Discard any mapping we have here; note I can't use MADV_REMOVE
> +         * or fallocate to make the hole since I don't want to lose
> +         * data that's already arrived in the shared process.
> +         * TODO: How to do hugepage
> +         */
> +        ret = madvise((void *)(uintptr_t)dev_region->mmap_addr,
> +                      dev_region->size + dev_region->mmap_offset,
> +                      MADV_DONTNEED);
> +        if (ret) {
> +            fprintf(stderr,
> +                    "%s: Failed to madvise(DONTNEED) region %d: %s\n",
> +                    __func__, i, strerror(errno));
> +        }
> +        /*
> +         * Turn off transparent hugepages so we dont get lose wakeups
> +         * in neighbouring pages.
> +         * TODO: Turn this backon later.
> +         */
> +        ret = madvise((void *)(uintptr_t)dev_region->mmap_addr,
> +                      dev_region->size + dev_region->mmap_offset,
> +                      MADV_NOHUGEPAGE);
> +        if (ret) {
> +            /*
> +             * Note: This can happen legally on kernels that are
> configured
> +             * without madvise'able hugepages
> +             */
> +            fprintf(stderr,
> +                    "%s: Failed to madvise(NOHUGEPAGE) region %d: %s\n",
> +                    __func__, i, strerror(errno));
> +        }
> +        struct uffdio_register reg_struct;
> +        reg_struct.range.start = (uintptr_t)dev_region->mmap_addr;
> +        reg_struct.range.len = dev_region->size + dev_region->mmap_offset;
> +        reg_struct.mode = UFFDIO_REGISTER_MODE_MISSING;
> +
> +        if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, &reg_struct)) {
> +            vu_panic(dev, "%s: Failed to userfault region %d "
> +                          "@%p + size:%zx offset: %zx: (ufd=%d)%s\n",
> +                     __func__, i,
> +                     dev_region->mmap_addr,
> +                     dev_region->size, dev_region->mmap_offset,
> +                     dev->postcopy_ufd, strerror(errno));
> +            return false;
> +        }
> +        if (!(reg_struct.ioctls & ((__u64)1 << _UFFDIO_COPY))) {
> +            vu_panic(dev, "%s Region (%d) doesn't support COPY",
> +                     __func__, i);
> +            return false;
> +        }
> +        DPRINT("%s: region %d: Registered userfault for %"
> +               PRIx64 " + %" PRIx64 "\n", __func__, i,
> +               (uint64_t)reg_struct.range.start,
> +               (uint64_t)reg_struct.range.len);
> +        /* Now it's registered we can let the client at it */
> +        if (mprotect((void *)(uintptr_t)dev_region->mmap_addr,
> +                     dev_region->size + dev_region->mmap_offset,
> +                     PROT_READ | PROT_WRITE)) {
> +            vu_panic(dev, "failed to mprotect region %d for postcopy
> (%s)",
> +                     i, strerror(errno));
> +            return false;
> +        }
> +        /* TODO: Stash 'zero' support flags somewhere */
> +#endif
> +    }
> +
> +    return true;
> +}
> +
> +static bool
>  vu_set_mem_table_exec_postcopy(VuDev *dev, VhostUserMsg *vmsg)
>  {
>      int i;
> @@ -655,74 +733,7 @@ vu_set_mem_table_exec_postcopy(VuDev *dev,
> VhostUserMsg *vmsg)
>      }
>
>      /* OK, now we can go and register the memory and generate faults */
> -    for (i = 0; i < dev->nregions; i++) {
> -        VuDevRegion *dev_region = &dev->regions[i];
> -        int ret;
> -#ifdef UFFDIO_REGISTER
> -        /* We should already have an open ufd. Mark each memory
> -         * range as ufd.
> -         * Discard any mapping we have here; note I can't use MADV_REMOVE
> -         * or fallocate to make the hole since I don't want to lose
> -         * data that's already arrived in the shared process.
> -         * TODO: How to do hugepage
> -         */
> -        ret = madvise((void *)(uintptr_t)dev_region->mmap_addr,
> -                      dev_region->size + dev_region->mmap_offset,
> -                      MADV_DONTNEED);
> -        if (ret) {
> -            fprintf(stderr,
> -                    "%s: Failed to madvise(DONTNEED) region %d: %s\n",
> -                    __func__, i, strerror(errno));
> -        }
> -        /* Turn off transparent hugepages so we dont get lose wakeups
> -         * in neighbouring pages.
> -         * TODO: Turn this backon later.
> -         */
> -        ret = madvise((void *)(uintptr_t)dev_region->mmap_addr,
> -                      dev_region->size + dev_region->mmap_offset,
> -                      MADV_NOHUGEPAGE);
> -        if (ret) {
> -            /* Note: This can happen legally on kernels that are
> configured
> -             * without madvise'able hugepages
> -             */
> -            fprintf(stderr,
> -                    "%s: Failed to madvise(NOHUGEPAGE) region %d: %s\n",
> -                    __func__, i, strerror(errno));
> -        }
> -        struct uffdio_register reg_struct;
> -        reg_struct.range.start = (uintptr_t)dev_region->mmap_addr;
> -        reg_struct.range.len = dev_region->size + dev_region->mmap_offset;
> -        reg_struct.mode = UFFDIO_REGISTER_MODE_MISSING;
> -
> -        if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, &reg_struct)) {
> -            vu_panic(dev, "%s: Failed to userfault region %d "
> -                          "@%p + size:%zx offset: %zx: (ufd=%d)%s\n",
> -                     __func__, i,
> -                     dev_region->mmap_addr,
> -                     dev_region->size, dev_region->mmap_offset,
> -                     dev->postcopy_ufd, strerror(errno));
> -            return false;
> -        }
> -        if (!(reg_struct.ioctls & ((__u64)1 << _UFFDIO_COPY))) {
> -            vu_panic(dev, "%s Region (%d) doesn't support COPY",
> -                     __func__, i);
> -            return false;
> -        }
> -        DPRINT("%s: region %d: Registered userfault for %"
> -               PRIx64 " + %" PRIx64 "\n", __func__, i,
> -               (uint64_t)reg_struct.range.start,
> -               (uint64_t)reg_struct.range.len);
> -        /* Now it's registered we can let the client at it */
> -        if (mprotect((void *)(uintptr_t)dev_region->mmap_addr,
> -                     dev_region->size + dev_region->mmap_offset,
> -                     PROT_READ | PROT_WRITE)) {
> -            vu_panic(dev, "failed to mprotect region %d for postcopy
> (%s)",
> -                     i, strerror(errno));
> -            return false;
> -        }
> -        /* TODO: Stash 'zero' support flags somewhere */
> -#endif
> -    }
> +    (void)generate_faults(dev);
>

>      return false;
>  }
> --
> 1.8.3.1
>
>
diff mbox series

Patch

diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
index 3bca996..cccfa22 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -584,6 +584,84 @@  map_ring(VuDev *dev, VuVirtq *vq)
 }
 
 static bool
+generate_faults(VuDev *dev) {
+    int i;
+    for (i = 0; i < dev->nregions; i++) {
+        VuDevRegion *dev_region = &dev->regions[i];
+        int ret;
+#ifdef UFFDIO_REGISTER
+        /*
+         * We should already have an open ufd. Mark each memory
+         * range as ufd.
+         * Discard any mapping we have here; note I can't use MADV_REMOVE
+         * or fallocate to make the hole since I don't want to lose
+         * data that's already arrived in the shared process.
+         * TODO: How to do hugepage
+         */
+        ret = madvise((void *)(uintptr_t)dev_region->mmap_addr,
+                      dev_region->size + dev_region->mmap_offset,
+                      MADV_DONTNEED);
+        if (ret) {
+            fprintf(stderr,
+                    "%s: Failed to madvise(DONTNEED) region %d: %s\n",
+                    __func__, i, strerror(errno));
+        }
+        /*
+         * Turn off transparent hugepages so we dont get lose wakeups
+         * in neighbouring pages.
+         * TODO: Turn this backon later.
+         */
+        ret = madvise((void *)(uintptr_t)dev_region->mmap_addr,
+                      dev_region->size + dev_region->mmap_offset,
+                      MADV_NOHUGEPAGE);
+        if (ret) {
+            /*
+             * Note: This can happen legally on kernels that are configured
+             * without madvise'able hugepages
+             */
+            fprintf(stderr,
+                    "%s: Failed to madvise(NOHUGEPAGE) region %d: %s\n",
+                    __func__, i, strerror(errno));
+        }
+        struct uffdio_register reg_struct;
+        reg_struct.range.start = (uintptr_t)dev_region->mmap_addr;
+        reg_struct.range.len = dev_region->size + dev_region->mmap_offset;
+        reg_struct.mode = UFFDIO_REGISTER_MODE_MISSING;
+
+        if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, &reg_struct)) {
+            vu_panic(dev, "%s: Failed to userfault region %d "
+                          "@%p + size:%zx offset: %zx: (ufd=%d)%s\n",
+                     __func__, i,
+                     dev_region->mmap_addr,
+                     dev_region->size, dev_region->mmap_offset,
+                     dev->postcopy_ufd, strerror(errno));
+            return false;
+        }
+        if (!(reg_struct.ioctls & ((__u64)1 << _UFFDIO_COPY))) {
+            vu_panic(dev, "%s Region (%d) doesn't support COPY",
+                     __func__, i);
+            return false;
+        }
+        DPRINT("%s: region %d: Registered userfault for %"
+               PRIx64 " + %" PRIx64 "\n", __func__, i,
+               (uint64_t)reg_struct.range.start,
+               (uint64_t)reg_struct.range.len);
+        /* Now it's registered we can let the client at it */
+        if (mprotect((void *)(uintptr_t)dev_region->mmap_addr,
+                     dev_region->size + dev_region->mmap_offset,
+                     PROT_READ | PROT_WRITE)) {
+            vu_panic(dev, "failed to mprotect region %d for postcopy (%s)",
+                     i, strerror(errno));
+            return false;
+        }
+        /* TODO: Stash 'zero' support flags somewhere */
+#endif
+    }
+
+    return true;
+}
+
+static bool
 vu_set_mem_table_exec_postcopy(VuDev *dev, VhostUserMsg *vmsg)
 {
     int i;
@@ -655,74 +733,7 @@  vu_set_mem_table_exec_postcopy(VuDev *dev, VhostUserMsg *vmsg)
     }
 
     /* OK, now we can go and register the memory and generate faults */
-    for (i = 0; i < dev->nregions; i++) {
-        VuDevRegion *dev_region = &dev->regions[i];
-        int ret;
-#ifdef UFFDIO_REGISTER
-        /* We should already have an open ufd. Mark each memory
-         * range as ufd.
-         * Discard any mapping we have here; note I can't use MADV_REMOVE
-         * or fallocate to make the hole since I don't want to lose
-         * data that's already arrived in the shared process.
-         * TODO: How to do hugepage
-         */
-        ret = madvise((void *)(uintptr_t)dev_region->mmap_addr,
-                      dev_region->size + dev_region->mmap_offset,
-                      MADV_DONTNEED);
-        if (ret) {
-            fprintf(stderr,
-                    "%s: Failed to madvise(DONTNEED) region %d: %s\n",
-                    __func__, i, strerror(errno));
-        }
-        /* Turn off transparent hugepages so we dont get lose wakeups
-         * in neighbouring pages.
-         * TODO: Turn this backon later.
-         */
-        ret = madvise((void *)(uintptr_t)dev_region->mmap_addr,
-                      dev_region->size + dev_region->mmap_offset,
-                      MADV_NOHUGEPAGE);
-        if (ret) {
-            /* Note: This can happen legally on kernels that are configured
-             * without madvise'able hugepages
-             */
-            fprintf(stderr,
-                    "%s: Failed to madvise(NOHUGEPAGE) region %d: %s\n",
-                    __func__, i, strerror(errno));
-        }
-        struct uffdio_register reg_struct;
-        reg_struct.range.start = (uintptr_t)dev_region->mmap_addr;
-        reg_struct.range.len = dev_region->size + dev_region->mmap_offset;
-        reg_struct.mode = UFFDIO_REGISTER_MODE_MISSING;
-
-        if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER, &reg_struct)) {
-            vu_panic(dev, "%s: Failed to userfault region %d "
-                          "@%p + size:%zx offset: %zx: (ufd=%d)%s\n",
-                     __func__, i,
-                     dev_region->mmap_addr,
-                     dev_region->size, dev_region->mmap_offset,
-                     dev->postcopy_ufd, strerror(errno));
-            return false;
-        }
-        if (!(reg_struct.ioctls & ((__u64)1 << _UFFDIO_COPY))) {
-            vu_panic(dev, "%s Region (%d) doesn't support COPY",
-                     __func__, i);
-            return false;
-        }
-        DPRINT("%s: region %d: Registered userfault for %"
-               PRIx64 " + %" PRIx64 "\n", __func__, i,
-               (uint64_t)reg_struct.range.start,
-               (uint64_t)reg_struct.range.len);
-        /* Now it's registered we can let the client at it */
-        if (mprotect((void *)(uintptr_t)dev_region->mmap_addr,
-                     dev_region->size + dev_region->mmap_offset,
-                     PROT_READ | PROT_WRITE)) {
-            vu_panic(dev, "failed to mprotect region %d for postcopy (%s)",
-                     i, strerror(errno));
-            return false;
-        }
-        /* TODO: Stash 'zero' support flags somewhere */
-#endif
-    }
+    (void)generate_faults(dev);
 
     return false;
 }