Re: [Qemu-devel] [PATCH V5 1/5] throttle: Add a new throttling API implementing continuous leaky bucket.

[Stefan Hajnoczi <stefanha@gmail.com>]

On Mon, Aug 12, 2013 at 06:53:12PM +0200, Benoît Canet wrote:
> +#ifndef THROTTLING_H
> +#define THROTTLING_H

THROTTLE_H

> +
> +#include <stdint.h>
> +#include "qemu-common.h"
> +#include "qemu/timer.h"
> +
> +#define NANOSECONDS_PER_SECOND  1000000000.0
> +
> +#define BUCKETS_COUNT 6
> +
> +typedef enum {
> +    THROTTLE_BPS_TOTAL = 0,
> +    THROTTLE_BPS_READ  = 1,
> +    THROTTLE_BPS_WRITE = 2,
> +    THROTTLE_OPS_TOTAL = 3,
> +    THROTTLE_OPS_READ  = 4,
> +    THROTTLE_OPS_WRITE = 5,
> +} BucketType;
> +
> +typedef struct LeakyBucket {
> +    double  ups;            /* units per second */
> +    double  max;            /* leaky bucket max in units */
> +    double  bucket;         /* bucket in units */

These comments aren't very clear to me :).  So I guess bps or iops would
be in ups.  Max would be the total budget or maximum burst.  Bucket
might be the current level.

> +} LeakyBucket;
> +
> +/* The following structure is used to configure a ThrottleState
> + * It contains a bit of state: the bucket field of the LeakyBucket structure.
> + * However it allows to keep the code clean and the bucket field is reset to
> + * zero at the right time.
> + */
> +typedef struct ThrottleConfig {
> +    LeakyBucket buckets[6]; /* leaky buckets */

s/6/THROTTLE_TYPE_MAX/

> +    uint64_t unit_size;     /* size of an unit in bytes */
> +    uint64_t op_size;       /* size of an operation in units */

It's not clear yet why we need both unit_size *and* op_size.  I thought
you would have a single granularity field for accounting big requests as
multiple iops.

> +/* This function make a bucket leak
> + *
> + * @bkt:   the bucket to make leak
> + * @delta: the time delta

delta is in nanoseconds.  Probably best to call it delta_ns.

> +/* destroy a timer */
> +static void throttle_timer_destroy(QEMUTimer **timer)
> +{
> +    assert(*timer != NULL);
> +
> +    if (qemu_timer_pending(*timer)) {
> +        qemu_del_timer(*timer);
> +    }

You can always call qemu_del_timer(), the timer doesn't need to be pending.

> +/* fix bucket parameters */
> +static void throttle_fix_bucket(LeakyBucket *bkt)
> +{
> +    double min = bkt->ups / 10;
> +    /* zero bucket level */
> +    bkt->bucket = 0;
> +
> +    /* take care of not using cpu and also improve throttling precision */
> +    if (bkt->ups &&
> +        bkt->max < min) {
> +        bkt->max = min;
> +    }
> +}

This function seems like magic.  What is really going on here?  Why
divide by 10 and when does this case happen?

> +
> +/* take care of canceling a timer */
> +static void throttle_cancel_timer(QEMUTimer *timer)
> +{
> +    assert(timer != NULL);
> +    if (!qemu_timer_pending(timer)) {
> +        return;
> +    }

No need to check pending first.