Message ID | 1305233867-4367-2-git-send-email-jvrao@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
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
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
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 --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