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

[Ashijeet Acharya]

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

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

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

Thinking about what you said about re-scaling a bit more, since I will
be dropping the conversion of 'downtime' to nanoseconds part, there
will be no issue of overflow anymore at that point and can drop the
bounds check along with the comments. Although, I think its
appropriate to move the bounds check code below to
qmp_migrate_set_downtime() while multiplying with '1000' to convert to
milliseconds (this is applicable only for the old commads). After this
dropping 'max_downtime' and other substitutions will work quite fine.

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