diff mbox

[v2,2/2] throttle: make throttle_config(throttle_get_config()) symmetric

Message ID 20170228111935.10826-3-stefanha@redhat.com
State New
Headers show

Commit Message

Stefan Hajnoczi Feb. 28, 2017, 11:19 a.m. UTC 
Throttling has a weird property that throttle_get_config() does not
always return the same throttling settings that were given with
throttle_config().  In other words, the set and get functions aren't
symmetric.

If .max is 0 then the throttling code assigns a default value of .avg /
10 in throttle_config().  This is an implementation detail of the
throttling algorithm.  When throttle_get_config() is called the .max
value returned should still be 0.

Users are exposed to this quirk via "info block" or "query-block"
monitor commands.  This has caused confusion because it looks like a bug
when an unexpected value is reported.

This patch hides the .max value adjustment in throttle_get_config() and
updates test-throttle.c appropriately.

Reported-by: Nini Gu <ngu@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/test-throttle.c |  8 ++++----
 util/throttle.c       | 16 ++++++++++++++++
 2 files changed, 20 insertions(+), 4 deletions(-)

Comments

Alberto Garcia Feb. 28, 2017, 12:17 p.m. UTC | #1 
On Tue 28 Feb 2017 12:19:35 PM CET, Stefan Hajnoczi wrote:
> +/* undo internal bucket parameter changes (see throttle_fix_bucket()) */
> +static void throttle_unfix_bucket(LeakyBucket *bkt)
> +{
> +    double min = bkt->avg / 10;
> +
> +    if (bkt->max == min) {
> +        bkt->max = 0;
> +    }
> +}

I guess you could do the more general if (bkt->max < bkt->avg), but your
solution is also fine with me.

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto
Stefan Hajnoczi March 1, 2017, 10:44 a.m. UTC | #2 
On Tue, Feb 28, 2017 at 01:17:55PM +0100, Alberto Garcia wrote:
> On Tue 28 Feb 2017 12:19:35 PM CET, Stefan Hajnoczi wrote:
> > +/* undo internal bucket parameter changes (see throttle_fix_bucket()) */
> > +static void throttle_unfix_bucket(LeakyBucket *bkt)
> > +{
> > +    double min = bkt->avg / 10;
> > +
> > +    if (bkt->max == min) {
> > +        bkt->max = 0;
> > +    }
> > +}
> 
> I guess you could do the more general if (bkt->max < bkt->avg), but your
> solution is also fine with me.
> 
> Reviewed-by: Alberto Garcia <berto@igalia.com>

I did that originally because I try to avoid floating-point equality.
The test case uses an invalid setting though (->max = 1, ->avg = 56)
which would be rejected if given on the command-line.

Thinking about this again, it's probably better to modify the test case
and use max < avg.

Will send a v3.

Stefan
diff mbox

Patch

diff --git a/tests/test-throttle.c b/tests/test-throttle.c
index 363b59a..3d6bb82 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -205,8 +205,8 @@  static void test_config_functions(void)
     orig_cfg.buckets[THROTTLE_OPS_READ].avg  = 69;
     orig_cfg.buckets[THROTTLE_OPS_WRITE].avg = 23;
 
-    orig_cfg.buckets[THROTTLE_BPS_TOTAL].max = 0; /* should be corrected */
-    orig_cfg.buckets[THROTTLE_BPS_READ].max  = 1; /* should not be corrected */
+    orig_cfg.buckets[THROTTLE_BPS_TOTAL].max = 0;
+    orig_cfg.buckets[THROTTLE_BPS_READ].max  = 1;
     orig_cfg.buckets[THROTTLE_BPS_WRITE].max = 120;
 
     orig_cfg.buckets[THROTTLE_OPS_TOTAL].max = 150;
@@ -246,8 +246,8 @@  static void test_config_functions(void)
     g_assert(final_cfg.buckets[THROTTLE_OPS_READ].avg  == 69);
     g_assert(final_cfg.buckets[THROTTLE_OPS_WRITE].avg == 23);
 
-    g_assert(final_cfg.buckets[THROTTLE_BPS_TOTAL].max == 15.3);/* fixed */
-    g_assert(final_cfg.buckets[THROTTLE_BPS_READ].max  == 1);   /* not fixed */
+    g_assert(final_cfg.buckets[THROTTLE_BPS_TOTAL].max == 0);
+    g_assert(final_cfg.buckets[THROTTLE_BPS_READ].max  == 1);
     g_assert(final_cfg.buckets[THROTTLE_BPS_WRITE].max == 120);
 
     g_assert(final_cfg.buckets[THROTTLE_OPS_TOTAL].max == 150);
diff --git a/util/throttle.c b/util/throttle.c
index 3817d9b..d87b421 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -380,6 +380,16 @@  static void throttle_fix_bucket(LeakyBucket *bkt)
     }
 }
 
+/* undo internal bucket parameter changes (see throttle_fix_bucket()) */
+static void throttle_unfix_bucket(LeakyBucket *bkt)
+{
+    double min = bkt->avg / 10;
+
+    if (bkt->max == min) {
+        bkt->max = 0;
+    }
+}
+
 /* take care of canceling a timer */
 static void throttle_cancel_timer(QEMUTimer *timer)
 {
@@ -420,7 +430,13 @@  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]);
+    }
 }