From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53990) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZFIyi-0007gH-PC for qemu-devel@nongnu.org; Wed, 15 Jul 2015 05:27:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZFIyd-00051U-Ox for qemu-devel@nongnu.org; Wed, 15 Jul 2015 05:27:48 -0400 Received: from mx3-phx2.redhat.com ([209.132.183.24]:59782) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZFIyd-00050q-89 for qemu-devel@nongnu.org; Wed, 15 Jul 2015 05:27:43 -0400 Date: Wed, 15 Jul 2015 05:27:41 -0400 (EDT) From: Pankaj Gupta Message-ID: <2056567138.33277675.1436952461823.JavaMail.zimbra@redhat.com> In-Reply-To: <20150715081518.GQ10280@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> <805915598.33089083.1436943906471.JavaMail.zimbra@redhat.com> <20150715081518.GQ10280@grmbl.mre> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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 > On (Wed) 15 Jul 2015 [03:05:06], Pankaj Gupta wrote: > >=20 > >=20 > > ----- 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. > > >=20 > > > 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. > > > >=20 > > > > This patch also moves out 'request size' logic out to > > > > 'check_request' function so that can be re-used. > > > >=20 > > > > Signed-off-by: Pankaj Gupta > > > > --- > > > > =C2=A0hw/virtio/virtio-rng.c | 36 ++++++++++++++++++++++++---------= --- > > > > =C2=A01 file changed, 24 insertions(+), 12 deletions(-) > > > >=20 > > > > 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) > > > > =C2=A0 =C2=A0 =C2=A0return in; > > > > =C2=A0} > > > > =C2=A0 > > > > +static size_t check_request(VirtIORNG *vrng) > > > > +{ > > > > + =C2=A0 =C2=A0size_t size; > > > > + =C2=A0 =C2=A0unsigned quota; > > > > + > > > > + =C2=A0 =C2=A0if (vrng->quota_remaining < 0) { > > > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0quota =3D 0; > > > > + =C2=A0 =C2=A0} else { > > > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0quota =3D MIN((uint64_t)vrng->quota_re= maining, > > > > (uint64_t)UINT32_MAX); > > > > + =C2=A0 =C2=A0} > > > > + =C2=A0 =C2=A0size =3D get_request_size(vrng->vq, quota); > > > > + > > > > + =C2=A0 =C2=A0trace_virtio_rng_request(vrng, size, quota); > > > > + > > > > + =C2=A0 =C2=A0size =3D MIN(vrng->quota_remaining, size); > > > > + =C2=A0 =C2=A0return size; > > > > +} > > > > + > > > > =C2=A0static void virtio_rng_process(VirtIORNG *vrng); > > > > =C2=A0 > > > > =C2=A0/* Send data from a char device over to the guest */ > > > > @@ -72,7 +90,6 @@ static void chr_read(void *opaque, const void *bu= f, > > > > size_t size) > > > > =C2=A0static void virtio_rng_process(VirtIORNG *vrng) > > > > =C2=A0{ > > > > =C2=A0 =C2=A0 =C2=A0size_t size; > > > > - =C2=A0 =C2=A0unsigned quota; > > > > =C2=A0 > > > > =C2=A0 =C2=A0 =C2=A0if (!is_guest_ready(vrng)) { > > > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return; > > > > @@ -83,17 +100,8 @@ static void virtio_rng_process(VirtIORNG *vrng) > > > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + > > > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 vrng->conf.period_ms); > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vrng->activat= e_timer =3D false; > > > > =C2=A0 =C2=A0 =C2=A0} > > > > + =C2=A0 =C2=A0size =3D check_request(vrng); > > > > =C2=A0 > > > > - =C2=A0 =C2=A0if (vrng->quota_remaining < 0) { > > > > - =C2=A0 =C2=A0 =C2=A0 =C2=A0quota =3D 0; > > > > - =C2=A0 =C2=A0} else { > > > > - =C2=A0 =C2=A0 =C2=A0 =C2=A0quota =3D MIN((uint64_t)vrng->quota_re= maining, > > > > (uint64_t)UINT32_MAX); > > > > - =C2=A0 =C2=A0} > > > > - =C2=A0 =C2=A0size =3D get_request_size(vrng->vq, quota); > > > > - > > > > - =C2=A0 =C2=A0trace_virtio_rng_request(vrng, size, quota); > > > > - > > > > - =C2=A0 =C2=A0size =3D MIN(vrng->quota_remaining, size); > > > > =C2=A0 =C2=A0 =C2=A0if (size) { > > > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0rng_backend_request_entropy(vrng-= >rng, size, chr_read, vrng); > > > > =C2=A0 =C2=A0 =C2=A0} > > > > @@ -142,9 +150,13 @@ static int virtio_rng_load(QEMUFile *f, void > > > > *opaque, > > > > int version_id) > > > > =C2=A0static void check_rate_limit(void *opaque) > > > > =C2=A0{ > > > > =C2=A0 =C2=A0 =C2=A0VirtIORNG *vrng =3D opaque; > > > > + =C2=A0 =C2=A0size_t size; > > > > =C2=A0 > > > > =C2=A0 =C2=A0 =C2=A0vrng->quota_remaining =3D vrng->conf.max_bytes; > > > > - =C2=A0 =C2=A0virtio_rng_process(vrng); > > > > + =C2=A0 =C2=A0size =3D check_request(vrng); > > > > + =C2=A0 =C2=A0if (size > 0) { > > > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0virtio_rng_process(vrng); > > > > + =C2=A0 =C2=A0} > > >=20 > > > Why is it necessary to call check_request() here, instead of just > > > calling get_request_size()? =C2=A0We want to call virtio_rng_process(= ) in > > > case the guest has queued up another buffer, and for that, > > > get_request_size() is sufficient. =C2=A0check_request() is anyway som= ething > > > that virtio_rng_process() is going to do as soon as it's called... > >=20 > > get_request_size calls 'virtqueue_get_avail_bytes' > >=20 > > both take quota, which we are setting to zero when 'vrng->quota_remaini= ng' > > < 0. >=20 > Point is we get called in this function when the timer expires, and > we're re-setting the quota value to the configured value. =C2=A0So all th= e > other calculations are useless for determining whether we have a > buffer queued up or not. =C2=A0The other calculations are going to be mad= e > immediately afterwards in the virtio_rng_process() function. yes. I agree. >=20 > > virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes, > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned int *out_bytes, > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned max_in_bytes, unsigned > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0max_out_bytes) > >=20 > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0----> quota > >=20 > > We read descriptors as soon as we get input request size > quota (0 for= -ve > > quota_remaining) > > if (in_total >=3D max_in_bytes && out_total >=3D max_out_bytes) { > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto done; > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 } > >=20 > > 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. >=20 > Not much benefit in executing the same code twice in succession. =C2=A0Ou= r > 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 >=20 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0Amit >=20