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

[Pankaj Gupta]

> > 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.

o.k.
> 
> > > > 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.

In my current logic. That would avoid quota to refill if timeslice/quota 
expires once.
We already had default values of timeslice/quota. 

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.

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.
> 
> > > * 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.

sure. Will do this.
> 
> > > * 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.

Only thing which stopped me was :

I did not want to use/call 'get_request_size()' twice (duplicate code).
And I don't want to change 'virtio_rng_process()'.

Even if I provide size in virtio_rng_process(), this interface is being used
at multiple places.

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.

> 
> Makes sense?
yes. :)
> 
>                 Amit
>