Re: [PATCH v3 07/14] migration/multifd: Replace p->pages with an union pointer

[Peter Xu]

On Thu, Aug 01, 2024 at 09:35:09AM -0300, Fabiano Rosas wrote:
> We want multifd to be able to handle more types of data than just ram
> pages. To start decoupling multifd from pages, replace p->pages
> (MultiFDPages_t) with the new type MultiFDSendData that hides the
> client payload inside an union.
> 
> The general idea here is to isolate functions that *need* to handle
> MultiFDPages_t and move them in the future to multifd-ram.c, while
> multifd.c will stay with only the core functions that handle
> MultiFDSendData/MultiFDRecvData.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

[...]

> +static MultiFDSendData *multifd_send_data_alloc(void)
> +{
> +    size_t max_payload_size, size_minus_payload;
> +
> +    /*
> +     * 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(), 
> 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);
> +
> +    return g_malloc0(size_minus_payload + max_payload_size);
> +}

Hmm I didn't notice the hole issue for sure..

For the mid term we really should remove this in one way or another.. what
I was thinking is mentioned in the other thread:

https://lore.kernel.org/qemu-devel/ZsZZFwws5tlOMmZk@x1n/

I hope we can simply statically define offset[] to be the max.

I don't think we must stick with size-per-packet, in this case IMHO we
should choose whatever is easier for us, and I never worried on regression
yet so far as long as the relevant n_pages is still relatively large. Not
to mention AFAIU for production use, x86/s390 always uses 4K psize, while
arm64 doesn't yet have a stable kvm-avail vcpu model, which might be a
bigger issue as of now to solve..

Let's see how it goes..

-- 
Peter Xu