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

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(-)

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

----- 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
> 
>

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