Re: [Qemu-devel] [PATCH] block/throttle: Use host clock type

[Paolo Bonzini <pbonzini@redhat.com>]

On 13/03/2015 09:27, Fam Zheng wrote:
> On Fri, 03/13 09:08, Paolo Bonzini wrote:
>>
>>
>> On 13/03/2015 07:35, Fam Zheng wrote:
>>> Throttle timers won't make any progress when VCPU is not running, which
>>> is prone to stall the request queue in cases like utils, qtest,
>>> suspending, and live migration, unless carefully handled. What we do now
>>> is crude. For example in bdrv_drain_all, requests are resumed
>>> immediately without consulting throttling timer. Unfortunately
>>> bdrv_drain_all is so widely used that there may be too many holes that
>>> guest could bypass throttling.
>>>
>>> If we use the host clock, we can just trust the nested poll when waiting
>>> for requests.
>>>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> ---
>>>  block.c               |  2 +-
>>>  tests/test-throttle.c | 14 +++++++-------
>>
>> I think test-throttle.c should use the vm_clock.  At some point it was
>> managing the clock manually (by overriding cpu_get_clock from
>> libqemustub.a), and that's only possible with QEMU_CLOCK_VIRTUAL.
> 
> Ah! That is in iotests 093 (hint: authord by Fam Zheng :-/), which WILL be
> complicated if block.c switches away from QEMU_CLOCK_VIRTUAL. But I'll do the
> work if we decide to make this change.
> 
> As to tests/test-throttle.c, I don't see its dependency on clock type, so
> either way should work and I don't mind keeping it as-is at all.

If there's another way to do the same thing, I'd prefer it.

For example, can we call bdrv_drain_all() at the beginning of
do_vm_stop, before pausing the VCPUs?

>> As to block.c, I'll leave the review to the block folks.  But I think
>> QEMU_CLOCK_REALTIME is preferrable.
> 
> Real time clock should be fine, but we should review that the code handles
> clock reversing.

QEMU_CLOCK_HOST is the one that follows the wall clock;
QEMU_CLOCK_REALTIME is monotonic. :)

Paolo