[Qemu-devel] [RFC 01/13] postcopy/migration: Split fault related state into struct UserfaultState

[zhanghailiang]

Split fault related state from MigrationIncomingState struct, and put
them all into a new struct UserfaultState. We will add this state into
struct MigrationState in later patch.

We also fix some helper functions to use the new type.

Signed-off-by: zhanghailiang <address@hidden>
---
 include/migration/migration.h    | 20 ++++----
 include/migration/postcopy-ram.h |  2 +-
 include/qemu/typedefs.h          |  1 +
 migration/postcopy-ram.c         | 99 +++++++++++++++++++++++-----------------
 migration/savevm.c               |  2 +-
 5 files changed, 72 insertions(+), 52 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index d9494b8..4c80939 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -79,6 +79,16 @@ typedef enum {
     POSTCOPY_INCOMING_END
 } PostcopyState;
 
+struct UserfaultState {
+    bool           have_fault_thread;
+    QemuThread     fault_thread;
+    QemuSemaphore  fault_thread_sem;
+    /* For the kernel to send us notifications */
+    int       userfault_fd;
+    /* To tell the fault_thread to quit */
+    int       userfault_quit_fd;
+};
+
 /* State for the incoming migration */
 struct MigrationIncomingState {
     QEMUFile *from_src_file;
@@ -89,22 +99,16 @@ struct MigrationIncomingState {
      */
     QemuEvent main_thread_load_event;
 
-    bool           have_fault_thread;
-    QemuThread     fault_thread;
-    QemuSemaphore  fault_thread_sem;
-
     bool           have_listen_thread;
     QemuThread     listen_thread;
     QemuSemaphore  listen_thread_sem;
 
-    /* For the kernel to send us notifications */
-    int       userfault_fd;
-    /* To tell the fault_thread to quit */
-    int       userfault_quit_fd;
     QEMUFile *to_src_file;
     QemuMutex rp_mutex;    /* We send replies from multiple threads */
     void     *postcopy_tmp_page;
 
+    UserfaultState userfault_state;
+
     /* See savevm.c */
     LoadStateEntry_Head loadvm_handlers;
 };
diff --git a/include/migration/postcopy-ram.h b/include/migration/postcopy-ram.h
index b6a7491..e30978f 100644
--- a/include/migration/postcopy-ram.h
+++ b/include/migration/postcopy-ram.h
@@ -20,7 +20,7 @@ bool postcopy_ram_supported_by_host(void);
  * Make all of RAM sensitive to accesses to areas that haven't yet been written
  * and wire up anything necessary to deal with it.
  */
-int postcopy_ram_enable_notify(MigrationIncomingState *mis);
+int postcopy_ram_enable_notify(UserfaultState *us);
 
 /*
  * Initialise postcopy-ram, setting the RAM to a state where we can go into
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 78fe6e8..eda3063 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -41,6 +41,7 @@ typedef struct MemoryListener MemoryListener;
 typedef struct MemoryMappingList MemoryMappingList;
 typedef struct MemoryRegion MemoryRegion;
 typedef struct MemoryRegionSection MemoryRegionSection;
+typedef struct UserfaultState UserfaultState;
 typedef struct MigrationIncomingState MigrationIncomingState;
 typedef struct MigrationParams MigrationParams;
 typedef struct MigrationState MigrationState;
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 3946aa9..38245d4 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -233,7 +233,7 @@ static int init_range(const char *block_name, void 
*host_addr,
 static int cleanup_range(const char *block_name, void *host_addr,
                         ram_addr_t offset, ram_addr_t length, void *opaque)
 {
-    MigrationIncomingState *mis = opaque;
+    UserfaultState *us = opaque;
     struct uffdio_range range_struct;
     trace_postcopy_cleanup_range(block_name, host_addr, offset, length);
 
@@ -251,7 +251,7 @@ static int cleanup_range(const char *block_name, void 
*host_addr,
     range_struct.start = (uintptr_t)host_addr;
     range_struct.len = length;
 
-    if (ioctl(mis->userfault_fd, UFFDIO_UNREGISTER, &range_struct)) {
+    if (ioctl(us->userfault_fd, UFFDIO_UNREGISTER, &range_struct)) {
         error_report("%s: userfault unregister %s", __func__, strerror(errno));
 
         return -1;
@@ -274,36 +274,47 @@ int postcopy_ram_incoming_init(MigrationIncomingState 
*mis, size_t ram_pages)
     return 0;
 }
 
-/*
- * At the end of a migration where postcopy_ram_incoming_init was called.
- */
-int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
+static int postcopy_ram_disable_notify(UserfaultState *us)
 {
-    trace_postcopy_ram_incoming_cleanup_entry();
-
-    if (mis->have_fault_thread) {
+    if (us->have_fault_thread) {
         uint64_t tmp64;
 
-        if (qemu_ram_foreach_block(cleanup_range, mis)) {
+        if (qemu_ram_foreach_block(cleanup_range, us)) {
             return -1;
         }
+
         /*
-         * Tell the fault_thread to exit, it's an eventfd that should
-         * currently be at 0, we're going to increment it to 1
-         */
+        * Tell the fault_thread to exit, it's an eventfd that should
+        * currently be at 0, we're going to increment it to 1
+        */
         tmp64 = 1;
-        if (write(mis->userfault_quit_fd, &tmp64, 8) == 8) {
+
+        if (write(us->userfault_quit_fd, &tmp64, 8) == 8) {
             trace_postcopy_ram_incoming_cleanup_join();
-            qemu_thread_join(&mis->fault_thread);
+            qemu_thread_join(&us->fault_thread);
         } else {
             /* Not much we can do here, but may as well report it */
             error_report("%s: incrementing userfault_quit_fd: %s", __func__,
                          strerror(errno));
         }
+
         trace_postcopy_ram_incoming_cleanup_closeuf();
-        close(mis->userfault_fd);
-        close(mis->userfault_quit_fd);
-        mis->have_fault_thread = false;
+        close(us->userfault_fd);
+        close(us->userfault_quit_fd);
+        us->have_fault_thread = false;
+    }
+    return 0;
+}
+
+/*
+ * At the end of a migration where postcopy_ram_incoming_init was called.
+ */
+int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
+{
+    trace_postcopy_ram_incoming_cleanup_entry();
+
+    if (postcopy_ram_disable_notify(&mis->userfault_state) < 0) {
+        return 0;
     }
 
     qemu_balloon_inhibit(false);
@@ -376,7 +387,7 @@ static int ram_block_enable_notify(const char *block_name, 
void *host_addr,
                                    ram_addr_t offset, ram_addr_t length,
                                    void *opaque)
 {
-    MigrationIncomingState *mis = opaque;
+    UserfaultState *us = opaque;
     struct uffdio_register reg_struct;
 
     reg_struct.range.start = (uintptr_t)host_addr;
@@ -384,7 +395,7 @@ static int ram_block_enable_notify(const char *block_name, 
void *host_addr,
     reg_struct.mode = UFFDIO_REGISTER_MODE_MISSING;
 
     /* Now tell our userfault_fd that it's responsible for this area */
-    if (ioctl(mis->userfault_fd, UFFDIO_REGISTER, &reg_struct)) {
+    if (ioctl(us->userfault_fd, UFFDIO_REGISTER, &reg_struct)) {
         error_report("%s userfault register: %s", __func__, strerror(errno));
         return -1;
     }
@@ -397,15 +408,17 @@ static int ram_block_enable_notify(const char 
*block_name, void *host_addr,
  */
 static void *postcopy_ram_fault_thread(void *opaque)
 {
-    MigrationIncomingState *mis = opaque;
+    UserfaultState *us = opaque;
     struct uffd_msg msg;
     int ret;
     size_t hostpagesize = getpagesize();
     RAMBlock *rb = NULL;
     RAMBlock *last_rb = NULL; /* last RAMBlock we sent part of */
+    MigrationIncomingState *mis = container_of(us, MigrationIncomingState,
+                                               userfault_state);
 
     trace_postcopy_ram_fault_thread_entry();
-    qemu_sem_post(&mis->fault_thread_sem);
+    qemu_sem_post(&us->fault_thread_sem);
 
     while (true) {
         ram_addr_t rb_offset;
@@ -417,10 +430,10 @@ static void *postcopy_ram_fault_thread(void *opaque)
          * however we can be told to quit via userfault_quit_fd which is
          * an eventfd
          */
-        pfd[0].fd = mis->userfault_fd;
+        pfd[0].fd = us->userfault_fd;
         pfd[0].events = POLLIN;
         pfd[0].revents = 0;
-        pfd[1].fd = mis->userfault_quit_fd;
+        pfd[1].fd = us->userfault_quit_fd;
         pfd[1].events = POLLIN; /* Waiting for eventfd to go positive */
         pfd[1].revents = 0;
 
@@ -434,7 +447,8 @@ static void *postcopy_ram_fault_thread(void *opaque)
             break;
         }
 
-        ret = read(mis->userfault_fd, &msg, sizeof(msg));
+        ret = read(us->userfault_fd, &msg, sizeof(msg));
+
         if (ret != sizeof(msg)) {
             if (errno == EAGAIN) {
                 /*
@@ -491,11 +505,11 @@ static void *postcopy_ram_fault_thread(void *opaque)
     return NULL;
 }
 
-int postcopy_ram_enable_notify(MigrationIncomingState *mis)
+int postcopy_ram_enable_notify(UserfaultState *us)
 {
     /* Open the fd for the kernel to give us userfaults */
-    mis->userfault_fd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
-    if (mis->userfault_fd == -1) {
+    us->userfault_fd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
+    if (us->userfault_fd == -1) {
         error_report("%s: Failed to open userfault fd: %s", __func__,
                      strerror(errno));
         return -1;
@@ -505,28 +519,28 @@ int postcopy_ram_enable_notify(MigrationIncomingState 
*mis)
      * Although the host check already tested the API, we need to
      * do the check again as an ABI handshake on the new fd.
      */
-    if (!ufd_version_check(mis->userfault_fd)) {
+    if (!ufd_version_check(us->userfault_fd)) {
         return -1;
     }
 
     /* Now an eventfd we use to tell the fault-thread to quit */
-    mis->userfault_quit_fd = eventfd(0, EFD_CLOEXEC);
-    if (mis->userfault_quit_fd == -1) {
+    us->userfault_quit_fd = eventfd(0, EFD_CLOEXEC);
+    if (us->userfault_quit_fd == -1) {
         error_report("%s: Opening userfault_quit_fd: %s", __func__,
                      strerror(errno));
-        close(mis->userfault_fd);
+        close(us->userfault_fd);
         return -1;
     }
 
-    qemu_sem_init(&mis->fault_thread_sem, 0);
-    qemu_thread_create(&mis->fault_thread, "postcopy/fault",
-                       postcopy_ram_fault_thread, mis, QEMU_THREAD_JOINABLE);
-    qemu_sem_wait(&mis->fault_thread_sem);
-    qemu_sem_destroy(&mis->fault_thread_sem);
-    mis->have_fault_thread = true;
+    qemu_sem_init(&us->fault_thread_sem, 0);
+    qemu_thread_create(&us->fault_thread, "postcopy/fault",
+                       postcopy_ram_fault_thread, us, QEMU_THREAD_JOINABLE);
+    qemu_sem_wait(&us->fault_thread_sem);
+    qemu_sem_destroy(&us->fault_thread_sem);
+    us->have_fault_thread = true;
 
     /* Mark so that we get notified of accesses to unwritten areas */
-    if (qemu_ram_foreach_block(ram_block_enable_notify, mis)) {
+    if (qemu_ram_foreach_block(ram_block_enable_notify, us)) {
         return -1;
     }
 
@@ -559,7 +573,7 @@ int postcopy_place_page(MigrationIncomingState *mis, void 
*host, void *from)
      * 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 (ioctl(mis->userfault_state.userfault_fd, UFFDIO_COPY, &copy_struct)) {
         int e = errno;
         error_report("%s: %s copy host: %p from: %p",
                      __func__, strerror(e), host, from);
@@ -583,7 +597,8 @@ int postcopy_place_page_zero(MigrationIncomingState *mis, 
void *host)
     zero_struct.range.len = getpagesize();
     zero_struct.mode = 0;
 
-    if (ioctl(mis->userfault_fd, UFFDIO_ZEROPAGE, &zero_struct)) {
+    if (ioctl(mis->userfault_state.userfault_fd, UFFDIO_ZEROPAGE,
+              &zero_struct)) {
         int e = errno;
         error_report("%s: %s zero host: %p",
                      __func__, strerror(e), host);
@@ -651,7 +666,7 @@ int postcopy_ram_prepare_discard(MigrationIncomingState 
*mis)
     return -1;
 }
 
-int postcopy_ram_enable_notify(MigrationIncomingState *mis)
+int postcopy_ram_enable_notify(UserfaultState *us, int mode)
 {
     assert(0);
     return -1;
diff --git a/migration/savevm.c b/migration/savevm.c
index 0ad1b93..9b22498 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1467,7 +1467,7 @@ static int 
loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
      * However, at this point the CPU shouldn't be running, and the IO
      * shouldn't be doing anything yet so don't actually expect requests
      */
-    if (postcopy_ram_enable_notify(mis)) {
+    if (postcopy_ram_enable_notify(&mis->userfault_state)) {
         return -1;
     }
 
-- 
1.8.3.1