[PATCH 0/8] migration: cleanups, fixes and micro-optimizations

[PATCH 0/8] migration: cleanups, fixes and micro-optimizations

[Bin Guo]

This series collects several small improvements to the migration
subsystem:

- Bug fix: off-by-one in multifd recv channel ID validation that
  could allow an out-of-bounds write (patch 5).
- Micro-optimizations: cache migrate_multifd_channels() in hot
  paths (patches 7-8), use stack-allocated bitmap instead of
  heap (patch 4), avoid per-element heap churn in vmstate ptr
  marker field (patch 3).
- Cleanups: collapse migration_fd_valid into a single boolean
  expression (patch 1), replace strcpy("") with explicit NUL
  termination (patch 2), merge thread-join and cleanup loops in
  multifd_recv_cleanup (patch 6).

No functional change intended except for the off-by-one fix in
patch 5.

Bin Guo (8):
  migration/fd: collapse migration_fd_valid into single boolean
    expression
  migration/global_state: replace strcpy("") with explicit NUL
    termination
  migration/vmstate: avoid per-element heap churn in vmsd ptr marker
    field
  migration/savevm: use stack-allocated bitmap in
    configuration_validate_capabilities
  migration/multifd: fix off-by-one in recv channel ID validation
  migration/multifd: merge thread-join and cleanup loops in
    multifd_recv_cleanup
  migration/multifd: cache migrate_multifd_channels() in send/recv hot
    paths
  migration/multifd: cache channel count in multifd_send_sync_main

 migration/fd.c           | 10 +---------
 migration/global_state.c |  2 +-
 migration/multifd.c      | 32 +++++++++++++++++--------------
 migration/savevm.c       |  5 ++---
 migration/vmstate.c      | 41 ++++++++++++++++------------------------
 5 files changed, 38 insertions(+), 52 deletions(-)

-- 
2.50.1 (Apple Git-155)

[PATCH 1/8] migration/fd: collapse migration_fd_valid into single boolean expression

[Bin Guo]

Two if/return-true blocks followed by a final return false
are equivalent to a single OR-expression and easier to scan.

Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
---
 migration/fd.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/migration/fd.c b/migration/fd.c
index f05f95992f..939cdda4e1 100644
--- a/migration/fd.c
+++ b/migration/fd.c
@@ -39,15 +39,7 @@ static bool fd_is_pipe(int fd)
 
 static bool migration_fd_valid(int fd)
 {
-    if (fd_is_socket(fd)) {
-        return true;
-    }
-
-    if (fd_is_pipe(fd)) {
-        return true;
-    }
-
-    return false;
+    return fd_is_socket(fd) || fd_is_pipe(fd);
 }
 
 QIOChannel *fd_connect_outgoing(MigrationState *s, const char *fdname,
-- 
2.50.1 (Apple Git-155)

Re: [PATCH 1/8] migration/fd: collapse migration_fd_valid into single boolean expression

[Fabiano Rosas]

Bin Guo <guobin@linux.alibaba.com> writes:

> Two if/return-true blocks followed by a final return false
> are equivalent to a single OR-expression and easier to scan.
>
> Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
> ---
>  migration/fd.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/migration/fd.c b/migration/fd.c
> index f05f95992f..939cdda4e1 100644
> --- a/migration/fd.c
> +++ b/migration/fd.c
> @@ -39,15 +39,7 @@ static bool fd_is_pipe(int fd)
>  
>  static bool migration_fd_valid(int fd)
>  {
> -    if (fd_is_socket(fd)) {
> -        return true;
> -    }
> -
> -    if (fd_is_pipe(fd)) {
> -        return true;
> -    }
> -
> -    return false;
> +    return fd_is_socket(fd) || fd_is_pipe(fd);
>  }
>  
>  QIOChannel *fd_connect_outgoing(MigrationState *s, const char *fdname,

Let's not invest effort in replacing perfectly good code.

[PATCH 2/8] migration/global_state: replace strcpy("") with explicit NUL termination

[Bin Guo]

Drop the unnecessary strcpy of an empty literal (and its spurious
(char *)& cast) in favor of a direct NUL store, which avoids the
libc call and hides no bugs behind a cast.

Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
---
 migration/global_state.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/global_state.c b/migration/global_state.c
index c1f90fce0f..91fefdf525 100644
--- a/migration/global_state.c
+++ b/migration/global_state.c
@@ -148,7 +148,7 @@ static const VMStateDescription vmstate_globalstate = {
 void register_global_state(void)
 {
     /* We would use it independently that we receive it */
-    strcpy((char *)&global_state.runstate, "");
+    global_state.runstate[0] = '\0';
     global_state.received = false;
     vmstate_register(NULL, 0, &vmstate_globalstate, &global_state);
 }
-- 
2.50.1 (Apple Git-155)

Re: [PATCH 2/8] migration/global_state: replace strcpy("") with explicit NUL termination

[Fabiano Rosas]

Bin Guo <guobin@linux.alibaba.com> writes:

> Drop the unnecessary strcpy of an empty literal (and its spurious
> (char *)& cast) in favor of a direct NUL store, which avoids the
> libc call and hides no bugs behind a cast.
>
> Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
> ---
>  migration/global_state.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/migration/global_state.c b/migration/global_state.c
> index c1f90fce0f..91fefdf525 100644
> --- a/migration/global_state.c
> +++ b/migration/global_state.c
> @@ -148,7 +148,7 @@ static const VMStateDescription vmstate_globalstate = {
>  void register_global_state(void)
>  {
>      /* We would use it independently that we receive it */
> -    strcpy((char *)&global_state.runstate, "");
> +    global_state.runstate[0] = '\0';
>      global_state.received = false;
>      vmstate_register(NULL, 0, &vmstate_globalstate, &global_state);
>  }

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH 3/8] migration/vmstate: avoid per-element heap churn in vmsd ptr marker field

[Bin Guo]

For every NULL slot in a VMS_ARRAY_OF_POINTER (or every entry of a
dynamic array), the saver allocates a 1-element fake VMStateField via
g_new0 and frees it again right after the save. For arrays of
thousands of entries this is thousands of malloc/free pairs on the
hot save path.

Replace the heap-allocated marker with a stack-resident field
populated by an init helper. The caller passes a pointer to a local
VMStateField, the helper fills it in (still asserting the
precondition), and no g_free is needed.

Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
---
 migration/vmstate.c | 41 ++++++++++++++++-------------------------
 1 file changed, 16 insertions(+), 25 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 2f13b48a37..9ced78532f 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -59,29 +59,23 @@ vmstate_field_exists(const VMStateDescription *vmsd, const VMStateField *field,
  * array of a VMS_ARRAY_OF_POINTER VMSD field.  It's needed because we
  * can't dereference the NULL pointer.
  */
-static const VMStateField *
-vmsd_create_ptr_marker_field(const VMStateField *field)
+static void
+vmsd_init_ptr_marker_field(VMStateField *fake, const VMStateField *field)
 {
-    VMStateField *fake = g_new0(VMStateField, 1);
-
     /* It can only happen on an array of pointers! */
     assert(field->flags & VMS_ARRAY_OF_POINTER);
 
-    /* Some of fake's properties should match the original's */
-    fake->name = field->name;
-    fake->version_id = field->version_id;
-
-    /* Do not need "field_exists" check as it always exists */
-    fake->field_exists = NULL;
-
-    /* See vmstate_info_ptr_marker - use 1 byte to represent ptr status */
-    fake->size = 1;
-    fake->info = &vmstate_info_ptr_marker;
-    fake->flags = VMS_SINGLE;
-
-    /* All the rest fields shouldn't matter.. */
-
-    return (const VMStateField *)fake;
+    /* See vmstate_info_ptr_marker - 1 byte represents ptr status */
+    *fake = (VMStateField) {
+        .name = field->name,
+        .version_id = field->version_id,
+        /* Marker always exists, no field_exists callback needed */
+        .field_exists = NULL,
+        .size = 1,
+        .info = &vmstate_info_ptr_marker,
+        .flags = VMS_SINGLE,
+        /* All other fields stay zero-initialised */
+    };
 }
 
 static int vmstate_n_elems(void *opaque, const VMStateField *field)
@@ -680,6 +674,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
             for (i = 0; i < n_elems; i++) {
                 void *curr_elem = first_elem + size * i;
                 const VMStateField *inner_field;
+                VMStateField marker_field;
                 /* maximum number of elements to compress in the JSON blob */
                 int max_elems = vmsd_can_compress(field) ? (n_elems - i) : 1;
                 bool use_marker_field, is_null = false;
@@ -693,7 +688,8 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
                 use_marker_field = use_dynamic_array || is_null;
 
                 if (use_marker_field) {
-                    inner_field = vmsd_create_ptr_marker_field(field);
+                    vmsd_init_ptr_marker_field(&marker_field, field);
+                    inner_field = &marker_field;
                 } else {
                     inner_field = field;
                 }
@@ -730,11 +726,6 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
                                                     inner_field, vmdesc_loop,
                                                     i, max_elems, errp);
 
-                /* If we used a fake temp field.. free it now */
-                if (use_marker_field) {
-                    g_clear_pointer((gpointer *)&inner_field, g_free);
-                }
-
                 if (!ok) {
                     goto out;
                 }
-- 
2.50.1 (Apple Git-155)

Re: [PATCH 3/8] migration/vmstate: avoid per-element heap churn in vmsd ptr marker field

[Fabiano Rosas]

Bin Guo <guobin@linux.alibaba.com> writes:

> For every NULL slot in a VMS_ARRAY_OF_POINTER (or every entry of a
> dynamic array), the saver allocates a 1-element fake VMStateField via
> g_new0 and frees it again right after the save. For arrays of
> thousands of entries this is thousands of malloc/free pairs on the
> hot save path.
>
> Replace the heap-allocated marker with a stack-resident field
> populated by an init helper. The caller passes a pointer to a local
> VMStateField, the helper fills it in (still asserting the
> precondition), and no g_free is needed.
>
> Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
> ---
>  migration/vmstate.c | 41 ++++++++++++++++-------------------------
>  1 file changed, 16 insertions(+), 25 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 2f13b48a37..9ced78532f 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -59,29 +59,23 @@ vmstate_field_exists(const VMStateDescription *vmsd, const VMStateField *field,
>   * array of a VMS_ARRAY_OF_POINTER VMSD field.  It's needed because we
>   * can't dereference the NULL pointer.
>   */
> -static const VMStateField *
> -vmsd_create_ptr_marker_field(const VMStateField *field)
> +static void
> +vmsd_init_ptr_marker_field(VMStateField *fake, const VMStateField *field)
>  {
> -    VMStateField *fake = g_new0(VMStateField, 1);
> -
>      /* It can only happen on an array of pointers! */
>      assert(field->flags & VMS_ARRAY_OF_POINTER);
>  
> -    /* Some of fake's properties should match the original's */
> -    fake->name = field->name;
> -    fake->version_id = field->version_id;
> -
> -    /* Do not need "field_exists" check as it always exists */
> -    fake->field_exists = NULL;
> -
> -    /* See vmstate_info_ptr_marker - use 1 byte to represent ptr status */
> -    fake->size = 1;
> -    fake->info = &vmstate_info_ptr_marker;
> -    fake->flags = VMS_SINGLE;
> -
> -    /* All the rest fields shouldn't matter.. */
> -
> -    return (const VMStateField *)fake;
> +    /* See vmstate_info_ptr_marker - 1 byte represents ptr status */
> +    *fake = (VMStateField) {
> +        .name = field->name,
> +        .version_id = field->version_id,
> +        /* Marker always exists, no field_exists callback needed */
> +        .field_exists = NULL,
> +        .size = 1,
> +        .info = &vmstate_info_ptr_marker,
> +        .flags = VMS_SINGLE,
> +        /* All other fields stay zero-initialised */
> +    };
>  }
>  
>  static int vmstate_n_elems(void *opaque, const VMStateField *field)
> @@ -680,6 +674,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>              for (i = 0; i < n_elems; i++) {
>                  void *curr_elem = first_elem + size * i;
>                  const VMStateField *inner_field;
> +                VMStateField marker_field;
>                  /* maximum number of elements to compress in the JSON blob */
>                  int max_elems = vmsd_can_compress(field) ? (n_elems - i) : 1;
>                  bool use_marker_field, is_null = false;
> @@ -693,7 +688,8 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>                  use_marker_field = use_dynamic_array || is_null;
>  
>                  if (use_marker_field) {
> -                    inner_field = vmsd_create_ptr_marker_field(field);
> +                    vmsd_init_ptr_marker_field(&marker_field, field);
> +                    inner_field = &marker_field;
>                  } else {
>                      inner_field = field;
>                  }
> @@ -730,11 +726,6 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>                                                      inner_field, vmdesc_loop,
>                                                      i, max_elems, errp);
>  
> -                /* If we used a fake temp field.. free it now */
> -                if (use_marker_field) {
> -                    g_clear_pointer((gpointer *)&inner_field, g_free);
> -                }
> -
>                  if (!ok) {
>                      goto out;
>                  }

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH 4/8] migration/savevm: use stack-allocated bitmap in configuration_validate_capabilities

[Bin Guo]

configuration_validate_capabilities() allocates a bitmap on the heap
to track source capabilities via bitmap_new()/g_free().  Since
MIGRATION_CAPABILITY__MAX is a small compile-time constant (< 64),
a heap allocation for a bitmap this small is wasteful: it adds
malloc/free overhead and a potential cache miss for a transient
8-byte allocation.

Replace with DECLARE_BITMAP() on the stack and bitmap_zero() to
initialize.  This eliminates the heap round-trip entirely.

Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
---
 migration/savevm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index d1dd696c17..23adaf9dd9 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -347,10 +347,10 @@ static bool configuration_validate_capabilities(SaveState *state)
 {
     bool ret = true;
     MigrationState *s = migrate_get_current();
-    unsigned long *source_caps_bm;
+    DECLARE_BITMAP(source_caps_bm, MIGRATION_CAPABILITY__MAX);
     int i;
 
-    source_caps_bm = bitmap_new(MIGRATION_CAPABILITY__MAX);
+    bitmap_zero(source_caps_bm, MIGRATION_CAPABILITY__MAX);
     for (i = 0; i < state->caps_count; i++) {
         MigrationCapability capability = state->capabilities[i];
         set_bit(capability, source_caps_bm);
@@ -373,7 +373,6 @@ static bool configuration_validate_capabilities(SaveState *state)
         }
     }
 
-    g_free(source_caps_bm);
     return ret;
 }
 
-- 
2.50.1 (Apple Git-155)

Re: [PATCH 4/8] migration/savevm: use stack-allocated bitmap in configuration_validate_capabilities

[Fabiano Rosas]

Bin Guo <guobin@linux.alibaba.com> writes:

> configuration_validate_capabilities() allocates a bitmap on the heap
> to track source capabilities via bitmap_new()/g_free().  Since
> MIGRATION_CAPABILITY__MAX is a small compile-time constant (< 64),
> a heap allocation for a bitmap this small is wasteful: it adds
> malloc/free overhead and a potential cache miss for a transient
> 8-byte allocation.
>
> Replace with DECLARE_BITMAP() on the stack and bitmap_zero() to
> initialize.  This eliminates the heap round-trip entirely.
>
> Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
> ---
>  migration/savevm.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index d1dd696c17..23adaf9dd9 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -347,10 +347,10 @@ static bool configuration_validate_capabilities(SaveState *state)
>  {
>      bool ret = true;
>      MigrationState *s = migrate_get_current();
> -    unsigned long *source_caps_bm;
> +    DECLARE_BITMAP(source_caps_bm, MIGRATION_CAPABILITY__MAX);
>      int i;
>  
> -    source_caps_bm = bitmap_new(MIGRATION_CAPABILITY__MAX);
> +    bitmap_zero(source_caps_bm, MIGRATION_CAPABILITY__MAX);
>      for (i = 0; i < state->caps_count; i++) {
>          MigrationCapability capability = state->capabilities[i];
>          set_bit(capability, source_caps_bm);
> @@ -373,7 +373,6 @@ static bool configuration_validate_capabilities(SaveState *state)
>          }
>      }
>  
> -    g_free(source_caps_bm);
>      return ret;
>  }

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH 5/8] migration/multifd: fix off-by-one in recv channel ID validation

[Bin Guo]

multifd_recv_initial_packet() validates the channel ID received from
the source against the configured number of channels. The current
check uses '>' which allows msg.id == N to pass through. This ID is
then used to index multifd_recv_state->params[msg.id], which was
allocated with g_new0(MultiFDRecvParams, N) -- an out-of-bounds
access.

A malicious or buggy source could send id == N and cause heap
corruption on the destination.

Fix by changing '>' to '>='. Also fix the error message to say
"exceeds channel count" for accuracy.

Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
---
 migration/multifd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index 035cb70f7b..b3eef875cc 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -210,9 +210,9 @@ static int multifd_recv_initial_packet(QIOChannel *c, Error **errp)
         return -1;
     }
 
-    if (msg.id > migrate_multifd_channels()) {
-        error_setg(errp, "multifd: received channel id %u is greater than "
-                   "number of channels %u", msg.id, migrate_multifd_channels());
+    if (msg.id >= migrate_multifd_channels()) {
+        error_setg(errp, "multifd: received channel id %u exceeds "
+                   "channel count %u", msg.id, migrate_multifd_channels());
         return -1;
     }
 
-- 
2.50.1 (Apple Git-155)

Re: [PATCH 5/8] migration/multifd: fix off-by-one in recv channel ID validation

[Fabiano Rosas]

Bin Guo <guobin@linux.alibaba.com> writes:

> multifd_recv_initial_packet() validates the channel ID received from
> the source against the configured number of channels. The current
> check uses '>' which allows msg.id == N to pass through. This ID is
> then used to index multifd_recv_state->params[msg.id], which was
> allocated with g_new0(MultiFDRecvParams, N) -- an out-of-bounds
> access.
>
> A malicious or buggy source could send id == N and cause heap
> corruption on the destination.
>
> Fix by changing '>' to '>='. Also fix the error message to say
> "exceeds channel count" for accuracy.
>
> Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
> ---
>  migration/multifd.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 035cb70f7b..b3eef875cc 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -210,9 +210,9 @@ static int multifd_recv_initial_packet(QIOChannel *c, Error **errp)
>          return -1;
>      }
>  
> -    if (msg.id > migrate_multifd_channels()) {
> -        error_setg(errp, "multifd: received channel id %u is greater than "
> -                   "number of channels %u", msg.id, migrate_multifd_channels());
> +    if (msg.id >= migrate_multifd_channels()) {
> +        error_setg(errp, "multifd: received channel id %u exceeds "
> +                   "channel count %u", msg.id, migrate_multifd_channels());
>          return -1;
>      }

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH 6/8] migration/multifd: merge thread-join and cleanup loops in multifd_recv_cleanup

[Bin Guo]

multifd_recv_cleanup() iterates over all receive channels twice:
one loop to join threads and another to clean up each channel.  Unlike
the send side where all threads must be signalled before any is joined
(to avoid deadlock), on the receive side the threads are already
terminated by multifd_recv_terminate_threads().  Each join simply
waits for an already-terminated thread, so the join and cleanup for
channel i are independent of channel j, and the two loops can safely
be merged into one.

This cuts the iteration count in half and improves locality: the
thread's resources are freed immediately after its join.

Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
---
 migration/multifd.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index b3eef875cc..67ee9bdf5e 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -1153,15 +1153,14 @@ void multifd_recv_cleanup(void)
         return;
     }
     multifd_recv_terminate_threads(NULL);
+
     for (i = 0; i < migrate_multifd_channels(); i++) {
         MultiFDRecvParams *p = &multifd_recv_state->params[i];
 
         if (p->thread_created) {
             qemu_thread_join(&p->thread);
         }
-    }
-    for (i = 0; i < migrate_multifd_channels(); i++) {
-        multifd_recv_cleanup_channel(&multifd_recv_state->params[i]);
+        multifd_recv_cleanup_channel(p);
     }
     multifd_recv_cleanup_state();
 }
-- 
2.50.1 (Apple Git-155)

Re: [PATCH 6/8] migration/multifd: merge thread-join and cleanup loops in multifd_recv_cleanup

[Fabiano Rosas]

Bin Guo <guobin@linux.alibaba.com> writes:

> multifd_recv_cleanup() iterates over all receive channels twice:
> one loop to join threads and another to clean up each channel.

ok

> Unlike the send side where all threads must be signalled before any is
> joined (to avoid deadlock),

Not super relevant what the send side does. It's just a similar piece of
code, that's all.

> on the receive side the threads are already
> terminated by multifd_recv_terminate_threads().  Each join simply
> waits for an already-terminated thread, so the join and cleanup for
> channel i are independent of channel j, and the two loops can safely
> be merged into one.
>

We do really need to join all threads before proceeding. There's no
guarantee on the the order in which they'll terminate.

> This cuts the iteration count in half and improves locality: the
> thread's resources are freed immediately after its join.
>
> Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
> ---
>  migration/multifd.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index b3eef875cc..67ee9bdf5e 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -1153,15 +1153,14 @@ void multifd_recv_cleanup(void)
>          return;
>      }
>      multifd_recv_terminate_threads(NULL);
> +
>      for (i = 0; i < migrate_multifd_channels(); i++) {
>          MultiFDRecvParams *p = &multifd_recv_state->params[i];
>  
>          if (p->thread_created) {
>              qemu_thread_join(&p->thread);
>          }
> -    }
> -    for (i = 0; i < migrate_multifd_channels(); i++) {
> -        multifd_recv_cleanup_channel(&multifd_recv_state->params[i]);
> +        multifd_recv_cleanup_channel(p);
>      }
>      multifd_recv_cleanup_state();
>  }

[PATCH 7/8] migration/multifd: cache migrate_multifd_channels() in send/recv hot paths

[Bin Guo]

multifd_send() and multifd_recv() are on the per-page-batch hot path
of live migration.  Both functions call migrate_multifd_channels()
multiple times (3-4 calls each) for modulo arithmetic in the
round-robin channel selection loop.

Each call goes through migrate_get_current() -> dereference
MigrationState -> read parameters.multifd_channels.  While each
individual call is cheap, these functions execute for every page
batch during the entire migration, easily millions of times.

Cache the return value in a local variable at function entry.  The
channel count is fixed for the duration of a migration and cannot
change mid-flight.

For multifd_send(): 3 calls reduced to 1.
For multifd_recv(): 4 calls reduced to 1.

Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
---
 migration/multifd.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index 67ee9bdf5e..cc2fa90204 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -362,13 +362,15 @@ bool multifd_send(MultiFDSendData **send_data)
     /* We wait here, until at least one channel is ready */
     qemu_sem_wait(&multifd_send_state->channels_ready);
 
+    int thread_count = migrate_multifd_channels();
+
     /*
      * next_channel can remain from a previous migration that was
      * using more channels, so ensure it doesn't overflow if the
      * limit is lower now.
      */
-    next_channel %= migrate_multifd_channels();
-    for (i = next_channel;; i = (i + 1) % migrate_multifd_channels()) {
+    next_channel %= thread_count;
+    for (i = next_channel;; i = (i + 1) % thread_count) {
         if (multifd_send_should_exit()) {
             return false;
         }
@@ -378,7 +380,7 @@ bool multifd_send(MultiFDSendData **send_data)
          * sender thread can clear it.
          */
         if (qatomic_read(&p->pending_job) == false) {
-            next_channel = (i + 1) % migrate_multifd_channels();
+            next_channel = (i + 1) % thread_count;
             break;
         }
     }
@@ -998,6 +1000,7 @@ bool multifd_recv(void)
     int i;
     static int next_recv_channel;
     MultiFDRecvParams *p = NULL;
+    int thread_count = migrate_multifd_channels();
     MultiFDRecvData *data = multifd_recv_state->data;
 
     /*
@@ -1005,8 +1008,8 @@ bool multifd_recv(void)
      * using more channels, so ensure it doesn't overflow if the
      * limit is lower now.
      */
-    next_recv_channel %= migrate_multifd_channels();
-    for (i = next_recv_channel;; i = (i + 1) % migrate_multifd_channels()) {
+    next_recv_channel %= thread_count;
+    for (i = next_recv_channel;; i = (i + 1) % thread_count) {
         if (multifd_recv_should_exit()) {
             return false;
         }
@@ -1014,7 +1017,7 @@ bool multifd_recv(void)
         p = &multifd_recv_state->params[i];
 
         if (qatomic_read(&p->pending_job) == false) {
-            next_recv_channel = (i + 1) % migrate_multifd_channels();
+            next_recv_channel = (i + 1) % thread_count;
             break;
         }
     }
-- 
2.50.1 (Apple Git-155)

Re: [PATCH 7/8] migration/multifd: cache migrate_multifd_channels() in send/recv hot paths

[Fabiano Rosas]

Bin Guo <guobin@linux.alibaba.com> writes:

> multifd_send() and multifd_recv() are on the per-page-batch hot path
> of live migration.  Both functions call migrate_multifd_channels()
> multiple times (3-4 calls each) for modulo arithmetic in the
> round-robin channel selection loop.
>
> Each call goes through migrate_get_current() -> dereference
> MigrationState -> read parameters.multifd_channels.  While each
> individual call is cheap, these functions execute for every page
> batch during the entire migration, easily millions of times.
>
> Cache the return value in a local variable at function entry.  The
> channel count is fixed for the duration of a migration and cannot
> change mid-flight.
>
> For multifd_send(): 3 calls reduced to 1.
> For multifd_recv(): 4 calls reduced to 1.
>
> Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
> ---
>  migration/multifd.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 67ee9bdf5e..cc2fa90204 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -362,13 +362,15 @@ bool multifd_send(MultiFDSendData **send_data)
>      /* We wait here, until at least one channel is ready */
>      qemu_sem_wait(&multifd_send_state->channels_ready);
>  
> +    int thread_count = migrate_multifd_channels();
> +
>      /*
>       * next_channel can remain from a previous migration that was
>       * using more channels, so ensure it doesn't overflow if the
>       * limit is lower now.
>       */
> -    next_channel %= migrate_multifd_channels();
> -    for (i = next_channel;; i = (i + 1) % migrate_multifd_channels()) {
> +    next_channel %= thread_count;
> +    for (i = next_channel;; i = (i + 1) % thread_count) {
>          if (multifd_send_should_exit()) {
>              return false;
>          }
> @@ -378,7 +380,7 @@ bool multifd_send(MultiFDSendData **send_data)
>           * sender thread can clear it.
>           */
>          if (qatomic_read(&p->pending_job) == false) {
> -            next_channel = (i + 1) % migrate_multifd_channels();
> +            next_channel = (i + 1) % thread_count;
>              break;
>          }
>      }
> @@ -998,6 +1000,7 @@ bool multifd_recv(void)
>      int i;
>      static int next_recv_channel;
>      MultiFDRecvParams *p = NULL;
> +    int thread_count = migrate_multifd_channels();
>      MultiFDRecvData *data = multifd_recv_state->data;
>  
>      /*
> @@ -1005,8 +1008,8 @@ bool multifd_recv(void)
>       * using more channels, so ensure it doesn't overflow if the
>       * limit is lower now.
>       */
> -    next_recv_channel %= migrate_multifd_channels();
> -    for (i = next_recv_channel;; i = (i + 1) % migrate_multifd_channels()) {
> +    next_recv_channel %= thread_count;
> +    for (i = next_recv_channel;; i = (i + 1) % thread_count) {
>          if (multifd_recv_should_exit()) {
>              return false;
>          }
> @@ -1014,7 +1017,7 @@ bool multifd_recv(void)
>          p = &multifd_recv_state->params[i];
>  
>          if (qatomic_read(&p->pending_job) == false) {
> -            next_recv_channel = (i + 1) % migrate_multifd_channels();
> +            next_recv_channel = (i + 1) % thread_count;
>              break;
>          }
>      }

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH 8/8] migration/multifd: cache channel count in multifd_send_sync_main

[Bin Guo]

multifd_send_sync_main() is called once per RAM synchronization round
during live migration.  It iterates over all multifd channels twice
(signal loop + wait loop), calling migrate_multifd_channels()
independently in each loop header.

Cache migrate_multifd_channels() in a local thread_count variable at
function entry, matching the pattern already used in
multifd_send_setup() and multifd_recv_setup().  This eliminates 2
redundant config lookups per sync call.

Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
---
 migration/multifd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index cc2fa90204..bc8aacd660 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -611,13 +611,15 @@ static int multifd_zero_copy_flush(QIOChannel *c)
 int multifd_send_sync_main(MultiFDSyncReq req)
 {
     int i;
+    int thread_count;
     bool flush_zero_copy;
 
     assert(req != MULTIFD_SYNC_NONE);
 
+    thread_count = migrate_multifd_channels();
     flush_zero_copy = migrate_zero_copy_send();
 
-    for (i = 0; i < migrate_multifd_channels(); i++) {
+    for (i = 0; i < thread_count; i++) {
         MultiFDSendParams *p = &multifd_send_state->params[i];
 
         if (multifd_send_should_exit()) {
@@ -634,7 +636,7 @@ int multifd_send_sync_main(MultiFDSyncReq req)
         qatomic_set(&p->pending_sync, req);
         qemu_sem_post(&p->sem);
     }
-    for (i = 0; i < migrate_multifd_channels(); i++) {
+    for (i = 0; i < thread_count; i++) {
         MultiFDSendParams *p = &multifd_send_state->params[i];
 
         if (multifd_send_should_exit()) {
-- 
2.50.1 (Apple Git-155)

Re: [PATCH 8/8] migration/multifd: cache channel count in multifd_send_sync_main

[Fabiano Rosas]

Bin Guo <guobin@linux.alibaba.com> writes:

> multifd_send_sync_main() is called once per RAM synchronization round
> during live migration.  It iterates over all multifd channels twice
> (signal loop + wait loop), calling migrate_multifd_channels()
> independently in each loop header.
>
> Cache migrate_multifd_channels() in a local thread_count variable at
> function entry, matching the pattern already used in
> multifd_send_setup() and multifd_recv_setup().  This eliminates 2
> redundant config lookups per sync call.
>
> Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
> ---
>  migration/multifd.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index cc2fa90204..bc8aacd660 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -611,13 +611,15 @@ static int multifd_zero_copy_flush(QIOChannel *c)
>  int multifd_send_sync_main(MultiFDSyncReq req)
>  {
>      int i;
> +    int thread_count;
>      bool flush_zero_copy;
>  
>      assert(req != MULTIFD_SYNC_NONE);
>  
> +    thread_count = migrate_multifd_channels();
>      flush_zero_copy = migrate_zero_copy_send();
>  
> -    for (i = 0; i < migrate_multifd_channels(); i++) {
> +    for (i = 0; i < thread_count; i++) {
>          MultiFDSendParams *p = &multifd_send_state->params[i];
>  
>          if (multifd_send_should_exit()) {
> @@ -634,7 +636,7 @@ int multifd_send_sync_main(MultiFDSyncReq req)
>          qatomic_set(&p->pending_sync, req);
>          qemu_sem_post(&p->sem);
>      }
> -    for (i = 0; i < migrate_multifd_channels(); i++) {
> +    for (i = 0; i < thread_count; i++) {
>          MultiFDSendParams *p = &multifd_send_state->params[i];
>  
>          if (multifd_send_should_exit()) {

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 0/8] migration: cleanups, fixes and micro-optimizations

[Peter Xu]

On Mon, May 18, 2026 at 07:01:04PM +0800, Bin Guo wrote:
> This series collects several small improvements to the migration
> subsystem:
> 
> - Bug fix: off-by-one in multifd recv channel ID validation that
>   could allow an out-of-bounds write (patch 5).
> - Micro-optimizations: cache migrate_multifd_channels() in hot
>   paths (patches 7-8), use stack-allocated bitmap instead of
>   heap (patch 4), avoid per-element heap churn in vmstate ptr
>   marker field (patch 3).
> - Cleanups: collapse migration_fd_valid into a single boolean
>   expression (patch 1), replace strcpy("") with explicit NUL
>   termination (patch 2), merge thread-join and cleanup loops in
>   multifd_recv_cleanup (patch 6).
> 
> No functional change intended except for the off-by-one fix in
> patch 5.
> 
> Bin Guo (8):
>   migration/fd: collapse migration_fd_valid into single boolean
>     expression
>   migration/global_state: replace strcpy("") with explicit NUL
>     termination
>   migration/vmstate: avoid per-element heap churn in vmsd ptr marker
>     field
>   migration/savevm: use stack-allocated bitmap in
>     configuration_validate_capabilities
>   migration/multifd: fix off-by-one in recv channel ID validation
>   migration/multifd: merge thread-join and cleanup loops in
>     multifd_recv_cleanup
>   migration/multifd: cache migrate_multifd_channels() in send/recv hot
>     paths
>   migration/multifd: cache channel count in multifd_send_sync_main

For patch 1, I tend to agree with Fabiano; I don't clearly see why the
oneliner is always better.  Oneliners can be harder to read to some (for
most of the cases, myself included..).

For patch 6, I confess I can't see an issue with the current patch, but
since there's a discussion, let it happen.

I queued the rest, thanks.

-- 
Peter Xu