| Message ID | 1455119605-31261-5-git-send-email-lprosek@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
> > > 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 > >
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
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
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 --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); };
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(-)