[Qemu-devel] [PATCH 0/6] multifd: a new mechanism for send thread sync

[Qemu-devel] [PATCH 0/6] multifd: a new mechanism for send thread sync

[Wei Yang]

Current send thread could work while the sync mechanism has some problem:

  * has spuriously wakeup
  * number of channels_ready will *overflow* the number of real channels

The reason is:

  * if MULTIFD_FLAG_SYNC is set in the middle of send thread running, there
    is one more spurious wakeup
  * if MULTIFD_FLAG_SYNC is set when send thread is not running, there is one
    more channels_ready be triggered 

To solve this situation, one new mechanism is introduced to synchronize send
threads. The idea is simple, a new field *sync* is introduced to indicate a
synchronization is required.

Wei Yang (6):
  migration/multifd: move MultiFDSendParams handling into
    multifd_send_fill_packet()
  migration/multifd: notify channels_ready when send thread starts
  migration/multifd: use sync field to synchronize send threads
  migration/multifd: used must not be 0 for a pending job
  migration/multifd: use boolean for pending_job is enough
  migration/multifd: there is no spurious wakeup now

 migration/ram.c | 62 +++++++++++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 23 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH 1/6] migration/multifd: move MultiFDSendParams handling into multifd_send_fill_packet()

[Wei Yang]

Currently there is only one user of multifd_send_fill_packet(). We
enlarge the responsibility of it to adjust MultiFDSendParams.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index bd356764ff..a4e7587648 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -787,9 +787,11 @@ static void multifd_pages_clear(MultiFDPages_t *pages)
 static void multifd_send_fill_packet(MultiFDSendParams *p)
 {
     MultiFDPacket_t *packet = p->packet;
+    MultiFDPages_t *pages = p->pages;
     uint32_t page_max = MULTIFD_PACKET_SIZE / qemu_target_page_size();
     int i;
 
+    p->next_packet_size = pages->used * qemu_target_page_size();
     packet->magic = cpu_to_be32(MULTIFD_MAGIC);
     packet->version = cpu_to_be32(MULTIFD_VERSION);
     packet->flags = cpu_to_be32(p->flags);
@@ -805,6 +807,10 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
     for (i = 0; i < p->pages->used; i++) {
         packet->offset[i] = cpu_to_be64(p->pages->offset[i]);
     }
+    p->flags = 0;
+    p->num_packets++;
+    p->num_pages += pages->used;
+    p->pages->used = 0;
 }
 
 static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
@@ -1097,12 +1103,7 @@ static void *multifd_send_thread(void *opaque)
             uint64_t packet_num = p->packet_num;
             uint32_t flags = p->flags;
 
-            p->next_packet_size = used * qemu_target_page_size();
             multifd_send_fill_packet(p);
-            p->flags = 0;
-            p->num_packets++;
-            p->num_pages += used;
-            p->pages->used = 0;
             qemu_mutex_unlock(&p->mutex);
 
             trace_multifd_send(p->id, packet_num, used, flags,
-- 
2.19.1

[Qemu-devel] [PATCH 2/6] migration/multifd: notify channels_ready when send thread starts

[Wei Yang]

multifd_send_state->channels_ready is initialized to 0. It is proper to
let main thread know we are ready when thread start running.

Current implementation works since ram_save_setup() calls
multifd_send_sync_main() which wake up send thread and posts
channels_ready. This behavior will introduce some unpredictable
situation and disturb the semaphore value.

This is a preparation patch to use another mechanism to do send thread
synchronization to avoid post channels_ready in this case. So this patch
posts channels_ready when send threads start running.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/migration/ram.c b/migration/ram.c
index a4e7587648..f9e53ac413 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1093,6 +1093,8 @@ static void *multifd_send_thread(void *opaque)
     }
     /* initial packet */
     p->num_packets = 1;
+    /* let main thread know we are ready */
+    qemu_sem_post(&multifd_send_state->channels_ready);
 
     while (true) {
         qemu_sem_wait(&p->sem);
-- 
2.19.1

[Qemu-devel] [PATCH 3/6] migration/multifd: use sync field to synchronize send threads

[Wei Yang]

Add a field in MultiFDSendParams to indicate there is a request to
synchronize send threads.

By doing so, send_thread will just post sem_sync on synchronization
request and channels_ready will not *overflow*.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index f9e53ac413..9982930392 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -640,6 +640,8 @@ typedef struct {
     QemuMutex mutex;
     /* is this channel thread running */
     bool running;
+    /* should sync this channel */
+    bool sync;
     /* should this thread finish */
     bool quit;
     /* thread has work to do */
@@ -1065,8 +1067,7 @@ static void multifd_send_sync_main(void)
         qemu_mutex_lock(&p->mutex);
 
         p->packet_num = multifd_send_state->packet_num++;
-        p->flags |= MULTIFD_FLAG_SYNC;
-        p->pending_job++;
+        p->sync = true;
         qemu_mutex_unlock(&p->mutex);
         qemu_sem_post(&p->sem);
     }
@@ -1129,10 +1130,27 @@ static void *multifd_send_thread(void *opaque)
             p->pending_job--;
             qemu_mutex_unlock(&p->mutex);
 
-            if (flags & MULTIFD_FLAG_SYNC) {
-                qemu_sem_post(&multifd_send_state->sem_sync);
-            }
             qemu_sem_post(&multifd_send_state->channels_ready);
+        } else if (p->sync) {
+            uint64_t packet_num = p->packet_num;
+            uint32_t flags = p->flags;
+            assert(!p->pages->used);
+
+            p->flags |= MULTIFD_FLAG_SYNC;
+            multifd_send_fill_packet(p);
+            p->sync = false;
+            qemu_mutex_unlock(&p->mutex);
+
+            trace_multifd_send(p->id, packet_num, 0, flags | MULTIFD_FLAG_SYNC,
+                               p->next_packet_size);
+
+            ret = qio_channel_write_all(p->c, (void *)p->packet,
+                                        p->packet_len, &local_err);
+            if (ret != 0) {
+                break;
+            }
+
+            qemu_sem_post(&multifd_send_state->sem_sync);
         } else if (p->quit) {
             qemu_mutex_unlock(&p->mutex);
             break;
@@ -1196,7 +1214,7 @@ int multifd_save_setup(void)
 
         qemu_mutex_init(&p->mutex);
         qemu_sem_init(&p->sem, 0);
-        p->quit = false;
+        p->sync = p->quit = false;
         p->pending_job = 0;
         p->id = i;
         p->pages = multifd_pages_init(page_count);
-- 
2.19.1

[Qemu-devel] [PATCH 4/6] migration/multifd: used must not be 0 for a pending job

[Wei Yang]

After thread synchronization request is handled in another case, this
means when we only get pending_job when there is used pages.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 9982930392..3e48795608 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1118,12 +1118,11 @@ static void *multifd_send_thread(void *opaque)
                 break;
             }
 
-            if (used) {
-                ret = qio_channel_writev_all(p->c, p->pages->iov,
+            assert(used);
+            ret = qio_channel_writev_all(p->c, p->pages->iov,
                                              used, &local_err);
-                if (ret != 0) {
-                    break;
-                }
+            if (ret != 0) {
+                break;
             }
 
             qemu_mutex_lock(&p->mutex);
-- 
2.19.1

[Qemu-devel] [PATCH 5/6] migration/multifd: use boolean for pending_job is enough

[Wei Yang]

After synchronization request is handled in another case, there only
could be one pending_job for one send thread at most.

This is fine to use boolean to represent this behavior.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 3e48795608..831b15833b 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -645,7 +645,7 @@ typedef struct {
     /* should this thread finish */
     bool quit;
     /* thread has work to do */
-    int pending_job;
+    bool pending_job;
     /* array of pages to sent */
     MultiFDPages_t *pages;
     /* packet allocated len */
@@ -942,7 +942,7 @@ static void multifd_send_pages(void)
 
         qemu_mutex_lock(&p->mutex);
         if (!p->pending_job) {
-            p->pending_job++;
+            p->pending_job = true;
             next_channel = (i + 1) % migrate_multifd_channels();
             break;
         }
@@ -1126,7 +1126,7 @@ static void *multifd_send_thread(void *opaque)
             }
 
             qemu_mutex_lock(&p->mutex);
-            p->pending_job--;
+            p->pending_job = false;
             qemu_mutex_unlock(&p->mutex);
 
             qemu_sem_post(&multifd_send_state->channels_ready);
@@ -1213,8 +1213,7 @@ int multifd_save_setup(void)
 
         qemu_mutex_init(&p->mutex);
         qemu_sem_init(&p->sem, 0);
-        p->sync = p->quit = false;
-        p->pending_job = 0;
+        p->sync = p->quit = p->pending_job = false;
         p->id = i;
         p->pages = multifd_pages_init(page_count);
         p->packet_len = sizeof(MultiFDPacket_t)
-- 
2.19.1

[Qemu-devel] [PATCH 6/6] migration/multifd: there is no spurious wakeup now

[Wei Yang]

The spurious wakeup is gone.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 831b15833b..2490631d52 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1153,9 +1153,6 @@ static void *multifd_send_thread(void *opaque)
         } else if (p->quit) {
             qemu_mutex_unlock(&p->mutex);
             break;
-        } else {
-            qemu_mutex_unlock(&p->mutex);
-            /* sometimes there are spurious wakeups */
         }
     }
 
-- 
2.19.1