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

[Fabiano Rosas <farosas@suse.de>]

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

> From: Peter Xu <peterx@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 <peterx@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.szmigiero@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?

>      g_free(data);
>  }
>  
> diff --git a/migration/multifd.h b/migration/multifd.h
> index ddc617db9acb..f7811cc0d0cb 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -115,9 +115,13 @@ typedef struct {
>      uint32_t num;
>      /* number of normal pages */
>      uint32_t normal_num;
> +    /*
> +     * Pointer to the ramblock.  NOTE: it's caller's responsibility to make
> +     * sure the pointer is always valid!
> +     */
>      RAMBlock *block;
> -    /* offset of each page */
> -    ram_addr_t offset[];
> +    /* offset array of each page, managed by multifd */
> +    ram_addr_t *offset;
>  } MultiFDPages_t;
>  
>  struct MultiFDRecvData {
> @@ -140,7 +144,7 @@ typedef enum {
>      MULTIFD_PAYLOAD_DEVICE_STATE,
>  } MultiFDPayloadType;
>  
> -typedef union MultiFDPayload {
> +typedef struct MultiFDPayload {
>      MultiFDPages_t ram;
>      MultiFDDeviceState_t device_state;
>  } MultiFDPayload;
> @@ -392,12 +396,11 @@ void multifd_ram_save_cleanup(void);
>  int multifd_ram_flush_and_sync(QEMUFile *f);
>  bool multifd_ram_sync_per_round(void);
>  bool multifd_ram_sync_per_section(void);
> -size_t multifd_ram_payload_size(void);
> +void multifd_ram_payload_alloc(MultiFDPages_t *pages);
> +void multifd_ram_payload_free(MultiFDPages_t *pages);
>  void multifd_ram_fill_packet(MultiFDSendParams *p);
>  int multifd_ram_unfill_packet(MultiFDRecvParams *p, Error **errp);
>  
> -size_t multifd_device_state_payload_size(void);
> -
>  void multifd_send_data_clear_device_state(MultiFDDeviceState_t *device_state);
>  
>  void multifd_device_state_send_setup(void);