Juan Quintela <quint...@redhat.com> writes:
> Markus Armbruster <arm...@redhat.com> wrote:
>> Juan Quintela <quint...@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 <quint...@redhat.com>
>>> Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
>>> Signed-off-by: Juan Quintela <quint...@redhat.com>
>>>
>>> ---
>>>
>>> Rename each-iteration to after-each-section
>>>
>>> Signed-off-by: Juan Quintela <quint...@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.
