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

[Peter Xu <peterx@redhat.com>]

On Mon, Jul 22, 2024 at 06:20:28PM -0300, Fabiano Rosas wrote:
> 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

Actually that's exactly what I think is right.. it looks to me now that we
could bypass anything in MultifdOps (including recv()) but let device state
be a parallel layer of MultifdOps itself, leaving MultifdOps only for
compressors.

And yeah, I still remember you just renamed it from recv_pages() to
recv()..  it's just that now when think it again it looks like cleaner to
make it only about pages..

> 
> >
> > 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.

Yes it should be separate.

And what I meant is we don't need all these noops, but recv() keeps being
ignored just like above, then for sender side, right now it's:

            ret = multifd_send_state->ops->send_prepare(p, &local_err);
            if (migrate_mapped_ram()) {
                file_write_ramblock_iov();
            } else {
                ret = qio_channel_writev_full_all();
            }

VFIO can process device state in parallel, so:

    if (ram) {
        ret = multifd_send_state->ops->send_prepare(p, &local_err);
        if (migrate_mapped_ram()) {
                file_write_ramblock_iov();
        } else {
                qio_channel_writev_full_all();
        }
    } else {
        // device state handling
        multifd_send_device_prepare(...);
        ...
        qio_channel_writev_full_all();
    }

Then MultifdOps doesn't apply to device states.

-- 
Peter Xu