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

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> 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:-)

I'm fine with this approach, as long as commit messages and comments
reflect reality :)

>>> +#                                   (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

s/trhough/through/

> #                                   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?

Yes, except the comment suggests value false does something, which isn't
true, yet.

Possible solutions:

1. Accept only configurations that work as advertized:

   Patch1: add code to reject value false with a suitable "not
   implemented" error message.  Since the behavior is temporary within a
   single series, documenting this feels optional.

   Patch2: no change.

   Patch3: drop the code rejecting false.

2. Document configurations that don't yet work as advertized:

   Patch1: doc comment states the capability is not yet implemented.

   Patch2: no change.

   Patch3: drop the comment.

No need to mess with documenting temporary default true in either case.

>>> +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.

Matter of taste, you pick what you like best.

I'd simply start with

      return true;              /* TODO implement */

and replace it with the real expression when its callers are ready for
it.