Re: [RFC PATCH 6/7] migration/multifd: Move payload storage out of the channel parameters

[Peter Xu]

On Thu, Jul 18, 2024 at 06:27:32PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Thu, Jul 18, 2024 at 04:39:00PM -0300, Fabiano Rosas wrote:
> >> v2 is ready, but unfortunately this approach doesn't work. When client A
> >> takes the payload, it fills it with it's data, which may include
> >> allocating memory. MultiFDPages_t does that for the offset. This means
> >> we need a round of free/malloc at every packet sent. For every client
> >> and every allocation they decide to do.
> >
> > Shouldn't be a blocker?  E.g. one option is:
> >
> >     /* Allocate both the pages + offset[] */
> >     MultiFDPages_t *pages = g_malloc0(sizeof(MultiFDPages_t) +
> >                                       sizeof(ram_addr_t) * n, 1);
> >     pages->allocated = n;
> >     pages->offset = &pages[1];
> >
> > Or.. we can also make offset[] dynamic size, if that looks less tricky:
> >
> > typedef struct {
> >     /* number of used pages */
> >     uint32_t num;
> >     /* number of normal pages */
> >     uint32_t normal_num;
> >     /* number of allocated pages */
> >     uint32_t allocated;
> >     RAMBlock *block;
> >     /* offset of each page */
> >     ram_addr_t offset[0];
> > } MultiFDPages_t;
> 
> I think you missed the point. If we hold a pointer inside the payload,
> we lose the reference when the other client takes the structure and puts
> its own data there. So we'll need to alloc/free everytime we send a
> packet.

For option 1: when the buffer switch happens, MultiFDPages_t will switch as
a whole, including its offset[], because its offset[] always belong to this
MultiFDPages_t.  So yes, we want to lose that *offset reference together
with MultiFDPages_t here, so the offset[] always belongs to one single
MultiFDPages_t object for its lifetime.

For option 2: I meant MultiFDPages_t will have no offset[] pointer anymore,
but make it part of the struct (MultiFDPages_t.offset[]).  Logically it's
the same as option 1 but maybe slight cleaner.  We just need to make it
sized 0 so as to be dynamic in size.

Hmm.. is it the case?

-- 
Peter Xu