Re: [PATCH v2 2/7] migration/multifd: Allow to sync with sender threads only

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Dec 06, 2024 at 10:26:06AM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > Teach multifd_send_sync_main() to sync with threads only.
>> >
>> > We already have such requests, which is when mapped-ram is enabled with
>> > multifd.  In that case, no SYNC messages will be pushed to the stream when
>> > multifd syncs the sender threads because there's no destination threads
>> > waiting for that.  The whole point of the sync is to make sure all threads
>> > flushed their jobs.
>> 
>> s/flushed/finished/ otherwise we risk confusing people.
>
> done.
>
>> 
>> >
>> > So fundamentally we have a request to do the sync in different ways:
>> >
>> >   - Either to sync the threads only,
>> >   - Or to sync the threads but also with the destination side.
>> >
>> > Mapped-ram did it already because of the use_packet check in the sync
>> > handler of the sender thread.  It works.
>> >
>> > However it may stop working when e.g. VFIO may start to reuse multifd
>> > channels to push device states.  In that case VFIO has similar request on
>> > "thread-only sync" however we can't check a flag because such sync request
>> > can still come from RAM which needs the on-wire notifications.
>> >
>> > Paving way for that by allowing the multifd_send_sync_main() to specify
>> > what kind of sync the caller needs.  We can use it for mapped-ram already.
>> >
>> > No functional change intended.
>> >
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >  migration/multifd.h        | 19 ++++++++++++++++---
>> >  migration/multifd-nocomp.c |  7 ++++++-
>> >  migration/multifd.c        | 15 +++++++++------
>> >  3 files changed, 31 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/migration/multifd.h b/migration/multifd.h
>> > index 50d58c0c9c..bd337631ec 100644
>> > --- a/migration/multifd.h
>> > +++ b/migration/multifd.h
>> > @@ -19,6 +19,18 @@
>> >  typedef struct MultiFDRecvData MultiFDRecvData;
>> >  typedef struct MultiFDSendData MultiFDSendData;
>> >  
>> > +typedef enum {
>> > +    /* No sync request */
>> > +    MULTIFD_SYNC_NONE = 0,
>> > +    /* Sync locally on the sender threads without pushing messages */
>> > +    MULTIFD_SYNC_LOCAL,
>> > +    /*
>> > +     * Sync not only on the sender threads, but also push "SYNC" message 
>> > to
>> > +     * the wire (which is for a remote sync).
>> 
>> s/SYNC/MULTIFD_FLAG_SYNC/
>> 
>> Do we need to also mention that this needs to be paired with a
>> multifd_recv_sync_main() via the emission of the
>> RAM_SAVE_FLAG_MULTIFD_FLUSH flag on the stream?
>
> If we want to mention something, IMO it would be better about what happens
> on the src, not dest.  It can be too hard to follow if we connect that
> directly to the dest behavior.
>
> Does this look good to you?
>
>     /*
>      * Sync not only on the sender threads, but also push MULTIFD_FLAG_SYNC
>      * message to the wire for each iochannel (which is for a remote sync).
>      *
>      * When remote sync is used, need to be paired with a follow up
>      * RAM_SAVE_FLAG_EOS / RAM_SAVE_FLAG_MULTIFD_FLUSH message on the main
>      * channel.
>      */

Yes, thanks.