Re: [Qemu-devel] [PATCH v1 7/8] throttle: Add throttle group support

[Fam Zheng <famz@redhat.com>]

On Tue, 10/07 15:24, Benoît Canet wrote:
> The throttle group support use a cooperative round robin scheduling algorithm.
> 
> The principle of the algorithm are simple:

s/principle/principles/

> - Each BDS of the group is used as a token in a circular way.
> - The active BDS compute if a wait must be done and arm the right timer.
> - If a wait must be done the token timer will be armed so the token will become
>   the next active BDS.
> 
> Signed-off-by: Benoit Canet <benoit.canet@nodalink.com>
> ---
>  block.c                   | 191 ++++++++++++++++++++++++++++++++++++++++------
>  block/qapi.c              |   7 +-
>  block/throttle-groups.c   |   2 +-
>  blockdev.c                |  19 ++++-
>  hmp.c                     |   4 +-
>  include/block/block.h     |   3 +-
>  include/block/block_int.h |   9 ++-
>  qapi/block-core.json      |   5 +-
>  qemu-options.hx           |   1 +
>  qmp-commands.hx           |   3 +-
>  10 files changed, 209 insertions(+), 35 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 527ea48..e7e5607 100644
> --- a/block.c
> +++ b/block.c
> @@ -36,6 +36,7 @@
>  #include "qmp-commands.h"
>  #include "qemu/timer.h"
>  #include "qapi-event.h"
> +#include "block/throttle-groups.h"
>  
>  #ifdef CONFIG_BSD
>  #include <sys/types.h>
> @@ -129,7 +130,9 @@ void bdrv_set_io_limits(BlockDriverState *bs,
>  {
>      int i;
>  
> -    throttle_config(&bs->throttle_state, &bs->throttle_timers, cfg);
> +    throttle_group_lock(bs->throttle_state);
> +    throttle_config(bs->throttle_state, &bs->throttle_timers, cfg);
> +    throttle_group_unlock(bs->throttle_state);
>  
>      for (i = 0; i < 2; i++) {
>          qemu_co_enter_next(&bs->throttled_reqs[i]);
> @@ -156,34 +159,99 @@ static bool bdrv_start_throttled_reqs(BlockDriverState *bs)
>      return drained;
>  }
>  
> +static void bdrv_throttle_group_add(BlockDriverState *bs)
> +{
> +    int i;
> +    BlockDriverState *token;
> +
> +    for (i = 0; i < 2; i++) {
> +        /* Get the BlockDriverState having the round robin token */
> +        token = throttle_group_token(bs->throttle_state, i);
> +
> +        /* If the ThrottleGroup is new set the current BlockDriverState as
> +         * token
> +         */
> +        if (!token) {
> +            throttle_group_set_token(bs->throttle_state, bs, i);
> +        }
> +
> +    }
> +
> +    throttle_group_register_bs(bs->throttle_state, bs);
> +}
> +
> +static void bdrv_throttle_group_remove(BlockDriverState *bs)
> +{
> +    BlockDriverState *token;
> +    int i;
> +
> +    for (i = 0; i < 2; i++) {
> +        /* Get the BlockDriverState having the round robin token */
> +        token = throttle_group_token(bs->throttle_state, i);
> +        /* if this bs is the current token set the next bs as token */
> +        if (token == bs) {
> +            token = throttle_group_next_bs(token);
> +            /* take care of the case where bs is the only bs of the group */
> +            if (token == bs) {
> +                token = NULL;
> +            }
> +            throttle_group_set_token(bs->throttle_state, token, i);
> +        }
> +    }
> +
> +    /* remove the current bs from the list */
> +    QLIST_REMOVE(bs, round_robin);
> +}
> +
>  void bdrv_io_limits_disable(BlockDriverState *bs)
>  {
> +
> +    throttle_group_lock(bs->throttle_state);
>      bs->io_limits_enabled = false;
> +    throttle_group_unlock(bs->throttle_state);
>  
>      bdrv_start_throttled_reqs(bs);
>  
> +    throttle_group_lock(bs->throttle_state);
> +    bdrv_throttle_group_remove(bs);
> +    throttle_group_unlock(bs->throttle_state);
> +
> +    throttle_group_unref(bs->throttle_state);
> +    bs->throttle_state = NULL;
> +
>      throttle_timers_destroy(&bs->throttle_timers);
>  }
>  
>  static void bdrv_throttle_read_timer_cb(void *opaque)
>  {
>      BlockDriverState *bs = opaque;
> -    throttle_timer_fired(&bs->throttle_state, false);
> +
> +    throttle_group_lock(bs->throttle_state);
> +    throttle_timer_fired(bs->throttle_state, false);
> +    throttle_group_unlock(bs->throttle_state);
> +
>      qemu_co_enter_next(&bs->throttled_reqs[0]);
>  }
>  
>  static void bdrv_throttle_write_timer_cb(void *opaque)
>  {
>      BlockDriverState *bs = opaque;
> -    throttle_timer_fired(&bs->throttle_state, true);
> +
> +    throttle_group_lock(bs->throttle_state);
> +    throttle_timer_fired(bs->throttle_state, true);
> +    throttle_group_unlock(bs->throttle_state);
> +
>      qemu_co_enter_next(&bs->throttled_reqs[1]);
>  }
>  
>  /* should be called before bdrv_set_io_limits if a limit is set */
> -void bdrv_io_limits_enable(BlockDriverState *bs)
> +void bdrv_io_limits_enable(BlockDriverState *bs, const char *group)

Does this mean that after this series, all the throttle_states must be
contained inside its own throttle group? If so, we could embed ThrottleGroup
fields in ThrottleState.

It's weird when a function called throttle_group_compare takes a parameter of
ThrottleState pointer, and cast it back to ThrottleGroup with container_of.

Fam