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

[Fam Zheng <famz@redhat.com>]

On Fri, 03/13 12:09, Paolo Bonzini wrote:
> 
> 
> 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?

Even with that, I still don't understand why block jobs should stop making
progress together with VCPUs.

IMO following host clock is the right things to do, because in the IO throttle
context, we are mostly refering to host resources (host_BW=host_IO/host_time).

> 
> >> 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. :)

I totally misread :)

Fam