Re: [RFC PATCH v2 0/9] migration/multifd: Remove multifd_send_state->pages

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Mon, Jul 22, 2024 at 05:21:48PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Mon, Jul 22, 2024 at 02:59:05PM -0300, Fabiano Rosas wrote:
>> >> Hi,
>> >> 
>> >> In this v2 I took Peter's suggestion of keeping the channels' pointers
>> >> and moving only the extra slot. The major changes are in patches 5 and
>> >> 9. Patch 3 introduces the structure:
>> >> 
>> >> typedef enum {
>> >>     MULTIFD_PAYLOAD_NONE,
>> >>     MULTIFD_PAYLOAD_RAM,
>> >> } MultiFDPayloadType;
>> >> 
>> >> struct MultiFDSendData {
>> >>     MultiFDPayloadType type;
>> >>     union {
>> >>         MultiFDPages_t ram;
>> >>     } u;
>> >> };
>> >> 
>> >> I added a NONE type so we can use it to tell when the channel has
>> >> finished sending a packet, since we'll need to switch types between
>> >> clients anyway. This avoids having to introduce a 'size', or 'free'
>> >> variable.
>> >
>> > This at least looks better to me, thanks.
>> >
>> >> 
>> >> WHAT'S MISSING:
>> >> 
>> >> - The support for calling multifd_send() concurrently. Maciej has this
>> >>   in his series so I didn't touch it.
>> >> 
>> >> - A way of adding methods for the new payload type. Currently, the
>> >>   compression methods are somewhat coupled with ram migration, so I'm
>> >>   not sure how to proceed.
>> >
>> > What is this one?  Why compression methods need new payload?  Aren't they
>> > ram-typed?
>> 
>> The data we transport is MultiFDPages_t, yes, but the MultiFDMethods are
>> either nocomp, or the compression-specific methods
>> (e.g. zlib_send_prepare).
>> 
>> How do we add methods for the upcoming new payload types? I don't expect
>> us to continue using nocomp and then do "if (ram)... else if
>> (device_state) ..." inside of them. I would expect us to rename
>> s/nocomp/ram/ and add a new set of MultiFDMethods for the new data type
>> (e.g. vfio_send_prepare, vmstate_send_prepare, etc).
>> 
>> multifd_nocomp_ops -> multifd_ram_ops // rename
>> multifd_zlib_ops   // existing
>> multifd_device_ops // new
>> 
>> The challenge here is that the current framework is nocomp
>> vs. compression. It needs to become ram + compression vs. other types.
>
> IMHO we can keep multifd_ops[] only for RAM.  There's only send_prepare()
> that device state will need, and so far it's only (referring Maciej's
> code):
>
> static int nocomp_send_prepare_device_state(MultiFDSendParams *p,
>                                             Error **errp)
> {
>     multifd_send_prepare_header_device_state(p);
>
>     assert(!(p->flags & MULTIFD_FLAG_SYNC));
>
>     p->next_packet_size = p->device_state->buf_len;
>     if (p->next_packet_size > 0) {
>         p->iov[p->iovs_num].iov_base = p->device_state->buf;
>         p->iov[p->iovs_num].iov_len = p->next_packet_size;
>         p->iovs_num++;
>     }
>
>     p->flags |= MULTIFD_FLAG_NOCOMP | MULTIFD_FLAG_DEVICE_STATE;
>
>     multifd_send_fill_packet_device_state(p);
>
>     return 0;
> }
>
> None of other multifd_ops are used.

There's also a conditional around device_state when calling
->recv(). That could seems like it could go to a hook.

https://lore.kernel.org/r/41dedaf2c9abebb5e45f88c052daa26320715a92.1718717584.git.maciej.szmigiero@oracle.com

>
> I think we can directly invoke this part of device state code in
> multifd_send_thread() for now.  So far I think it should be ok.

It's not just that. There's also a check for "if (ram)" at every call to
multifd_ops to avoid calling the ram code when doing the device
migration. It would be way easier to just set noop functions for those.

static MultiFDMethods multifd_devstate_ops = {
    .send_setup = noop_send_setup,
    .send_cleanup = noop_send_cleanup,
    .send_prepare = devstate_send_prepare,
    .recv_setup = noop_recv_setup,
    .recv_cleanup = noop_recv_cleanup,
    .recv = devstate_recv
};

I'm not saying this needs to be done in this series though. But I do
think that's the correct design choice for the long term.