From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47967) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZEWw6-0005Wd-2t for qemu-devel@nongnu.org; Mon, 13 Jul 2015 02:09:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZEWw1-0005zr-26 for qemu-devel@nongnu.org; Mon, 13 Jul 2015 02:09:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56227) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZEWw0-0005zn-Qc for qemu-devel@nongnu.org; Mon, 13 Jul 2015 02:09:48 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id E6DD8AB989 for ; Mon, 13 Jul 2015 06:09:47 +0000 (UTC) Date: Mon, 13 Jul 2015 11:39:38 +0530 From: Amit Shah Message-ID: <20150713060938.GA17241@grmbl.mre> References: <1436520840-28742-1-git-send-email-pagupta@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1436520840-28742-1-git-send-email-pagupta@redhat.com> 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: Pankaj Gupta Cc: qemu-devel@nongnu.org, mst@redhat.com On (Fri) 10 Jul 2015 [15:04:00], Pankaj Gupta wrote: > Timer 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. 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. > This resulted in triggring of timer even when quota > is not exhausted at all and resulting in extra processing. > > 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. Can you say how you tested this? Mainly interested in seeing the results in these cases: * No quota/timer specified on command line * Quota+timer specified on command line, and guest keeps asking host for unlimited entropy, e.g. by doing 'dd if=/dev/hwrng of=/dev/null' in the guest. * Ensure quota restrictions are maintained, and we're not giving more data than configured. 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. (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.) I tested one scenario using the trace events. With some quota and a timer value specified on the cmdline, before patch, I get tons of trace events before the guest is even up. After applying the patch, I don't get any trace events. So that's progress! I have one question: > Signed-off-by: Pankaj Gupta > --- > hw/virtio/virtio-rng.c | 15 ++++++++------- > include/hw/virtio/virtio-rng.h | 1 + > 2 files changed, 9 insertions(+), 7 deletions(-) > > 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) > return; > } > > + if (vrng->activate_timer) { > + timer_mod(vrng->rate_limit_timer, > + qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + vrng->conf.period_ms); > + vrng->activate_timer = false; > + } > + > if (vrng->quota_remaining < 0) { > quota = 0; > } else { > @@ -139,8 +145,7 @@ static void check_rate_limit(void *opaque) > > vrng->quota_remaining = vrng->conf.max_bytes; > virtio_rng_process(vrng); > - timer_mod(vrng->rate_limit_timer, > - qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + vrng->conf.period_ms); > + vrng->activate_timer = true; > } We're processing an older request first, and then firing the timer. What's the use of doing it this way? Why even do this? 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? Amit