| Message ID | 1385473433-15844-1-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
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)
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
[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?
-----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-----
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.
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.
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
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(-)