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

[Markus Armbruster <armbru@redhat.com>]

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

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