diff mbox

[01/25,virtio-9p] Add infrastructure to support glib threads and coroutines.

Message ID 1305233867-4367-2-git-send-email-jvrao@linux.vnet.ibm.com
State New
Headers show

Commit Message

jvrao May 12, 2011, 8:57 p.m. UTC 
This patch is originally made by Arun Bharadwaj for glib support.
Later Harsh Prateek Bora added coroutines support.

Signed-off-by: Arun R Bharadwaj <arun@linux.vnet.ibm.com>
Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
Signed-off-by: Venkateswararao Jujjuri "<jvrao@linux.vnet.ibm.com>
---
 Makefile.objs              |    2 +
 hw/9pfs/virtio-9p-coth.c   |   68 ++++++++++++++++++++++++++++++++++++++++++++
 hw/9pfs/virtio-9p-coth.h   |   45 +++++++++++++++++++++++++++++
 hw/9pfs/virtio-9p-device.c |   26 +++++++++++++++-
 hw/9pfs/virtio-9p.h        |    1 -
 5 files changed, 139 insertions(+), 3 deletions(-)
 create mode 100644 hw/9pfs/virtio-9p-coth.c
 create mode 100644 hw/9pfs/virtio-9p-coth.h

Comments

Stefan Hajnoczi May 13, 2011, 5:48 a.m. UTC | #1 
On Thu, May 12, 2011 at 9:57 PM, Venkateswararao Jujjuri (JV)
<jvrao@linux.vnet.ibm.com> wrote:
> +/* v9fs glib thread pool */
> +V9fsThPool v9fs_pool;

This should be static and an init function in this file could perform
initialization.  Right now the initialization is inlined in
virtio-9p-device.c.

> +void v9fs_qemu_submit_request(V9fsRequest *req)
> +{
> +    V9fsThPool *p = &v9fs_pool;
> +
> +    req->done = 0;
> +    p->requests = g_list_append(p->requests, req);
> +    g_thread_pool_push(v9fs_pool.pool, req, NULL);
> +}
> +
> +void v9fs_qemu_process_req_done(void *arg)
> +{
> +    struct V9fsThPool *p = &v9fs_pool;
> +    char byte;
> +    ssize_t len;
> +    GList *cur_req, *next_req;
> +
> +    do {
> +        len = read(p->rfd, &byte, sizeof(byte));
> +    } while (len == -1 && errno == EINTR);
> +
> +    for (cur_req = p->requests; cur_req != NULL; cur_req = next_req) {
> +        V9fsRequest *req = cur_req->data;
> +        next_req = g_list_next(cur_req);
> +
> +        if (!req->done) {
> +            continue;
> +        }

The requests list is only used for completion, why not enqueue only
completed requests and get rid of the done field.  Then this function
doesn't have to skip over in-flight requests.

> +        Coroutine *entry = req->coroutine;
> +        qemu_coroutine_enter(entry, NULL);
> +
> +        p->requests = g_list_delete_link(p->requests, cur_req);

Does this actually free the req?

> +static int notifier_fds[2];

Why global?

> +    if (!g_thread_supported()) {
> +        g_thread_init(NULL);
> +    }

The logic looks inverted.  But if GThread isn't supported where in big
trouble so we should probably exit?

Stefan
jvrao May 13, 2011, 3:47 p.m. UTC | #2 
On 05/12/2011 10:48 PM, Stefan Hajnoczi wrote:
> On Thu, May 12, 2011 at 9:57 PM, Venkateswararao Jujjuri (JV)
> <jvrao@linux.vnet.ibm.com>  wrote:
>> +/* v9fs glib thread pool */
>> +V9fsThPool v9fs_pool;
> This should be static and an init function in this file could perform
> initialization.  Right now the initialization is inlined in
> virtio-9p-device.c.
>
>> +void v9fs_qemu_submit_request(V9fsRequest *req)
>> +{
>> +    V9fsThPool *p =&v9fs_pool;
>> +
>> +    req->done = 0;
>> +    p->requests = g_list_append(p->requests, req);
>> +    g_thread_pool_push(v9fs_pool.pool, req, NULL);
>> +}
>> +
>> +void v9fs_qemu_process_req_done(void *arg)
>> +{
>> +    struct V9fsThPool *p =&v9fs_pool;
>> +    char byte;
>> +    ssize_t len;
>> +    GList *cur_req, *next_req;
>> +
>> +    do {
>> +        len = read(p->rfd,&byte, sizeof(byte));
>> +    } while (len == -1&&  errno == EINTR);
>> +
>> +    for (cur_req = p->requests; cur_req != NULL; cur_req = next_req) {
>> +        V9fsRequest *req = cur_req->data;
>> +        next_req = g_list_next(cur_req);
>> +
>> +        if (!req->done) {
>> +            continue;
>> +        }
> The requests list is only used for completion, why not enqueue only
> completed requests and get rid of the done field.  Then this function
> doesn't have to skip over in-flight requests.
>
>> +        Coroutine *entry = req->coroutine;
>> +        qemu_coroutine_enter(entry, NULL);
>> +
>> +        p->requests = g_list_delete_link(p->requests, cur_req);
> Does this actually free the req?
>
>> +static int notifier_fds[2];
> Why global?
>
>> +    if (!g_thread_supported()) {
>> +        g_thread_init(NULL);
>> +    }
> The logic looks inverted.  But if GThread isn't supported where in big
> trouble so we should probably exit?
>
Name of the function is little weird .. it basically checking if the 
thread infrastructure
is initialized or not. One can call g_thread_init() unconditionally.. 
but this may be good
practice.

http://developer.gnome.org/glib/2.28/glib-Threads.html#g-thread-supported

We will address all your comments. Thanks for the review.

- JV

> Stefan
Stefan Hajnoczi May 13, 2011, 3:49 p.m. UTC | #3 
On Fri, May 13, 2011 at 4:47 PM, Venkateswararao Jujjuri
<jvrao@linux.vnet.ibm.com> wrote:
> On 05/12/2011 10:48 PM, Stefan Hajnoczi wrote:
>>
>> On Thu, May 12, 2011 at 9:57 PM, Venkateswararao Jujjuri (JV)
>> <jvrao@linux.vnet.ibm.com>  wrote:
>>>
>>> +/* v9fs glib thread pool */
>>> +V9fsThPool v9fs_pool;
>>
>> This should be static and an init function in this file could perform
>> initialization.  Right now the initialization is inlined in
>> virtio-9p-device.c.
>>
>>> +void v9fs_qemu_submit_request(V9fsRequest *req)
>>> +{
>>> +    V9fsThPool *p =&v9fs_pool;
>>> +
>>> +    req->done = 0;
>>> +    p->requests = g_list_append(p->requests, req);
>>> +    g_thread_pool_push(v9fs_pool.pool, req, NULL);
>>> +}
>>> +
>>> +void v9fs_qemu_process_req_done(void *arg)
>>> +{
>>> +    struct V9fsThPool *p =&v9fs_pool;
>>> +    char byte;
>>> +    ssize_t len;
>>> +    GList *cur_req, *next_req;
>>> +
>>> +    do {
>>> +        len = read(p->rfd,&byte, sizeof(byte));
>>> +    } while (len == -1&&  errno == EINTR);
>>> +
>>> +    for (cur_req = p->requests; cur_req != NULL; cur_req = next_req) {
>>> +        V9fsRequest *req = cur_req->data;
>>> +        next_req = g_list_next(cur_req);
>>> +
>>> +        if (!req->done) {
>>> +            continue;
>>> +        }
>>
>> The requests list is only used for completion, why not enqueue only
>> completed requests and get rid of the done field.  Then this function
>> doesn't have to skip over in-flight requests.
>>
>>> +        Coroutine *entry = req->coroutine;
>>> +        qemu_coroutine_enter(entry, NULL);
>>> +
>>> +        p->requests = g_list_delete_link(p->requests, cur_req);
>>
>> Does this actually free the req?
>>
>>> +static int notifier_fds[2];
>>
>> Why global?
>>
>>> +    if (!g_thread_supported()) {
>>> +        g_thread_init(NULL);
>>> +    }
>>
>> The logic looks inverted.  But if GThread isn't supported where in big
>> trouble so we should probably exit?
>>
> Name of the function is little weird .. it basically checking if the thread
> infrastructure
> is initialized or not. One can call g_thread_init() unconditionally.. but
> this may be good
> practice.
>
> http://developer.gnome.org/glib/2.28/glib-Threads.html#g-thread-supported

Thanks I was just checking it out and noticed they a function with a
clearer name since 2.20:
http://developer.gnome.org/glib/2.28/glib-Threads.html#g-thread-get-initialized

Stefan
diff mbox

Patch

diff --git a/Makefile.objs b/Makefile.objs
index 3873f10..96f6a24 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -297,8 +297,10 @@  hw-obj-$(CONFIG_SOUND) += $(sound-obj-y)
 9pfs-nested-$(CONFIG_VIRTFS) = virtio-9p.o virtio-9p-debug.o
 9pfs-nested-$(CONFIG_VIRTFS) +=  virtio-9p-local.o virtio-9p-xattr.o
 9pfs-nested-$(CONFIG_VIRTFS) +=   virtio-9p-xattr-user.o virtio-9p-posix-acl.o
+9pfs-nested-$(CONFIG_VIRTFS) += virtio-9p-coth.o
 
 hw-obj-$(CONFIG_REALLY_VIRTFS) += $(addprefix 9pfs/, $(9pfs-nested-y))
+$(addprefix 9pfs/, $(9pfs-nested-y)): QEMU_CFLAGS+=$(GLIB_CFLAGS)
 
 
 ######################################################################
diff --git a/hw/9pfs/virtio-9p-coth.c b/hw/9pfs/virtio-9p-coth.c
new file mode 100644
index 0000000..edd3cde
--- /dev/null
+++ b/hw/9pfs/virtio-9p-coth.c
@@ -0,0 +1,68 @@ 
+/*
+ * Virtio 9p backend
+ *
+ * Copyright IBM, Corp. 2010
+ *
+ * Authors:
+ *  Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
+ *  Venkateswararao Jujjuri(JV) <jvrao@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#include "fsdev/qemu-fsdev.h"
+#include "qemu-thread.h"
+#include "qemu-coroutine.h"
+#include "virtio-9p-coth.h"
+
+/* v9fs glib thread pool */
+V9fsThPool v9fs_pool;
+
+void v9fs_qemu_submit_request(V9fsRequest *req)
+{
+    V9fsThPool *p = &v9fs_pool;
+
+    req->done = 0;
+    p->requests = g_list_append(p->requests, req);
+    g_thread_pool_push(v9fs_pool.pool, req, NULL);
+}
+
+void v9fs_qemu_process_req_done(void *arg)
+{
+    struct V9fsThPool *p = &v9fs_pool;
+    char byte;
+    ssize_t len;
+    GList *cur_req, *next_req;
+
+    do {
+        len = read(p->rfd, &byte, sizeof(byte));
+    } while (len == -1 && errno == EINTR);
+
+    for (cur_req = p->requests; cur_req != NULL; cur_req = next_req) {
+        V9fsRequest *req = cur_req->data;
+        next_req = g_list_next(cur_req);
+
+        if (!req->done) {
+            continue;
+        }
+
+        Coroutine *entry = req->coroutine;
+        qemu_coroutine_enter(entry, NULL);
+
+        p->requests = g_list_delete_link(p->requests, cur_req);
+    }
+}
+
+void v9fs_thread_routine(gpointer data, gpointer user_data)
+{
+    V9fsRequest *req = data;
+    char byte = 0;
+    ssize_t len;
+    req->func(req);
+    req->done = 1;
+    do {
+        len = write(v9fs_pool.wfd, &byte, sizeof(byte));
+    } while (len == -1 && errno == EINTR);
+}
diff --git a/hw/9pfs/virtio-9p-coth.h b/hw/9pfs/virtio-9p-coth.h
new file mode 100644
index 0000000..fdc44f6
--- /dev/null
+++ b/hw/9pfs/virtio-9p-coth.h
@@ -0,0 +1,45 @@ 
+/*
+ * Virtio 9p backend
+ *
+ * Copyright IBM, Corp. 2010
+ *
+ * Authors:
+ *  Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
+ *  Venkateswararao Jujjuri(JV) <jvrao@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef _QEMU_VIRTIO_9P_COTH_H
+#define _QEMU_VIRTIO_9P_COTH_H
+
+#include "qemu-thread.h"
+#include "qemu-coroutine.h"
+#include <glib.h>
+
+typedef struct V9fsRequest {
+    void (*func)(struct V9fsRequest *req);
+
+    /* Flag to indicate that request is satisfied, ready for post-processing */
+    int done;
+    Coroutine *coroutine;
+} V9fsRequest;
+
+typedef struct V9fsThPool {
+    GThreadPool *pool;
+    GList *requests;
+    int rfd;
+    int wfd;
+} V9fsThPool;
+
+/* v9fs glib thread pool */
+extern V9fsThPool v9fs_pool;
+
+extern void v9fs_thread_routine(gpointer data, gpointer user_data);
+extern void v9fs_qemu_process_req_done(void *arg);
+extern void v9fs_qemu_submit_request(V9fsRequest *req);
+
+
+#endif
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index a2b6acc..21fb310 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -18,6 +18,9 @@ 
 #include "virtio-9p.h"
 #include "fsdev/qemu-fsdev.h"
 #include "virtio-9p-xattr.h"
+#include "virtio-9p-coth.h"
+
+static int notifier_fds[2];
 
 static uint32_t virtio_9p_get_features(VirtIODevice *vdev, uint32_t features)
 {
@@ -49,14 +52,13 @@  VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
     int i, len;
     struct stat stat;
     FsTypeEntry *fse;
-
+    V9fsThPool *p = &v9fs_pool;
 
     s = (V9fsState *)virtio_common_init("virtio-9p",
                                     VIRTIO_ID_9P,
                                     sizeof(struct virtio_9p_config)+
                                     MAX_TAG_LEN,
                                     sizeof(V9fsState));
-
     /* initialize pdu allocator */
     QLIST_INIT(&s->free_list);
     for (i = 0; i < (MAX_REQ - 1); i++) {
@@ -132,6 +134,26 @@  VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf)
                         s->tag_len;
     s->vdev.get_config = virtio_9p_get_config;
 
+    if (!g_thread_supported()) {
+        g_thread_init(NULL);
+    }
+
+    if (qemu_pipe(notifier_fds) == -1) {
+        fprintf(stderr, "failed to create fd's for virtio-9p\n");
+        exit(1);
+    }
+
+    p->pool = g_thread_pool_new(v9fs_thread_routine, p, -1, FALSE, NULL);
+    p->rfd = notifier_fds[0];
+    p->wfd = notifier_fds[1];
+
+    fcntl(p->rfd, F_SETFL, O_NONBLOCK);
+    fcntl(p->wfd, F_SETFL, O_NONBLOCK);
+
+    qemu_set_fd_handler(p->rfd, v9fs_qemu_process_req_done, NULL, NULL);
+
+    (void) v9fs_qemu_submit_request;
+
     return &s->vdev;
 }
 
diff --git a/hw/9pfs/virtio-9p.h b/hw/9pfs/virtio-9p.h
index 2bfbe62..699d3ab 100644
--- a/hw/9pfs/virtio-9p.h
+++ b/hw/9pfs/virtio-9p.h
@@ -510,5 +510,4 @@  static inline size_t do_pdu_unpack(void *dst, struct iovec *sg, int sg_count,
 }
 
 extern void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq);
-
 #endif