From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34866) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZEZZe-0000DP-Uz for qemu-devel@nongnu.org; Mon, 13 Jul 2015 04:58:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZEZZZ-0002WY-Ng for qemu-devel@nongnu.org; Mon, 13 Jul 2015 04:58:54 -0400 Received: from mx4-phx2.redhat.com ([209.132.183.25]:46103) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZEZZZ-0002WO-Dw for qemu-devel@nongnu.org; Mon, 13 Jul 2015 04:58:49 -0400 Date: Mon, 13 Jul 2015 04:58:47 -0400 (EDT) From: Pankaj Gupta Message-ID: <1088791981.31627608.1436777927219.JavaMail.zimbra@redhat.com> In-Reply-To: <20150713084738.GA10280@grmbl.mre> References: <1436520840-28742-1-git-send-email-pagupta@redhat.com> <20150713060938.GA17241@grmbl.mre> <2021183425.31524217.1436770416516.JavaMail.zimbra@redhat.com> <20150713073445.GB17241@grmbl.mre> <1500679824.31589674.1436774461897.JavaMail.zimbra@redhat.com> <20150713084738.GA10280@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 > On (Mon) 13 Jul 2015 [04:01:01], Pankaj Gupta wrote: > >=20 > > > > Hi Amit, > > > >=20 > > > > Thanks for the review. > > > >=20 > > > > >=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. > > > >=20 > > > > Sorry! for not being clear on this. By rate limit I meant same. > > > > I used a broader term. > > >=20 > > > My comment was to the 'value of quota and timer slice is decided base= d > > > on entropy source rate in host' - admins will usually not decide base= d > > > on what sources the host has - they will typically decide how much a > > > guest is supposed to consume. > >=20 > > o.k. > > >=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 fi= rst request comes. > > > > =C2=A0=C2=A0 =C2=A0So, timer gets fired after (1 << 16) ms time. > > >=20 > > > But in case a quota or a timer isn't specified, the timer shouldn't b= e > > > armed in the first place. > >=20 > > In my current logic. That would avoid quota to refill if timeslice/quot= a > > expires once. > > We already had default values of timeslice/quota. > >=20 > > If we go ahead to remove default values we need some number to do the > > check. Separate > > that from check for user provided number because user can also use same > > number and if it is > > -ve it will fail user value validation check. > >=20 > > If we have to think about all this, there will be some more changes. So= , no > > time slice default timer > > was simpler of all and not have big impact. timer is firing in (1 << 16= ) > > ms. >=20 > Yes, other changes are fine too (in a different patch/series). Sure. I will do this in separate patch/series. >=20 > > > > > * Quota+timer specified on command line, and guest keeps asking h= ost > > > > > =C2=A0 for unlimited entropy, e.g. by doing 'dd if=3D/dev/hwrng > > > > > =C2=A0 of=3D/dev/null' > > > > > =C2=A0 in the guest. > > > >=20 > > > > =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' > > >=20 > > > OK - that's similar. =C2=A0I like dd because when dd is stopped, it g= ives > > > the rate at which data was received, so it helps in seeing if we're > > > getting more rate than what was specified on the cmdline. > >=20 > > sure. Will do this. > > >=20 > > > > > * 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 da= ta > > > > >=20 > > > > > For these tests, it's helpful to use the host's /dev/urandom as t= he > > > > > source, since that can give data faster to the guest than the def= ault > > > > > /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 gettin= g > > > > > entropy due to rate-limiting.) > > > >=20 > > > > =C2=A0=C2=A0Agree. > > > > =C2=A0=C2=A0Will test this as well. > > > >=20 > > > > >=20 > > > > > I tested one scenario using the trace events. =C2=A0With some quo= ta 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 th= e patch, > > > > > I > > > > > don't get any trace events. =C2=A0So that's progress! > > > >=20 > > > > 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 *vr= ng) > > > > > > =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_clock_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_by= tes; > > > > > > =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_clock_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 time= r. > > > > > What's the use of doing it this way? =C2=A0Why even do this? > > > >=20 > > > > I also had this query. If we don't call this after resetting > > > > 'vrng->quota_remaining' > > > > further requests does not work. It looks to me some limitation in > > > > earlier > > > > code when > > > > 'vrng->quota_remaining' goes to < =3D 0. A self request is needed t= o > > > > reset > > > > things. > > > >=20 > > > > I will try to find the answer. > > >=20 > > > OK so I actually read through the thing, and this is useful for such = a > > > scenario: > > >=20 > > > assume our rate-limit is at 4KB/s. > > >=20 > > > * guest queues up multiple requests, say 4KB, 8KB, 12KB. > > > * we will serve the first request in the queue, which is 4KB. > > > * then, check_rate_limit() is triggered, and we serve the 2nd request= . > > > * since the 2nd request is for 8KB, but we can only give 4KB in 1 > > > =C2=A0 second, we only give the guest 4KB. =C2=A0We then re-arm the t= imer so > > > =C2=A0 that we can get to the next request in the list. =C2=A0Without= this > > > =C2=A0 re-arming, the already-queued request will not get attention (= till > > > =C2=A0 the next time the guest writes something to the queue. > > > * we then serve the 3rd request for 12KB, again with 4KB of data. > > >=20 > > > One thing to observe here is that we just service the minimum data we > > > can without waiting to service the entire request (i.e. we give the > > > guest 4KB of data, and not 8 or 12 KB. =C2=A0We could do that by wait= ing > > > for the timer to expire, and then servicing the entire request. > > > This current way is simpler, and better. =C2=A0If the guest isn't hap= py to > > > receive just 4KB when it asked for 12, it can ask again. > > >=20 > > > That also saves us the trouble with live migration: if we hold onto a > > > buffer, that becomes state, and we'll have to migrate it as well. > > > This current model saves us from doing that. > > >=20 > > > And now that I type all this, I recall having thought about these > > > things initially. =C2=A0Don't know if I wrote it up somewhere, or if = there > > > are email conversations on this subject... > > >=20 > > > 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. =C2=A0If the return value is > 0, it means w= e have > > > a buffer queued up by the guest, and we can then call > >=20 > > > virtio_rng_process() and set activated to true. =C2=A0Else, no need t= o call > > > virtio_rng_process at all, and the job for the timer is done, since > > > there are no more outstanding requests from the guest. > >=20 > > Only thing which stopped me was : > >=20 > > I did not want to use/call 'get_request_size()' twice (duplicate code). > > And I don't want to change 'virtio_rng_process()'. >=20 > The purpose for the two calls will be different, it's fine to do that. >=20 > > Even if I provide size in virtio_rng_process(), this interface is being > > used > > at multiple places. > >=20 > > If you are o.k I can call 'get_request_size()' in 'check_rate_limit()' = and > > avoid > > timer reset if no request. But I agree this will be more optimised. >=20 > Yes, sure. >=20 > It can be a separate patch in this series. Sure, will do in separate patch and resubmit the series. Thanks, Pankaj >=20 > =09=09Amit >=20