On Mon, Jul 13, 2015 at 01:04:45PM +0530, Amit Shah wrote:
> On (Mon) 13 Jul 2015 [02:53:36], Pankaj Gupta wrote:
> > Hi Amit,
> >
> > Thanks for the review.
> >
> > >
> > > 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.
> >
> > Sorry! for not being clear on this. By rate limit I meant same.
> > I used a broader term.
>
> My comment was to the 'value of quota and timer slice is decided based
> on entropy source rate in host' - admins will usually not decide based
> on what sources the host has - they will typically decide how much a
> guest is supposed to consume.
>
> > > > 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
> > Tested this scenario. I am setting timer when first request comes.
> > So, timer gets fired after (1 << 16) ms time.
>
> But in case a quota or a timer isn't specified, the timer shouldn't be
> armed in the first place.
>
> > > * 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.
> >
> > I did not do 'dd if=/dev/hwrng of=/dev/null'.
> > Did cat '/dev/hwrng' && '/dev/random'
>
> OK - that's similar. I like dd because when dd is stopped, it gives
> 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.
>
> > > * Ensure quota restrictions are maintained, and we're not giving more
> > > data than configured.
> > Ensured. We are either giving < = requested data
> > >
> > > 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.)
> >
> > Agree.
> > Will test this as well.
> >
> > >
> > > 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!
> >
> > Thanks.
> > >
> > > I have one question:
> > >
> > > > Signed-off-by: Pankaj Gupta <pagu...@redhat.com>
> > > > ---
> > > > 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 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 < = 0. A self request is needed to reset
> > things.
> >
> > I will try to find the answer.
>
> OK so I actually read through the thing, and this is useful for such a
> scenario:
>
> assume our rate-limit is at 4KB/s.
>
> * 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
> second, we only give the guest 4KB. We then re-arm the timer so
> that we can get to the next request in the list. Without this
> re-arming, the already-queued request will not get attention (till
> the next time the guest writes something to the queue.
> * we then serve the 3rd request for 12KB, again with 4KB of data.
>
> 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. We could do that by waiting
> for the timer to expire, and then servicing the entire request.
> This current way is simpler, and better. If the guest isn't happy to
> receive just 4KB when it asked for 12, it can ask again.
>
> 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.
Timers hold state and have to be migrated, this seems to be missing
here.
> And now that I type all this, I recall having thought about these
> things initially. Don't know if I wrote it up somewhere, or if there
> are email conversations on this subject...
>
> 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. If the return value is > 0, it means we have
> a buffer queued up by the guest, and we can then call
> virtio_rng_process() and set activated to true. Else, no need to call
> virtio_rng_process at all, and the job for the timer is done, since
> there are no more outstanding requests from the guest.
>
> Makes sense?
>
> Amit
