From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54327) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZEXcX-0004sH-K4 for qemu-devel@nongnu.org; Mon, 13 Jul 2015 02:53:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZEXcR-0004Zm-CG for qemu-devel@nongnu.org; Mon, 13 Jul 2015 02:53:45 -0400 Received: from mx5-phx2.redhat.com ([209.132.183.37]:49272) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZEXcR-0004Ze-4q for qemu-devel@nongnu.org; Mon, 13 Jul 2015 02:53:39 -0400 Date: Mon, 13 Jul 2015 02:53:36 -0400 (EDT) From: Pankaj Gupta Message-ID: <2021183425.31524217.1436770416516.JavaMail.zimbra@redhat.com> In-Reply-To: <20150713060938.GA17241@grmbl.mre> References: <1436520840-28742-1-git-send-email-pagupta@redhat.com> <20150713060938.GA17241@grmbl.mre> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] virtio-rng: Bump up quota value only when guest requests entropy List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amit Shah Cc: qemu-devel@nongnu.org, mst@redhat.com Hi Amit, Thanks for the review. >=20 > On (Fri) 10 Jul 2015 [15:04:00], Pankaj Gupta wrote: > > =C2=A0 =C2=A0Timer was added in virtio-rng to rate limit the > > entropy. It used to trigger at regular intervals to > > bump up the quota value. The value of quota and timer > > slice is decided based on entropy source rate in host. >=20 > It doesn't necessarily depnd on the source rate in the host - all we > want the quota+timer to do is to limit the amount of data a guest can > take from the host - to ensure one (potentially rogue) guest does not > use up all the entropy from the host. Sorry! for not being clear on this. By rate limit I meant same. I used a broader term. >=20 > > This resulted in triggring of timer even when quota > > is not exhausted at all and resulting in extra processing. > >=20 > > This patch triggers timer only when guest requests for > > entropy. As soon as first request from guest for entropy > > comes we set the timer. Timer bumps up the quota value > > when it gets triggered. >=20 > Can you say how you tested this? >=20 > Mainly interested in seeing the results in these cases: >=20 > * No quota/timer specified on command line =C2=A0=C2=A0 =C2=A0Tested this scenario. I am setting timer when first requ= est comes. =C2=A0=C2=A0 =C2=A0So, timer gets fired after (1 << 16) ms time.=20 > * Quota+timer specified on command line, and guest keeps asking host > =C2=A0 for unlimited entropy, e.g. by doing 'dd if=3D/dev/hwrng of=3D/dev= /null' > =C2=A0 in the guest. =C2=A0=C2=A0 =C2=A0I did not do =C2=A0'dd if=3D/dev/hwrng of=3D/dev/null'. =C2=A0=C2=A0 =C2=A0Did cat '/dev/hwrng' && '/dev/random' > * Ensure quota restrictions are maintained, and we're not giving more > =C2=A0 data than configured. =C2=A0=C2=A0 =C2=A0Ensured. We are either giving < =3D requested data >=20 > For these tests, it's helpful to use the host's /dev/urandom as the > source, since that can give data faster to the guest than the default > /dev/random. =C2=A0(Otherwise, if the host itself blocks on /dev/random, > the guest may not get entropy due to that reason vs it not getting > entropy due to rate-limiting.) =C2=A0=C2=A0Agree. =C2=A0=C2=A0Will test this as well. >=20 > I tested one scenario using the trace events. =C2=A0With some quota and a > timer value specified on the cmdline, before patch, I get tons of > trace events before the guest is even up. =C2=A0After applying the patch,= I > don't get any trace events. =C2=A0So that's progress! Thanks. >=20 > I have one question: >=20 > > Signed-off-by: Pankaj Gupta > > --- > > =C2=A0hw/virtio/virtio-rng.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 | 15 ++++++++-= ------ > > =C2=A0include/hw/virtio/virtio-rng.h | =C2=A01 + > > =C2=A02 files changed, 9 insertions(+), 7 deletions(-) > >=20 > > diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c > > index 22b1d87..8774a0c 100644 > > --- a/hw/virtio/virtio-rng.c > > +++ b/hw/virtio/virtio-rng.c > > @@ -78,6 +78,12 @@ static void virtio_rng_process(VirtIORNG *vrng) > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return; > > =C2=A0 =C2=A0 =C2=A0} > > =C2=A0 > > + =C2=A0 =C2=A0if (vrng->activate_timer) { > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0timer_mod(vrng->rate_limit_timer, > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_c= lock_get_ms(QEMU_CLOCK_VIRTUAL) + > > vrng->conf.period_ms); > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0vrng->activate_timer =3D false; > > + =C2=A0 =C2=A0} > > + > > =C2=A0 =C2=A0 =C2=A0if (vrng->quota_remaining < 0) { > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0quota =3D 0; > > =C2=A0 =C2=A0 =C2=A0} else { > > @@ -139,8 +145,7 @@ static void check_rate_limit(void *opaque) > > =C2=A0 > > =C2=A0 =C2=A0 =C2=A0vrng->quota_remaining =3D vrng->conf.max_bytes; > > =C2=A0 =C2=A0 =C2=A0virtio_rng_process(vrng); > > - =C2=A0 =C2=A0timer_mod(vrng->rate_limit_timer, > > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_c= lock_get_ms(QEMU_CLOCK_VIRTUAL) + > > vrng->conf.period_ms); > > + =C2=A0 =C2=A0vrng->activate_timer =3D true; > > =C2=A0} >=20 > We're processing an older request first, and then firing the timer. > What's the use of doing it this way? =C2=A0Why even do this? I also had this query. If we don't call this after resetting 'vrng->quota_r= emaining'=20 further requests does not work. It looks to me some limitation in earlier c= ode when=20 'vrng->quota_remaining' goes to < =3D 0. A self request is needed to reset = things. I will try to find the answer. >=20 > I know this is how the code was written originally, but since you've > looked at it, do you know why this is the way it is? =C2=A0=C2=A0No >=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 >