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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 16/08/2013 13:45, Stefan Hajnoczi ha scritto:
>> > +#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;

Please remove the "= N" from the enums, and add BUCKETS_COUNT here.

>> > +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.

I also suggest replacing "ups" with "avg", since it's the average
throughput that the leaky bucket allows after the initial burst has
emptied the bucket.

>> +    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.

IIUC the ops buckets account operations in op_size / unit_size units,
while the bps buckets account operations in 1 / unit_size units, or
something like that.  But it needs clarification indeed.

Paolo