On Tue, 10/07 15:24, Benoît Canet wrote:
> Group throttling will share ThrottleState between multiple bs.
> As a consequence the ThrottleState will be accessed by multiple aio context.
>
> Timers are tied to their aio context so they must go out of the ThrottleState
> structure.
>
> 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.
>
> Signed-off-by: Benoit Canet <benoit.ca...@nodalink.com>
> ---
> 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(-)
>
> 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;
>
> - throttle_config(&bs->throttle_state, cfg);
> + throttle_config(&bs->throttle_state, &bs->throttle_timers, cfg);
>
> for (i = 0; i < 2; i++) {
> qemu_co_enter_next(&bs->throttled_reqs[i]);
> @@ -162,7 +162,7 @@ void bdrv_io_limits_disable(BlockDriverState *bs)
>
> bdrv_start_throttled_reqs(bs);
>
> - throttle_destroy(&bs->throttle_state);
> + throttle_timers_destroy(&bs->throttle_timers);
> }
>
> static void bdrv_throttle_read_timer_cb(void *opaque)
> @@ -181,12 +181,13 @@ static void bdrv_throttle_write_timer_cb(void *opaque)
> 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 = true;
> }
>
> @@ -200,7 +201,9 @@ static void bdrv_io_limits_intercept(BlockDriverState *bs,
> bool is_write)
> {
> /* does this io must wait */
> - bool must_wait = throttle_schedule_timer(&bs->throttle_state, is_write);
> + bool must_wait = throttle_schedule_timer(&bs->throttle_state,
> + &bs->throttle_timers,
> + is_write);
>
> /* 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(BlockDriverState *bs,
>
>
> /* 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_timers,
> + is_write)) {
> return;
> }
>
> @@ -1990,6 +1994,9 @@ static void bdrv_move_feature_fields(BlockDriverState
> *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] = bs_src->throttled_reqs[0];
> bs_dest->throttled_reqs[1] = bs_src->throttled_reqs[1];
> bs_dest->io_limits_enabled = bs_src->io_limits_enabled;
> @@ -2052,7 +2059,7 @@ void bdrv_swap(BlockDriverState *bs_new,
> BlockDriverState *bs_old)
> assert(bs_new->job == NULL);
> assert(bs_new->dev == NULL);
> assert(bs_new->io_limits_enabled == false);
> - assert(!throttle_have_timer(&bs_new->throttle_state));
> + assert(!throttle_timers_are_init(&bs_new->throttle_timers));
>
> tmp = *bs_new;
> *bs_new = *bs_old;
> @@ -2070,7 +2077,7 @@ void bdrv_swap(BlockDriverState *bs_new,
> BlockDriverState *bs_old)
> assert(bs_new->dev == NULL);
> assert(bs_new->job == NULL);
> assert(bs_new->io_limits_enabled == false);
> - assert(!throttle_have_timer(&bs_new->throttle_state));
> + assert(!throttle_timers_are_init(&bs_new->throttle_timers));
>
> /* insert the nodes back into the graph node list if needed */
> if (bs_new->node_name[0] != '\0') {
> @@ -5746,7 +5753,7 @@ void bdrv_detach_aio_context(BlockDriverState *bs)
> }
>
> 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_context);
> }
>
> 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 {
>
> /* I/O throttling */
> ThrottleState throttle_state;
> + ThrottleTimers throttle_timers;
> CoQueue throttled_reqs[2];
> bool io_limits_enabled;
>
> 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 */
>
> @@ -72,7 +75,7 @@ typedef struct ThrottleState {
> QEMUTimerCB *read_timer_cb;
> QEMUTimerCB *write_timer_cb;
> void *timer_opaque;
> -} ThrottleState;
> +} ThrottleTimers;
>
> /* 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);
>
> /* 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);
>
> -void throttle_destroy(ThrottleState *ts);
> +void throttle_timers_destroy(ThrottleTimers *tt);
>
> -void throttle_detach_aio_context(ThrottleState *ts);
> +void throttle_timers_detach_aio_context(ThrottleTimers *tt);
>
> -void throttle_attach_aio_context(ThrottleState *ts, AioContext *new_context);
> +void throttle_timers_attach_aio_context(ThrottleTimers *tt,
> + AioContext *new_context);
>
> -bool throttle_have_timer(ThrottleState *ts);
> +bool throttle_timers_are_init(ThrottleTimers *tt);
Suggest s/_are_init/_initialized/
>
> /* configuration */
> bool throttle_enabled(ThrottleConfig *cfg);
> @@ -108,12 +114,16 @@ bool throttle_conflicting(ThrottleConfig *cfg);
>
> bool throttle_is_valid(ThrottleConfig *cfg);
>
> -void throttle_config(ThrottleState *ts, ThrottleConfig *cfg);
> +void throttle_config(ThrottleState *ts,
> + ThrottleTimers *tt,
> + ThrottleConfig *cfg);
>
> void throttle_get_config(ThrottleState *ts, ThrottleConfig *cfg);
>
> /* usage */
> -bool throttle_schedule_timer(ThrottleState *ts, bool is_write);
> +bool throttle_schedule_timer(ThrottleState *ts,
> + ThrottleTimers *tt,
> + bool is_write);
>
> void throttle_account(ThrottleState *ts, bool is_write, uint64_t size);
>
<snip>
Thanks,
Fam
