Re: [Qemu-devel] [PATCH 08/13] throttle: Add support for burst periods

[Kevin Wolf <kwolf@redhat.com>]

Am 05.02.2016 um 11:59 hat Alberto Garcia geschrieben:
> This patch adds support for burst periods to the throttling code.
> With this feature the user can keep performing bursts as defined by
> the LeakyBucket.max rate for a configurable period of time.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  include/qemu/throttle.h | 41 +++++++++++++++++++++++----
>  util/throttle.c         | 73 ++++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 96 insertions(+), 18 deletions(-)
> 
> diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
> index 8ec8951..63df690 100644
> --- a/include/qemu/throttle.h
> +++ b/include/qemu/throttle.h
> @@ -2,7 +2,7 @@
>   * QEMU throttling infrastructure
>   *
>   * Copyright (C) Nodalink, EURL. 2013-2014
> - * Copyright (C) Igalia, S.L. 2015
> + * Copyright (C) Igalia, S.L. 2015-2016
>   *
>   * Authors:
>   *   Benoît Canet <benoit.canet@nodalink.com>
> @@ -42,16 +42,47 @@ typedef enum {
>  } BucketType;
>  
>  /*
> - * The max parameter of the leaky bucket throttling algorithm can be used to
> - * allow the guest to do bursts.
> - * The max value is a pool of I/O that the guest can use without being throttled
> - * at all. Throttling is triggered once this pool is empty.
> + * This module implements I/O limits using the leaky bucket
> + * algorithm. The code is independent of the I/O units, but it is
> + * currently used for bytes per second and operations per second.
> + *
> + * Three parameters can be set by the user:
> + *
> + * - avg: the desired I/O limits in units per second.
> + * - max: the limit during bursts, also in units per second.
> + * - burst_length: the maximum length of the burst period, in seconds.
> + *
> + * Here's how it works:
> + *
> + * - The bucket level (number of performed I/O units) is kept in
> + *   bkt.level and leaks at a rate of bkt.avg units per second.
> + *
> + * - The size of the bucket is bkt.max * bkt.burst_length. Once the
> + *   bucket is full no more I/O is performed until the bucket leaks
> + *   again. This is what makes the I/O rate bkt.avg.
> + *
> + * - The bkt.avg rate does not apply until the bucket is full,
> + *   allowing the user to do bursts until then. The I/O limit during
> + *   bursts is bkt.max. To enforce this limit we keep an additional
> + *   bucket in bkt.burst_length that leaks at a rate of bkt.max units
> + *   per second.
> + *
> + * - Because of all of the above, the user can perform I/O at a
> + *   maximum of bkt.max units per second for at most bkt.burst_length
> + *   seconds in a row. After that the bucket will be full and the I/O
> + *   rate will go down to bkt.avg.
> + *
> + * - Since the bucket always leaks at a rate of bkt.avg, this also
> + *   determines how much the user needs to wait before being able to
> + *   do bursts again.
>   */

Good summary, thanks!

>  typedef struct LeakyBucket {
>      double  avg;              /* average goal in units per second */
>      double  max;              /* leaky bucket max burst in units */
>      double  level;            /* bucket level in units */
> +    double  burst_level;      /* bucket level in units (for computing bursts) */
> +    unsigned burst_length;    /* max length of the burst period, in seconds */
>  } LeakyBucket;
>  
>  /* The following structure is used to configure a ThrottleState
> diff --git a/util/throttle.c b/util/throttle.c
> index 6a01cee..371c769 100644
> --- a/util/throttle.c
> +++ b/util/throttle.c
> @@ -41,6 +41,14 @@ void throttle_leak_bucket(LeakyBucket *bkt, int64_t delta_ns)
>  
>      /* make the bucket leak */
>      bkt->level = MAX(bkt->level - leak, 0);
> +
> +    /* if we allow bursts for more than one second we also need to
> +     * keep track of bkt->burst_level so the bkt->max goal per second
> +     * is attained */
> +    if (bkt->burst_length > 1) {
> +        leak = (bkt->max * (double) delta_ns) / NANOSECONDS_PER_SECOND;
> +        bkt->burst_level = MAX(bkt->burst_level - leak, 0);
> +    }
>  }
>  
>  /* Calculate the time delta since last leak and make proportionals leaks
> @@ -91,13 +99,24 @@ int64_t throttle_compute_wait(LeakyBucket *bkt)
>          return 0;
>      }
>  
> -    extra = bkt->level - bkt->max;
> +    /* If the bucket is full then we have to wait */
> +    extra = bkt->level - bkt->max * bkt->burst_length;
> +    if (extra > 0) {
> +        return throttle_do_compute_wait(bkt->avg, extra);
> +    }
>  
> -    if (extra <= 0) {
> -        return 0;
> +    /* If the bucket is not full yet we have to make sure that we
> +     * fulfill the goal of bkt->max units per second. */
> +    if (bkt->burst_length > 1) {
> +        /* We use 1/10 of the max value to smooth the throttling.
> +         * See throttle_fix_bucket() for more details. */
> +        extra = bkt->burst_level - bkt->max / 10;

I don't understand the connection between throttle_fix_bucket() and
this.

throttle_fix_bucket() seems to set a default rate for bursts, which kind
of makes sense to me (but what's the point when this is lower than the
average rate?)

Here we work on a bkt->max that is either supplied by the user and
should therefore be respected, or the default in throttle_fix_bucket()
has already been applied.

What this line does is letting the request wait for more than would be
strictly necessary, or in other words, for the last second before the
bucket runs full, you only allow a tenth of the actual maximum rate.

I understand that having any burst at all helps, so the default that
throttle_fix_bucket() sets used to make sense. I'm not so sure that it
still makes sense with its max < avg setting (max used to be additional
units on top of avg, now it's measured on its own). For the divison by
10 here, however, I'm still puzzled.

What am I missing?

> +        if (extra > 0) {
> +            return throttle_do_compute_wait(bkt->max, extra);
> +        }
>      }
>  
> -    return throttle_do_compute_wait(bkt->avg, extra);
> +    return 0;
>  }

Kevin