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

[Hailiang Zhang <zhang.zhanghailiang@huawei.com>]

On 2015/12/10 2:50, Dr. David Alan Gilbert wrote:
> * 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:
>

OK, i will fix it, thanks.

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