[Qemu-devel] [RFC PATCH 0/2] High downtime with 95+ throttle pct

[Qemu-devel] [RFC PATCH 0/2] High downtime with 95+ throttle pct

[Yury Kotov]

Hi,

I wrote a test for migration auto converge and found out a strange thing:
1. Enable auto converge
2. Set max-bandwidth 1Gb/s
3. Set downtime-limit 1ms
4. Run standard test (just writes a byte per page)
5. Wait for converge
6. It's converged with 99% throttle percentage
7. The result downtime was about 300-600ms   <<<<

It's much higher than expected 1ms. I figured out that cpu_throttle_thread()
function sleeps for 100ms+ for high throttle percentage (>=95%) in VCPU thread.
And it sleeps even after a cpu kick.

I tried to fix it by using timedwait for ms part of sleep.
E.g timedwait(halt_cond, 1ms) + usleep(500).

But I'm not sure about using timedwait function here with qemu_global_mutex.
The original function uses qemu_mutex_unlock_iothread + qemu_mutex_lock_iothread
It differs from locking/unlocking (inside timedwait) qemu_global_mutex
because of using qemu_bql_mutex_lock_func function which could be anything.
This is why the series is RFC.

What do you think?
Thanks!

Yury Kotov (2):
  qemu-thread: Add qemu_cond_timedwait
  cpus: Fix throttling during vm_stop

 cpus.c                   | 27 +++++++++++++++++++--------
 include/qemu/thread.h    | 12 ++++++++++++
 util/qemu-thread-posix.c | 40 ++++++++++++++++++++++++++++------------
 util/qemu-thread-win32.c | 16 ++++++++++++++++
 4 files changed, 75 insertions(+), 20 deletions(-)

-- 
2.22.0

[Qemu-devel] [RFC PATCH 1/2] qemu-thread: Add qemu_cond_timedwait

[Yury Kotov]

Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
---
 include/qemu/thread.h    | 12 ++++++++++++
 util/qemu-thread-posix.c | 40 ++++++++++++++++++++++++++++------------
 util/qemu-thread-win32.c | 16 ++++++++++++++++
 3 files changed, 56 insertions(+), 12 deletions(-)

diff --git a/include/qemu/thread.h b/include/qemu/thread.h
index 55d83a907c..80898c5db4 100644
--- a/include/qemu/thread.h
+++ b/include/qemu/thread.h
@@ -140,6 +140,18 @@ static inline void (qemu_cond_wait)(QemuCond *cond, QemuMutex *mutex)
     qemu_cond_wait(cond, mutex);
 }
 
+void qemu_cond_timedwait_impl(QemuCond *cond, QemuMutex *mutex, int ms,
+                              const char *file, const int line);
+
+#define qemu_cond_timedwait(cond, mutex, ms) \
+        qemu_cond_timedwait_impl(cond, mutex, ms, __FILE__, __LINE__)
+
+static inline void (qemu_cond_timedwait)(QemuCond *cond, QemuMutex *mutex,
+                                         int ms)
+{
+    qemu_cond_timedwait(cond, mutex, ms);
+}
+
 void qemu_sem_init(QemuSemaphore *sem, int init);
 void qemu_sem_post(QemuSemaphore *sem);
 void qemu_sem_wait(QemuSemaphore *sem);
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index 1bf5e65dea..eed777d9ec 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -36,6 +36,18 @@ static void error_exit(int err, const char *msg)
     abort();
 }
 
+static void compute_abs_deadline(struct timespec *ts, int ms)
+{
+    struct timeval tv;
+    gettimeofday(&tv, NULL);
+    ts->tv_nsec = tv.tv_usec * 1000 + (ms % 1000) * 1000000;
+    ts->tv_sec = tv.tv_sec + ms / 1000;
+    if (ts->tv_nsec >= 1000000000) {
+        ts->tv_sec++;
+        ts->tv_nsec -= 1000000000;
+    }
+}
+
 void qemu_mutex_init(QemuMutex *mutex)
 {
     int err;
@@ -164,6 +176,22 @@ void qemu_cond_wait_impl(QemuCond *cond, QemuMutex *mutex, const char *file, con
         error_exit(err, __func__);
 }
 
+void qemu_cond_timedwait_impl(QemuCond *cond, QemuMutex *mutex, int ms,
+                              const char *file, const int line)
+{
+    int err;
+    struct timespec ts;
+
+    assert(cond->initialized);
+    trace_qemu_mutex_unlock(mutex, file, line);
+    compute_abs_deadline(&ts, ms);
+    err = pthread_cond_timedwait(&cond->cond, &mutex->lock, &ts);
+    trace_qemu_mutex_locked(mutex, file, line);
+    if (err && err != ETIMEDOUT) {
+        error_exit(err, __func__);
+    }
+}
+
 void qemu_sem_init(QemuSemaphore *sem, int init)
 {
     int rc;
@@ -238,18 +266,6 @@ void qemu_sem_post(QemuSemaphore *sem)
 #endif
 }
 
-static void compute_abs_deadline(struct timespec *ts, int ms)
-{
-    struct timeval tv;
-    gettimeofday(&tv, NULL);
-    ts->tv_nsec = tv.tv_usec * 1000 + (ms % 1000) * 1000000;
-    ts->tv_sec = tv.tv_sec + ms / 1000;
-    if (ts->tv_nsec >= 1000000000) {
-        ts->tv_sec++;
-        ts->tv_nsec -= 1000000000;
-    }
-}
-
 int qemu_sem_timedwait(QemuSemaphore *sem, int ms)
 {
     int rc;
diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index 572f88535d..5faa01cb61 100644
--- a/util/qemu-thread-win32.c
+++ b/util/qemu-thread-win32.c
@@ -145,6 +145,22 @@ void qemu_cond_wait_impl(QemuCond *cond, QemuMutex *mutex, const char *file, con
     qemu_mutex_post_lock(mutex, file, line);
 }
 
+void qemu_cond_timedwait_impl(QemuCond *cond, QemuMutex *mutex, int ms,
+                              const char *file, const int line)
+{
+    int rc = 0;
+
+    assert(cond->initialized);
+    trace_qemu_mutex_unlock(mutex, file, line);
+    if (!SleepConditionVariableSRW(&cond->var, &mutex->lock, ms, 0)) {
+        rc = GetLastError();
+    }
+    trace_qemu_mutex_locked(mutex, file, line);
+    if (rc && rc != ERROR_TIMEOUT) {
+        error_exit(rc, __func__);
+    }
+}
+
 void qemu_sem_init(QemuSemaphore *sem, int init)
 {
     /* Manual reset.  */
-- 
2.22.0

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

[Yury Kotov]

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

Re: [Qemu-devel] [RFC PATCH 0/2] High downtime with 95+ throttle pct

[Dr. David Alan Gilbert]

* Yury Kotov (yury-kotov@yandex-team.ru) wrote:
> Hi,
> 
> I wrote a test for migration auto converge and found out a strange thing:
> 1. Enable auto converge
> 2. Set max-bandwidth 1Gb/s
> 3. Set downtime-limit 1ms
> 4. Run standard test (just writes a byte per page)
> 5. Wait for converge
> 6. It's converged with 99% throttle percentage
> 7. The result downtime was about 300-600ms   <<<<
> 
> It's much higher than expected 1ms. I figured out that cpu_throttle_thread()
> function sleeps for 100ms+ for high throttle percentage (>=95%) in VCPU thread.
> And it sleeps even after a cpu kick.
> 
> I tried to fix it by using timedwait for ms part of sleep.
> E.g timedwait(halt_cond, 1ms) + usleep(500).
> 
> But I'm not sure about using timedwait function here with qemu_global_mutex.
> The original function uses qemu_mutex_unlock_iothread + qemu_mutex_lock_iothread
> It differs from locking/unlocking (inside timedwait) qemu_global_mutex
> because of using qemu_bql_mutex_lock_func function which could be anything.
> This is why the series is RFC.
> 
> What do you think?

Would qemu_sem_timedwait work for your use?  I use it in
migration_thread for the bandwidth limiting and allowing that to be
woken up.

Dave

> Thanks!
> 
> Yury Kotov (2):
>   qemu-thread: Add qemu_cond_timedwait
>   cpus: Fix throttling during vm_stop
> 
>  cpus.c                   | 27 +++++++++++++++++++--------
>  include/qemu/thread.h    | 12 ++++++++++++
>  util/qemu-thread-posix.c | 40 ++++++++++++++++++++++++++++------------
>  util/qemu-thread-win32.c | 16 ++++++++++++++++
>  4 files changed, 75 insertions(+), 20 deletions(-)
> 
> -- 
> 2.22.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC PATCH 0/2] High downtime with 95+ throttle pct

[Yury Kotov]

10.07.2019, 12:57, "Dr. David Alan Gilbert" <dgilbert@redhat.com>:
> * Yury Kotov (yury-kotov@yandex-team.ru) wrote:
>>  Hi,
>>
>>  I wrote a test for migration auto converge and found out a strange thing:
>>  1. Enable auto converge
>>  2. Set max-bandwidth 1Gb/s
>>  3. Set downtime-limit 1ms
>>  4. Run standard test (just writes a byte per page)
>>  5. Wait for converge
>>  6. It's converged with 99% throttle percentage
>>  7. The result downtime was about 300-600ms <<<<
>>
>>  It's much higher than expected 1ms. I figured out that cpu_throttle_thread()
>>  function sleeps for 100ms+ for high throttle percentage (>=95%) in VCPU thread.
>>  And it sleeps even after a cpu kick.
>>
>>  I tried to fix it by using timedwait for ms part of sleep.
>>  E.g timedwait(halt_cond, 1ms) + usleep(500).
>>
>>  But I'm not sure about using timedwait function here with qemu_global_mutex.
>>  The original function uses qemu_mutex_unlock_iothread + qemu_mutex_lock_iothread
>>  It differs from locking/unlocking (inside timedwait) qemu_global_mutex
>>  because of using qemu_bql_mutex_lock_func function which could be anything.
>>  This is why the series is RFC.
>>
>>  What do you think?
>
> Would qemu_sem_timedwait work for your use? I use it in
> migration_thread for the bandwidth limiting and allowing that to be
> woken up.

It's a good idea and it should work. But it's more complicated than reusing
halt_cond. I see that it's ok to use qemu_cond_wait in cpus.c so I hope it's
ok to use qemu_cond_timedwait too. But if it isn't then using qemu_sem_timedwait
is a good fallback I think.

Regards,
Yury

>
> Dave
>
>>  Thanks!
>>
>>  Yury Kotov (2):
>>    qemu-thread: Add qemu_cond_timedwait
>>    cpus: Fix throttling during vm_stop
>>
>>   cpus.c | 27 +++++++++++++++++++--------
>>   include/qemu/thread.h | 12 ++++++++++++
>>   util/qemu-thread-posix.c | 40 ++++++++++++++++++++++++++++------------
>>   util/qemu-thread-win32.c | 16 ++++++++++++++++
>>   4 files changed, 75 insertions(+), 20 deletions(-)
>>
>>  --
>>  2.22.0
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Yury Kotov]

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.

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

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

[Paolo Bonzini]

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
> 

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

[Yury Kotov]

15.07.2019, 14:00, "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.
>

Sorry, perhaps I was not accurate enough.

To fix the bug I changed the logic of cpu_throttle_thread() function.
Before this function called qemu_mutex_(un)lock_iothread which encapsulates
work with qemu_global_mutex.

Now, this calls qemu_cond_timedwait(..., &qemu_global_mutex, ...) which also
unlocks/locks qemu_global_mutex. But, in theory, behavior of
qemu_mutex_(un)lock_iothread may differ from simple locking/unlocking of
qemu_global_mutex.

So, I'm not sure is such change is ok or not.

> 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

Regards,
Yury

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

[Paolo Bonzini]

On 15/07/19 14:36, Yury Kotov wrote:
> Sorry, perhaps I was not accurate enough.
> 
> To fix the bug I changed the logic of cpu_throttle_thread() function.
> Before this function called qemu_mutex_(un)lock_iothread which encapsulates
> work with qemu_global_mutex.
> 
> Now, this calls qemu_cond_timedwait(..., &qemu_global_mutex, ...) which also
> unlocks/locks qemu_global_mutex. But, in theory, behavior of
> qemu_mutex_(un)lock_iothread may differ from simple locking/unlocking of
> qemu_global_mutex.
> 
> So, I'm not sure is such change is ok or not.

Ah, I see.  No, it's okay.  The only difference is setting/clearing
iothread_locked which doesn't matter here.

Paolo