Re: [PATCH v4 17/33] migration/multifd: Make MultiFDSendData a struct

[Maciej S. Szmigiero]

On 7.02.2025 15:36, Fabiano Rosas wrote:

"Maciej S. Szmigiero" <m...@maciej.szmigiero.name> writes:

From: Peter Xu <pet...@redhat.com>

The newly introduced device state buffer can be used for either storing
VFIO's read() raw data, but already also possible to store generic device
states.  After noticing that device states may not easily provide a max
buffer size (also the fact that RAM MultiFDPages_t after all also want to
have flexibility on managing offset[] array), it may not be a good idea to
stick with union on MultiFDSendData.. as it won't play well with such
flexibility.

Switch MultiFDSendData to a struct.

It won't consume a lot more space in reality, after all the real buffers
were already dynamically allocated, so it's so far only about the two
structs (pages, device_state) that will be duplicated, but they're small.

With this, we can remove the pretty hard to understand alloc size logic.
Because now we can allocate offset[] together with the SendData, and
properly free it when the SendData is freed.

Signed-off-by: Peter Xu <pet...@redhat.com>
[MSS: Make sure to clear possible device state payload before freeing
MultiFDSendData, remove placeholders for other patches not included]
Signed-off-by: Maciej S. Szmigiero <maciej.szmigi...@oracle.com>
---
  migration/multifd-device-state.c |  5 -----
  migration/multifd-nocomp.c       | 13 ++++++-------
  migration/multifd.c              | 25 +++++++------------------
  migration/multifd.h              | 15 +++++++++------
  4 files changed, 22 insertions(+), 36 deletions(-)

diff --git a/migration/multifd-device-state.c b/migration/multifd-device-state.c
index 2207bea9bf8a..d1674b432ff2 100644
--- a/migration/multifd-device-state.c
+++ b/migration/multifd-device-state.c
@@ -16,11 +16,6 @@ static QemuMutex queue_job_mutex;

static MultiFDSendData *device_state_send; -size_t multifd_device_state_payload_size(void)

-{
-    return sizeof(MultiFDDeviceState_t);
-}
-
  void multifd_device_state_send_setup(void)
  {
      qemu_mutex_init(&queue_job_mutex);
diff --git a/migration/multifd-nocomp.c b/migration/multifd-nocomp.c
index c00804652383..ffe75256c9fb 100644
--- a/migration/multifd-nocomp.c
+++ b/migration/multifd-nocomp.c
@@ -25,15 +25,14 @@

static MultiFDSendData *multifd_ram_send; -size_t multifd_ram_payload_size(void)

+void multifd_ram_payload_alloc(MultiFDPages_t *pages)
  {
-    uint32_t n = multifd_ram_page_count();
+    pages->offset = g_new0(ram_addr_t, multifd_ram_page_count());
+}

- /*

-     * We keep an array of page offsets at the end of MultiFDPages_t,
-     * add space for it in the allocation.
-     */
-    return sizeof(MultiFDPages_t) + n * sizeof(ram_addr_t);
+void multifd_ram_payload_free(MultiFDPages_t *pages)
+{
+    g_clear_pointer(&pages->offset, g_free);
  }

void multifd_ram_save_setup(void)

diff --git a/migration/multifd.c b/migration/multifd.c
index 61b061a33d35..0b61b8192231 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -105,26 +105,12 @@ struct {

MultiFDSendData *multifd_send_data_alloc(void)

  {
-    size_t max_payload_size, size_minus_payload;
+    MultiFDSendData *new = g_new0(MultiFDSendData, 1);

- /*

-     * MultiFDPages_t has a flexible array at the end, account for it
-     * when allocating MultiFDSendData. Use max() in case other types
-     * added to the union in the future are larger than
-     * (MultiFDPages_t + flex array).
-     */
-    max_payload_size = MAX(multifd_ram_payload_size(),
-                           multifd_device_state_payload_size());
-    max_payload_size = MAX(max_payload_size, sizeof(MultiFDPayload));
-
-    /*
-     * Account for any holes the compiler might insert. We can't pack
-     * the structure because that misaligns the members and triggers
-     * Waddress-of-packed-member.
-     */
-    size_minus_payload = sizeof(MultiFDSendData) - sizeof(MultiFDPayload);
+    multifd_ram_payload_alloc(&new->u.ram);
+    /* Device state allocates its payload on-demand */

- return g_malloc0(size_minus_payload + max_payload_size);

+    return new;
  }

void multifd_send_data_clear(MultiFDSendData *data)

@@ -151,8 +137,11 @@ void multifd_send_data_free(MultiFDSendData *data)
          return;
      }

+ /* This also free's device state payload */

      multifd_send_data_clear(data);

+ multifd_ram_payload_free(&data->u.ram);

+

Shouldn't this be added to the switch statement at
multifd_send_data_clear() instead?

I think the intention is that RAM pages are allocated at MultiFDSendData
instance allocation time and stay allocated for its entire lifetime -
because we know RAM pages packet data size upfront and also that's what
multifd send threads will be mostly sending.

In contrast with RAM, device state allocates its payload on-demand since
its size is unknown and can vary between each multifd_queue_device_state()
invocation. This payload is free'd after it gets send by a multifd send
thread.

There's even a comment about this in multifd_send_data_alloc():

    multifd_ram_payload_alloc(&new->u.ram);
    /* Device state allocates its payload on-demand */

Thanks,
Maciej