Re: [PATCH v5 1/3] multifd: Create property multifd-sync-after-each-section

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> We used to synchronize all channels at the end of each RAM section
>> sent.  That is not needed, so preparing to only synchronize once every
>> full round in latests patches.
>>
>> Notice that we initialize the property as true.  We will change the
>> default when we introduce the new mechanism.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>
>> ---
>>
>> Rename each-iteration to after-each-section
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  qapi/migration.json   | 10 +++++++++-
>>  migration/migration.h |  1 +
>>  hw/core/machine.c     |  1 +
>>  migration/migration.c | 15 +++++++++++++--
>>  4 files changed, 24 insertions(+), 3 deletions(-)
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index c84fa10e86..2907241b9c 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -478,6 +478,13 @@
>>  #                    should not affect the correctness of postcopy 
>> migration.
>>  #                    (since 7.1)
>>  #
>> +# @multifd-sync-after-each-section: Synchronize channels after each
>> +#                                   section is sent.
>
> What does it mean to synchronize channels?
>
> When would I want to, and why?
>
>> +#                                                     We used to do
>> +#                                   that in the past, but it is
>> +#                                   suboptimal.
>
> This isn't particularly helpful, I'm afraid.
>
>> +#                                   Default value is true until all code is 
>> in.
>
> As far as I can tell, it's actually *unused* for now, and a later patch
> will put it to use ...

We (well, libvert preffers) want capabilities to be false by default.
When I introduce a new capability/parameter:
- Patch1: I introduce the capability/parameter, it does nothing yet.
- Patch2: I conditionalize the old code on this capability.
          Default value is true (old code).
- Patch3: I introduce the new code to implement the feature.
          At this point I change the default.

Depending on complexity, Patch2 and 3 can be a series, but you get the
idea O:-)

>> +#                                   (since 8.0)

Retry.  What about:

# @multifd-sync-after-each-section: flush each channel after each
#                                   section sent.  This assures that
#                                   we can't mix pages from one
#                                   iteration through the dirty bitmap
#                                   with pages for the following
#                                   iteration.  We really only need to
#                                   do this flush after we have go
#                                   trhough all the dirty bitmap.  For
#                                   historical reasons, we do that after
#                                   each section.  This is suboptimal
#                                   (we flush too many times).
#                                   Default value is true until the code
#                                   to implement it is in tree.
#                                   (since 8.0)


Better?

>> +bool migrate_multifd_sync_after_each_section(void)
>> +{
>> +    MigrationState *s = migrate_get_current();
>> +
>> +    return true;
>> +    // We will change this when code gets in.
>> +    return 
>> s->enabled_capabilities[MIGRATION_CAPABILITY_MULTIFD_SYNC_AFTER_EACH_SECTION];
>
> ... here.
>
> No warning about unreachable code?  Checking... nope, gcc seems to not
> to care.

Yeap.  Gcc thinks this is ok.
In others try's I have done:

    return true || 
s->enabled_capabilities[MIGRATION_CAPABILITY_MULTIFD_SYNC_AFTER_EACH_SECTION];

If you preffer I can change to this, not strong opinions.

Later, Juan.