From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60389) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YWKeb-0006rQ-2v for qemu-devel@nongnu.org; Fri, 13 Mar 2015 04:09:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YWKeV-00050B-Vz for qemu-devel@nongnu.org; Fri, 13 Mar 2015 04:09:09 -0400 Received: from mail-wg0-x22d.google.com ([2a00:1450:400c:c00::22d]:34186) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YWKeV-000503-LW for qemu-devel@nongnu.org; Fri, 13 Mar 2015 04:09:03 -0400 Received: by wgha1 with SMTP id a1so21608292wgh.1 for ; Fri, 13 Mar 2015 01:09:03 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <55029B1A.6000802@redhat.com> Date: Fri, 13 Mar 2015 09:08:58 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1426228529-15969-1-git-send-email-famz@redhat.com> In-Reply-To: <1426228529-15969-1-git-send-email-famz@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] block/throttle: Use host clock type List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel 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 > --- > 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); > >