Re: [Qemu-devel] [PATCH 5/5] cpu: throttle: fix throttle time slice

[Peter Xu]

On Mon, Mar 27, 2017 at 03:21:28PM +0800, Peter Xu wrote:
> IIUC the throttle idea is that: we split a CPU_THROTTLE_TIMESLICE_NS
> time slice into two parts - one for vcpu, one for throttle thread (which
> will suspend the thread by a sleep). However current algorithm on
> calculating the working piece and sleeping piece is strange.
> 
> Assume a 99% throttle, what we want is to merely stop vcpu from running,
> but the old logic will just first let the vcpu run for a very long
> time (which is "CPU_THROTTLE_TIMESLICE_NS / (1-pct)" = 1 second) before
> doing anything else.
> 
> Fixing it up to the simplest but imho accurate logic.

Oh, looks like I need to switch the two pct below... :)

> 
> CC: Paolo Bonzini <address@hidden>
> CC: Richard Henderson <address@hidden>
> CC: Jason J. Herne <address@hidden>
> Signed-off-by: Peter Xu <address@hidden>
> ---
>  cpus.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 167d961..7976ce4 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -633,7 +633,6 @@ static const VMStateDescription vmstate_timers = {
>  static void cpu_throttle_thread(CPUState *cpu, run_on_cpu_data opaque)
>  {
>      double pct;
> -    double throttle_ratio;
>      long sleeptime_ns;
>  
>      if (!cpu_throttle_get_percentage()) {
> @@ -641,8 +640,7 @@ static void cpu_throttle_thread(CPUState *cpu, 
> run_on_cpu_data opaque)
>      }
>  
>      pct = (double)cpu_throttle_get_percentage()/100;
> -    throttle_ratio = pct / (1 - pct);
> -    sleeptime_ns = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE_NS);
> +    sleeptime_ns = (long)((1 - pct) * CPU_THROTTLE_TIMESLICE_NS);
                             ^^^^^^^^^
                             here it should be "pct", while...

>  
>      qemu_mutex_unlock_iothread();
>      atomic_set(&cpu->throttle_thread_scheduled, 0);
> @@ -668,7 +666,7 @@ static void cpu_throttle_timer_tick(void *opaque)
>  
>      pct = (double)cpu_throttle_get_percentage()/100;
>      timer_mod(throttle_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT) +
> -                                   CPU_THROTTLE_TIMESLICE_NS / (1-pct));
> +                                   CPU_THROTTLE_TIMESLICE_NS * pct);
                                                                  ^^^
                                                here it should be (1 - pct)

I'll wait for review comment on the raw idea, to see whether I will
need a repost. Sorry for the misunderstanding.

-- peterx