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