[Qemu-devel] [PATCH RFC 0/2] improve rng-egd perf

[Qemu-devel] [PATCH RFC 0/2] improve rng-egd perf

[Amos Kong]

This patchset try to improve rng-egd backends performance by
pre-reading data to buffer from egd socket.

We can't improve the performance by too large buffer, so I set the
patchset as RFC. We might have better solution for this.

Amos Kong (2):
  rng-egd: improve egd backend performance
  rng-egd: introduce a parameter to set buffer size

 backends/rng-egd.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 129 insertions(+), 20 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH RFC 1/2] rng-egd: improve egd backend performance

[Amos Kong]

Bugzilla: https://bugs.launchpad.net/qemu/+bug/1253563

We have a requests queue to cache the random data, but the second
will come in when the first request is returned, so we always
only have one items in the queue. It effects the performance.

This patch changes the IOthread to fill a fixed buffer with
random data from egd socket, request_entropy() will return
data to virtio queue if buffer has available data.

(test with a fast source, disguised egd socket)
 # cat /dev/urandom | nc -l localhost 8003
 # qemu .. -chardev socket,host=localhost,port=8003,id=chr0 \
        -object rng-egd,chardev=chr0,id=rng0,buf_size=1024 \
        -device virtio-rng-pci,rng=rng0

  bytes     kb/s
  ------    ----
  131072 ->  835
   65536 ->  652
   32768 ->  356
   16384 ->  182
    8192 ->   99
    4096 ->   52
    2048 ->   30
    1024 ->   15
     512 ->    8
     256 ->    4
     128 ->    3
      64 ->    2

Signed-off-by: Amos Kong <akong@redhat.com>
---
 backends/rng-egd.c | 131 +++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 111 insertions(+), 20 deletions(-)

diff --git a/backends/rng-egd.c b/backends/rng-egd.c
index 62226d5..d317c61 100644
--- a/backends/rng-egd.c
+++ b/backends/rng-egd.c
@@ -18,6 +18,8 @@
 #define TYPE_RNG_EGD "rng-egd"
 #define RNG_EGD(obj) OBJECT_CHECK(RngEgd, (obj), TYPE_RNG_EGD)
 
+#define BUFFER_SIZE 65536
+
 typedef struct RngEgd
 {
     RngBackend parent;
@@ -28,6 +30,7 @@ typedef struct RngEgd
     EntropyReceiveFunc *receive_entropy;
     GSList *requests;
     void *opaque;
+    size_t req_size;
 } RngEgd;
 
 typedef struct RngRequest
@@ -37,9 +40,57 @@ typedef struct RngRequest
     size_t size;
 } RngRequest;
 
-static void rng_egd_request_entropy(RngBackend *b, size_t size,
-                                    EntropyReceiveFunc *receive_entropy,
-                                    void *opaque)
+
+static void rng_egd_free_request(RngRequest *req)
+{
+    g_free(req->data);
+    g_free(req);
+}
+
+static int get_available_data_size(RngEgd *s)
+{
+    GSList *i;
+    RngRequest *req;
+    int total = 0;
+
+    for (i = s->requests; i; i = i->next) {
+        req = i->data;
+        total += req->offset;
+    }
+    return total;
+}
+
+static int get_free_buf_size(RngEgd *s)
+{
+
+    GSList *i;
+    RngRequest *req;
+    int total = 0;
+
+    for (i = s->requests; i; i = i->next) {
+        req = i->data;
+        total += req->size - req->offset;
+    }
+    return total;
+}
+
+static int get_total_buf_size(RngEgd *s)
+{
+
+    GSList *i;
+    RngRequest *req;
+    int total = 0;
+
+    for (i = s->requests; i; i = i->next) {
+        req = i->data;
+        total += req->size;
+    }
+    return total;
+}
+
+static void rng_egd_append_request(RngBackend *b, size_t size,
+                                   EntropyReceiveFunc *receive_entropy,
+                                   void *opaque)
 {
     RngEgd *s = RNG_EGD(b);
     RngRequest *req;
@@ -69,21 +120,60 @@ static void rng_egd_request_entropy(RngBackend *b, size_t size,
     s->requests = g_slist_append(s->requests, req);
 }
 
-static void rng_egd_free_request(RngRequest *req)
+
+static void rng_egd_expend_request(RngEgd *s, size_t size,
+                                   EntropyReceiveFunc *receive_entropy,
+                                   void *opaque)
 {
-    g_free(req->data);
-    g_free(req);
+    GSList *cur = s->requests;
+
+    while (size > 0 && cur) {
+        RngRequest *req = cur->data;
+        int len = MIN(size, req->offset);
+
+        s->receive_entropy(s->opaque, req->data, len);
+        req->offset -= len;
+        size -= len;
+        cur = cur->next;
+    }
+}
+
+static void rng_egd_request_entropy(RngBackend *b, size_t size,
+                                    EntropyReceiveFunc *receive_entropy,
+                                    void *opaque)
+{
+    RngEgd *s = RNG_EGD(b);
+
+    s->receive_entropy = receive_entropy;
+    s->opaque = opaque;
+    s->req_size += size;
+
+    if (get_available_data_size(s) >= size) {
+        rng_egd_expend_request(s, size, receive_entropy, opaque);
+        s->req_size -= size;
+    }
+
+    int total_size = get_total_buf_size(s);
+
+    while (total_size < BUFFER_SIZE)  {
+        int add_size = MIN(BUFFER_SIZE - total_size, 255);
+        total_size += add_size;
+        rng_egd_append_request(b, add_size, receive_entropy, opaque);
+    }
 }
 
 static int rng_egd_chr_can_read(void *opaque)
 {
     RngEgd *s = RNG_EGD(opaque);
-    GSList *i;
     int size = 0;
 
-    for (i = s->requests; i; i = i->next) {
-        RngRequest *req = i->data;
-        size += req->size - req->offset;
+    size = get_free_buf_size(s);
+
+    if (size == 0 && s->req_size > 0) {
+        int len = MIN(s->req_size, get_available_data_size(s));
+        rng_egd_expend_request(s, len, s->receive_entropy, opaque);
+        s->req_size -= len;
+        size = get_free_buf_size(s);
     }
 
     return size;
@@ -93,24 +183,25 @@ static void rng_egd_chr_read(void *opaque, const uint8_t *buf, int size)
 {
     RngEgd *s = RNG_EGD(opaque);
     size_t buf_offset = 0;
+    int len;
+    GSList *cur = s->requests;
 
     while (size > 0 && s->requests) {
-        RngRequest *req = s->requests->data;
-        int len = MIN(size, req->size - req->offset);
+        RngRequest *req = cur->data;
+        len = MIN(size, req->size - req->offset);
 
         memcpy(req->data + req->offset, buf + buf_offset, len);
         buf_offset += len;
         req->offset += len;
         size -= len;
-
-        if (req->offset == req->size) {
-            s->requests = g_slist_remove_link(s->requests, s->requests);
-
-            s->receive_entropy(s->opaque, req->data, req->size);
-
-            rng_egd_free_request(req);
-        }
+        cur = cur->next;
     }
+    if (s->req_size > 0) {
+        len = MIN(s->req_size, get_available_data_size(s));
+        rng_egd_expend_request(s, len, s->receive_entropy, opaque);
+        s->req_size -= len;
+    }
+
 }
 
 static void rng_egd_free_requests(RngEgd *s)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH RFC 1/2] rng-egd: improve egd backend performance

[Amit Shah]

On (Mon) 09 Dec 2013 [22:10:12], Amos Kong wrote:
> Bugzilla: https://bugs.launchpad.net/qemu/+bug/1253563
> 
> We have a requests queue to cache the random data, but the second
> will come in when the first request is returned, so we always
> only have one items in the queue. It effects the performance.
> 
> This patch changes the IOthread to fill a fixed buffer with
> random data from egd socket, request_entropy() will return
> data to virtio queue if buffer has available data.
> 
> (test with a fast source, disguised egd socket)
>  # cat /dev/urandom | nc -l localhost 8003
>  # qemu .. -chardev socket,host=localhost,port=8003,id=chr0 \
>         -object rng-egd,chardev=chr0,id=rng0,buf_size=1024 \
>         -device virtio-rng-pci,rng=rng0

First thing I can think of is the egd protocol has a length field in
the header, and if that isn't properly filled, the results are bound
to be erratic.

I haven't been able to find a spec detailing the way egd API, perhaps
Anthony knows how to best pass data to qemu via egd.

		Amit

Re: [Qemu-devel] [PATCH RFC 1/2] rng-egd: improve egd backend performance

[Anthony Liguori]

On Mon, Dec 16, 2013 at 8:36 AM, Amit Shah <amit.shah@redhat.com> wrote:
> On (Mon) 09 Dec 2013 [22:10:12], Amos Kong wrote:
>> Bugzilla: https://bugs.launchpad.net/qemu/+bug/1253563
>>
>> We have a requests queue to cache the random data, but the second
>> will come in when the first request is returned, so we always
>> only have one items in the queue. It effects the performance.
>>
>> This patch changes the IOthread to fill a fixed buffer with
>> random data from egd socket, request_entropy() will return
>> data to virtio queue if buffer has available data.
>>
>> (test with a fast source, disguised egd socket)
>>  # cat /dev/urandom | nc -l localhost 8003
>>  # qemu .. -chardev socket,host=localhost,port=8003,id=chr0 \
>>         -object rng-egd,chardev=chr0,id=rng0,buf_size=1024 \
>>         -device virtio-rng-pci,rng=rng0
>
> First thing I can think of is the egd protocol has a length field in
> the header, and if that isn't properly filled, the results are bound
> to be erratic.

The test is bogus.

egd is a protocol.  You can't just pipe /dev/urandom into it.

Regards,

Anthony Liguori

>
> I haven't been able to find a spec detailing the way egd API, perhaps
> Anthony knows how to best pass data to qemu via egd.
>
>                 Amit

Re: [Qemu-devel] [PATCH RFC 1/2] rng-egd: improve egd backend performance

[Amit Shah]

On (Mon) 16 Dec 2013 [15:19:31], Anthony Liguori wrote:
> On Mon, Dec 16, 2013 at 8:36 AM, Amit Shah <amit.shah@redhat.com> wrote:
> > On (Mon) 09 Dec 2013 [22:10:12], Amos Kong wrote:
> >> Bugzilla: https://bugs.launchpad.net/qemu/+bug/1253563
> >>
> >> We have a requests queue to cache the random data, but the second
> >> will come in when the first request is returned, so we always
> >> only have one items in the queue. It effects the performance.
> >>
> >> This patch changes the IOthread to fill a fixed buffer with
> >> random data from egd socket, request_entropy() will return
> >> data to virtio queue if buffer has available data.
> >>
> >> (test with a fast source, disguised egd socket)
> >>  # cat /dev/urandom | nc -l localhost 8003
> >>  # qemu .. -chardev socket,host=localhost,port=8003,id=chr0 \
> >>         -object rng-egd,chardev=chr0,id=rng0,buf_size=1024 \
> >>         -device virtio-rng-pci,rng=rng0
> >
> > First thing I can think of is the egd protocol has a length field in
> > the header, and if that isn't properly filled, the results are bound
> > to be erratic.
> 
> The test is bogus.
> 
> egd is a protocol.  You can't just pipe /dev/urandom into it.

Can you suggest a way to test this the right way?

		Amit

Re: [Qemu-devel] [PATCH RFC 1/2] rng-egd: improve egd backend performance

[Amos Kong]

On Tue, Dec 17, 2013 at 11:22:14AM +0530, Amit Shah wrote:
> On (Mon) 16 Dec 2013 [15:19:31], Anthony Liguori wrote:
> > On Mon, Dec 16, 2013 at 8:36 AM, Amit Shah <amit.shah@redhat.com> wrote:
> > > On (Mon) 09 Dec 2013 [22:10:12], Amos Kong wrote:
> > >> Bugzilla: https://bugs.launchpad.net/qemu/+bug/1253563
> > >>
> > >> We have a requests queue to cache the random data, but the second
> > >> will come in when the first request is returned, so we always
> > >> only have one items in the queue. It effects the performance.
> > >>
> > >> This patch changes the IOthread to fill a fixed buffer with
> > >> random data from egd socket, request_entropy() will return
> > >> data to virtio queue if buffer has available data.
> > >>
> > >> (test with a fast source, disguised egd socket)
> > >>  # cat /dev/urandom | nc -l localhost 8003
> > >>  # qemu .. -chardev socket,host=localhost,port=8003,id=chr0 \
> > >>         -object rng-egd,chardev=chr0,id=rng0,buf_size=1024 \
> > >>         -device virtio-rng-pci,rng=rng0
> > >

We have a wiki about about configing egd.
http://fedoraproject.org/wiki/QA:Testcase_Virtualization_VirtioRNG

> > > First thing I can think of is the egd protocol has a length field in
> > > the header, and if that isn't properly filled, the results are bound
> > > to be erratic.

The header setup in rng-egd.c is correct, we can check the debug
output of egd.pl

> > The test is bogus.
> > 
> > egd is a protocol.  You can't just pipe /dev/urandom into it.

In my test host, When I use the egd-socket, it is very slow.
So I use a quick souce /dev/urandom, we ignore the egd protocol
here, it might be wrong.
 
> Can you suggest a way to test this the right way?
 
It seems we should still use egd.pl to setup a daemon socket.
But how to make it very quick? We can't verify the performance
improvement if the source is too slow.

Can we use "--bottomless" option for egd.pl? it will not decrement
entropy count. When I use this option, the speed (without my patches)
is about 13 kB/s.

> 		Amit

-- 
			Amos.

Re: [Qemu-devel] [PATCH RFC 1/2] rng-egd: improve egd backend performance

[Markus Armbruster]

Amos Kong <akong@redhat.com> writes:

> Bugzilla: https://bugs.launchpad.net/qemu/+bug/1253563
>
> We have a requests queue to cache the random data, but the second
> will come in when the first request is returned, so we always
> only have one items in the queue. It effects the performance.
>
> This patch changes the IOthread to fill a fixed buffer with
> random data from egd socket, request_entropy() will return
> data to virtio queue if buffer has available data.
>
> (test with a fast source, disguised egd socket)
>  # cat /dev/urandom | nc -l localhost 8003
>  # qemu .. -chardev socket,host=localhost,port=8003,id=chr0 \
>         -object rng-egd,chardev=chr0,id=rng0,buf_size=1024 \
>         -device virtio-rng-pci,rng=rng0
>
>   bytes     kb/s
>   ------    ----
>   131072 ->  835
>    65536 ->  652
>    32768 ->  356
>    16384 ->  182
>     8192 ->   99
>     4096 ->   52
>     2048 ->   30
>     1024 ->   15
>      512 ->    8
>      256 ->    4
>      128 ->    3
>       64 ->    2

I'm not familiar with the rng-egd code, but perhaps my question has
value anyway: could agressive reading ahead on a source of randomness
cause trouble by depleting the source?

Consider a server restarting a few dozen guests after reboot, where each
guest's QEMU then tries to slurp in a couple of KiB of randomness.  How
does this behave?

Re: [Qemu-devel] [PATCH RFC 1/2] rng-egd: improve egd backend performance

[Amit Shah]

On (Tue) 17 Dec 2013 [08:47:34], Markus Armbruster wrote:
> Amos Kong <akong@redhat.com> writes:
> 
> > Bugzilla: https://bugs.launchpad.net/qemu/+bug/1253563
> >
> > We have a requests queue to cache the random data, but the second
> > will come in when the first request is returned, so we always
> > only have one items in the queue. It effects the performance.
> >
> > This patch changes the IOthread to fill a fixed buffer with
> > random data from egd socket, request_entropy() will return
> > data to virtio queue if buffer has available data.
> >
> > (test with a fast source, disguised egd socket)
> >  # cat /dev/urandom | nc -l localhost 8003
> >  # qemu .. -chardev socket,host=localhost,port=8003,id=chr0 \
> >         -object rng-egd,chardev=chr0,id=rng0,buf_size=1024 \
> >         -device virtio-rng-pci,rng=rng0
> >
> >   bytes     kb/s
> >   ------    ----
> >   131072 ->  835
> >    65536 ->  652
> >    32768 ->  356
> >    16384 ->  182
> >     8192 ->   99
> >     4096 ->   52
> >     2048 ->   30
> >     1024 ->   15
> >      512 ->    8
> >      256 ->    4
> >      128 ->    3
> >       64 ->    2
> 
> I'm not familiar with the rng-egd code, but perhaps my question has
> value anyway: could agressive reading ahead on a source of randomness
> cause trouble by depleting the source?
> 
> Consider a server restarting a few dozen guests after reboot, where each
> guest's QEMU then tries to slurp in a couple of KiB of randomness.  How
> does this behave?

It is a problem and we surely don't want such a problem.  So far, it
looks like we need to test this differently.  I'm not at all sure
these numbers will be meaningful once we get a proper test going.

		Amit

Re: [Qemu-devel] [PATCH RFC 1/2] rng-egd: improve egd backend performance

[Giuseppe Scrivano]

Markus Armbruster <armbru@redhat.com> writes:

> Amos Kong <akong@redhat.com> writes:
>
>> Bugzilla: https://bugs.launchpad.net/qemu/+bug/1253563
>>
>> We have a requests queue to cache the random data, but the second
>> will come in when the first request is returned, so we always
>> only have one items in the queue. It effects the performance.
>>
>> This patch changes the IOthread to fill a fixed buffer with
>> random data from egd socket, request_entropy() will return
>> data to virtio queue if buffer has available data.
>>
>> (test with a fast source, disguised egd socket)
>>  # cat /dev/urandom | nc -l localhost 8003
>>  # qemu .. -chardev socket,host=localhost,port=8003,id=chr0 \
>>         -object rng-egd,chardev=chr0,id=rng0,buf_size=1024 \
>>         -device virtio-rng-pci,rng=rng0
>>
>>   bytes     kb/s
>>   ------    ----
>>   131072 ->  835
>>    65536 ->  652
>>    32768 ->  356
>>    16384 ->  182
>>     8192 ->   99
>>     4096 ->   52
>>     2048 ->   30
>>     1024 ->   15
>>      512 ->    8
>>      256 ->    4
>>      128 ->    3
>>       64 ->    2
>
> I'm not familiar with the rng-egd code, but perhaps my question has
> value anyway: could agressive reading ahead on a source of randomness
> cause trouble by depleting the source?
>
> Consider a server restarting a few dozen guests after reboot, where each
> guest's QEMU then tries to slurp in a couple of KiB of randomness.  How
> does this behave?

I hit this performance problem while I was working on RNG devices
support in virt-manager and I also noticed that the bottleneck is in the
egd backend that slowly response to requests.  I thought as well about
adding a buffer but to handle it trough a new message type in the EGD
protocol.  The new message type informs the EGD daemon of the buffer
size and that the buffer data has a lower priority that the daemon
should fill when there are no other queued requests.  Could such
approach solve the scenario you've described?

Cheers,
Giuseppe

Re: [Qemu-devel] [PATCH RFC 1/2] rng-egd: improve egd backend performance

[Varad Gautam]

On Tue, Dec 17, 2013 at 12:33 PM, Amos Kong <akong@redhat.com> wrote:
>
> In my test host, When I use the egd-socket, it is very slow.
> So I use a quick souce /dev/urandom, we ignore the egd protocol
> here, it might be wrong.
>
> > Can you suggest a way to test this the right way?
>
> It seems we should still use egd.pl to setup a daemon socket.
> But how to make it very quick? We can't verify the performance
> improvement if the source is too slow.
>
> Can we use "--bottomless" option for egd.pl? it will not decrement
> entropy count. When I use this option, the speed (without my patches)
> is about 13 kB/s.

Is egd is more likely to be found running *as a substitute* on host machines
without a /dev/random device? If so, speed becomes a major issue if it has
not been paired with a hardware source, as it gets entropy by using the
output of various the programs it calls.

In that case, instead of having egd running on the host, would it be better
to have the guests run their own copy of egd if needed? This would keep
the entropy available on the guests independent of each other, and remove
the issue of a single guest overusing and depleting the host's entropy for
everyone else.

Otherwise, we could use the `--bottomless` option to make it fast for testing,
but in practice, as the README suggests it won't be good enough for
generating keys. Since it communicates through sockets, we can build the
qemu back-end this way.

Theoretically, would mixing entropy from egd (software-generated) with
/dev/random (hardware event triggered) produce a better entropy source
than each of these individually?  I know that /dev/random is pretty good, but if
it can be mixed with other sources and still be useful, it can be made to last
longer.

Varad

On Wed, Dec 18, 2013 at 3:35 PM, Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> Markus Armbruster <armbru@redhat.com> writes:
>
>> Amos Kong <akong@redhat.com> writes:
>>
>>> Bugzilla: https://bugs.launchpad.net/qemu/+bug/1253563
>>>
>>> We have a requests queue to cache the random data, but the second
>>> will come in when the first request is returned, so we always
>>> only have one items in the queue. It effects the performance.
>>>
>>> This patch changes the IOthread to fill a fixed buffer with
>>> random data from egd socket, request_entropy() will return
>>> data to virtio queue if buffer has available data.
>>>
>>> (test with a fast source, disguised egd socket)
>>>  # cat /dev/urandom | nc -l localhost 8003
>>>  # qemu .. -chardev socket,host=localhost,port=8003,id=chr0 \
>>>         -object rng-egd,chardev=chr0,id=rng0,buf_size=1024 \
>>>         -device virtio-rng-pci,rng=rng0
>>>
>>>   bytes     kb/s
>>>   ------    ----
>>>   131072 ->  835
>>>    65536 ->  652
>>>    32768 ->  356
>>>    16384 ->  182
>>>     8192 ->   99
>>>     4096 ->   52
>>>     2048 ->   30
>>>     1024 ->   15
>>>      512 ->    8
>>>      256 ->    4
>>>      128 ->    3
>>>       64 ->    2
>>
>> I'm not familiar with the rng-egd code, but perhaps my question has
>> value anyway: could agressive reading ahead on a source of randomness
>> cause trouble by depleting the source?
>>
>> Consider a server restarting a few dozen guests after reboot, where each
>> guest's QEMU then tries to slurp in a couple of KiB of randomness.  How
>> does this behave?
>
> I hit this performance problem while I was working on RNG devices
> support in virt-manager and I also noticed that the bottleneck is in the
> egd backend that slowly response to requests.  I thought as well about
> adding a buffer but to handle it trough a new message type in the EGD
> protocol.  The new message type informs the EGD daemon of the buffer
> size and that the buffer data has a lower priority that the daemon
> should fill when there are no other queued requests.  Could such
> approach solve the scenario you've described?
>
> Cheers,
> Giuseppe

Re: [Qemu-devel] [PATCH RFC 1/2] rng-egd: improve egd backend performance

[Amos Kong]

On Wed, Dec 18, 2013 at 11:05:14AM +0100, Giuseppe Scrivano wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Amos Kong <akong@redhat.com> writes:
> >
> >> Bugzilla: https://bugs.launchpad.net/qemu/+bug/1253563
> >>
> >> We have a requests queue to cache the random data, but the second
> >> will come in when the first request is returned, so we always
> >> only have one items in the queue. It effects the performance.
> >>
> >> This patch changes the IOthread to fill a fixed buffer with
> >> random data from egd socket, request_entropy() will return
> >> data to virtio queue if buffer has available data.
> >>
> >> (test with a fast source, disguised egd socket)
> >>  # cat /dev/urandom | nc -l localhost 8003
> >>  # qemu .. -chardev socket,host=localhost,port=8003,id=chr0 \
> >>         -object rng-egd,chardev=chr0,id=rng0,buf_size=1024 \
> >>         -device virtio-rng-pci,rng=rng0
> >>
> >>   bytes     kb/s
> >>   ------    ----
> >>   131072 ->  835
> >>    65536 ->  652
> >>    32768 ->  356
> >>    16384 ->  182
> >>     8192 ->   99
> >>     4096 ->   52
> >>     2048 ->   30
> >>     1024 ->   15
> >>      512 ->    8
> >>      256 ->    4
> >>      128 ->    3
> >>       64 ->    2
> >
> > I'm not familiar with the rng-egd code, but perhaps my question has
> > value anyway: could agressive reading ahead on a source of randomness
> > cause trouble by depleting the source?
> >
> > Consider a server restarting a few dozen guests after reboot, where each
> > guest's QEMU then tries to slurp in a couple of KiB of randomness.  How
> > does this behave?

Hi Giuseppe,
 
> I hit this performance problem while I was working on RNG devices
> support in virt-manager and I also noticed that the bottleneck is in the
> egd backend that slowly response to requests.

o Current situation:
  rng-random backend reads data from non-blocking character devices
  New entropy request will be sent from guest when last request is processed,
  so the request queue can only cache one request.
  Almost all the request size is 64 bytes.
  Egd socket responses the request slowly.

o Solution 1: pre-reading, perf is improved, but cost much memory 
  In my V1 patch, I tried to add a configurable buffer to pre-read data
  from egd socket. The performance was improved but it used a big memory
  as the buffer.

o Solution 2: pre-sending request to egd socket, improve is trivial
  I did another test, we just pre-send entropy request to egd socket, not
  really read the data to a buffer.

o Solution 3: eyeless poll, not good
  Always returns an integer in rng_egd_chr_can_read(), the perf can be 
  improved to 120 kB/s, it reduce the delay caused by poll mechanism.

o Solution 4:
  Try to use the new message type to improve the response speed of egd socket

o Solution 5:
  non-block read?

> I thought as well about
> adding a buffer but to handle it trough a new message type in the EGD
> protocol.  The new message type informs the EGD daemon of the buffer
> size and that the buffer data has a lower priority that the daemon

lower priority or higher priority? we need the daemon respons our request quickly.

> should fill when there are no other queued requests.  Could such
> approach solve the scenario you've described?

I will try. Do you know the name of new message type? can you show me
an example?

QEMU code:
  uint8_t header[2];
  header[0] = 0x02;  /* 0x01: returns len + data, 0x02: only returns data*/
  header[1] = len;
  qemu_chr_fe_write(s->chr, header, sizeof(header));
 
> Cheers,
> Giuseppe

-- 
			Amos.

Re: [Qemu-devel] [PATCH RFC 1/2] rng-egd: improve egd backend performance

[Amit Shah]

On (Wed) 08 Jan 2014 [17:14:41], Amos Kong wrote:
> On Wed, Dec 18, 2013 at 11:05:14AM +0100, Giuseppe Scrivano wrote:
> > Markus Armbruster <armbru@redhat.com> writes:
> > 
> > > Amos Kong <akong@redhat.com> writes:
> > >
> > >> Bugzilla: https://bugs.launchpad.net/qemu/+bug/1253563
> > >>
> > >> We have a requests queue to cache the random data, but the second
> > >> will come in when the first request is returned, so we always
> > >> only have one items in the queue. It effects the performance.
> > >>
> > >> This patch changes the IOthread to fill a fixed buffer with
> > >> random data from egd socket, request_entropy() will return
> > >> data to virtio queue if buffer has available data.
> > >>
> > >> (test with a fast source, disguised egd socket)
> > >>  # cat /dev/urandom | nc -l localhost 8003
> > >>  # qemu .. -chardev socket,host=localhost,port=8003,id=chr0 \
> > >>         -object rng-egd,chardev=chr0,id=rng0,buf_size=1024 \
> > >>         -device virtio-rng-pci,rng=rng0
> > >>
> > >>   bytes     kb/s
> > >>   ------    ----
> > >>   131072 ->  835
> > >>    65536 ->  652
> > >>    32768 ->  356
> > >>    16384 ->  182
> > >>     8192 ->   99
> > >>     4096 ->   52
> > >>     2048 ->   30
> > >>     1024 ->   15
> > >>      512 ->    8
> > >>      256 ->    4
> > >>      128 ->    3
> > >>       64 ->    2
> > >
> > > I'm not familiar with the rng-egd code, but perhaps my question has
> > > value anyway: could agressive reading ahead on a source of randomness
> > > cause trouble by depleting the source?
> > >
> > > Consider a server restarting a few dozen guests after reboot, where each
> > > guest's QEMU then tries to slurp in a couple of KiB of randomness.  How
> > > does this behave?
> 
> Hi Giuseppe,
>  
> > I hit this performance problem while I was working on RNG devices
> > support in virt-manager and I also noticed that the bottleneck is in the
> > egd backend that slowly response to requests.
> 
> o Current situation:
>   rng-random backend reads data from non-blocking character devices
>   New entropy request will be sent from guest when last request is processed,
>   so the request queue can only cache one request.
>   Almost all the request size is 64 bytes.
>   Egd socket responses the request slowly.
> 
> o Solution 1: pre-reading, perf is improved, but cost much memory 
>   In my V1 patch, I tried to add a configurable buffer to pre-read data
>   from egd socket. The performance was improved but it used a big memory
>   as the buffer.

I really dislike buffering random numbers or entropy from the host,
let's rule these options out.

> o Solution 2: pre-sending request to egd socket, improve is trivial
>   I did another test, we just pre-send entropy request to egd socket, not
>   really read the data to a buffer.
> 
> o Solution 3: eyeless poll, not good
>   Always returns an integer in rng_egd_chr_can_read(), the perf can be 
>   improved to 120 kB/s, it reduce the delay caused by poll mechanism.
> 
> o Solution 4:
>   Try to use the new message type to improve the response speed of egd socket
> 
> o Solution 5:
>   non-block read?

I'd just say let the "problem" be.  I don't really get the point of
egd.  The egd backend was something Anthony wanted, but I can't
remember if there has been enough justification for it.  Certainly the
protocol isn't documented, and not using the backend doesn't give us
drawbacks.

Moreover, reasonable guests won't request for a whole lot of random
numbers in a short interval, so the theoretical performance problem
we're seeing is just going to remain theoretical for well-behaved
guests.

We have enough documentation by now about this issue, I say let's just
drop this patch and worry about this only if there's a proven need to
better things here.

		Amit

Re: [Qemu-devel] [PATCH RFC 1/2] rng-egd: improve egd backend performance

[Amos Kong]

On Wed, Jan 08, 2014 at 09:53:02PM +0530, Amit Shah wrote:
> On (Wed) 08 Jan 2014 [17:14:41], Amos Kong wrote:
> > On Wed, Dec 18, 2013 at 11:05:14AM +0100, Giuseppe Scrivano wrote:
> > > Markus Armbruster <armbru@redhat.com> writes:
> > > 
> > > > Amos Kong <akong@redhat.com> writes:
> > > >
> > > >> Bugzilla: https://bugs.launchpad.net/qemu/+bug/1253563
> > > >>
> > > >> We have a requests queue to cache the random data, but the second
> > > >> will come in when the first request is returned, so we always
> > > >> only have one items in the queue. It effects the performance.
> > > >>
> > > >> This patch changes the IOthread to fill a fixed buffer with
> > > >> random data from egd socket, request_entropy() will return
> > > >> data to virtio queue if buffer has available data.
> > > >>
> > > >> (test with a fast source, disguised egd socket)
> > > >>  # cat /dev/urandom | nc -l localhost 8003
> > > >>  # qemu .. -chardev socket,host=localhost,port=8003,id=chr0 \
> > > >>         -object rng-egd,chardev=chr0,id=rng0,buf_size=1024 \
> > > >>         -device virtio-rng-pci,rng=rng0
> > > >>
> > > >>   bytes     kb/s
> > > >>   ------    ----
> > > >>   131072 ->  835
> > > >>    65536 ->  652
> > > >>    32768 ->  356
> > > >>    16384 ->  182
> > > >>     8192 ->   99
> > > >>     4096 ->   52
> > > >>     2048 ->   30
> > > >>     1024 ->   15
> > > >>      512 ->    8
> > > >>      256 ->    4
> > > >>      128 ->    3
> > > >>       64 ->    2
> > > >
> > > > I'm not familiar with the rng-egd code, but perhaps my question has
> > > > value anyway: could agressive reading ahead on a source of randomness
> > > > cause trouble by depleting the source?
> > > >
> > > > Consider a server restarting a few dozen guests after reboot, where each
> > > > guest's QEMU then tries to slurp in a couple of KiB of randomness.  How
> > > > does this behave?
> > 
> > Hi Giuseppe,
> >  
> > > I hit this performance problem while I was working on RNG devices
> > > support in virt-manager and I also noticed that the bottleneck is in the
> > > egd backend that slowly response to requests.
> > 
> > o Current situation:
> >   rng-random backend reads data from non-blocking character devices
> >   New entropy request will be sent from guest when last request is processed,
> >   so the request queue can only cache one request.
> >   Almost all the request size is 64 bytes.
> >   Egd socket responses the request slowly.
> > 
> > o Solution 1: pre-reading, perf is improved, but cost much memory 
> >   In my V1 patch, I tried to add a configurable buffer to pre-read data
> >   from egd socket. The performance was improved but it used a big memory
> >   as the buffer.
> 
> I really dislike buffering random numbers or entropy from the host,
> let's rule these options out.

Agree.
The main reason is the slow source, using buffers and pre-reading is
just abuse the resource & sync read API in qemu.
 
> > o Solution 2: pre-sending request to egd socket, improve is trivial
> >   I did another test, we just pre-send entropy request to egd socket, not
> >   really read the data to a buffer.
> > 
> > o Solution 3: eyeless poll, not good
> >   Always returns an integer in rng_egd_chr_can_read(), the perf can be 
> >   improved to 120 kB/s, it reduce the delay caused by poll mechanism.
> > 
> > o Solution 4:
> >   Try to use the new message type to improve the response speed of egd socket
> > 
> > o Solution 5:
> >   non-block read?
> 
> I'd just say let the "problem" be.  I don't really get the point of
> egd.  The egd backend was something Anthony wanted, but I can't
> remember if there has been enough justification for it.  Certainly the
> protocol isn't documented, and not using the backend doesn't give us
> drawbacks.
 
http://miketeo.net/wp/index.php/2009/06/09/egd-entropy-gathering-daemon-client-protocol.html

> Moreover, reasonable guests won't request for a whole lot of random
> numbers in a short interval, so the theoretical performance problem
> we're seeing is just going to remain theoretical for well-behaved
> guests.
> 
> We have enough documentation by now about this issue, I say let's just
> drop this patch and worry about this only if there's a proven need to
> better things here.

We always recommend the users to use rng-random backend. Only use
rng-egd backend when host uses a USB entropy device.

Let's see if there exists some problem when the rng backend speed is
about 5 kB/s.
 
> 		Amit

-- 
			Amos.

[Qemu-devel] [PATCH RFC 2/2] rng-egd: introduce a parameter to set buffer size

[Amos Kong]

This patch makes the buffer size configurable, the max
buffer size is 65536.

 -object rng-egd,chardev=chr0,id=rng0,buf_size=1024

Signed-off-by: Amos Kong <akong@redhat.com>
---
 backends/rng-egd.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/backends/rng-egd.c b/backends/rng-egd.c
index d317c61..4e5ba18 100644
--- a/backends/rng-egd.c
+++ b/backends/rng-egd.c
@@ -18,7 +18,7 @@
 #define TYPE_RNG_EGD "rng-egd"
 #define RNG_EGD(obj) OBJECT_CHECK(RngEgd, (obj), TYPE_RNG_EGD)
 
-#define BUFFER_SIZE 65536
+#define MAX_BUFFER_SIZE 65536
 
 typedef struct RngEgd
 {
@@ -31,6 +31,7 @@ typedef struct RngEgd
     GSList *requests;
     void *opaque;
     size_t req_size;
+    uint32_t buf_size;
 } RngEgd;
 
 typedef struct RngRequest
@@ -154,9 +155,16 @@ static void rng_egd_request_entropy(RngBackend *b, size_t size,
     }
 
     int total_size = get_total_buf_size(s);
+    int buf_size;
 
-    while (total_size < BUFFER_SIZE)  {
-        int add_size = MIN(BUFFER_SIZE - total_size, 255);
+    if (s->buf_size != 0) {
+        buf_size = MIN(s->buf_size, MAX_BUFFER_SIZE);
+    } else {
+        buf_size = MAX_BUFFER_SIZE;
+    }
+
+    while (total_size < buf_size)  {
+        int add_size = MIN(buf_size - total_size, 255);
         total_size += add_size;
         rng_egd_append_request(b, add_size, receive_entropy, opaque);
     }
@@ -253,6 +261,15 @@ static void rng_egd_opened(RngBackend *b, Error **errp)
                           NULL, s);
 }
 
+static void rng_egd_set_buf_size(Object *obj, const char *value, Error **errp)
+{
+    RngBackend *b = RNG_BACKEND(obj);
+    RngEgd *s = RNG_EGD(b);
+
+    s->buf_size = atoi(value);
+    assert(s->buf_size > 0);
+}
+
 static void rng_egd_set_chardev(Object *obj, const char *value, Error **errp)
 {
     RngBackend *b = RNG_BACKEND(obj);
@@ -281,6 +298,7 @@ static void rng_egd_init(Object *obj)
     object_property_add_str(obj, "chardev",
                             rng_egd_get_chardev, rng_egd_set_chardev,
                             NULL);
+    object_property_add_str(obj, "buf_size", NULL, rng_egd_set_buf_size, NULL);
 }
 
 static void rng_egd_finalize(Object *obj)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH RFC 2/2] rng-egd: introduce a parameter to set buffer size

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 899 bytes --]

On 12/09/2013 07:10 AM, Amos Kong wrote:
> This patch makes the buffer size configurable, the max
> buffer size is 65536.
> 
>  -object rng-egd,chardev=chr0,id=rng0,buf_size=1024
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  backends/rng-egd.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)

> @@ -281,6 +298,7 @@ static void rng_egd_init(Object *obj)
>      object_property_add_str(obj, "chardev",
>                              rng_egd_get_chardev, rng_egd_set_chardev,
>                              NULL);
> +    object_property_add_str(obj, "buf_size", NULL, rng_egd_set_buf_size, NULL);
>  }

Will libvirt ever need to set this property?  And is it easily
discoverable whether qemu supports or lacks this property?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH RFC 2/2] rng-egd: introduce a parameter to set buffer size

[Amos Kong]

[-- Attachment #1: Type: text/plain, Size: 1241 bytes --]

On Tue, Dec 10, 2013 at 09:58:19AM -0700, Eric Blake wrote:
> On 12/09/2013 07:10 AM, Amos Kong wrote:

Hi Eric,

> > This patch makes the buffer size configurable, the max
> > buffer size is 65536.
> > 
> >  -object rng-egd,chardev=chr0,id=rng0,buf_size=1024
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  backends/rng-egd.c | 24 +++++++++++++++++++++---
> >  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> > @@ -281,6 +298,7 @@ static void rng_egd_init(Object *obj)
> >      object_property_add_str(obj, "chardev",
> >                              rng_egd_get_chardev, rng_egd_set_chardev,
> >                              NULL);
> > +    object_property_add_str(obj, "buf_size", NULL, rng_egd_set_buf_size, NULL);
> >  }
> 
> Will libvirt ever need to set this property?  And is it easily
> discoverable whether qemu supports or lacks this property?

This is ajust a RFC patch, we might fix the performance with other
method. If it won't cost too much memory for buf, we can remove
this parameter.

In this patch, we have a default buffer size.
 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org



-- 
			Amos.

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]