Re: [Qemu-devel] [PATCH COLO-Frame v11 19/39] COLO: Add checkpoint-delay parameter for migrate-set-parameters

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* zhanghailiang (zhang.zhanghailiang@huawei.com) wrote:
> Add checkpoint-delay parameter for migrate-set-parameters, so that
> we can control the checkpoint frequency when COLO is in periodic mode.
> 
> Cc: Luiz Capitulino <lcapitulino@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>

It's probably a good idea to make this x-  but other than that:

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


> ---
> v11:
> - Move this patch ahead of the patch where uses 'checkpoint_delay'
>  (Dave's suggestion)
> v10:
> - Fix related qmp command
> ---
>  hmp.c                 |  7 +++++++
>  migration/migration.c | 23 ++++++++++++++++++++++-
>  qapi-schema.json      | 19 ++++++++++++++++---
>  qmp-commands.hx       |  2 +-
>  4 files changed, 46 insertions(+), 5 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index 2140605..ec038f1 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -284,6 +284,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>          monitor_printf(mon, " %s: %" PRId64,
>              MigrationParameter_lookup[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT],
>              params->x_cpu_throttle_increment);
> +        monitor_printf(mon, " %s: %" PRId64,
> +            MigrationParameter_lookup[MIGRATION_PARAMETER_CHECKPOINT_DELAY],
> +            params->checkpoint_delay);
>          monitor_printf(mon, "\n");
>      }
>  
> @@ -1237,6 +1240,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>      bool has_decompress_threads = false;
>      bool has_x_cpu_throttle_initial = false;
>      bool has_x_cpu_throttle_increment = false;
> +    bool has_checkpoint_delay = false;
>      int i;
>  
>      for (i = 0; i < MIGRATION_PARAMETER_MAX; i++) {
> @@ -1256,6 +1260,8 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>                  break;
>              case MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT:
>                  has_x_cpu_throttle_increment = true;
> +            case MIGRATION_PARAMETER_CHECKPOINT_DELAY:
> +                has_checkpoint_delay = true;
>                  break;
>              }
>              qmp_migrate_set_parameters(has_compress_level, value,
> @@ -1263,6 +1269,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>                                         has_decompress_threads, value,
>                                         has_x_cpu_throttle_initial, value,
>                                         has_x_cpu_throttle_increment, value,
> +                                       has_checkpoint_delay, value,
>                                         &err);
>              break;
>          }
> diff --git a/migration/migration.c b/migration/migration.c
> index a4c690d..48545f3 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -56,6 +56,11 @@
>  /* Migration XBZRLE default cache size */
>  #define DEFAULT_MIGRATE_CACHE_SIZE (64 * 1024 * 1024)
>  
> +/* The delay time (in ms) between two COLO checkpoints
> + * Note: Please change this default value to 10000 when we support hybrid mode.
> + */
> +#define DEFAULT_MIGRATE_CHECKPOINT_DELAY 200
> +
>  static NotifierList migration_state_notifiers =
>      NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
>  
> @@ -91,6 +96,8 @@ MigrationState *migrate_get_current(void)
>                  DEFAULT_MIGRATE_X_CPU_THROTTLE_INITIAL,
>          .parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT] =
>                  DEFAULT_MIGRATE_X_CPU_THROTTLE_INCREMENT,
> +        .parameters[MIGRATION_PARAMETER_CHECKPOINT_DELAY] =
> +                DEFAULT_MIGRATE_CHECKPOINT_DELAY,
>      };
>  
>      if (!once) {
> @@ -530,6 +537,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>              s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL];
>      params->x_cpu_throttle_increment =
>              s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT];
> +    params->checkpoint_delay =
> +            s->parameters[MIGRATION_PARAMETER_CHECKPOINT_DELAY];
>  
>      return params;
>  }
> @@ -736,7 +745,10 @@ void qmp_migrate_set_parameters(bool has_compress_level,
>                                  bool has_x_cpu_throttle_initial,
>                                  int64_t x_cpu_throttle_initial,
>                                  bool has_x_cpu_throttle_increment,
> -                                int64_t x_cpu_throttle_increment, Error **errp)
> +                                int64_t x_cpu_throttle_increment,
> +                                bool has_checkpoint_delay,
> +                                int64_t checkpoint_delay,
> +                                Error **errp)
>  {
>      MigrationState *s = migrate_get_current();
>  
> @@ -771,6 +783,11 @@ void qmp_migrate_set_parameters(bool has_compress_level,
>                     "x_cpu_throttle_increment",
>                     "an integer in the range of 1 to 99");
>      }
> +    if (has_checkpoint_delay && (checkpoint_delay < 0)) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> +                    "checkpoint_delay",
> +                    "is invalid, it should be positive");
> +    }
>  
>      if (has_compress_level) {
>          s->parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL] = compress_level;
> @@ -791,6 +808,10 @@ void qmp_migrate_set_parameters(bool has_compress_level,
>          s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT] =
>                                                      x_cpu_throttle_increment;
>      }
> +
> +    if (has_checkpoint_delay) {
> +        s->parameters[MIGRATION_PARAMETER_CHECKPOINT_DELAY] = checkpoint_delay;
> +    }
>  }
>  
>  void qmp_migrate_start_postcopy(Error **errp)
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5ee089d..b4b47ed 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -625,11 +625,16 @@
>  # @x-cpu-throttle-increment: throttle percentage increase each time
>  #                            auto-converge detects that migration is not making
>  #                            progress. The default value is 10. (Since 2.5)
> +#
> +# @checkpoint-delay: The delay time (in ms) between two COLO checkpoints in
> +#          periodic mode. (Since 2.6)
> +#
>  # Since: 2.4
>  ##
>  { 'enum': 'MigrationParameter',
>    'data': ['compress-level', 'compress-threads', 'decompress-threads',
> -           'x-cpu-throttle-initial', 'x-cpu-throttle-increment'] }
> +           'x-cpu-throttle-initial', 'x-cpu-throttle-increment',
> +           'checkpoint-delay' ] }
>  
>  #
>  # @migrate-set-parameters
> @@ -649,6 +654,9 @@
>  # @x-cpu-throttle-increment: throttle percentage increase each time
>  #                            auto-converge detects that migration is not making
>  #                            progress. The default value is 10. (Since 2.5)
> +#
> +# @checkpoint-delay: the delay time between two checkpoints. (Since 2.6)
> +#
>  # Since: 2.4
>  ##
>  { 'command': 'migrate-set-parameters',
> @@ -656,7 +664,8 @@
>              '*compress-threads': 'int',
>              '*decompress-threads': 'int',
>              '*x-cpu-throttle-initial': 'int',
> -            '*x-cpu-throttle-increment': 'int'} }
> +            '*x-cpu-throttle-increment': 'int',
> +            '*checkpoint-delay': 'int' } }
>  
>  #
>  # @MigrationParameters
> @@ -675,6 +684,8 @@
>  #                            auto-converge detects that migration is not making
>  #                            progress. The default value is 10. (Since 2.5)
>  #
> +# @checkpoint-delay: the delay time between two COLO checkpoints
> +#
>  # Since: 2.4
>  ##
>  { 'struct': 'MigrationParameters',
> @@ -682,7 +693,9 @@
>              'compress-threads': 'int',
>              'decompress-threads': 'int',
>              'x-cpu-throttle-initial': 'int',
> -            'x-cpu-throttle-increment': 'int'} }
> +            'x-cpu-throttle-increment': 'int',
> +            'checkpoint-delay': 'int'} }
> +
>  ##
>  # @query-migrate-parameters
>  #
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 92042d8..c39f2f0 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3664,7 +3664,7 @@ EQMP
>      {
>          .name       = "migrate-set-parameters",
>          .args_type  =
> -            "compress-level:i?,compress-threads:i?,decompress-threads:i?",
> +            "compress-level:i?,compress-threads:i?,decompress-threads:i?,checkpoint-delay:i?",
>          .mhandler.cmd_new = qmp_marshal_migrate_set_parameters,
>      },
>  SQMP
> -- 
> 1.8.3.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK