[Qemu-devel] (no subject)

[Qemu-devel] (no subject)

[Pankaj Gupta]

Subject: [PATCH 0/2 v2] virtio-rng: Avoid uncessary timer trigger to bump up quota value 

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.

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         |   51 +++++++++++++++++++++++++----------------
 include/hw/virtio/virtio-rng.h |    1 
 2 files changed, 33 insertions(+), 19 deletions(-)

[Qemu-devel] [PATCH 1/2 v2] 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>
---
 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

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

[Amit Shah]

On (Tue) 14 Jul 2015 [13:03:09], Pankaj Gupta wrote:
> 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>

		Amit

[Qemu-devel] [PATCH 2/2 v2] 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 rearm the timer else we don't do any thing.

This patch also moves out 'request size' logic out to 
'check_request' function so that can be re-used.

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

diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index 8774a0c..dca5064 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -37,6 +37,24 @@ static size_t get_request_size(VirtQueue *vq, unsigned quota)
     return in;
 }
 
+static size_t check_request(VirtIORNG *vrng)
+{
+    size_t size;
+    unsigned quota;
+
+    if (vrng->quota_remaining < 0) {
+        quota = 0;
+    } else {
+        quota = MIN((uint64_t)vrng->quota_remaining, (uint64_t)UINT32_MAX);
+    }
+    size = get_request_size(vrng->vq, quota);
+
+    trace_virtio_rng_request(vrng, size, quota);
+
+    size = MIN(vrng->quota_remaining, size);
+    return size;
+}
+
 static void virtio_rng_process(VirtIORNG *vrng);
 
 /* Send data from a char device over to the guest */
@@ -72,7 +90,6 @@ static void chr_read(void *opaque, const void *buf, size_t size)
 static void virtio_rng_process(VirtIORNG *vrng)
 {
     size_t size;
-    unsigned quota;
 
     if (!is_guest_ready(vrng)) {
         return;
@@ -83,17 +100,8 @@ static void virtio_rng_process(VirtIORNG *vrng)
                    qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + vrng->conf.period_ms);
 	vrng->activate_timer = false;
     }
+    size = check_request(vrng);
 
-    if (vrng->quota_remaining < 0) {
-        quota = 0;
-    } else {
-        quota = MIN((uint64_t)vrng->quota_remaining, (uint64_t)UINT32_MAX);
-    }
-    size = get_request_size(vrng->vq, quota);
-
-    trace_virtio_rng_request(vrng, size, quota);
-
-    size = MIN(vrng->quota_remaining, size);
     if (size) {
         rng_backend_request_entropy(vrng->rng, size, chr_read, vrng);
     }
@@ -142,9 +150,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 = check_request(vrng);
+    if (size > 0) {
+        virtio_rng_process(vrng);
+    }
     vrng->activate_timer = true;
 }
 
-- 
1.9.3

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

[Amit Shah]

On (Tue) 14 Jul 2015 [13:03:10], 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 rearm the timer else we don't do any thing.
> 
> This patch also moves out 'request size' logic out to 
> 'check_request' function so that can be re-used.
> 
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
>  hw/virtio/virtio-rng.c | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
> index 8774a0c..dca5064 100644
> --- a/hw/virtio/virtio-rng.c
> +++ b/hw/virtio/virtio-rng.c
> @@ -37,6 +37,24 @@ static size_t get_request_size(VirtQueue *vq, unsigned quota)
>      return in;
>  }
>  
> +static size_t check_request(VirtIORNG *vrng)
> +{
> +    size_t size;
> +    unsigned quota;
> +
> +    if (vrng->quota_remaining < 0) {
> +        quota = 0;
> +    } else {
> +        quota = MIN((uint64_t)vrng->quota_remaining, (uint64_t)UINT32_MAX);
> +    }
> +    size = get_request_size(vrng->vq, quota);
> +
> +    trace_virtio_rng_request(vrng, size, quota);
> +
> +    size = MIN(vrng->quota_remaining, size);
> +    return size;
> +}
> +
>  static void virtio_rng_process(VirtIORNG *vrng);
>  
>  /* Send data from a char device over to the guest */
> @@ -72,7 +90,6 @@ static void chr_read(void *opaque, const void *buf, size_t size)
>  static void virtio_rng_process(VirtIORNG *vrng)
>  {
>      size_t size;
> -    unsigned quota;
>  
>      if (!is_guest_ready(vrng)) {
>          return;
> @@ -83,17 +100,8 @@ static void virtio_rng_process(VirtIORNG *vrng)
>                     qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + vrng->conf.period_ms);
>  	vrng->activate_timer = false;
>      }
> +    size = check_request(vrng);
>  
> -    if (vrng->quota_remaining < 0) {
> -        quota = 0;
> -    } else {
> -        quota = MIN((uint64_t)vrng->quota_remaining, (uint64_t)UINT32_MAX);
> -    }
> -    size = get_request_size(vrng->vq, quota);
> -
> -    trace_virtio_rng_request(vrng, size, quota);
> -
> -    size = MIN(vrng->quota_remaining, size);
>      if (size) {
>          rng_backend_request_entropy(vrng->rng, size, chr_read, vrng);
>      }
> @@ -142,9 +150,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 = check_request(vrng);
> +    if (size > 0) {
> +        virtio_rng_process(vrng);
> +    }

Why is it necessary to call check_request() here, instead of just
calling get_request_size()?  We want to call virtio_rng_process() in
case the guest has queued up another buffer, and for that,
get_request_size() is sufficient.  check_request() is anyway something
that virtio_rng_process() is going to do as soon as it's called...

		Amit

Re: [Qemu-devel] [PATCH 2/2 v2] 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: Wednesday, 15 July, 2015 12:09:57 PM
> Subject: Re: [Qemu-devel] [PATCH 2/2 v2] virtio-rng: Serve pending request if any after timer bumps up quota.
> 
> On (Tue) 14 Jul 2015 [13:03:10], 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 rearm the timer else we don't do any thing.
> > 
> > This patch also moves out 'request size' logic out to
> > 'check_request' function so that can be re-used.
> > 
> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > ---
> >  hw/virtio/virtio-rng.c | 36 ++++++++++++++++++++++++------------
> >  1 file changed, 24 insertions(+), 12 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
> > index 8774a0c..dca5064 100644
> > --- a/hw/virtio/virtio-rng.c
> > +++ b/hw/virtio/virtio-rng.c
> > @@ -37,6 +37,24 @@ static size_t get_request_size(VirtQueue *vq, unsigned
> > quota)
> >      return in;
> >  }
> >  
> > +static size_t check_request(VirtIORNG *vrng)
> > +{
> > +    size_t size;
> > +    unsigned quota;
> > +
> > +    if (vrng->quota_remaining < 0) {
> > +        quota = 0;
> > +    } else {
> > +        quota = MIN((uint64_t)vrng->quota_remaining,
> > (uint64_t)UINT32_MAX);
> > +    }
> > +    size = get_request_size(vrng->vq, quota);
> > +
> > +    trace_virtio_rng_request(vrng, size, quota);
> > +
> > +    size = MIN(vrng->quota_remaining, size);
> > +    return size;
> > +}
> > +
> >  static void virtio_rng_process(VirtIORNG *vrng);
> >  
> >  /* Send data from a char device over to the guest */
> > @@ -72,7 +90,6 @@ static void chr_read(void *opaque, const void *buf,
> > size_t size)
> >  static void virtio_rng_process(VirtIORNG *vrng)
> >  {
> >      size_t size;
> > -    unsigned quota;
> >  
> >      if (!is_guest_ready(vrng)) {
> >          return;
> > @@ -83,17 +100,8 @@ static void virtio_rng_process(VirtIORNG *vrng)
> >                     qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
> >                     vrng->conf.period_ms);
> >  	vrng->activate_timer = false;
> >      }
> > +    size = check_request(vrng);
> >  
> > -    if (vrng->quota_remaining < 0) {
> > -        quota = 0;
> > -    } else {
> > -        quota = MIN((uint64_t)vrng->quota_remaining,
> > (uint64_t)UINT32_MAX);
> > -    }
> > -    size = get_request_size(vrng->vq, quota);
> > -
> > -    trace_virtio_rng_request(vrng, size, quota);
> > -
> > -    size = MIN(vrng->quota_remaining, size);
> >      if (size) {
> >          rng_backend_request_entropy(vrng->rng, size, chr_read, vrng);
> >      }
> > @@ -142,9 +150,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 = check_request(vrng);
> > +    if (size > 0) {
> > +        virtio_rng_process(vrng);
> > +    }
> 
> Why is it necessary to call check_request() here, instead of just
> calling get_request_size()?  We want to call virtio_rng_process() in
> case the guest has queued up another buffer, and for that,
> get_request_size() is sufficient.  check_request() is anyway something
> that virtio_rng_process() is going to do as soon as it's called...

get_request_size calls 'virtqueue_get_avail_bytes'

both take quota, which we are setting to zero when 'vrng->quota_remaining' < 0.

virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
                               unsigned int *out_bytes,
                               unsigned max_in_bytes, unsigned max_out_bytes)

                                    |
                                     ----> quota

We read descriptors as soon as we get input request size > quota (0 for -ve quota_remaining)
if (in_total >= max_in_bytes && out_total >= max_out_bytes) {
                goto done;
            }

So, I thought better to keep same logic even if we get -ve quota value(which we might not get here)
and reuse the code.

> 
> 		Amit
> 
> 

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

[Amit Shah]

On (Wed) 15 Jul 2015 [03:05:06], Pankaj Gupta wrote:
> 
> 
> ----- Original Message -----
> > From: "Amit Shah" <amit.shah@redhat.com>
> > To: "Pankaj Gupta" <pagupta@redhat.com>
> > Cc: qemu-devel@nongnu.org, mst@redhat.com
> > Sent: Wednesday, 15 July, 2015 12:09:57 PM
> > Subject: Re: [Qemu-devel] [PATCH 2/2 v2] virtio-rng: Serve pending request if any after timer bumps up quota.
> > 
> > On (Tue) 14 Jul 2015 [13:03:10], 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 rearm the timer else we don't do any thing.
> > > 
> > > This patch also moves out 'request size' logic out to
> > > 'check_request' function so that can be re-used.
> > > 
> > > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > > ---
> > >  hw/virtio/virtio-rng.c | 36 ++++++++++++++++++++++++------------
> > >  1 file changed, 24 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
> > > index 8774a0c..dca5064 100644
> > > --- a/hw/virtio/virtio-rng.c
> > > +++ b/hw/virtio/virtio-rng.c
> > > @@ -37,6 +37,24 @@ static size_t get_request_size(VirtQueue *vq, unsigned
> > > quota)
> > >      return in;
> > >  }
> > >  
> > > +static size_t check_request(VirtIORNG *vrng)
> > > +{
> > > +    size_t size;
> > > +    unsigned quota;
> > > +
> > > +    if (vrng->quota_remaining < 0) {
> > > +        quota = 0;
> > > +    } else {
> > > +        quota = MIN((uint64_t)vrng->quota_remaining,
> > > (uint64_t)UINT32_MAX);
> > > +    }
> > > +    size = get_request_size(vrng->vq, quota);
> > > +
> > > +    trace_virtio_rng_request(vrng, size, quota);
> > > +
> > > +    size = MIN(vrng->quota_remaining, size);
> > > +    return size;
> > > +}
> > > +
> > >  static void virtio_rng_process(VirtIORNG *vrng);
> > >  
> > >  /* Send data from a char device over to the guest */
> > > @@ -72,7 +90,6 @@ static void chr_read(void *opaque, const void *buf,
> > > size_t size)
> > >  static void virtio_rng_process(VirtIORNG *vrng)
> > >  {
> > >      size_t size;
> > > -    unsigned quota;
> > >  
> > >      if (!is_guest_ready(vrng)) {
> > >          return;
> > > @@ -83,17 +100,8 @@ static void virtio_rng_process(VirtIORNG *vrng)
> > >                     qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
> > >                     vrng->conf.period_ms);
> > >  	vrng->activate_timer = false;
> > >      }
> > > +    size = check_request(vrng);
> > >  
> > > -    if (vrng->quota_remaining < 0) {
> > > -        quota = 0;
> > > -    } else {
> > > -        quota = MIN((uint64_t)vrng->quota_remaining,
> > > (uint64_t)UINT32_MAX);
> > > -    }
> > > -    size = get_request_size(vrng->vq, quota);
> > > -
> > > -    trace_virtio_rng_request(vrng, size, quota);
> > > -
> > > -    size = MIN(vrng->quota_remaining, size);
> > >      if (size) {
> > >          rng_backend_request_entropy(vrng->rng, size, chr_read, vrng);
> > >      }
> > > @@ -142,9 +150,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 = check_request(vrng);
> > > +    if (size > 0) {
> > > +        virtio_rng_process(vrng);
> > > +    }
> > 
> > Why is it necessary to call check_request() here, instead of just
> > calling get_request_size()?  We want to call virtio_rng_process() in
> > case the guest has queued up another buffer, and for that,
> > get_request_size() is sufficient.  check_request() is anyway something
> > that virtio_rng_process() is going to do as soon as it's called...
> 
> get_request_size calls 'virtqueue_get_avail_bytes'
> 
> both take quota, which we are setting to zero when 'vrng->quota_remaining' < 0.

Point is we get called in this function when the timer expires, and
we're re-setting the quota value to the configured value.  So all the
other calculations are useless for determining whether we have a
buffer queued up or not.  The other calculations are going to be made
immediately afterwards in the virtio_rng_process() function.

> virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
>                                unsigned int *out_bytes,
>                                unsigned max_in_bytes, unsigned max_out_bytes)
> 
>                                     |
>                                      ----> quota
> 
> We read descriptors as soon as we get input request size > quota (0 for -ve quota_remaining)
> if (in_total >= max_in_bytes && out_total >= max_out_bytes) {
>                 goto done;
>             }
> 
> So, I thought better to keep same logic even if we get -ve quota value(which we might not get here)
> and reuse the code.

Not much benefit in executing the same code twice in succession.  Our
only goal here is to determine whether we have a pending buffer in the
vq.

		Amit

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

[Pankaj Gupta]

> On (Wed) 15 Jul 2015 [03:05:06], Pankaj Gupta wrote:
> > 
> > 
> > ----- Original Message -----
> > > From: "Amit Shah" <amit.shah@redhat.com>
> > > To: "Pankaj Gupta" <pagupta@redhat.com>
> > > Cc: qemu-devel@nongnu.org, mst@redhat.com
> > > Sent: Wednesday, 15 July, 2015 12:09:57 PM
> > > Subject: Re: [Qemu-devel] [PATCH 2/2 v2] virtio-rng: Serve pending
> > > request if any after timer bumps up quota.
> > > 
> > > On (Tue) 14 Jul 2015 [13:03:10], 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 rearm the timer else we don't do any thing.
> > > > 
> > > > This patch also moves out 'request size' logic out to
> > > > 'check_request' function so that can be re-used.
> > > > 
> > > > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > > > ---
> > > >  hw/virtio/virtio-rng.c | 36 ++++++++++++++++++++++++------------
> > > >  1 file changed, 24 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
> > > > index 8774a0c..dca5064 100644
> > > > --- a/hw/virtio/virtio-rng.c
> > > > +++ b/hw/virtio/virtio-rng.c
> > > > @@ -37,6 +37,24 @@ static size_t get_request_size(VirtQueue *vq,
> > > > unsigned
> > > > quota)
> > > >      return in;
> > > >  }
> > > >  
> > > > +static size_t check_request(VirtIORNG *vrng)
> > > > +{
> > > > +    size_t size;
> > > > +    unsigned quota;
> > > > +
> > > > +    if (vrng->quota_remaining < 0) {
> > > > +        quota = 0;
> > > > +    } else {
> > > > +        quota = MIN((uint64_t)vrng->quota_remaining,
> > > > (uint64_t)UINT32_MAX);
> > > > +    }
> > > > +    size = get_request_size(vrng->vq, quota);
> > > > +
> > > > +    trace_virtio_rng_request(vrng, size, quota);
> > > > +
> > > > +    size = MIN(vrng->quota_remaining, size);
> > > > +    return size;
> > > > +}
> > > > +
> > > >  static void virtio_rng_process(VirtIORNG *vrng);
> > > >  
> > > >  /* Send data from a char device over to the guest */
> > > > @@ -72,7 +90,6 @@ static void chr_read(void *opaque, const void *buf,
> > > > size_t size)
> > > >  static void virtio_rng_process(VirtIORNG *vrng)
> > > >  {
> > > >      size_t size;
> > > > -    unsigned quota;
> > > >  
> > > >      if (!is_guest_ready(vrng)) {
> > > >          return;
> > > > @@ -83,17 +100,8 @@ static void virtio_rng_process(VirtIORNG *vrng)
> > > >                     qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
> > > >                     vrng->conf.period_ms);
> > > >          vrng->activate_timer = false;
> > > >      }
> > > > +    size = check_request(vrng);
> > > >  
> > > > -    if (vrng->quota_remaining < 0) {
> > > > -        quota = 0;
> > > > -    } else {
> > > > -        quota = MIN((uint64_t)vrng->quota_remaining,
> > > > (uint64_t)UINT32_MAX);
> > > > -    }
> > > > -    size = get_request_size(vrng->vq, quota);
> > > > -
> > > > -    trace_virtio_rng_request(vrng, size, quota);
> > > > -
> > > > -    size = MIN(vrng->quota_remaining, size);
> > > >      if (size) {
> > > >          rng_backend_request_entropy(vrng->rng, size, chr_read, vrng);
> > > >      }
> > > > @@ -142,9 +150,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 = check_request(vrng);
> > > > +    if (size > 0) {
> > > > +        virtio_rng_process(vrng);
> > > > +    }
> > > 
> > > Why is it necessary to call check_request() here, instead of just
> > > calling get_request_size()?  We want to call virtio_rng_process() in
> > > case the guest has queued up another buffer, and for that,
> > > get_request_size() is sufficient.  check_request() is anyway something
> > > that virtio_rng_process() is going to do as soon as it's called...
> > 
> > get_request_size calls 'virtqueue_get_avail_bytes'
> > 
> > both take quota, which we are setting to zero when 'vrng->quota_remaining'
> > < 0.
> 
> Point is we get called in this function when the timer expires, and
> we're re-setting the quota value to the configured value.  So all the
> other calculations are useless for determining whether we have a
> buffer queued up or not.  The other calculations are going to be made
> immediately afterwards in the virtio_rng_process() function.

yes. I agree.
> 
> > virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
> >                                unsigned int *out_bytes,
> >                                unsigned max_in_bytes, unsigned
> >                                max_out_bytes)
> > 
> >                                     |
> >                                      ----> quota
> > 
> > We read descriptors as soon as we get input request size > quota (0 for -ve
> > quota_remaining)
> > if (in_total >= max_in_bytes && out_total >= max_out_bytes) {
> >                 goto done;
> >             }
> > 
> > So, I thought better to keep same logic even if we get -ve quota
> > value(which we might not get here)
> > and reuse the code.
> 
> Not much benefit in executing the same code twice in succession.  Our
> only goal here is to determine whether we have a pending buffer in the
> vq.

o.k. I will use 'get_request_size' here and resubmit patch.

Thanks,
Pankaj
> 
>                 Amit
>