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

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

[Pankaj Gupta]

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

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;
 }
 
 static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
@@ -196,13 +201,9 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
 
     vrng->vq = virtio_add_queue(vdev, 8, handle_input);
     vrng->quota_remaining = vrng->conf.max_bytes;
-
     vrng->rate_limit_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
                                                check_rate_limit, vrng);
-
-    timer_mod(vrng->rate_limit_timer,
-                   qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + vrng->conf.period_ms);
-
+    vrng->activate_timer = true;
     register_savevm(dev, "virtio-rng", -1, 1, virtio_rng_save,
                     virtio_rng_load, vrng);
 }
diff --git a/include/hw/virtio/virtio-rng.h b/include/hw/virtio/virtio-rng.h
index 0316488..3f07de7 100644
--- a/include/hw/virtio/virtio-rng.h
+++ b/include/hw/virtio/virtio-rng.h
@@ -44,6 +44,7 @@ typedef struct VirtIORNG {
      */
     QEMUTimer *rate_limit_timer;
     int64_t quota_remaining;
+    bool activate_timer;
 } VirtIORNG;
 
 #endif
-- 
1.9.3

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

[Amit Shah]

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

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.

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

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

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

> 
> 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?
  No
> 
>                 Amit
> 
>

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

Makes sense?

		Amit

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

[Michael S. Tsirkin]

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

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

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

[Amit Shah]

On (Mon) 13 Jul 2015 [10:55:55], Michael S. Tsirkin wrote:
> On Mon, Jul 13, 2015 at 01:04:45PM +0530, Amit Shah wrote:

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

We do invoke virtio_rng_process in the load path, so that outstanding
requests are serviced.

Also, after migration, the guest will fetch data from the new host, so
the new host will start doing the rate-limit.  We can afford to not
carry over the rate-limiting state we had from the src host.


		Amit

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 <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()'.

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
>

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

[Amit Shah]

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

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

		Amit

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

[Pankaj Gupta]

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

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

[Michael S. Tsirkin]

On Fri, Jul 10, 2015 at 03:04:00PM +0530, Pankaj Gupta wrote:
> @@ -196,13 +201,9 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
>  
>      vrng->vq = virtio_add_queue(vdev, 8, handle_input);
>      vrng->quota_remaining = vrng->conf.max_bytes;
> -
>      vrng->rate_limit_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
>                                                 check_rate_limit, vrng);

BTW I'm not sure QEMU_CLOCK_VIRTUAL is right here.
We are trying to limit the rate at host.
Does it matter that guest is not running?

-- 
MST

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

[Pankaj Gupta]

> On Fri, Jul 10, 2015 at 03:04:00PM +0530, Pankaj Gupta wrote:
> > @@ -196,13 +201,9 @@ static void virtio_rng_device_realize(DeviceState
> > *dev, Error **errp)
> >  
> >      vrng->vq = virtio_add_queue(vdev, 8, handle_input);
> >      vrng->quota_remaining = vrng->conf.max_bytes;
> > -
> >      vrng->rate_limit_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
> >                                                 check_rate_limit, vrng);
> 
> BTW I'm not sure QEMU_CLOCK_VIRTUAL is right here.
> We are trying to limit the rate at host.
> Does it matter that guest is not running?

I think, now we will be serving entropy requests only when Guest send those.
In any case Guest should be up and running.
> 
> --
> MST
> 
>