From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58995) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZFGki-0005yV-Vw for qemu-devel@nongnu.org; Wed, 15 Jul 2015 03:05:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZFGke-0000do-Db for qemu-devel@nongnu.org; Wed, 15 Jul 2015 03:05:12 -0400 Received: from mx5-phx2.redhat.com ([209.132.183.37]:52792) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZFGke-0000dA-5x for qemu-devel@nongnu.org; Wed, 15 Jul 2015 03:05:08 -0400 Date: Wed, 15 Jul 2015 03:05:06 -0400 (EDT) From: Pankaj Gupta Message-ID: <805915598.33089083.1436943906471.JavaMail.zimbra@redhat.com> In-Reply-To: <20150715063957.GL10280@grmbl.mre> References: <1436859190-20002-1-git-send-email-pagupta@redhat.com> <1436859190-20002-3-git-send-email-pagupta@redhat.com> <20150715063957.GL10280@grmbl.mre> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2 v2] virtio-rng: Serve pending request if any after timer bumps up quota. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amit Shah Cc: qemu-devel@nongnu.org, mst@redhat.com ----- Original Message ----- > From: "Amit Shah" > To: "Pankaj Gupta" > 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 > > --- > > 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 > >