| Message ID | 112e28c8da085ffd59381d86f3acb0b8f75d5f9a.1502979710.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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
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>
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]); - } }
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(-)