Peter Xu <pet...@redhat.com> writes:
> On Thu, Dec 05, 2024 at 05:16:05PM -0300, Fabiano Rosas wrote:
>> Peter Xu <pet...@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.
>> >
>> > 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 <pet...@redhat.com>
>> > ---
>> > migration/multifd.h | 16 +++++++++++++---
>> > migration/multifd-nocomp.c | 8 +++++++-
>> > migration/multifd.c | 14 ++++++++------
>> > 3 files changed, 28 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/migration/multifd.h b/migration/multifd.h
>> > index 50d58c0c9c..6b2f60a917 100644
>> > --- a/migration/multifd.h
>> > +++ b/migration/multifd.h
>> > @@ -19,6 +19,15 @@
>> > typedef struct MultiFDRecvData MultiFDRecvData;
>> > typedef struct MultiFDSendData MultiFDSendData;
>> >
>> > +typedef enum {
>> > + /* No sync request */
>> > + MULTIFD_SYNC_NONE = 0,
>> > + /* Sync on the sender threads without pushing messages */
>> > + MULTIFD_SYNC_THREADS,
>> > + /* Sync on the sender threads, meanwhile push "SYNC" message to the
>> > wire */
>>
>> s/meanwhile//
>>
>> > + MULTIFD_SYNC_THREADS_AND_NOTIFY,
>> > +} MultiFDSyncReq;
>>
>> I think I'd prefer the local vs. remote terminology I introduced in my
>> proposal [1] for cleaning up the multifd_flush_after_each_section() code:
>
> I'm ok with your naming, as long as the comment will explain.
>
>>
>> LOCAL - sync the local threads between themselves
>> REMOTE - put a message on the stream for the remote end to perform a
>> sync on their threads.
>>
>> Down below you're passing the
>> MULTIFD_SYNC_THREADS_AND_NOTIFY into the send thread, but the "sync
>> threads" part of this is really done outside the thread, so that part
>> doesn't have a meaning inside the thread.
>>
>> 1- https://lore.kernel.org/r/875xo8n4ue....@suse.de
>>
>> Also, please provide your input there^, it would be nice to unify the
>> terminology and reasoning about both changes.
>
> Yes, I'm mostly flushing my inbox in time order unless prioritized, so I'm
> getting there today or tomorrow.
>
>>
>> > +
>> > bool multifd_send_setup(void);
>> > void multifd_send_shutdown(void);
>> > void multifd_send_channel_created(void);
>> > @@ -28,7 +37,7 @@ void multifd_recv_shutdown(void);
>> > bool multifd_recv_all_channels_created(void);
>> > void multifd_recv_new_channel(QIOChannel *ioc, Error **errp);
>> > void multifd_recv_sync_main(void);
>> > -int multifd_send_sync_main(void);
>> > +int multifd_send_sync_main(MultiFDSyncReq req);
>> > bool multifd_queue_page(RAMBlock *block, ram_addr_t offset);
>> > bool multifd_recv(void);
>> > MultiFDRecvData *multifd_get_recv_data(void);
>> > @@ -143,7 +152,7 @@ typedef struct {
>> > /* multifd flags for each packet */
>> > uint32_t flags;
>> > /*
>> > - * The sender thread has work to do if either of below boolean is set.
>> > + * The sender thread has work to do if either of below field is set.
>> > *
>> > * @pending_job: a job is pending
>> > * @pending_sync: a sync request is pending
>> > @@ -152,7 +161,8 @@ typedef struct {
>> > * cleared by the multifd sender threads.
>> > */
>> > bool pending_job;
>> > - bool pending_sync;
>> > + MultiFDSyncReq pending_sync;
>> > +
>> > MultiFDSendData *data;
>> >
>> > /* thread local variables. No locking required */
>> > diff --git a/migration/multifd-nocomp.c b/migration/multifd-nocomp.c
>> > index 55191152f9..f64c4c9abd 100644
>> > --- a/migration/multifd-nocomp.c
>> > +++ b/migration/multifd-nocomp.c
>> > @@ -345,6 +345,8 @@ retry:
>> >
>> > int multifd_ram_flush_and_sync(void)
>> > {
>> > + MultiFDSyncReq req;
>> > +
>> > if (!migrate_multifd()) {
>> > return 0;
>> > }
>> > @@ -356,7 +358,11 @@ int multifd_ram_flush_and_sync(void)
>> > }
>> > }
>> >
>> > - return multifd_send_sync_main();
>> > + /* File migrations only need to sync with threads */
>> > + req = migrate_mapped_ram() ?
>> > + MULTIFD_SYNC_THREADS : MULTIFD_SYNC_THREADS_AND_NOTIFY;
>> > +
>> > + return multifd_send_sync_main(req);
>> > }
>> >
>> > bool multifd_send_prepare_common(MultiFDSendParams *p)
>> > diff --git a/migration/multifd.c b/migration/multifd.c
>> > index 498e71fd10..77645e87a0 100644
>> > --- a/migration/multifd.c
>> > +++ b/migration/multifd.c
>> > @@ -523,7 +523,7 @@ static int multifd_zero_copy_flush(QIOChannel *c)
>> > return ret;
>> > }
>> >
>> > -int multifd_send_sync_main(void)
>> > +int multifd_send_sync_main(MultiFDSyncReq req)
>> > {
>> > int i;
>> > bool flush_zero_copy;
>> > @@ -543,8 +543,8 @@ int multifd_send_sync_main(void)
>> > * We should be the only user so far, so not possible to be set by
>> > * others concurrently.
>> > */
>> > - assert(qatomic_read(&p->pending_sync) == false);
>> > - qatomic_set(&p->pending_sync, true);
>> > + assert(qatomic_read(&p->pending_sync) == MULTIFD_SYNC_NONE);
>> > + qatomic_set(&p->pending_sync, req);
>>
>> Hmm, isn't it easier to skip the whole loop if req ==
>> MULTIFD_SYNC_THREADS? I don't remember why we kept this loop here for
>> mapped-ram.
>
> The "thread-only" version of request (or, in your preferred naming, "local"
> sync request) says: "please flush all the works enqueued in sender thread".
> Sync is still needed even for mapped-ram to make sure pwrite()s all land.
> Also needed for VFIO.
I think I remember now, what's needed is to release p->sem and wait on
p->sem_sync (one in each of these loops). We don't need to set the
pending_sync flag if it's not going to be used:
multifd_send_sync_main:
for () {
...
if (remote_sync) {
assert(qatomic_read(&p->pending_sync) == false);
qatomic_set(&p->pending_sync, true);
}
qemu_sem_post(&p->sem);
}
for () {
...
qemu_sem_wait(&multifd_send_state->channels_ready);
qemu_sem_wait(&p->sem_sync);
}
in multifd_send_thread:
if (qatomic_load_acquire(&p->pending_job)) {
...
qatomic_store_release(&p->pending_job, false);
} else if (qatomic_read(&p->pending_sync)) {
...
p->flags = MULTIFD_FLAG_SYNC;
qatomic_set(&p->pending_sync, false);
qemu_sem_post(&p->sem_sync);
} else {
qemu_sem_post(&p->sem_sync);
}
Is this clearer? Then we avoid the enum altogether, a boolean would
suffice.
>
>>
>> > qemu_sem_post(&p->sem);
>> > }
>> > for (i = 0; i < migrate_multifd_channels(); i++) {
>> > @@ -635,14 +635,16 @@ static void *multifd_send_thread(void *opaque)
>> > */
>> > qatomic_store_release(&p->pending_job, false);
>> > } else {
>> > + MultiFDSyncReq req = qatomic_read(&p->pending_sync);
>> > +
>> > /*
>> > * If not a normal job, must be a sync request. Note that
>> > * pending_sync is a standalone flag (unlike pending_job), so
>> > * it doesn't require explicit memory barriers.
>> > */
>> > - assert(qatomic_read(&p->pending_sync));
>> > + assert(req != MULTIFD_SYNC_NONE);
>> >
>> > - if (use_packets) {
>> > + if (req == MULTIFD_SYNC_THREADS_AND_NOTIFY) {
>>
>> Good, more explicit.
>>
>> > p->flags = MULTIFD_FLAG_SYNC;
>> > multifd_send_fill_packet(p);
>> > ret = qio_channel_write_all(p->c, (void *)p->packet,
>> > @@ -654,7 +656,7 @@ static void *multifd_send_thread(void *opaque)
>> > stat64_add(&mig_stats.multifd_bytes, p->packet_len);
>> > }
>> >
>> > - qatomic_set(&p->pending_sync, false);
>> > + qatomic_set(&p->pending_sync, MULTIFD_SYNC_NONE);
>>
