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

[Pankaj Gupta <pagupta@redhat.com>]

> On (Mon) 13 Jul 2015 [04:01:01], 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.
> > 
> > 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.
> 
> Yes, other changes are fine too (in a different patch/series).
Sure. I will do this in separate patch/series.
> 
> > > > > * 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 <pagupta@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.
> > > 
> > > 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()'.
> 
> The purpose for the two calls will be different, it's fine to do that.
> 
> > 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.
> 
> Yes, sure.
> 
> It can be a separate patch in this series.

Sure, will do in separate patch and resubmit the series.

Thanks,
Pankaj
> 
> 		Amit
>