Re: [Qemu-devel] [PATCH 3/4] throttle: Remove throttle_fix_bucket() / throttle_unfix_bucket()

[Manos Pitsidianakis]

On Mon, Aug 21, 2017 at 02:10:59PM +0200, Alberto Garcia wrote:

On Sat 19 Aug 2017 05:23:12 PM CEST, Manos Pitsidianakis wrote:

-    /* If the bucket is full then we have to wait */
-    extra = bkt->level - bkt->max * bkt->burst_length;
+    if (!bkt->max) {
+        /* If bkt->max is 0 we still want to allow short bursts of I/O
+         * from the guest, otherwise every other request will be throttled
+         * and performance will suffer considerably. */
+        bucket_size = bkt->avg / 10;
+        burst_bucket_size = 0;

Might be wrong, but shouldn't that be
    bucket_size = (bkt->avg / 10) * bkt->burst_length?
    burst_bucket_size = (bkt->avg / 10) / 10;

if !bkt->max then the user didn't define any bursts, and the I/O is only
throttled to the rate set in bkt->avg.

+    } else {
+        /* If we have a burst limit then we have to wait until all I/O
+         * at burst rate has finished before throttling to bkt->avg */
+        bucket_size = bkt->max * bkt->burst_length;
+        burst_bucket_size = bkt->max / 10;
+    }
+
+    /* If the main bucket is full then we have to wait */
+    extra = bkt->level - bucket_size;

because it used to be that if bkt->max is 0 then
 extra = bkt->level - (bkt->avg /10) * bkt->burst_length; //fixed value
 and now it's
 extra = bkt->level - (bkt->avg / 10);

 and

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

-    /* If the bucket is not full yet we have to make sure that we
-     * fulfill the goal of bkt->max units per second. */
+    /* If the main bucket is not full yet we still have to check the
+     * burst bucket in order to enforce the burst limit */
    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;
+        extra = bkt->burst_level - burst_bucket_size;

This used to be

    extra = bkt->burst_level - (bkt->avg / 10) / 10;
and now it's
    extra = bkt->burst_level  - 0;

No, if bkt->burst_length > 1 then bkt->max != 0, and therefore
burst_bucket_size is never 0 either.

Perhaps you missed the ! in the first "if (!bkt->max)" ?

Now it makes sense, thanks :) I reread the patch and you can add

Reviewed-by: Manos Pitsidianakis <el13...@mail.ntua.gr>

signature.asc
Description: PGP signature