On Mon, Jul 22, 2024 at 06:20:28PM -0300, Fabiano Rosas wrote:
> Peter Xu <pet...@redhat.com> writes:
>
> > On Mon, Jul 22, 2024 at 05:21:48PM -0300, Fabiano Rosas wrote:
> >> Peter Xu <pet...@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.szmigi...@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
