[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V5 1/5] throttle: Add a new throttling API imple
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH V5 1/5] throttle: Add a new throttling API implementing continuous leaky bucket. |
Date: |
Fri, 16 Aug 2013 13:45:05 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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.
- [Qemu-devel] [PATCH V5 0/5] Continuous Leaky Bucket Throttling, Benoît Canet, 2013/08/12
- [Qemu-devel] [PATCH V5 1/5] throttle: Add a new t hrottling API implementing continuous leaky bucket., Benoît Canet, 2013/08/12
- [Qemu-devel] [PATCH V5 2/5] throttle: Add units t ests, Benoît Canet, 2013/08/12
- [Qemu-devel] [PATCH V5 3/5] block: Enable the new throttling code in the block layer., Benoît Canet, 2013/08/12
- [Qemu-devel] [PATCH V5 4/5] block: Add support fo r throttling burst max in QMP and the command line., Benoît Canet, 2013/08/12
- [Qemu-devel] [PATCH V5 5/5] block: Add iops_sec tor_count to do the iops accounting for a given io siz e., Benoît Canet, 2013/08/12