From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49274) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WkakL-0005EU-RH for qemu-devel@nongnu.org; Wed, 14 May 2014 11:05:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WkakG-00021Z-37 for qemu-devel@nongnu.org; Wed, 14 May 2014 11:05:29 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:35714 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WkakF-00021M-QM for qemu-devel@nongnu.org; Wed, 14 May 2014 11:05:24 -0400 Date: Wed, 14 May 2014 17:05:58 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140514150558.GA5955@irqsave.net> References: <1400077367-23409-1-git-send-email-stefanha@redhat.com> <1400077367-23409-2-git-send-email-stefanha@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1400077367-23409-2-git-send-email-stefanha@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/3] throttle: add throttle_detach/attach_aio_context() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , =?iso-8859-1?Q?Beno=EEt?= Canet , qemu-devel@nongnu.org The Wednesday 14 May 2014 =E0 16:22:45 (+0200), Stefan Hajnoczi wrote : > Block I/O throttling uses timers and currently always adds them to the > main loop. Throttling will break if bdrv_set_aio_context() is used to > move a BlockDriverState to a different AioContext. >=20 > This patch adds throttle_detach/attach_aio_context() interfaces so the s/so the/to the/ ?=20 > throttling timers and uses them to move timers to the new AioContext. > Note that bdrv_set_aio_context() already drains all requests so we're > sure no throttled requests are pending. >=20 > The test cases need to be updated since the throttle_init() interface > has changed. >=20 > Cc: Beno=EEt Canet > Signed-off-by: Stefan Hajnoczi > --- > block.c | 7 +++++++ > include/qemu/throttle.h | 10 ++++++++++ > tests/test-throttle.c | 25 ++++++++++++++++++++----- > util/throttle.c | 27 +++++++++++++++++++++++---- > 4 files changed, 60 insertions(+), 9 deletions(-) >=20 > diff --git a/block.c b/block.c > index 189fc0d..b30bcd5 100644 > --- a/block.c > +++ b/block.c > @@ -179,6 +179,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, > bdrv_throttle_read_timer_cb, > bdrv_throttle_write_timer_cb, > @@ -5532,6 +5533,9 @@ void bdrv_detach_aio_context(BlockDriverState *bs= ) > return; > } > =20 > + if (bs->io_limits_enabled) { > + throttle_detach_aio_context(&bs->throttle_state); > + } > if (bs->drv->bdrv_detach_aio_context) { > bs->drv->bdrv_detach_aio_context(bs); > } > @@ -5563,6 +5567,9 @@ void bdrv_attach_aio_context(BlockDriverState *bs= , > if (bs->drv->bdrv_attach_aio_context) { > bs->drv->bdrv_attach_aio_context(bs, new_context); > } > + if (bs->io_limits_enabled) { > + throttle_attach_aio_context(&bs->throttle_state, new_context); > + } > } > =20 > void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_contex= t) > diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h > index ab29b0b..b890613 100644 > --- a/include/qemu/throttle.h > +++ b/include/qemu/throttle.h > @@ -67,6 +67,11 @@ typedef struct ThrottleState { Can we make sure this header knows about AioContext in case there is another throttle user not block related ? > int64_t previous_leak; /* timestamp of the last leak done */ > QEMUTimer * timers[2]; /* timers used to do the throttling */ > QEMUClockType clock_type; /* the clock used */ > + > + /* Callbacks */ > + QEMUTimerCB *read_timer_cb; > + QEMUTimerCB *write_timer_cb; > + void *timer_opaque; > } ThrottleState; > =20 > /* operations on single leaky buckets */ > @@ -82,6 +87,7 @@ bool throttle_compute_timer(ThrottleState *ts, > =20 > /* init/destroy cycle */ > void throttle_init(ThrottleState *ts, > + AioContext *aio_context, > QEMUClockType clock_type, > void (read_timer)(void *), > void (write_timer)(void *), > @@ -89,6 +95,10 @@ void throttle_init(ThrottleState *ts, > =20 > void throttle_destroy(ThrottleState *ts); > =20 > +void throttle_detach_aio_context(ThrottleState *ts); > + > +void throttle_attach_aio_context(ThrottleState *ts, AioContext *new_co= ntext); > + > bool throttle_have_timer(ThrottleState *ts); > =20 > /* configuration */ > diff --git a/tests/test-throttle.c b/tests/test-throttle.c > index 1d4ffd3..5fa5000 100644 > --- a/tests/test-throttle.c > +++ b/tests/test-throttle.c > @@ -12,8 +12,10 @@ > =20 > #include > #include > +#include "block/aio.h" And remove this one eventually. > #include "qemu/throttle.h" > =20 > +AioContext *ctx; > LeakyBucket bkt; > ThrottleConfig cfg; > ThrottleState ts; > @@ -104,7 +106,8 @@ static void test_init(void) > memset(&ts, 1, sizeof(ts)); > =20 > /* init the structure */ > - throttle_init(&ts, QEMU_CLOCK_VIRTUAL, read_timer_cb, write_timer_= cb, &ts); > + throttle_init(&ts, ctx, QEMU_CLOCK_VIRTUAL, > + read_timer_cb, write_timer_cb, &ts); > =20 > /* check initialized fields */ > g_assert(ts.clock_type =3D=3D QEMU_CLOCK_VIRTUAL); > @@ -126,7 +129,8 @@ static void test_init(void) > static void test_destroy(void) > { > int i; > - throttle_init(&ts, QEMU_CLOCK_VIRTUAL, read_timer_cb, write_timer_= cb, &ts); > + throttle_init(&ts, ctx, QEMU_CLOCK_VIRTUAL, > + read_timer_cb, write_timer_cb, &ts); > throttle_destroy(&ts); > for (i =3D 0; i < 2; i++) { > g_assert(!ts.timers[i]); > @@ -165,7 +169,8 @@ static void test_config_functions(void) > =20 > orig_cfg.op_size =3D 1; > =20 > - throttle_init(&ts, QEMU_CLOCK_VIRTUAL, read_timer_cb, write_timer_= cb, &ts); > + throttle_init(&ts, ctx, QEMU_CLOCK_VIRTUAL, > + read_timer_cb, write_timer_cb, &ts); > /* structure reset by throttle_init previous_leak should be null *= / > g_assert(!ts.previous_leak); > throttle_config(&ts, &orig_cfg); > @@ -324,7 +329,8 @@ static void test_have_timer(void) > g_assert(!throttle_have_timer(&ts)); > =20 > /* init the structure */ > - throttle_init(&ts, QEMU_CLOCK_VIRTUAL, read_timer_cb, write_timer_= cb, &ts); > + throttle_init(&ts, ctx, QEMU_CLOCK_VIRTUAL, > + read_timer_cb, write_timer_cb, &ts); > =20 > /* timer set by init should return true */ > g_assert(throttle_have_timer(&ts)); > @@ -357,7 +363,8 @@ static bool do_test_accounting(bool is_ops, /* are = we testing bps or ops */ > =20 > cfg.op_size =3D op_size; > =20 > - throttle_init(&ts, QEMU_CLOCK_VIRTUAL, read_timer_cb, write_timer_= cb, &ts); > + throttle_init(&ts, ctx, QEMU_CLOCK_VIRTUAL, > + read_timer_cb, write_timer_cb, &ts); > throttle_config(&ts, &cfg); > =20 > /* account a read */ > @@ -461,7 +468,15 @@ static void test_accounting(void) > =20 > int main(int argc, char **argv) > { > + GSource *src; > + > init_clocks(); > + > + ctx =3D aio_context_new(); > + src =3D aio_get_g_source(ctx); > + g_source_attach(src, NULL); > + g_source_unref(src); > + > do {} while (g_main_context_iteration(NULL, false)); > =20 > /* tests in the same order as the header function declarations */ > diff --git a/util/throttle.c b/util/throttle.c > index 02e6f15..f976ac7 100644 > --- a/util/throttle.c > +++ b/util/throttle.c > @@ -22,6 +22,7 @@ > =20 > #include "qemu/throttle.h" > #include "qemu/timer.h" > +#include "block/aio.h" > =20 > /* This function make a bucket leak > * > @@ -157,8 +158,18 @@ bool throttle_compute_timer(ThrottleState *ts, > return false; > } > =20 > +/* Add timers to event loop */ > +void throttle_attach_aio_context(ThrottleState *ts, AioContext *new_co= ntext) > +{ > + ts->timers[0] =3D aio_timer_new(new_context, ts->clock_type, SCALE= _NS, > + ts->read_timer_cb, ts->timer_opaque)= ; > + ts->timers[1] =3D aio_timer_new(new_context, ts->clock_type, SCALE= _NS, > + ts->write_timer_cb, ts->timer_opaque= ); > +} > + > /* To be called first on the ThrottleState */ > void throttle_init(ThrottleState *ts, > + AioContext *aio_context, > QEMUClockType clock_type, > QEMUTimerCB *read_timer_cb, > QEMUTimerCB *write_timer_cb, > @@ -167,8 +178,10 @@ void throttle_init(ThrottleState *ts, > memset(ts, 0, sizeof(ThrottleState)); > =20 > ts->clock_type =3D clock_type; > - ts->timers[0] =3D timer_new_ns(clock_type, read_timer_cb, timer_op= aque); > - ts->timers[1] =3D timer_new_ns(clock_type, write_timer_cb, timer_o= paque); > + ts->read_timer_cb =3D read_timer_cb; > + ts->write_timer_cb =3D write_timer_cb; > + ts->timer_opaque =3D timer_opaque; > + throttle_attach_aio_context(ts, aio_context); > } > =20 > /* destroy a timer */ > @@ -181,8 +194,8 @@ static void throttle_timer_destroy(QEMUTimer **time= r) > *timer =3D NULL; > } > =20 > -/* To be called last on the ThrottleState */ > -void throttle_destroy(ThrottleState *ts) > +/* Remove timers from event loop */ > +void throttle_detach_aio_context(ThrottleState *ts) > { > int i; > =20 > @@ -191,6 +204,12 @@ void throttle_destroy(ThrottleState *ts) > } > } > =20 > +/* To be called last on the ThrottleState */ > +void throttle_destroy(ThrottleState *ts) > +{ > + throttle_detach_aio_context(ts); > +} > + > /* is any throttling timer configured */ > bool throttle_have_timer(ThrottleState *ts) > { > --=20 > 1.9.0 >=20