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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

As to block.c, I'll leave the review to the block folks.  But I think
QEMU_CLOCK_REALTIME is preferrable.

>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 191a847..11f9065 100644
> --- a/block.c
> +++ b/block.c
> @@ -184,7 +184,7 @@ void bdrv_io_limits_enable(BlockDriverState *bs)
>      assert(!bs->io_limits_enabled);
>      throttle_init(&bs->throttle_state,
>                    bdrv_get_aio_context(bs),
> -                  QEMU_CLOCK_VIRTUAL,
> +                  QEMU_CLOCK_HOST,
>                    bdrv_throttle_read_timer_cb,
>                    bdrv_throttle_write_timer_cb,
>                    bs);
> diff --git a/tests/test-throttle.c b/tests/test-throttle.c
> index d8ba415..1fb1792 100644
> --- a/tests/test-throttle.c
> +++ b/tests/test-throttle.c
> @@ -107,11 +107,11 @@ static void test_init(void)
>      memset(&ts, 1, sizeof(ts));
>  
>      /* init the structure */
> -    throttle_init(&ts, ctx, QEMU_CLOCK_VIRTUAL,
> +    throttle_init(&ts, ctx, QEMU_CLOCK_HOST,
>                    read_timer_cb, write_timer_cb, &ts);
>  
>      /* check initialized fields */
> -    g_assert(ts.clock_type == QEMU_CLOCK_VIRTUAL);
> +    g_assert(ts.clock_type == QEMU_CLOCK_HOST);
>      g_assert(ts.timers[0]);
>      g_assert(ts.timers[1]);
>  
> @@ -130,7 +130,7 @@ static void test_init(void)
>  static void test_destroy(void)
>  {
>      int i;
> -    throttle_init(&ts, ctx, QEMU_CLOCK_VIRTUAL,
> +    throttle_init(&ts, ctx, QEMU_CLOCK_HOST,
>                    read_timer_cb, write_timer_cb, &ts);
>      throttle_destroy(&ts);
>      for (i = 0; i < 2; i++) {
> @@ -170,7 +170,7 @@ static void test_config_functions(void)
>  
>      orig_cfg.op_size = 1;
>  
> -    throttle_init(&ts, ctx, QEMU_CLOCK_VIRTUAL,
> +    throttle_init(&ts, ctx, QEMU_CLOCK_HOST,
>                    read_timer_cb, write_timer_cb, &ts);
>      /* structure reset by throttle_init previous_leak should be null */
>      g_assert(!ts.previous_leak);
> @@ -330,7 +330,7 @@ static void test_have_timer(void)
>      g_assert(!throttle_have_timer(&ts));
>  
>      /* init the structure */
> -    throttle_init(&ts, ctx, QEMU_CLOCK_VIRTUAL,
> +    throttle_init(&ts, ctx, QEMU_CLOCK_HOST,
>                    read_timer_cb, write_timer_cb, &ts);
>  
>      /* timer set by init should return true */
> @@ -345,7 +345,7 @@ static void test_detach_attach(void)
>      memset(&ts, 0, sizeof(ts));
>  
>      /* init the structure */
> -    throttle_init(&ts, ctx, QEMU_CLOCK_VIRTUAL,
> +    throttle_init(&ts, ctx, QEMU_CLOCK_HOST,
>                    read_timer_cb, write_timer_cb, &ts);
>  
>      /* timer set by init should return true */
> @@ -387,7 +387,7 @@ static bool do_test_accounting(bool is_ops, /* are we testing bps or ops */
>  
>      cfg.op_size = op_size;
>  
> -    throttle_init(&ts, ctx, QEMU_CLOCK_VIRTUAL,
> +    throttle_init(&ts, ctx, QEMU_CLOCK_HOST,
>                    read_timer_cb, write_timer_cb, &ts);
>      throttle_config(&ts, &cfg);
>  
>