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.
There's one particular instance of the previous code where bkt->max
could have a non-integer value: that's in throttle_fix_bucket() when
bkt->max is initialized to bkt->avg / 10. This is now an integer
division and the result is rounded. We don't need to worry about this
because:
a) with the magnitudes we're dealing with (bytes per second, I/O
operations per second) the limits are likely to be always
multiples of 10.
b) even if they weren't this doesn't affect the actual limits, only
the algorithm that makes the throttling smoother.
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
include/qemu/throttle.h | 4 ++--
util/throttle.c | 7 ++-----
2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index d056008c18..ec37ac0fcb 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 b2a52b8b34..7856931f67 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -111,7 +111,7 @@ int64_t throttle_compute_wait(LeakyBucket *bkt)
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 - (double) bkt->max / 10;
if (extra > 0) {
return throttle_do_compute_wait(bkt->max, extra);
}
@@ -361,8 +361,6 @@ bool throttle_is_valid(ThrottleConfig *cfg, Error **errp)
/* fix bucket parameters */
static void throttle_fix_bucket(LeakyBucket *bkt)
{
- double min;
-
/* zero bucket level */
bkt->level = bkt->burst_level = 0;
@@ -374,9 +372,8 @@ static void throttle_fix_bucket(LeakyBucket *bkt)
* Having a max burst value of 100ms of the average will help smooth the
* throttling
*/
- min = bkt->avg / 10;
if (bkt->avg && !bkt->max) {
- bkt->max = min;
+ bkt->max = (bkt->avg + 5) / 10;
}
}
--
2.11.0
On 08/07/2017 11:15 AM, 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. > > There's one particular instance of the previous code where bkt->max > could have a non-integer value: that's in throttle_fix_bucket() when > bkt->max is initialized to bkt->avg / 10. This is now an integer > division and the result is rounded. We don't need to worry about this > because: > > a) with the magnitudes we're dealing with (bytes per second, I/O > operations per second) the limits are likely to be always > multiples of 10. > > b) even if they weren't this doesn't affect the actual limits, only > the algorithm that makes the throttling smoother. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > include/qemu/throttle.h | 4 ++-- > util/throttle.c | 7 ++----- > 2 files changed, 4 insertions(+), 7 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Mon, Aug 07, 2017 at 07:15:29PM +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. > > There's one particular instance of the previous code where bkt->max > could have a non-integer value: that's in throttle_fix_bucket() when > bkt->max is initialized to bkt->avg / 10. This is now an integer > division and the result is rounded. We don't need to worry about this > because: > > a) with the magnitudes we're dealing with (bytes per second, I/O > operations per second) the limits are likely to be always > multiples of 10. > > b) even if they weren't this doesn't affect the actual limits, only > the algorithm that makes the throttling smoother. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > include/qemu/throttle.h | 4 ++-- > util/throttle.c | 7 ++----- > 2 files changed, 4 insertions(+), 7 deletions(-) Why is this marked for-2.10? Does it fix a bug? Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On Tue 08 Aug 2017 12:00:30 PM CEST, Stefan Hajnoczi wrote: > On Mon, Aug 07, 2017 at 07:15:29PM +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. >> > Why is this marked for-2.10? Does it fix a bug? I was under the impression that Markus wanted to change the QAPI types of the throttling fields in BlockDeviceInfo for 2.10 as well, so this patch is relevant. If that's not the case we can ignore it for now and leave it for the next cycle. Sorry for the noise! Berto
On Tue 08 Aug 2017 12:17:12 PM CEST, Alberto Garcia wrote: > I was under the impression that Markus wanted to change the QAPI types > of the throttling fields in BlockDeviceInfo for 2.10 as well, so this > patch is relevant. I just saw that his series is still an RFC, so we can leave this patch for 2.11. Sorry again! Berto
On 08/08/2017 05:00 AM, Stefan Hajnoczi wrote: > On Mon, Aug 07, 2017 at 07:15:29PM +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. >> > Why is this marked for-2.10? Does it fix a bug? Theoretically, converting between int64_t and double loses precision on any values larger than 2^53. In all practicality, though, if you expect throttling to be precise through 2^53 (approximately 16 orders of magnitude), you're crazy. I also don't see any change to potential division-by-zero actions (where differences in NaN vs. SIGFPE each have their own advantages, but changing between them can be construed as a bug fix). So I'm also having a hard time seeing this as 2.10 material. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Tue 08 Aug 2017 05:11:27 PM CEST, Eric Blake wrote: >> Why is this marked for-2.10? Does it fix a bug? > > Theoretically, converting between int64_t and double loses precision > on any values larger than 2^53. In all practicality, though, if you > expect throttling to be precise through 2^53 (approximately 16 orders > of magnitude), you're crazy. You cannot, THROTTLE_VALUE_MAX is set to 10^15, which is smaller than 2^50. > So I'm also having a hard time seeing this as 2.10 material. Right, as I said that was a wrong assumption from my side :) No need for this in 2.10. Berto
On Mon, Aug 07, 2017 at 07:15:29PM +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. > > There's one particular instance of the previous code where bkt->max > could have a non-integer value: that's in throttle_fix_bucket() when > bkt->max is initialized to bkt->avg / 10. This is now an integer > division and the result is rounded. We don't need to worry about this > because: > > a) with the magnitudes we're dealing with (bytes per second, I/O > operations per second) the limits are likely to be always > multiples of 10. > > b) even if they weren't this doesn't affect the actual limits, only > the algorithm that makes the throttling smoother. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > include/qemu/throttle.h | 4 ++-- > util/throttle.c | 7 ++----- > 2 files changed, 4 insertions(+), 7 deletions(-) Thanks, applied to my block-next tree: https://github.com/stefanha/qemu/commits/block-next Stefan
© 2016 - 2024 Red Hat, Inc.