From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45720) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UZQin-0000ix-Qn for qemu-devel@nongnu.org; Mon, 06 May 2013 15:05:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UZQil-0006CC-JW for qemu-devel@nongnu.org; Mon, 06 May 2013 15:05:13 -0400 Received: from mail-qa0-x22e.google.com ([2607:f8b0:400d:c00::22e]:59828) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UZQil-0006C7-Ee for qemu-devel@nongnu.org; Mon, 06 May 2013 15:05:11 -0400 Received: by mail-qa0-f46.google.com with SMTP id g10so1493059qah.12 for ; Mon, 06 May 2013 12:05:11 -0700 (PDT) Sender: fluxion Date: Mon, 6 May 2013 14:03:24 -0500 From: mdroth Message-ID: <20130506190324.GG1685@vm> References: <1367597032-28934-1-git-send-email-mdroth@linux.vnet.ibm.com> <1367597032-28934-8-git-send-email-mdroth@linux.vnet.ibm.com> <51876168.60501@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <51876168.60501@redhat.com> Subject: Re: [Qemu-devel] [PATCH 7/9] iohandler: associate with main event loop via a QSource List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org, stefanha@redhat.com, qemulist@gmail.com On Mon, May 06, 2013 at 09:53:12AM +0200, Paolo Bonzini wrote: > Il 03/05/2013 18:03, Michael Roth ha scritto: > > This introduces a GlibQContext wrapper around the main GMainContext > > event loop, and associates iohandlers with it via a QSource (which > > GlibQContext creates a GSource from so that it can be driven via > > GLib. A subsequent patch will drive the GlibQContext directly) > > > > We also add "QContext-aware" functionality to iohandler interfaces > > so that they can be bound to other QContext event loops, and add > > non-global set_fd_handler() interfaces to facilitate this. This is made > > possible by simply searching a given QContext for a QSource by the name > > of "iohandler" so that we can attach event handlers to the associated > > IOHandlerState. > > > > Signed-off-by: Michael Roth > > This patch is why I think that this is a bit overengineered. The main > loop is always glib-based, there should be no need to go through the > QSource abstraction. > > BTW, this is broken for Win32. The right thing to do here is to first > convert iohandler to a GSource in such a way that it works for both > POSIX and Win32, and then (if needed) we can later convert GSource to > QSource. Yup, forgot to note that Win32 was broken and on my TODO. I'll work on that and stick to using GSources for now. > > Paolo > > > --- > > include/qemu/main-loop.h | 31 +++++- > > iohandler.c | 238 ++++++++++++++++++++++++++++++++-------------- > > main-loop.c | 21 +++- > > 3 files changed, 213 insertions(+), 77 deletions(-) > > > > diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h > > index 6f0200a..dbadf9f 100644 > > --- a/include/qemu/main-loop.h > > +++ b/include/qemu/main-loop.h > > @@ -26,6 +26,7 @@ > > #define QEMU_MAIN_LOOP_H 1 > > > > #include "block/aio.h" > > +#include "qcontext/qcontext.h" > > > > #define SIG_IPI SIGUSR1 > > > > @@ -168,9 +169,24 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque); > > > > /* async I/O support */ > > > > +#define QSOURCE_IOHANDLER "iohandler" > > + > > typedef void IOReadHandler(void *opaque, const uint8_t *buf, int size); > > typedef int IOCanReadHandler(void *opaque); > > > > +QContext *qemu_get_qcontext(void); > > +/** > > + * iohandler_attach: Attach a QSource to a QContext > > + * > > + * This enables the use of IOHandler interfaces such as > > + * set_fd_handler() on the given QContext. IOHandler lists will be > > + * tracked/handled/dispatched based on a named QSource that is added to > > + * the QContext > > + * > > + * @ctx: A QContext to add an IOHandler QSource to > > + */ > > +void iohandler_attach(QContext *ctx); > > + > > /** > > * qemu_set_fd_handler2: Register a file descriptor with the main loop > > * > > @@ -217,6 +233,13 @@ int qemu_set_fd_handler2(int fd, > > IOHandler *fd_write, > > void *opaque); > > > > +int set_fd_handler2(QContext *ctx, > > + int fd, > > + IOCanReadHandler *fd_read_poll, > > + IOHandler *fd_read, > > + IOHandler *fd_write, > > + void *opaque); > > + > > /** > > * qemu_set_fd_handler: Register a file descriptor with the main loop > > * > > @@ -250,6 +273,12 @@ int qemu_set_fd_handler(int fd, > > IOHandler *fd_write, > > void *opaque); > > > > +int set_fd_handler(QContext *ctx, > > + int fd, > > + IOHandler *fd_read, > > + IOHandler *fd_write, > > + void *opaque); > > + > > #ifdef CONFIG_POSIX > > /** > > * qemu_add_child_watch: Register a child process for reaping. > > @@ -302,8 +331,6 @@ void qemu_mutex_unlock_iothread(void); > > /* internal interfaces */ > > > > void qemu_fd_register(int fd); > > -void qemu_iohandler_fill(GArray *pollfds); > > -void qemu_iohandler_poll(GArray *pollfds, int rc); > > > > QEMUBH *qemu_bh_new(QEMUBHFunc *cb, void *opaque); > > void qemu_bh_schedule_idle(QEMUBH *bh); > > diff --git a/iohandler.c b/iohandler.c > > index ae2ef8f..8625272 100644 > > --- a/iohandler.c > > +++ b/iohandler.c > > @@ -41,38 +41,170 @@ typedef struct IOHandlerRecord { > > int fd; > > int pollfds_idx; > > bool deleted; > > + GPollFD pfd; > > + bool pfd_added; > > } IOHandlerRecord; > > > > -static QLIST_HEAD(, IOHandlerRecord) io_handlers = > > - QLIST_HEAD_INITIALIZER(io_handlers); > > +typedef struct IOHandlerState { > > + QLIST_HEAD(, IOHandlerRecord) io_handlers; > > +} IOHandlerState; > > > > +static bool iohandler_prepare(QSource *qsource, int *timeout) > > +{ > > + QSourceClass *qsourcek = QSOURCE_GET_CLASS(qsource); > > + IOHandlerState *s = qsourcek->get_user_data(qsource); > > + IOHandlerRecord *ioh; > > > > -/* XXX: fd_read_poll should be suppressed, but an API change is > > - necessary in the character devices to suppress fd_can_read(). */ > > -int qemu_set_fd_handler2(int fd, > > - IOCanReadHandler *fd_read_poll, > > - IOHandler *fd_read, > > - IOHandler *fd_write, > > - void *opaque) > > + QLIST_FOREACH(ioh, &s->io_handlers, next) { > > + int events = 0; > > + > > + if (ioh->deleted) > > + continue; > > + > > + if (ioh->fd_read && > > + (!ioh->fd_read_poll || > > + ioh->fd_read_poll(ioh->opaque) != 0)) { > > + events |= G_IO_IN | G_IO_HUP | G_IO_ERR; > > + } > > + if (ioh->fd_write) { > > + events |= G_IO_OUT | G_IO_ERR; > > + } > > + if (events) { > > + ioh->pfd.fd = ioh->fd; > > + ioh->pfd.events = events; > > + if (!ioh->pfd_added) { > > + qsourcek->add_poll(qsource, &ioh->pfd); > > + ioh->pfd_added = true; > > + } > > + } else { > > + ioh->pfd.events = 0; > > + ioh->pfd.revents = 0; > > + } > > + } > > + *timeout = 10; > > + return false; > > +} > > + > > +static bool iohandler_check(QSource *qsource) > > { > > + QSourceClass *sourcek = QSOURCE_GET_CLASS(qsource); > > + IOHandlerState *s = sourcek->get_user_data(qsource); > > IOHandlerRecord *ioh; > > > > + QLIST_FOREACH(ioh, &s->io_handlers, next) { > > + if (ioh->deleted) { > > + continue; > > + } > > + if (ioh->pfd.revents) { > > + return true; > > + } > > + } > > + > > + return false; > > +} > > + > > +static bool iohandler_dispatch(QSource *qsource) > > +{ > > + QSourceClass *sourcek = QSOURCE_GET_CLASS(qsource); > > + IOHandlerState *s = sourcek->get_user_data(qsource); > > + IOHandlerRecord *pioh, *ioh; > > + > > + QLIST_FOREACH_SAFE(ioh, &s->io_handlers, next, pioh) { > > + int revents = ioh->pfd.revents; > > + if (!ioh->deleted && ioh->fd_read && > > + (revents && (G_IO_IN | G_IO_HUP | G_IO_ERR))) { > > + ioh->fd_read(ioh->opaque); > > + } > > + > > + if (!ioh->deleted && ioh->fd_write && > > + (revents & (G_IO_OUT | G_IO_ERR))) { > > + ioh->fd_write(ioh->opaque); > > + } > > + > > + /* Do this last in case read/write handlers marked it for deletion */ > > + if (ioh->deleted) { > > + if (ioh->pfd_added) { > > + sourcek->remove_poll(qsource, &ioh->pfd); > > + } > > + QLIST_REMOVE(ioh, next); > > + g_free(ioh); > > + } > > + } > > + > > + return true; > > +} > > + > > +static void iohandler_finalize(QSource *qsource) > > +{ > > + QSourceClass *sourcek = QSOURCE_GET_CLASS(qsource); > > + IOHandlerState *s = sourcek->get_user_data(qsource); > > + IOHandlerRecord *pioh, *ioh; > > + > > + QLIST_FOREACH_SAFE(ioh, &s->io_handlers, next, pioh) { > > + if (ioh->pfd_added) { > > + sourcek->remove_poll(qsource, &ioh->pfd); > > + } > > + QLIST_REMOVE(ioh, next); > > + g_free(ioh); > > + } > > + > > + g_free(s); > > +} > > + > > +static const QSourceFuncs iohandler_funcs = { > > + iohandler_prepare, > > + iohandler_check, > > + iohandler_dispatch, > > + iohandler_finalize, > > +}; > > + > > +void iohandler_attach(QContext *ctx) > > +{ > > + IOHandlerState *s; > > + QSource *qsource; > > + > > + s = g_new0(IOHandlerState, 1); > > + QLIST_INIT(&s->io_handlers); > > + > > + qsource = qsource_new(iohandler_funcs, NULL, QSOURCE_IOHANDLER, s); > > + qcontext_attach(ctx, qsource, NULL); > > +} > > + > > +int set_fd_handler2(QContext *ctx, > > + int fd, > > + IOCanReadHandler *fd_read_poll, > > + IOHandler *fd_read, > > + IOHandler *fd_write, > > + void *opaque) > > +{ > > + QSourceClass *sourcek; > > + QSource *source; > > + IOHandlerState *s; > > + IOHandlerRecord *ioh; > > + > > + source = qcontext_find_source_by_name(ctx, QSOURCE_IOHANDLER); > > + if (!source) { > > + assert(0); > > + } > > + sourcek = QSOURCE_GET_CLASS(source); > > + s = sourcek->get_user_data(source); > > + > > assert(fd >= 0); > > > > if (!fd_read && !fd_write) { > > - QLIST_FOREACH(ioh, &io_handlers, next) { > > + QLIST_FOREACH(ioh, &s->io_handlers, next) { > > if (ioh->fd == fd) { > > ioh->deleted = 1; > > break; > > } > > } > > } else { > > - QLIST_FOREACH(ioh, &io_handlers, next) { > > + QLIST_FOREACH(ioh, &s->io_handlers, next) { > > if (ioh->fd == fd) > > goto found; > > } > > ioh = g_malloc0(sizeof(IOHandlerRecord)); > > - QLIST_INSERT_HEAD(&io_handlers, ioh, next); > > + QLIST_INSERT_HEAD(&s->io_handlers, ioh, next); > > found: > > ioh->fd = fd; > > ioh->fd_read_poll = fd_read_poll; > > @@ -86,74 +218,34 @@ int qemu_set_fd_handler2(int fd, > > return 0; > > } > > > > -int qemu_set_fd_handler(int fd, > > - IOHandler *fd_read, > > - IOHandler *fd_write, > > - void *opaque) > > +int set_fd_handler(QContext *ctx, > > + int fd, > > + IOHandler *fd_read, > > + IOHandler *fd_write, > > + void *opaque) > > { > > - return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque); > > + return set_fd_handler2(ctx, fd, NULL, fd_read, fd_write, opaque); > > } > > > > -void qemu_iohandler_fill(GArray *pollfds) > > +/* XXX: fd_read_poll should be suppressed, but an API change is > > + necessary in the character devices to suppress fd_can_read(). */ > > +int qemu_set_fd_handler2(int fd, > > + IOCanReadHandler *fd_read_poll, > > + IOHandler *fd_read, > > + IOHandler *fd_write, > > + void *opaque) > > { > > - IOHandlerRecord *ioh; > > - > > - QLIST_FOREACH(ioh, &io_handlers, next) { > > - int events = 0; > > - > > - if (ioh->deleted) > > - continue; > > - if (ioh->fd_read && > > - (!ioh->fd_read_poll || > > - ioh->fd_read_poll(ioh->opaque) != 0)) { > > - events |= G_IO_IN | G_IO_HUP | G_IO_ERR; > > - } > > - if (ioh->fd_write) { > > - events |= G_IO_OUT | G_IO_ERR; > > - } > > - if (events) { > > - GPollFD pfd = { > > - .fd = ioh->fd, > > - .events = events, > > - }; > > - ioh->pollfds_idx = pollfds->len; > > - g_array_append_val(pollfds, pfd); > > - } else { > > - ioh->pollfds_idx = -1; > > - } > > - } > > + return set_fd_handler2(qemu_get_qcontext(), fd, > > + fd_read_poll, fd_read, fd_write, > > + opaque); > > } > > > > -void qemu_iohandler_poll(GArray *pollfds, int ret) > > +int qemu_set_fd_handler(int fd, > > + IOHandler *fd_read, > > + IOHandler *fd_write, > > + void *opaque) > > { > > - if (ret > 0) { > > - IOHandlerRecord *pioh, *ioh; > > - > > - QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) { > > - int revents = 0; > > - > > - if (!ioh->deleted && ioh->pollfds_idx != -1) { > > - GPollFD *pfd = &g_array_index(pollfds, GPollFD, > > - ioh->pollfds_idx); > > - revents = pfd->revents; > > - } > > - > > - if (!ioh->deleted && ioh->fd_read && > > - (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR))) { > > - ioh->fd_read(ioh->opaque); > > - } > > - if (!ioh->deleted && ioh->fd_write && > > - (revents & (G_IO_OUT | G_IO_ERR))) { > > - ioh->fd_write(ioh->opaque); > > - } > > - > > - /* Do this last in case read/write handlers marked it for deletion */ > > - if (ioh->deleted) { > > - QLIST_REMOVE(ioh, next); > > - g_free(ioh); > > - } > > - } > > - } > > + return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque); > > } > > > > /* reaping of zombies. right now we're not passing the status to > > diff --git a/main-loop.c b/main-loop.c > > index f46aece..ae284a6 100644 > > --- a/main-loop.c > > +++ b/main-loop.c > > @@ -27,6 +27,8 @@ > > #include "slirp/slirp.h" > > #include "qemu/main-loop.h" > > #include "block/aio.h" > > +#include "qcontext/qcontext.h" > > +#include "qcontext/glib-qcontext.h" > > > > #ifndef _WIN32 > > > > @@ -107,6 +109,13 @@ static int qemu_signal_init(void) > > } > > #endif > > > > +static QContext *qemu_qcontext; > > + > > +QContext *qemu_get_qcontext(void) > > +{ > > + return qemu_qcontext; > > +} > > + > > static AioContext *qemu_aio_context; > > > > AioContext *qemu_get_aio_context(void) > > @@ -128,6 +137,7 @@ int qemu_init_main_loop(void) > > { > > int ret; > > GSource *src; > > + Error *err = NULL; > > > > init_clocks(); > > if (init_timer_alarm() < 0) { > > @@ -135,6 +145,15 @@ int qemu_init_main_loop(void) > > exit(1); > > } > > > > + qemu_qcontext = QCONTEXT(glib_qcontext_new("main", false, &err)); > > + if (err) { > > + g_warning("error initializing main qcontext"); > > + error_free(err); > > + return -1; > > + } > > + > > + iohandler_attach(qemu_qcontext); > > + > > ret = qemu_signal_init(); > > if (ret) { > > return ret; > > @@ -464,9 +483,7 @@ int main_loop_wait(int nonblocking) > > slirp_update_timeout(&timeout); > > slirp_pollfds_fill(gpollfds); > > #endif > > - qemu_iohandler_fill(gpollfds); > > ret = os_host_main_loop_wait(timeout); > > - qemu_iohandler_poll(gpollfds, ret); > > #ifdef CONFIG_SLIRP > > slirp_pollfds_poll(gpollfds, (ret < 0)); > > #endif > > >