[Qemu-devel] [PATCH] migration: Improve accuracy of vCPU throttling with per-vCPU timers

[Qemu-devel] [PATCH] migration: Improve accuracy of vCPU throttling with per-vCPU timers

[Cosmin Marin]

During auto-convergence live migration, the configured throttling rate
is not matched in practice. Experimental measurements of throughput for
a memory-write intensive workload indicate disparities between expected
and measured throttle rate - when set to 99%, the actual throttle rate
was 95%. The workload spawns multiple threads (#threads equals #vCPUs)
that dirty most of the VM's memory in an infinite loop.

The root cause is the usage of a VM-wide timer to schedule and execute
asynchronously cpu_throttle_thread() on the vCPUs. Firstly, there are
scalability limitations at scheduling time as a VM-wide (global) loop
must iterate over all vCPUs while running atomic operations (i.e., may
induce delays between vCPUs); moreover, if a vCPU is already running
cpu_throttle_thread() (!DONE) it is skipped (i.e., may induce uneven
aggregate sleep times across vCPUs). Secondly, there is a race condition
between the vCPU threads and the 'scheduling' (migration) thread as a
vCPU thread needs to release the iothread lock, sleep, reacquire the
lock and mark "itself" as completed (DONE). Configuring correct per-vCPU
sleep intervals using this model is non-trivial.

To address the above issues, per-vCPU timers replace the per-VM timer.
The migration thread globally decides the throttling level while each
vCPU thread calculates the equivalent sleep times and sleeps
accordingly. The following table summarizes the results obtained by
running the workload on a 22vCPUs/45GB VM in both scenarios.

+----------------------------------------------------------------+
|          |      per-VM Timer        |   per-vCPU Timer         |
|  Target  |==========================|==========================|
| Throttle | Throughput |    Actual   | Throughput |    Actual   |
|    (%)   |   (GBps)   | Throttle(%) |   (GBps)   | Throttle(%) |
|----------|------------|-------------|------------|-------------|
|         0|     ~493.50|            0|     ~493.50|           0 |
|        20|      395.65|        19.81|      390.35|        20.88|
|        30|      356.43|        27.76|      342.39|        30.60|
|        40|      317.26|        35.69|      293.99|        40.41|
|        50|      268.78|        45.52|      244.95|        50.35|
|        60|      214.61|        56.50|      195.23|        60.43|
|        70|      164.72|        66.61|      147.55|        70.09|
|        80|      112.62|        77.17|       98.52|        80.03|
|        90|       57.09|        88.43|       47.90|        90.29|
|        99|       26.87|        94.55|        3.11|        99.36|
+----------------------------------------------------------------+

The results support a per-vCPU timer model as it produces more accurate
throttling.

Signed-off-by: Cosmin Marin <cosmin@nutanix.com>
---
 cpus.c            | 29 +++++++++++++++--------------
 include/qom/cpu.h |  4 ++--
 2 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/cpus.c b/cpus.c
index dde3b7b981..c2bd3babf6 100644
--- a/cpus.c
+++ b/cpus.c
@@ -80,7 +80,6 @@ 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
@@ -792,40 +791,42 @@ static void cpu_throttle_thread(CPUState *cpu, run_on_cpu_data opaque)
     qemu_mutex_unlock_iothread();
     g_usleep(sleeptime_ns / 1000); /* Convert ns to us for usleep call */
     qemu_mutex_lock_iothread();
-    atomic_set(&cpu->throttle_thread_scheduled, 0);
 }
 
 static void cpu_throttle_timer_tick(void *opaque)
 {
-    CPUState *cpu;
+    CPUState *cpu = (CPUState *)opaque;
     double pct;
 
     /* Stop the timer if needed */
     if (!cpu_throttle_get_percentage()) {
         return;
     }
-    CPU_FOREACH(cpu) {
-        if (!atomic_xchg(&cpu->throttle_thread_scheduled, 1)) {
-            async_run_on_cpu(cpu, cpu_throttle_thread,
-                             RUN_ON_CPU_NULL);
-        }
-    }
+    
+    async_run_on_cpu(cpu, cpu_throttle_thread, RUN_ON_CPU_NULL);
 
     pct = (double)cpu_throttle_get_percentage()/100;
-    timer_mod(throttle_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT) +
+    timer_mod(cpu->throttle_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT) +
                                    CPU_THROTTLE_TIMESLICE_NS / (1-pct));
 }
 
 void cpu_throttle_set(int new_throttle_pct)
 {
+    CPUState *cpu;
+    double pct;
+
     /* Ensure throttle percentage is within valid range */
     new_throttle_pct = MIN(new_throttle_pct, CPU_THROTTLE_PCT_MAX);
     new_throttle_pct = MAX(new_throttle_pct, CPU_THROTTLE_PCT_MIN);
 
     atomic_set(&throttle_percentage, new_throttle_pct);
 
-    timer_mod(throttle_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT) +
-                                       CPU_THROTTLE_TIMESLICE_NS);
+    pct = (double)new_throttle_pct/100;
+    CPU_FOREACH(cpu) {
+        timer_mod_anticipate(cpu->throttle_timer,
+                qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT) +
+                CPU_THROTTLE_TIMESLICE_NS / (1-pct));
+    }
 }
 
 void cpu_throttle_stop(void)
@@ -848,8 +849,6 @@ void cpu_ticks_init(void)
     seqlock_init(&timers_state.vm_clock_seqlock);
     qemu_spin_init(&timers_state.vm_clock_lock);
     vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
-    throttle_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL_RT,
-                                           cpu_throttle_timer_tick, NULL);
 }
 
 void configure_icount(QemuOpts *opts, Error **errp)
@@ -1267,6 +1266,8 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
     qemu_thread_get_self(cpu->thread);
     cpu->thread_id = qemu_get_thread_id();
     cpu->can_do_io = 1;
+    cpu->throttle_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL_RT,
+            cpu_throttle_timer_tick, cpu);
     current_cpu = cpu;
 
     r = kvm_init_vcpu(cpu);
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 5ee0046b62..5a11baec69 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -439,10 +439,10 @@ struct CPUState {
     /* shared by kvm, hax and hvf */
     bool vcpu_dirty;
 
-    /* Used to keep track of an outstanding cpu throttle thread for migration
+    /* Used to cyclically trigger vCPU throttling during VM migration
      * autoconverge
      */
-    bool throttle_thread_scheduled;
+    QEMUTimer *throttle_timer;
 
     bool ignore_memory_transaction_failures;
 
-- 
2.16.5

Re: [Qemu-devel] [PATCH] migration: Improve accuracy of vCPU throttling with per-vCPU timers

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190614161106.218854-1-cosmin@nutanix.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20190614161106.218854-1-cosmin@nutanix.com
Subject: [Qemu-devel] [PATCH] migration: Improve accuracy of vCPU throttling with per-vCPU timers
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20190614161106.218854-1-cosmin@nutanix.com -> patchew/20190614161106.218854-1-cosmin@nutanix.com
Switched to a new branch 'test'
4497032 migration: Improve accuracy of vCPU throttling with per-vCPU timers

=== OUTPUT BEGIN ===
ERROR: trailing whitespace
#91: FILE: cpus.c:805:
+    $

ERROR: spaces required around that '/' (ctx:VxV)
#113: FILE: cpus.c:824:
+    pct = (double)new_throttle_pct/100;
                                   ^

ERROR: spaces required around that '-' (ctx:VxV)
#117: FILE: cpus.c:828:
+                CPU_THROTTLE_TIMESLICE_NS / (1-pct));
                                               ^

WARNING: Block comments use a leading /* on a separate line
#149: FILE: include/qom/cpu.h:442:
+    /* Used to cyclically trigger vCPU throttling during VM migration

total: 3 errors, 1 warnings, 88 lines checked

Commit 4497032a7d79 (migration: Improve accuracy of vCPU throttling with per-vCPU timers) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190614161106.218854-1-cosmin@nutanix.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH] migration: Improve accuracy of vCPU throttling with per-vCPU timers

[Peter Xu]

On Fri, Jun 14, 2019 at 09:11:06AM -0700, Cosmin Marin wrote:
> During auto-convergence live migration, the configured throttling rate
> is not matched in practice. Experimental measurements of throughput for
> a memory-write intensive workload indicate disparities between expected
> and measured throttle rate - when set to 99%, the actual throttle rate
> was 95%. The workload spawns multiple threads (#threads equals #vCPUs)
> that dirty most of the VM's memory in an infinite loop.
> 
> The root cause is the usage of a VM-wide timer to schedule and execute
> asynchronously cpu_throttle_thread() on the vCPUs. Firstly, there are
> scalability limitations at scheduling time as a VM-wide (global) loop
> must iterate over all vCPUs while running atomic operations (i.e., may
> induce delays between vCPUs); moreover, if a vCPU is already running
> cpu_throttle_thread() (!DONE) it is skipped (i.e., may induce uneven
> aggregate sleep times across vCPUs). Secondly, there is a race condition
> between the vCPU threads and the 'scheduling' (migration) thread as a
> vCPU thread needs to release the iothread lock, sleep, reacquire the
> lock and mark "itself" as completed (DONE). Configuring correct per-vCPU
> sleep intervals using this model is non-trivial.
> 
> To address the above issues, per-vCPU timers replace the per-VM timer.
> The migration thread globally decides the throttling level while each
> vCPU thread calculates the equivalent sleep times and sleeps
> accordingly. The following table summarizes the results obtained by
> running the workload on a 22vCPUs/45GB VM in both scenarios.
> 
> +----------------------------------------------------------------+
> |          |      per-VM Timer        |   per-vCPU Timer         |
> |  Target  |==========================|==========================|
> | Throttle | Throughput |    Actual   | Throughput |    Actual   |
> |    (%)   |   (GBps)   | Throttle(%) |   (GBps)   | Throttle(%) |
> |----------|------------|-------------|------------|-------------|
> |         0|     ~493.50|            0|     ~493.50|           0 |
> |        20|      395.65|        19.81|      390.35|        20.88|
> |        30|      356.43|        27.76|      342.39|        30.60|
> |        40|      317.26|        35.69|      293.99|        40.41|
> |        50|      268.78|        45.52|      244.95|        50.35|
> |        60|      214.61|        56.50|      195.23|        60.43|
> |        70|      164.72|        66.61|      147.55|        70.09|
> |        80|      112.62|        77.17|       98.52|        80.03|
> |        90|       57.09|        88.43|       47.90|        90.29|
> |        99|       26.87|        94.55|        3.11|        99.36|
> +----------------------------------------------------------------+

Hi, Cosmin,

The numbers are really nice... though I am unsure about whether this
patch will help in scalability or even make it worth?  After all it'll
create N timers instead of one when we have N vcpus.

What I feel like is that async_run_on_cpu() should be fairly fast
except the fact you mentioned that we'll skip the throttling
conditionally if it's still ongoing, as if we see the numbers the old
code throttling is always less than the target.

Have you tried to simply remove the throttle_thread_scheduled
variable, and trigger the throttle unconditionally (but still with one
timer)?

> 
> The results support a per-vCPU timer model as it produces more accurate
> throttling.
> 
> Signed-off-by: Cosmin Marin <cosmin@nutanix.com>
> ---
>  cpus.c            | 29 +++++++++++++++--------------
>  include/qom/cpu.h |  4 ++--
>  2 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index dde3b7b981..c2bd3babf6 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -80,7 +80,6 @@ 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
> @@ -792,40 +791,42 @@ static void cpu_throttle_thread(CPUState *cpu, run_on_cpu_data opaque)
>      qemu_mutex_unlock_iothread();
>      g_usleep(sleeptime_ns / 1000); /* Convert ns to us for usleep call */
>      qemu_mutex_lock_iothread();
> -    atomic_set(&cpu->throttle_thread_scheduled, 0);
>  }
>  
>  static void cpu_throttle_timer_tick(void *opaque)
>  {
> -    CPUState *cpu;
> +    CPUState *cpu = (CPUState *)opaque;
>      double pct;
>  
>      /* Stop the timer if needed */
>      if (!cpu_throttle_get_percentage()) {
>          return;
>      }
> -    CPU_FOREACH(cpu) {
> -        if (!atomic_xchg(&cpu->throttle_thread_scheduled, 1)) {
> -            async_run_on_cpu(cpu, cpu_throttle_thread,
> -                             RUN_ON_CPU_NULL);
> -        }
> -    }
> +    
> +    async_run_on_cpu(cpu, cpu_throttle_thread, RUN_ON_CPU_NULL);
>  
>      pct = (double)cpu_throttle_get_percentage()/100;
> -    timer_mod(throttle_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT) +
> +    timer_mod(cpu->throttle_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT) +
>                                     CPU_THROTTLE_TIMESLICE_NS / (1-pct));
>  }
>  
>  void cpu_throttle_set(int new_throttle_pct)
>  {
> +    CPUState *cpu;
> +    double pct;
> +
>      /* Ensure throttle percentage is within valid range */
>      new_throttle_pct = MIN(new_throttle_pct, CPU_THROTTLE_PCT_MAX);
>      new_throttle_pct = MAX(new_throttle_pct, CPU_THROTTLE_PCT_MIN);
>  
>      atomic_set(&throttle_percentage, new_throttle_pct);
>  
> -    timer_mod(throttle_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT) +
> -                                       CPU_THROTTLE_TIMESLICE_NS);
> +    pct = (double)new_throttle_pct/100;
> +    CPU_FOREACH(cpu) {
> +        timer_mod_anticipate(cpu->throttle_timer,
> +                qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT) +
> +                CPU_THROTTLE_TIMESLICE_NS / (1-pct));
> +    }
>  }
>  
>  void cpu_throttle_stop(void)
> @@ -848,8 +849,6 @@ void cpu_ticks_init(void)
>      seqlock_init(&timers_state.vm_clock_seqlock);
>      qemu_spin_init(&timers_state.vm_clock_lock);
>      vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
> -    throttle_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL_RT,
> -                                           cpu_throttle_timer_tick, NULL);
>  }
>  
>  void configure_icount(QemuOpts *opts, Error **errp)
> @@ -1267,6 +1266,8 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
>      qemu_thread_get_self(cpu->thread);
>      cpu->thread_id = qemu_get_thread_id();
>      cpu->can_do_io = 1;
> +    cpu->throttle_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL_RT,
> +            cpu_throttle_timer_tick, cpu);
>      current_cpu = cpu;
>  
>      r = kvm_init_vcpu(cpu);
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 5ee0046b62..5a11baec69 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -439,10 +439,10 @@ struct CPUState {
>      /* shared by kvm, hax and hvf */
>      bool vcpu_dirty;
>  
> -    /* Used to keep track of an outstanding cpu throttle thread for migration
> +    /* Used to cyclically trigger vCPU throttling during VM migration
>       * autoconverge
>       */
> -    bool throttle_thread_scheduled;
> +    QEMUTimer *throttle_timer;
>  
>      bool ignore_memory_transaction_failures;
>  
> -- 
> 2.16.5
> 
> 

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH] migration: Improve accuracy of vCPU throttling with per-vCPU timers

[Cosmin Marin]

On 17/06/2019, 04:46, "Peter Xu" <peterx@redhat.com> wrote:

    On Fri, Jun 14, 2019 at 09:11:06AM -0700, Cosmin Marin wrote:
    > During auto-convergence live migration, the configured throttling rate
    > is not matched in practice. Experimental measurements of throughput for
    > a memory-write intensive workload indicate disparities between expected
    > and measured throttle rate - when set to 99%, the actual throttle rate
    > was 95%. The workload spawns multiple threads (#threads equals #vCPUs)
    > that dirty most of the VM's memory in an infinite loop.
    > 
    > The root cause is the usage of a VM-wide timer to schedule and execute
    > asynchronously cpu_throttle_thread() on the vCPUs. Firstly, there are
    > scalability limitations at scheduling time as a VM-wide (global) loop
    > must iterate over all vCPUs while running atomic operations (i.e., may
    > induce delays between vCPUs); moreover, if a vCPU is already running
    > cpu_throttle_thread() (!DONE) it is skipped (i.e., may induce uneven
    > aggregate sleep times across vCPUs). Secondly, there is a race condition
    > between the vCPU threads and the 'scheduling' (migration) thread as a
    > vCPU thread needs to release the iothread lock, sleep, reacquire the
    > lock and mark "itself" as completed (DONE). Configuring correct per-vCPU
    > sleep intervals using this model is non-trivial.
    > 
    > To address the above issues, per-vCPU timers replace the per-VM timer.
    > The migration thread globally decides the throttling level while each
    > vCPU thread calculates the equivalent sleep times and sleeps
    > accordingly. The following table summarizes the results obtained by
    > running the workload on a 22vCPUs/45GB VM in both scenarios.
    > 
    > +----------------------------------------------------------------+
    > |          |      per-VM Timer        |   per-vCPU Timer         |
    > |  Target  |==========================|==========================|
    > | Throttle | Throughput |    Actual   | Throughput |    Actual   |
    > |    (%)   |   (GBps)   | Throttle(%) |   (GBps)   | Throttle(%) |
    > |----------|------------|-------------|------------|-------------|
    > |         0|     ~493.50|            0|     ~493.50|           0 |
    > |        20|      395.65|        19.81|      390.35|        20.88|
    > |        30|      356.43|        27.76|      342.39|        30.60|
    > |        40|      317.26|        35.69|      293.99|        40.41|
    > |        50|      268.78|        45.52|      244.95|        50.35|
    > |        60|      214.61|        56.50|      195.23|        60.43|
    > |        70|      164.72|        66.61|      147.55|        70.09|
    > |        80|      112.62|        77.17|       98.52|        80.03|
    > |        90|       57.09|        88.43|       47.90|        90.29|
    > |        99|       26.87|        94.55|        3.11|        99.36|
    > +----------------------------------------------------------------+
    
    Hi, Cosmin,
    
    The numbers are really nice... though I am unsure about whether this
    patch will help in scalability or even make it worth?  After all it'll
    create N timers instead of one when we have N vcpus.
    
	Hi Peter,

	thanks for reviewing the patch. Indeed, I agree that it's almost impossible to determine which solution it's better from the scalability perspective. However, I feel that using per-vCPU timers is the only way for ensuring correctness of the throttling ratio.

	It's a bit unclear to me how the throttling ratio inconsistency can be fixed by using a single timer even avoiding the conditional timer re-arming.  Could you provide more details about the use of a single timer ?

    What I feel like is that async_run_on_cpu() should be fairly fast
    except the fact you mentioned that we'll skip the throttling
    conditionally if it's still ongoing, as if we see the numbers the old
    code throttling is always less than the target.
    
    Have you tried to simply remove the throttle_thread_scheduled
    variable, and trigger the throttle unconditionally (but still with one
    timer)?
    
    > 
    > The results support a per-vCPU timer model as it produces more accurate
    > throttling.
    > 
    > Signed-off-by: Cosmin Marin <cosmin@nutanix.com>
    > ---
    >  cpus.c            | 29 +++++++++++++++--------------
    >  include/qom/cpu.h |  4 ++--
    >  2 files changed, 17 insertions(+), 16 deletions(-)
    > 
    > diff --git a/cpus.c b/cpus.c
    > index dde3b7b981..c2bd3babf6 100644
    > --- a/cpus.c
    > +++ b/cpus.c
    > @@ -80,7 +80,6 @@ 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
    > @@ -792,40 +791,42 @@ static void cpu_throttle_thread(CPUState *cpu, run_on_cpu_data opaque)
    >      qemu_mutex_unlock_iothread();
    >      g_usleep(sleeptime_ns / 1000); /* Convert ns to us for usleep call */
    >      qemu_mutex_lock_iothread();
    > -    atomic_set(&cpu->throttle_thread_scheduled, 0);
    >  }
    >  
    >  static void cpu_throttle_timer_tick(void *opaque)
    >  {
    > -    CPUState *cpu;
    > +    CPUState *cpu = (CPUState *)opaque;
    >      double pct;
    >  
    >      /* Stop the timer if needed */
    >      if (!cpu_throttle_get_percentage()) {
    >          return;
    >      }
    > -    CPU_FOREACH(cpu) {
    > -        if (!atomic_xchg(&cpu->throttle_thread_scheduled, 1)) {
    > -            async_run_on_cpu(cpu, cpu_throttle_thread,
    > -                             RUN_ON_CPU_NULL);
    > -        }
    > -    }
    > +    
    > +    async_run_on_cpu(cpu, cpu_throttle_thread, RUN_ON_CPU_NULL);
    >  
    >      pct = (double)cpu_throttle_get_percentage()/100;
    > -    timer_mod(throttle_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT) +
    > +    timer_mod(cpu->throttle_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT) +
    >                                     CPU_THROTTLE_TIMESLICE_NS / (1-pct));
    >  }
    >  
    >  void cpu_throttle_set(int new_throttle_pct)
    >  {
    > +    CPUState *cpu;
    > +    double pct;
    > +
    >      /* Ensure throttle percentage is within valid range */
    >      new_throttle_pct = MIN(new_throttle_pct, CPU_THROTTLE_PCT_MAX);
    >      new_throttle_pct = MAX(new_throttle_pct, CPU_THROTTLE_PCT_MIN);
    >  
    >      atomic_set(&throttle_percentage, new_throttle_pct);
    >  
    > -    timer_mod(throttle_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT) +
    > -                                       CPU_THROTTLE_TIMESLICE_NS);
    > +    pct = (double)new_throttle_pct/100;
    > +    CPU_FOREACH(cpu) {
    > +        timer_mod_anticipate(cpu->throttle_timer,
    > +                qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT) +
    > +                CPU_THROTTLE_TIMESLICE_NS / (1-pct));
    > +    }
    >  }
    >  
    >  void cpu_throttle_stop(void)
    > @@ -848,8 +849,6 @@ void cpu_ticks_init(void)
    >      seqlock_init(&timers_state.vm_clock_seqlock);
    >      qemu_spin_init(&timers_state.vm_clock_lock);
    >      vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
    > -    throttle_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL_RT,
    > -                                           cpu_throttle_timer_tick, NULL);
    >  }
    >  
    >  void configure_icount(QemuOpts *opts, Error **errp)
    > @@ -1267,6 +1266,8 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
    >      qemu_thread_get_self(cpu->thread);
    >      cpu->thread_id = qemu_get_thread_id();
    >      cpu->can_do_io = 1;
    > +    cpu->throttle_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL_RT,
    > +            cpu_throttle_timer_tick, cpu);
    >      current_cpu = cpu;
    >  
    >      r = kvm_init_vcpu(cpu);
    > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
    > index 5ee0046b62..5a11baec69 100644
    > --- a/include/qom/cpu.h
    > +++ b/include/qom/cpu.h
    > @@ -439,10 +439,10 @@ struct CPUState {
    >      /* shared by kvm, hax and hvf */
    >      bool vcpu_dirty;
    >  
    > -    /* Used to keep track of an outstanding cpu throttle thread for migration
    > +    /* Used to cyclically trigger vCPU throttling during VM migration
    >       * autoconverge
    >       */
    > -    bool throttle_thread_scheduled;
    > +    QEMUTimer *throttle_timer;
    >  
    >      bool ignore_memory_transaction_failures;
    >  
    > -- 
    > 2.16.5
    > 
    > 
    
    Regards,
    
    -- 
    Peter Xu
    

Re: [Qemu-devel] [PATCH] migration: Improve accuracy of vCPU throttling with per-vCPU timers

[Peter Xu]

On Tue, Jun 18, 2019 at 12:25:43PM +0000, Cosmin Marin wrote:
> 	Hi Peter,
> 
> 	thanks for reviewing the patch. Indeed, I agree that it's almost impossible to determine which solution it's better from the scalability perspective. However, I feel that using per-vCPU timers is the only way for ensuring correctness of the throttling ratio.

The thing is that your patch actually contains two changes:

1. use N timers instead of one.

2. remove throttle_thread_scheduled check, so we do the throttle
   always

Here what I'm worried is that _maybe_ the 2nd item is the one that
really helped.

Note that there is a side effect that we might queue more than one
work on one specific cpu if we queue it too fast, but it does not
block us from trying it out to identify which item (1 or 2 or both)
really helped here.  Then if we think that (queuing too much) is an
issue then we can discuss on how to fix it since current patch will
have this problem as well.

> 
> 	It's a bit unclear to me how the throttling ratio inconsistency can be fixed by using a single timer even avoiding the conditional timer re-arming.  Could you provide more details about the use of a single timer ?
  
It'll be simply a patch only contains the changes for item (2) above.
To be explicit, it is:

-------------------------------------------------------------------
diff --git a/cpus.c b/cpus.c
index dde3b7b981..bb59675b98 100644
--- a/cpus.c
+++ b/cpus.c
@@ -792,7 +792,6 @@ static void cpu_throttle_thread(CPUState *cpu, run_on_cpu_data opaque)
     qemu_mutex_unlock_iothread();
     g_usleep(sleeptime_ns / 1000); /* Convert ns to us for usleep call */
     qemu_mutex_lock_iothread();
-    atomic_set(&cpu->throttle_thread_scheduled, 0);
 }

 static void cpu_throttle_timer_tick(void *opaque)
@@ -805,10 +804,7 @@ static void cpu_throttle_timer_tick(void *opaque)
         return;
     }
     CPU_FOREACH(cpu) {
-        if (!atomic_xchg(&cpu->throttle_thread_scheduled, 1)) {
-            async_run_on_cpu(cpu, cpu_throttle_thread,
-                             RUN_ON_CPU_NULL);
-        }
+        async_run_on_cpu(cpu, cpu_throttle_thread, RUN_ON_CPU_NULL);
     }

     pct = (double)cpu_throttle_get_percentage()/100;
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 5ee0046b62..0bd34fbb70 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -439,11 +439,6 @@ struct CPUState {
     /* shared by kvm, hax and hvf */
     bool vcpu_dirty;

-    /* Used to keep track of an outstanding cpu throttle thread for migration
-     * autoconverge
-     */
-    bool throttle_thread_scheduled;
-
     bool ignore_memory_transaction_failures;

     struct hax_vcpu_state *hax_vcpu;
-------------------------------------------------------------------

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH] migration: Improve accuracy of vCPU throttling with per-vCPU timers

[Cosmin Marin]

On 18/06/2019, 15:51, "Peter Xu" <peterx@redhat.com> wrote:

    On Tue, Jun 18, 2019 at 12:25:43PM +0000, Cosmin Marin wrote:
    > 	Hi Peter,
    > 
    > 	thanks for reviewing the patch. Indeed, I agree that it's almost impossible to determine which solution it's better from the scalability perspective. However, I feel that using per-vCPU timers is the only way for ensuring correctness of the throttling ratio.
    
    The thing is that your patch actually contains two changes:
    
    1. use N timers instead of one.
    
    2. remove throttle_thread_scheduled check, so we do the throttle
       always
    
    Here what I'm worried is that _maybe_ the 2nd item is the one that
    really helped.
    
	C: The removal of *throttle_thread_scheduled* is a consequence of the per-vCPU model only. In this model, each of the vCPUs schedules work just for itself (as part of the timer's firing callback) - there's no global point of control - therefore, the variable isn't helpful for scheduling anymore.

    Note that there is a side effect that we might queue more than one
    work on one specific cpu if we queue it too fast, but it does not
    block us from trying it out to identify which item (1 or 2 or both)
    really helped here.  Then if we think that (queuing too much) is an
    issue then we can discuss on how to fix it since current patch will
    have this problem as well.
    
	C: I believe that in the per-vCPU timer implementation we cannot queue more than one piece of work because, here, the vCPU queues work for itself and that happens only when the timer fires - so, the two "states" - scheduling and sleeping - are mutually exclusive running from the same thread context. 
    > 
    > 	It's a bit unclear to me how the throttling ratio inconsistency can be fixed by using a single timer even avoiding the conditional timer re-arming.  Could you provide more details about the use of a single timer ?

	C: I feel like in this case it will sleep too much running into a problem similar to the one solved by 90bb0c0; under heavy throttling more than one work item may be scheduled.
      

    It'll be simply a patch only contains the changes for item (2) above.
    To be explicit, it is:
    
    -------------------------------------------------------------------
    diff --git a/cpus.c b/cpus.c
    index dde3b7b981..bb59675b98 100644
    --- a/cpus.c
    +++ b/cpus.c
    @@ -792,7 +792,6 @@ static void cpu_throttle_thread(CPUState *cpu, run_on_cpu_data opaque)
         qemu_mutex_unlock_iothread();
         g_usleep(sleeptime_ns / 1000); /* Convert ns to us for usleep call */
         qemu_mutex_lock_iothread();
    -    atomic_set(&cpu->throttle_thread_scheduled, 0);
     }
    
     static void cpu_throttle_timer_tick(void *opaque)
    @@ -805,10 +804,7 @@ static void cpu_throttle_timer_tick(void *opaque)
             return;
         }
         CPU_FOREACH(cpu) {
    -        if (!atomic_xchg(&cpu->throttle_thread_scheduled, 1)) {
    -            async_run_on_cpu(cpu, cpu_throttle_thread,
    -                             RUN_ON_CPU_NULL);
    -        }
    +        async_run_on_cpu(cpu, cpu_throttle_thread, RUN_ON_CPU_NULL);
         }
    
         pct = (double)cpu_throttle_get_percentage()/100;
    diff --git a/include/qom/cpu.h b/include/qom/cpu.h
    index 5ee0046b62..0bd34fbb70 100644
    --- a/include/qom/cpu.h
    +++ b/include/qom/cpu.h
    @@ -439,11 +439,6 @@ struct CPUState {
         /* shared by kvm, hax and hvf */
         bool vcpu_dirty;
    
    -    /* Used to keep track of an outstanding cpu throttle thread for migration
    -     * autoconverge
    -     */
    -    bool throttle_thread_scheduled;
    -
         bool ignore_memory_transaction_failures;
    
         struct hax_vcpu_state *hax_vcpu;
    -------------------------------------------------------------------
    
    Regards,
    
    -- 
    Peter Xu
    

Re: [Qemu-devel] [PATCH] migration: Improve accuracy of vCPU throttling with per-vCPU timers

[Peter Xu]

On Tue, Jun 18, 2019 at 04:52:09PM +0000, Cosmin Marin wrote:
> 
> 
> On 18/06/2019, 15:51, "Peter Xu" <peterx@redhat.com> wrote:
> 
>     On Tue, Jun 18, 2019 at 12:25:43PM +0000, Cosmin Marin wrote:
>     > 	Hi Peter,
>     > 
>     > 	thanks for reviewing the patch. Indeed, I agree that it's almost impossible to determine which solution it's better from the scalability perspective. However, I feel that using per-vCPU timers is the only way for ensuring correctness of the throttling ratio.
>     
>     The thing is that your patch actually contains two changes:
>     
>     1. use N timers instead of one.
>     
>     2. remove throttle_thread_scheduled check, so we do the throttle
>        always
>     
>     Here what I'm worried is that _maybe_ the 2nd item is the one that
>     really helped.
>     
> 	C: The removal of *throttle_thread_scheduled* is a consequence of the per-vCPU model only. In this model, each of the vCPUs schedules work just for itself (as part of the timer's firing callback) - there's no global point of control - therefore, the variable isn't helpful for scheduling anymore.
> 
>     Note that there is a side effect that we might queue more than one
>     work on one specific cpu if we queue it too fast, but it does not
>     block us from trying it out to identify which item (1 or 2 or both)
>     really helped here.  Then if we think that (queuing too much) is an
>     issue then we can discuss on how to fix it since current patch will
>     have this problem as well.
>     
> 	C: I believe that in the per-vCPU timer implementation we cannot queue more than one piece of work because, here, the vCPU queues work for itself and that happens only when the timer fires - so, the two "states" - scheduling and sleeping - are mutually exclusive running from the same thread context. 

I think this is the place where I'm in question with - I don't think
they are using the same context.  IMO the timer will always be run in
the main thread no matter you use per-cpu timer or not, however the
sleeping part should be run on per-cpu.

A simple way to verify it would be: break at cpu_throttle_timer_tick()
to see which thread it is running in.

>     > 
>     > 	It's a bit unclear to me how the throttling ratio inconsistency can be fixed by using a single timer even avoiding the conditional timer re-arming.  Could you provide more details about the use of a single timer ?
> 
> 	C: I feel like in this case it will sleep too much running into a problem similar to the one solved by 90bb0c0; under heavy throttling more than one work item may be scheduled.

Right.  So I feel like we need a solution that will avoid this problem
but at the same time keep the proper accuracy of the throttling.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH] migration: Improve accuracy of vCPU throttling with per-vCPU timers

[Cosmin Marin]

On 19/06/2019, 02:35, "Peter Xu" <peterx@redhat.com> wrote:

    On Tue, Jun 18, 2019 at 04:52:09PM +0000, Cosmin Marin wrote:
    > 
    > 
    > On 18/06/2019, 15:51, "Peter Xu" <peterx@redhat.com> wrote:
    > 
    >     On Tue, Jun 18, 2019 at 12:25:43PM +0000, Cosmin Marin wrote:
    >     > 	Hi Peter,
    >     > 
    >     > 	thanks for reviewing the patch. Indeed, I agree that it's almost impossible to determine which solution it's better from the scalability perspective. However, I feel that using per-vCPU timers is the only way for ensuring correctness of the throttling ratio.
    >     
    >     The thing is that your patch actually contains two changes:
    >     
    >     1. use N timers instead of one.
    >     
    >     2. remove throttle_thread_scheduled check, so we do the throttle
    >        always
    >     
    >     Here what I'm worried is that _maybe_ the 2nd item is the one that
    >     really helped.
    >     
    > 	C: The removal of *throttle_thread_scheduled* is a consequence of the per-vCPU model only. In this model, each of the vCPUs schedules work just for itself (as part of the timer's firing callback) - there's no global point of control - therefore, the variable isn't helpful for scheduling anymore.
    > 
    >     Note that there is a side effect that we might queue more than one
    >     work on one specific cpu if we queue it too fast, but it does not
    >     block us from trying it out to identify which item (1 or 2 or both)
    >     really helped here.  Then if we think that (queuing too much) is an
    >     issue then we can discuss on how to fix it since current patch will
    >     have this problem as well.
    >     
    > 	C: I believe that in the per-vCPU timer implementation we cannot queue more than one piece of work because, here, the vCPU queues work for itself and that happens only when the timer fires - so, the two "states" - scheduling and sleeping - are mutually exclusive running from the same thread context. 
    
    I think this is the place where I'm in question with - I don't think
    they are using the same context.  IMO the timer will always be run in
    the main thread no matter you use per-cpu timer or not, however the
    sleeping part should be run on per-cpu.
    
    A simple way to verify it would be: break at cpu_throttle_timer_tick()
    to see which thread it is running in.
    
	You're absolutely right, it was indeed a confusion I made (I've run a test in which I printed the thread IDs to confirm as well). However, I believe that there are two contributing factors preventing the scheduling of more than one piece of work: 
		- the timer's firing period is always greater than the vCPU's sleeping interval, therefore the timer won't fire while a vCPU is sleeping and as a consequence no additional work is scheduled (as long as the start of the sleeping time does not "move to the right" towards the next firing of the timer)
		- the timer's callback schedules work for one vCPU only (simple & fast) preventing additional delays between work executions on different vCPUs or potential overlapping of timer firing with vCPU sleeps 

    >     > 
    >     > 	It's a bit unclear to me how the throttling ratio inconsistency can be fixed by using a single timer even avoiding the conditional timer re-arming.  Could you provide more details about the use of a single timer ?
    > 
    > 	C: I feel like in this case it will sleep too much running into a problem similar to the one solved by 90bb0c0; under heavy throttling more than one work item may be scheduled.
    
    Right.  So I feel like we need a solution that will avoid this problem
    but at the same time keep the proper accuracy of the throttling.
    
	IMO the patch achieves both goals without putting too much pressure on the main thread when running the callbacks; we could see no problem related to the main thread/callbacks in any of the tests we ran.

    Thanks,
    
    -- 
    Peter Xu
    
Thanks,
Cosmin

Re: [Qemu-devel] [PATCH] migration: Improve accuracy of vCPU throttling with per-vCPU timers

[Peter Xu]

On Wed, Jun 19, 2019 at 03:23:28PM +0000, Cosmin Marin wrote:
> 
> 
> On 19/06/2019, 02:35, "Peter Xu" <peterx@redhat.com> wrote:
> 
>     On Tue, Jun 18, 2019 at 04:52:09PM +0000, Cosmin Marin wrote:
>     > 
>     > 
>     > On 18/06/2019, 15:51, "Peter Xu" <peterx@redhat.com> wrote:
>     > 
>     >     On Tue, Jun 18, 2019 at 12:25:43PM +0000, Cosmin Marin wrote:
>     >     > 	Hi Peter,
>     >     > 
>     >     > 	thanks for reviewing the patch. Indeed, I agree that it's almost impossible to determine which solution it's better from the scalability perspective. However, I feel that using per-vCPU timers is the only way for ensuring correctness of the throttling ratio.
>     >     
>     >     The thing is that your patch actually contains two changes:
>     >     
>     >     1. use N timers instead of one.
>     >     
>     >     2. remove throttle_thread_scheduled check, so we do the throttle
>     >        always
>     >     
>     >     Here what I'm worried is that _maybe_ the 2nd item is the one that
>     >     really helped.
>     >     
>     > 	C: The removal of *throttle_thread_scheduled* is a consequence of the per-vCPU model only. In this model, each of the vCPUs schedules work just for itself (as part of the timer's firing callback) - there's no global point of control - therefore, the variable isn't helpful for scheduling anymore.
>     > 
>     >     Note that there is a side effect that we might queue more than one
>     >     work on one specific cpu if we queue it too fast, but it does not
>     >     block us from trying it out to identify which item (1 or 2 or both)
>     >     really helped here.  Then if we think that (queuing too much) is an
>     >     issue then we can discuss on how to fix it since current patch will
>     >     have this problem as well.
>     >     
中央党史>     > 	C: I believe that in the per-vCPU timer implementation we cannot queue more than one piece of work because, here, the vCPU queues work for itself and that happens only when the timer fires - so, the two "states" - scheduling and sleeping - are mutually exclusive running from the same thread context. 
>     
>     I think this is the place where I'm in question with - I don't think
>     they are using the same context.  IMO the timer will always be run in
>     the main thread no matter you use per-cpu timer or not, however the
>     sleeping part should be run on per-cpu.
>     
>     A simple way to verify it would be: break at cpu_throttle_timer_tick()
>     to see which thread it is running in.
>     
> 	You're absolutely right, it was indeed a confusion I made (I've run a test in which I printed the thread IDs to confirm as well). However, I believe that there are two contributing factors preventing the scheduling of more than one piece of work: 
> 		- the timer's firing period is always greater than the vCPU's sleeping interval, therefore the timer won't fire while a vCPU is sleeping and as a consequence no additional work is scheduled (as long as the start of the sleeping time does not "move to the right" towards the next firing of the timer)

I suspect the timer could still fire during vcpu sleeping.  The old
code have had that problem from the very beginning and that's why we
have had the throttle_thread_scheduled, AFAICT.  Meanwhile I cannot
see why per-cpu timer could help to avoid this.

The problem is async_run_on_cpu() will only queue the work onto the
CPU, but it never guarantees that when the work will be scheduled on
the CPU.  The delay should be unpredictable.

> 		- the timer's callback schedules work for one vCPU only (simple & fast) preventing additional delays between work executions on different vCPUs or potential overlapping of timer firing with vCPU sleeps 

Splitting the single timer into per-cpu timers doesn't help at all IMO
because you'll need to call async_run_on_cpu() as many times as
before.  Although you'll be with different timer contexts, but you are
still with the _same_ main thread context for all these timers so you
should even need more time to schedule these timers in total.  With
that, it seems to me that it's even more overhead and it could bring
more delays comparing to the old code rather than helping anything.

If you can schedule these timers on separate threads, then I would
agree. But I don't see how it could happen easily.

> 
>     >     > 
>     >     > 	It's a bit unclear to me how the throttling ratio inconsistency can be fixed by using a single timer even avoiding the conditional timer re-arming.  Could you provide more details about the use of a single timer ?
>     > 
>     > 	C: I feel like in this case it will sleep too much running into a problem similar to the one solved by 90bb0c0; under heavy throttling more than one work item may be scheduled.
>     
>     Right.  So I feel like we need a solution that will avoid this problem
>     but at the same time keep the proper accuracy of the throttling.
>     
> 	IMO the patch achieves both goals without putting too much pressure on the main thread when running the callbacks; we could see no problem related to the main thread/callbacks in any of the tests we ran.

Yeah I would fully believe that it won't bring problem to main thread
if we're with tens of vcpus.  I'm just a bit worried that we might
move the code even a bit further towards scalable by using split
timers.

Regards,

-- 
Peter Xu