[Qemu-devel] [PATCH 0/2 v3] virtio-rng: Avoid uncessary timer trigger to bump up quota value

[Qemu-devel] [PATCH 0/2 v3] virtio-rng: Avoid uncessary timer trigger to bump up quota value

[Pankaj Gupta]

Timer was added in virtio-rng to rate limit the
entropy. It used to trigger at regular intervals to
bump up the quota value. The value of quota and timer
is used to ensure single guest should not use up all the 
entropy from the host.
This resulted in triggering of timer even when quota
is not exhausted at all and resulting in extra processing.

This series has two patches:

patch1 : Bump up quota value only when guest requests entropy.
patch2 : Serve pending request if any after timer bumps up quota.

Changes from v1:
Amit Shah : Serve pending request if any after timer bumps up quota.
            Add testing details.
Changes from v2:
Amit Shah : Use 'get_request_size' function to check if there is any 
            request pending.

I tested this with '/dev/urandom' at host side. Below are the details:

* Quota+timer specified on command line, 
  Ran in Guest 'dd if=/dev/hwrng of=/dev/null'
  
  - Quota (4096 bytes), Time slice (1000 ms)

    48152 bytes (49 KB) copied, 12.0005 s, 4.1 kB/s
    48640 bytes (49 KB) copied, 12.0014 s, 4.1 kB/s

  - Quota (8192), Time slice (1000 ms)
    65536 bytes  (66 KB) copied, 8.00088 s, 8.2 kB/s
    146944 bytes (147 KB)copied, 18.0021 s, 8.2 kB/s

  - No quota/timer specified on command line, takes default values.
    Quota (INT64_MAX), Time slice(65536 ms)
    8050688 bytes (8.1 MB) copied, 93.1198 s, 86.5 kB/s
    35568128 bytes (36 MB) copied, 408.823 s, 87.0 kB/s


 hw/virtio/virtio-rng.c         |   21 +++++++++++++--------
 include/hw/virtio/virtio-rng.h |    1 +
 2 files changed, 14 insertions(+), 8 deletions(-)

[Qemu-devel] [PATCH 1/2 v3] virtio-rng: Bump up quota value only when guest requests entropy

[Pankaj Gupta]

This patch triggers timer only when guest requests for 
entropy. As soon as first request from guest for entropy 
comes we set the timer. Timer bumps up the quota value 
when it gets triggered.

Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
Reviewed-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio/virtio-rng.c         | 15 ++++++++-------
 include/hw/virtio/virtio-rng.h |  1 +
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index 22b1d87..8774a0c 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -78,6 +78,12 @@ static void virtio_rng_process(VirtIORNG *vrng)
         return;
     }
 
+    if (vrng->activate_timer) {
+        timer_mod(vrng->rate_limit_timer,
+                   qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + vrng->conf.period_ms);
+        vrng->activate_timer = false;
+    }
+
     if (vrng->quota_remaining < 0) {
         quota = 0;
     } else {
@@ -139,8 +145,7 @@ static void check_rate_limit(void *opaque)
 
     vrng->quota_remaining = vrng->conf.max_bytes;
     virtio_rng_process(vrng);
-    timer_mod(vrng->rate_limit_timer,
-                   qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + vrng->conf.period_ms);
+    vrng->activate_timer = true;
 }
 
 static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
@@ -196,13 +201,9 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
 
     vrng->vq = virtio_add_queue(vdev, 8, handle_input);
     vrng->quota_remaining = vrng->conf.max_bytes;
-
     vrng->rate_limit_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
                                                check_rate_limit, vrng);
-
-    timer_mod(vrng->rate_limit_timer,
-                   qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + vrng->conf.period_ms);
-
+    vrng->activate_timer = true;
     register_savevm(dev, "virtio-rng", -1, 1, virtio_rng_save,
                     virtio_rng_load, vrng);
 }
diff --git a/include/hw/virtio/virtio-rng.h b/include/hw/virtio/virtio-rng.h
index 0316488..3f07de7 100644
--- a/include/hw/virtio/virtio-rng.h
+++ b/include/hw/virtio/virtio-rng.h
@@ -44,6 +44,7 @@ typedef struct VirtIORNG {
      */
     QEMUTimer *rate_limit_timer;
     int64_t quota_remaining;
+    bool activate_timer;
 } VirtIORNG;
 
 #endif
-- 
1.9.3

[Qemu-devel] [PATCH 2/2 v3] virtio-rng: Serve pending request if any after timer bumps up quota.

[Pankaj Gupta]

We are arming timer when we get first request from guest.
Even if guest pulls all the data we will be serving guest
only when timer bumps up new quota. When timer expires
we check if we have a pending request from guest, we
serve it and re-arm the timer else we don't do any thing.

Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
---
 hw/virtio/virtio-rng.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index ae04352..3d9a002 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -142,9 +142,13 @@ static int virtio_rng_load(QEMUFile *f, void *opaque, int version_id)
 static void check_rate_limit(void *opaque)
 {
     VirtIORNG *vrng = opaque;
+    size_t size;
 
     vrng->quota_remaining = vrng->conf.max_bytes;
-    virtio_rng_process(vrng);
+    size = get_request_size(vrng->vq, 0);
+    if (size > 0) {
+        virtio_rng_process(vrng);
+    }
     vrng->activate_timer = true;
 }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH 2/2 v3] virtio-rng: Serve pending request if any after timer bumps up quota.

[Amit Shah]

On (Wed) 15 Jul 2015 [17:46:48], Pankaj Gupta wrote:
> We are arming timer when we get first request from guest.
> Even if guest pulls all the data we will be serving guest
> only when timer bumps up new quota. When timer expires
> we check if we have a pending request from guest, we
> serve it and re-arm the timer else we don't do any thing.
> 
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
>  hw/virtio/virtio-rng.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
> index ae04352..3d9a002 100644
> --- a/hw/virtio/virtio-rng.c
> +++ b/hw/virtio/virtio-rng.c
> @@ -142,9 +142,13 @@ static int virtio_rng_load(QEMUFile *f, void *opaque, int version_id)
>  static void check_rate_limit(void *opaque)
>  {
>      VirtIORNG *vrng = opaque;
> +    size_t size;
>  
>      vrng->quota_remaining = vrng->conf.max_bytes;
> -    virtio_rng_process(vrng);
> +    size = get_request_size(vrng->vq, 0);
> +    if (size > 0) {
> +        virtio_rng_process(vrng);
> +    }
>      vrng->activate_timer = true;

Ah, this isn't helping us much here; we might as well return in a
similar way from virtio_rng_process.

What I had written earlier was slightly different than this:

> So now with your changes, here is what we can do: instead of just
> activating the timer in check_rate_limit(), we can issue a
> get_request_size() call.  If the return value is > 0, it means we have
> a buffer queued up by the guest, and we can then call
> virtio_rng_process() and set activated to true.  Else, no need to call
> virtio_rng_process at all, and the job for the timer is done, since
> there are no more outstanding requests from the guest.

Anyway we can look at that later, patch 1 is already a big improvement
so I think we should just stick with that, and think about other
things in a different series.

Thanks,


		Amit

Re: [Qemu-devel] [PATCH 2/2 v3] virtio-rng: Serve pending request if any after timer bumps up quota.

[Pankaj Gupta]

----- Original Message -----
> From: "Amit Shah" <amit.shah@redhat.com>
> To: "Pankaj Gupta" <pagupta@redhat.com>
> Cc: qemu-devel@nongnu.org, mst@redhat.com
> Sent: Thursday, 16 July, 2015 11:35:36 AM
> Subject: Re: [Qemu-devel] [PATCH 2/2 v3] virtio-rng: Serve pending request if any after timer bumps up quota.
> 
> On (Wed) 15 Jul 2015 [17:46:48], Pankaj Gupta wrote:
> > We are arming timer when we get first request from guest.
> > Even if guest pulls all the data we will be serving guest
> > only when timer bumps up new quota. When timer expires
> > we check if we have a pending request from guest, we
> > serve it and re-arm the timer else we don't do any thing.
> > 
> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > ---
> >  hw/virtio/virtio-rng.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
> > index ae04352..3d9a002 100644
> > --- a/hw/virtio/virtio-rng.c
> > +++ b/hw/virtio/virtio-rng.c
> > @@ -142,9 +142,13 @@ static int virtio_rng_load(QEMUFile *f, void *opaque,
> > int version_id)
> >  static void check_rate_limit(void *opaque)
> >  {
> >      VirtIORNG *vrng = opaque;
> > +    size_t size;
> >  
> >      vrng->quota_remaining = vrng->conf.max_bytes;
> > -    virtio_rng_process(vrng);
> > +    size = get_request_size(vrng->vq, 0);
> > +    if (size > 0) {
> > +        virtio_rng_process(vrng);
> > +    }
> >      vrng->activate_timer = true;
> 
> Ah, this isn't helping us much here; we might as well return in a
> similar way from virtio_rng_process.
> 
> What I had written earlier was slightly different than this:
> 
> > So now with your changes, here is what we can do: instead of just
> > activating the timer in check_rate_limit(), we can issue a
> > get_request_size() call.  If the return value is > 0, it means we have
> > a buffer queued up by the guest, and we can then call
> > virtio_rng_process() and set activated to true.  Else, no need to call
> > virtio_rng_process at all, and the job for the timer is done, since
> > there are no more outstanding requests from the guest.
> 
> Anyway we can look at that later, patch 1 is already a big improvement
> so I think we should just stick with that, and think about other
> things in a different series.

Sure.

Thanks,
Pankaj
> 
> Thanks,
> 
> 
>                 Amit
> 
> 

Re: [Qemu-devel] [PATCH 2/2 v3] virtio-rng: Serve pending request if any after timer bumps up quota.

[Amit Shah]

On (Thu) 16 Jul 2015 [02:34:34], Pankaj Gupta wrote:
> > Anyway we can look at that later, patch 1 is already a big improvement
> > so I think we should just stick with that, and think about other
> > things in a different series.
> 
> Sure.

I think we can apply patch 1 for 2.4, since it reduces the wakeups we
cause.  If you can post powertop numbers, that'll be great.

I'll do a pull req in the meantime.

Thanks,

		Amit