On Fri, Feb 13, 2015 at 06:06:15PM +0200, Alberto Garcia wrote:
> The throttle group support use a cooperative round robin scheduling algorithm.
>
> The principles of the algorithm are simple:
> - 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: Alberto Garcia <be...@igalia.com>
> ---
> block.c | 212
> +++++++++++++++++++++++++++++++++++++++-------
> 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, 220 insertions(+), 45 deletions(-)
>
> diff --git a/block.c b/block.c
> index 642ef04..625f1c8 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>
> @@ -130,7 +131,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]);
> @@ -157,34 +160,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);
> + }
> +
> + }
Does it make sense to move this inside throttle_group_register_bs()?
Then bdrv_throttle_group_add() could be dropped and
throttle_group_register_bs() is called directly.
> +
> + throttle_group_register_bs(bs->throttle_state, bs);
> +}
> +
> +static void bdrv_throttle_group_remove(BlockDriverState *bs)
The name is a hint that this doesn't belong in block.c. This function
should be throttle_group_unregister_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);
What are the locking rules? I don't understand why throttle_state must
be locked to modify bs->io_limits_enabled.
>
> 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);
This pattern suggests throttle_timer_fired() should acquire the lock
internally instead.
pgpt3QoZSgrQk.pgp
Description: PGP signature
