Re: [RFC PATCH v2 4/9] migration/multifd: Make MultiFDPages_t:offset a flexible array member

[Peter Xu]

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 <farosas@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