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

[Qemu-devel] [PATCH v2 0/3] 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. This method ensures that all migrations
will eventually converge.

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
-----------
v2
- Add cpu throttle ratio output to hmp/qmp info/query migrate commands
v1
- Initial

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

 arch_init.c           | 95 +++++++++++++++++----------------------------------
 cpus.c                | 62 +++++++++++++++++++++++++++++++++
 hmp.c                 |  5 +++
 include/qom/cpu.h     | 46 +++++++++++++++++++++++++
 migration/migration.c | 14 ++++++++
 qapi-schema.json      |  3 +-
 6 files changed, 161 insertions(+), 64 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 1/3] 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 start function and provide a ratio of
sleep time to normal execution time.

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

diff --git a/cpus.c b/cpus.c
index de6469f..7568357 100644
--- a/cpus.c
+++ b/cpus.c
@@ -64,6 +64,9 @@
 
 #endif /* CONFIG_LINUX */
 
+/* Number of ms between cpu throttle operations */
+#define CPU_THROTTLE_TIMESLICE 10
+
 static CPUState *next_cpu;
 int64_t max_delay;
 int64_t max_advance;
@@ -919,6 +922,65 @@ static void qemu_kvm_wait_io_event(CPUState *cpu)
     qemu_wait_io_event_common(cpu);
 }
 
+static void cpu_throttle_thread(void *opq)
+{
+    CPUState *cpu = (CPUState *)opq;
+    long sleeptime_ms = (long)(cpu->throttle_ratio * CPU_THROTTLE_TIMESLICE);
+
+    /* Stop the timer if needed */
+    if (cpu->throttle_timer_stop) {
+        timer_del(cpu->throttle_timer);
+        timer_free(cpu->throttle_timer);
+        cpu->throttle_timer = NULL;
+        return;
+    }
+
+    qemu_mutex_unlock_iothread();
+    g_usleep(sleeptime_ms * 1000); /* Convert ms to us for usleep call */
+    qemu_mutex_lock_iothread();
+
+    timer_mod(cpu->throttle_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
+                                   CPU_THROTTLE_TIMESLICE);
+}
+
+static void cpu_throttle_timer_pop(void *opq)
+{
+    CPUState *cpu = (CPUState *)opq;
+
+    async_run_on_cpu(cpu, cpu_throttle_thread, cpu);
+}
+
+void cpu_throttle_start(CPUState *cpu, float throttle_ratio)
+{
+    assert(throttle_ratio > 0);
+    cpu->throttle_ratio = throttle_ratio;
+
+    if (!cpu_throttle_active(cpu)) {
+        cpu->throttle_timer = timer_new_ms(QEMU_CLOCK_REALTIME,
+                                           cpu_throttle_timer_pop, cpu);
+        timer_mod(cpu->throttle_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
+                                       CPU_THROTTLE_TIMESLICE);
+        cpu->throttle_timer_stop = false;
+    }
+}
+
+void cpu_throttle_stop(CPUState *cpu)
+{
+    assert(cpu_throttle_active(cpu));
+    cpu->throttle_timer_stop = true;
+}
+
+bool cpu_throttle_active(CPUState *cpu)
+{
+    return (cpu->throttle_timer != NULL);
+}
+
+float cpu_throttle_get_ratio(CPUState *cpu)
+{
+    assert(cpu_throttle_active(cpu));
+    return cpu->throttle_ratio;
+}
+
 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..9d16e6a 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -310,6 +310,11 @@ struct CPUState {
     uint32_t can_do_io;
     int32_t exception_index; /* used by m68k TCG */
 
+    /* vcpu throttling controls */
+    QEMUTimer *throttle_timer;
+    bool throttle_timer_stop;
+    float throttle_ratio;
+
     /* Note that this is accessed at the start of every TB via a negative
        offset from AREG0.  Leave this field at the end so as to make the
        (absolute value) offset as small as possible.  This reduces code
@@ -553,6 +558,47 @@ CPUState *qemu_get_cpu(int index);
  */
 bool cpu_exists(int64_t id);
 
+/**
+ * cpu_throttle_start:
+ * @cpu: The vcpu to throttle
+ *
+ * Throttles a vcpu by forcing it to sleep. The duration of the sleep is a
+ * ratio of sleep time to running time. A ratio of 1.0 corresponds to a 50%
+ * duty cycle (example: 10ms sleep for every 10ms awake).
+ *
+ * cpu_throttle_start can be called as needed to adjust the throttle ratio.
+ * Once the throttling starts, it will remain in effect until cpu_throttle_stop
+ * is called.
+ */
+void cpu_throttle_start(CPUState *cpu, float throttle_ratio);
+
+/**
+ * cpu_throttle_stop:
+ * @cpu: The vcpu to stop throttling
+ *
+ * Stops the vcpu throttling started by cpu_throttle_start.
+ */
+void cpu_throttle_stop(CPUState *cpu);
+
+/**
+ * cpu_throttle_active:
+ * @cpu: The vcpu to check
+ *
+ * Returns %true if this vcpu is currently being throttled, %false otherwise.
+ */
+bool cpu_throttle_active(CPUState *cpu);
+
+/**
+ * cpu_throttle_get_ratio:
+ * @cpu: The vcpu whose throttle ratio to return.
+ *
+ * Returns the ratio being used to throttle this vcpu. See cpu_throttle_start
+ * for details.
+ *
+ * Returns The ratio being used to throttle this vcpu.
+ */
+float cpu_throttle_get_ratio(CPUState *cpu);
+
 #ifndef CONFIG_USER_ONLY
 
 typedef void (*CPUInterruptHandler)(CPUState *, int);
-- 
1.9.1

[Qemu-devel] [PATCH v2 2/3] 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           | 95 +++++++++++++++++----------------------------------
 migration/migration.c |  9 +++++
 2 files changed, 41 insertions(+), 63 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 23d3feb..73ae494 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,31 @@ 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. For these workloads we will continue to
+ * increase throttling until the guest is paused long enough to complete the
+ * migration. This essentially becomes a non-live migration.
+ */
+static void mig_throttle_guest_down(void)
+{
+    CPUState *cpu;
+
+    CPU_FOREACH(cpu) {
+        /* We have not started throttling yet. Lets start it.*/
+        if (!cpu_throttle_active(cpu)) {
+            cpu_throttle_start(cpu, 0.2);
+        }
+
+        /* Throttling is already in place. Just increase the throttling rate */
+        else {
+            cpu_throttle_start(cpu, cpu_throttle_get_ratio(cpu) * 2);
+        }
+    }
+}
+
 /* 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 +737,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 +1220,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 +1323,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 +1930,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 732d229..c9545df 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 */
 
@@ -731,6 +732,7 @@ int64_t migrate_xbzrle_cache_size(void)
 static void *migration_thread(void *opaque)
 {
     MigrationState *s = opaque;
+    CPUState *cpu;
     int64_t initial_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
     int64_t setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
     int64_t initial_bytes = 0;
@@ -814,6 +816,13 @@ static void *migration_thread(void *opaque)
         }
     }
 
+    /* If we enabled cpu throttling for auto-converge, turn it off. */
+    CPU_FOREACH(cpu) {
+        if (cpu_throttle_active(cpu)) {
+            cpu_throttle_stop(cpu);
+        }
+    }
+
     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 v2 3/3] qmp/hmp: Add throttle ratio to query-migrate and info migrate

[Jason J. Herne]

Report throttle ratio 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      | 3 ++-
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/hmp.c b/hmp.c
index e17852d..cb3c137 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_ratio) {
+        monitor_printf(mon, "cpu throttle ratio : %0.2f\n",
+                       info->x_cpu_throttle_ratio);
+    }
+
     qapi_free_MigrationInfo(info);
     qapi_free_MigrationCapabilityStatusList(caps);
 }
diff --git a/migration/migration.c b/migration/migration.c
index c9545df..98cc03a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -263,6 +263,11 @@ MigrationInfo *qmp_query_migrate(Error **errp)
             info->disk->total = blk_mig_bytes_total();
         }
 
+        if (cpu_throttle_active(first_cpu)) {
+            info->has_x_cpu_throttle_ratio = true;
+            info->x_cpu_throttle_ratio = cpu_throttle_get_ratio(first_cpu);
+        }
+
         get_xbzrle_cache_stats(info);
         break;
     case MIGRATION_STATUS_COMPLETED:
diff --git a/qapi-schema.json b/qapi-schema.json
index f97ffa1..5e732e0 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -483,7 +483,8 @@
            '*total-time': 'int',
            '*expected-downtime': 'int',
            '*downtime': 'int',
-           '*setup-time': 'int'} }
+           '*setup-time': 'int',
+           '*x-cpu-throttle-ratio': 'number'} }
 
 ##
 # @query-migrate
-- 
1.9.1

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

[Eric Blake]

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

On 06/02/2015 11:46 AM, Jason J. Herne 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           | 95 +++++++++++++++++----------------------------------
>  migration/migration.c |  9 +++++
>  2 files changed, 41 insertions(+), 63 deletions(-)
> 

> +static void mig_throttle_guest_down(void)
> +{
> +    CPUState *cpu;
> +
> +    CPU_FOREACH(cpu) {
> +        /* We have not started throttling yet. Lets start it.*/

s/Lets/Let's/
s|it.*/|it. */|

> +        if (!cpu_throttle_active(cpu)) {
> +            cpu_throttle_start(cpu, 0.2);
> +        }
> +
> +        /* Throttling is already in place. Just increase the throttling rate */
> +        else {

Unusual layout.  More typical would be:

if (...) {
   ...
} else {
   /* comment */
   ...
}

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


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

Re: [Qemu-devel] [PATCH v2 3/3] qmp/hmp: Add throttle ratio to query-migrate and info migrate

[Eric Blake]

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

On 06/02/2015 11:46 AM, Jason J. Herne wrote:
> Report throttle ratio 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      | 3 ++-
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hmp.c b/hmp.c
> index e17852d..cb3c137 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_ratio) {
> +        monitor_printf(mon, "cpu throttle ratio : %0.2f\n",

s/ :/:/

How big or small can the ratio get? Is %g going to be nicer than %f if
the ratio goes through a large range of possibilities?

> +++ b/qapi-schema.json
> @@ -483,7 +483,8 @@
>             '*total-time': 'int',
>             '*expected-downtime': 'int',
>             '*downtime': 'int',
> -           '*setup-time': 'int'} }
> +           '*setup-time': 'int',
> +           '*x-cpu-throttle-ratio': 'number'} }

Even though it is marked experimental, it is still worth documenting
this parameter, and include mention of how to interpret it (0.0 means no
throttling, 1.0 means 50% duty cycle, 2.0 means 33% duty cycle, right?).
Documentation should mention '(since 2.4)'

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


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

Re: [Qemu-devel] [PATCH v2 1/3] cpu: Provide vcpu throttling interface

[Juan Quintela]

"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 start function and provide a ratio of
> sleep time to normal execution time.
>
> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
> Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>



Hi

sorry that I replied to your previous submission, I had "half done" the
mail from yesterday, I still think that all applies.


> ---
>  cpus.c            | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/qom/cpu.h | 46 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 108 insertions(+)
>
> diff --git a/cpus.c b/cpus.c
> index de6469f..7568357 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -64,6 +64,9 @@
>  
>  #endif /* CONFIG_LINUX */
>  
> +/* Number of ms between cpu throttle operations */
> +#define CPU_THROTTLE_TIMESLICE 10


We are checking for throotling on each cpu each 10ms.
But on patch 2 we can see that we only change the throotling each
time that we call migration_bitmap_sync(), that only happens each round
through all the pages. Normally auto-converge only matters for machines
with lots of memory, so this is going to happen each more than 10ms (we
change it each 4 passes).  You changed it to each 2 passes, and you add
it a 0.2.  I think  that I would preffer to just have it each single
pass, but add a 0.1 each pass?  simpler and end result would be the same?


This is what the old code did (new code do exactly the same, just in the
previous patch)

-static void mig_sleep_cpu(void *opq)
-{
-    qemu_mutex_unlock_iothread();
-    g_usleep(30*1000);
-    qemu_mutex_lock_iothread();
-}

So, what we are doing, calling async_run_on_cpu() with this function.

To make things short, we end here:

static void flush_queued_work(CPUState *cpu)
{
    struct qemu_work_item *wi;

    if (cpu->queued_work_first == NULL) {
        return;
    }

    while ((wi = cpu->queued_work_first)) {
        cpu->queued_work_first = wi->next;
        wi->func(wi->data);
        wi->done = true;
        if (wi->free) {
            g_free(wi);
        }
    }
    cpu->queued_work_last = NULL;
    qemu_cond_broadcast(&qemu_work_cond);
}

So, we are walking a list that is protected with the iothread_lock, and
we are dropping the lock in the middle of the walk .....  Just hoping
that nothing else is calling run_async_on_cpu() from a different place
....


Could we change this to something like:

diff --git a/kvm-all.c b/kvm-all.c
index 17a3771..ae9635f 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1787,6 +1787,7 @@ int kvm_cpu_exec(CPUState *cpu)
 {
     struct kvm_run *run = cpu->kvm_run;
     int ret, run_ret;
+    long throotling_time;

     DPRINTF("kvm_cpu_exec()\n");

@@ -1813,8 +1814,15 @@ int kvm_cpu_exec(CPUState *cpu)
              */
             qemu_cpu_kick_self();
         }
+        throotling_time = cpu->throotling_time;
+        cpu->throotling_time = 0;
+        cpu->sleeping_time += throotling_time;
         qemu_mutex_unlock_iothread();

+
+        if (throotling_time) {
+            g_usleep(throttling_time);
+        }
         run_ret = kvm_vcpu_ioctl(cpu, KVM_RUN, 0);

         qemu_mutex_lock_iothread();



And we handle where we are doing now what throotling_time and
sleeping_time should be?  No need to drop throotling_time/lock/whatever.

It adds an if on the fast path, but we have a call to the hypervisor
each time, so it shouldn't be so bad, no?

For tcp/xen we should seach for a different place to put this code, but
you get the idea.  Reason for putting it here is because this is where
the iothread is dropped, so we don't lost any locking, any other place
that we put it needs review with respect to this.


Jason, I am really, really sorry to have open this big can of worms on
your patch.  Now you know why I was telling that "improving"
autoconverge was not as easy as it looked.

With respect ot the last comment, I also think that we can include the
Jason patches.  They are one imprevement over what we have now.  It is
just that it needs more improvements.

Later, Juan.

Re: [Qemu-devel] [PATCH v2 3/3] qmp/hmp: Add throttle ratio to query-migrate and info migrate

[Jason J. Herne]

On 06/02/2015 04:11 PM, Eric Blake wrote:
> On 06/02/2015 11:46 AM, Jason J. Herne wrote:
>> Report throttle ratio 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      | 3 ++-
>>   3 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/hmp.c b/hmp.c
>> index e17852d..cb3c137 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_ratio) {
>> +        monitor_printf(mon, "cpu throttle ratio : %0.2f\n",
>
> s/ :/:/
>

Will fix, Thanks.

> How big or small can the ratio get? Is %g going to be nicer than %f if
> the ratio goes through a large range of possibilities?
>


You are correct in your interpretation below. So a ratio of 10.0 would be
90.9% throttled. A ratio of 100 would be 99% throttled. Given that, I think
we're ok with %f.

>> +++ b/qapi-schema.json
>> @@ -483,7 +483,8 @@
>>              '*total-time': 'int',
>>              '*expected-downtime': 'int',
>>              '*downtime': 'int',
>> -           '*setup-time': 'int'} }
>> +           '*setup-time': 'int',
>> +           '*x-cpu-throttle-ratio': 'number'} }
>
> Even though it is marked experimental, it is still worth documenting
> this parameter, and include mention of how to interpret it (0.0 means no
> throttling, 1.0 means 50% duty cycle, 2.0 means 33% duty cycle, right?).
> Documentation should mention '(since 2.4)'
>

Will fix.

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

Re: [Qemu-devel] [PATCH v2 1/3] cpu: Provide vcpu throttling interface

[Jason J. Herne]

On 06/03/2015 03:56 AM, Juan Quintela 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 start function and provide a ratio of
>> sleep time to normal execution time.
>>
>> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
>> Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>
>
>
> Hi
>
> sorry that I replied to your previous submission, I had "half done" the
> mail from yesterday, I still think that all applies.
>
>
>> ---
>>   cpus.c            | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/qom/cpu.h | 46 +++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 108 insertions(+)
>>
>> diff --git a/cpus.c b/cpus.c
>> index de6469f..7568357 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -64,6 +64,9 @@
>>
>>   #endif /* CONFIG_LINUX */
>>
>> +/* Number of ms between cpu throttle operations */
>> +#define CPU_THROTTLE_TIMESLICE 10
>
>
> We are checking for throotling on each cpu each 10ms.
> But on patch 2 we can see that we only change the throotling each
> time that we call migration_bitmap_sync(), that only happens each round
> through all the pages. Normally auto-converge only matters for machines
> with lots of memory, so this is going to happen each more than 10ms (we
> change it each 4 passes).  You changed it to each 2 passes, and you add
> it a 0.2.  I think  that I would preffer to just have it each single
> pass, but add a 0.1 each pass?  simpler and end result would be the same?
>
>

Well, we certainly could make it run every pass but I think it would get
a little too aggressive then. The reason is, we do not increment the 
throttle
rate by adding 0.2 each time. We increment it by multiplying the current 
rate
by 2. So by doing that every pass we are doubling the exponential growth
rate. I will admit the numbers I chose are hardly scientific... I chose them
because they seemed to provide a decent balance of "throttling 
aggression" in
my workloads.

> This is what the old code did (new code do exactly the same, just in the
> previous patch)
>
> -static void mig_sleep_cpu(void *opq)
> -{
> -    qemu_mutex_unlock_iothread();
> -    g_usleep(30*1000);
> -    qemu_mutex_lock_iothread();
> -}
>
> So, what we are doing, calling async_run_on_cpu() with this function.
>
> To make things short, we end here:
>
> static void flush_queued_work(CPUState *cpu)
> {
>      struct qemu_work_item *wi;
>
>      if (cpu->queued_work_first == NULL) {
>          return;
>      }
>
>      while ((wi = cpu->queued_work_first)) {
>          cpu->queued_work_first = wi->next;
>          wi->func(wi->data);
>          wi->done = true;
>          if (wi->free) {
>              g_free(wi);
>          }
>      }
>      cpu->queued_work_last = NULL;
>      qemu_cond_broadcast(&qemu_work_cond);
> }
>
> So, we are walking a list that is protected with the iothread_lock, and
> we are dropping the lock in the middle of the walk .....  Just hoping
> that nothing else is calling run_async_on_cpu() from a different place
> ....
>
>
> Could we change this to something like:
>
> diff --git a/kvm-all.c b/kvm-all.c
> index 17a3771..ae9635f 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1787,6 +1787,7 @@ int kvm_cpu_exec(CPUState *cpu)
>   {
>       struct kvm_run *run = cpu->kvm_run;
>       int ret, run_ret;
> +    long throotling_time;
>
>       DPRINTF("kvm_cpu_exec()\n");
>
> @@ -1813,8 +1814,15 @@ int kvm_cpu_exec(CPUState *cpu)
>                */
>               qemu_cpu_kick_self();
>           }
> +        throotling_time = cpu->throotling_time;
> +        cpu->throotling_time = 0;
> +        cpu->sleeping_time += throotling_time;
>           qemu_mutex_unlock_iothread();
>
> +
> +        if (throotling_time) {
> +            g_usleep(throttling_time);
> +        }
>           run_ret = kvm_vcpu_ioctl(cpu, KVM_RUN, 0);
>
>           qemu_mutex_lock_iothread();
>
>
>
> And we handle where we are doing now what throotling_time and
> sleeping_time should be?  No need to drop throotling_time/lock/whatever.
>
> It adds an if on the fast path, but we have a call to the hypervisor
> each time, so it shouldn't be so bad, no?
>
> For tcp/xen we should seach for a different place to put this code, but
> you get the idea.  Reason for putting it here is because this is where
> the iothread is dropped, so we don't lost any locking, any other place
> that we put it needs review with respect to this.
>
>
> Jason, I am really, really sorry to have open this big can of worms on
> your patch.  Now you know why I was telling that "improving"
> autoconverge was not as easy as it looked.
>
> With respect ot the last comment, I also think that we can include the
> Jason patches.  They are one imprevement over what we have now.  It is
> just that it needs more improvements.
>

Yes, I understand what you are suggesting here. I can certainly look into it
if you would prefer that rather than accept the current design. The reason I
did things using the timer and thread was because it fit into the old
auto-converge code very nicely. Does it make sense to go forward with my
current design (modified as per your earlier suggestions), and then follow
up with your proposal at a later date?

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

Re: [Qemu-devel] [PATCH v2 1/3] cpu: Provide vcpu throttling interface

[Dr. David Alan Gilbert]

* Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
> On 06/03/2015 03:56 AM, Juan Quintela 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 start function and provide a ratio of
> >>sleep time to normal execution time.
> >>
> >>Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
> >>Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> >
> >
> >
> >Hi
> >
> >sorry that I replied to your previous submission, I had "half done" the
> >mail from yesterday, I still think that all applies.
> >
> >
> >>---
> >>  cpus.c            | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/qom/cpu.h | 46 +++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 108 insertions(+)
> >>
> >>diff --git a/cpus.c b/cpus.c
> >>index de6469f..7568357 100644
> >>--- a/cpus.c
> >>+++ b/cpus.c
> >>@@ -64,6 +64,9 @@
> >>
> >>  #endif /* CONFIG_LINUX */
> >>
> >>+/* Number of ms between cpu throttle operations */
> >>+#define CPU_THROTTLE_TIMESLICE 10
> >
> >
> >We are checking for throotling on each cpu each 10ms.
> >But on patch 2 we can see that we only change the throotling each
> >time that we call migration_bitmap_sync(), that only happens each round
> >through all the pages. Normally auto-converge only matters for machines
> >with lots of memory, so this is going to happen each more than 10ms (we
> >change it each 4 passes).  You changed it to each 2 passes, and you add
> >it a 0.2.  I think  that I would preffer to just have it each single
> >pass, but add a 0.1 each pass?  simpler and end result would be the same?
> >
> >
> 
> Well, we certainly could make it run every pass but I think it would get
> a little too aggressive then. The reason is, we do not increment the
> throttle
> rate by adding 0.2 each time. We increment it by multiplying the current
> rate
> by 2. So by doing that every pass we are doubling the exponential growth
> rate. I will admit the numbers I chose are hardly scientific... I chose them
> because they seemed to provide a decent balance of "throttling aggression"
> in
> my workloads.

That's the advantage of making them parameters.

Dave

> >This is what the old code did (new code do exactly the same, just in the
> >previous patch)
> >
> >-static void mig_sleep_cpu(void *opq)
> >-{
> >-    qemu_mutex_unlock_iothread();
> >-    g_usleep(30*1000);
> >-    qemu_mutex_lock_iothread();
> >-}
> >
> >So, what we are doing, calling async_run_on_cpu() with this function.
> >
> >To make things short, we end here:
> >
> >static void flush_queued_work(CPUState *cpu)
> >{
> >     struct qemu_work_item *wi;
> >
> >     if (cpu->queued_work_first == NULL) {
> >         return;
> >     }
> >
> >     while ((wi = cpu->queued_work_first)) {
> >         cpu->queued_work_first = wi->next;
> >         wi->func(wi->data);
> >         wi->done = true;
> >         if (wi->free) {
> >             g_free(wi);
> >         }
> >     }
> >     cpu->queued_work_last = NULL;
> >     qemu_cond_broadcast(&qemu_work_cond);
> >}
> >
> >So, we are walking a list that is protected with the iothread_lock, and
> >we are dropping the lock in the middle of the walk .....  Just hoping
> >that nothing else is calling run_async_on_cpu() from a different place
> >....
> >
> >
> >Could we change this to something like:
> >
> >diff --git a/kvm-all.c b/kvm-all.c
> >index 17a3771..ae9635f 100644
> >--- a/kvm-all.c
> >+++ b/kvm-all.c
> >@@ -1787,6 +1787,7 @@ int kvm_cpu_exec(CPUState *cpu)
> >  {
> >      struct kvm_run *run = cpu->kvm_run;
> >      int ret, run_ret;
> >+    long throotling_time;
> >
> >      DPRINTF("kvm_cpu_exec()\n");
> >
> >@@ -1813,8 +1814,15 @@ int kvm_cpu_exec(CPUState *cpu)
> >               */
> >              qemu_cpu_kick_self();
> >          }
> >+        throotling_time = cpu->throotling_time;
> >+        cpu->throotling_time = 0;
> >+        cpu->sleeping_time += throotling_time;
> >          qemu_mutex_unlock_iothread();
> >
> >+
> >+        if (throotling_time) {
> >+            g_usleep(throttling_time);
> >+        }
> >          run_ret = kvm_vcpu_ioctl(cpu, KVM_RUN, 0);
> >
> >          qemu_mutex_lock_iothread();
> >
> >
> >
> >And we handle where we are doing now what throotling_time and
> >sleeping_time should be?  No need to drop throotling_time/lock/whatever.
> >
> >It adds an if on the fast path, but we have a call to the hypervisor
> >each time, so it shouldn't be so bad, no?
> >
> >For tcp/xen we should seach for a different place to put this code, but
> >you get the idea.  Reason for putting it here is because this is where
> >the iothread is dropped, so we don't lost any locking, any other place
> >that we put it needs review with respect to this.
> >
> >
> >Jason, I am really, really sorry to have open this big can of worms on
> >your patch.  Now you know why I was telling that "improving"
> >autoconverge was not as easy as it looked.
> >
> >With respect ot the last comment, I also think that we can include the
> >Jason patches.  They are one imprevement over what we have now.  It is
> >just that it needs more improvements.
> >
> 
> Yes, I understand what you are suggesting here. I can certainly look into it
> if you would prefer that rather than accept the current design. The reason I
> did things using the timer and thread was because it fit into the old
> auto-converge code very nicely. Does it make sense to go forward with my
> current design (modified as per your earlier suggestions), and then follow
> up with your proposal at a later date?
> 
> -- 
> -- Jason J. Herne (jjherne@linux.vnet.ibm.com)
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 1/3] cpu: Provide vcpu throttling interface

[Jason J. Herne]

On 06/03/2015 02:03 PM, Dr. David Alan Gilbert wrote:
> * Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
>> On 06/03/2015 03:56 AM, Juan Quintela wrote:
>>> "Jason J. Herne" <jjherne@linux.vnet.ibm.com> wrote:
...
>>> We are checking for throotling on each cpu each 10ms.
>>> But on patch 2 we can see that we only change the throotling each
>>> time that we call migration_bitmap_sync(), that only happens each round
>>> through all the pages. Normally auto-converge only matters for machines
>>> with lots of memory, so this is going to happen each more than 10ms (we
>>> change it each 4 passes).  You changed it to each 2 passes, and you add
>>> it a 0.2.  I think  that I would preffer to just have it each single
>>> pass, but add a 0.1 each pass?  simpler and end result would be the same?
>>>
>>>
>>
>> Well, we certainly could make it run every pass but I think it would get
>> a little too aggressive then. The reason is, we do not increment the
>> throttle
>> rate by adding 0.2 each time. We increment it by multiplying the current
>> rate
>> by 2. So by doing that every pass we are doubling the exponential growth
>> rate. I will admit the numbers I chose are hardly scientific... I chose them
>> because they seemed to provide a decent balance of "throttling aggression"
>> in
>> my workloads.
>
> That's the advantage of making them parameters.

I see your point. Expecting the user to configure these parameters
seems a bit much. But I guess, in theory, it is better to have the
ability to change them and not need it, than need it and not have it
right?

So, as you stated earlier these should hook into MigrationParams
somehow? I'll admit this is the first I've seen this construct. If
this is the optimal location for the two controls (x-throttle-initial,
x-throttle-multiplier?) I can add them there. Will keep defaults of
0.2 for initial and 2.0 for multiplier(is there a better name?)?

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

Re: [Qemu-devel] [PATCH v2 1/3] cpu: Provide vcpu throttling interface

[Jason J. Herne]

On 06/03/2015 02:11 PM, Jason J. Herne wrote:
> On 06/03/2015 02:03 PM, Dr. David Alan Gilbert wrote:
>> * Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
>>> On 06/03/2015 03:56 AM, Juan Quintela wrote:
>>>> "Jason J. Herne" <jjherne@linux.vnet.ibm.com> wrote:
> ...
>>>> We are checking for throotling on each cpu each 10ms.
>>>> But on patch 2 we can see that we only change the throotling each
>>>> time that we call migration_bitmap_sync(), that only happens each round
>>>> through all the pages. Normally auto-converge only matters for machines
>>>> with lots of memory, so this is going to happen each more than 10ms (we
>>>> change it each 4 passes).  You changed it to each 2 passes, and you add
>>>> it a 0.2.  I think  that I would preffer to just have it each single
>>>> pass, but add a 0.1 each pass?  simpler and end result would be the
>>>> same?
>>>>
>>>>
>>>
>>> Well, we certainly could make it run every pass but I think it would get
>>> a little too aggressive then. The reason is, we do not increment the
>>> throttle
>>> rate by adding 0.2 each time. We increment it by multiplying the current
>>> rate
>>> by 2. So by doing that every pass we are doubling the exponential growth
>>> rate. I will admit the numbers I chose are hardly scientific... I
>>> chose them
>>> because they seemed to provide a decent balance of "throttling
>>> aggression"
>>> in
>>> my workloads.
>>
>> That's the advantage of making them parameters.
>
> I see your point. Expecting the user to configure these parameters
> seems a bit much. But I guess, in theory, it is better to have the
> ability to change them and not need it, than need it and not have it
> right?
>
> So, as you stated earlier these should hook into MigrationParams
> somehow? I'll admit this is the first I've seen this construct. If
> this is the optimal location for the two controls (x-throttle-initial,
> x-throttle-multiplier?) I can add them there. Will keep defaults of
> 0.2 for initial and 2.0 for multiplier(is there a better name?)?
>

So I'm attempting add the initial throttle value and the multiplier to 
MigrationParameters and I've come across a problem. 
hmp_migrate_set_parameter assumes all parameters are ints. Apparently 
floating point is not allowed...

     void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
     {
         const char *param = qdict_get_str(qdict, "parameter");
         int value = qdict_get_int(qdict, "value");

Also from hmp-commands.hx

     {
         .name       = "migrate_set_parameter",
         .args_type  = "parameter:s,value:i",
         .params     = "parameter value",
         .help       = "Set the parameter for migration",
         .mhandler.cmd = hmp_migrate_set_parameter,
         .command_completion = migrate_set_parameter_completion,
     },

I'm hoping someone already has an idea for dealing with this problem? If 
not, I suppose this is a good add-on for Dave's discussion on 
redesigning MigrationParameters.

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

Re: [Qemu-devel] [PATCH v2 1/3] cpu: Provide vcpu throttling interface

[Dr. David Alan Gilbert]

* Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
> On 06/03/2015 02:11 PM, Jason J. Herne wrote:
> >On 06/03/2015 02:03 PM, Dr. David Alan Gilbert wrote:
> >>* Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
> >>>On 06/03/2015 03:56 AM, Juan Quintela wrote:
> >>>>"Jason J. Herne" <jjherne@linux.vnet.ibm.com> wrote:
> >...
> >>>>We are checking for throotling on each cpu each 10ms.
> >>>>But on patch 2 we can see that we only change the throotling each
> >>>>time that we call migration_bitmap_sync(), that only happens each round
> >>>>through all the pages. Normally auto-converge only matters for machines
> >>>>with lots of memory, so this is going to happen each more than 10ms (we
> >>>>change it each 4 passes).  You changed it to each 2 passes, and you add
> >>>>it a 0.2.  I think  that I would preffer to just have it each single
> >>>>pass, but add a 0.1 each pass?  simpler and end result would be the
> >>>>same?
> >>>>
> >>>>
> >>>
> >>>Well, we certainly could make it run every pass but I think it would get
> >>>a little too aggressive then. The reason is, we do not increment the
> >>>throttle
> >>>rate by adding 0.2 each time. We increment it by multiplying the current
> >>>rate
> >>>by 2. So by doing that every pass we are doubling the exponential growth
> >>>rate. I will admit the numbers I chose are hardly scientific... I
> >>>chose them
> >>>because they seemed to provide a decent balance of "throttling
> >>>aggression"
> >>>in
> >>>my workloads.
> >>
> >>That's the advantage of making them parameters.
> >
> >I see your point. Expecting the user to configure these parameters
> >seems a bit much. But I guess, in theory, it is better to have the
> >ability to change them and not need it, than need it and not have it
> >right?
> >
> >So, as you stated earlier these should hook into MigrationParams
> >somehow? I'll admit this is the first I've seen this construct. If
> >this is the optimal location for the two controls (x-throttle-initial,
> >x-throttle-multiplier?) I can add them there. Will keep defaults of
> >0.2 for initial and 2.0 for multiplier(is there a better name?)?
> >
> 
> So I'm attempting add the initial throttle value and the multiplier to
> MigrationParameters and I've come across a problem.
> hmp_migrate_set_parameter assumes all parameters are ints. Apparently
> floating point is not allowed...
> 
>     void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>     {
>         const char *param = qdict_get_str(qdict, "parameter");
>         int value = qdict_get_int(qdict, "value");
> 
> Also from hmp-commands.hx
> 
>     {
>         .name       = "migrate_set_parameter",
>         .args_type  = "parameter:s,value:i",
>         .params     = "parameter value",
>         .help       = "Set the parameter for migration",
>         .mhandler.cmd = hmp_migrate_set_parameter,
>         .command_completion = migrate_set_parameter_completion,
>     },
> 
> I'm hoping someone already has an idea for dealing with this problem? If
> not, I suppose this is a good add-on for Dave's discussion on redesigning
> MigrationParameters.

Oh, that's yet another problem; hadn't thought about this one.
I don't think the suggestions I had in the previous mail would help that one
either;   It might work if you flipped the type to 's' and then parsed
that in the hmp code.

(cc'ing Markus in)
Dave

> 
> -- 
> -- Jason J. Herne (jjherne@linux.vnet.ibm.com)
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 1/3] cpu: Provide vcpu throttling interface

[Jason J. Herne]

On 06/09/2015 04:06 AM, Dr. David Alan Gilbert wrote:
> * Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
>> On 06/03/2015 02:11 PM, Jason J. Herne wrote:
>>> On 06/03/2015 02:03 PM, Dr. David Alan Gilbert wrote:
>>>> * Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
>>>>> On 06/03/2015 03:56 AM, Juan Quintela wrote:
>>>>>> "Jason J. Herne" <jjherne@linux.vnet.ibm.com> wrote:
>>> ...
>>>>>> We are checking for throotling on each cpu each 10ms.
>>>>>> But on patch 2 we can see that we only change the throotling each
>>>>>> time that we call migration_bitmap_sync(), that only happens each round
>>>>>> through all the pages. Normally auto-converge only matters for machines
>>>>>> with lots of memory, so this is going to happen each more than 10ms (we
>>>>>> change it each 4 passes).  You changed it to each 2 passes, and you add
>>>>>> it a 0.2.  I think  that I would preffer to just have it each single
>>>>>> pass, but add a 0.1 each pass?  simpler and end result would be the
>>>>>> same?
>>>>>>
>>>>>>
>>>>>
>>>>> Well, we certainly could make it run every pass but I think it would get
>>>>> a little too aggressive then. The reason is, we do not increment the
>>>>> throttle
>>>>> rate by adding 0.2 each time. We increment it by multiplying the current
>>>>> rate
>>>>> by 2. So by doing that every pass we are doubling the exponential growth
>>>>> rate. I will admit the numbers I chose are hardly scientific... I
>>>>> chose them
>>>>> because they seemed to provide a decent balance of "throttling
>>>>> aggression"
>>>>> in
>>>>> my workloads.
>>>>
>>>> That's the advantage of making them parameters.
>>>
>>> I see your point. Expecting the user to configure these parameters
>>> seems a bit much. But I guess, in theory, it is better to have the
>>> ability to change them and not need it, than need it and not have it
>>> right?
>>>
>>> So, as you stated earlier these should hook into MigrationParams
>>> somehow? I'll admit this is the first I've seen this construct. If
>>> this is the optimal location for the two controls (x-throttle-initial,
>>> x-throttle-multiplier?) I can add them there. Will keep defaults of
>>> 0.2 for initial and 2.0 for multiplier(is there a better name?)?
>>>
>>
>> So I'm attempting add the initial throttle value and the multiplier to
>> MigrationParameters and I've come across a problem.
>> hmp_migrate_set_parameter assumes all parameters are ints. Apparently
>> floating point is not allowed...
>>
>>      void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>>      {
>>          const char *param = qdict_get_str(qdict, "parameter");
>>          int value = qdict_get_int(qdict, "value");
>>
>> Also from hmp-commands.hx
>>
>>      {
>>          .name       = "migrate_set_parameter",
>>          .args_type  = "parameter:s,value:i",
>>          .params     = "parameter value",
>>          .help       = "Set the parameter for migration",
>>          .mhandler.cmd = hmp_migrate_set_parameter,
>>          .command_completion = migrate_set_parameter_completion,
>>      },
>>
>> I'm hoping someone already has an idea for dealing with this problem? If
>> not, I suppose this is a good add-on for Dave's discussion on redesigning
>> MigrationParameters.
>
> Oh, that's yet another problem; hadn't thought about this one.
> I don't think the suggestions I had in the previous mail would help that one
> either;   It might work if you flipped the type to 's' and then parsed
> that in the hmp code.
>

I could change it to a string, and then parse the data on a case-by-case 
basis in the switch/case logic. I feel like this is making a bad 
situation worse... But I don't see an easy way around it.

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

Re: [Qemu-devel] [PATCH v2 1/3] cpu: Provide vcpu throttling interface

[Dr. David Alan Gilbert]

* Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
> On 06/09/2015 04:06 AM, Dr. David Alan Gilbert wrote:
> >* Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
> >>On 06/03/2015 02:11 PM, Jason J. Herne wrote:
> >>>On 06/03/2015 02:03 PM, Dr. David Alan Gilbert wrote:
> >>>>* Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
> >>>>>On 06/03/2015 03:56 AM, Juan Quintela wrote:
> >>>>>>"Jason J. Herne" <jjherne@linux.vnet.ibm.com> wrote:
> >>>...
> >>>>>>We are checking for throotling on each cpu each 10ms.
> >>>>>>But on patch 2 we can see that we only change the throotling each
> >>>>>>time that we call migration_bitmap_sync(), that only happens each round
> >>>>>>through all the pages. Normally auto-converge only matters for machines
> >>>>>>with lots of memory, so this is going to happen each more than 10ms (we
> >>>>>>change it each 4 passes).  You changed it to each 2 passes, and you add
> >>>>>>it a 0.2.  I think  that I would preffer to just have it each single
> >>>>>>pass, but add a 0.1 each pass?  simpler and end result would be the
> >>>>>>same?
> >>>>>>
> >>>>>>
> >>>>>
> >>>>>Well, we certainly could make it run every pass but I think it would get
> >>>>>a little too aggressive then. The reason is, we do not increment the
> >>>>>throttle
> >>>>>rate by adding 0.2 each time. We increment it by multiplying the current
> >>>>>rate
> >>>>>by 2. So by doing that every pass we are doubling the exponential growth
> >>>>>rate. I will admit the numbers I chose are hardly scientific... I
> >>>>>chose them
> >>>>>because they seemed to provide a decent balance of "throttling
> >>>>>aggression"
> >>>>>in
> >>>>>my workloads.
> >>>>
> >>>>That's the advantage of making them parameters.
> >>>
> >>>I see your point. Expecting the user to configure these parameters
> >>>seems a bit much. But I guess, in theory, it is better to have the
> >>>ability to change them and not need it, than need it and not have it
> >>>right?
> >>>
> >>>So, as you stated earlier these should hook into MigrationParams
> >>>somehow? I'll admit this is the first I've seen this construct. If
> >>>this is the optimal location for the two controls (x-throttle-initial,
> >>>x-throttle-multiplier?) I can add them there. Will keep defaults of
> >>>0.2 for initial and 2.0 for multiplier(is there a better name?)?
> >>>
> >>
> >>So I'm attempting add the initial throttle value and the multiplier to
> >>MigrationParameters and I've come across a problem.
> >>hmp_migrate_set_parameter assumes all parameters are ints. Apparently
> >>floating point is not allowed...
> >>
> >>     void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
> >>     {
> >>         const char *param = qdict_get_str(qdict, "parameter");
> >>         int value = qdict_get_int(qdict, "value");
> >>
> >>Also from hmp-commands.hx
> >>
> >>     {
> >>         .name       = "migrate_set_parameter",
> >>         .args_type  = "parameter:s,value:i",
> >>         .params     = "parameter value",
> >>         .help       = "Set the parameter for migration",
> >>         .mhandler.cmd = hmp_migrate_set_parameter,
> >>         .command_completion = migrate_set_parameter_completion,
> >>     },
> >>
> >>I'm hoping someone already has an idea for dealing with this problem? If
> >>not, I suppose this is a good add-on for Dave's discussion on redesigning
> >>MigrationParameters.
> >
> >Oh, that's yet another problem; hadn't thought about this one.
> >I don't think the suggestions I had in the previous mail would help that one
> >either;   It might work if you flipped the type to 's' and then parsed
> >that in the hmp code.
> >
> 
> I could change it to a string, and then parse the data on a case-by-case
> basis in the switch/case logic. I feel like this is making a bad situation
> worse... But I don't see an easy way around it.

I think the easiest 'solution' for this is to make the parameter an integer percentage
rather than as a float.  Not that pretty but easier than fighting the
interface code.

Dave


> 
> -- 
> -- Jason J. Herne (jjherne@linux.vnet.ibm.com)
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 1/3] cpu: Provide vcpu throttling interface

[Jason J. Herne]

On 06/10/2015 04:45 AM, Dr. David Alan Gilbert wrote:
> * Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
>> On 06/09/2015 04:06 AM, Dr. David Alan Gilbert wrote:
>>> * Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
>>>> On 06/03/2015 02:11 PM, Jason J. Herne wrote:
>>>>> On 06/03/2015 02:03 PM, Dr. David Alan Gilbert wrote:
>>>>>> * Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
>>>>>>> On 06/03/2015 03:56 AM, Juan Quintela wrote:
>>>>>>>> "Jason J. Herne" <jjherne@linux.vnet.ibm.com> wrote:
>>>>> ...
>>>>>>>> We are checking for throotling on each cpu each 10ms.
>>>>>>>> But on patch 2 we can see that we only change the throotling each
>>>>>>>> time that we call migration_bitmap_sync(), that only happens each round
>>>>>>>> through all the pages. Normally auto-converge only matters for machines
>>>>>>>> with lots of memory, so this is going to happen each more than 10ms (we
>>>>>>>> change it each 4 passes).  You changed it to each 2 passes, and you add
>>>>>>>> it a 0.2.  I think  that I would preffer to just have it each single
>>>>>>>> pass, but add a 0.1 each pass?  simpler and end result would be the
>>>>>>>> same?
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> Well, we certainly could make it run every pass but I think it would get
>>>>>>> a little too aggressive then. The reason is, we do not increment the
>>>>>>> throttle
>>>>>>> rate by adding 0.2 each time. We increment it by multiplying the current
>>>>>>> rate
>>>>>>> by 2. So by doing that every pass we are doubling the exponential growth
>>>>>>> rate. I will admit the numbers I chose are hardly scientific... I
>>>>>>> chose them
>>>>>>> because they seemed to provide a decent balance of "throttling
>>>>>>> aggression"
>>>>>>> in
>>>>>>> my workloads.
>>>>>>
>>>>>> That's the advantage of making them parameters.
>>>>>
>>>>> I see your point. Expecting the user to configure these parameters
>>>>> seems a bit much. But I guess, in theory, it is better to have the
>>>>> ability to change them and not need it, than need it and not have it
>>>>> right?
>>>>>
>>>>> So, as you stated earlier these should hook into MigrationParams
>>>>> somehow? I'll admit this is the first I've seen this construct. If
>>>>> this is the optimal location for the two controls (x-throttle-initial,
>>>>> x-throttle-multiplier?) I can add them there. Will keep defaults of
>>>>> 0.2 for initial and 2.0 for multiplier(is there a better name?)?
>>>>>
>>>>
>>>> So I'm attempting add the initial throttle value and the multiplier to
>>>> MigrationParameters and I've come across a problem.
>>>> hmp_migrate_set_parameter assumes all parameters are ints. Apparently
>>>> floating point is not allowed...
>>>>
>>>>      void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>>>>      {
>>>>          const char *param = qdict_get_str(qdict, "parameter");
>>>>          int value = qdict_get_int(qdict, "value");
>>>>
>>>> Also from hmp-commands.hx
>>>>
>>>>      {
>>>>          .name       = "migrate_set_parameter",
>>>>          .args_type  = "parameter:s,value:i",
>>>>          .params     = "parameter value",
>>>>          .help       = "Set the parameter for migration",
>>>>          .mhandler.cmd = hmp_migrate_set_parameter,
>>>>          .command_completion = migrate_set_parameter_completion,
>>>>      },
>>>>
>>>> I'm hoping someone already has an idea for dealing with this problem? If
>>>> not, I suppose this is a good add-on for Dave's discussion on redesigning
>>>> MigrationParameters.
>>>
>>> Oh, that's yet another problem; hadn't thought about this one.
>>> I don't think the suggestions I had in the previous mail would help that one
>>> either;   It might work if you flipped the type to 's' and then parsed
>>> that in the hmp code.
>>>
>>
>> I could change it to a string, and then parse the data on a case-by-case
>> basis in the switch/case logic. I feel like this is making a bad situation
>> worse... But I don't see an easy way around it.
>
> I think the easiest 'solution' for this is to make the parameter an integer percentage
> rather than as a float.  Not that pretty but easier than fighting the
> interface code.
>

Yes, I'm starting to feel this way :). Another option I'd like to 
collect opinions on it to change hmp's migrate_set_parameter to accept 
argument type O. As per monitor.c:

  * 'O'          option string of the form NAME=VALUE,...
  *              parsed according to QemuOptsList given by its name
  *              Example: 'device:O' uses qemu_device_opts.
  *              Restriction: only lists with empty desc are supported

This would allow arbitrary data types and allow several parameters to be 
set at once right? It looks like it would be a relatively 
straightforward change to make. Opinions?

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

Re: [Qemu-devel] [PATCH v2 1/3] cpu: Provide vcpu throttling interface

[Markus Armbruster]

"Jason J. Herne" <jjherne@linux.vnet.ibm.com> writes:

> On 06/10/2015 04:45 AM, Dr. David Alan Gilbert wrote:
>> * Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
>>> On 06/09/2015 04:06 AM, Dr. David Alan Gilbert wrote:
>>>> * Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
>>>>> On 06/03/2015 02:11 PM, Jason J. Herne wrote:
>>>>>> On 06/03/2015 02:03 PM, Dr. David Alan Gilbert wrote:
>>>>>>> * Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
>>>>>>>> On 06/03/2015 03:56 AM, Juan Quintela wrote:
>>>>>>>>> "Jason J. Herne" <jjherne@linux.vnet.ibm.com> wrote:
>>>>>> ...
>>>>>>>>> We are checking for throotling on each cpu each 10ms.
>>>>>>>>> But on patch 2 we can see that we only change the throotling each
>>>>>>>>> time that we call migration_bitmap_sync(), that only happens each round
>>>>>>>>> through all the pages. Normally auto-converge only matters for machines
>>>>>>>>> with lots of memory, so this is going to happen each more than 10ms (we
>>>>>>>>> change it each 4 passes).  You changed it to each 2 passes, and you add
>>>>>>>>> it a 0.2.  I think  that I would preffer to just have it each single
>>>>>>>>> pass, but add a 0.1 each pass?  simpler and end result would be the
>>>>>>>>> same?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> Well, we certainly could make it run every pass but I think it would get
>>>>>>>> a little too aggressive then. The reason is, we do not increment the
>>>>>>>> throttle
>>>>>>>> rate by adding 0.2 each time. We increment it by multiplying the current
>>>>>>>> rate
>>>>>>>> by 2. So by doing that every pass we are doubling the exponential growth
>>>>>>>> rate. I will admit the numbers I chose are hardly scientific... I
>>>>>>>> chose them
>>>>>>>> because they seemed to provide a decent balance of "throttling
>>>>>>>> aggression"
>>>>>>>> in
>>>>>>>> my workloads.
>>>>>>>
>>>>>>> That's the advantage of making them parameters.
>>>>>>
>>>>>> I see your point. Expecting the user to configure these parameters
>>>>>> seems a bit much. But I guess, in theory, it is better to have the
>>>>>> ability to change them and not need it, than need it and not have it
>>>>>> right?
>>>>>>
>>>>>> So, as you stated earlier these should hook into MigrationParams
>>>>>> somehow? I'll admit this is the first I've seen this construct. If
>>>>>> this is the optimal location for the two controls (x-throttle-initial,
>>>>>> x-throttle-multiplier?) I can add them there. Will keep defaults of
>>>>>> 0.2 for initial and 2.0 for multiplier(is there a better name?)?
>>>>>>
>>>>>
>>>>> So I'm attempting add the initial throttle value and the multiplier to
>>>>> MigrationParameters and I've come across a problem.
>>>>> hmp_migrate_set_parameter assumes all parameters are ints. Apparently
>>>>> floating point is not allowed...
>>>>>
>>>>>      void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>>>>>      {
>>>>>          const char *param = qdict_get_str(qdict, "parameter");
>>>>>          int value = qdict_get_int(qdict, "value");
>>>>>
>>>>> Also from hmp-commands.hx
>>>>>
>>>>>      {
>>>>>          .name       = "migrate_set_parameter",
>>>>>          .args_type  = "parameter:s,value:i",
>>>>>          .params     = "parameter value",
>>>>>          .help       = "Set the parameter for migration",
>>>>>          .mhandler.cmd = hmp_migrate_set_parameter,
>>>>>          .command_completion = migrate_set_parameter_completion,
>>>>>      },
>>>>>
>>>>> I'm hoping someone already has an idea for dealing with this problem? If
>>>>> not, I suppose this is a good add-on for Dave's discussion on redesigning
>>>>> MigrationParameters.
>>>>
>>>> Oh, that's yet another problem; hadn't thought about this one.
>>>> I don't think the suggestions I had in the previous mail would help that one
>>>> either;   It might work if you flipped the type to 's' and then parsed
>>>> that in the hmp code.
>>>>
>>>
>>> I could change it to a string, and then parse the data on a case-by-case
>>> basis in the switch/case logic. I feel like this is making a bad situation
>>> worse... But I don't see an easy way around it.

Ugh.

Differently ugh: make value's declared type a double, then reject
unwanted values depending on the parameter string.

>> I think the easiest 'solution' for this is to make the parameter an
>> integer percentage
>> rather than as a float.  Not that pretty but easier than fighting the
>> interface code.

If you can make do with int, you don't have to change the HMP command.

If I understand your problem correctly (I only skimmed, sorry), we're
talking about two parameters: an initial throttling factor (range (0,1],
percentage should do) and a "throttling multiplier", but I didn't quite
get what it's supposed to do, or what its acceptable range would be.

> Yes, I'm starting to feel this way :). Another option I'd like to
> collect opinions on it to change hmp's migrate_set_parameter to accept
> argument type O. As per monitor.c:
>
>  * 'O'          option string of the form NAME=VALUE,...
>  *              parsed according to QemuOptsList given by its name
>  *              Example: 'device:O' uses qemu_device_opts.
>  *              Restriction: only lists with empty desc are supported
>
> This would allow arbitrary data types and allow several parameters to
> be set at once right? It looks like it would be a relatively
> straightforward change to make. Opinions?

Incompatible change.  "migrate_set_parameter P V" becomes
"migrate_set_parameter P=V".  I guess we could accept that.

I'd prefer that to doing the parsing yourself, because it has a better
chance at keeping the parsing consistent.