"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?
> 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);
