Re: [PATCH v2 06/29] migration: Add auto-pause capability

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Oct 23, 2023 at 05:35:45PM -0300, Fabiano Rosas wrote:
> Add a capability that allows the management layer to delegate to QEMU
> the decision of whether to pause a VM and perform a non-live
> migration. Depending on the type of migration being performed, this
> could bring performance benefits.

I'm not really see what problem this is solving.

Mgmt apps are perfectly capable of pausing the VM before issuing
the migrate operation.

IMHO changing the default pause behaviour when certain migration
capabilties are set creates an unneccessary suprise for mgmt
apps. Having to then also add an extra capability to turn off
this new feature just adds to the migration maint burden.

> 
> Note that the capability is enabled by default but at this moment no
> migration scheme is making use of it.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/migration.c | 19 +++++++++++++++++++
>  migration/options.c   |  9 +++++++++
>  migration/options.h   |  1 +
>  qapi/migration.json   |  6 +++++-
>  4 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index a6efbd837a..8b0c3b0911 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -124,6 +124,20 @@ migration_channels_and_uri_compatible(const char *uri, Error **errp)
>      return true;
>  }
>  
> +static bool migration_should_pause(const char *uri)
> +{
> +    if (!migrate_auto_pause()) {
> +        return false;
> +    }
> +
> +    /*
> +     * Return true for migration schemes that benefit from a nonlive
> +     * migration.
> +     */
> +
> +    return false;
> +}
> +
>  static gint page_request_addr_cmp(gconstpointer ap, gconstpointer bp)
>  {
>      uintptr_t a = (uintptr_t) ap, b = (uintptr_t) bp;
> @@ -1724,6 +1738,11 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>          }
>      }
>  
> +    if (migration_should_pause(uri)) {
> +        global_state_store();
> +        vm_stop_force_state(RUN_STATE_PAUSED);
> +    }
> +
>      if (strstart(uri, "tcp:", &p) ||
>          strstart(uri, "unix:", NULL) ||
>          strstart(uri, "vsock:", NULL)) {
> diff --git a/migration/options.c b/migration/options.c
> index 42fb818956..c3def757fe 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -200,6 +200,8 @@ Property migration_properties[] = {
>      DEFINE_PROP_MIG_CAP("x-switchover-ack",
>                          MIGRATION_CAPABILITY_SWITCHOVER_ACK),
>      DEFINE_PROP_MIG_CAP("x-dirty-limit", MIGRATION_CAPABILITY_DIRTY_LIMIT),
> +    DEFINE_PROP_BOOL("x-auto-pause", MigrationState,
> +                     capabilities[MIGRATION_CAPABILITY_AUTO_PAUSE], true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -210,6 +212,13 @@ bool migrate_auto_converge(void)
>      return s->capabilities[MIGRATION_CAPABILITY_AUTO_CONVERGE];
>  }
>  
> +bool migrate_auto_pause(void)
> +{
> +    MigrationState *s = migrate_get_current();
> +
> +    return s->capabilities[MIGRATION_CAPABILITY_AUTO_PAUSE];
> +}
> +
>  bool migrate_background_snapshot(void)
>  {
>      MigrationState *s = migrate_get_current();
> diff --git a/migration/options.h b/migration/options.h
> index 237f2d6b4a..d1ba5c9de7 100644
> --- a/migration/options.h
> +++ b/migration/options.h
> @@ -24,6 +24,7 @@ extern Property migration_properties[];
>  /* capabilities */
>  
>  bool migrate_auto_converge(void);
> +bool migrate_auto_pause(void);
>  bool migrate_background_snapshot(void);
>  bool migrate_block(void);
>  bool migrate_colo(void);
> diff --git a/qapi/migration.json b/qapi/migration.json
> index db3df12d6c..74f12adc0e 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -523,6 +523,10 @@
>  #     and can result in more stable read performance.  Requires KVM
>  #     with accelerator property "dirty-ring-size" set.  (Since 8.1)
>  #
> +# @auto-pause: If enabled, allows QEMU to decide whether to pause the
> +#     VM before migration for an optimal migration performance.
> +#     Enabled by default. (since 8.1)
> +#
>  # Features:
>  #
>  # @unstable: Members @x-colo and @x-ignore-shared are experimental.
> @@ -539,7 +543,7 @@
>             { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
>             'validate-uuid', 'background-snapshot',
>             'zero-copy-send', 'postcopy-preempt', 'switchover-ack',
> -           'dirty-limit'] }
> +           'dirty-limit', 'auto-pause'] }
>  
>  ##
>  # @MigrationCapabilityStatus:
> -- 
> 2.35.3
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|