[Qemu-devel] [PATCH] virtio-rng: correct the default limit rate

[Qemu-devel] [PATCH] virtio-rng: correct the default limit rate

[Amos Kong]

We have the following comment about the default limit rate:

/* Set a default rate limit of 2^47 bytes per minute or roughly 2TB/s.  If
 * you have an entropy source capable of generating more entropy than this
 * and you can pass it through via virtio-rng, then hats off to you.  Until
 * then, this is unlimited for all practical purposes.
 */

But the current rate is (INT64_MAX) bytes per (1 << 16) ms, it's 128,000 TB/s

2TB/s is a reasonable rate, so this patch fixes the code, not update the document.

* change the default period to 60,000 ms --> 1 mins
* change the default max-bytes to 2^47 bytes --> INT64_MAX >> 16

 INT64_MAX  =  0x 8000 0000 0000 0000 - 1 = 2 ^ 63 - 1
 INT64_MAX >> 16 =  0x 8000 0000 0000 - 1 = 2 ^ 47

Signed-off-by: Amos Kong <akong@redhat.com>
---
 include/hw/virtio/virtio-rng.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/hw/virtio/virtio-rng.h b/include/hw/virtio/virtio-rng.h
index debaa15..d64e804 100644
--- a/include/hw/virtio/virtio-rng.h
+++ b/include/hw/virtio/virtio-rng.h
@@ -53,7 +53,7 @@ typedef struct VirtIORNG {
 */
 #define DEFINE_VIRTIO_RNG_PROPERTIES(_state, _conf_field)                    \
         DEFINE_PROP_UINT64("max-bytes", _state, _conf_field.max_bytes,       \
-                           INT64_MAX),                                       \
-        DEFINE_PROP_UINT32("period", _state, _conf_field.period_ms, 1 << 16)
+                           INT64_MAX >> 16),                                 \
+        DEFINE_PROP_UINT32("period", _state, _conf_field.period_ms, 60000)
 
 #endif
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] virtio-rng: correct the default limit rate

[Paolo Bonzini]

Il 26/11/2013 14:43, Amos Kong ha scritto:
> /* Set a default rate limit of 2^47 bytes per minute or roughly 2TB/s.  If
>  * you have an entropy source capable of generating more entropy than this
>  * and you can pass it through via virtio-rng, then hats off to you.  Until
>  * then, this is unlimited for all practical purposes.
>  */
> 
> But the current rate is (INT64_MAX) bytes per (1 << 16) ms, it's 128,000 TB/s

You are changing:

* max-bytes from 2^63 to 2^47

* period from 65536 to 60000

For a user, changing only period would have no effect, the limit rate
would remain effectively infinite.  Changing max-bytes would give a 7%
higher rate after your patch.

Not a big deal, and max-bytes is easier to explain after your patch
(bytes/minute) than before (bytes/65536ms).

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> * change the default period to 60,000 ms --> 1 mins
> * change the default max-bytes to 2^47 bytes --> INT64_MAX >> 16
> 
>  INT64_MAX  =  0x 8000 0000 0000 0000 - 1 = 2 ^ 63 - 1
>  INT64_MAX >> 16 =  0x 8000 0000 0000 - 1 = 2 ^ 47
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  include/hw/virtio/virtio-rng.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio-rng.h b/include/hw/virtio/virtio-rng.h
> index debaa15..d64e804 100644
> --- a/include/hw/virtio/virtio-rng.h
> +++ b/include/hw/virtio/virtio-rng.h
> @@ -53,7 +53,7 @@ typedef struct VirtIORNG {
>  */
>  #define DEFINE_VIRTIO_RNG_PROPERTIES(_state, _conf_field)                    \
>          DEFINE_PROP_UINT64("max-bytes", _state, _conf_field.max_bytes,       \
> -                           INT64_MAX),                                       \
> -        DEFINE_PROP_UINT32("period", _state, _conf_field.period_ms, 1 << 16)
> +                           INT64_MAX >> 16),                                 \
> +        DEFINE_PROP_UINT32("period", _state, _conf_field.period_ms, 60000)

Re: [Qemu-devel] [PATCH] virtio-rng: correct the default limit rate

[Amos Kong]

On Tue, Nov 26, 2013 at 02:58:26PM +0100, Paolo Bonzini wrote:
> Il 26/11/2013 14:43, Amos Kong ha scritto:
> > /* Set a default rate limit of 2^47 bytes per minute or roughly 2TB/s.  If
> >  * you have an entropy source capable of generating more entropy than this
> >  * and you can pass it through via virtio-rng, then hats off to you.  Until
> >  * then, this is unlimited for all practical purposes.
> >  */
> > 
> > But the current rate is (INT64_MAX) bytes per (1 << 16) ms, it's 128,000 TB/s
> 
> You are changing:
> 
> * max-bytes from 2^63 to 2^47
> 
> * period from 65536 to 60000
> 
> For a user, changing only period would have no effect, the limit rate
> would remain effectively infinite.  Changing max-bytes would give a 7%
> higher rate after your patch.

The rate will be effected when period OR max-bytes is changed.

Before patch (as documenet described): 2^47 / 65536 = 1.953125 TB/s
Applied patch:  2^47 / 600,000 = 2.1328125 TB/s

(2.1328125 - 1.953125) / 1.953125 = 0.092

It increased 9.2%, as you said, not a big deal.
 
> Not a big deal, and max-bytes is easier to explain after your patch
> (bytes/minute) than before (bytes/65536ms).
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
 
Thanks for the quick feedback.

Amos

Re: [Qemu-devel] [PATCH] virtio-rng: correct the default limit rate

[Eric Blake]

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

[adding libvirt]

On 11/26/2013 06:58 AM, Paolo Bonzini wrote:
> Il 26/11/2013 14:43, Amos Kong ha scritto:
>> /* Set a default rate limit of 2^47 bytes per minute or roughly 2TB/s.  If
>>  * you have an entropy source capable of generating more entropy than this
>>  * and you can pass it through via virtio-rng, then hats off to you.  Until
>>  * then, this is unlimited for all practical purposes.
>>  */
>>
>> But the current rate is (INT64_MAX) bytes per (1 << 16) ms, it's 128,000 TB/s
> 
> You are changing:
> 
> * max-bytes from 2^63 to 2^47
> 
> * period from 65536 to 60000
> 
> For a user, changing only period would have no effect, the limit rate
> would remain effectively infinite.  Changing max-bytes would give a 7%
> higher rate after your patch.
> 
> Not a big deal, and max-bytes is easier to explain after your patch
> (bytes/minute) than before (bytes/65536ms).
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 

Hmm.  Libvirt is already converting a user's rate of bytes/period into
the qemu parameters, defaulting to 1 second as its default period.  Am I
correct that as long as libvirt specified both rate AND period, then
this change has no impact (and that the 7% change occurs if you specify
period while leaving max-bytes alone)?  Or is this an ABI change where
libvirt will have to be taught to be smart enough to know whether it is
old qemu or new qemu to adjust how libvirt does its calculations when
converting the user's rate into qemu terms?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH] virtio-rng: correct the default limit rate

[Paolo Bonzini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 26/11/2013 15:32, Eric Blake ha scritto:
> Hmm.  Libvirt is already converting a user's rate of bytes/period
> into the qemu parameters, defaulting to 1 second as its default
> period.  Am I correct that as long as libvirt specified both rate
> AND period, then this change has no impact (and that the 7% change
> occurs if you specify period while leaving max-bytes alone)?  Or is
> this an ABI change where libvirt will have to be taught to be smart
> enough to know whether it is old qemu or new qemu to adjust how
> libvirt does its calculations when converting the user's rate into
> qemu terms?

Yes, it is the former---i.e. you're right.

Paolo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJSlLPjAAoJEBvWZb6bTYbyDg8QAIk8BIh5ZY0w4aF7AHufxpmn
XtvbGIAdb+ztNBtPrL5HJXAvaOvbZNfktmKDA7Cee1Cq8opJPUZQDpwmv7xUshpG
yc1YQu3Djb0/kCDV0oNiMQARSwxJ3XMR94SSLfmlI3b7lJVDeviouhV4UdjS3eal
ZNWUXya+1bloOC+zX4P0atB5hyrpVhcVHddWLsIs4+6EYcOatHPgl5CK5f5uxq+P
5qkAe1FJTHMkBOJ86kis3s57ZTTqIlkvFiqtJY4IViifHjbMR4PJraO6EQigOxf6
VdV+MIkPm3GA6F3SYOoZNacITBsaRXTJ0kjD+BjUUAFONB7X8//Uob5gSUgmkED7
ATqiDnqSq1jwyV4FAVwYDB397ViLAZYd9k/ai4QZLaclyMqHQKx+RCNN7dmFwCuL
EYK0Z9AkoVkJJQQUcERO3ps/T89+tlBVQhBKpae1V2ZKJqCGxLaf0344S+kFicYk
bqp87ra5L5J8Qdn+/eBMJsQNa5NDWjKU0TvZSr3tGGoGKKamzwFmnsTzcZ9DHoTZ
IVcdHAbToCYnqbnJzORoxFVycSrY8B2AhZOqIz5YQgNV1LDPcK7hzcXZ+rpBu5mU
zF9MFHBKrGJo7l4GyC4jFa5osDfGOyGzMyoSvlMuFTG8CUXN/QLmcpzE+NL1QlCj
lL7TLkbhYKrQxUPw37+P
=mb5e
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH] virtio-rng: correct the default limit rate

[Amos Kong]

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

On Tue, Nov 26, 2013 at 07:32:37AM -0700, Eric Blake wrote:
> [adding libvirt]
> 
> On 11/26/2013 06:58 AM, Paolo Bonzini wrote:
> > Il 26/11/2013 14:43, Amos Kong ha scritto:
> >> /* Set a default rate limit of 2^47 bytes per minute or roughly 2TB/s.  If
> >>  * you have an entropy source capable of generating more entropy than this
> >>  * and you can pass it through via virtio-rng, then hats off to you.  Until
> >>  * then, this is unlimited for all practical purposes.
> >>  */
> >>
> >> But the current rate is (INT64_MAX) bytes per (1 << 16) ms, it's 128,000 TB/s
> > 
> > You are changing:
> > 
> > * max-bytes from 2^63 to 2^47
> > 
> > * period from 65536 to 60000
> > 
> > For a user, changing only period would have no effect, the limit rate
> > would remain effectively infinite.  Changing max-bytes would give a 7%
> > higher rate after your patch.
> > 
> > Not a big deal, and max-bytes is easier to explain after your patch
> > (bytes/minute) than before (bytes/65536ms).
> > 
> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> 
> Hmm.  Libvirt is already converting a user's rate of bytes/period into
> the qemu parameters, defaulting to 1 second as its default period.

So libvirt will always pass a fixed 1 second period to qemu?

> Am I
> correct that as long as libvirt specified both rate AND period, then
> this change has no impact (and that the 7% change occurs if you specify
> period while leaving max-bytes alone)?

>  Or is this an ABI change where
> libvirt will have to be taught to be smart enough to know whether it is
> old qemu or new qemu to adjust how libvirt does its calculations when
> converting the user's rate into qemu terms?

Nothing need to do in Libvirt for _this patch_

No API change here, just change default rate from 1.9 TB/s to 2.1 TB/s
This patch didn't change another limit logic.


== Effect of the period parameter ==

When we set a fixed ratespeed, we can use different periods. The
period still effect the stable of system IO. 
If the period is too large, and we set a higher speed, then IO will wave.

Example:  the IO max speed is 20 M/s, and we test 5 mins
          it clear that the first period is better

_Theory_ Condition:

* period 1:   20M / 1s
  from 0 ~ 20 second, read packets,
  from 21 ~ 100 second, wait ...

  from 0 ~ 20 second, read packets,
  from 21 ~ 100 second, wait ...

  from 260 ~ 281 second, read packets,
  from 281 ~ 300 second, wait ...

* period 2:   100M / 5
  from 0 ~ 60 second, read packets,
  from 61 ~ 300 second, wait ...

Smaller period is better, but smaller period will cause more timer
expired, and IO wave will be balance by scheduling other process.

So Libvirt should pass period & max-bytes in xml to qemu without
converting.

-- 
			Amos.

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH] virtio-rng: correct the default limit rate

[Eric Blake]

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

On 11/26/2013 08:23 AM, Amos Kong wrote:

>>
>> Hmm.  Libvirt is already converting a user's rate of bytes/period into
>> the qemu parameters, defaulting to 1 second as its default period.
> 
> So libvirt will always pass a fixed 1 second period to qemu?

You forced me to check the code :)

We're safe.  Libvirt lets the user control both rate and period in the
XML.  If the user specifies neither, then libvirt doesn't pass max-bytes
or period (but that's the default for unlimited, and you argued that
even with your changes, the default is still effectively unlimited); the
same is true if the user specifies period but not rate (a different
period with no maximum is still effectively unlimited throughput, so it
didn't really matter that we don't pass period on through).  It is only
when the user specifies rate that libvirt passes anything on to qemu;
and in that case, libvirt always passes both max-bytes and period
(either with a period set by user, or with the period forced to 1000).

> 
> Nothing need to do in Libvirt for _this patch_

Agreed.  Phew.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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