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