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

[Qemu-devel] [PATCH v4 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
-----------
v4
- Switch to a static timer
- Use QEMU_CLOCK_VIRTUAL_RT instead of QEMU_CLOCK_REALTIME
- Get rid of throttle_timer_stop, use throttle_percentage = 0 instead
- Calculate throttle_ratio directly in cpu_throttle_thread
- Consolidate timer mod operations to single function
- Remove some unneeded cpu_throttle_active() checks
- Check for throttle_percentage == 0 in cpu_throttle_thread
- Change throttle_percentage to unsigned int
- Renamed cpu_throttle_timer_pop to cpu_throttle_timer_tick
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           | 88 ++++++++++++++++++---------------------------------
 cpus.c                | 66 ++++++++++++++++++++++++++++++++++++++
 hmp.c                 | 21 ++++++++++++
 include/qom/cpu.h     | 38 ++++++++++++++++++++++
 migration/migration.c | 57 +++++++++++++++++++++++++++++++--
 qapi-schema.json      | 40 ++++++++++++++++++++---
 6 files changed, 246 insertions(+), 64 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v4 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            | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/qom/cpu.h | 38 ++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+)

diff --git a/cpus.c b/cpus.c
index de6469f..6f86da0 100644
--- a/cpus.c
+++ b/cpus.c
@@ -68,6 +68,14 @@ static CPUState *next_cpu;
 int64_t max_delay;
 int64_t max_advance;
 
+/* vcpu throttling controls */
+static QEMUTimer *throttle_timer;
+static unsigned int throttle_percentage;
+
+#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();
@@ -486,10 +494,68 @@ static const VMStateDescription vmstate_timers = {
     }
 };
 
+static void cpu_throttle_thread(void *opaque)
+{
+    double pct = (double)throttle_percentage/100;
+    double throttle_ratio = pct / (1 - pct);
+    long sleeptime_ms = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE);
+
+    if (!throttle_percentage) {
+        return;
+    }
+
+    qemu_mutex_unlock_iothread();
+    g_usleep(sleeptime_ms * 1000); /* Convert ms to us for usleep call */
+    qemu_mutex_lock_iothread();
+}
+
+static void cpu_throttle_timer_tick(void *opaque)
+{
+    CPUState *cpu;
+
+    /* Stop the timer if needed */
+    if (!throttle_percentage) {
+        return;
+    }
+    CPU_FOREACH(cpu) {
+        async_run_on_cpu(cpu, cpu_throttle_thread, NULL);
+    }
+
+    timer_mod(throttle_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL_RT) +
+                                   CPU_THROTTLE_TIMESLICE);
+}
+
+void cpu_throttle_set(int new_throttle_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);
+
+    timer_mod(throttle_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL_RT) +
+                                       CPU_THROTTLE_TIMESLICE);
+}
+
+void cpu_throttle_stop(void)
+{
+    throttle_percentage = 0;
+}
+
+bool cpu_throttle_active(void)
+{
+    return (throttle_percentage != 0);
+}
+
+int cpu_throttle_get_percentage(void)
+{
+    return throttle_percentage;
+}
+
 void cpu_ticks_init(void)
 {
     seqlock_init(&timers_state.vm_clock_seqlock, NULL);
     vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
+    throttle_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL_RT,
+                                           cpu_throttle_timer_tick, NULL);
 }
 
 void configure_icount(QemuOpts *opts, Error **errp)
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 v4 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>
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..52b7e07 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 percentage of time guest cpus are throttled
+#                          when migration auto-converge is activated.
+#                          (Since 2.4)
+#
+# @x-cpu-throttle-increment: throttle percentage 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 percentage of time guest cpus are throttled
+#                          when migration auto-converge is activated.
+#                          (Since 2.4)
+#
+# @x-cpu-throttle-increment: throttle percentage 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 percentage of time guest cpus are throttled
+#                          when migration auto-converge is activated.
+#                          (Since 2.4)
+#
+# @x-cpu-throttle-increment: throttle percentage 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 v4 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           | 88 ++++++++++++++++++---------------------------------
 migration/migration.c |  4 +++
 2 files changed, 34 insertions(+), 58 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 23d3feb..35f8eb0 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,7 +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
@@ -1913,51 +1933,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 v4 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>
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 52b7e07..c2bd8ce 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 v4 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>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.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 v4 1/5] cpu: Provide vcpu throttling interface

[Paolo Bonzini]

On 02/07/2015 18:36, Jason J. Herne wrote:
> +static void cpu_throttle_thread(void *opaque)
> +{
> +    double pct = (double)throttle_percentage/100;
> +    double throttle_ratio = pct / (1 - pct);
> +    long sleeptime_ms = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE);
> +
> +    if (!throttle_percentage) {
> +        return;
> +    }
> +
> +    qemu_mutex_unlock_iothread();
> +    g_usleep(sleeptime_ms * 1000); /* Convert ms to us for usleep call */
> +    qemu_mutex_lock_iothread();
> +}
> +
> +static void cpu_throttle_timer_tick(void *opaque)
> +{
> +    CPUState *cpu;
> +
> +    /* Stop the timer if needed */
> +    if (!throttle_percentage) {
> +        return;
> +    }
> +    CPU_FOREACH(cpu) {
> +        async_run_on_cpu(cpu, cpu_throttle_thread, NULL);
> +    }
> +
> +    timer_mod(throttle_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL_RT) +
> +                                   CPU_THROTTLE_TIMESLICE);
> +}

This could cause callbacks to pile up I think.  David, do you have any
idea how to fix it?

Paolo

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

[Andreas Färber]

Am 02.07.2015 um 18:36 schrieb 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            | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/qom/cpu.h | 38 ++++++++++++++++++++++++++++++++
>  2 files changed, 104 insertions(+)

No objections from my side, but the interesting code is outside my area.

I feel we (including myself) are abusing include/qom/cpu.h (here there's
not even a single CPUState argument) but I don't have a better
suggestion. At some point we'll need to revisit the cpu.h vs. cpu-all.h
etc. split or even introduce something new.

I'm preparing a qom-cpu pull and assume this will go through the
migration tree when finalized.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

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

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 02/07/2015 18:36, Jason J. Herne wrote:
> > +static void cpu_throttle_thread(void *opaque)
> > +{
> > +    double pct = (double)throttle_percentage/100;
> > +    double throttle_ratio = pct / (1 - pct);
> > +    long sleeptime_ms = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE);
> > +
> > +    if (!throttle_percentage) {
> > +        return;
> > +    }
> > +
> > +    qemu_mutex_unlock_iothread();
> > +    g_usleep(sleeptime_ms * 1000); /* Convert ms to us for usleep call */
> > +    qemu_mutex_lock_iothread();
> > +}
> > +
> > +static void cpu_throttle_timer_tick(void *opaque)
> > +{
> > +    CPUState *cpu;
> > +
> > +    /* Stop the timer if needed */
> > +    if (!throttle_percentage) {
> > +        return;
> > +    }
> > +    CPU_FOREACH(cpu) {
> > +        async_run_on_cpu(cpu, cpu_throttle_thread, NULL);
> > +    }
> > +
> > +    timer_mod(throttle_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL_RT) +
> > +                                   CPU_THROTTLE_TIMESLICE);
> > +}
> 
> This could cause callbacks to pile up I think.  David, do you have any
> idea how to fix it?

I don't know the timer code well enough.

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

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

[Jason J. Herne]

On 07/02/2015 12:43 PM, Paolo Bonzini wrote:
>
>
> On 02/07/2015 18:36, Jason J. Herne wrote:
>> +static void cpu_throttle_thread(void *opaque)
>> +{
>> +    double pct = (double)throttle_percentage/100;
>> +    double throttle_ratio = pct / (1 - pct);
>> +    long sleeptime_ms = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE);
>> +
>> +    if (!throttle_percentage) {
>> +        return;
>> +    }
>> +
>> +    qemu_mutex_unlock_iothread();
>> +    g_usleep(sleeptime_ms * 1000); /* Convert ms to us for usleep call */
>> +    qemu_mutex_lock_iothread();
>> +}
>> +
>> +static void cpu_throttle_timer_tick(void *opaque)
>> +{
>> +    CPUState *cpu;
>> +
>> +    /* Stop the timer if needed */
>> +    if (!throttle_percentage) {
>> +        return;
>> +    }
>> +    CPU_FOREACH(cpu) {
>> +        async_run_on_cpu(cpu, cpu_throttle_thread, NULL);
>> +    }
>> +
>> +    timer_mod(throttle_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL_RT) +
>> +                                   CPU_THROTTLE_TIMESLICE);
>> +}
>
> This could cause callbacks to pile up I think.  David, do you have any
> idea how to fix it?
>
> Paolo
>
>
>

I'm not sure how callbacks can pile up here. If the vcpus are running 
then their thread's will execute the callbacks. If they are not running 
then the use of QEMU_CLOCK_VIRTUAL_RT will prevent the callbacks from 
stacking because the timer is not running, right?


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

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

[Paolo Bonzini]

On 13/07/2015 16:43, Jason J. Herne wrote:
>>>
>>> +    CPU_FOREACH(cpu) {
>>> +        async_run_on_cpu(cpu, cpu_throttle_thread, NULL);
>>> +    }
>>> +
>>> +    timer_mod(throttle_timer,
>>> qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL_RT) +
>>> +                                   CPU_THROTTLE_TIMESLICE);
>>> +}
>>
>> This could cause callbacks to pile up I think.  David, do you have any
>> idea how to fix it?
> 
> I'm not sure how callbacks can pile up here. If the vcpus are running
> then their thread's will execute the callbacks. If they are not running
> then the use of QEMU_CLOCK_VIRTUAL_RT will prevent the callbacks from
> stacking because the timer is not running, right?

Couldn't the iothread starve the VCPUs?  They need to take the iothread
lock in order to process the callbacks.

Paolo

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

[Jason J. Herne]

On 07/13/2015 11:14 AM, Paolo Bonzini wrote:
>
>
> On 13/07/2015 16:43, Jason J. Herne wrote:
>>>>
>>>> +    CPU_FOREACH(cpu) {
>>>> +        async_run_on_cpu(cpu, cpu_throttle_thread, NULL);
>>>> +    }
>>>> +
>>>> +    timer_mod(throttle_timer,
>>>> qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL_RT) +
>>>> +                                   CPU_THROTTLE_TIMESLICE);
>>>> +}
>>>
>>> This could cause callbacks to pile up I think.  David, do you have any
>>> idea how to fix it?
>>
>> I'm not sure how callbacks can pile up here. If the vcpus are running
>> then their thread's will execute the callbacks. If they are not running
>> then the use of QEMU_CLOCK_VIRTUAL_RT will prevent the callbacks from
>> stacking because the timer is not running, right?
>
> Couldn't the iothread starve the VCPUs?  They need to take the iothread
> lock in order to process the callbacks.
>

Yes, I can see the possibility here. I'm not sure what to do about it 
though.

Maybe this is wishful thinking :) But if the iothread lock cannot be 
acquired then
the cpu cannot run thereby preventing the guest from changing a ton of 
pages.
This will have the effect of indirectly throttling the guest which will 
allow
us to advance to the non-live phase of migration rather quickly. And again,
if we are starving on the iothread lock then the guest vcpus are not 
executing and
QEMU_CLOCK_VIRTUAL_RT is not ticking, right? This will also limit the 
number of
stacked callbacks to a very low number. Unless I've missing something?

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

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

[Paolo Bonzini]

On 15/07/2015 14:40, Jason J. Herne wrote:
>>> I'm not sure how callbacks can pile up here. If the vcpus are
>>> running then their thread's will execute the callbacks. If they
>>> are not running then the use of QEMU_CLOCK_VIRTUAL_RT will
>>> prevent the callbacks from stacking because the timer is not
>>> running, right?
>> 
>> Couldn't the iothread starve the VCPUs?  They need to take the
>> iothread lock in order to process the callbacks.
> 
> Yes, I can see the possibility here. I'm not sure what to do about
> it though.
> 
> Maybe this is wishful thinking :) But if the iothread lock cannot be 
> acquired then the cpu cannot run thereby preventing the guest from
> changing a ton of pages. This will have the effect of indirectly
> throttling the guest which will allow us to advance to the non-live
> phase of migration rather quickly.

Makes sense.  On the other hand this wouldn't prevent callbacks from
piling up for a short time because...

> And again, if we are starving on
> the iothread lock then the guest vcpus are not executing and 
> QEMU_CLOCK_VIRTUAL_RT is not ticking, right?

... you are talking about stolen time, and QEMU_CLOCK_VIRTUAL_RT does
count stolen time (stolen time is different for each VCPU, so you would
have a different clock for each VCPU).

QEMU_CLOCK_VIRTUAL and QEMU_CLOCK_VIRTUAL_RT(*) only pause across
stop/cont.  (By the way, the two are the same with KVM).

However, something like

	if (!atomic_xchg(&cpu->throttle_thread_scheduled, 1)) {
	    async_run_on_cpu(cpu, cpu_throttle_thread, NULL);
	}

...
	atomic_set(&cpu->throttle_thread_scheduled, 0);
	g_usleep(...);

should be enough.  You'd still have many timers that could starve the
VCPUs but, as you pointed out, in that case migration would hopefully
finish pretty fast.

Paolo

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

[Jason J. Herne]

On 07/15/2015 08:54 AM, Paolo Bonzini wrote:
>
>
> On 15/07/2015 14:40, Jason J. Herne wrote:
>>>> I'm not sure how callbacks can pile up here. If the vcpus are
>>>> running then their thread's will execute the callbacks. If they
>>>> are not running then the use of QEMU_CLOCK_VIRTUAL_RT will
>>>> prevent the callbacks from stacking because the timer is not
>>>> running, right?
>>>
>>> Couldn't the iothread starve the VCPUs?  They need to take the
>>> iothread lock in order to process the callbacks.
>>
>> Yes, I can see the possibility here. I'm not sure what to do about
>> it though.
>>
>> Maybe this is wishful thinking :) But if the iothread lock cannot be
>> acquired then the cpu cannot run thereby preventing the guest from
>> changing a ton of pages. This will have the effect of indirectly
>> throttling the guest which will allow us to advance to the non-live
>> phase of migration rather quickly.
>
> Makes sense.  On the other hand this wouldn't prevent callbacks from
> piling up for a short time because...
>
>> And again, if we are starving on
>> the iothread lock then the guest vcpus are not executing and
>> QEMU_CLOCK_VIRTUAL_RT is not ticking, right?
>
> ... you are talking about stolen time, and QEMU_CLOCK_VIRTUAL_RT does
> count stolen time (stolen time is different for each VCPU, so you would
> have a different clock for each VCPU).
>
> QEMU_CLOCK_VIRTUAL and QEMU_CLOCK_VIRTUAL_RT(*) only pause across
> stop/cont.  (By the way, the two are the same with KVM).
>
> However, something like
>
> 	if (!atomic_xchg(&cpu->throttle_thread_scheduled, 1)) {
> 	    async_run_on_cpu(cpu, cpu_throttle_thread, NULL);
> 	}
>
> ...
> 	atomic_set(&cpu->throttle_thread_scheduled, 0);
> 	g_usleep(...);
>
> should be enough.  You'd still have many timers that could starve the
> VCPUs but, as you pointed out, in that case migration would hopefully
> finish pretty fast.
>
> Paolo
>
>

Paolo, Andreas & David, thanks for the review comments.

Has this advanced enough for a reviewed-by? The only remaining 
objections I can find are:

1. Using atomic operations to manage throttle_percentage. I'm not sure 
where atomics are applicable here. If this is still a concern hopefully 
someone can explain.

2. Callback stacking. And it seems like we are convinced that it is not 
a big issue. Anyone disagree?

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

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

[Paolo Bonzini]

On 16/07/2015 16:21, Jason J. Herne wrote:
> 1. Using atomic operations to manage throttle_percentage. I'm not sure
> where atomics are applicable here. If this is still a concern hopefully
> someone can explain.

I would use atomic_read/atomic_set in cpu_throttle_set, 
cpu_throttle_stop, cpu_throttle_active, cpu_throttle_get_percentage.  
In addition, the function naming seems to be a bit inconsistent: please 
rename cpu_throttle_set to cpu_throttle_set_percentage.

Second, here:

>> +static void cpu_throttle_thread(void *opaque)
>> +{
>> + double pct = (double)throttle_percentage/100;

Please use cpu_throttle_get_percentage(), and

>> + double throttle_ratio = pct / (1 - pct);
>> + long sleeptime_ms = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE);

... move these computations below the if.

I'm also not sure about throttle_ratio, why is it needed?  If pct >= 0.5 you
end up with throttle_ratio >= 1, i.e. no way for the CPU to do any work.  This
would definitely cause a problem with callbacks piling up.

>> + if (!throttle_percentage) {
>> + return;
>> + }
>> +
>> + qemu_mutex_unlock_iothread();
>> + g_usleep(sleeptime_ms * 1000); /* Convert ms to us for usleep call */
>> + qemu_mutex_lock_iothread();
>> +}
>> +

> 2. Callback stacking. And it seems like we are convinced that it is not
> a big issue. Anyone disagree?

I think it's not a big issue to have many timers, but it is a big issue to have many callbacks.  What I suggested is this:

    if (!atomic_xchg(&cpu->throttle_thread_scheduled, 1)) {
        async_run_on_cpu(cpu, cpu_throttle_thread, NULL);
    }

and in the callback:

    atomic_set(&cpu->throttle_thread_scheduled, 0);
    g_usleep(...); 

Paolo

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

[Jason J. Herne]

On 07/23/2015 05:59 AM, Paolo Bonzini wrote:
>
>
> On 16/07/2015 16:21, Jason J. Herne wrote:
>> 1. Using atomic operations to manage throttle_percentage. I'm not sure
>> where atomics are applicable here. If this is still a concern hopefully
>> someone can explain.
>
> I would use atomic_read/atomic_set in cpu_throttle_set,
> cpu_throttle_stop, cpu_throttle_active, cpu_throttle_get_percentage.
> In addition, the function naming seems to be a bit inconsistent: please
> rename cpu_throttle_set to cpu_throttle_set_percentage.
>
> Second, here:
>
>>> +static void cpu_throttle_thread(void *opaque)
>>> +{
>>> + double pct = (double)throttle_percentage/100;
>
> Please use cpu_throttle_get_percentage(), and
>
>>> + double throttle_ratio = pct / (1 - pct);
>>> + long sleeptime_ms = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE);
>
> ... move these computations below the if.
>
> I'm also not sure about throttle_ratio, why is it needed?  If pct >= 0.5 you
> end up with throttle_ratio >= 1, i.e. no way for the CPU to do any work.  This
> would definitely cause a problem with callbacks piling up.
>

Throttle ratio is relative to CPU_THROTTLE_TIMESLICE. Take a look at how
throttle_ratio is used in the calculation:

long sleeptime_ms = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE);

A value of 1 means we sleep the same amount of time that we execute.

>>> + if (!throttle_percentage) {
>>> + return;
>>> + }
>>> +
>>> + qemu_mutex_unlock_iothread();
>>> + g_usleep(sleeptime_ms * 1000); /* Convert ms to us for usleep call */
>>> + qemu_mutex_lock_iothread();
>>> +}
>>> +
>
>> 2. Callback stacking. And it seems like we are convinced that it is not
>> a big issue. Anyone disagree?
>
> I think it's not a big issue to have many timers, but it is a big issue to have many callbacks.  What I suggested is this:
>
>      if (!atomic_xchg(&cpu->throttle_thread_scheduled, 1)) {
>          async_run_on_cpu(cpu, cpu_throttle_thread, NULL);
>      }
>
> and in the callback:
>
>      atomic_set(&cpu->throttle_thread_scheduled, 0);
>      g_usleep(...);
>
> Paolo
>
>

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

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

[Paolo Bonzini]

On 31/07/2015 19:12, Jason J. Herne wrote:
>>
>>
> 
> Throttle ratio is relative to CPU_THROTTLE_TIMESLICE. Take a look at how
> throttle_ratio is used in the calculation:
> 
> long sleeptime_ms = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE);
> 
> A value of 1 means we sleep the same amount of time that we execute.

But that doesn't work if your timer runs every CPU_THROTTLE_TIMESLICE
milliseconds, and thus schedules async work every CPU_THROTTLE_TIMESLICE
milliseconds.

The timer would have to be scheduled every (throttle_ratio + 1) *
CPU_THROTTLE_TIMESLICE milliseconds, i.e. CPU_THROTTLE_TIMESLICE /
(1-pct) milliseconds.

Paolo

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

[Jason J. Herne]

On 07/31/2015 01:16 PM, Paolo Bonzini wrote:
>
>
> On 31/07/2015 19:12, Jason J. Herne wrote:
>>>
>>>
>>
>> Throttle ratio is relative to CPU_THROTTLE_TIMESLICE. Take a look at how
>> throttle_ratio is used in the calculation:
>>
>> long sleeptime_ms = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE);
>>
>> A value of 1 means we sleep the same amount of time that we execute.
>
> But that doesn't work if your timer runs every CPU_THROTTLE_TIMESLICE
> milliseconds, and thus schedules async work every CPU_THROTTLE_TIMESLICE
> milliseconds.
>
> The timer would have to be scheduled every (throttle_ratio + 1) *
> CPU_THROTTLE_TIMESLICE milliseconds, i.e. CPU_THROTTLE_TIMESLICE /
> (1-pct) milliseconds.
>
> Paolo

Doh! Yep :). This problem is an artifact of moving the timer_mod from
cpu_throttle_thread into cpu_throttle_timer_tick. I'll have to go back
to the review comments and look at why that was done.

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

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

[Jason J. Herne]

On 07/31/2015 01:42 PM, Jason J. Herne wrote:
> On 07/31/2015 01:16 PM, Paolo Bonzini wrote:
>>
>>
>> On 31/07/2015 19:12, Jason J. Herne wrote:
>>>>
>>>>
>>>
>>> Throttle ratio is relative to CPU_THROTTLE_TIMESLICE. Take a look at how
>>> throttle_ratio is used in the calculation:
>>>
>>> long sleeptime_ms = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE);
>>>
>>> A value of 1 means we sleep the same amount of time that we execute.
>>
>> But that doesn't work if your timer runs every CPU_THROTTLE_TIMESLICE
>> milliseconds, and thus schedules async work every CPU_THROTTLE_TIMESLICE
>> milliseconds.
>>
>> The timer would have to be scheduled every (throttle_ratio + 1) *
>> CPU_THROTTLE_TIMESLICE milliseconds, i.e. CPU_THROTTLE_TIMESLICE /
>> (1-pct) milliseconds.
>>
>> Paolo
>
> Doh! Yep :). This problem is an artifact of moving the timer_mod from
> cpu_throttle_thread into cpu_throttle_timer_tick. I'll have to go back
> to the review comments and look at why that was done.

So, we made that change in v3 to eliminate the per cpu timer. With a per
cpu timer we avoid this problem and we no longer need to worry about
a throttle_thread_scheduled, and timers stacking. Paolo, you had originally
argued in favor of this change. With what we know now, do you still think
having only a single timer is best? Or should I switch back to a timer per
cpu? With a timer per cpu we can simply reset the timer immediately after
the sleep.

I guess an alternative would be for the last cpu to complete its sleep to
reset the timer in cpu_throttle_thread. We would need an atomic flag in
CPUState and a loop to run it and bail out if any cpu has the flag set.

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

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

[Paolo Bonzini]

On 31/07/2015 20:11, Jason J. Herne wrote:
>>>
>>
>> Doh! Yep :). This problem is an artifact of moving the timer_mod from
>> cpu_throttle_thread into cpu_throttle_timer_tick. I'll have to go back
>> to the review comments and look at why that was done.
> 
> So, we made that change in v3 to eliminate the per cpu timer. With a per
> cpu timer we avoid this problem and we no longer need to worry about
> a throttle_thread_scheduled, and timers stacking. Paolo, you had originally
> argued in favor of this change. With what we know now, do you still think
> having only a single timer is best? Or should I switch back to a timer per
> cpu? With a timer per cpu we can simply reset the timer immediately after
> the sleep.

It's okay to have a single timer, only the formulas have to be
corrected: either you remove the pct/(1-pct) from the callback or you
add a /(1-pct) to the timer_mod.

Paolo

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

[Jason J. Herne]

On 08/01/2015 05:40 AM, Paolo Bonzini wrote:
>
>
> On 31/07/2015 20:11, Jason J. Herne wrote:
>>>>
>>>
>>> Doh! Yep :). This problem is an artifact of moving the timer_mod from
>>> cpu_throttle_thread into cpu_throttle_timer_tick. I'll have to go back
>>> to the review comments and look at why that was done.
>>
>> So, we made that change in v3 to eliminate the per cpu timer. With a per
>> cpu timer we avoid this problem and we no longer need to worry about
>> a throttle_thread_scheduled, and timers stacking. Paolo, you had originally
>> argued in favor of this change. With what we know now, do you still think
>> having only a single timer is best? Or should I switch back to a timer per
>> cpu? With a timer per cpu we can simply reset the timer immediately after
>> the sleep.
>
> It's okay to have a single timer, only the formulas have to be
> corrected: either you remove the pct/(1-pct) from the callback or you
> add a /(1-pct) to the timer_mod.
>
> Paolo

Paolo,

You are correct here. I've adjusted the timer formula and tested. 
Everything seems to be playing nicely now. Sorry it took me a month to 
get to this. I got pulled into some critical work and improved 
auto-converge took a back seat. I know it is a pain to go back to 
something you have not seen in a month so I appreciate any attention 
this gets :). A new patch set will be inbound shortly...

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