On Fri, Aug 19, 2022 at 7:03 AM Juan Quintela <quint...@redhat.com> wrote:
>
> Leonardo Brás <leob...@redhat.com> wrote:
> > On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
> >> Use of flags with respect to locking was incensistant. For the
> >> sending side:
> >> - it was set to 0 with mutex held on the multifd channel.
> >> - MULTIFD_FLAG_SYNC was set with mutex held on the migration thread.
> >> - Everything else was done without the mutex held on the multifd channel.
> >>
> >> On the reception side, it is not used on the migration thread, only on
> >> the multifd channels threads.
> >>
> >> So we move it to the multifd channels thread only variables, and we
> >> introduce a new bool sync_needed on the send side to pass that information.
> >>
> >> Signed-off-by: Juan Quintela <quint...@redhat.com>
> >> ---
> >> migration/multifd.h | 10 ++++++----
> >> migration/multifd.c | 23 +++++++++++++----------
> >> 2 files changed, 19 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/migration/multifd.h b/migration/multifd.h
> >> index 36f899c56f..a67cefc0a2 100644
> >> --- a/migration/multifd.h
> >> +++ b/migration/multifd.h
> >> @@ -98,12 +98,12 @@ typedef struct {
> >
> > Just noticed having no name in 'typedef struct' line makes it harder to
> > understand what is going on.
>
> It is common idiom in QEMU. The principal reason is that if you don't
> want anyone to use "struct MultiFDSendParams" but MultiFDSendParams, the
> best way to achieve that is to do it this way.
I agree, but a comment after the typedef could help reviewing. Something like
typedef struct { /* MultiFDSendParams */
...
} MultiFDSendParams
Becomes this in diff:
diff --git a/migration/multifd.h b/migration/multifd.h
index 134e6a7f19..93bb3a7f4a 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -90,6 +90,7 @@ typedef struct { /* MultiFDSendParams */
[...]
>
> >> @@ -172,6 +172,8 @@ typedef struct {
> >>
> >> /* pointer to the packet */
> >> MultiFDPacket_t *packet;
> >> + /* multifd flags for each packet */
> >> + uint32_t flags;
> >> /* size of the next packet that contains pages */
> >> uint32_t next_packet_size;
> >> /* packets sent through this channel */
> >
> > So, IIUC, the struct member flags got moved down (same struct) to an area
> > described as thread-local, meaning it does not need locking.
> >
> > Interesting, I haven't noticed this different areas in the same struct.
>
> It has changed in the last two weeks or so in upstream (it has been on
> this patchset for several months.)
Nice :)
>
>
> >
> >> diff --git a/migration/multifd.c b/migration/multifd.c
> >> index e25b529235..09a40a9135 100644
> >> --- a/migration/multifd.c
> >> +++ b/migration/multifd.c
> >> @@ -602,7 +602,7 @@ int multifd_send_sync_main(QEMUFile *f)
> >> }
> >>
> >> p->packet_num = multifd_send_state->packet_num++;
> >> - p->flags |= MULTIFD_FLAG_SYNC;
> >> + p->sync_needed = true;
> >> p->pending_job++;
> >> qemu_mutex_unlock(&p->mutex);
> >> qemu_sem_post(&p->sem);
> >> @@ -658,7 +658,11 @@ static void *multifd_send_thread(void *opaque)
> >>
> >> if (p->pending_job) {
> >> uint64_t packet_num = p->packet_num;
> >> - uint32_t flags = p->flags;
> >> + p->flags = 0;
> >> + if (p->sync_needed) {
> >> + p->flags |= MULTIFD_FLAG_SYNC;
> >> + p->sync_needed = false;
> >> + }
> >
> > Any particular reason why doing p->flags = 0, then p->flags |=
> > MULTIFD_FLAG_SYNC
> > ?
>
> It is a bitmap field, and if there is anything on the future, we need to
> set it. I agree that when there is only one flag, it seems "weird".
>
> > [1] Couldn't it be done without the |= , since it's already being set to
> > zero
> > before? (becoming "p->flags = MULTIFD_FLAG_SYNC" )
>
> As said, easier to modify later, and also easier if we want to setup a
> flag by default.
Yeah, I agree. It makes sense now.
Thanks
>
> I agree that it is a matter of style/taste.
>
> >> p->normal_num = 0;
> >>
> >> if (use_zero_copy_send) {
> >> @@ -680,14 +684,13 @@ static void *multifd_send_thread(void *opaque)
> >> }
> >> }
> >> multifd_send_fill_packet(p);
> >> - p->flags = 0;
> >> p->num_packets++;
> >> p->total_normal_pages += p->normal_num;
> >> p->pages->num = 0;
> >> p->pages->block = NULL;
> >> qemu_mutex_unlock(&p->mutex);
> >>
> >> - trace_multifd_send(p->id, packet_num, p->normal_num, flags,
> >> + trace_multifd_send(p->id, packet_num, p->normal_num, p->flags,
> >> p->next_packet_size);
> >>
> >> if (use_zero_copy_send) {
> >> @@ -715,7 +718,7 @@ static void *multifd_send_thread(void *opaque)
> >> p->pending_job--;
> >> qemu_mutex_unlock(&p->mutex);
> >>
> >> - if (flags & MULTIFD_FLAG_SYNC) {
> >> + if (p->flags & MULTIFD_FLAG_SYNC) {
> >> qemu_sem_post(&p->sem_sync);
> >> }
> >> qemu_sem_post(&multifd_send_state->channels_ready);
> >
> > IIUC it uses p->sync_needed to keep the sync info, instead of the previous
> > flags
> > local var, and thus it can set p->flags = 0 earlier. Seems to not change any
> > behavior AFAICS.
>
> The protection of the global flags was being wrong. That is the reason
> that I decided to change it to the sync_needed.
>
> The problem was that at some point we were still sending a packet (that
> shouldn't have the SYNC flag enabled), but we received a
> multifd_main_sync() and it got enabled anyways. The easier way that I
> found te fix it was this way.
>
> Problem was difficult to detect, that is the reason that I change it
> this way.
Oh, I see.
>
> >> - if (flags & MULTIFD_FLAG_SYNC) {
> >> + if (sync_needed) {
> >> qemu_sem_post(&multifd_recv_state->sem_sync);
> >> qemu_sem_wait(&p->sem_sync);
> >> }
> >
> > Ok, IIUC this part should have the same behavior as before, but using a bool
> > instead of an u32.
>
> I changed it to make sure that we only checked the flags at the
> beggining of the function, with the lock taken.
Thanks for sharing!
Best regards,
Leo
>
> >
> > FWIW:
> > Reviewed-by: Leonardo Bras <leob...@redhat.com>
>
> Thanks, Juan.
>
