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

[Peter Xu]

On Tue, Jul 23, 2024 at 02:48:48PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > 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.
> 
> To avoid getting into bikeshed territory, I think we should postpone
> this discussion until after Maciej's series is merged, so we can speak
> more concretely about the implications. It's easy enough to go from your
> suggestion to mine than the other way around, so let's leave at that.
> 
> I had it already written, so more of my reasoning below, if you're
> interested.

I never thought this is bikeshedding.. What we're discussing now is exactly
what should appear in Maciej's code, am I right?  I thought we should
figure it out before it's merged, if that's the case..

And whose suggestion isn't that important, IMO.  We simply try to discuss
this technically and reach a consensus.. no matter who proposed what.

> ======
> 
> We already have the send/recv threads structured in relation to what we
> do inside the hooks. You're just defining a function that's not a hook,
> but it has the same signature and responsibilities and needs to be
> called at the same moment.
> 
> I think the dissonance here is that you don't see the multifd thread
> code and the payloads (ram, device) as separate layers. Payload-specific
> code should not be at top level. Otherwise, it breaks any semblance of
> proper layering:
> 
> - payload code will have access to MultiFD*Params, which has a bunch of
>   control variables for the loop, the semaphores, etc. that should not
>   be touched;
> 
> - payload code ends up influencing the flow of the thread
>   function. E.g. when zero_copy_send used to dictate whether we'd have
>   separate IO for the packet or not.
> 
> - temporary variables needed by the payload code will have to be
>   declared inside the thread funcion, which makes tempting to use them
>   across payload types and also in the thread code itself;
> 
> - it creates doubt as to whether new changes go inside the hooks, in the
>   if/else or outside of it;
> 
> Think about how easy it has has been to review and merge the various
> compression features we had. It doesn't matter how much they mess up
> inside the hooks, it will never cause the dreaded "Memory content
> inconsistency at ..." error from check_guest_ram(). At least not in a
> way that affects other people. Now compare that with for instance the
> zero-page work, or even mapped-ram, that required a bunch of changes to
> the multifd control flow itself (e.g. all of the sync changes w/
> mapped-ram).

I think there's one issue where we only support one MultiFDMethods as of
now to be active, while the "clients" of multifd can be >1 from payload
POV.  It means I'm not sure how VFIO can provide a MultiFDMethods if it
will overwrite what should be there to define how to process RAM..

Then, we should logically allow VFIO migration to happen with RAM being
compressed with ZSTD/ZLIB/whatever, right?  The question is which
MultiFDMethods we should assign if they're the same layer in this case..

-- 
Peter Xu