From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54703) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xbk9U-0002xT-7y for qemu-devel@nongnu.org; Wed, 08 Oct 2014 01:51:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xbk9P-0004t9-1e for qemu-devel@nongnu.org; Wed, 08 Oct 2014 01:51:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49987) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xbk9O-0004sy-QD for qemu-devel@nongnu.org; Wed, 08 Oct 2014 01:51:02 -0400 Date: Wed, 8 Oct 2014 13:51:04 +0800 From: Fam Zheng Message-ID: <20141008055104.GA15829@fam-t430.nay.redhat.com> References: <1412688279-8312-1-git-send-email-benoit.canet@nodalink.com> <1412688279-8312-2-git-send-email-benoit.canet@nodalink.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1412688279-8312-2-git-send-email-benoit.canet@nodalink.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v1 1/8] throttle: Extract timers from ThrottleState into a separate ThrottleTimers structure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?Beno=EEt?= Canet Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com On Tue, 10/07 15:24, Beno=EEt Canet wrote: > Group throttling will share ThrottleState between multiple bs. > As a consequence the ThrottleState will be accessed by multiple aio con= text. >=20 > Timers are tied to their aio context so they must go out of the Throttl= eState structure. >=20 > This commit pave the way for each bs of a common ThrottleState to have = it's own s/pave/paves/ And a few trivial comments below. Otherwise looks good. > timer. >=20 > Signed-off-by: Benoit Canet > --- > block.c | 35 ++++++++++++-------- > include/block/block_int.h | 1 + > include/qemu/throttle.h | 36 +++++++++++++-------- > tests/test-throttle.c | 82 ++++++++++++++++++++++++++-------------= -------- > util/throttle.c | 73 ++++++++++++++++++++++++---------------= -- > 5 files changed, 134 insertions(+), 93 deletions(-) >=20 > diff --git a/block.c b/block.c > index d3aebeb..f209f55 100644 > --- a/block.c > +++ b/block.c > @@ -129,7 +129,7 @@ void bdrv_set_io_limits(BlockDriverState *bs, > { > int i; > =20 > - throttle_config(&bs->throttle_state, cfg); > + throttle_config(&bs->throttle_state, &bs->throttle_timers, cfg); > =20 > for (i =3D 0; i < 2; i++) { > qemu_co_enter_next(&bs->throttled_reqs[i]); > @@ -162,7 +162,7 @@ void bdrv_io_limits_disable(BlockDriverState *bs) > =20 > bdrv_start_throttled_reqs(bs); > =20 > - throttle_destroy(&bs->throttle_state); > + throttle_timers_destroy(&bs->throttle_timers); > } > =20 > static void bdrv_throttle_read_timer_cb(void *opaque) > @@ -181,12 +181,13 @@ static void bdrv_throttle_write_timer_cb(void *op= aque) > 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, > - bs); > + throttle_init(&bs->throttle_state); > + throttle_timers_init(&bs->throttle_timers, > + bdrv_get_aio_context(bs), > + QEMU_CLOCK_VIRTUAL, > + bdrv_throttle_read_timer_cb, > + bdrv_throttle_write_timer_cb, > + bs); > bs->io_limits_enabled =3D true; > } > =20 > @@ -200,7 +201,9 @@ static void bdrv_io_limits_intercept(BlockDriverSta= te *bs, > bool is_write) > { > /* does this io must wait */ > - bool must_wait =3D throttle_schedule_timer(&bs->throttle_state, is= _write); > + bool must_wait =3D throttle_schedule_timer(&bs->throttle_state, > + &bs->throttle_timers, > + is_write); > =20 > /* if must wait or any request of this type throttled queue the IO= */ > if (must_wait || > @@ -213,7 +216,8 @@ static void bdrv_io_limits_intercept(BlockDriverSta= te *bs, > =20 > =20 > /* if the next request must wait -> do nothing */ > - if (throttle_schedule_timer(&bs->throttle_state, is_write)) { > + if (throttle_schedule_timer(&bs->throttle_state, &bs->throttle_tim= ers, > + is_write)) { > return; > } > =20 > @@ -1990,6 +1994,9 @@ static void bdrv_move_feature_fields(BlockDriverS= tate *bs_dest, > memcpy(&bs_dest->throttle_state, > &bs_src->throttle_state, > sizeof(ThrottleState)); > + memcpy(&bs_dest->throttle_timers, > + &bs_src->throttle_timers, > + sizeof(ThrottleTimers)); > bs_dest->throttled_reqs[0] =3D bs_src->throttled_reqs[0]; > bs_dest->throttled_reqs[1] =3D bs_src->throttled_reqs[1]; > bs_dest->io_limits_enabled =3D bs_src->io_limits_enabled; > @@ -2052,7 +2059,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDri= verState *bs_old) > assert(bs_new->job =3D=3D NULL); > assert(bs_new->dev =3D=3D NULL); > assert(bs_new->io_limits_enabled =3D=3D false); > - assert(!throttle_have_timer(&bs_new->throttle_state)); > + assert(!throttle_timers_are_init(&bs_new->throttle_timers)); > =20 > tmp =3D *bs_new; > *bs_new =3D *bs_old; > @@ -2070,7 +2077,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDri= verState *bs_old) > assert(bs_new->dev =3D=3D NULL); > assert(bs_new->job =3D=3D NULL); > assert(bs_new->io_limits_enabled =3D=3D false); > - assert(!throttle_have_timer(&bs_new->throttle_state)); > + assert(!throttle_timers_are_init(&bs_new->throttle_timers)); > =20 > /* insert the nodes back into the graph node list if needed */ > if (bs_new->node_name[0] !=3D '\0') { > @@ -5746,7 +5753,7 @@ void bdrv_detach_aio_context(BlockDriverState *bs= ) > } > =20 > if (bs->io_limits_enabled) { > - throttle_detach_aio_context(&bs->throttle_state); > + throttle_timers_detach_aio_context(&bs->throttle_timers); > } > if (bs->drv->bdrv_detach_aio_context) { > bs->drv->bdrv_detach_aio_context(bs); > @@ -5782,7 +5789,7 @@ void bdrv_attach_aio_context(BlockDriverState *bs= , > bs->drv->bdrv_attach_aio_context(bs, new_context); > } > if (bs->io_limits_enabled) { > - throttle_attach_aio_context(&bs->throttle_state, new_context); > + throttle_timers_attach_aio_context(&bs->throttle_timers, new_c= ontext); > } > =20 > QLIST_FOREACH(ban, &bs->aio_notifiers, list) { > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 8d86a6c..7af126f 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -356,6 +356,7 @@ struct BlockDriverState { > =20 > /* I/O throttling */ > ThrottleState throttle_state; > + ThrottleTimers throttle_timers;=20 > CoQueue throttled_reqs[2]; > bool io_limits_enabled; > =20 > diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h > index b890613..b89d4d8 100644 > --- a/include/qemu/throttle.h > +++ b/include/qemu/throttle.h > @@ -65,6 +65,9 @@ typedef struct ThrottleConfig { > typedef struct ThrottleState { > ThrottleConfig cfg; /* configuration */ > int64_t previous_leak; /* timestamp of the last leak done */ > +} ThrottleState; > + > +typedef struct ThrottleTimers { > QEMUTimer * timers[2]; /* timers used to do the throttling */ Since you are touching this structure, maybe also squash in: - QEMUTimer * timers[2]; /* timers used to do the throttling */ + QEMUTimer *timers[2]; /* timers used to do the throttling */ > QEMUClockType clock_type; /* the clock used */ > =20 > @@ -72,7 +75,7 @@ typedef struct ThrottleState { > QEMUTimerCB *read_timer_cb; > QEMUTimerCB *write_timer_cb; > void *timer_opaque; > -} ThrottleState; > +} ThrottleTimers; > =20 > /* operations on single leaky buckets */ > void throttle_leak_bucket(LeakyBucket *bkt, int64_t delta); > @@ -86,20 +89,23 @@ bool throttle_compute_timer(ThrottleState *ts, > int64_t *next_timestamp); > =20 > /* init/destroy cycle */ > -void throttle_init(ThrottleState *ts, > - AioContext *aio_context, > - QEMUClockType clock_type, > - void (read_timer)(void *), > - void (write_timer)(void *), > - void *timer_opaque); > +void throttle_init(ThrottleState *ts); > + > +void throttle_timers_init(ThrottleTimers *tt, > + AioContext *aio_context, > + QEMUClockType clock_type, > + QEMUTimerCB *read_timer_cb, > + QEMUTimerCB *write_timer_cb, > + void *timer_opaque); > =20 > -void throttle_destroy(ThrottleState *ts); > +void throttle_timers_destroy(ThrottleTimers *tt); > =20 > -void throttle_detach_aio_context(ThrottleState *ts); > +void throttle_timers_detach_aio_context(ThrottleTimers *tt); > =20 > -void throttle_attach_aio_context(ThrottleState *ts, AioContext *new_co= ntext); > +void throttle_timers_attach_aio_context(ThrottleTimers *tt, > + AioContext *new_context); > =20 > -bool throttle_have_timer(ThrottleState *ts); > +bool throttle_timers_are_init(ThrottleTimers *tt); Suggest s/_are_init/_initialized/ > =20 > /* configuration */ > bool throttle_enabled(ThrottleConfig *cfg); > @@ -108,12 +114,16 @@ bool throttle_conflicting(ThrottleConfig *cfg); > =20 > bool throttle_is_valid(ThrottleConfig *cfg); > =20 > -void throttle_config(ThrottleState *ts, ThrottleConfig *cfg); > +void throttle_config(ThrottleState *ts, > + ThrottleTimers *tt, > + ThrottleConfig *cfg); > =20 > void throttle_get_config(ThrottleState *ts, ThrottleConfig *cfg); > =20 > /* usage */ > -bool throttle_schedule_timer(ThrottleState *ts, bool is_write); > +bool throttle_schedule_timer(ThrottleState *ts, > + ThrottleTimers *tt, > + bool is_write); > =20 > void throttle_account(ThrottleState *ts, bool is_write, uint64_t size)= ; > =20 Thanks, Fam