Re: [Qemu-devel] [RFC PATCH 2/2] cpus: Fix throttling during vm_stop

[Yury Kotov]

Hi,

10.07.2019, 12:26, "Yury Kotov" <address@hidden>:
> Throttling thread sleeps in VCPU thread. For high throttle percentage
> this sleep is more than 10ms. E.g. for 60% - 15ms, for 99% - 990ms.
> vm_stop() kicks all VCPUs and waits for them. It's called at the end of
> migration and because of the long sleep the migration downtime might be
> more than 100ms even for downtime-limit 1ms.
> Use qemu_cond_timedwait for high percentage to wake up during vm_stop.
>
> Signed-off-by: Yury Kotov <address@hidden>
> ---
>  cpus.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index ffc57119ca..3c069cdc33 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -74,6 +74,8 @@
>
>  #endif /* CONFIG_LINUX */
>
> +static QemuMutex qemu_global_mutex;
> +
>  int64_t max_delay;
>  int64_t max_advance;
>
> @@ -776,7 +778,7 @@ static void cpu_throttle_thread(CPUState *cpu, 
> run_on_cpu_data opaque)
>  {
>      double pct;
>      double throttle_ratio;
> - long sleeptime_ns;
> + int64_t sleeptime_ns;
>
>      if (!cpu_throttle_get_percentage()) {
>          return;
> @@ -784,11 +786,22 @@ 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);
> -
> - qemu_mutex_unlock_iothread();
> - g_usleep(sleeptime_ns / 1000); /* Convert ns to us for usleep call */
> - qemu_mutex_lock_iothread();
> + /* Add 1ns to fix double's rounding error (like 0.9999999...) */
> + sleeptime_ns = (int64_t)(throttle_ratio * CPU_THROTTLE_TIMESLICE_NS + 1);
> +
> + while (sleeptime_ns >= SCALE_MS && !cpu->stop) {
> + int64_t start, end;
> + start = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> + qemu_cond_timedwait(cpu->halt_cond, &qemu_global_mutex,

Paolo, Richard, please tell me what you think.
I'm not sure is it correct to use qemu_cond_timedwait() here?
I see that qemu_cond_timedwait()/qemu_cond_wait() and
qemu_mutex_(un)lock_iothread() have a different behavior in some cases.
But there are some similar using of qemu_cond_wait with halt_cond, so may be
it's ok to use qemu_cond_timedwait() here too.

> + sleeptime_ns / SCALE_MS);
> + end = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> + sleeptime_ns -= end - start;
> + }
> + if (sleeptime_ns >= SCALE_US && !cpu->stop) {
> + qemu_mutex_unlock_iothread();
> + g_usleep(sleeptime_ns / SCALE_US);
> + qemu_mutex_lock_iothread();
> + }
>      atomic_set(&cpu->throttle_thread_scheduled, 0);
>  }
>
> @@ -1166,8 +1179,6 @@ static void qemu_init_sigbus(void)
>  }
>  #endif /* !CONFIG_LINUX */
>
> -static QemuMutex qemu_global_mutex;
> -
>  static QemuThread io_thread;
>
>  /* cpu creation */
> --
> 2.22.0

Regards,
Yury