Re: [PATCH v7 06/12] multifd: Make flags field thread local

[Juan Quintela]

Leonardo Bras Soares Passos <leobras@redhat.com> wrote:
> On Fri, Aug 19, 2022 at 7:03 AM Juan Quintela <quintela@redhat.com> wrote:
>>
>> Leonardo Brás <leobras@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 <quintela@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

You have a point here.  Not putting a comment, putting the real thing.

Thanks, Juan.