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

[Amit Shah <amit.shah@redhat.com>]

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