15.07.2019, 14:00, "Paolo Bonzini" <pbonz...@redhat.com>:
> On 15/07/19 11:40, Yury Kotov wrote:
>> Hi,
>>
>> 10.07.2019, 12:26, "Yury Kotov" <yury-ko...@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-ko...@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
