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

[Peter Xu <peterx@redhat.com>]

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?

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

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

-- 
Peter Xu