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

[Paolo Bonzini <pbonzini@redhat.com>]

On 15/07/19 11:40, Yury Kotov wrote:
> Hi,
> 
> 10.07.2019, 12:26, "Yury Kotov" <yury-kotov@yandex-team.ru>:
>> 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 <yury-kotov@yandex-team.ru>
>> ---
>>  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.

Back in the day, Windows didn't have condition variables and making the
implementation robust and efficient was a mess---so there was no
qemu_cond_timedwait.  Semapshores are also a wee bit more scalable, so
qemu_sem_timedwait was introduced.

Now, I don't think it's an issue to add qemu_cond_timedwait.

Thanks,

Paolo

> 
>> + 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
>