diff mbox

[v2,4/4] rng: add request queue support to rng-random

Message ID 1455119605-31261-5-git-send-email-lprosek@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ladi Prosek Feb. 10, 2016, 3:53 p.m. UTC
Requests are now created in the RngBackend parent class and the
code path is shared by both rng-egd and rng-random.

This commit fixes the rng-random implementation which currently
processes only one request at a time and simply discards all
but the most recent one. In the guest this manifests as delayed
completion of reads from virtio-rng, i.e. a read is completed
only after another read is issued.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 backends/rng-egd.c    | 16 ++--------------
 backends/rng-random.c | 43 +++++++++++++++++++------------------------
 backends/rng.c        | 13 ++++++++++++-
 include/sysemu/rng.h  |  3 +--
 4 files changed, 34 insertions(+), 41 deletions(-)

Comments

Paolo Bonzini Feb. 10, 2016, 4:23 p.m. UTC | #1
On 10/02/2016 16:53, Ladi Prosek wrote:
> +        req->size = size;
> +        req->receive_entropy = receive_entropy;
> +        req->opaque = opaque;
> +        req->data = g_malloc(req->size);
> +
> +        k->request_entropy(s, req);
> +
> +        s->requests = g_slist_append(s->requests, req);
>      }

g_slist_append has to traverse the entire list to find the place to add
the node.  You probably are better off using QSIMPLEQ (which is an
intrusive list unlike GSList).

Thanks,

Paolo
Ladi Prosek Feb. 10, 2016, 4:40 p.m. UTC | #2
> 
> 
> On 10/02/2016 16:53, Ladi Prosek wrote:
> > +        req->size = size;
> > +        req->receive_entropy = receive_entropy;
> > +        req->opaque = opaque;
> > +        req->data = g_malloc(req->size);
> > +
> > +        k->request_entropy(s, req);
> > +
> > +        s->requests = g_slist_append(s->requests, req);
> >      }
> 
> g_slist_append has to traverse the entire list to find the place to add
> the node.  You probably are better off using QSIMPLEQ (which is an
> intrusive list unlike GSList).

This is what rng-egd does today and I would argue that since the expected
length of the list is very small - it's going to be longer than 1 only
very rarely - a simple lightweight data structure is a better choice than
trying to be O(1) in the worst case.

I'll be happy to switch to QSIMPLEQ if you want though. Your call.

Thanks!
Ladi
 
> Thanks,
> 
> Paolo
> 
>
Paolo Bonzini Feb. 10, 2016, 4:40 p.m. UTC | #3
On 10/02/2016 17:40, Ladi Prosek wrote:
>>
>>
>> On 10/02/2016 16:53, Ladi Prosek wrote:
>>> +        req->size = size;
>>> +        req->receive_entropy = receive_entropy;
>>> +        req->opaque = opaque;
>>> +        req->data = g_malloc(req->size);
>>> +
>>> +        k->request_entropy(s, req);
>>> +
>>> +        s->requests = g_slist_append(s->requests, req);
>>>      }
>>
>> g_slist_append has to traverse the entire list to find the place to add
>> the node.  You probably are better off using QSIMPLEQ (which is an
>> intrusive list unlike GSList).
> 
> This is what rng-egd does today and I would argue that since the expected
> length of the list is very small - it's going to be longer than 1 only
> very rarely - a simple lightweight data structure is a better choice than
> trying to be O(1) in the worst case.
> 
> I'll be happy to switch to QSIMPLEQ if you want though. Your call.

Ok, it can be done on top I guess.  I'll let others review the patches
more closely!

Paolo
Amit Shah March 2, 2016, 9:56 a.m. UTC | #4
On (Wed) 10 Feb 2016 [16:53:25], Ladi Prosek wrote:
> Requests are now created in the RngBackend parent class and the
> code path is shared by both rng-egd and rng-random.
> 
> This commit fixes the rng-random implementation which currently
> processes only one request at a time and simply discards all
> but the most recent one. In the guest this manifests as delayed
> completion of reads from virtio-rng, i.e. a read is completed
> only after another read is issued.

Nice commit message, but one convention I like is: when you're fixing
something, refer to the bug in past tense, so it's not confusing with
existing behaviour (i.e. since you're already fixing that bug in this
patch, it's better to write in the past tense about this bug).

Also, since you mentioned the move from stack-based allocation to
heap-based in the cover letter, and since this patch actually does
that, please include a one-liner here for it too.

Otherwise, the series is fine.

		Amit
Ladi Prosek March 2, 2016, 10:07 a.m. UTC | #5
On Wed, Mar 2, 2016 at 10:56 AM, Amit Shah <amit.shah@redhat.com> wrote:
> On (Wed) 10 Feb 2016 [16:53:25], Ladi Prosek wrote:
>> Requests are now created in the RngBackend parent class and the
>> code path is shared by both rng-egd and rng-random.
>>
>> This commit fixes the rng-random implementation which currently
>> processes only one request at a time and simply discards all
>> but the most recent one. In the guest this manifests as delayed
>> completion of reads from virtio-rng, i.e. a read is completed
>> only after another read is issued.
>
> Nice commit message, but one convention I like is: when you're fixing
> something, refer to the bug in past tense, so it's not confusing with
> existing behaviour (i.e. since you're already fixing that bug in this
> patch, it's better to write in the past tense about this bug).
>
> Also, since you mentioned the move from stack-based allocation to
> heap-based in the cover letter, and since this patch actually does
> that, please include a one-liner here for it too.

Thanks for the review. I'll update the commit messages and send v3.

> Otherwise, the series is fine.
>
>                 Amit
diff mbox

Patch

diff --git a/backends/rng-egd.c b/backends/rng-egd.c
index 8f2bd16..30332ed 100644
--- a/backends/rng-egd.c
+++ b/backends/rng-egd.c
@@ -27,20 +27,10 @@  typedef struct RngEgd
     char *chr_name;
 } RngEgd;
 
-static void rng_egd_request_entropy(RngBackend *b, size_t size,
-                                    EntropyReceiveFunc *receive_entropy,
-                                    void *opaque)
+static void rng_egd_request_entropy(RngBackend *b, RngRequest *req)
 {
     RngEgd *s = RNG_EGD(b);
-    RngRequest *req;
-
-    req = g_malloc(sizeof(*req));
-
-    req->offset = 0;
-    req->size = size;
-    req->receive_entropy = receive_entropy;
-    req->opaque = opaque;
-    req->data = g_malloc(req->size);
+    size_t size = req->size;
 
     while (size > 0) {
         uint8_t header[2];
@@ -54,8 +44,6 @@  static void rng_egd_request_entropy(RngBackend *b, size_t size,
 
         size -= len;
     }
-
-    s->parent.requests = g_slist_append(s->parent.requests, req);
 }
 
 static int rng_egd_chr_can_read(void *opaque)
diff --git a/backends/rng-random.c b/backends/rng-random.c
index 8cdad6a..a6cb385 100644
--- a/backends/rng-random.c
+++ b/backends/rng-random.c
@@ -22,10 +22,6 @@  struct RndRandom
 
     int fd;
     char *filename;
-
-    EntropyReceiveFunc *receive_func;
-    void *opaque;
-    size_t size;
 };
 
 /**
@@ -38,36 +34,35 @@  struct RndRandom
 static void entropy_available(void *opaque)
 {
     RndRandom *s = RNG_RANDOM(opaque);
-    uint8_t buffer[s->size];
-    ssize_t len;
 
-    len = read(s->fd, buffer, s->size);
-    if (len < 0 && errno == EAGAIN) {
-        return;
+    while (s->parent.requests != NULL) {
+        RngRequest *req = s->parent.requests->data;
+        ssize_t len;
+
+        len = read(s->fd, req->data, req->size);
+        if (len < 0 && errno == EAGAIN) {
+            return;
+        }
+        g_assert(len != -1);
+
+        req->receive_entropy(req->opaque, req->data, len);
+
+        rng_backend_finalize_request(&s->parent, req);
     }
-    g_assert(len != -1);
-
-    s->receive_func(s->opaque, buffer, len);
-    s->receive_func = NULL;
 
+    /* We've drained all requests, the fd handler can be reset. */
     qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
 }
 
-static void rng_random_request_entropy(RngBackend *b, size_t size,
-                                        EntropyReceiveFunc *receive_entropy,
-                                        void *opaque)
+static void rng_random_request_entropy(RngBackend *b, RngRequest *req)
 {
     RndRandom *s = RNG_RANDOM(b);
 
-    if (s->receive_func) {
-        s->receive_func(s->opaque, NULL, 0);
+    if (s->parent.requests == NULL) {
+        /* If there are no pending requests yet, we need to
+         * install our fd handler. */
+        qemu_set_fd_handler(s->fd, entropy_available, NULL, s);
     }
-
-    s->receive_func = receive_entropy;
-    s->opaque = opaque;
-    s->size = size;
-
-    qemu_set_fd_handler(s->fd, entropy_available, NULL, s);
 }
 
 static void rng_random_opened(RngBackend *b, Error **errp)
diff --git a/backends/rng.c b/backends/rng.c
index 014cb9d..277a41b 100644
--- a/backends/rng.c
+++ b/backends/rng.c
@@ -20,9 +20,20 @@  void rng_backend_request_entropy(RngBackend *s, size_t size,
                                  void *opaque)
 {
     RngBackendClass *k = RNG_BACKEND_GET_CLASS(s);
+    RngRequest *req;
 
     if (k->request_entropy) {
-        k->request_entropy(s, size, receive_entropy, opaque);
+        req = g_malloc(sizeof(*req));
+
+        req->offset = 0;
+        req->size = size;
+        req->receive_entropy = receive_entropy;
+        req->opaque = opaque;
+        req->data = g_malloc(req->size);
+
+        k->request_entropy(s, req);
+
+        s->requests = g_slist_append(s->requests, req);
     }
 }
 
diff --git a/include/sysemu/rng.h b/include/sysemu/rng.h
index c2c9035..a7ed580 100644
--- a/include/sysemu/rng.h
+++ b/include/sysemu/rng.h
@@ -46,8 +46,7 @@  struct RngBackendClass
 {
     ObjectClass parent_class;
 
-    void (*request_entropy)(RngBackend *s, size_t size,
-                            EntropyReceiveFunc *receive_entropy, void *opaque);
+    void (*request_entropy)(RngBackend *s, RngRequest *req);
 
     void (*opened)(RngBackend *s, Error **errp);
 };