From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36525) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V9qj4-0003mp-ON for qemu-devel@nongnu.org; Thu, 15 Aug 2013 02:08:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V9qiv-0008UI-I9 for qemu-devel@nongnu.org; Thu, 15 Aug 2013 02:08:02 -0400 Received: from e28smtp06.in.ibm.com ([122.248.162.6]:53700) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V9qiu-0008O1-KC for qemu-devel@nongnu.org; Thu, 15 Aug 2013 02:07:53 -0400 Received: from /spool/local by e28smtp06.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 15 Aug 2013 11:28:29 +0530 Received: from d28relay02.in.ibm.com (d28relay02.in.ibm.com [9.184.220.59]) by d28dlp02.in.ibm.com (Postfix) with ESMTP id 12F84394004D for ; Thu, 15 Aug 2013 11:37:36 +0530 (IST) Received: from d28av01.in.ibm.com (d28av01.in.ibm.com [9.184.220.63]) by d28relay02.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r7F698Kc37617708 for ; Thu, 15 Aug 2013 11:39:09 +0530 Received: from d28av01.in.ibm.com (localhost [127.0.0.1]) by d28av01.in.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id r7F67goP011619 for ; Thu, 15 Aug 2013 11:37:43 +0530 Message-ID: <520C7019.5040309@linux.vnet.ibm.com> Date: Thu, 15 Aug 2013 14:07:21 +0800 From: Wenchao Xia MIME-Version: 1.0 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> In-Reply-To: <51876168.60501@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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: qemulist@gmail.com, aliguori@us.ibm.com, Michael Roth , stefanha@redhat.com, qemu-devel@nongnu.org 于 2013-5-6 15:53, Paolo Bonzini 写道: > 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. > I thought to use glib-based eventloop before, but found that AioContext's pending request need to be flushed in release operation, which can't be done in g_main_context_release. This brings difficult to do every thing based on glib's API. Do you think we should stick to QContext to wrap or hide GMainContext? > 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. > > 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 >> > > -- Best Regards Wenchao Xia