Re: [Qemu-devel] [PATCH v5] migrate: Move max-bandwidth and downtime-limit to migrate_set_parameter

[Eric Blake]

On 09/09/2016 02:02 PM, Ashijeet Acharya wrote:
> Mark old-commands for speed and downtime as deprecated.

Maybe s/old-commands for speed and downtime/the old commands
'migrate_set_speed' and 'migrate_set_downtime'/

> Move max-bandwidth and downtime-limit into migrate-set-parameters for
> setting maximum migration speed and expected downtime limit parameters
> respectively.
> Change downtime units to milliseconds (only for new-command) and update
> the query part in both hmp and qmp qemu control interfaces.

This part is fine.

> NOTE: This patch is solely based on Eric's new boxed parameters from QAPI
> patch series.
> 

This paragraph is useful to reviewers, but won't make sense in the long
run (as my patch series will presumably already be in git first, since
yours does indeed depend on mine), so it can go better...

> Signed-off-by: Ashijeet Acharya <address@hidden>
> ---

...here, along with your changelog.

> @@ -1295,6 +1307,20 @@ void hmp_migrate_set_parameter(Monitor *mon, const 
> QDict *qdict)
>                  p.has_tls_hostname = true;
>                  p.tls_hostname = (char *) valuestr;
>                  break;
> +            case MIGRATION_PARAMETER_MAX_BANDWIDTH:
> +                p.has_max_bandwidth = true;
> +                valuebw = qemu_strtosz(valuestr, &endp);
> +                if (valuebw < 0 || (size_t)valuebw != valuebw
> +                    || *endp != '\0') {
> +                    error_setg(&err, "Invalid size %s", valuestr);
> +                goto cleanup;

Indentation is off for the goto.

> 
> +++ b/migration/migration.c
> @@ -44,6 +44,10 @@
>  #define BUFFER_DELAY     100
>  #define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY)
>  
> +/* Time in nanoseconds we are allowed to stop the source,
> + * for sending the last part */
> +#define DEFAULT_MIGRATE_SET_DOWNTIME 300000000
> +

I would have expected that the 'static uint64_t max_downtime'
declaration would completely disappear, now that you are moving all the
data to live inside the rest of the migration parameters.  More on that
below [1]

> @@ -804,6 +813,20 @@ void qmp_migrate_set_parameters(MigrationParameters 
> *params, Error **errp)
>                     "cpu_throttle_increment",
>                     "an integer in the range of 1 to 99");
>      }
> +    if (params->has_max_bandwidth &&
> +        (params->max_bandwidth < 0 || params->max_bandwidth > SIZE_MAX)) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> +                   "max_bandwidth",
> +                   "an integer in the range of 0 to SIZE_MAX");

Might be nicer to give a numeric value instead of assuming the reader
knows what value SIZE_MAX has, but not the end of the world.

> +        return;
> +    }
> +    if (params->has_downtime_limit &&
> +        (params->downtime_limit < 0 || params->downtime_limit > 2000000)) {

You are setting a new maximum limit smaller than what the code
previously allowed; while I think that 2000 seconds as a maximum
downtime is indeed more generous than anyone will ever use in real life,
you should at least document in the commit message that your new smaller
upper limit is intentional.

> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> +                   "downtime_limit",
> +                   "an integer in the range of 0 to 2000000 milliseconds");
> +        return;
> +    }
>  
>      if (params->has_compress_level) {
>          s->parameters.compress_level = params->compress_level;
> @@ -828,6 +851,20 @@ void qmp_migrate_set_parameters(MigrationParameters 
> *params, Error **errp)
>          g_free(s->parameters.tls_hostname);
>          s->parameters.tls_hostname = g_strdup(params->tls_hostname);
>      }
> +    if (params->has_max_bandwidth) {
> +        s->parameters.max_bandwidth = params->max_bandwidth;
> +        if (s->to_dst_file) {
> +            qemu_file_set_rate_limit(s->to_dst_file,
> +                                s->parameters.max_bandwidth / 
> XFER_LIMIT_RATIO);
> +        }
> +    }
> +    if (params->has_downtime_limit) {
> +        params->downtime_limit *= 1000000; /* convert to nanoseconds */

I'd update the comment to include an additional phrase: "safe from
overflow, since we capped the upper limit above"

> +
> +        s = migrate_get_current();
> +        max_downtime = params->downtime_limit;
> +        s->parameters.downtime_limit = max_downtime;

[1] Uggh.  s->parameters shares the same type as params
(MigrationParameters), but we are using it to hold a limit in
nanoseconds in one instance and a limit in milliseconds in the other.
Which means we have to scale any time we assign from one field to the
other, and cannot use simple copies between the two locations.  What's
more, you are then STILL using the file-level variable 'max_downtime'
(in nanoseconds) rather than the new s->parameters.downtime_limit.  If
s->parameters is good enough, then we don't need the file-scope
'max_downtime'.

I would MUCH rather see us consistently use the SAME scale (milliseconds
is fine), where a single point of documentation in the .json file
correctly describes ALL uses of the downtime_limit member of
MigrationParameters.  Even if that means splitting this into a
multi-patch series (one to convert all existing uses of max_downtime to
a scale of milliseconds, the second to then convert from max_downtime
over to the new s->parameters.downtime_limit).

>  
> @@ -1159,30 +1196,27 @@ int64_t qmp_query_migrate_cache_size(Error **errp)
>      return migrate_xbzrle_cache_size();
>  }
>  
> -void qmp_migrate_set_speed(int64_t value, Error **errp)
> +void qmp_migrate_set_speed(int64_t valuebw, Error **errp)

Why did we need to rename value to valuebw?

>  {
> -    MigrationState *s;
> -
> -    if (value < 0) {
> -        value = 0;
> -    }
> -    if (value > SIZE_MAX) {
> -        value = SIZE_MAX;
> -    }
> +    MigrationParameters p = {
> +        .has_max_bandwidth = true,
> +        .max_bandwidth = valuebw,
> +    };
>  
> -    s = migrate_get_current();
> -    s->bandwidth_limit = value;
> -    if (s->to_dst_file) {
> -        qemu_file_set_rate_limit(s->to_dst_file,
> -                                 s->bandwidth_limit / XFER_LIMIT_RATIO);
> -    }
> +    qmp_migrate_set_parameters(&p, errp);
>  }
>  
> -void qmp_migrate_set_downtime(double value, Error **errp)
> +void qmp_migrate_set_downtime(double valuedowntime, Error **errp)

Again, why rename value?

> +++ b/qapi-schema.json

> @@ -1782,6 +1797,8 @@
>  #
>  # Returns: nothing on success
>  #
> +# Notes: This command is deprecated in favor of 'migrate-set-parameters'
> +#
>  # Since: 0.14.0
>  ##
>  { 'command': 'migrate_set_downtime', 'data': {'value': 'number'} }
> @@ -1795,7 +1812,7 @@
>  #
>  # Returns: nothing on success
>  #
> -# Notes: A value lesser than zero will be automatically round up to zero.
> +# Notes: This command is deprecated in favor of 'migrate-set-parameters'

Do we need to drop the old note about behavior on negative inputs?  On
the one hand, the new interface doesn't suffer from that interface wart,
so the old interface is the only place where the note is even useful; on
the other hand, since we don't want people to use the old interface, not
documenting the wart seems fine to me.

> @@ -3813,7 +3815,7 @@ EQMP
>      {
>          .name       = "migrate-set-parameters",
>          .args_type  =
> -            
> "compress-level:i?,compress-threads:i?,decompress-threads:i?,cpu-throttle-initial:i?,cpu-throttle-increment:i?",
> +            
> "compress-level:i?,compress-threads:i?,decompress-threads:i?,cpu-throttle-initial:i?,cpu-throttle-increment:i?,max-bandwidth:i?,downtime-limit:i?",

Trivial conflict with Marc-Andre's series on qapi-next that removes
.args_type altogether.

Getting closer, but I still think re-scaling max_downtime should be done
separately from moving it into MigrationParameters, and that we
absolutely do not want to use two different scales for various
MigrationParameters->downtime_limit uses.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature