[Qemu-devel] [PATCH 0/4] Misc throttle fixes

[Qemu-devel] [PATCH 0/4] Misc throttle fixes

[Alberto Garcia]

Hi all,

this series contains a few small changes to the throttling code and
its documentation.

Stefan, once this is reviewed, can you please remove the "Make
LeakyBucket.avg and LeakyBucket.max integer types" commit (id
1744e9a12cbcebff2e) from your block-next branch? This series contains
a new version of that patch that replaces the one that you have.

Regards,

Berto

Alberto Garcia (4):
  throttle: Fix wrong variable name in the header documentation
  throttle: Update the throttle_fix_bucket() documentation
  throttle: Remove throttle_fix_bucket() / throttle_unfix_bucket()
  throttle: Make LeakyBucket.avg and LeakyBucket.max integer types

 include/qemu/throttle.h |  6 ++---
 util/throttle.c         | 67 +++++++++++++++++--------------------------------
 2 files changed, 26 insertions(+), 47 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH 1/4] throttle: Fix wrong variable name in the header documentation

[Alberto Garcia]

The level of the burst bucket is stored in bkt.burst_level, not
bkt.burst_lenght.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 include/qemu/throttle.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index d056008c18..66a8ac10a4 100644
--- a/include/qemu/throttle.h
+++ b/include/qemu/throttle.h
@@ -63,7 +63,7 @@ typedef enum {
  * - 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
+ *   bucket in bkt.burst_level 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
-- 
2.11.0

[Qemu-devel] [PATCH 2/4] throttle: Update the throttle_fix_bucket() documentation

[Alberto Garcia]

The way the throttling algorithm works is that requests start being
throttled once the bucket level exceeds the burst limit. When we get
there the bucket leaks at the level set by the user (bkt->avg), and
that leak rate is what prevents guest I/O from exceeding the desired
limit.

If we don't allow bursts (i.e. bkt->max == 0) then we can start
throttling requests immediately. The problem with keeping the
threshold at 0 is that it only allows one request at a time, and as
soon as there's a bit of I/O from the guest every other request will
be throttled and performance will suffer considerably. That can even
make the guest unable to reach the throttle limit if that limit is
high enough, and that happens regardless of the block scheduler used
by the guest.

Increasing that threshold gives flexibility to the guest, allowing it
to perform short bursts of I/O before being throttled. Increasing the
threshold too much does not make a difference in the long run (because
it's the leak rate what defines the actual throughput) but it does
allow the guest to perform longer initial bursts and exceed the
throttle limit for a short while.

A burst value of bkt->avg / 10 allows the guest to perform 100ms'
worth of I/O at the target rate without being throttled.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 util/throttle.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/util/throttle.c b/util/throttle.c
index b2a52b8b34..9a6bda813c 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -366,14 +366,9 @@ static void throttle_fix_bucket(LeakyBucket *bkt)
     /* zero bucket level */
     bkt->level = bkt->burst_level = 0;
 
-    /* The following is done to cope with the Linux CFQ block scheduler
-     * which regroup reads and writes by block of 100ms in the guest.
-     * When they are two process one making reads and one making writes cfq
-     * make a pattern looking like the following:
-     * WWWWWWWWWWWRRRRRRRRRRRRRRWWWWWWWWWWWWWwRRRRRRRRRRRRRRRRR
-     * Having a max burst value of 100ms of the average will help smooth the
-     * throttling
-     */
+    /* 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. */
     min = bkt->avg / 10;
     if (bkt->avg && !bkt->max) {
         bkt->max = min;
-- 
2.11.0

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

[Alberto Garcia]

The throttling code can change internally the value of bkt->max if it
hasn't been set by the user. The problem with this is that if we want
to retrieve the original value we have to undo this change first. This
is ugly and unnecessary: this patch removes the throttle_fix_bucket()
and throttle_unfix_bucket() functions completely and moves the logic
to throttle_compute_wait().

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 util/throttle.c | 62 +++++++++++++++++++++------------------------------------
 1 file changed, 23 insertions(+), 39 deletions(-)

diff --git a/util/throttle.c b/util/throttle.c
index 9a6bda813c..1f29cf9918 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -95,23 +95,36 @@ static int64_t throttle_do_compute_wait(double limit, double extra)
 int64_t throttle_compute_wait(LeakyBucket *bkt)
 {
     double extra; /* the number of extra units blocking the io */
+    double bucket_size;   /* I/O before throttling to bkt->avg */
+    double burst_bucket_size; /* Before throttling to bkt->max */
 
     if (!bkt->avg) {
         return 0;
     }
 
-    /* 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;
+    } 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;
     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;
         if (extra > 0) {
             return throttle_do_compute_wait(bkt->max, extra);
         }
@@ -358,31 +371,6 @@ bool throttle_is_valid(ThrottleConfig *cfg, Error **errp)
     return true;
 }
 
-/* fix bucket parameters */
-static void throttle_fix_bucket(LeakyBucket *bkt)
-{
-    double min;
-
-    /* zero bucket level */
-    bkt->level = bkt->burst_level = 0;
-
-    /* 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. */
-    min = bkt->avg / 10;
-    if (bkt->avg && !bkt->max) {
-        bkt->max = min;
-    }
-}
-
-/* undo internal bucket parameter changes (see throttle_fix_bucket()) */
-static void throttle_unfix_bucket(LeakyBucket *bkt)
-{
-    if (bkt->max < bkt->avg) {
-        bkt->max = 0;
-    }
-}
-
 /* Used to configure the throttle
  *
  * @ts: the throttle state we are working on
@@ -397,8 +385,10 @@ void throttle_config(ThrottleState *ts,
 
     ts->cfg = *cfg;
 
+    /* Zero bucket level */
     for (i = 0; i < BUCKETS_COUNT; i++) {
-        throttle_fix_bucket(&ts->cfg.buckets[i]);
+        ts->cfg.buckets[i].level = 0;
+        ts->cfg.buckets[i].burst_level = 0;
     }
 
     ts->previous_leak = qemu_clock_get_ns(clock_type);
@@ -411,13 +401,7 @@ void throttle_config(ThrottleState *ts,
  */
 void throttle_get_config(ThrottleState *ts, ThrottleConfig *cfg)
 {
-    int i;
-
     *cfg = ts->cfg;
-
-    for (i = 0; i < BUCKETS_COUNT; i++) {
-        throttle_unfix_bucket(&cfg->buckets[i]);
-    }
 }
 
 
-- 
2.11.0

[Qemu-devel] [PATCH 4/4] throttle: Make LeakyBucket.avg and LeakyBucket.max integer types

[Alberto Garcia]

Both the throttling limits set with the throttling.iops-* and
throttling.bps-* options and their QMP equivalents defined in the
BlockIOThrottle struct are integer values.

Those limits are also reported in the BlockDeviceInfo struct and they
are integers there as well.

Therefore there's no reason to store them internally as double and do
the conversion everytime we're setting or querying them, so this patch
uses int64_t for those types.

LeakyBucket.level and LeakyBucket.burst_level do however remain double
because their value changes depending on the fraction of time elapsed
since the previous I/O operation.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 include/qemu/throttle.h | 4 ++--
 util/throttle.c         | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index 66a8ac10a4..c466f6ccaa 100644
--- a/include/qemu/throttle.h
+++ b/include/qemu/throttle.h
@@ -77,8 +77,8 @@ typedef enum {
  */
 
 typedef struct LeakyBucket {
-    double  avg;              /* average goal in units per second */
-    double  max;              /* leaky bucket max burst in units */
+    int64_t avg;              /* average goal in units per second */
+    int64_t 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 */
diff --git a/util/throttle.c b/util/throttle.c
index 1f29cf9918..55a2451a14 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -106,13 +106,13 @@ int64_t throttle_compute_wait(LeakyBucket *bkt)
         /* 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;
+        bucket_size = (double) bkt->avg / 10;
         burst_bucket_size = 0;
     } 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;
+        burst_bucket_size = (double) bkt->max / 10;
     }
 
     /* If the main bucket is full then we have to wait */
-- 
2.11.0

Re: [Qemu-devel] [PATCH 1/4] throttle: Fix wrong variable name in the header documentation

[Manos Pitsidianakis]

[-- Attachment #1: Type: text/plain, Size: 1057 bytes --]

On Thu, Aug 17, 2017 at 05:28:12PM +0300, Alberto Garcia wrote:
>The level of the burst bucket is stored in bkt.burst_level, not
>bkt.burst_lenght.

s/lenght/length, otherwise:

Reviewed-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
>
>Signed-off-by: Alberto Garcia <berto@igalia.com>
>---
> include/qemu/throttle.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
>index d056008c18..66a8ac10a4 100644
>--- a/include/qemu/throttle.h
>+++ b/include/qemu/throttle.h
>@@ -63,7 +63,7 @@ typedef enum {
>  * - 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
>+ *   bucket in bkt.burst_level 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
>-- 
>2.11.0
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 4/4] throttle: Make LeakyBucket.avg and LeakyBucket.max integer types

[Manos Pitsidianakis]

[-- Attachment #1: Type: text/plain, Size: 2665 bytes --]

On Thu, Aug 17, 2017 at 05:28:15PM +0300, Alberto Garcia wrote:
>Both the throttling limits set with the throttling.iops-* and
>throttling.bps-* options and their QMP equivalents defined in the
>BlockIOThrottle struct are integer values.
>
>Those limits are also reported in the BlockDeviceInfo struct and they
>are integers there as well.
>
>Therefore there's no reason to store them internally as double and do
>the conversion everytime we're setting or querying them, so this patch
>uses int64_t for those types.
>
>LeakyBucket.level and LeakyBucket.burst_level do however remain double
>because their value changes depending on the fraction of time elapsed
>since the previous I/O operation.
>
>Signed-off-by: Alberto Garcia <berto@igalia.com>

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

> include/qemu/throttle.h | 4 ++--
> util/throttle.c         | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
>index 66a8ac10a4..c466f6ccaa 100644
>--- a/include/qemu/throttle.h
>+++ b/include/qemu/throttle.h
>@@ -77,8 +77,8 @@ typedef enum {
>  */
>
> typedef struct LeakyBucket {
>-    double  avg;              /* average goal in units per second */
>-    double  max;              /* leaky bucket max burst in units */
>+    int64_t avg;              /* average goal in units per second */
>+    int64_t 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 */
>diff --git a/util/throttle.c b/util/throttle.c
>index 1f29cf9918..55a2451a14 100644
>--- a/util/throttle.c
>+++ b/util/throttle.c
>@@ -106,13 +106,13 @@ int64_t throttle_compute_wait(LeakyBucket *bkt)
>         /* 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;
>+        bucket_size = (double) bkt->avg / 10;
>         burst_bucket_size = 0;
>     } 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;
>+        burst_bucket_size = (double) bkt->max / 10;
>     }
>
>     /* If the main bucket is full then we have to wait */
>-- 
>2.11.0
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Manos Pitsidianakis]

[-- Attachment #1: Type: text/plain, Size: 4823 bytes --]

On Thu, Aug 17, 2017 at 05:28:14PM +0300, Alberto Garcia wrote:
>The throttling code can change internally the value of bkt->max if it
>hasn't been set by the user. The problem with this is that if we want
>to retrieve the original value we have to undo this change first. This
>is ugly and unnecessary: this patch removes the throttle_fix_bucket()
>and throttle_unfix_bucket() functions completely and moves the logic
>to throttle_compute_wait().
>
>Signed-off-by: Alberto Garcia <berto@igalia.com>
>---
> util/throttle.c | 62 +++++++++++++++++++++------------------------------------
> 1 file changed, 23 insertions(+), 39 deletions(-)
>
>diff --git a/util/throttle.c b/util/throttle.c
>index 9a6bda813c..1f29cf9918 100644
>--- a/util/throttle.c
>+++ b/util/throttle.c
>@@ -95,23 +95,36 @@ static int64_t throttle_do_compute_wait(double limit, double extra)
> int64_t throttle_compute_wait(LeakyBucket *bkt)
> {
>     double extra; /* the number of extra units blocking the io */
>+    double bucket_size;   /* I/O before throttling to bkt->avg */
>+    double burst_bucket_size; /* Before throttling to bkt->max */
>
>     if (!bkt->avg) {
>         return 0;
>     }
>
>-    /* 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;

>+    } 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;


>         if (extra > 0) {
>             return throttle_do_compute_wait(bkt->max, extra);
>         }
>@@ -358,31 +371,6 @@ bool throttle_is_valid(ThrottleConfig *cfg, Error **errp)
>     return true;
> }
>
>-/* fix bucket parameters */
>-static void throttle_fix_bucket(LeakyBucket *bkt)
>-{
>-    double min;
>-
>-    /* zero bucket level */
>-    bkt->level = bkt->burst_level = 0;
>-
>-    /* 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. */
>-    min = bkt->avg / 10;
>-    if (bkt->avg && !bkt->max) {
>-        bkt->max = min;
>-    }
>-}
>-
>-/* undo internal bucket parameter changes (see throttle_fix_bucket()) */
>-static void throttle_unfix_bucket(LeakyBucket *bkt)
>-{
>-    if (bkt->max < bkt->avg) {
>-        bkt->max = 0;
>-    }
>-}
>-
> /* Used to configure the throttle
>  *
>  * @ts: the throttle state we are working on
>@@ -397,8 +385,10 @@ void throttle_config(ThrottleState *ts,
>
>     ts->cfg = *cfg;
>
>+    /* Zero bucket level */
>     for (i = 0; i < BUCKETS_COUNT; i++) {
>-        throttle_fix_bucket(&ts->cfg.buckets[i]);
>+        ts->cfg.buckets[i].level = 0;
>+        ts->cfg.buckets[i].burst_level = 0;
>     }
>
>     ts->previous_leak = qemu_clock_get_ns(clock_type);
>@@ -411,13 +401,7 @@ void throttle_config(ThrottleState *ts,
>  */
> void throttle_get_config(ThrottleState *ts, ThrottleConfig *cfg)
> {
>-    int i;
>-
>     *cfg = ts->cfg;
>-
>-    for (i = 0; i < BUCKETS_COUNT; i++) {
>-        throttle_unfix_bucket(&cfg->buckets[i]);
>-    }
> }
>
>
>-- 
>2.11.0
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Alberto Garcia]

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)" ?

Berto

Re: [Qemu-devel] [PATCH 1/4] throttle: Fix wrong variable name in the header documentation

[Alberto Garcia]

On Sat 19 Aug 2017 04:29:47 PM CEST, Manos Pitsidianakis wrote:
> On Thu, Aug 17, 2017 at 05:28:12PM +0300, Alberto Garcia wrote:
>>The level of the burst bucket is stored in bkt.burst_level, not
>>bkt.burst_lenght.
>
> s/lenght/length, otherwise:

Oh my, fixing a typo by introducing another typo :-)

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

Berto

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

[Manos Pitsidianakis]

[-- Attachment #1: Type: text/plain, Size: 2486 bytes --]

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 <el13635@mail.ntua.gr>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]