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

[Fabiano Rosas <farosas@suse.de>]

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.

>
>> 
>> - Moving all the multifd ram code into multifd-ram.c after this^ is
>>   sorted out.
>> 
>> CI run: https://gitlab.com/farosas/qemu/-/pipelines/1381005020
>> 
>> v1:

(1)

>> https://lore.kernel.org/r/20240620212111.29319-1-farosas@suse.de
>> 
>> First of all, apologies for the roughness of the series. I'm off for
>> the next couple of weeks and wanted to put something together early
>> for your consideration.
>
> PS: I assume this is old content, or I'll envy you on how frequent you can
> take days off!..

That't the v1 cover letter (1). I don't like to post v2 with "here's
what changed" without having the previous cover-letter at hand for
referencing.

>
>> 
>> This series is a refactoring (based on an earlier, off-list
>> attempt[0]), aimed to remove the usage of the MultiFDPages_t type in
>> the multifd core. If we're going to add support for more data types to
>> multifd, we first need to clean that up.
>> 
>> This time around this work was prompted by Maciej's series[1]. I see
>> you're having to add a bunch of is_device_state checks to work around
>> the rigidity of the code.
>> 
>> Aside from the VFIO work, there is also the intent (coming back from
>> Juan's ideas) to make multifd the default code path for migration,
>> which will have to include the vmstate migration and anything else we
>> put on the stream via QEMUFile.
>> 
>> I have long since been bothered by having 'pages' sprinkled all over
>> the code, so I might be coming at this with a bit of a narrow focus,
>> but I believe in order to support more types of payloads in multifd,
>> we need to first allow the scheduling at multifd_send_pages() to be
>> independent of MultiFDPages_t. So here it is. Let me know what you
>> think.
>> 
>> (as I said, I'll be off for a couple of weeks, so feel free to
>> incorporate any of this code if it's useful. Or to ignore it
>> completely).
>> 
>> CI run: https://gitlab.com/farosas/qemu/-/pipelines/1340992028
>> 
>> 0- https://github.com/farosas/qemu/commits/multifd-packet-cleanups/
>> 1- https://lore.kernel.org/r/cover.1718717584.git.maciej.szmigiero@oracle.com
>> 
>> Fabiano Rosas (9):
>>   migration/multifd: Reduce access to p->pages
>>   migration/multifd: Pass in MultiFDPages_t to file_write_ramblock_iov
>>   migration/multifd: Introduce MultiFDSendData
>>   migration/multifd: Make MultiFDPages_t:offset a flexible array member
>>   migration/multifd: Replace p->pages with an union pointer
>>   migration/multifd: Move pages accounting into
>>     multifd_send_zero_page_detect()
>>   migration/multifd: Isolate ram pages packet data
>>   migration/multifd: Don't send ram data during SYNC
>>   migration/multifd: Replace multifd_send_state->pages with client data
>> 
>>  migration/file.c              |   3 +-
>>  migration/file.h              |   2 +-
>>  migration/multifd-qpl.c       |  10 +-
>>  migration/multifd-uadk.c      |   9 +-
>>  migration/multifd-zero-page.c |   9 +-
>>  migration/multifd-zlib.c      |   4 +-
>>  migration/multifd-zstd.c      |   4 +-
>>  migration/multifd.c           | 239 +++++++++++++++++++++-------------
>>  migration/multifd.h           |  37 ++++--
>>  migration/ram.c               |   1 +
>>  10 files changed, 201 insertions(+), 117 deletions(-)
>> 
>> 
>> base-commit: a7ddb48bd1363c8bcdf42776d320289c42191f01
>> -- 
>> 2.35.3
>>