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

[Peter Xu <peterx@redhat.com>]

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.

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.

-- 
Peter Xu