Re: [Qemu-devel] [PATCH v1 1/8] throttle: Extract timers from ThrottleState into a separate ThrottleTimers structure

[Fam Zheng]

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 <address@hidden>
> ---
>  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