Re: [PATCH v7 08/12] multifd: Add capability to enable/disable zero_page

[Leonardo Brás]

On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
> We have to enable it by default until we introduce the new code.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> ---
> 
> 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.

IIUC, the idea of a capability is to introduce some new features to the code,
and let users enable or disable it. 

If it introduce a new capability, is not very intuitive to think that it will be
always true for older versions, and false for new ones.

I would suggest adding it as MULTIFD_ZERO_PAGE, and let it disabled for now.
When the full feature gets introduced, the capability could be enabled by
default, if desired.

What do you think?

Best regards,
Leo


> ---
>  qapi/migration.json   |  8 +++++++-
>  migration/migration.h |  1 +
>  hw/core/machine.c     |  1 +
>  migration/migration.c | 16 +++++++++++++++-
>  4 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 81185d4311..dc981236ff 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.
> +#                  (since 7.1)
> +#
>  # 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:
> diff --git a/migration/migration.h b/migration/migration.h
> index cdad8aceaa..58b245b138 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -415,6 +415,7 @@ int migrate_multifd_channels(void);
>  MultiFDCompression migrate_multifd_compression(void);
>  int migrate_multifd_zlib_level(void);
>  int migrate_multifd_zstd_level(void);
> +bool migrate_use_main_zero_page(void);
>  
>  #ifdef CONFIG_LINUX
>  bool migrate_use_zero_copy_send(void);
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index a673302cce..2624b75ab4 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -43,6 +43,7 @@
>  GlobalProperty hw_compat_7_0[] = {
>      { "arm-gicv3-common", "force-8-bit-prio", "on" },
>      { "nvme-ns", "eui64-default", "on"},
> +    { "migration", "main-zero-page", "true" },
>  };
>  const size_t hw_compat_7_0_len = G_N_ELEMENTS(hw_compat_7_0);
>  
> diff --git a/migration/migration.c b/migration/migration.c
> index e03f698a3c..ce3e5cc0cd 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -164,7 +164,8 @@ 
> INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
>      MIGRATION_CAPABILITY_XBZRLE,
>      MIGRATION_CAPABILITY_X_COLO,
>      MIGRATION_CAPABILITY_VALIDATE_UUID,
> -    MIGRATION_CAPABILITY_ZERO_COPY_SEND);
> +    MIGRATION_CAPABILITY_ZERO_COPY_SEND,
> +    MIGRATION_CAPABILITY_MAIN_ZERO_PAGE);
>  
>  /* When we add fault tolerance, we could have several
>     migrations at once.  For now we don't need to add
> @@ -2592,6 +2593,17 @@ bool migrate_use_multifd(void)
>      return s->enabled_capabilities[MIGRATION_CAPABILITY_MULTIFD];
>  }
>  
> +bool migrate_use_main_zero_page(void)
> +{
> +    MigrationState *s;
> +
> +    s = migrate_get_current();
> +
> +    // We will enable this when we add the right code.
> +    // return s->enabled_capabilities[MIGRATION_CAPABILITY_MAIN_ZERO_PAGE];
> +    return true;
> +}
> +
>  bool migrate_pause_before_switchover(void)
>  {
>      MigrationState *s;
> @@ -4406,6 +4418,8 @@ static Property migration_properties[] = {
>      DEFINE_PROP_MIG_CAP("x-zero-copy-send",
>              MIGRATION_CAPABILITY_ZERO_COPY_SEND),
>  #endif
> +    DEFINE_PROP_MIG_CAP("main-zero-page",
> +            MIGRATION_CAPABILITY_MAIN_ZERO_PAGE),
>  
>      DEFINE_PROP_END_OF_LIST(),
>  };