| Message ID | 7b02baba8e6ddb23ef7c349d312b9b631db09d7e.1741124640.git.maciej.szmigiero@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Multifd | expand |
Fabiano, Could you please ack (or not) this patch please ? Thanks, C. On 3/4/25 23:03, Maciej S. Szmigiero wrote: > 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 e383e75b1a02..64d8ca180167 100644 > --- a/migration/multifd-device-state.c > +++ b/migration/multifd-device-state.c > @@ -20,11 +20,6 @@ static struct { > MultiFDSendData *send_data; > } *multifd_send_device_state; > > -size_t multifd_device_state_payload_size(void) > -{ > - return sizeof(MultiFDDeviceState_t); > -} > - > void multifd_device_state_send_setup(void) > { > assert(!multifd_send_device_state); > 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 3625c9a37c0e..dfb5189f0ea3 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)); > + multifd_ram_payload_alloc(&new->u.ram); > + /* Device state allocates its payload on-demand */ > > - /* > - * 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); > - > - 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); > + > g_free(data); > } > > diff --git a/migration/multifd.h b/migration/multifd.h > index aa679d8bbe83..2d337e7b3b52 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; > @@ -394,12 +398,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); >
"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> Acked-by: Fabiano Rosas <farosas@suse.de>
diff --git a/migration/multifd-device-state.c b/migration/multifd-device-state.c index e383e75b1a02..64d8ca180167 100644 --- a/migration/multifd-device-state.c +++ b/migration/multifd-device-state.c @@ -20,11 +20,6 @@ static struct { MultiFDSendData *send_data; } *multifd_send_device_state; -size_t multifd_device_state_payload_size(void) -{ - return sizeof(MultiFDDeviceState_t); -} - void multifd_device_state_send_setup(void) { assert(!multifd_send_device_state); 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 3625c9a37c0e..dfb5189f0ea3 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)); + multifd_ram_payload_alloc(&new->u.ram); + /* Device state allocates its payload on-demand */ - /* - * 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); - - 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); + g_free(data); } diff --git a/migration/multifd.h b/migration/multifd.h index aa679d8bbe83..2d337e7b3b52 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; @@ -394,12 +398,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);