[Qemu-devel] [PATCH v3 0/4] rng-random: implement request queue

[Qemu-devel] [PATCH v3 0/4] rng-random: implement request queue

[Ladi Prosek]

As suggested by Paolo, I have moved the RngRequest implementation
up to the RngBackend parent class and made both child classes use
it. Apart from the refactoring, the only functional change
compared to v1 is the use of heap instead of stack allocation for
the read buffer in rng-random.

The parent class takes care of creating new requests and adding
them to the queue, as well as removing them from the queue and
deleting them. Child classes have access to the raw GSList * to
do whatever else they need to do (walk the queue, peek the head
of the queue, ..)

Changes from v2:
- updated commit message in 1/4 and 4/4 as requested by Amit

[Qemu-devel] [PATCH v3 1/4] rng: remove the unused request cancellation code

[Ladi Prosek]

rng_backend_cancel_requests had no callers and none of the code
deleted in this commit ever ran.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 backends/rng-egd.c   | 12 ------------
 backends/rng.c       |  9 ---------
 include/sysemu/rng.h | 11 -----------
 3 files changed, 32 deletions(-)

diff --git a/backends/rng-egd.c b/backends/rng-egd.c
index 2de5cd5..0b2976a 100644
--- a/backends/rng-egd.c
+++ b/backends/rng-egd.c
@@ -125,17 +125,6 @@ static void rng_egd_free_requests(RngEgd *s)
     s->requests = NULL;
 }
 
-static void rng_egd_cancel_requests(RngBackend *b)
-{
-    RngEgd *s = RNG_EGD(b);
-
-    /* We simply delete the list of pending requests.  If there is data in the 
-     * queue waiting to be read, this is okay, because there will always be
-     * more data than we requested originally
-     */
-    rng_egd_free_requests(s);
-}
-
 static void rng_egd_opened(RngBackend *b, Error **errp)
 {
     RngEgd *s = RNG_EGD(b);
@@ -213,7 +202,6 @@ static void rng_egd_class_init(ObjectClass *klass, void *data)
     RngBackendClass *rbc = RNG_BACKEND_CLASS(klass);
 
     rbc->request_entropy = rng_egd_request_entropy;
-    rbc->cancel_requests = rng_egd_cancel_requests;
     rbc->opened = rng_egd_opened;
 }
 
diff --git a/backends/rng.c b/backends/rng.c
index b7820ef..2f2f3ee 100644
--- a/backends/rng.c
+++ b/backends/rng.c
@@ -26,15 +26,6 @@ void rng_backend_request_entropy(RngBackend *s, size_t size,
     }
 }
 
-void rng_backend_cancel_requests(RngBackend *s)
-{
-    RngBackendClass *k = RNG_BACKEND_GET_CLASS(s);
-
-    if (k->cancel_requests) {
-        k->cancel_requests(s);
-    }
-}
-
 static bool rng_backend_prop_get_opened(Object *obj, Error **errp)
 {
     RngBackend *s = RNG_BACKEND(obj);
diff --git a/include/sysemu/rng.h b/include/sysemu/rng.h
index 0a27c9b..c7da17d 100644
--- a/include/sysemu/rng.h
+++ b/include/sysemu/rng.h
@@ -38,7 +38,6 @@ struct RngBackendClass
 
     void (*request_entropy)(RngBackend *s, size_t size,
                             EntropyReceiveFunc *receive_entropy, void *opaque);
-    void (*cancel_requests)(RngBackend *s);
 
     void (*opened)(RngBackend *s, Error **errp);
 };
@@ -69,14 +68,4 @@ struct RngBackend
 void rng_backend_request_entropy(RngBackend *s, size_t size,
                                  EntropyReceiveFunc *receive_entropy,
                                  void *opaque);
-
-/**
- * rng_backend_cancel_requests:
- * @s: the backend to cancel all pending requests in
- *
- * Cancels all pending requests submitted by @rng_backend_request_entropy.  This
- * should be used by a device during reset or in preparation for live migration
- * to stop tracking any request.
- */
-void rng_backend_cancel_requests(RngBackend *s);
 #endif
-- 
2.5.0

[Qemu-devel] [PATCH v3 2/4] rng: move request queue from RngEgd to RngBackend

[Ladi Prosek]

The 'requests' field now lives in the RngBackend parent class.
There are no functional changes in this commit.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 backends/rng-egd.c   | 28 +++++++++-------------------
 include/sysemu/rng.h | 11 +++++++++++
 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/backends/rng-egd.c b/backends/rng-egd.c
index 0b2976a..b061362 100644
--- a/backends/rng-egd.c
+++ b/backends/rng-egd.c
@@ -25,19 +25,8 @@ typedef struct RngEgd
 
     CharDriverState *chr;
     char *chr_name;
-
-    GSList *requests;
 } RngEgd;
 
-typedef struct RngRequest
-{
-    EntropyReceiveFunc *receive_entropy;
-    uint8_t *data;
-    void *opaque;
-    size_t offset;
-    size_t size;
-} RngRequest;
-
 static void rng_egd_request_entropy(RngBackend *b, size_t size,
                                     EntropyReceiveFunc *receive_entropy,
                                     void *opaque)
@@ -66,7 +55,7 @@ static void rng_egd_request_entropy(RngBackend *b, size_t size,
         size -= len;
     }
 
-    s->requests = g_slist_append(s->requests, req);
+    s->parent.requests = g_slist_append(s->parent.requests, req);
 }
 
 static void rng_egd_free_request(RngRequest *req)
@@ -81,7 +70,7 @@ static int rng_egd_chr_can_read(void *opaque)
     GSList *i;
     int size = 0;
 
-    for (i = s->requests; i; i = i->next) {
+    for (i = s->parent.requests; i; i = i->next) {
         RngRequest *req = i->data;
         size += req->size - req->offset;
     }
@@ -94,8 +83,8 @@ static void rng_egd_chr_read(void *opaque, const uint8_t *buf, int size)
     RngEgd *s = RNG_EGD(opaque);
     size_t buf_offset = 0;
 
-    while (size > 0 && s->requests) {
-        RngRequest *req = s->requests->data;
+    while (size > 0 && s->parent.requests) {
+        RngRequest *req = s->parent.requests->data;
         int len = MIN(size, req->size - req->offset);
 
         memcpy(req->data + req->offset, buf + buf_offset, len);
@@ -104,7 +93,8 @@ static void rng_egd_chr_read(void *opaque, const uint8_t *buf, int size)
         size -= len;
 
         if (req->offset == req->size) {
-            s->requests = g_slist_remove_link(s->requests, s->requests);
+            s->parent.requests = g_slist_remove_link(s->parent.requests,
+                                                     s->parent.requests);
 
             req->receive_entropy(req->opaque, req->data, req->size);
 
@@ -117,12 +107,12 @@ static void rng_egd_free_requests(RngEgd *s)
 {
     GSList *i;
 
-    for (i = s->requests; i; i = i->next) {
+    for (i = s->parent.requests; i; i = i->next) {
         rng_egd_free_request(i->data);
     }
 
-    g_slist_free(s->requests);
-    s->requests = NULL;
+    g_slist_free(s->parent.requests);
+    s->parent.requests = NULL;
 }
 
 static void rng_egd_opened(RngBackend *b, Error **errp)
diff --git a/include/sysemu/rng.h b/include/sysemu/rng.h
index c7da17d..084164c 100644
--- a/include/sysemu/rng.h
+++ b/include/sysemu/rng.h
@@ -25,6 +25,7 @@
 #define RNG_BACKEND_CLASS(klass) \
     OBJECT_CLASS_CHECK(RngBackendClass, (klass), TYPE_RNG_BACKEND)
 
+typedef struct RngRequest RngRequest;
 typedef struct RngBackendClass RngBackendClass;
 typedef struct RngBackend RngBackend;
 
@@ -32,6 +33,15 @@ typedef void (EntropyReceiveFunc)(void *opaque,
                                   const void *data,
                                   size_t size);
 
+struct RngRequest
+{
+    EntropyReceiveFunc *receive_entropy;
+    uint8_t *data;
+    void *opaque;
+    size_t offset;
+    size_t size;
+};
+
 struct RngBackendClass
 {
     ObjectClass parent_class;
@@ -48,6 +58,7 @@ struct RngBackend
 
     /*< protected >*/
     bool opened;
+    GSList *requests;
 };
 
 /**
-- 
2.5.0

[Qemu-devel] [PATCH v3 3/4] rng: move request queue cleanup from RngEgd to RngBackend

[Ladi Prosek]

RngBackend is now in charge of cleaning up the linked list on
instance finalization. It also exposes a function to finalize
individual RngRequest instances, called by its child classes.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 backends/rng-egd.c   | 25 +------------------------
 backends/rng.c       | 32 ++++++++++++++++++++++++++++++++
 include/sysemu/rng.h | 12 ++++++++++++
 3 files changed, 45 insertions(+), 24 deletions(-)

diff --git a/backends/rng-egd.c b/backends/rng-egd.c
index b061362..8f2bd16 100644
--- a/backends/rng-egd.c
+++ b/backends/rng-egd.c
@@ -58,12 +58,6 @@ static void rng_egd_request_entropy(RngBackend *b, size_t size,
     s->parent.requests = g_slist_append(s->parent.requests, req);
 }
 
-static void rng_egd_free_request(RngRequest *req)
-{
-    g_free(req->data);
-    g_free(req);
-}
-
 static int rng_egd_chr_can_read(void *opaque)
 {
     RngEgd *s = RNG_EGD(opaque);
@@ -93,28 +87,13 @@ static void rng_egd_chr_read(void *opaque, const uint8_t *buf, int size)
         size -= len;
 
         if (req->offset == req->size) {
-            s->parent.requests = g_slist_remove_link(s->parent.requests,
-                                                     s->parent.requests);
-
             req->receive_entropy(req->opaque, req->data, req->size);
 
-            rng_egd_free_request(req);
+            rng_backend_finalize_request(&s->parent, req);
         }
     }
 }
 
-static void rng_egd_free_requests(RngEgd *s)
-{
-    GSList *i;
-
-    for (i = s->parent.requests; i; i = i->next) {
-        rng_egd_free_request(i->data);
-    }
-
-    g_slist_free(s->parent.requests);
-    s->parent.requests = NULL;
-}
-
 static void rng_egd_opened(RngBackend *b, Error **errp)
 {
     RngEgd *s = RNG_EGD(b);
@@ -183,8 +162,6 @@ static void rng_egd_finalize(Object *obj)
     }
 
     g_free(s->chr_name);
-
-    rng_egd_free_requests(s);
 }
 
 static void rng_egd_class_init(ObjectClass *klass, void *data)
diff --git a/backends/rng.c b/backends/rng.c
index 2f2f3ee..014cb9d 100644
--- a/backends/rng.c
+++ b/backends/rng.c
@@ -64,6 +64,30 @@ static void rng_backend_prop_set_opened(Object *obj, bool value, Error **errp)
     s->opened = true;
 }
 
+static void rng_backend_free_request(RngRequest *req)
+{
+    g_free(req->data);
+    g_free(req);
+}
+
+static void rng_backend_free_requests(RngBackend *s)
+{
+    GSList *i;
+
+    for (i = s->requests; i; i = i->next) {
+        rng_backend_free_request(i->data);
+    }
+
+    g_slist_free(s->requests);
+    s->requests = NULL;
+}
+
+void rng_backend_finalize_request(RngBackend *s, RngRequest *req)
+{
+    s->requests = g_slist_remove(s->requests, req);
+    rng_backend_free_request(req);
+}
+
 static void rng_backend_init(Object *obj)
 {
     object_property_add_bool(obj, "opened",
@@ -72,6 +96,13 @@ static void rng_backend_init(Object *obj)
                              NULL);
 }
 
+static void rng_backend_finalize(Object *obj)
+{
+    RngBackend *s = RNG_BACKEND(obj);
+
+    rng_backend_free_requests(s);
+}
+
 static void rng_backend_class_init(ObjectClass *oc, void *data)
 {
     UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
@@ -84,6 +115,7 @@ static const TypeInfo rng_backend_info = {
     .parent = TYPE_OBJECT,
     .instance_size = sizeof(RngBackend),
     .instance_init = rng_backend_init,
+    .instance_finalize = rng_backend_finalize,
     .class_size = sizeof(RngBackendClass),
     .class_init = rng_backend_class_init,
     .abstract = true,
diff --git a/include/sysemu/rng.h b/include/sysemu/rng.h
index 084164c..c2c9035 100644
--- a/include/sysemu/rng.h
+++ b/include/sysemu/rng.h
@@ -61,6 +61,7 @@ struct RngBackend
     GSList *requests;
 };
 
+
 /**
  * rng_backend_request_entropy:
  * @s: the backend to request entropy from
@@ -79,4 +80,15 @@ struct RngBackend
 void rng_backend_request_entropy(RngBackend *s, size_t size,
                                  EntropyReceiveFunc *receive_entropy,
                                  void *opaque);
+
+/**
+ * rng_backend_free_request:
+ * @s: the backend that created the request
+ * @req: the request to finalize
+ *
+ * Used by child rng backend classes to finalize requests once they've been
+ * processed. The request is removed from the list of active requests and
+ * deleted.
+ */
+void rng_backend_finalize_request(RngBackend *s, RngRequest *req);
 #endif
-- 
2.5.0

[Qemu-devel] [PATCH v3 4/4] rng: add request queue support to rng-random

[Ladi Prosek]

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 processed
only one request at a time and simply discarded all but the most
recent one. In the guest this manifested as delayed completion
of reads from virtio-rng, i.e. a read was completed only after
another read was issued.

By switching rng-random to use the same request queue as rng-egd,
the unsafe stack-based allocation of the entropy buffer is
eliminated and replaced with g_malloc.

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

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);
 };
-- 
2.5.0

Re: [Qemu-devel] [PATCH v3 0/4] rng-random: implement request queue

[Amit Shah]

On (Thu) 03 Mar 2016 [09:37:14], Ladi Prosek wrote:
> As suggested by Paolo, I have moved the RngRequest implementation
> up to the RngBackend parent class and made both child classes use
> it. Apart from the refactoring, the only functional change
> compared to v1 is the use of heap instead of stack allocation for
> the read buffer in rng-random.
> 
> The parent class takes care of creating new requests and adding
> them to the queue, as well as removing them from the queue and
> deleting them. Child classes have access to the raw GSList * to
> do whatever else they need to do (walk the queue, peek the head
> of the queue, ..)
> 
> Changes from v2:
> - updated commit message in 1/4 and 4/4 as requested by Amit

Reviewed-by: Amit Shah <amit.shah@redhat.com>

Thanks,

		Amit