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

[Qemu-devel] [PATCH for-2.10] 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.

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

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

[Eric Blake]

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

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

[Stefan Hajnoczi]

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

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>

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

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

[Alberto Garcia]

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

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

[Alberto Garcia]

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

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

[Eric Blake]

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

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

[Alberto Garcia]

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

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

[Stefan Hajnoczi]

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

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

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