Misc throttle fixes
PATCH v2 1/7 throttle: Fix wrong variable name in the header documentation
In reply to Alberto GarciaThe level of the burst bucket is stored in bkt.burst_level, not bkt.burst_length.
--- include/qemu/throttle.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/qemu/throttle.h +1 -1
@@ -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
Signature · 1 line
2.11.0
PATCH v2 3/7 throttle: Make throttle_is_valid() a bit less verbose
In reply to Alberto GarciaUse a pointer to the bucket instead of repeating cfg->buckets[i] all the time. This makes the code more concise and will help us expand the checks later and save a few line breaks.
--- util/throttle.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)
util/throttle.c +7 -8
@@ -324,32 +324,31 @@ bool throttle_is_valid(ThrottleConfig *cfg, Error **errp) }for (i =0 ; i <BUCKETS_COUNT ; i++) { -if (cfg->buckets[i].avg <0 || - cfg->buckets[i].max <0 || - cfg->buckets[i].avg >THROTTLE_VALUE_MAX || - cfg->buckets[i].max >THROTTLE_VALUE_MAX ) { +LeakyBucket *bkt = &cfg->buckets[i]; +if (bkt->avg <0 || bkt->max <0 || + bkt->avg >THROTTLE_VALUE_MAX || bkt->max >THROTTLE_VALUE_MAX ) {error_setg (errp,"bps/iops/max values must be within [0, %lld]" ,THROTTLE_VALUE_MAX );return false; } -if (!cfg->buckets[i].burst_length) { +if (!bkt->burst_length) {error_setg (errp,"the burst length cannot be 0" );return false; } -if (cfg->buckets[i].burst_length >1 && !cfg->buckets[i].max) { +if (bkt->burst_length >1 && !bkt->max) {error_setg (errp,"burst length set without burst rate" );return false; } -if (cfg->buckets[i].max && !cfg->buckets[i].avg) { +if (bkt->max && !bkt->avg) {error_setg (errp,"bps_max/iops_max require corresponding" " bps/iops values" );return false; } -if (cfg->buckets[i].max && cfg->buckets[i].max < cfg->buckets[i].avg) { +if (bkt->max && bkt->max < bkt->avg) {error_setg (errp,"bps_max/iops_max cannot be lower than bps/iops" );return false; }
Signature · 1 line
2.11.0
PATCH v2 4/7 throttle: Remove throttle_fix_bucket() / throttle_unfix_bucket()
In reply to Alberto GarciaThe 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().
--- util/throttle.c | 62 +++++++++++++++++++++------------------------------------ 1 file changed, 23 insertions(+), 39 deletions(-)
util/throttle.c +23 -39
@@ -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); }
@@ -357,31 +370,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
@@ -396,8 +384,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);
@@ -410,13 +400,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]); - } }
Signature · 1 line
2.11.0
PATCH v2 7/7 throttle: Test the valid range of config values
In reply to Alberto Garcia--- tests/test-throttle.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+)
tests/test-throttle.c +77 -0
@@ -378,6 +378,82 @@ static void test_is_valid(void)test_is_valid_for_value (1 , true); } +static void test_ranges (void ) +{ +int i; + +for (i =0 ; i <BUCKETS_COUNT ; i++) { +LeakyBucket *b = &cfg.buckets[i]; +throttle_config_init (&cfg); + +/* avg = 0 means throttling is disabled, but the config is valid */ + b->avg =0 ; +g_assert (throttle_is_valid (&cfg,NULL )); +g_assert (!throttle_enabled (&cfg)); + +/* These are valid configurations (values <= THROTTLE_VALUE_MAX) */ + b->avg =1 ; +g_assert (throttle_is_valid (&cfg,NULL )); + + b->avg =THROTTLE_VALUE_MAX ; +g_assert (throttle_is_valid (&cfg,NULL )); + + b->avg =THROTTLE_VALUE_MAX ; + b->max =THROTTLE_VALUE_MAX ; +g_assert (throttle_is_valid (&cfg,NULL )); + +/* Values over THROTTLE_VALUE_MAX are not allowed */ + b->avg =THROTTLE_VALUE_MAX +1 ; +g_assert (!throttle_is_valid (&cfg,NULL )); + + b->avg =THROTTLE_VALUE_MAX ; + b->max =THROTTLE_VALUE_MAX +1 ; +g_assert (!throttle_is_valid (&cfg,NULL )); + +/* burst_length must be between 1 and THROTTLE_VALUE_MAX */ + b->avg =1 ; + b->max =1 ; + b->burst_length =0 ; +g_assert (!throttle_is_valid (&cfg,NULL )); + + b->avg =1 ; + b->max =1 ; + b->burst_length =1 ; +g_assert (throttle_is_valid (&cfg,NULL )); + + b->avg =1 ; + b->max =1 ; + b->burst_length =THROTTLE_VALUE_MAX ; +g_assert (throttle_is_valid (&cfg,NULL )); + + b->avg =1 ; + b->max =1 ; + b->burst_length =THROTTLE_VALUE_MAX +1 ; +g_assert (!throttle_is_valid (&cfg,NULL )); + +/* burst_length * max cannot exceed THROTTLE_VALUE_MAX */ + b->avg =1 ; + b->max =2 ; + b->burst_length =THROTTLE_VALUE_MAX /2 ; +g_assert (throttle_is_valid (&cfg,NULL )); + + b->avg =1 ; + b->max =3 ; + b->burst_length =THROTTLE_VALUE_MAX /2 ; +g_assert (!throttle_is_valid (&cfg,NULL )); + + b->avg =1 ; + b->max =THROTTLE_VALUE_MAX ; + b->burst_length =1 ; +g_assert (throttle_is_valid (&cfg,NULL )); + + b->avg =1 ; + b->max =THROTTLE_VALUE_MAX ; + b->burst_length =2 ; +g_assert (!throttle_is_valid (&cfg,NULL )); + } +} +static void test_max_is_missing_limit (void ) {int i;
@@ -669,6 +745,7 @@ int main(int argc, char **argv)g_test_add_func ("/throttle/config/enabled" , test_enabled);g_test_add_func ("/throttle/config/conflicting" , test_conflicting_config);g_test_add_func ("/throttle/config/is_valid" , test_is_valid); +g_test_add_func ("/throttle/config/ranges" , test_ranges);g_test_add_func ("/throttle/config/max" , test_max_is_missing_limit);g_test_add_func ("/throttle/config/iops_size" , test_iops_size_is_missing_limit);
Signature · 1 line
2.11.0
PATCH v2 5/7 throttle: Make LeakyBucket.avg and LeakyBucket.max integer types
In reply to Alberto GarciaBoth 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 uint64_t for those types. Let's also use an unsigned type because we don't allow negative values anyway.
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.
--- include/qemu/throttle.h | 4 ++-- tests/test-throttle.c | 3 ++- util/throttle.c | 7 +++---- 3 files changed, 7 insertions(+), 7 deletions(-)
include/qemu/throttle.h +2 -2
@@ -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 */ + uint64_t avg; /* average goal in units per second */ + uint64_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 */
tests/test-throttle.c +2 -1
@@ -284,13 +284,14 @@ static void test_enabled(void)for (i =0 ; i <BUCKETS_COUNT ; i++) {throttle_config_init (&cfg);set_cfg_value (false, i,150 ); +g_assert (throttle_is_valid (&cfg,NULL ));g_assert (throttle_enabled (&cfg)); }for (i =0 ; i <BUCKETS_COUNT ; i++) {throttle_config_init (&cfg);set_cfg_value (false, i,-150 ); -g_assert (!throttle_enabled (&cfg)); +g_assert (!throttle_is_valid (&cfg,NULL )); } }
util/throttle.c +3 -4
@@ -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 */
@@ -338,8 +338,7 @@ bool throttle_is_valid(ThrottleConfig *cfg, Error **errp)for (i =0 ; i <BUCKETS_COUNT ; i++) {LeakyBucket *bkt = &cfg->buckets[i]; -if (bkt->avg <0 || bkt->max <0 || - bkt->avg >THROTTLE_VALUE_MAX || bkt->max >THROTTLE_VALUE_MAX ) { +if (bkt->avg >THROTTLE_VALUE_MAX || bkt->max >THROTTLE_VALUE_MAX ) {error_setg (errp,"bps/iops/max values must be within [0, %lld]" ,THROTTLE_VALUE_MAX );return false;
Signature · 1 line
2.11.0
v2 cover Misc throttle fixes
Hi,
here's the new version of my patch series with misc fixes for the throttling code.
Stefan, once this is reviewed, can you please remove the "Make LeakyBucket.avg and LeakyBucket.max integer types" commit (id 218f470639117a8e39) from your block-next branch? This series contains a new version of that patch that replaces the one that you have.
v2:
- Patch 3: Make the code a bit more concise
- Patch 5: LeakyBucket.avg and LeakyBucket.max are now unsigned
- Patch 6: Make burst_length 64bit and check its range
- Patch 7: Add unit tests
v1: https://lists.gnu.org/archive/html/qemu-block/2017-08/msg00753.html
- Initial version
Output of backport-diff against v1:
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
001/7:[----] [--] 'throttle: Fix wrong variable name in the header documentation' 002/7:[----] [--] 'throttle: Update the throttle_fix_bucket() documentation' 003/7:[down] 'throttle: Make throttle_is_valid() a bit less verbose' 004/7:[----] [--] 'throttle: Remove throttle_fix_bucket() / throttle_unfix_bucket()' 005/7:[0010] [FC] 'throttle: Make LeakyBucket.avg and LeakyBucket.max integer types' 006/7:[down] 'throttle: Make burst_length 64bit and add range checks' 007/7:[down] 'throttle: Test the valid range of config values'
Alberto Garcia (7):
throttle: Fix wrong variable name in the header documentation
throttle: Update the throttle_fix_bucket() documentation
throttle: Make throttle_is_valid() a bit less verbose
throttle: Remove throttle_fix_bucket() / throttle_unfix_bucket()
throttle: Make LeakyBucket.avg and LeakyBucket.max integer types
throttle: Make burst_length 64bit and add range checks
throttle: Test the valid range of config values
include/qemu/throttle.h | 8 ++--- tests/test-throttle.c | 80 ++++++++++++++++++++++++++++++++++++++++++++- util/throttle.c | 86 +++++++++++++++++++------------------------------ 3 files changed, 117 insertions(+), 57 deletions(-)
Signature · 1 line
2.11.0
PATCH v2 2/7 throttle: Update the throttle_fix_bucket() documentation
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.
--- util/throttle.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
util/throttle.c +3 -8
@@ -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;
Signature · 1 line
2.11.0
PATCH v2 6/7 throttle: Make burst_length 64bit and add range checks
In reply to Alberto GarciaLeakyBucket.burst_length is defined as an unsigned integer but the code never checks for overflows and it only makes sure that the value is not 0.
In practice this means that the user can set something like throttling.iops-total-max-length=4294967300 despite being larger than UINT_MAX and the final value after casting to unsigned int will be 4.
This patch changes the data type to uint64_t. This does not increase the storage size of LeakyBucket, and allows us to assign the value directly from qemu_opt_get_number() or BlockIOThrottle and then do the checks directly in throttle_is_valid().
The value of burst_length does not have a specific upper limit, but since the bucket size is defined by max * burst_length we have to prevent overflows. Instead of going for UINT64_MAX or something similar this patch reuses THROTTLE_VALUE_MAX, which allows I/O bursts of 1 GiB/s for 10 days in a row.
--- include/qemu/throttle.h | 2 +- util/throttle.c | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-)
include/qemu/throttle.h +1 -1
@@ -81,7 +81,7 @@ typedef struct LeakyBucket {uint64_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 */ +uint64_t burst_length;/* max length of the burst period, in seconds */ } LeakyBucket;/* The following structure is used to configure a ThrottleState
util/throttle.c +5 -0
@@ -354,6 +354,11 @@ bool throttle_is_valid(ThrottleConfig *cfg, Error **errp)return false; } +if (bkt->max && bkt->burst_length >THROTTLE_VALUE_MAX / bkt->max) { +error_setg (errp,"burst length too high for this burst rate" ); +return false; + } +if (bkt->max && !bkt->avg) {error_setg (errp,"bps_max/iops_max require corresponding" " bps/iops values" );
Signature · 1 line
2.11.0
Stefan Hajnoczi
AppliedQuoted 44 lines · Alberto Garcia
Hi,
here's the new version of my patch series with misc fixes for the throttling code.
Stefan, once this is reviewed, can you please remove the "Make LeakyBucket.avg and LeakyBucket.max integer types" commit (id
218f470639117a8e39) from your block-next branch? This series contains a new version of that patch that replaces the one that you have.v2:
- Patch 3: Make the code a bit more concise
- Patch 5: LeakyBucket.avg and LeakyBucket.max are now unsigned
- Patch 6: Makeburst_length64bit and check its range
- Patch 7: Add unit testsv1: https://lists.gnu.org/archive/html/qemu-block/2017-08/msg00753.html
- Initial versionOutput of backport-diff against v1:
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively001/7:[----] [--] 'throttle: Fix wrong variable name in the header documentation' 002/7:[----] [--] 'throttle: Update the
throttle_fix_bucket()documentation' 003/7:[down] 'throttle: Makethrottle_is_valid()a bit less verbose' 004/7:[----] [--] 'throttle: Removethrottle_fix_bucket()/throttle_unfix_bucket()' 005/7:[0010] [FC] 'throttle: Make LeakyBucket.avg and LeakyBucket.max integer types' 006/7:[down] 'throttle: Makeburst_length64bit and add range checks' 007/7:[down] 'throttle: Test the valid range of config values'Alberto Garcia (7):
throttle: Fix wrong variable name in the header documentation
throttle: Update thethrottle_fix_bucket()documentation
throttle: Makethrottle_is_valid()a bit less verbose
throttle: Removethrottle_fix_bucket()/throttle_unfix_bucket()
throttle: Make LeakyBucket.avg and LeakyBucket.max integer types
throttle: Makeburst_length64bit and add range checks
throttle: Test the valid range of config values
Quoted 9 lines
include/qemu/throttle.h | 8 ++--- tests/test-throttle.c | 80 ++++++++++++++++++++++++++++++++++++++++++++- util/throttle.c | 86 +++++++++++++++++++------------------------------ 3 files changed, 117 insertions(+), 57 deletions(-)Signature · 1 line
2.11.0
I have dropped the previous "Make LeakyBucket.avg and LeakyBucket.max integer types" commit and merged this series instead.
Thanks, applied to my block-next tree:
https://github.com/stefanha/qemu/commits/block-next
Stefan
Re: [Qemu-devel] [PATCH v2 2/7] throttle: Update the throttle_fix_bucket() documentation
In reply to Alberto GarciaQuoted 22 lines · 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.
Quoted 7 lines
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 <[email protected]>--- util/throttle.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2 3/7] throttle: Make throttle_is_valid() a bit less verbose
In reply to Alberto GarciaQuoted 14 lines · Alberto Garcia
Use a pointer to the bucket instead of repeating cfg->buckets[i] all the time. This makes the code more concise and will help us expand the checks later and save a few line breaks.
Signed-off-by Alberto Garcia <[email protected]>--- util/throttle.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)util/throttle.c +0
-0
util/throttle.c bool throttle_is_valid(ThrottleConfig *cfg, Error **errp) · line 328
Quoted 9 lines
util/throttle.c +2
-4@@ -324,32 +324,31 @@ bool throttle_is_valid(ThrottleConfig *cfg, Error **errp) }for (i =0 ; i <BUCKETS_COUNT ; i++) { -if (cfg->buckets[i].avg <0 || - cfg->buckets[i].max <0 || - cfg->buckets[i].avg >THROTTLE_VALUE_MAX || - cfg->buckets[i].max >THROTTLE_VALUE_MAX ) { +LeakyBucket *bkt = &cfg->buckets[i]; + if (bkt->avg < 0 || bkt->max < 0 ||
Up to the maintainer, but I'd include a blank line between declarations and code.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2 5/7] throttle: Make LeakyBucket.avg and LeakyBucket.max integer types
In reply to Alberto GarciaQuoted 16 lines · 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
uint64_tfor those types. Let's also use an unsigned type because we don't allow negative values anyway.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.
Quoted 7 lines
Signed-off-by Alberto Garcia <[email protected]>--- include/qemu/throttle.h | 4 ++-- tests/test-throttle.c | 3 ++- util/throttle.c | 7 +++---- 3 files changed, 7 insertions(+), 7 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2 6/7] throttle: Make burst_length 64bit and add range checks
In reply to Alberto GarciaQuoted 19 lines · Alberto Garcia
LeakyBucket.burst_length is defined as an unsigned integer but the code never checks for overflows and it only makes sure that the value is not 0.
In practice this means that the user can set something like throttling.iops-total-max-length=4294967300 despite being larger than
UINT_MAXand the final value after casting to unsigned int will be 4.This patch changes the data type to
uint64_t. This does not increase the storage size of LeakyBucket, and allows us to assign the value directly fromqemu_opt_get_number()or BlockIOThrottle and then do the checks directly inthrottle_is_valid().The value of
burst_lengthdoes not have a specific upper limit, but since the bucket size is defined by max *burst_lengthwe have to prevent overflows. Instead of going forUINT64_MAXor something similar this patch reusesTHROTTLE_VALUE_MAX, which allows I/O bursts of 1 GiB/s for 10 days in a row.
Quoted 6 lines
Signed-off-by Alberto Garcia <[email protected]>--- include/qemu/throttle.h | 2 +- util/throttle.c | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org