Re: [PATCH v2 08/11] multifd: Add capability to enable/disable zero_page

[Juan Quintela <quintela@redhat.com>]

Markus Armbruster <armbru@redhat.com> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> We have to enable it by default until we introduce the new code.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> The subject doesn't quite match the patch to the QAPI schema.  It claims
> "capability to enable/disable zero_page", but ...


And here I am, making a full of myself (again).

Will change the documentation/commit information, what is right is the
code.

Thanks a lot.

Later, Juan.

>
>> ---
>>
>> Change it to a capability.  As capabilities are off by default, have
>> to change MULTIFD_ZERO_PAGE to MAIN_ZERO_PAGE, so it is false for
>> default, and true for older versions.
>> ---
>>  qapi/migration.json   |  8 +++++++-
>>  migration/migration.h |  1 +
>>  hw/core/machine.c     |  1 +
>>  migration/migration.c | 13 ++++++++++++-
>>  4 files changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index 88ecf86ac8..ac5bc071a9 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -472,12 +472,18 @@
>>  #                  Requires that QEMU be permitted to use locked memory
>>  #                  for guest RAM pages.
>>  #                  (since 7.1)
>> +#
>>  # @postcopy-preempt: If enabled, the migration process will allow postcopy
>>  #                    requests to preempt precopy stream, so postcopy requests
>>  #                    will be handled faster.  This is a performance feature and
>>  #                    should not affect the correctness of postcopy migration.
>>  #                    (since 7.1)
>>  #
>> +# @main-zero-page: If enabled, the detection of zero pages will be
>> +#                  done on the main thread.  Otherwise it is done on
>> +#                  the multifd threads.
>
> ... here, we add a capability to shift certain work to another thread.
> No "enable/disable" as far as I can tell.  Which one is right?
>
> What's the default?
>
> Not this patch's fault, but needs fixing: we neglect to document the
> default for several other parameters.
>
> Wordsmithing nitpick: suggest "done by the thread" or maybe "done in the
> thread".
>
> @main-zero-page suggests this is about a special zero page.  Perhaps I
> can think of a clearer name, but first I need to be sure what the thing
> is about.
>
>> +#                  (since 8.0)
>> +#
>>  # Features:
>>  # @unstable: Members @x-colo and @x-ignore-shared are experimental.
>>  #
>> @@ -492,7 +498,7 @@
>>             'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
>>             { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
>>             'validate-uuid', 'background-snapshot',
>> -           'zero-copy-send', 'postcopy-preempt'] }
>> +           'zero-copy-send', 'postcopy-preempt', 'main-zero-page'] }
>>  
>>  ##
>>  # @MigrationCapabilityStatus: