[Qemu-devel] [PATCH v3 0/5] migration: Dynamic cpu throttling for auto-converge

[Qemu-devel] [PATCH v3 0/5] migration: Dynamic cpu throttling for auto-converge

[Jason J. Herne]

This patch set provides a new method for throttling a vcpu and makes use of said
method to dynamically increase cpu throttling during an autoconverge
migration until the migration completes.

The method used here for throttling vcpus is likely not the best. However, I
believe that it is preferable to what is used for autoconverge today.

This work is related to the following discussion:
https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg00287.html

Changelog
-----------
v3
- Cpu throttling parameters moved from CPUState to global scope
- Throttling interface now uses a percentage instead of ratio
- Migration parameters added to allow tweaking of how aggressive throttling is
- Add throttle active check to the throttle stop routine.
- Remove asserts from throttle start/stop functions and instead force the input
  to fall within the acceptable range
- Rename cpu_throttle_start to cpu_throttle_set to better describe its purpose
- Remove unneeded object casts
- Fixed monitor output formatting
- Comment cleanups
v2
- Add cpu throttle ratio output to hmp/qmp info/query migrate commands
v1
- Initial

Jason J. Herne (5):
  cpu: Provide vcpu throttling interface
  migration: Parameters for auto-converge cpu throttling
  migration: Dynamic cpu throttling for auto-converge
  qmp/hmp: Add throttle ratio to query-migrate and info migrate
  migration: Disambiguate MAX_THROTTLE

 arch_init.c           | 93 +++++++++++++++++----------------------------------
 cpus.c                | 76 +++++++++++++++++++++++++++++++++++++++++
 hmp.c                 | 21 ++++++++++++
 include/qom/cpu.h     | 38 +++++++++++++++++++++
 migration/migration.c | 57 +++++++++++++++++++++++++++++--
 qapi-schema.json      | 40 +++++++++++++++++++---
 6 files changed, 256 insertions(+), 69 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v3 1/5] cpu: Provide vcpu throttling interface

[Jason J. Herne]

Provide a method to throttle guest cpu execution. CPUState is augmented with
timeout controls and throttle start/stop functions. To throttle the guest cpu
the caller simply has to call the throttle set function and provide a percentage
of throttle time.

Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
 cpus.c            | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/qom/cpu.h | 38 ++++++++++++++++++++++++++++
 2 files changed, 114 insertions(+)

diff --git a/cpus.c b/cpus.c
index de6469f..f57cf4f 100644
--- a/cpus.c
+++ b/cpus.c
@@ -68,6 +68,16 @@ static CPUState *next_cpu;
 int64_t max_delay;
 int64_t max_advance;
 
+/* vcpu throttling controls */
+static QEMUTimer *throttle_timer;
+static bool throttle_timer_stop;
+static int throttle_percentage;
+static float throttle_ratio;
+
+#define CPU_THROTTLE_PCT_MIN 1
+#define CPU_THROTTLE_PCT_MAX 99
+#define CPU_THROTTLE_TIMESLICE 10
+
 bool cpu_is_stopped(CPUState *cpu)
 {
     return cpu->stopped || !runstate_is_running();
@@ -919,6 +929,72 @@ static void qemu_kvm_wait_io_event(CPUState *cpu)
     qemu_wait_io_event_common(cpu);
 }
 
+static void cpu_throttle_thread(void *opaque)
+{
+    long sleeptime_ms = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE);
+
+    qemu_mutex_unlock_iothread();
+    g_usleep(sleeptime_ms * 1000); /* Convert ms to us for usleep call */
+    qemu_mutex_lock_iothread();
+
+    timer_mod(throttle_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
+                                   CPU_THROTTLE_TIMESLICE);
+}
+
+static void cpu_throttle_timer_pop(void *opaque)
+{
+    CPUState *cpu;
+
+    /* Stop the timer if needed */
+    if (throttle_timer_stop) {
+        timer_del(throttle_timer);
+        timer_free(throttle_timer);
+        throttle_timer = NULL;
+        return;
+    }
+
+    CPU_FOREACH(cpu) {
+        async_run_on_cpu(cpu, cpu_throttle_thread, NULL);
+    }
+}
+
+void cpu_throttle_set(int new_throttle_pct)
+{
+    double pct;
+
+    /* Ensure throttle percentage is within valid range */
+    new_throttle_pct = MIN(new_throttle_pct, CPU_THROTTLE_PCT_MAX);
+    throttle_percentage = MAX(new_throttle_pct, CPU_THROTTLE_PCT_MIN);
+
+    pct = (double)throttle_percentage/100;
+    throttle_ratio = pct / (1 - pct);
+
+    if (!cpu_throttle_active()) {
+        throttle_timer_stop = false;
+        throttle_timer = timer_new_ms(QEMU_CLOCK_REALTIME,
+                                           cpu_throttle_timer_pop, NULL);
+        timer_mod(throttle_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
+                                       CPU_THROTTLE_TIMESLICE);
+    }
+}
+
+void cpu_throttle_stop(void)
+{
+    if (cpu_throttle_active()) {
+        throttle_timer_stop = true;
+    }
+}
+
+bool cpu_throttle_active(void)
+{
+    return (throttle_timer != NULL);
+}
+
+int cpu_throttle_get_percentage(void)
+{
+    return throttle_percentage;
+}
+
 static void *qemu_kvm_cpu_thread_fn(void *arg)
 {
     CPUState *cpu = arg;
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 39f0f19..56eb964 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -553,6 +553,44 @@ CPUState *qemu_get_cpu(int index);
  */
 bool cpu_exists(int64_t id);
 
+/**
+ * cpu_throttle_set:
+ * @new_throttle_pct: Percent of sleep time to running time.
+ *                    Valid range is 1 to 99.
+ *
+ * Throttles all vcpus by forcing them to sleep for the given percentage of
+ * time. A throttle_percentage of 50 corresponds to a 50% duty cycle roughly.
+ * (example: 10ms sleep for every 10ms awake).
+ *
+ * cpu_throttle_set can be called as needed to adjust new_throttle_pct.
+ * Once the throttling starts, it will remain in effect until cpu_throttle_stop
+ * is called.
+ */
+void cpu_throttle_set(int new_throttle_pct);
+
+/**
+ * cpu_throttle_stop:
+ *
+ * Stops the vcpu throttling started by cpu_throttle_set.
+ */
+void cpu_throttle_stop(void);
+
+/**
+ * cpu_throttle_active:
+ *
+ * Returns %true if the vcpus are currently being throttled, %false otherwise.
+ */
+bool cpu_throttle_active(void);
+
+/**
+ * cpu_throttle_get_percentage:
+ *
+ * Returns the vcpu throttle percentage. See cpu_throttle_set for details.
+ *
+ * Returns The throttle percentage in range 1 to 99.
+ */
+int cpu_throttle_get_percentage(void);
+
 #ifndef CONFIG_USER_ONLY
 
 typedef void (*CPUInterruptHandler)(CPUState *, int);
-- 
1.9.1

[Qemu-devel] [PATCH v3 2/5] migration: Parameters for auto-converge cpu throttling

[Jason J. Herne]

Add migration parameters to allow the user to adjust the parameters
that control cpu throttling when auto-converge is in effect. The added
parameters are as follows:

x-cpu-throttle-initial : Initial percantage of time guest cpus are throttled
when migration auto-converge is activated.

x-cpu-throttle-increment: throttle percantage increase each time
auto-converge detects that migration is not making progress.

Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
---
 hmp.c                 | 16 ++++++++++++++++
 migration/migration.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 qapi-schema.json      | 33 ++++++++++++++++++++++++++++++---
 3 files changed, 91 insertions(+), 4 deletions(-)

diff --git a/hmp.c b/hmp.c
index e17852d..eb65998 100644
--- a/hmp.c
+++ b/hmp.c
@@ -269,6 +269,12 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, " %s: %" PRId64,
             MigrationParameter_lookup[MIGRATION_PARAMETER_DECOMPRESS_THREADS],
             params->decompress_threads);
+        monitor_printf(mon, " %s: %" PRId64,
+            MigrationParameter_lookup[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL],
+            params->x_cpu_throttle_initial);
+        monitor_printf(mon, " %s: %" PRId64,
+            MigrationParameter_lookup[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT],
+            params->x_cpu_throttle_increment);
         monitor_printf(mon, "\n");
     }
 
@@ -1216,6 +1222,8 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
     bool has_compress_level = false;
     bool has_compress_threads = false;
     bool has_decompress_threads = false;
+    bool has_x_cpu_throttle_initial = false;
+    bool has_x_cpu_throttle_increment = false;
     int i;
 
     for (i = 0; i < MIGRATION_PARAMETER_MAX; i++) {
@@ -1230,10 +1238,18 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
             case MIGRATION_PARAMETER_DECOMPRESS_THREADS:
                 has_decompress_threads = true;
                 break;
+            case MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL:
+                has_x_cpu_throttle_initial = true;
+                break;
+            case MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT:
+                has_x_cpu_throttle_increment = true;
+                break;
             }
             qmp_migrate_set_parameters(has_compress_level, value,
                                        has_compress_threads, value,
                                        has_decompress_threads, value,
+                                       has_x_cpu_throttle_initial, value,
+                                       has_x_cpu_throttle_increment, value,
                                        &err);
             break;
         }
diff --git a/migration/migration.c b/migration/migration.c
index 732d229..05790e9 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -40,6 +40,9 @@
 #define DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT 2
 /*0: means nocompress, 1: best speed, ... 9: best compress ratio */
 #define DEFAULT_MIGRATE_COMPRESS_LEVEL 1
+/* Define default autoconverge cpu throttle migration parameters */
+#define DEFAULT_MIGRATE_X_CPU_THROTTLE_INITIAL 20
+#define DEFAULT_MIGRATE_X_CPU_THROTTLE_INCREMENT 10
 
 /* Migration XBZRLE default cache size */
 #define DEFAULT_MIGRATE_CACHE_SIZE (64 * 1024 * 1024)
@@ -66,6 +69,10 @@ MigrationState *migrate_get_current(void)
                 DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
         .parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS] =
                 DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
+        .parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL] =
+                DEFAULT_MIGRATE_X_CPU_THROTTLE_INITIAL,
+        .parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT] =
+                DEFAULT_MIGRATE_X_CPU_THROTTLE_INCREMENT,
     };
 
     return &current_migration;
@@ -199,6 +206,10 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
             s->parameters[MIGRATION_PARAMETER_COMPRESS_THREADS];
     params->decompress_threads =
             s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS];
+    params->x_cpu_throttle_initial =
+            s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL];
+    params->x_cpu_throttle_increment =
+            s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT];
 
     return params;
 }
@@ -321,7 +332,11 @@ void qmp_migrate_set_parameters(bool has_compress_level,
                                 bool has_compress_threads,
                                 int64_t compress_threads,
                                 bool has_decompress_threads,
-                                int64_t decompress_threads, Error **errp)
+                                int64_t decompress_threads,
+                                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)
 {
     MigrationState *s = migrate_get_current();
 
@@ -344,6 +359,18 @@ void qmp_migrate_set_parameters(bool has_compress_level,
                   "is invalid, it should be in the range of 1 to 255");
         return;
     }
+    if (has_x_cpu_throttle_initial &&
+            (x_cpu_throttle_initial < 1 || x_cpu_throttle_initial > 99)) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  "x_cpu_throttle_initial",
+                  "an integer in the range of 1 to 99");
+    }
+    if (has_x_cpu_throttle_increment &&
+            (x_cpu_throttle_increment < 1 || x_cpu_throttle_increment > 99)) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  "x_cpu_throttle_increment",
+                  "an integer in the range of 1 to 99");
+    }
 
     if (has_compress_level) {
         s->parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL] = compress_level;
@@ -355,6 +382,15 @@ void qmp_migrate_set_parameters(bool has_compress_level,
         s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS] =
                                                     decompress_threads;
     }
+    if (has_x_cpu_throttle_initial) {
+        s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL] =
+                                                    x_cpu_throttle_initial;
+    }
+
+    if (has_x_cpu_throttle_increment) {
+        s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT] =
+                                                    x_cpu_throttle_increment;
+    }
 }
 
 /* shared migration helpers */
@@ -470,6 +506,10 @@ static MigrationState *migrate_init(const MigrationParams *params)
             s->parameters[MIGRATION_PARAMETER_COMPRESS_THREADS];
     int decompress_thread_count =
             s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS];
+    int x_cpu_throttle_initial =
+            s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL];
+    int x_cpu_throttle_increment =
+            s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT];
 
     memcpy(enabled_capabilities, s->enabled_capabilities,
            sizeof(enabled_capabilities));
@@ -485,6 +525,10 @@ static MigrationState *migrate_init(const MigrationParams *params)
                compress_thread_count;
     s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS] =
                decompress_thread_count;
+    s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL] =
+                x_cpu_throttle_initial;
+    s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT] =
+                x_cpu_throttle_increment;
     s->bandwidth_limit = bandwidth_limit;
     s->state = MIGRATION_STATUS_SETUP;
     trace_migrate_set_state(MIGRATION_STATUS_SETUP);
diff --git a/qapi-schema.json b/qapi-schema.json
index f97ffa1..7dd324e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -587,10 +587,18 @@
 #          compression, so set the decompress-threads to the number about 1/4
 #          of compress-threads is adequate.
 #
+# @x-cpu-throttle-initial: Initial percantage of time guest cpus are throttled
+#                          when migration auto-converge is activated.
+#                          (Since 2.4)
+#
+# @x-cpu-throttle-increment: throttle percantage increase each time
+#                            auto-converge detects that migration is not making
+#                            progress. (Since 2.4)
 # Since: 2.4
 ##
 { 'enum': 'MigrationParameter',
-  'data': ['compress-level', 'compress-threads', 'decompress-threads'] }
+  'data': ['compress-level', 'compress-threads', 'decompress-threads',
+           'x-cpu-throttle-initial', 'x-cpu-throttle-increment'] }
 
 #
 # @migrate-set-parameters
@@ -603,12 +611,22 @@
 #
 # @decompress-threads: decompression thread count
 #
+# @x-cpu-throttle-initial: Initial percantage of time guest cpus are throttled
+#                          when migration auto-converge is activated.
+#                          (Since 2.4)
+#
+# @x-cpu-throttle-increment: throttle percantage increase each time
+#                            auto-converge detects that migration is not making
+#                            progress. (Since 2.4)
+#
 # Since: 2.4
 ##
 { 'command': 'migrate-set-parameters',
   'data': { '*compress-level': 'int',
             '*compress-threads': 'int',
-            '*decompress-threads': 'int'} }
+            '*decompress-threads': 'int',
+            '*x-cpu-throttle-initial': 'int',
+            '*x-cpu-throttle-increment': 'int'} }
 
 #
 # @MigrationParameters
@@ -618,13 +636,22 @@
 # @compress-threads: compression thread count
 #
 # @decompress-threads: decompression thread count
+# @x-cpu-throttle-initial: Initial percantage of time guest cpus are throttled
+#                          when migration auto-converge is activated.
+#                          (Since 2.4)
+#
+# @x-cpu-throttle-increment: throttle percantage increase each time
+#                            auto-converge detects that migration is not making
+#                            progress. (Since 2.4)
 #
 # Since: 2.4
 ##
 { 'struct': 'MigrationParameters',
   'data': { 'compress-level': 'int',
             'compress-threads': 'int',
-            'decompress-threads': 'int'} }
+            'decompress-threads': 'int',
+            'x-cpu-throttle-initial': 'int',
+            'x-cpu-throttle-increment': 'int'} }
 ##
 # @query-migrate-parameters
 #
-- 
1.9.1

[Qemu-devel] [PATCH v3 3/5] migration: Dynamic cpu throttling for auto-converge

[Jason J. Herne]

Remove traditional auto-converge static 30ms throttling code and replace it
with a dynamic throttling algorithm.

Additionally, be more aggressive when deciding when to start throttling.
Previously we waited until four unproductive memory passes. Now we begin
throttling after only two unproductive memory passes. Four seemed quite
arbitrary and only waiting for two passes allows us to complete the migration
faster.

Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
---
 arch_init.c           | 93 +++++++++++++++++----------------------------------
 migration/migration.c |  4 +++
 2 files changed, 34 insertions(+), 63 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 23d3feb..d456527 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -111,9 +111,7 @@ int graphic_depth = 32;
 #endif
 
 const uint32_t arch_type = QEMU_ARCH;
-static bool mig_throttle_on;
 static int dirty_rate_high_cnt;
-static void check_guest_throttling(void);
 
 static uint64_t bitmap_sync_count;
 
@@ -487,6 +485,29 @@ static size_t save_page_header(QEMUFile *f, RAMBlock *block, ram_addr_t offset)
     return size;
 }
 
+/* Reduce amount of guest cpu execution to hopefully slow down memory writes.
+ * If guest dirty memory rate is reduced below the rate at which we can
+ * transfer pages to the destination then we should be able to complete
+ * migration. Some workloads dirty memory way too fast and will not effectively
+ * converge, even with auto-converge.
+ */
+static void mig_throttle_guest_down(void)
+{
+    MigrationState *s = migrate_get_current();
+    uint64_t pct_initial =
+            s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL];
+    uint64_t pct_icrement =
+            s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT];
+
+    /* We have not started throttling yet. Let's start it. */
+    if (!cpu_throttle_active()) {
+        cpu_throttle_set(pct_initial);
+    } else {
+        /* Throttling already on, just increase the rate */
+        cpu_throttle_set(cpu_throttle_get_percentage() + pct_icrement);
+    }
+}
+
 /* Update the xbzrle cache to reflect a page that's been sent as all 0.
  * The important thing is that a stale (not-yet-0'd) page be replaced
  * by the new data.
@@ -714,21 +735,21 @@ static void migration_bitmap_sync(void)
             /* The following detection logic can be refined later. For now:
                Check to see if the dirtied bytes is 50% more than the approx.
                amount of bytes that just got transferred since the last time we
-               were in this routine. If that happens >N times (for now N==4)
-               we turn on the throttle down logic */
+               were in this routine. If that happens twice, start or increase
+               throttling */
             bytes_xfer_now = ram_bytes_transferred();
+
             if (s->dirty_pages_rate &&
                (num_dirty_pages_period * TARGET_PAGE_SIZE >
                    (bytes_xfer_now - bytes_xfer_prev)/2) &&
-               (dirty_rate_high_cnt++ > 4)) {
+               (dirty_rate_high_cnt++ >= 2)) {
                     trace_migration_throttle();
-                    mig_throttle_on = true;
                     dirty_rate_high_cnt = 0;
+                    mig_throttle_guest_down();
              }
              bytes_xfer_prev = bytes_xfer_now;
-        } else {
-             mig_throttle_on = false;
         }
+
         if (migrate_use_xbzrle()) {
             if (iterations_prev != acct_info.iterations) {
                 acct_info.xbzrle_cache_miss_rate =
@@ -1197,7 +1218,6 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
     RAMBlock *block;
     int64_t ram_bitmap_pages; /* Size of bitmap in pages, including gaps */
 
-    mig_throttle_on = false;
     dirty_rate_high_cnt = 0;
     bitmap_sync_count = 0;
     migration_bitmap_sync_init();
@@ -1301,12 +1321,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
         }
         pages_sent += pages;
         acct_info.iterations++;
-        check_guest_throttling();
-        /* we want to check in the 1st loop, just in case it was the 1st time
-           and we had to sync the dirty bitmap.
-           qemu_get_clock_ns() is a bit expensive, so we only check each some
-           iterations
-        */
+
         if ((i & 63) == 0) {
             uint64_t t1 = (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - t0) / 1000000;
             if (t1 > MAX_WAIT) {
@@ -1913,51 +1928,3 @@ TargetInfo *qmp_query_target(Error **errp)
     return info;
 }
 
-/* Stub function that's gets run on the vcpu when its brought out of the
-   VM to run inside qemu via async_run_on_cpu()*/
-static void mig_sleep_cpu(void *opq)
-{
-    qemu_mutex_unlock_iothread();
-    g_usleep(30*1000);
-    qemu_mutex_lock_iothread();
-}
-
-/* To reduce the dirty rate explicitly disallow the VCPUs from spending
-   much time in the VM. The migration thread will try to catchup.
-   Workload will experience a performance drop.
-*/
-static void mig_throttle_guest_down(void)
-{
-    CPUState *cpu;
-
-    qemu_mutex_lock_iothread();
-    CPU_FOREACH(cpu) {
-        async_run_on_cpu(cpu, mig_sleep_cpu, NULL);
-    }
-    qemu_mutex_unlock_iothread();
-}
-
-static void check_guest_throttling(void)
-{
-    static int64_t t0;
-    int64_t        t1;
-
-    if (!mig_throttle_on) {
-        return;
-    }
-
-    if (!t0)  {
-        t0 = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
-        return;
-    }
-
-    t1 = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
-
-    /* If it has been more than 40 ms since the last time the guest
-     * was throttled then do it again.
-     */
-    if (40 < (t1-t0)/1000000) {
-        mig_throttle_guest_down();
-        t0 = t1;
-    }
-}
diff --git a/migration/migration.c b/migration/migration.c
index 05790e9..7708c54 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -25,6 +25,7 @@
 #include "qemu/thread.h"
 #include "qmp-commands.h"
 #include "trace.h"
+#include "qom/cpu.h"
 
 #define MAX_THROTTLE  (32 << 20)      /* Migration speed throttling */
 
@@ -858,6 +859,9 @@ static void *migration_thread(void *opaque)
         }
     }
 
+    /* If we enabled cpu throttling for auto-converge, turn it off. */
+    cpu_throttle_stop();
+
     qemu_mutex_lock_iothread();
     if (s->state == MIGRATION_STATUS_COMPLETED) {
         int64_t end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
-- 
1.9.1

[Qemu-devel] [PATCH v3 4/5] qmp/hmp: Add throttle ratio to query-migrate and info migrate

[Jason J. Herne]

Report throttle percentage in info migrate and query-migrate responses when
cpu throttling is active.

Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
---
 hmp.c                 | 5 +++++
 migration/migration.c | 5 +++++
 qapi-schema.json      | 7 ++++++-
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/hmp.c b/hmp.c
index eb65998..36bc76e 100644
--- a/hmp.c
+++ b/hmp.c
@@ -229,6 +229,11 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
                        info->xbzrle_cache->overflow);
     }
 
+    if (info->has_x_cpu_throttle_percentage) {
+        monitor_printf(mon, "cpu throttle percentage: %" PRIu64 "\n",
+                       info->x_cpu_throttle_percentage);
+    }
+
     qapi_free_MigrationInfo(info);
     qapi_free_MigrationCapabilityStatusList(caps);
 }
diff --git a/migration/migration.c b/migration/migration.c
index 7708c54..b29450a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -274,6 +274,11 @@ MigrationInfo *qmp_query_migrate(Error **errp)
             info->disk->total = blk_mig_bytes_total();
         }
 
+        if (cpu_throttle_active()) {
+            info->has_x_cpu_throttle_percentage = true;
+            info->x_cpu_throttle_percentage = cpu_throttle_get_percentage();
+        }
+
         get_xbzrle_cache_stats(info);
         break;
     case MIGRATION_STATUS_COMPLETED:
diff --git a/qapi-schema.json b/qapi-schema.json
index 7dd324e..cd36256 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -474,6 +474,10 @@
 #        may be expensive, but do not actually occur during the iterative
 #        migration rounds themselves. (since 1.6)
 #
+# @x-cpu-throttle-percentage: #optional percentage of time guest cpus are being
+#       throttled during auto-converge. This is only present when auto-converge
+#       has started throttling guest cpus. (Since 2.4)
+#
 # Since: 0.14.0
 ##
 { 'struct': 'MigrationInfo',
@@ -483,7 +487,8 @@
            '*total-time': 'int',
            '*expected-downtime': 'int',
            '*downtime': 'int',
-           '*setup-time': 'int'} }
+           '*setup-time': 'int',
+           '*x-cpu-throttle-percentage': 'int'} }
 
 ##
 # @query-migrate
-- 
1.9.1

[Qemu-devel] [PATCH v3 5/5] migration: Disambiguate MAX_THROTTLE

[Jason J. Herne]

Migration has a define for MAX_THROTTLE. Update comment to clarify that this is
used for throttling transfer speed. Hopefully this will prevent it from being
confused with a guest cpu throttling entity.

Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
---
 migration/migration.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index b29450a..b9faeb0 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -27,7 +27,7 @@
 #include "trace.h"
 #include "qom/cpu.h"
 
-#define MAX_THROTTLE  (32 << 20)      /* Migration speed throttling */
+#define MAX_THROTTLE  (32 << 20)      /* Migration transfer speed throttling */
 
 /* Amount of time to allocate to each "chunk" of bandwidth-throttled
  * data. */
-- 
1.9.1

Re: [Qemu-devel] [PATCH v3 2/5] migration: Parameters for auto-converge cpu throttling

[Dr. David Alan Gilbert]

* Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
> Add migration parameters to allow the user to adjust the parameters
> that control cpu throttling when auto-converge is in effect. The added
> parameters are as follows:
> 
> x-cpu-throttle-initial : Initial percantage of time guest cpus are throttled
> when migration auto-converge is activated.
> 
> x-cpu-throttle-increment: throttle percantage increase each time
> auto-converge detects that migration is not making progress.
> 
> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>

Other than your spelling of 'percantage':

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hmp.c                 | 16 ++++++++++++++++
>  migration/migration.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>  qapi-schema.json      | 33 ++++++++++++++++++++++++++++++---
>  3 files changed, 91 insertions(+), 4 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index e17852d..eb65998 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -269,6 +269,12 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>          monitor_printf(mon, " %s: %" PRId64,
>              MigrationParameter_lookup[MIGRATION_PARAMETER_DECOMPRESS_THREADS],
>              params->decompress_threads);
> +        monitor_printf(mon, " %s: %" PRId64,
> +            MigrationParameter_lookup[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL],
> +            params->x_cpu_throttle_initial);
> +        monitor_printf(mon, " %s: %" PRId64,
> +            MigrationParameter_lookup[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT],
> +            params->x_cpu_throttle_increment);
>          monitor_printf(mon, "\n");
>      }
>  
> @@ -1216,6 +1222,8 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>      bool has_compress_level = false;
>      bool has_compress_threads = false;
>      bool has_decompress_threads = false;
> +    bool has_x_cpu_throttle_initial = false;
> +    bool has_x_cpu_throttle_increment = false;
>      int i;
>  
>      for (i = 0; i < MIGRATION_PARAMETER_MAX; i++) {
> @@ -1230,10 +1238,18 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>              case MIGRATION_PARAMETER_DECOMPRESS_THREADS:
>                  has_decompress_threads = true;
>                  break;
> +            case MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL:
> +                has_x_cpu_throttle_initial = true;
> +                break;
> +            case MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT:
> +                has_x_cpu_throttle_increment = true;
> +                break;
>              }
>              qmp_migrate_set_parameters(has_compress_level, value,
>                                         has_compress_threads, value,
>                                         has_decompress_threads, value,
> +                                       has_x_cpu_throttle_initial, value,
> +                                       has_x_cpu_throttle_increment, value,
>                                         &err);
>              break;
>          }
> diff --git a/migration/migration.c b/migration/migration.c
> index 732d229..05790e9 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -40,6 +40,9 @@
>  #define DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT 2
>  /*0: means nocompress, 1: best speed, ... 9: best compress ratio */
>  #define DEFAULT_MIGRATE_COMPRESS_LEVEL 1
> +/* Define default autoconverge cpu throttle migration parameters */
> +#define DEFAULT_MIGRATE_X_CPU_THROTTLE_INITIAL 20
> +#define DEFAULT_MIGRATE_X_CPU_THROTTLE_INCREMENT 10
>  
>  /* Migration XBZRLE default cache size */
>  #define DEFAULT_MIGRATE_CACHE_SIZE (64 * 1024 * 1024)
> @@ -66,6 +69,10 @@ MigrationState *migrate_get_current(void)
>                  DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
>          .parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS] =
>                  DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
> +        .parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL] =
> +                DEFAULT_MIGRATE_X_CPU_THROTTLE_INITIAL,
> +        .parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT] =
> +                DEFAULT_MIGRATE_X_CPU_THROTTLE_INCREMENT,
>      };
>  
>      return &current_migration;
> @@ -199,6 +206,10 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>              s->parameters[MIGRATION_PARAMETER_COMPRESS_THREADS];
>      params->decompress_threads =
>              s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS];
> +    params->x_cpu_throttle_initial =
> +            s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL];
> +    params->x_cpu_throttle_increment =
> +            s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT];
>  
>      return params;
>  }
> @@ -321,7 +332,11 @@ void qmp_migrate_set_parameters(bool has_compress_level,
>                                  bool has_compress_threads,
>                                  int64_t compress_threads,
>                                  bool has_decompress_threads,
> -                                int64_t decompress_threads, Error **errp)
> +                                int64_t decompress_threads,
> +                                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)
>  {
>      MigrationState *s = migrate_get_current();
>  
> @@ -344,6 +359,18 @@ void qmp_migrate_set_parameters(bool has_compress_level,
>                    "is invalid, it should be in the range of 1 to 255");
>          return;
>      }
> +    if (has_x_cpu_throttle_initial &&
> +            (x_cpu_throttle_initial < 1 || x_cpu_throttle_initial > 99)) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> +                  "x_cpu_throttle_initial",
> +                  "an integer in the range of 1 to 99");
> +    }
> +    if (has_x_cpu_throttle_increment &&
> +            (x_cpu_throttle_increment < 1 || x_cpu_throttle_increment > 99)) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> +                  "x_cpu_throttle_increment",
> +                  "an integer in the range of 1 to 99");
> +    }
>  
>      if (has_compress_level) {
>          s->parameters[MIGRATION_PARAMETER_COMPRESS_LEVEL] = compress_level;
> @@ -355,6 +382,15 @@ void qmp_migrate_set_parameters(bool has_compress_level,
>          s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS] =
>                                                      decompress_threads;
>      }
> +    if (has_x_cpu_throttle_initial) {
> +        s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL] =
> +                                                    x_cpu_throttle_initial;
> +    }
> +
> +    if (has_x_cpu_throttle_increment) {
> +        s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT] =
> +                                                    x_cpu_throttle_increment;
> +    }
>  }
>  
>  /* shared migration helpers */
> @@ -470,6 +506,10 @@ static MigrationState *migrate_init(const MigrationParams *params)
>              s->parameters[MIGRATION_PARAMETER_COMPRESS_THREADS];
>      int decompress_thread_count =
>              s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS];
> +    int x_cpu_throttle_initial =
> +            s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL];
> +    int x_cpu_throttle_increment =
> +            s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT];
>  
>      memcpy(enabled_capabilities, s->enabled_capabilities,
>             sizeof(enabled_capabilities));
> @@ -485,6 +525,10 @@ static MigrationState *migrate_init(const MigrationParams *params)
>                 compress_thread_count;
>      s->parameters[MIGRATION_PARAMETER_DECOMPRESS_THREADS] =
>                 decompress_thread_count;
> +    s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL] =
> +                x_cpu_throttle_initial;
> +    s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT] =
> +                x_cpu_throttle_increment;
>      s->bandwidth_limit = bandwidth_limit;
>      s->state = MIGRATION_STATUS_SETUP;
>      trace_migrate_set_state(MIGRATION_STATUS_SETUP);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index f97ffa1..7dd324e 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -587,10 +587,18 @@
>  #          compression, so set the decompress-threads to the number about 1/4
>  #          of compress-threads is adequate.
>  #
> +# @x-cpu-throttle-initial: Initial percantage of time guest cpus are throttled
> +#                          when migration auto-converge is activated.
> +#                          (Since 2.4)
> +#
> +# @x-cpu-throttle-increment: throttle percantage increase each time
> +#                            auto-converge detects that migration is not making
> +#                            progress. (Since 2.4)
>  # Since: 2.4
>  ##
>  { 'enum': 'MigrationParameter',
> -  'data': ['compress-level', 'compress-threads', 'decompress-threads'] }
> +  'data': ['compress-level', 'compress-threads', 'decompress-threads',
> +           'x-cpu-throttle-initial', 'x-cpu-throttle-increment'] }
>  
>  #
>  # @migrate-set-parameters
> @@ -603,12 +611,22 @@
>  #
>  # @decompress-threads: decompression thread count
>  #
> +# @x-cpu-throttle-initial: Initial percantage of time guest cpus are throttled
> +#                          when migration auto-converge is activated.
> +#                          (Since 2.4)
> +#
> +# @x-cpu-throttle-increment: throttle percantage increase each time
> +#                            auto-converge detects that migration is not making
> +#                            progress. (Since 2.4)
> +#
>  # Since: 2.4
>  ##
>  { 'command': 'migrate-set-parameters',
>    'data': { '*compress-level': 'int',
>              '*compress-threads': 'int',
> -            '*decompress-threads': 'int'} }
> +            '*decompress-threads': 'int',
> +            '*x-cpu-throttle-initial': 'int',
> +            '*x-cpu-throttle-increment': 'int'} }
>  
>  #
>  # @MigrationParameters
> @@ -618,13 +636,22 @@
>  # @compress-threads: compression thread count
>  #
>  # @decompress-threads: decompression thread count
> +# @x-cpu-throttle-initial: Initial percantage of time guest cpus are throttled
> +#                          when migration auto-converge is activated.
> +#                          (Since 2.4)
> +#
> +# @x-cpu-throttle-increment: throttle percantage increase each time
> +#                            auto-converge detects that migration is not making
> +#                            progress. (Since 2.4)
>  #
>  # Since: 2.4
>  ##
>  { 'struct': 'MigrationParameters',
>    'data': { 'compress-level': 'int',
>              'compress-threads': 'int',
> -            'decompress-threads': 'int'} }
> +            'decompress-threads': 'int',
> +            'x-cpu-throttle-initial': 'int',
> +            'x-cpu-throttle-increment': 'int'} }
>  ##
>  # @query-migrate-parameters
>  #
> -- 
> 1.9.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 3/5] migration: Dynamic cpu throttling for auto-converge

[Dr. David Alan Gilbert]

* Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
> Remove traditional auto-converge static 30ms throttling code and replace it
> with a dynamic throttling algorithm.
> 
> Additionally, be more aggressive when deciding when to start throttling.
> Previously we waited until four unproductive memory passes. Now we begin
> throttling after only two unproductive memory passes. Four seemed quite
> arbitrary and only waiting for two passes allows us to complete the migration
> faster.
> 
> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
> Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
>  arch_init.c           | 93 +++++++++++++++++----------------------------------
>  migration/migration.c |  4 +++
>  2 files changed, 34 insertions(+), 63 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 23d3feb..d456527 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -111,9 +111,7 @@ int graphic_depth = 32;
>  #endif
>  
>  const uint32_t arch_type = QEMU_ARCH;
> -static bool mig_throttle_on;
>  static int dirty_rate_high_cnt;
> -static void check_guest_throttling(void);
>  
>  static uint64_t bitmap_sync_count;
>  
> @@ -487,6 +485,29 @@ static size_t save_page_header(QEMUFile *f, RAMBlock *block, ram_addr_t offset)
>      return size;
>  }
>  
> +/* Reduce amount of guest cpu execution to hopefully slow down memory writes.
> + * If guest dirty memory rate is reduced below the rate at which we can
> + * transfer pages to the destination then we should be able to complete
> + * migration. Some workloads dirty memory way too fast and will not effectively
> + * converge, even with auto-converge.
> + */
> +static void mig_throttle_guest_down(void)
> +{
> +    MigrationState *s = migrate_get_current();
> +    uint64_t pct_initial =
> +            s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL];
> +    uint64_t pct_icrement =
> +            s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT];
> +
> +    /* We have not started throttling yet. Let's start it. */
> +    if (!cpu_throttle_active()) {
> +        cpu_throttle_set(pct_initial);
> +    } else {
> +        /* Throttling already on, just increase the rate */
> +        cpu_throttle_set(cpu_throttle_get_percentage() + pct_icrement);
> +    }

Shouldn't this cap it at 100% ?

> +}
> +
>  /* Update the xbzrle cache to reflect a page that's been sent as all 0.
>   * The important thing is that a stale (not-yet-0'd) page be replaced
>   * by the new data.
> @@ -714,21 +735,21 @@ static void migration_bitmap_sync(void)
>              /* The following detection logic can be refined later. For now:
>                 Check to see if the dirtied bytes is 50% more than the approx.
>                 amount of bytes that just got transferred since the last time we
> -               were in this routine. If that happens >N times (for now N==4)
> -               we turn on the throttle down logic */
> +               were in this routine. If that happens twice, start or increase
> +               throttling */
>              bytes_xfer_now = ram_bytes_transferred();
> +
>              if (s->dirty_pages_rate &&
>                 (num_dirty_pages_period * TARGET_PAGE_SIZE >
>                     (bytes_xfer_now - bytes_xfer_prev)/2) &&
> -               (dirty_rate_high_cnt++ > 4)) {
> +               (dirty_rate_high_cnt++ >= 2)) {
>                      trace_migration_throttle();
> -                    mig_throttle_on = true;
>                      dirty_rate_high_cnt = 0;
> +                    mig_throttle_guest_down();
>               }
>               bytes_xfer_prev = bytes_xfer_now;
> -        } else {
> -             mig_throttle_on = false;
>          }
> +
>          if (migrate_use_xbzrle()) {
>              if (iterations_prev != acct_info.iterations) {
>                  acct_info.xbzrle_cache_miss_rate =
> @@ -1197,7 +1218,6 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>      RAMBlock *block;
>      int64_t ram_bitmap_pages; /* Size of bitmap in pages, including gaps */
>  
> -    mig_throttle_on = false;
>      dirty_rate_high_cnt = 0;
>      bitmap_sync_count = 0;
>      migration_bitmap_sync_init();
> @@ -1301,12 +1321,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>          }
>          pages_sent += pages;
>          acct_info.iterations++;
> -        check_guest_throttling();
> -        /* we want to check in the 1st loop, just in case it was the 1st time
> -           and we had to sync the dirty bitmap.
> -           qemu_get_clock_ns() is a bit expensive, so we only check each some
> -           iterations
> -        */
> +

Those comments are related to the code below aren't they, not the line you removed?

>          if ((i & 63) == 0) {
>              uint64_t t1 = (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - t0) / 1000000;
>              if (t1 > MAX_WAIT) {
> @@ -1913,51 +1928,3 @@ TargetInfo *qmp_query_target(Error **errp)
>      return info;
>  }
>  
> -/* Stub function that's gets run on the vcpu when its brought out of the
> -   VM to run inside qemu via async_run_on_cpu()*/
> -static void mig_sleep_cpu(void *opq)
> -{
> -    qemu_mutex_unlock_iothread();
> -    g_usleep(30*1000);
> -    qemu_mutex_lock_iothread();
> -}
> -
> -/* To reduce the dirty rate explicitly disallow the VCPUs from spending
> -   much time in the VM. The migration thread will try to catchup.
> -   Workload will experience a performance drop.
> -*/
> -static void mig_throttle_guest_down(void)
> -{
> -    CPUState *cpu;
> -
> -    qemu_mutex_lock_iothread();
> -    CPU_FOREACH(cpu) {
> -        async_run_on_cpu(cpu, mig_sleep_cpu, NULL);
> -    }
> -    qemu_mutex_unlock_iothread();
> -}
> -
> -static void check_guest_throttling(void)
> -{
> -    static int64_t t0;
> -    int64_t        t1;
> -
> -    if (!mig_throttle_on) {
> -        return;
> -    }
> -
> -    if (!t0)  {
> -        t0 = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> -        return;
> -    }
> -
> -    t1 = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> -
> -    /* If it has been more than 40 ms since the last time the guest
> -     * was throttled then do it again.
> -     */
> -    if (40 < (t1-t0)/1000000) {
> -        mig_throttle_guest_down();
> -        t0 = t1;
> -    }
> -}
> diff --git a/migration/migration.c b/migration/migration.c
> index 05790e9..7708c54 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -25,6 +25,7 @@
>  #include "qemu/thread.h"
>  #include "qmp-commands.h"
>  #include "trace.h"
> +#include "qom/cpu.h"
>  
>  #define MAX_THROTTLE  (32 << 20)      /* Migration speed throttling */
>  
> @@ -858,6 +859,9 @@ static void *migration_thread(void *opaque)
>          }
>      }
>  
> +    /* If we enabled cpu throttling for auto-converge, turn it off. */
> +    cpu_throttle_stop();
> +
>      qemu_mutex_lock_iothread();
>      if (s->state == MIGRATION_STATUS_COMPLETED) {
>          int64_t end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);

Is that cpu_throttle_stop() sufficient if I use 'migration cancel'
so that next time through it's all reset so that there's no throttling
at the beginning?

Dave

> -- 
> 1.9.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 1/5] cpu: Provide vcpu throttling interface

[Dr. David Alan Gilbert]

* Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
> Provide a method to throttle guest cpu execution. CPUState is augmented with
> timeout controls and throttle start/stop functions. To throttle the guest cpu
> the caller simply has to call the throttle set function and provide a percentage
> of throttle time.

I'm worried about atomicity and threads and all those fun things.

I think all the starting/stopping/setting the throttling level is done in the
migration thread; I think the timers run in the main/io thread?
So you really need to be careful with at least:
    throttle_timer_stop - which may have a minor effect  
    throttle_timer  - I worry about the way cpu_timer_active checks the pointer
                      yet it's freed when the timer goes off.   It's probably
                      not too bad because it never dereferences it.

So, probably need some atomic's in there (cc'ing paolo)

Dave

> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
> Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
>  cpus.c            | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/qom/cpu.h | 38 ++++++++++++++++++++++++++++
>  2 files changed, 114 insertions(+)
> 
> diff --git a/cpus.c b/cpus.c
> index de6469f..f57cf4f 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -68,6 +68,16 @@ static CPUState *next_cpu;
>  int64_t max_delay;
>  int64_t max_advance;
>  
> +/* vcpu throttling controls */
> +static QEMUTimer *throttle_timer;
> +static bool throttle_timer_stop;
> +static int throttle_percentage;

Unsigned?

> +static float throttle_ratio;
> +
> +#define CPU_THROTTLE_PCT_MIN 1
> +#define CPU_THROTTLE_PCT_MAX 99
> +#define CPU_THROTTLE_TIMESLICE 10
> +
>  bool cpu_is_stopped(CPUState *cpu)
>  {
>      return cpu->stopped || !runstate_is_running();
> @@ -919,6 +929,72 @@ static void qemu_kvm_wait_io_event(CPUState *cpu)
>      qemu_wait_io_event_common(cpu);
>  }
>  
> +static void cpu_throttle_thread(void *opaque)
> +{
> +    long sleeptime_ms = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE);
> +
> +    qemu_mutex_unlock_iothread();
> +    g_usleep(sleeptime_ms * 1000); /* Convert ms to us for usleep call */
> +    qemu_mutex_lock_iothread();
> +
> +    timer_mod(throttle_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
> +                                   CPU_THROTTLE_TIMESLICE);
> +}
> +
> +static void cpu_throttle_timer_pop(void *opaque)
> +{
> +    CPUState *cpu;
> +
> +    /* Stop the timer if needed */
> +    if (throttle_timer_stop) {
> +        timer_del(throttle_timer);
> +        timer_free(throttle_timer);
> +        throttle_timer = NULL;
> +        return;
> +    }
> +
> +    CPU_FOREACH(cpu) {
> +        async_run_on_cpu(cpu, cpu_throttle_thread, NULL);
> +    }
> +}

Why pop? I pop stacks, balloons and bubbles.

> +
> +void cpu_throttle_set(int new_throttle_pct)
> +{
> +    double pct;
> +
> +    /* Ensure throttle percentage is within valid range */
> +    new_throttle_pct = MIN(new_throttle_pct, CPU_THROTTLE_PCT_MAX);
> +    throttle_percentage = MAX(new_throttle_pct, CPU_THROTTLE_PCT_MIN);
> +
> +    pct = (double)throttle_percentage/100;
> +    throttle_ratio = pct / (1 - pct);
> +
> +    if (!cpu_throttle_active()) {
> +        throttle_timer_stop = false;
> +        throttle_timer = timer_new_ms(QEMU_CLOCK_REALTIME,
> +                                           cpu_throttle_timer_pop, NULL);
> +        timer_mod(throttle_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
> +                                       CPU_THROTTLE_TIMESLICE);
> +    }
> +}
> +
> +void cpu_throttle_stop(void)
> +{
> +    if (cpu_throttle_active()) {
> +        throttle_timer_stop = true;
> +    }
> +}
> +
> +bool cpu_throttle_active(void)
> +{
> +    return (throttle_timer != NULL);
> +}
> +
> +int cpu_throttle_get_percentage(void)
> +{
> +    return throttle_percentage;
> +}
> +
>  static void *qemu_kvm_cpu_thread_fn(void *arg)
>  {
>      CPUState *cpu = arg;
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 39f0f19..56eb964 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -553,6 +553,44 @@ CPUState *qemu_get_cpu(int index);
>   */
>  bool cpu_exists(int64_t id);
>  
> +/**
> + * cpu_throttle_set:
> + * @new_throttle_pct: Percent of sleep time to running time.
> + *                    Valid range is 1 to 99.
> + *
> + * Throttles all vcpus by forcing them to sleep for the given percentage of
> + * time. A throttle_percentage of 50 corresponds to a 50% duty cycle roughly.
> + * (example: 10ms sleep for every 10ms awake).
> + *
> + * cpu_throttle_set can be called as needed to adjust new_throttle_pct.
> + * Once the throttling starts, it will remain in effect until cpu_throttle_stop
> + * is called.
> + */
> +void cpu_throttle_set(int new_throttle_pct);
> +
> +/**
> + * cpu_throttle_stop:
> + *
> + * Stops the vcpu throttling started by cpu_throttle_set.
> + */
> +void cpu_throttle_stop(void);
> +
> +/**
> + * cpu_throttle_active:
> + *
> + * Returns %true if the vcpus are currently being throttled, %false otherwise.
> + */
> +bool cpu_throttle_active(void);
> +
> +/**
> + * cpu_throttle_get_percentage:
> + *
> + * Returns the vcpu throttle percentage. See cpu_throttle_set for details.
> + *
> + * Returns The throttle percentage in range 1 to 99.
> + */
> +int cpu_throttle_get_percentage(void);
> +
>  #ifndef CONFIG_USER_ONLY
>  
>  typedef void (*CPUInterruptHandler)(CPUState *, int);
> -- 
> 1.9.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 4/5] qmp/hmp: Add throttle ratio to query-migrate and info migrate

[Dr. David Alan Gilbert]

* Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
> Report throttle percentage in info migrate and query-migrate responses when
> cpu throttling is active.
> 
> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hmp.c                 | 5 +++++
>  migration/migration.c | 5 +++++
>  qapi-schema.json      | 7 ++++++-
>  3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/hmp.c b/hmp.c
> index eb65998..36bc76e 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -229,6 +229,11 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>                         info->xbzrle_cache->overflow);
>      }
>  
> +    if (info->has_x_cpu_throttle_percentage) {
> +        monitor_printf(mon, "cpu throttle percentage: %" PRIu64 "\n",
> +                       info->x_cpu_throttle_percentage);
> +    }
> +
>      qapi_free_MigrationInfo(info);
>      qapi_free_MigrationCapabilityStatusList(caps);
>  }
> diff --git a/migration/migration.c b/migration/migration.c
> index 7708c54..b29450a 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -274,6 +274,11 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>              info->disk->total = blk_mig_bytes_total();
>          }
>  
> +        if (cpu_throttle_active()) {
> +            info->has_x_cpu_throttle_percentage = true;
> +            info->x_cpu_throttle_percentage = cpu_throttle_get_percentage();
> +        }
> +
>          get_xbzrle_cache_stats(info);
>          break;
>      case MIGRATION_STATUS_COMPLETED:
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 7dd324e..cd36256 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -474,6 +474,10 @@
>  #        may be expensive, but do not actually occur during the iterative
>  #        migration rounds themselves. (since 1.6)
>  #
> +# @x-cpu-throttle-percentage: #optional percentage of time guest cpus are being
> +#       throttled during auto-converge. This is only present when auto-converge
> +#       has started throttling guest cpus. (Since 2.4)
> +#
>  # Since: 0.14.0
>  ##
>  { 'struct': 'MigrationInfo',
> @@ -483,7 +487,8 @@
>             '*total-time': 'int',
>             '*expected-downtime': 'int',
>             '*downtime': 'int',
> -           '*setup-time': 'int'} }
> +           '*setup-time': 'int',
> +           '*x-cpu-throttle-percentage': 'int'} }
>  
>  ##
>  # @query-migrate
> -- 
> 1.9.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 5/5] migration: Disambiguate MAX_THROTTLE

[Dr. David Alan Gilbert]

* Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
> Migration has a define for MAX_THROTTLE. Update comment to clarify that this is
> used for throttling transfer speed. Hopefully this will prevent it from being
> confused with a guest cpu throttling entity.
> 
> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Dave
> ---
>  migration/migration.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index b29450a..b9faeb0 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -27,7 +27,7 @@
>  #include "trace.h"
>  #include "qom/cpu.h"
>  
> -#define MAX_THROTTLE  (32 << 20)      /* Migration speed throttling */
> +#define MAX_THROTTLE  (32 << 20)      /* Migration transfer speed throttling */
>  
>  /* Amount of time to allocate to each "chunk" of bandwidth-throttled
>   * data. */
> -- 
> 1.9.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 3/5] migration: Dynamic cpu throttling for auto-converge

[Jason J. Herne]

On 06/26/2015 01:54 PM, Dr. David Alan Gilbert wrote:
> * Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
>> Remove traditional auto-converge static 30ms throttling code and replace it
>> with a dynamic throttling algorithm.
>>
>> Additionally, be more aggressive when deciding when to start throttling.
>> Previously we waited until four unproductive memory passes. Now we begin
>> throttling after only two unproductive memory passes. Four seemed quite
>> arbitrary and only waiting for two passes allows us to complete the migration
>> faster.
>>
>> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
>> Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>> ---
>>   arch_init.c           | 93 +++++++++++++++++----------------------------------
>>   migration/migration.c |  4 +++
>>   2 files changed, 34 insertions(+), 63 deletions(-)
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index 23d3feb..d456527 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -111,9 +111,7 @@ int graphic_depth = 32;
>>   #endif
>>
>>   const uint32_t arch_type = QEMU_ARCH;
>> -static bool mig_throttle_on;
>>   static int dirty_rate_high_cnt;
>> -static void check_guest_throttling(void);
>>
>>   static uint64_t bitmap_sync_count;
>>
>> @@ -487,6 +485,29 @@ static size_t save_page_header(QEMUFile *f, RAMBlock *block, ram_addr_t offset)
>>       return size;
>>   }
>>
>> +/* Reduce amount of guest cpu execution to hopefully slow down memory writes.
>> + * If guest dirty memory rate is reduced below the rate at which we can
>> + * transfer pages to the destination then we should be able to complete
>> + * migration. Some workloads dirty memory way too fast and will not effectively
>> + * converge, even with auto-converge.
>> + */
>> +static void mig_throttle_guest_down(void)
>> +{
>> +    MigrationState *s = migrate_get_current();
>> +    uint64_t pct_initial =
>> +            s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INITIAL];
>> +    uint64_t pct_icrement =
>> +            s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT];
>> +
>> +    /* We have not started throttling yet. Let's start it. */
>> +    if (!cpu_throttle_active()) {
>> +        cpu_throttle_set(pct_initial);
>> +    } else {
>> +        /* Throttling already on, just increase the rate */
>> +        cpu_throttle_set(cpu_throttle_get_percentage() + pct_icrement);
>> +    }
>
> Shouldn't this cap it at 100% ?
>

The code in cpu_throttle_set() actually caps it at 99 percent.

...
>> @@ -1197,7 +1218,6 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>>       RAMBlock *block;
>>       int64_t ram_bitmap_pages; /* Size of bitmap in pages, including gaps */
>>
>> -    mig_throttle_on = false;
>>       dirty_rate_high_cnt = 0;
>>       bitmap_sync_count = 0;
>>       migration_bitmap_sync_init();
>> @@ -1301,12 +1321,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>>           }
>>           pages_sent += pages;
>>           acct_info.iterations++;
>> -        check_guest_throttling();
>> -        /* we want to check in the 1st loop, just in case it was the 1st time
>> -           and we had to sync the dirty bitmap.
>> -           qemu_get_clock_ns() is a bit expensive, so we only check each some
>> -           iterations
>> -        */
>> +
>
> Those comments are related to the code below aren't they, not the line you removed?
>

Yes, oversight on my part. I will re-add them for v4 :) Thanks!!

>>           if ((i & 63) == 0) {
>>               uint64_t t1 = (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - t0) / 1000000;
>>               if (t1 > MAX_WAIT) {
>> @@ -1913,51 +1928,3 @@ TargetInfo *qmp_query_target(Error **errp)
>>       return info;
>>   }
>>
...
-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)

Re: [Qemu-devel] [PATCH v3 1/5] cpu: Provide vcpu throttling interface

[Jason J. Herne]

On 06/26/2015 02:07 PM, Dr. David Alan Gilbert wrote:
> * Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
>> Provide a method to throttle guest cpu execution. CPUState is augmented with
>> timeout controls and throttle start/stop functions. To throttle the guest cpu
>> the caller simply has to call the throttle set function and provide a percentage
>> of throttle time.
>
> I'm worried about atomicity and threads and all those fun things.
>
> I think all the starting/stopping/setting the throttling level is done in the
> migration thread; I think the timers run in the main/io thread?
> So you really need to be careful with at least:
>      throttle_timer_stop - which may have a minor effect
>      throttle_timer  - I worry about the way cpu_timer_active checks the pointer
>                        yet it's freed when the timer goes off.   It's probably
>                        not too bad because it never dereferences it.
>
> So, probably need some atomic's in there (cc'ing paolo)
>
> Dave
>

I think we're ok with respect to throttle_timer. As you mentioned, we 
rely on the
value only to know if throttling is active or not.

I'm not seeing any other race conditions or serialization issues. But 
that doesn't
mean the code is perfect either :)

>> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
>> Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>> ---
>>   cpus.c            | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/qom/cpu.h | 38 ++++++++++++++++++++++++++++
>>   2 files changed, 114 insertions(+)
>>
>> diff --git a/cpus.c b/cpus.c
>> index de6469f..f57cf4f 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -68,6 +68,16 @@ static CPUState *next_cpu;
>>   int64_t max_delay;
>>   int64_t max_advance;
>>
>> +/* vcpu throttling controls */
>> +static QEMUTimer *throttle_timer;
>> +static bool throttle_timer_stop;
>> +static int throttle_percentage;
>
> Unsigned?
>

Yep. Can do.

>> +static float throttle_ratio;
>> +
>> +#define CPU_THROTTLE_PCT_MIN 1
>> +#define CPU_THROTTLE_PCT_MAX 99
>> +#define CPU_THROTTLE_TIMESLICE 10
>> +
>>   bool cpu_is_stopped(CPUState *cpu)
>>   {
>>       return cpu->stopped || !runstate_is_running();
>> @@ -919,6 +929,72 @@ static void qemu_kvm_wait_io_event(CPUState *cpu)
>>       qemu_wait_io_event_common(cpu);
>>   }
>>
>> +static void cpu_throttle_thread(void *opaque)
>> +{
>> +    long sleeptime_ms = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE);
>> +
>> +    qemu_mutex_unlock_iothread();
>> +    g_usleep(sleeptime_ms * 1000); /* Convert ms to us for usleep call */
>> +    qemu_mutex_lock_iothread();
>> +
>> +    timer_mod(throttle_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
>> +                                   CPU_THROTTLE_TIMESLICE);
>> +}
>> +
>> +static void cpu_throttle_timer_pop(void *opaque)
>> +{
>> +    CPUState *cpu;
>> +
>> +    /* Stop the timer if needed */
>> +    if (throttle_timer_stop) {
>> +        timer_del(throttle_timer);
>> +        timer_free(throttle_timer);
>> +        throttle_timer = NULL;
>> +        return;
>> +    }
>> +
>> +    CPU_FOREACH(cpu) {
>> +        async_run_on_cpu(cpu, cpu_throttle_thread, NULL);
>> +    }
>> +}
>
> Why pop? I pop stacks, balloons and bubbles.
>

Hmmm... timer pops are a very common term in System Z land :). But then
again we do have a ton of odd terminology around here. Do you have a
better suggestion?  cpu_throttle_timer_expire? cpu_throttle_timer_tick?

>> +
>> +void cpu_throttle_set(int new_throttle_pct)
>> +{
>> +    double pct;
>> +
>> +    /* Ensure throttle percentage is within valid range */
>> +    new_throttle_pct = MIN(new_throttle_pct, CPU_THROTTLE_PCT_MAX);
>> +    throttle_percentage = MAX(new_throttle_pct, CPU_THROTTLE_PCT_MIN);
>> +
>> +    pct = (double)throttle_percentage/100;
>> +    throttle_ratio = pct / (1 - pct);
>> +
>> +    if (!cpu_throttle_active()) {
>> +        throttle_timer_stop = false;
>> +        throttle_timer = timer_new_ms(QEMU_CLOCK_REALTIME,
>> +                                           cpu_throttle_timer_pop, NULL);
>> +        timer_mod(throttle_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
>> +                                       CPU_THROTTLE_TIMESLICE);
>> +    }
>> +}
>> +
>> +void cpu_throttle_stop(void)
>> +{
>> +    if (cpu_throttle_active()) {
>> +        throttle_timer_stop = true;
>> +    }
>> +}
>> +
>> +bool cpu_throttle_active(void)
>> +{
>> +    return (throttle_timer != NULL);
>> +}
>> +
>> +int cpu_throttle_get_percentage(void)
>> +{
>> +    return throttle_percentage;
>> +}
>> +
>>   static void *qemu_kvm_cpu_thread_fn(void *arg)
>>   {
>>       CPUState *cpu = arg;
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index 39f0f19..56eb964 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -553,6 +553,44 @@ CPUState *qemu_get_cpu(int index);
>>    */
>>   bool cpu_exists(int64_t id);
>>
>> +/**
>> + * cpu_throttle_set:
>> + * @new_throttle_pct: Percent of sleep time to running time.
>> + *                    Valid range is 1 to 99.
>> + *
>> + * Throttles all vcpus by forcing them to sleep for the given percentage of
>> + * time. A throttle_percentage of 50 corresponds to a 50% duty cycle roughly.
>> + * (example: 10ms sleep for every 10ms awake).
>> + *
>> + * cpu_throttle_set can be called as needed to adjust new_throttle_pct.
>> + * Once the throttling starts, it will remain in effect until cpu_throttle_stop
>> + * is called.
>> + */
>> +void cpu_throttle_set(int new_throttle_pct);
>> +
>> +/**
>> + * cpu_throttle_stop:
>> + *
>> + * Stops the vcpu throttling started by cpu_throttle_set.
>> + */
>> +void cpu_throttle_stop(void);
>> +
>> +/**
>> + * cpu_throttle_active:
>> + *
>> + * Returns %true if the vcpus are currently being throttled, %false otherwise.
>> + */
>> +bool cpu_throttle_active(void);
>> +
>> +/**
>> + * cpu_throttle_get_percentage:
>> + *
>> + * Returns the vcpu throttle percentage. See cpu_throttle_set for details.
>> + *
>> + * Returns The throttle percentage in range 1 to 99.
>> + */
>> +int cpu_throttle_get_percentage(void);
>> +
>>   #ifndef CONFIG_USER_ONLY
>>
>>   typedef void (*CPUInterruptHandler)(CPUState *, int);
>> --
>> 1.9.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>

-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)

Re: [Qemu-devel] [PATCH v3 3/5] migration: Dynamic cpu throttling for auto-converge

[Jason J. Herne]

On 06/26/2015 01:54 PM, Dr. David Alan Gilbert wrote:
> * Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
...
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 05790e9..7708c54 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -25,6 +25,7 @@
>>   #include "qemu/thread.h"
>>   #include "qmp-commands.h"
>>   #include "trace.h"
>> +#include "qom/cpu.h"
>>
>>   #define MAX_THROTTLE  (32 << 20)      /* Migration speed throttling */
>>
>> @@ -858,6 +859,9 @@ static void *migration_thread(void *opaque)
>>           }
>>       }
>>
>> +    /* If we enabled cpu throttling for auto-converge, turn it off. */
>> +    cpu_throttle_stop();
>> +
>>       qemu_mutex_lock_iothread();
>>       if (s->state == MIGRATION_STATUS_COMPLETED) {
>>           int64_t end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>
> Is that cpu_throttle_stop() sufficient if I use 'migration cancel'
> so that next time through it's all reset so that there's no throttling
> at the beginning?
>

It will be reset when cpu_throttle_set is called again when auto-converge
re-enables throttling for the next migration. This happens in patch 3,
mig_throttle_guest_down():

Basically, cpu_throttle_set requires the user to provide a throttling
percentage. So starting a new throttling operation will overwrite the
previous value.

-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)

Re: [Qemu-devel] [PATCH v3 1/5] cpu: Provide vcpu throttling interface

[Dr. David Alan Gilbert]

* Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
> On 06/26/2015 02:07 PM, Dr. David Alan Gilbert wrote:
> >* Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
> >>Provide a method to throttle guest cpu execution. CPUState is augmented with
> >>timeout controls and throttle start/stop functions. To throttle the guest cpu
> >>the caller simply has to call the throttle set function and provide a percentage
> >>of throttle time.
> >
> >I'm worried about atomicity and threads and all those fun things.
> >
> >I think all the starting/stopping/setting the throttling level is done in the
> >migration thread; I think the timers run in the main/io thread?
> >So you really need to be careful with at least:
> >     throttle_timer_stop - which may have a minor effect
> >     throttle_timer  - I worry about the way cpu_timer_active checks the pointer
> >                       yet it's freed when the timer goes off.   It's probably
> >                       not too bad because it never dereferences it.
> >
> >So, probably need some atomic's in there (cc'ing paolo)
> >
> >Dave
> >
> 
> I think we're ok with respect to throttle_timer. As you mentioned, we rely
> on the
> value only to know if throttling is active or not.
> 
> I'm not seeing any other race conditions or serialization issues. But that
> doesn't
> mean the code is perfect either :)
> 
> >>Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
> >>Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> >>---
> >>  cpus.c            | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/qom/cpu.h | 38 ++++++++++++++++++++++++++++
> >>  2 files changed, 114 insertions(+)
> >>
> >>diff --git a/cpus.c b/cpus.c
> >>index de6469f..f57cf4f 100644
> >>--- a/cpus.c
> >>+++ b/cpus.c
> >>@@ -68,6 +68,16 @@ static CPUState *next_cpu;
> >>  int64_t max_delay;
> >>  int64_t max_advance;
> >>
> >>+/* vcpu throttling controls */
> >>+static QEMUTimer *throttle_timer;
> >>+static bool throttle_timer_stop;
> >>+static int throttle_percentage;
> >
> >Unsigned?
> >
> 
> Yep. Can do.
> 
> >>+static float throttle_ratio;
> >>+
> >>+#define CPU_THROTTLE_PCT_MIN 1
> >>+#define CPU_THROTTLE_PCT_MAX 99
> >>+#define CPU_THROTTLE_TIMESLICE 10
> >>+
> >>  bool cpu_is_stopped(CPUState *cpu)
> >>  {
> >>      return cpu->stopped || !runstate_is_running();
> >>@@ -919,6 +929,72 @@ static void qemu_kvm_wait_io_event(CPUState *cpu)
> >>      qemu_wait_io_event_common(cpu);
> >>  }
> >>
> >>+static void cpu_throttle_thread(void *opaque)
> >>+{
> >>+    long sleeptime_ms = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE);
> >>+
> >>+    qemu_mutex_unlock_iothread();
> >>+    g_usleep(sleeptime_ms * 1000); /* Convert ms to us for usleep call */
> >>+    qemu_mutex_lock_iothread();
> >>+
> >>+    timer_mod(throttle_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
> >>+                                   CPU_THROTTLE_TIMESLICE);
> >>+}
> >>+
> >>+static void cpu_throttle_timer_pop(void *opaque)
> >>+{
> >>+    CPUState *cpu;
> >>+
> >>+    /* Stop the timer if needed */
> >>+    if (throttle_timer_stop) {
> >>+        timer_del(throttle_timer);
> >>+        timer_free(throttle_timer);
> >>+        throttle_timer = NULL;
> >>+        return;
> >>+    }
> >>+
> >>+    CPU_FOREACH(cpu) {
> >>+        async_run_on_cpu(cpu, cpu_throttle_thread, NULL);
> >>+    }
> >>+}
> >
> >Why pop? I pop stacks, balloons and bubbles.
> >
> 
> Hmmm... timer pops are a very common term in System Z land :). But then
> again we do have a ton of odd terminology around here. Do you have a
> better suggestion?  cpu_throttle_timer_expire? cpu_throttle_timer_tick?

My preference would be _tick.

Dave

> 
> >>+
> >>+void cpu_throttle_set(int new_throttle_pct)
> >>+{
> >>+    double pct;
> >>+
> >>+    /* Ensure throttle percentage is within valid range */
> >>+    new_throttle_pct = MIN(new_throttle_pct, CPU_THROTTLE_PCT_MAX);
> >>+    throttle_percentage = MAX(new_throttle_pct, CPU_THROTTLE_PCT_MIN);
> >>+
> >>+    pct = (double)throttle_percentage/100;
> >>+    throttle_ratio = pct / (1 - pct);
> >>+
> >>+    if (!cpu_throttle_active()) {
> >>+        throttle_timer_stop = false;
> >>+        throttle_timer = timer_new_ms(QEMU_CLOCK_REALTIME,
> >>+                                           cpu_throttle_timer_pop, NULL);
> >>+        timer_mod(throttle_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
> >>+                                       CPU_THROTTLE_TIMESLICE);
> >>+    }
> >>+}
> >>+
> >>+void cpu_throttle_stop(void)
> >>+{
> >>+    if (cpu_throttle_active()) {
> >>+        throttle_timer_stop = true;
> >>+    }
> >>+}
> >>+
> >>+bool cpu_throttle_active(void)
> >>+{
> >>+    return (throttle_timer != NULL);
> >>+}
> >>+
> >>+int cpu_throttle_get_percentage(void)
> >>+{
> >>+    return throttle_percentage;
> >>+}
> >>+
> >>  static void *qemu_kvm_cpu_thread_fn(void *arg)
> >>  {
> >>      CPUState *cpu = arg;
> >>diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> >>index 39f0f19..56eb964 100644
> >>--- a/include/qom/cpu.h
> >>+++ b/include/qom/cpu.h
> >>@@ -553,6 +553,44 @@ CPUState *qemu_get_cpu(int index);
> >>   */
> >>  bool cpu_exists(int64_t id);
> >>
> >>+/**
> >>+ * cpu_throttle_set:
> >>+ * @new_throttle_pct: Percent of sleep time to running time.
> >>+ *                    Valid range is 1 to 99.
> >>+ *
> >>+ * Throttles all vcpus by forcing them to sleep for the given percentage of
> >>+ * time. A throttle_percentage of 50 corresponds to a 50% duty cycle roughly.
> >>+ * (example: 10ms sleep for every 10ms awake).
> >>+ *
> >>+ * cpu_throttle_set can be called as needed to adjust new_throttle_pct.
> >>+ * Once the throttling starts, it will remain in effect until cpu_throttle_stop
> >>+ * is called.
> >>+ */
> >>+void cpu_throttle_set(int new_throttle_pct);
> >>+
> >>+/**
> >>+ * cpu_throttle_stop:
> >>+ *
> >>+ * Stops the vcpu throttling started by cpu_throttle_set.
> >>+ */
> >>+void cpu_throttle_stop(void);
> >>+
> >>+/**
> >>+ * cpu_throttle_active:
> >>+ *
> >>+ * Returns %true if the vcpus are currently being throttled, %false otherwise.
> >>+ */
> >>+bool cpu_throttle_active(void);
> >>+
> >>+/**
> >>+ * cpu_throttle_get_percentage:
> >>+ *
> >>+ * Returns the vcpu throttle percentage. See cpu_throttle_set for details.
> >>+ *
> >>+ * Returns The throttle percentage in range 1 to 99.
> >>+ */
> >>+int cpu_throttle_get_percentage(void);
> >>+
> >>  #ifndef CONFIG_USER_ONLY
> >>
> >>  typedef void (*CPUInterruptHandler)(CPUState *, int);
> >>--
> >>1.9.1
> >>
> >--
> >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
> 
> -- 
> -- Jason J. Herne (jjherne@linux.vnet.ibm.com)
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 1/5] cpu: Provide vcpu throttling interface

[Jason J. Herne]

On 06/26/2015 03:02 PM, Jason J. Herne wrote:
> On 06/26/2015 02:07 PM, Dr. David Alan Gilbert wrote:
>> * Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
>>> Provide a method to throttle guest cpu execution. CPUState is
>>> augmented with
>>> timeout controls and throttle start/stop functions. To throttle the
>>> guest cpu
>>> the caller simply has to call the throttle set function and provide a
>>> percentage
>>> of throttle time.
>>
>> I'm worried about atomicity and threads and all those fun things.
>>
>> I think all the starting/stopping/setting the throttling level is done
>> in the
>> migration thread; I think the timers run in the main/io thread?
>> So you really need to be careful with at least:
>>      throttle_timer_stop - which may have a minor effect
>>      throttle_timer  - I worry about the way cpu_timer_active checks
>> the pointer
>>                        yet it's freed when the timer goes off.   It's
>> probably
>>                        not too bad because it never dereferences it.
>>
>> So, probably need some atomic's in there (cc'ing paolo)
>>
>> Dave
>>
>
> I think we're ok with respect to throttle_timer. As you mentioned, we
> rely on the
> value only to know if throttling is active or not.
>
> I'm not seeing any other race conditions or serialization issues. But
> that doesn't
> mean the code is perfect either :)
>

Is there any more discussion on this before I send a v4? Paolo?


>>> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
>>> Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>>> ---
>>>   cpus.c            | 76
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   include/qom/cpu.h | 38 ++++++++++++++++++++++++++++
>>>   2 files changed, 114 insertions(+)
>>>
>>> diff --git a/cpus.c b/cpus.c
>>> index de6469f..f57cf4f 100644
>>> --- a/cpus.c
>>> +++ b/cpus.c
>>> @@ -68,6 +68,16 @@ static CPUState *next_cpu;
>>>   int64_t max_delay;
>>>   int64_t max_advance;
>>>
>>> +/* vcpu throttling controls */
>>> +static QEMUTimer *throttle_timer;
>>> +static bool throttle_timer_stop;
>>> +static int throttle_percentage;
>>
>> Unsigned?
>>
>
> Yep. Can do.
>
>>> +static float throttle_ratio;
>>> +
>>> +#define CPU_THROTTLE_PCT_MIN 1
>>> +#define CPU_THROTTLE_PCT_MAX 99
>>> +#define CPU_THROTTLE_TIMESLICE 10
>>> +
>>>   bool cpu_is_stopped(CPUState *cpu)
>>>   {
>>>       return cpu->stopped || !runstate_is_running();
>>> @@ -919,6 +929,72 @@ static void qemu_kvm_wait_io_event(CPUState *cpu)
>>>       qemu_wait_io_event_common(cpu);
>>>   }
>>>
>>> +static void cpu_throttle_thread(void *opaque)
>>> +{
>>> +    long sleeptime_ms = (long)(throttle_ratio *
>>> CPU_THROTTLE_TIMESLICE);
>>> +
>>> +    qemu_mutex_unlock_iothread();
>>> +    g_usleep(sleeptime_ms * 1000); /* Convert ms to us for usleep
>>> call */
>>> +    qemu_mutex_lock_iothread();
>>> +
>>> +    timer_mod(throttle_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
>>> +                                   CPU_THROTTLE_TIMESLICE);
>>> +}
>>> +
>>> +static void cpu_throttle_timer_pop(void *opaque)
>>> +{
>>> +    CPUState *cpu;
>>> +
>>> +    /* Stop the timer if needed */
>>> +    if (throttle_timer_stop) {
>>> +        timer_del(throttle_timer);
>>> +        timer_free(throttle_timer);
>>> +        throttle_timer = NULL;
>>> +        return;
>>> +    }
>>> +
>>> +    CPU_FOREACH(cpu) {
>>> +        async_run_on_cpu(cpu, cpu_throttle_thread, NULL);
>>> +    }
>>> +}
>>
>> Why pop? I pop stacks, balloons and bubbles.
>>
>
> Hmmm... timer pops are a very common term in System Z land :). But then
> again we do have a ton of odd terminology around here. Do you have a
> better suggestion?  cpu_throttle_timer_expire? cpu_throttle_timer_tick?
>
>>> +
>>> +void cpu_throttle_set(int new_throttle_pct)
>>> +{
>>> +    double pct;
>>> +
>>> +    /* Ensure throttle percentage is within valid range */
>>> +    new_throttle_pct = MIN(new_throttle_pct, CPU_THROTTLE_PCT_MAX);
>>> +    throttle_percentage = MAX(new_throttle_pct, CPU_THROTTLE_PCT_MIN);
>>> +
>>> +    pct = (double)throttle_percentage/100;
>>> +    throttle_ratio = pct / (1 - pct);
>>> +
>>> +    if (!cpu_throttle_active()) {
>>> +        throttle_timer_stop = false;
>>> +        throttle_timer = timer_new_ms(QEMU_CLOCK_REALTIME,
>>> +                                           cpu_throttle_timer_pop,
>>> NULL);
>>> +        timer_mod(throttle_timer,
>>> qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
>>> +                                       CPU_THROTTLE_TIMESLICE);
>>> +    }
>>> +}
>>> +
>>> +void cpu_throttle_stop(void)
>>> +{
>>> +    if (cpu_throttle_active()) {
>>> +        throttle_timer_stop = true;
>>> +    }
>>> +}
>>> +
>>> +bool cpu_throttle_active(void)
>>> +{
>>> +    return (throttle_timer != NULL);
>>> +}
>>> +
>>> +int cpu_throttle_get_percentage(void)
>>> +{
>>> +    return throttle_percentage;
>>> +}
>>> +
>>>   static void *qemu_kvm_cpu_thread_fn(void *arg)
>>>   {
>>>       CPUState *cpu = arg;
>>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>>> index 39f0f19..56eb964 100644
>>> --- a/include/qom/cpu.h
>>> +++ b/include/qom/cpu.h
>>> @@ -553,6 +553,44 @@ CPUState *qemu_get_cpu(int index);
>>>    */
>>>   bool cpu_exists(int64_t id);
>>>
>>> +/**
>>> + * cpu_throttle_set:
>>> + * @new_throttle_pct: Percent of sleep time to running time.
>>> + *                    Valid range is 1 to 99.
>>> + *
>>> + * Throttles all vcpus by forcing them to sleep for the given
>>> percentage of
>>> + * time. A throttle_percentage of 50 corresponds to a 50% duty cycle
>>> roughly.
>>> + * (example: 10ms sleep for every 10ms awake).
>>> + *
>>> + * cpu_throttle_set can be called as needed to adjust new_throttle_pct.
>>> + * Once the throttling starts, it will remain in effect until
>>> cpu_throttle_stop
>>> + * is called.
>>> + */
>>> +void cpu_throttle_set(int new_throttle_pct);
>>> +
>>> +/**
>>> + * cpu_throttle_stop:
>>> + *
>>> + * Stops the vcpu throttling started by cpu_throttle_set.
>>> + */
>>> +void cpu_throttle_stop(void);
>>> +
>>> +/**
>>> + * cpu_throttle_active:
>>> + *
>>> + * Returns %true if the vcpus are currently being throttled, %false
>>> otherwise.
>>> + */
>>> +bool cpu_throttle_active(void);
>>> +
>>> +/**
>>> + * cpu_throttle_get_percentage:
>>> + *
>>> + * Returns the vcpu throttle percentage. See cpu_throttle_set for
>>> details.
>>> + *
>>> + * Returns The throttle percentage in range 1 to 99.
>>> + */
>>> +int cpu_throttle_get_percentage(void);
>>> +
>>>   #ifndef CONFIG_USER_ONLY
>>>
>>>   typedef void (*CPUInterruptHandler)(CPUState *, int);
>>> --
>>> 1.9.1
>>>
>> --
>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>
>

-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)

Re: [Qemu-devel] [PATCH v3 1/5] cpu: Provide vcpu throttling interface

[Paolo Bonzini]

On 25/06/2015 19:46, Jason J. Herne wrote:
> +static void cpu_throttle_thread(void *opaque)
> +{
> +    long sleeptime_ms = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE);
> +
> +    qemu_mutex_unlock_iothread();
> +    g_usleep(sleeptime_ms * 1000); /* Convert ms to us for usleep call */
> +    qemu_mutex_lock_iothread();
> +
> +    timer_mod(throttle_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
> +                                   CPU_THROTTLE_TIMESLICE);

The timer need not run while the VM is stopped.  Please use
QEMU_CLOCK_VIRTUAL_RT.

> +}
> +

Are you sure you want each CPU to set the timer?  I think this should be
done in cpu_throttle_timer_pop, or it could use timer_mod_anticipate.

> +static void cpu_throttle_timer_pop(void *opaque)
> +{
> +    CPUState *cpu;
> +
> +    /* Stop the timer if needed */
> +    if (throttle_timer_stop) {
> +        timer_del(throttle_timer);

timer_del is not needed in the timer callback.

Paolo

> +        timer_free(throttle_timer);
> +        throttle_timer = NULL;
> +        return;
> +    }
> +
> +    CPU_FOREACH(cpu) {
> +        async_run_on_cpu(cpu, cpu_throttle_thread, NULL);
> +    }
> +}
> +

Re: [Qemu-devel] [PATCH v3 1/5] cpu: Provide vcpu throttling interface

[Paolo Bonzini]

On 26/06/2015 20:07, Dr. David Alan Gilbert wrote:
> * Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
>> Provide a method to throttle guest cpu execution. CPUState is augmented with
>> timeout controls and throttle start/stop functions. To throttle the guest cpu
>> the caller simply has to call the throttle set function and provide a percentage
>> of throttle time.
> 
> I'm worried about atomicity and threads and all those fun things.
> 
> I think all the starting/stopping/setting the throttling level is done in the
> migration thread; I think the timers run in the main/io thread?
> So you really need to be careful with at least:
>     throttle_timer_stop - which may have a minor effect  
>     throttle_timer  - I worry about the way cpu_timer_active checks the pointer
>                       yet it's freed when the timer goes off.   It's probably
>                       not too bad because it never dereferences it.

Agreed.  I think the only atomic should be throttle_percentage; if zero,
throttling is inactive.

In particular, throttle_ratio can be computed in cpu_throttle_thread.

If you have exactly one variable that is shared between the threads,
everything is much simpler.

There is no need to allocate and free the timer; it's very cheap and in
fact we probably should convert to statically allocated timers sooner or
later.  So you can just create it once, for example in cpu_ticks_init.

Paolo

> So, probably need some atomic's in there (cc'ing paolo)
> 
> Dave
> 
>> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
>> Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>> ---
>>  cpus.c            | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/qom/cpu.h | 38 ++++++++++++++++++++++++++++
>>  2 files changed, 114 insertions(+)
>>
>> diff --git a/cpus.c b/cpus.c
>> index de6469f..f57cf4f 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -68,6 +68,16 @@ static CPUState *next_cpu;
>>  int64_t max_delay;
>>  int64_t max_advance;
>>  
>> +/* vcpu throttling controls */
>> +static QEMUTimer *throttle_timer;
>> +static bool throttle_timer_stop;
>> +static int throttle_percentage;
> 
> Unsigned?
> 
>> +static float throttle_ratio;
>> +
>> +#define CPU_THROTTLE_PCT_MIN 1
>> +#define CPU_THROTTLE_PCT_MAX 99
>> +#define CPU_THROTTLE_TIMESLICE 10
>> +
>>  bool cpu_is_stopped(CPUState *cpu)
>>  {
>>      return cpu->stopped || !runstate_is_running();
>> @@ -919,6 +929,72 @@ static void qemu_kvm_wait_io_event(CPUState *cpu)
>>      qemu_wait_io_event_common(cpu);
>>  }
>>  
>> +static void cpu_throttle_thread(void *opaque)
>> +{
>> +    long sleeptime_ms = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE);
>> +
>> +    qemu_mutex_unlock_iothread();
>> +    g_usleep(sleeptime_ms * 1000); /* Convert ms to us for usleep call */
>> +    qemu_mutex_lock_iothread();
>> +
>> +    timer_mod(throttle_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
>> +                                   CPU_THROTTLE_TIMESLICE);
>> +}
>> +
>> +static void cpu_throttle_timer_pop(void *opaque)
>> +{
>> +    CPUState *cpu;
>> +
>> +    /* Stop the timer if needed */
>> +    if (throttle_timer_stop) {
>> +        timer_del(throttle_timer);
>> +        timer_free(throttle_timer);
>> +        throttle_timer = NULL;
>> +        return;
>> +    }
>> +
>> +    CPU_FOREACH(cpu) {
>> +        async_run_on_cpu(cpu, cpu_throttle_thread, NULL);
>> +    }
>> +}
> 
> Why pop? I pop stacks, balloons and bubbles.
> 
>> +
>> +void cpu_throttle_set(int new_throttle_pct)
>> +{
>> +    double pct;
>> +
>> +    /* Ensure throttle percentage is within valid range */
>> +    new_throttle_pct = MIN(new_throttle_pct, CPU_THROTTLE_PCT_MAX);
>> +    throttle_percentage = MAX(new_throttle_pct, CPU_THROTTLE_PCT_MIN);
>> +
>> +    pct = (double)throttle_percentage/100;
>> +    throttle_ratio = pct / (1 - pct);
>> +
>> +    if (!cpu_throttle_active()) {
>> +        throttle_timer_stop = false;
>> +        throttle_timer = timer_new_ms(QEMU_CLOCK_REALTIME,
>> +                                           cpu_throttle_timer_pop, NULL);
>> +        timer_mod(throttle_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
>> +                                       CPU_THROTTLE_TIMESLICE);
>> +    }
>> +}
>> +
>> +void cpu_throttle_stop(void)
>> +{
>> +    if (cpu_throttle_active()) {
>> +        throttle_timer_stop = true;
>> +    }
>> +}
>> +
>> +bool cpu_throttle_active(void)
>> +{
>> +    return (throttle_timer != NULL);
>> +}
>> +
>> +int cpu_throttle_get_percentage(void)
>> +{
>> +    return throttle_percentage;
>> +}
>> +
>>  static void *qemu_kvm_cpu_thread_fn(void *arg)
>>  {
>>      CPUState *cpu = arg;
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index 39f0f19..56eb964 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -553,6 +553,44 @@ CPUState *qemu_get_cpu(int index);
>>   */
>>  bool cpu_exists(int64_t id);
>>  
>> +/**
>> + * cpu_throttle_set:
>> + * @new_throttle_pct: Percent of sleep time to running time.
>> + *                    Valid range is 1 to 99.
>> + *
>> + * Throttles all vcpus by forcing them to sleep for the given percentage of
>> + * time. A throttle_percentage of 50 corresponds to a 50% duty cycle roughly.
>> + * (example: 10ms sleep for every 10ms awake).
>> + *
>> + * cpu_throttle_set can be called as needed to adjust new_throttle_pct.
>> + * Once the throttling starts, it will remain in effect until cpu_throttle_stop
>> + * is called.
>> + */
>> +void cpu_throttle_set(int new_throttle_pct);
>> +
>> +/**
>> + * cpu_throttle_stop:
>> + *
>> + * Stops the vcpu throttling started by cpu_throttle_set.
>> + */
>> +void cpu_throttle_stop(void);
>> +
>> +/**
>> + * cpu_throttle_active:
>> + *
>> + * Returns %true if the vcpus are currently being throttled, %false otherwise.
>> + */
>> +bool cpu_throttle_active(void);
>> +
>> +/**
>> + * cpu_throttle_get_percentage:
>> + *
>> + * Returns the vcpu throttle percentage. See cpu_throttle_set for details.
>> + *
>> + * Returns The throttle percentage in range 1 to 99.
>> + */
>> +int cpu_throttle_get_percentage(void);
>> +
>>  #ifndef CONFIG_USER_ONLY
>>  
>>  typedef void (*CPUInterruptHandler)(CPUState *, int);
>> -- 
>> 1.9.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> 

Re: [Qemu-devel] [PATCH v3 1/5] cpu: Provide vcpu throttling interface

[Jason J. Herne]

On 07/01/2015 10:03 AM, Paolo Bonzini wrote:
>
>
> On 26/06/2015 20:07, Dr. David Alan Gilbert wrote:
>> * Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
>>> Provide a method to throttle guest cpu execution. CPUState is augmented with
>>> timeout controls and throttle start/stop functions. To throttle the guest cpu
>>> the caller simply has to call the throttle set function and provide a percentage
>>> of throttle time.
>>
>> I'm worried about atomicity and threads and all those fun things.
>>
>> I think all the starting/stopping/setting the throttling level is done in the
>> migration thread; I think the timers run in the main/io thread?
>> So you really need to be careful with at least:
>>      throttle_timer_stop - which may have a minor effect
>>      throttle_timer  - I worry about the way cpu_timer_active checks the pointer
>>                        yet it's freed when the timer goes off.   It's probably
>>                        not too bad because it never dereferences it.
>
> Agreed.  I think the only atomic should be throttle_percentage; if zero,
> throttling is inactive.
>
> In particular, throttle_ratio can be computed in cpu_throttle_thread.
>
> If you have exactly one variable that is shared between the threads,
> everything is much simpler.
>
> There is no need to allocate and free the timer; it's very cheap and in
> fact we probably should convert to statically allocated timers sooner or
> later.  So you can just create it once, for example in cpu_ticks_init.
>
> Paolo
>

Hi Paolo,
I like your suggestions. I'll switch to a static timer and create it in 
cpu_ticks_init.
So no need for a timer_free anywhere then?

-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)

Re: [Qemu-devel] [PATCH v3 1/5] cpu: Provide vcpu throttling interface

[Paolo Bonzini]

On 02/07/2015 16:25, Jason J. Herne wrote:
> Hi Paolo,
> I like your suggestions. I'll switch to a static timer and create it in
> cpu_ticks_init.
> So no need for a timer_free anywhere then?

Nope. :)

Paolo

Re: [Qemu-devel] [PATCH v3 1/5] cpu: Provide vcpu throttling interface

[Jason J. Herne]

On 07/01/2015 10:03 AM, Paolo Bonzini wrote:
>
>
> On 26/06/2015 20:07, Dr. David Alan Gilbert wrote:
>> * Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
>>> Provide a method to throttle guest cpu execution. CPUState is augmented with
>>> timeout controls and throttle start/stop functions. To throttle the guest cpu
>>> the caller simply has to call the throttle set function and provide a percentage
>>> of throttle time.
>>
>> I'm worried about atomicity and threads and all those fun things.
>>
>> I think all the starting/stopping/setting the throttling level is done in the
>> migration thread; I think the timers run in the main/io thread?
>> So you really need to be careful with at least:
>>      throttle_timer_stop - which may have a minor effect
>>      throttle_timer  - I worry about the way cpu_timer_active checks the pointer
>>                        yet it's freed when the timer goes off.   It's probably
>>                        not too bad because it never dereferences it.
>
> Agreed.  I think the only atomic should be throttle_percentage; if zero,
> throttling is inactive.
>
> In particular, throttle_ratio can be computed in cpu_throttle_thread.
>
> If you have exactly one variable that is shared between the threads,
> everything is much simpler.
>
> There is no need to allocate and free the timer; it's very cheap and in
> fact we probably should convert to statically allocated timers sooner or
> later.  So you can just create it once, for example in cpu_ticks_init.
>

I've made all of the changes you have suggested except adding atomics. 
I'm having
a bit of trouble figuring out what is needed here. Perhaps I should be using
atomic_read() to read throttle_percentage? If so, I don't undertand why. 
Rather
than trying to explain everything here I'm going to submit my V4 patches.
If any atomic operations are still necessary with V4 please let me know.

-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)

Re: [Qemu-devel] [PATCH v3 1/5] cpu: Provide vcpu throttling interface

[Paolo Bonzini]

On 02/07/2015 18:33, Jason J. Herne wrote:
> I've made all of the changes you have suggested except adding atomics.  I'm having
> a bit of trouble figuring out what is needed here. Perhaps I should be using
> atomic_read() to read throttle_percentage? If so, I don't undertand why. Rather
> than trying to explain everything here I'm going to submit my V4 patches.
> If any atomic operations are still necessary with V4 please let me know.

Fair enough!

Paolo