Re: [Qemu-devel] [PATCH] virtio-rng: Bump up quota value only when guest requests entropy

[Amit Shah]

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 <address@hidden>
> > > ---
> > >  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.

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