On Mon, Jul 22, 2024 at 02:59:09PM -0300, Fabiano Rosas wrote:
> We're about to use MultiFDPages_t from inside the MultiFDSendData
> payload union, which means we cannot have pointers to allocated data
> inside the pages structure, otherwise we'd lose the reference to that
> memory once another payload type touches the union. Move the offset
> array into the end of the structure and turn it into a flexible array
> member, so it is allocated along with the rest of MultiFDSendData in
> the next patches.
>
> Note that the ramblock pointer is still fine because the storage for
> it is not owned by the migration code.
>
> Signed-off-by: Fabiano Rosas <faro...@suse.de>
> ---
> migration/multifd.c | 21 ++++++---------------
> migration/multifd.h | 4 ++--
> 2 files changed, 8 insertions(+), 17 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 20a767157e..440319b361 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -389,22 +389,10 @@ static int multifd_recv_initial_packet(QIOChannel *c,
> Error **errp)
> return msg.id;
> }
>
> -static MultiFDPages_t *multifd_pages_init(uint32_t n)
> -{
> - MultiFDPages_t *pages = g_new0(MultiFDPages_t, 1);
> -
> - pages->allocated = n;
> - pages->offset = g_new0(ram_addr_t, n);
> -
> - return pages;
> -}
Considering this is the tricky object to allocate here, shall we keep the
function just for readability (and already dedups below two callers)? With
it, someone else will notice g_new0() stops working for MultiFDPages_t.
Some verbose comment would be nice too.
Maybe we can also move multifd_ram_payload_size() from the next patch to
here.
> -
> static void multifd_pages_clear(MultiFDPages_t *pages)
> {
> multifd_pages_reset(pages);
> pages->allocated = 0;
> - g_free(pages->offset);
> - pages->offset = NULL;
> g_free(pages);
> }
>
> @@ -1169,7 +1157,9 @@ bool multifd_send_setup(void)
> thread_count = migrate_multifd_channels();
> multifd_send_state = g_malloc0(sizeof(*multifd_send_state));
> multifd_send_state->params = g_new0(MultiFDSendParams, thread_count);
> - multifd_send_state->pages = multifd_pages_init(page_count);
> + multifd_send_state->pages = g_malloc0(sizeof(MultiFDPages_t) +
> + page_count * sizeof(ram_addr_t));
> + multifd_send_state->pages->allocated = page_count;
> qemu_sem_init(&multifd_send_state->channels_created, 0);
> qemu_sem_init(&multifd_send_state->channels_ready, 0);
> qatomic_set(&multifd_send_state->exiting, 0);
> @@ -1181,8 +1171,9 @@ bool multifd_send_setup(void)
> qemu_sem_init(&p->sem, 0);
> qemu_sem_init(&p->sem_sync, 0);
> p->id = i;
> - p->pages = multifd_pages_init(page_count);
> -
> + p->pages = g_malloc0(sizeof(MultiFDPages_t) +
> + page_count * sizeof(ram_addr_t));
> + p->pages->allocated = page_count;
> if (use_packets) {
> p->packet_len = sizeof(MultiFDPacket_t)
> + sizeof(uint64_t) * page_count;
> diff --git a/migration/multifd.h b/migration/multifd.h
> index c7b1ebe099..12d4247e23 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -78,9 +78,9 @@ typedef struct {
> uint32_t normal_num;
> /* number of allocated pages */
> uint32_t allocated;
> + RAMBlock *block;
> /* offset of each page */
> - ram_addr_t *offset;
> - RAMBlock *block;
> + ram_addr_t offset[];
> } MultiFDPages_t;
>
> struct MultiFDRecvData {
> --
> 2.35.3
>
--
Peter Xu
