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

[Ashijeet Acharya]

On Wed, Sep 14, 2016 at 12:52 AM, Eric Blake <address@hidden> wrote:
> 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...
>

Okay, I will improve the commit message.

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

I used SIZE_MAX because substituting it with its value looked a bit messy.
Should I use its actual value?

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

Yeah, I had a discussion with Dave about this one on the IRC.
Although 2000 seconds is a sufficiently large value for downtime as we
practically only use values in range of milliseconds but I will
include it in the commit message.

>
>> +        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"
>

Okay.

>> +
>> +        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 understand, but I left 'max_downtime' as it is so that I don't have
to substitute it everywhere in the file and add unnecessary additions
and deletions in the patch but I will remove it now as you recommend.

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

Dropping the old note seemed reasonable to me as the old-commands are
now only a wrapper around the new ones.
So the bounds check error applies on both.

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

Okay, I will send the updated patch soon.

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