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

[Pankaj Gupta <pagupta@redhat.com>]

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