All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v3] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp
@ 2016-09-06 13:39 Ashijeet Acharya
  2016-09-07  8:41 ` Juan Quintela
  2016-09-07 22:03 ` Eric Blake
  0 siblings, 2 replies; 8+ messages in thread
From: Ashijeet Acharya @ 2016-09-06 13:39 UTC (permalink / raw)
  To: lcapitulino
  Cc: quintela, amit.shah, eblake, armbru, dgilbert, pbonzini,
	berrange, qemu-devel, Ashijeet Acharya

Mark old-commands for speed and downtime as deprecated.
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.

Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
 hmp.c                         |  26 ++++++++++
 include/migration/migration.h |   1 -
 migration/migration.c         | 116 ++++++++++++++++++++++++++++++++----------
 qapi-schema.json              |  33 ++++++++++--
 qmp-commands.hx               |  14 +++--
 5 files changed, 156 insertions(+), 34 deletions(-)

diff --git a/hmp.c b/hmp.c
index cc2056e..639c3ad 100644
--- a/hmp.c
+++ b/hmp.c
@@ -304,6 +304,12 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, " %s: '%s'",
             MigrationParameter_lookup[MIGRATION_PARAMETER_TLS_HOSTNAME],
             params->tls_hostname ? : "");
+        monitor_printf(mon, " %s: %" PRId64,
+            MigrationParameter_lookup[MIGRATION_PARAMETER_MAX_BANDWIDTH],
+            params->max_bandwidth);
+        monitor_printf(mon, " %s: %" PRId64,
+            MigrationParameter_lookup[MIGRATION_PARAMETER_DOWNTIME_LIMIT],
+            params->downtime_limit);
         monitor_printf(mon, "\n");
     }
 
@@ -1193,6 +1199,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);
 }
 
+/* Kept for old-commands compatibility */
 void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict)
 {
     double value = qdict_get_double(qdict, "value");
@@ -1211,6 +1218,7 @@ void hmp_migrate_set_cache_size(Monitor *mon, const QDict *qdict)
     }
 }
 
+/* Kept for old-commands compatibility */
 void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict)
 {
     int64_t value = qdict_get_int(qdict, "value");
@@ -1251,7 +1259,9 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
 {
     const char *param = qdict_get_str(qdict, "parameter");
     const char *valuestr = qdict_get_str(qdict, "value");
+    int64_t valuebw = 0;
     long valueint = 0;
+    char *endp;
     Error *err = NULL;
     bool has_compress_level = false;
     bool has_compress_threads = false;
@@ -1260,6 +1270,8 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
     bool has_cpu_throttle_increment = false;
     bool has_tls_creds = false;
     bool has_tls_hostname = false;
+    bool has_max_bandwidth = false;
+    bool has_downtime_limit = false;
     bool use_int_value = false;
     int i;
 
@@ -1291,6 +1303,18 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
             case MIGRATION_PARAMETER_TLS_HOSTNAME:
                 has_tls_hostname = true;
                 break;
+            case MIGRATION_PARAMETER_MAX_BANDWIDTH:
+                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;
+                }
+                break;
+            case MIGRATION_PARAMETER_DOWNTIME_LIMIT:
+                has_downtime_limit = true;
+                use_int_value = true;
+                break;
             }
 
             if (use_int_value) {
@@ -1308,6 +1332,8 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
                                        has_cpu_throttle_increment, valueint,
                                        has_tls_creds, valuestr,
                                        has_tls_hostname, valuestr,
+                                       has_max_bandwidth, valuebw,
+                                       has_downtime_limit, valueint,
                                        &err);
             break;
         }
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 3c96623..a5429ee 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -129,7 +129,6 @@ struct MigrationSrcPageRequest {
 
 struct MigrationState
 {
-    int64_t bandwidth_limit;
     size_t bytes_xfer;
     size_t xfer_limit;
     QemuThread thread;
diff --git a/migration/migration.c b/migration/migration.c
index 955d5ee..8f620ee 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -44,6 +44,9 @@
 #define BUFFER_DELAY     100
 #define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY)
 
+/* Time in nanoseconds we are allowed to stop the source for to send last part */
+#define DEFAULT_MIGRATE_SET_DOWNTIME 300000000
+
 /* Default compression thread count */
 #define DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT 8
 /* Default decompression thread count, usually decompression is at
@@ -80,7 +83,6 @@ MigrationState *migrate_get_current(void)
     static bool once;
     static MigrationState current_migration = {
         .state = MIGRATION_STATUS_NONE,
-        .bandwidth_limit = MAX_THROTTLE,
         .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE,
         .mbps = -1,
         .parameters = {
@@ -89,6 +91,8 @@ MigrationState *migrate_get_current(void)
             .decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
             .cpu_throttle_initial = DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL,
             .cpu_throttle_increment = DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT,
+            .max_bandwidth = MAX_THROTTLE,
+            .downtime_limit = DEFAULT_MIGRATE_SET_DOWNTIME,
         },
     };
 
@@ -566,6 +570,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->cpu_throttle_increment = s->parameters.cpu_throttle_increment;
     params->tls_creds = g_strdup(s->parameters.tls_creds);
     params->tls_hostname = g_strdup(s->parameters.tls_hostname);
+    params->max_bandwidth = s->parameters.max_bandwidth;
+    params->downtime_limit = s->parameters.downtime_limit / 1000000;
 
     return params;
 }
@@ -773,6 +779,10 @@ void qmp_migrate_set_parameters(bool has_compress_level,
                                 const char *tls_creds,
                                 bool has_tls_hostname,
                                 const char *tls_hostname,
+                                bool has_max_bandwidth,
+                                int64_t max_bandwidth,
+                                bool has_downtime_limit,
+                                int64_t downtime_limit,
                                 Error **errp)
 {
     MigrationState *s = migrate_get_current();
@@ -808,6 +818,12 @@ void qmp_migrate_set_parameters(bool has_compress_level,
                    "cpu_throttle_increment",
                    "an integer in the range of 1 to 99");
     }
+    if (has_max_bandwidth &&
+            (max_bandwidth < 0 || max_bandwidth > SIZE_MAX)) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   "max_bandwidth",
+                   "invalid value");
+    }
 
     if (has_compress_level) {
         s->parameters.compress_level = compress_level;
@@ -832,6 +848,21 @@ void qmp_migrate_set_parameters(bool has_compress_level,
         g_free(s->parameters.tls_hostname);
         s->parameters.tls_hostname = g_strdup(tls_hostname);
     }
+    if (has_max_bandwidth) {
+        s->parameters.max_bandwidth = max_bandwidth;
+        if (s->to_dst_file) {
+            qemu_file_set_rate_limit(s->to_dst_file,
+                                s->parameters.max_bandwidth / XFER_LIMIT_RATIO);
+        }
+    }
+    if (has_downtime_limit) {
+        downtime_limit *= 1000000; /* convert to nanoseconds */
+
+        s = migrate_get_current();
+
+        max_downtime = downtime_limit;
+        s->parameters.downtime_limit = max_downtime;
+    }
 }
 
 
@@ -1163,30 +1194,63 @@ int64_t qmp_query_migrate_cache_size(Error **errp)
     return migrate_xbzrle_cache_size();
 }
 
-void qmp_migrate_set_speed(int64_t value, Error **errp)
-{
-    MigrationState *s;
-
-    if (value < 0) {
-        value = 0;
-    }
-    if (value > SIZE_MAX) {
-        value = SIZE_MAX;
-    }
-
-    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);
-    }
-}
-
-void qmp_migrate_set_downtime(double value, Error **errp)
-{
-    value *= 1e9;
-    value = MAX(0, MIN(UINT64_MAX, value));
-    max_downtime = (uint64_t)value;
+void qmp_migrate_set_speed(int64_t valuebw, Error **errp)
+{
+    bool has_compress_level = false;
+    bool has_compress_threads = false;
+    bool has_decompress_threads = false;
+    bool has_cpu_throttle_initial = false;
+    bool has_cpu_throttle_increment = false;
+    bool has_tls_creds = false;
+    bool has_tls_hostname = false;
+    bool has_max_bandwidth = true;
+    bool has_downtime_limit = false;
+    const char *valuestr = NULL;
+    long valueint = 0;
+    Error *err = NULL;
+
+    qmp_migrate_set_parameters(has_compress_level, valueint,
+                               has_compress_threads, valueint,
+                               has_decompress_threads, valueint,
+                               has_cpu_throttle_initial, valueint,
+                               has_cpu_throttle_increment, valueint,
+                               has_tls_creds, valuestr,
+                               has_tls_hostname, valuestr,
+                               has_max_bandwidth, valuebw,
+                               has_downtime_limit, valueint,
+                               &err);
+
+}
+
+void qmp_migrate_set_downtime(double valuedowntime, Error **errp)
+{
+    bool has_compress_level = false;
+    bool has_compress_threads = false;
+    bool has_decompress_threads = false;
+    bool has_cpu_throttle_initial = false;
+    bool has_cpu_throttle_increment = false;
+    bool has_tls_creds = false;
+    bool has_tls_hostname = false;
+    bool has_max_bandwidth = false;
+    bool has_downtime_limit = true;
+    const char *valuestr = NULL;
+    long valueint = 0;
+    int64_t valuebw = 0;
+    valuedowntime *= 1000; /* Convert to milliseconds */
+    valuedowntime = MAX(0, MIN(INT64_MAX, valuedowntime));
+    valuedowntime = (int64_t)valuedowntime;
+    Error *err = NULL;
+
+    qmp_migrate_set_parameters(has_compress_level, valueint,
+                               has_compress_threads, valueint,
+                               has_decompress_threads, valueint,
+                               has_cpu_throttle_initial, valueint,
+                               has_cpu_throttle_increment, valueint,
+                               has_tls_creds, valuestr,
+                               has_tls_hostname, valuestr,
+                               has_max_bandwidth, valuebw,
+                               has_downtime_limit, valuedowntime,
+                               &err);
 }
 
 bool migrate_postcopy_ram(void)
@@ -1858,7 +1922,7 @@ void migrate_fd_connect(MigrationState *s)
 
     qemu_file_set_blocking(s->to_dst_file, true);
     qemu_file_set_rate_limit(s->to_dst_file,
-                             s->bandwidth_limit / XFER_LIMIT_RATIO);
+                             s->parameters.max_bandwidth / XFER_LIMIT_RATIO);
 
     /* Notify before starting migration thread */
     notifier_list_notify(&migration_state_notifiers, s);
diff --git a/qapi-schema.json b/qapi-schema.json
index 5658723..a8ee2d4 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -637,12 +637,19 @@
 #                hostname must be provided so that the server's x509
 #                certificate identity can be validated. (Since 2.7)
 #
+# @max-bandwidth: to set maximum speed for migration. maximum speed in
+#                 bytes per second. (Since 2.8)
+#
+# @downtime-limit: set maximum tolerated downtime for migration. maximum downtime
+#                  in milliseconds (Since 2.8)
+#
 # Since: 2.4
 ##
 { 'enum': 'MigrationParameter',
   'data': ['compress-level', 'compress-threads', 'decompress-threads',
            'cpu-throttle-initial', 'cpu-throttle-increment',
-           'tls-creds', 'tls-hostname'] }
+           'tls-creds', 'tls-hostname', 'max-bandwidth',
+           'downtime-limit'] }
 
 #
 # @migrate-set-parameters
@@ -678,6 +685,12 @@
 #                hostname must be provided so that the server's x509
 #                certificate identity can be validated. (Since 2.7)
 #
+# @max-bandwidth: to set maximum speed for migration. maximum speed in
+#                 bytes per second. (Since 2.8)
+#
+# @downtime-limit: set maximum tolerated downtime for migration. maximum downtime
+#                  in milliseconds (Since 2.8)
+#
 # Since: 2.4
 ##
 { 'command': 'migrate-set-parameters',
@@ -687,7 +700,9 @@
             '*cpu-throttle-initial': 'int',
             '*cpu-throttle-increment': 'int',
             '*tls-creds': 'str',
-            '*tls-hostname': 'str'} }
+            '*tls-hostname': 'str',
+            '*max-bandwidth': 'int',
+            '*downtime-limit': 'int'} }
 
 #
 # @MigrationParameters
@@ -721,6 +736,12 @@
 #                hostname must be provided so that the server's x509
 #                certificate identity can be validated. (Since 2.7)
 #
+# @max-bandwidth: to set maximum speed for migration. maximum speed in
+#                 bytes per second. (Since 2.8)
+#
+# @downtime-limit: set maximum tolerated downtime for migration. maximum downtime
+#                  in milliseconds (Since 2.8)
+#
 # Since: 2.4
 ##
 { 'struct': 'MigrationParameters',
@@ -730,7 +751,9 @@
             'cpu-throttle-initial': 'int',
             'cpu-throttle-increment': 'int',
             'tls-creds': 'str',
-            'tls-hostname': 'str'} }
+            'tls-hostname': 'str',
+            'max-bandwidth': 'int',
+            'downtime-limit': 'int'} }
 ##
 # @query-migrate-parameters
 #
@@ -1812,6 +1835,8 @@
 #
 # Returns: nothing on success
 #
+# Notes: This command is deprecated in favour of 'migrate-set-parameters'
+#
 # Since: 0.14.0
 ##
 { 'command': 'migrate_set_downtime', 'data': {'value': 'number'} }
@@ -1825,7 +1850,7 @@
 #
 # Returns: nothing on success
 #
-# Notes: A value lesser than zero will be automatically round up to zero.
+# Notes: This command is deprecated in favour of 'migrate-set-parameters'
 #
 # Since: 0.14.0
 ##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6866264..8b050d0 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3790,7 +3790,9 @@ Set migration parameters
                           throttled for auto-converge (json-int)
 - "cpu-throttle-increment": set throttle increasing percentage for
                             auto-converge (json-int)
-
+- "max-bandwidth": set maximum speed for migrations (in bytes) (json-int)
+- "downtime-limit": set maximum tolerated downtime (in milliseconds) for
+                          migrations (json-int)
 Arguments:
 
 Example:
@@ -3803,7 +3805,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?",
         .mhandler.cmd_new = qmp_marshal_migrate_set_parameters,
     },
 SQMP
@@ -3820,6 +3822,10 @@ Query current migration parameters
                                     throttled (json-int)
          - "cpu-throttle-increment" : throttle increasing percentage for
                                       auto-converge (json-int)
+         - "max-bandwidth" : maximium migration speed in s/bytes/bytes per second
+                             (json-int)
+         - "downtime-limit" : maximum tolerated downtime of migration in
+                              milliseconds (json-int)
 
 Arguments:
 
@@ -3832,7 +3838,9 @@ Example:
          "cpu-throttle-increment": 10,
          "compress-threads": 8,
          "compress-level": 1,
-         "cpu-throttle-initial": 20
+         "cpu-throttle-initial": 20,
+         "max-bandwidth": 33554432,
+         "downtime-limit": 300
       }
    }
 
-- 
2.6.2

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v3] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp
  2016-09-06 13:39 [Qemu-devel] [PATCH v3] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp Ashijeet Acharya
@ 2016-09-07  8:41 ` Juan Quintela
  2016-09-07 12:52   ` Ashijeet Acharya
  2016-09-07 22:03 ` Eric Blake
  1 sibling, 1 reply; 8+ messages in thread
From: Juan Quintela @ 2016-09-07  8:41 UTC (permalink / raw)
  To: Ashijeet Acharya
  Cc: lcapitulino, amit.shah, eblake, armbru, dgilbert, pbonzini,
	berrange, qemu-devel

Ashijeet Acharya <ashijeetacharya@gmail.com> wrote:
> Mark old-commands for speed and downtime as deprecated.
> 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.
>
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>


Thanks, applied for 2.8.

Later, Juan.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v3] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp
  2016-09-07  8:41 ` Juan Quintela
@ 2016-09-07 12:52   ` Ashijeet Acharya
  0 siblings, 0 replies; 8+ messages in thread
From: Ashijeet Acharya @ 2016-09-07 12:52 UTC (permalink / raw)
  To: quintela
  Cc: lcapitulino, amit.shah, eblake, armbru, dgilbert, Paolo Bonzini,
	Daniel P. Berrange, QEMU Developers

On Wed, Sep 7, 2016 at 2:11 PM, Juan Quintela <quintela@redhat.com> wrote:
> Ashijeet Acharya <ashijeetacharya@gmail.com> wrote:
>> Mark old-commands for speed and downtime as deprecated.
>> 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.
>>
>> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
>
>
> Thanks, applied for 2.8.

Great! Thanks a lot.

Ashijeet
>
> Later, Juan.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v3] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp
  2016-09-06 13:39 [Qemu-devel] [PATCH v3] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp Ashijeet Acharya
  2016-09-07  8:41 ` Juan Quintela
@ 2016-09-07 22:03 ` Eric Blake
  2016-09-08  7:55   ` Ashijeet Acharya
  2016-09-08 14:41   ` Juan Quintela
  1 sibling, 2 replies; 8+ messages in thread
From: Eric Blake @ 2016-09-07 22:03 UTC (permalink / raw)
  To: Ashijeet Acharya, lcapitulino
  Cc: quintela, amit.shah, armbru, dgilbert, pbonzini, berrange, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 7601 bytes --]

On 09/06/2016 08:39 AM, Ashijeet Acharya wrote:
> Mark old-commands for speed and downtime as deprecated.
> 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.
> 
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---

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

Long line.  Also, a grammar issue:

s/source for to send/source, for sending the/

> @@ -832,6 +848,21 @@ void qmp_migrate_set_parameters(bool has_compress_level,
>          g_free(s->parameters.tls_hostname);
>          s->parameters.tls_hostname = g_strdup(tls_hostname);
>      }
> +    if (has_max_bandwidth) {
> +        s->parameters.max_bandwidth = max_bandwidth;
> +        if (s->to_dst_file) {
> +            qemu_file_set_rate_limit(s->to_dst_file,
> +                                s->parameters.max_bandwidth / XFER_LIMIT_RATIO);
> +        }
> +    }
> +    if (has_downtime_limit) {
> +        downtime_limit *= 1000000; /* convert to nanoseconds */

Are you sure this won't overflow?

> +void qmp_migrate_set_speed(int64_t valuebw, Error **errp)
> +{
> +    bool has_compress_level = false;
> +    bool has_compress_threads = false;
> +    bool has_decompress_threads = false;
> +    bool has_cpu_throttle_initial = false;
> +    bool has_cpu_throttle_increment = false;
> +    bool has_tls_creds = false;
> +    bool has_tls_hostname = false;
> +    bool has_max_bandwidth = true;
> +    bool has_downtime_limit = false;
> +    const char *valuestr = NULL;
> +    long valueint = 0;
> +    Error *err = NULL;
> +
> +    qmp_migrate_set_parameters(has_compress_level, valueint,
> +                               has_compress_threads, valueint,

Ugg. This looks gross.  No need to name a bunch of variables set to
false, when you can instead use QAPI's boxed conventions to pass a
pointer to a struct, and rely on 0-initialization of the struct to leave
all the parameters that you don't care about unmentioned.

> +                               has_decompress_threads, valueint,
> +                               has_cpu_throttle_initial, valueint,
> +                               has_cpu_throttle_increment, valueint,
> +                               has_tls_creds, valuestr,
> +                               has_tls_hostname, valuestr,
> +                               has_max_bandwidth, valuebw,
> +                               has_downtime_limit, valueint,
> +                               &err);
> +
> +}
> +
> +void qmp_migrate_set_downtime(double valuedowntime, Error **errp)
> +{
> +    bool has_compress_level = false;
> +    bool has_compress_threads = false;
> +    bool has_decompress_threads = false;
> +    bool has_cpu_throttle_initial = false;
> +    bool has_cpu_throttle_increment = false;
> +    bool has_tls_creds = false;
> +    bool has_tls_hostname = false;
> +    bool has_max_bandwidth = false;
> +    bool has_downtime_limit = true;

Again, gross.

> +    const char *valuestr = NULL;
> +    long valueint = 0;
> +    int64_t valuebw = 0;
> +    valuedowntime *= 1000; /* Convert to milliseconds */
> +    valuedowntime = MAX(0, MIN(INT64_MAX, valuedowntime));
> +    valuedowntime = (int64_t)valuedowntime;

Useless statement; the cast would already implicitly happen when passing
it to the function call.

> +    Error *err = NULL;
> +
> +    qmp_migrate_set_parameters(has_compress_level, valueint,
> +                               has_compress_threads, valueint,
> +                               has_decompress_threads, valueint,
> +                               has_cpu_throttle_initial, valueint,
> +                               has_cpu_throttle_increment, valueint,
> +                               has_tls_creds, valuestr,
> +                               has_tls_hostname, valuestr,
> +                               has_max_bandwidth, valuebw,
> +                               has_downtime_limit, valuedowntime,
> +                               &err);
>  }
>  
>  bool migrate_postcopy_ram(void)
> @@ -1858,7 +1922,7 @@ void migrate_fd_connect(MigrationState *s)
>  
>      qemu_file_set_blocking(s->to_dst_file, true);
>      qemu_file_set_rate_limit(s->to_dst_file,
> -                             s->bandwidth_limit / XFER_LIMIT_RATIO);
> +                             s->parameters.max_bandwidth / XFER_LIMIT_RATIO);
>  
>      /* Notify before starting migration thread */
>      notifier_list_notify(&migration_state_notifiers, s);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5658723..a8ee2d4 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -637,12 +637,19 @@
>  #                hostname must be provided so that the server's x509
>  #                certificate identity can be validated. (Since 2.7)
>  #
> +# @max-bandwidth: to set maximum speed for migration. maximum speed in
> +#                 bytes per second. (Since 2.8)
> +#
> +# @downtime-limit: set maximum tolerated downtime for migration. maximum downtime

Long line. Please wrap to stay under 80 columns.

> +#                  in milliseconds (Since 2.8)

Are we sure milliseconds is the desired scale?

>  { 'command': 'migrate-set-parameters',
> @@ -687,7 +700,9 @@
>              '*cpu-throttle-initial': 'int',
>              '*cpu-throttle-increment': 'int',
>              '*tls-creds': 'str',
> -            '*tls-hostname': 'str'} }
> +            '*tls-hostname': 'str',
> +            '*max-bandwidth': 'int',
> +            '*downtime-limit': 'int'} }

For that matter, would a floating point downtime-limit make any more
sense (in which case, I'd argue that having it be in seconds rather than
milliseconds may be nicer)?


> @@ -1812,6 +1835,8 @@
>  #
>  # Returns: nothing on success
>  #
> +# Notes: This command is deprecated in favour of 'migrate-set-parameters'

I don't know if we have a strong preference for US vs. UK spelling in
documentation.

> @@ -3803,7 +3805,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?",

Long line; you can break it up (but then again, we hope to get rid of
all .args_type lines once Marc-Andre's qapi work lands)

>          .mhandler.cmd_new = qmp_marshal_migrate_set_parameters,
>      },
>  SQMP
> @@ -3820,6 +3822,10 @@ Query current migration parameters
>                                      throttled (json-int)
>           - "cpu-throttle-increment" : throttle increasing percentage for
>                                        auto-converge (json-int)
> +         - "max-bandwidth" : maximium migration speed in s/bytes/bytes per second
> +                             (json-int)

Umm, that's not what you meant to type.

s,s/bytes/,,


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v3] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp
  2016-09-07 22:03 ` Eric Blake
@ 2016-09-08  7:55   ` Ashijeet Acharya
  2016-09-08 14:41   ` Juan Quintela
  1 sibling, 0 replies; 8+ messages in thread
From: Ashijeet Acharya @ 2016-09-08  7:55 UTC (permalink / raw)
  To: Eric Blake
  Cc: lcapitulino, quintela, amit.shah, armbru, dgilbert,
	Paolo Bonzini, Daniel P. Berrange, QEMU Developers

On Thu, Sep 8, 2016 at 3:33 AM, Eric Blake <eblake@redhat.com> wrote:
> On 09/06/2016 08:39 AM, Ashijeet Acharya wrote:
>> Mark old-commands for speed and downtime as deprecated.
>> 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.
>>
>> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
>> ---
>
>> +++ b/migration/migration.c
>> @@ -44,6 +44,9 @@
>>  #define BUFFER_DELAY     100
>>  #define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY)
>>
>> +/* Time in nanoseconds we are allowed to stop the source for to send last part */
>
> Long line.  Also, a grammar issue:
>
> s/source for to send/source, for sending the/

Okay, I will change it.

>
>> @@ -832,6 +848,21 @@ void qmp_migrate_set_parameters(bool has_compress_level,
>>          g_free(s->parameters.tls_hostname);
>>          s->parameters.tls_hostname = g_strdup(tls_hostname);
>>      }
>> +    if (has_max_bandwidth) {
>> +        s->parameters.max_bandwidth = max_bandwidth;
>> +        if (s->to_dst_file) {
>> +            qemu_file_set_rate_limit(s->to_dst_file,
>> +                                s->parameters.max_bandwidth / XFER_LIMIT_RATIO);
>> +        }
>> +    }
>> +    if (has_downtime_limit) {
>> +        downtime_limit *= 1000000; /* convert to nanoseconds */
>
> Are you sure this won't overflow?

Should I explicitly cast it? Like;
downtime_limit *= (int64_t)1000000;
This should work right?

>> +void qmp_migrate_set_speed(int64_t valuebw, Error **errp)
>> +{
>> +    bool has_compress_level = false;
>> +    bool has_compress_threads = false;
>> +    bool has_decompress_threads = false;
>> +    bool has_cpu_throttle_initial = false;
>> +    bool has_cpu_throttle_increment = false;
>> +    bool has_tls_creds = false;
>> +    bool has_tls_hostname = false;
>> +    bool has_max_bandwidth = true;
>> +    bool has_downtime_limit = false;
>> +    const char *valuestr = NULL;
>> +    long valueint = 0;
>> +    Error *err = NULL;
>> +
>> +    qmp_migrate_set_parameters(has_compress_level, valueint,
>> +                               has_compress_threads, valueint,
>
> Ugg. This looks gross.  No need to name a bunch of variables set to
> false, when you can instead use QAPI's boxed conventions to pass a
> pointer to a struct, and rely on 0-initialization of the struct to leave
> all the parameters that you don't care about unmentioned.
>

I know. But I was not sure if I can do something else.
'hmp.c' does it in a similar way. I am not aware of the method you described.
Maybe you can direct me to some place where I can code by example.
(Sorry! I am a newbie)

>> +                               has_decompress_threads, valueint,
>> +                               has_cpu_throttle_initial, valueint,
>> +                               has_cpu_throttle_increment, valueint,
>> +                               has_tls_creds, valuestr,
>> +                               has_tls_hostname, valuestr,
>> +                               has_max_bandwidth, valuebw,
>> +                               has_downtime_limit, valueint,
>> +                               &err);
>> +
>> +}
>> +
>> +void qmp_migrate_set_downtime(double valuedowntime, Error **errp)
>> +{
>> +    bool has_compress_level = false;
>> +    bool has_compress_threads = false;
>> +    bool has_decompress_threads = false;
>> +    bool has_cpu_throttle_initial = false;
>> +    bool has_cpu_throttle_increment = false;
>> +    bool has_tls_creds = false;
>> +    bool has_tls_hostname = false;
>> +    bool has_max_bandwidth = false;
>> +    bool has_downtime_limit = true;
>
> Again, gross.
>
>> +    const char *valuestr = NULL;
>> +    long valueint = 0;
>> +    int64_t valuebw = 0;
>> +    valuedowntime *= 1000; /* Convert to milliseconds */
>> +    valuedowntime = MAX(0, MIN(INT64_MAX, valuedowntime));
>> +    valuedowntime = (int64_t)valuedowntime;
>
> Useless statement; the cast would already implicitly happen when passing
> it to the function call.

Okay, I will remove that.

>
>> +    Error *err = NULL;
>> +
>> +    qmp_migrate_set_parameters(has_compress_level, valueint,
>> +                               has_compress_threads, valueint,
>> +                               has_decompress_threads, valueint,
>> +                               has_cpu_throttle_initial, valueint,
>> +                               has_cpu_throttle_increment, valueint,
>> +                               has_tls_creds, valuestr,
>> +                               has_tls_hostname, valuestr,
>> +                               has_max_bandwidth, valuebw,
>> +                               has_downtime_limit, valuedowntime,
>> +                               &err);
>>  }
>>
>>  bool migrate_postcopy_ram(void)
>> @@ -1858,7 +1922,7 @@ void migrate_fd_connect(MigrationState *s)
>>
>>      qemu_file_set_blocking(s->to_dst_file, true);
>>      qemu_file_set_rate_limit(s->to_dst_file,
>> -                             s->bandwidth_limit / XFER_LIMIT_RATIO);
>> +                             s->parameters.max_bandwidth / XFER_LIMIT_RATIO);
>>
>>      /* Notify before starting migration thread */
>>      notifier_list_notify(&migration_state_notifiers, s);
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 5658723..a8ee2d4 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -637,12 +637,19 @@
>>  #                hostname must be provided so that the server's x509
>>  #                certificate identity can be validated. (Since 2.7)
>>  #
>> +# @max-bandwidth: to set maximum speed for migration. maximum speed in
>> +#                 bytes per second. (Since 2.8)
>> +#
>> +# @downtime-limit: set maximum tolerated downtime for migration. maximum downtime
>
> Long line. Please wrap to stay under 80 columns.

Okay.

>
>> +#                  in milliseconds (Since 2.8)
>
> Are we sure milliseconds is the desired scale?

I am really confused regarding the scale for downtime now.
There seems to be a divided opinion on this one among developers with
everyone having their valid points.
I will stall the updated patch until we have a common ground on this one.

>
>>  { 'command': 'migrate-set-parameters',
>> @@ -687,7 +700,9 @@
>>              '*cpu-throttle-initial': 'int',
>>              '*cpu-throttle-increment': 'int',
>>              '*tls-creds': 'str',
>> -            '*tls-hostname': 'str'} }
>> +            '*tls-hostname': 'str',
>> +            '*max-bandwidth': 'int',
>> +            '*downtime-limit': 'int'} }
>
> For that matter, would a floating point downtime-limit make any more
> sense (in which case, I'd argue that having it be in seconds rather than
> milliseconds may be nicer)?

Yeah! Floating point seems useful only if we are talking in seconds.
A floating point value in milliseconds seems impractical as far as I understand.

>
>
>> @@ -1812,6 +1835,8 @@
>>  #
>>  # Returns: nothing on success
>>  #
>> +# Notes: This command is deprecated in favour of 'migrate-set-parameters'
>
> I don't know if we have a strong preference for US vs. UK spelling in
> documentation.

I can change it to 'favor' if you like.

>
>> @@ -3803,7 +3805,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?",
>
> Long line; you can break it up (but then again, we hope to get rid of
> all .args_type lines once Marc-Andre's qapi work lands)

Breaking it rejects the arguments after the break I guess and I get an
error if I use them in qmp later.
The line was long even before including 'max-bandwidth' and
'downtime-limit' anyway.

Ashijeet

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v3] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp
  2016-09-07 22:03 ` Eric Blake
  2016-09-08  7:55   ` Ashijeet Acharya
@ 2016-09-08 14:41   ` Juan Quintela
  2016-09-08 15:30     ` Ashijeet Acharya
  1 sibling, 1 reply; 8+ messages in thread
From: Juan Quintela @ 2016-09-08 14:41 UTC (permalink / raw)
  To: Eric Blake
  Cc: Ashijeet Acharya, lcapitulino, amit.shah, armbru, dgilbert,
	pbonzini, berrange, qemu-devel

Eric Blake <eblake@redhat.com> wrote:

>> +    if (has_max_bandwidth) {
>> +        s->parameters.max_bandwidth = max_bandwidth;
>> +        if (s->to_dst_file) {
>> +            qemu_file_set_rate_limit(s->to_dst_file,
>> +                                s->parameters.max_bandwidth / XFER_LIMIT_RATIO);
>> +        }
>> +    }
>> +    if (has_downtime_limit) {
>> +        downtime_limit *= 1000000; /* convert to nanoseconds */
>
> Are you sure this won't overflow?

Ashijeet, Eric mere means that if downtime_limit is bigger that
INT64_MAX/1000000, then you get an overflow with the multiplication.
Notice that if it overflows, the value is already quite big O:-)

2^63/1000/1000/1000/3600/24/365
292.47

Allowing "only" 292 years of downtime should be enough for the time
being (famous last words) O:-)


>
>> +void qmp_migrate_set_speed(int64_t valuebw, Error **errp)
>> +{
>> +    bool has_compress_level = false;
>> +    bool has_compress_threads = false;
>> +    bool has_decompress_threads = false;
>> +    bool has_cpu_throttle_initial = false;
>> +    bool has_cpu_throttle_increment = false;
>> +    bool has_tls_creds = false;
>> +    bool has_tls_hostname = false;
>> +    bool has_max_bandwidth = true;
>> +    bool has_downtime_limit = false;
>> +    const char *valuestr = NULL;
>> +    long valueint = 0;
>> +    Error *err = NULL;
>> +
>> +    qmp_migrate_set_parameters(has_compress_level, valueint,
>> +                               has_compress_threads, valueint,
>
> Ugg. This looks gross.  No need to name a bunch of variables set to
> false, when you can instead use QAPI's boxed conventions to pass a
> pointer to a struct, and rely on 0-initialization of the struct to leave
> all the parameters that you don't care about unmentioned.

We should change qmp_migrate_set_parameters to your new api.  I fully
agree that this is gross, but it is the way it was written, nothing to
do with this patch, really.

Ashijeet, if you want to do this change in a different patch before this
change, I am all for it, but that is independent of your change.

With all the other suggestions of Eric, I agree, or I don't care.
(If time is an int in milliseconds or a float, I don't really care one
way or another.  Whatever libvirt preffers).

Thanks, Juan.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v3] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp
  2016-09-08 14:41   ` Juan Quintela
@ 2016-09-08 15:30     ` Ashijeet Acharya
  2016-09-08 15:39       ` Eric Blake
  0 siblings, 1 reply; 8+ messages in thread
From: Ashijeet Acharya @ 2016-09-08 15:30 UTC (permalink / raw)
  To: quintela
  Cc: Eric Blake, lcapitulino, amit.shah, armbru, dgilbert,
	Paolo Bonzini, Daniel P. Berrange, QEMU Developers

On Thu, Sep 8, 2016 at 8:11 PM, Juan Quintela <quintela@redhat.com> wrote:
> Eric Blake <eblake@redhat.com> wrote:
>
>>> +    if (has_max_bandwidth) {
>>> +        s->parameters.max_bandwidth = max_bandwidth;
>>> +        if (s->to_dst_file) {
>>> +            qemu_file_set_rate_limit(s->to_dst_file,
>>> +                                s->parameters.max_bandwidth / XFER_LIMIT_RATIO);
>>> +        }
>>> +    }
>>> +    if (has_downtime_limit) {
>>> +        downtime_limit *= 1000000; /* convert to nanoseconds */
>>
>> Are you sure this won't overflow?
>
> Ashijeet, Eric mere means that if downtime_limit is bigger that
> INT64_MAX/1000000, then you get an overflow with the multiplication.

Oh I get it now.

> Notice that if it overflows, the value is already quite big O:-)
>
> 2^63/1000/1000/1000/3600/24/365
> 292.47
>
> Allowing "only" 292 years of downtime should be enough for the time
> being (famous last words) O:-)
>

Haha! 292 years seems sufficient. :-)

>
>>
>>> +void qmp_migrate_set_speed(int64_t valuebw, Error **errp)
>>> +{
>>> +    bool has_compress_level = false;
>>> +    bool has_compress_threads = false;
>>> +    bool has_decompress_threads = false;
>>> +    bool has_cpu_throttle_initial = false;
>>> +    bool has_cpu_throttle_increment = false;
>>> +    bool has_tls_creds = false;
>>> +    bool has_tls_hostname = false;
>>> +    bool has_max_bandwidth = true;
>>> +    bool has_downtime_limit = false;
>>> +    const char *valuestr = NULL;
>>> +    long valueint = 0;
>>> +    Error *err = NULL;
>>> +
>>> +    qmp_migrate_set_parameters(has_compress_level, valueint,
>>> +                               has_compress_threads, valueint,
>>
>> Ugg. This looks gross.  No need to name a bunch of variables set to
>> false, when you can instead use QAPI's boxed conventions to pass a
>> pointer to a struct, and rely on 0-initialization of the struct to leave
>> all the parameters that you don't care about unmentioned.
>
> We should change qmp_migrate_set_parameters to your new api.  I fully
> agree that this is gross, but it is the way it was written, nothing to
> do with this patch, really.

Yeah, you expressed your disgust about this in your 'to-do list of
migration mail' too I remember.

>
> Ashijeet, if you want to do this change in a different patch before this
> change, I am all for it, but that is independent of your change.

I can try, but I am not sure about it as I am very new to QAPI's methods.
Its better if I embarked upon such a task later with more knowledge of QAPI.

> With all the other suggestions of Eric, I agree, or I don't care.
> (If time is an int in milliseconds or a float, I don't really care one
> way or another.  Whatever libvirt preffers).

I am keeping it to milliseconds for now then!

Juan, do not apply the patch just yet. Let me rectify the small
mistakes regarding grammatical errors in comments and send the updated
patch in a few. Accept it after that.

Thanks
Ashijeet
>
> Thanks, Juan.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH v3] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp
  2016-09-08 15:30     ` Ashijeet Acharya
@ 2016-09-08 15:39       ` Eric Blake
  0 siblings, 0 replies; 8+ messages in thread
From: Eric Blake @ 2016-09-08 15:39 UTC (permalink / raw)
  To: Ashijeet Acharya, quintela
  Cc: lcapitulino, amit.shah, armbru, dgilbert, Paolo Bonzini,
	Daniel P. Berrange, QEMU Developers

[-- Attachment #1: Type: text/plain, Size: 2357 bytes --]

On 09/08/2016 10:30 AM, Ashijeet Acharya wrote:

>>>> +    if (has_downtime_limit) {
>>>> +        downtime_limit *= 1000000; /* convert to nanoseconds */
>>>
>>> Are you sure this won't overflow?
>>
>> Ashijeet, Eric mere means that if downtime_limit is bigger that
>> INT64_MAX/1000000, then you get an overflow with the multiplication.
> 
> Oh I get it now.
> 
>> Notice that if it overflows, the value is already quite big O:-)
>>
>> 2^63/1000/1000/1000/3600/24/365
>> 292.47
>>
>> Allowing "only" 292 years of downtime should be enough for the time
>> being (famous last words) O:-)
>>
> 
> Haha! 292 years seems sufficient. :-)

But it still means we should explicitly check that whatever number the
user inputs is not wrapping around to a smaller actual downtime -
especially since wraparound to a small number CAN be observed within a
human lifespan, for some values of wraparound.  Either give the user an
error (their input was too big; preferred) or silently convert the
user's too-large input to the maximum possible (they won't live long
enough to notice the difference, not ideal, but if reporting an error is
too hard, it is workable).


>>>> +
>>>> +    qmp_migrate_set_parameters(has_compress_level, valueint,
>>>> +                               has_compress_threads, valueint,
>>>
>>> Ugg. This looks gross.  No need to name a bunch of variables set to
>>> false, when you can instead use QAPI's boxed conventions to pass a
>>> pointer to a struct, and rely on 0-initialization of the struct to leave
>>> all the parameters that you don't care about unmentioned.
>>
>> We should change qmp_migrate_set_parameters to your new api.  I fully
>> agree that this is gross, but it is the way it was written, nothing to
>> do with this patch, really.
> 
> Yeah, you expressed your disgust about this in your 'to-do list of
> migration mail' too I remember.

Okay, I'll take the hint and submit a patch to convert migration to use
the new boxed parameters from QAPI.  It's an independent change, but you
would then rebase your series on top of my patch (or if yours lands
first, my work would remove the additions in yours, for some churn in
the code base, but no real harm done).

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-09-08 15:39 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-06 13:39 [Qemu-devel] [PATCH v3] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp Ashijeet Acharya
2016-09-07  8:41 ` Juan Quintela
2016-09-07 12:52   ` Ashijeet Acharya
2016-09-07 22:03 ` Eric Blake
2016-09-08  7:55   ` Ashijeet Acharya
2016-09-08 14:41   ` Juan Quintela
2016-09-08 15:30     ` Ashijeet Acharya
2016-09-08 15:39       ` Eric Blake

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.