| Message ID | 1366187964-14265-2-git-send-email-qemulist@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Apr 17, 2013 at 04:39:10PM +0800, Liu Ping Fan wrote: > +static gboolean prepare(GSource *src, gint *time) > +{ > + EventGSource *nsrc = (EventGSource *)src; > + int events = 0; > + > + if (!nsrc->readable && !nsrc->writable) { > + return false; > + } > + if (nsrc->readable && nsrc->readable(nsrc->opaque)) { > + events |= G_IO_IN; > + } > + if ((nsrc->writable) && nsrc->writable(nsrc->opaque)) { > + events |= G_IO_OUT; > + } G_IO_ERR, G_IO_HUP, G_IO_PRI? Here is the select(2) to GCondition mapping: rfds -> G_IO_IN | G_IO_HUP | G_IO_ERR wfds -> G_IO_OUT | G_IO_ERR xfds -> G_IO_PRI In other words, we're missing events by just using G_IO_IN and G_IO_OUT. Whether that matters depends on EventGSource users. For sockets it can matter. > +void event_source_release(EventGSource *src) > +{ > + g_source_destroy(&src->source); Leaks src. > +} > + > +GPollFD *events_source_get_gfd(EventsGSource *src, int fd) events_source_add_fd() seems like a better name since this function always allocates a new GPollFD, it never "gets" an existing one. > +{ > + GPollFD *retfd; > + unsigned long idx; > + > + idx = find_first_zero_bit(src->alloc_bmp, src->bmp_sz); > + if (idx == src->bmp_sz) { > + //idx = src->bmp_sz; Commented out line. > +void events_source_close_gfd(EventsGSource *src, GPollFD *pollfd) "close" usually means close(2). I suggest "remove" instead. > +EventsGSource *events_source_new(GSourceFuncs *funcs, GSourceFunc dispatch_cb, void *opaque) > +{ > + EventsGSource *src = (EventsGSource *)g_source_new(funcs, sizeof(EventsGSource)); > + > + /* 8bits size at initial */ > + src->bmp_sz = 8; > + src->alloc_bmp = g_malloc0(src->bmp_sz >> 3); This is unportable. alloc_bmp is unsigned long, you are allocating just one byte! Please drop the bitmap approach and use a doubly-linked list or another glib container type of your choice. It needs 3 operations: add, remove, and iterate. > +/* multi fd drive gsource*/ > +typedef struct EventsGSource { > + GSource source; > + /* 8 for initial, stand for 8 pollfds */ > + unsigned int bmp_sz; > + unsigned long *alloc_bmp; > + GPollFD *pollfds; > + void *opaque; > +} EventsGSource; > + > +EventsGSource *events_source_new(GSourceFuncs *funcs, GSourceFunc dispatch_cb, void *opaque); > +void events_source_release(EventsGSource *src); > +gboolean events_source_check(GSource *src); > +gboolean events_source_dispatch(GSource *src, GSourceFunc cb, gpointer data); > +GPollFD *events_source_get_gfd(EventsGSource *src, int fd); > +void events_source_close_gfd(EventsGSource *src, GPollFD *pollfd); Why are check/dispatch public? Perhaps events_source_new() just needs a prepare() argument instead of exposing GSourceFuncs.
On Thu, Apr 18, 2013 at 10:01 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Wed, Apr 17, 2013 at 04:39:10PM +0800, Liu Ping Fan wrote: >> +static gboolean prepare(GSource *src, gint *time) >> +{ >> + EventGSource *nsrc = (EventGSource *)src; >> + int events = 0; >> + >> + if (!nsrc->readable && !nsrc->writable) { >> + return false; >> + } >> + if (nsrc->readable && nsrc->readable(nsrc->opaque)) { >> + events |= G_IO_IN; >> + } >> + if ((nsrc->writable) && nsrc->writable(nsrc->opaque)) { >> + events |= G_IO_OUT; >> + } > > G_IO_ERR, G_IO_HUP, G_IO_PRI? > > Here is the select(2) to GCondition mapping: > rfds -> G_IO_IN | G_IO_HUP | G_IO_ERR > wfds -> G_IO_OUT | G_IO_ERR > xfds -> G_IO_PRI > Does G_IO_PRI only happen on read-in direction? > In other words, we're missing events by just using G_IO_IN and G_IO_OUT. > Whether that matters depends on EventGSource users. For sockets it can > matter. > I think you mean just prepare all of them, and let the dispatch decide how to handle them, right? >> +void event_source_release(EventGSource *src) >> +{ >> + g_source_destroy(&src->source); > > Leaks src. > All of the mem used by EventGSource are allocated by g_source_new, so g_source_destroy can reclaim all of them. >> +} >> + >> +GPollFD *events_source_get_gfd(EventsGSource *src, int fd) > > events_source_add_fd() seems like a better name since this function > always allocates a new GPollFD, it never "gets" an existing one. > Thanks for pointing out. But see the reply to "unportable alloc_bmp" below. >> +{ >> + GPollFD *retfd; >> + unsigned long idx; >> + >> + idx = find_first_zero_bit(src->alloc_bmp, src->bmp_sz); >> + if (idx == src->bmp_sz) { >> + //idx = src->bmp_sz; > > Commented out line. > Apply, >> +void events_source_close_gfd(EventsGSource *src, GPollFD *pollfd) > > "close" usually means close(2). I suggest "remove" instead. > >> +EventsGSource *events_source_new(GSourceFuncs *funcs, GSourceFunc dispatch_cb, void *opaque) >> +{ >> + EventsGSource *src = (EventsGSource *)g_source_new(funcs, sizeof(EventsGSource)); >> + >> + /* 8bits size at initial */ >> + src->bmp_sz = 8; >> + src->alloc_bmp = g_malloc0(src->bmp_sz >> 3); > > This is unportable. alloc_bmp is unsigned long, you are allocating just > one byte! > I had thought that resorting to bmp_sz to guarantee the bit-ops on alloc_bmp. And if EventsGSource->pollfds is allocated with 64 instance at initialize, it cost too much. I can fix it with more fine code when alloc_bmp's size growing. > Please drop the bitmap approach and use a doubly-linked list or another > glib container type of your choice. It needs 3 operations: add, remove, > and iterate. > But as the case for slirp, owning to network's connection and disconnection, the slirp's sockets can be dynamically changed quickly. The bitmap approach is something like slab, while glib container type lacks such support (maybe using two GArray inuse[], free[]). >> +/* multi fd drive gsource*/ >> +typedef struct EventsGSource { >> + GSource source; >> + /* 8 for initial, stand for 8 pollfds */ >> + unsigned int bmp_sz; >> + unsigned long *alloc_bmp; >> + GPollFD *pollfds; >> + void *opaque; >> +} EventsGSource; >> + >> +EventsGSource *events_source_new(GSourceFuncs *funcs, GSourceFunc dispatch_cb, void *opaque); >> +void events_source_release(EventsGSource *src); >> +gboolean events_source_check(GSource *src); >> +gboolean events_source_dispatch(GSource *src, GSourceFunc cb, gpointer data); >> +GPollFD *events_source_get_gfd(EventsGSource *src, int fd); >> +void events_source_close_gfd(EventsGSource *src, GPollFD *pollfd); > > Why are check/dispatch public? Perhaps events_source_new() just needs a > prepare() argument instead of exposing GSourceFuncs. Ok, that is more closed and reasonable. Thanks, Pingfan
On Fri, Apr 19, 2013 at 02:52:08PM +0800, liu ping fan wrote: > On Thu, Apr 18, 2013 at 10:01 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > > On Wed, Apr 17, 2013 at 04:39:10PM +0800, Liu Ping Fan wrote: > >> +static gboolean prepare(GSource *src, gint *time) > >> +{ > >> + EventGSource *nsrc = (EventGSource *)src; > >> + int events = 0; > >> + > >> + if (!nsrc->readable && !nsrc->writable) { > >> + return false; > >> + } > >> + if (nsrc->readable && nsrc->readable(nsrc->opaque)) { > >> + events |= G_IO_IN; > >> + } > >> + if ((nsrc->writable) && nsrc->writable(nsrc->opaque)) { > >> + events |= G_IO_OUT; > >> + } > > > > G_IO_ERR, G_IO_HUP, G_IO_PRI? > > > > Here is the select(2) to GCondition mapping: > > rfds -> G_IO_IN | G_IO_HUP | G_IO_ERR > > wfds -> G_IO_OUT | G_IO_ERR > > xfds -> G_IO_PRI > > > Does G_IO_PRI only happen on read-in direction? Yes. > > In other words, we're missing events by just using G_IO_IN and G_IO_OUT. > > Whether that matters depends on EventGSource users. For sockets it can > > matter. > > > I think you mean just prepare all of them, and let the dispatch decide > how to handle them, right? The user must decide which events to monitor. Otherwise the event loop may run at 100% CPU due to events that are monitored but not handled by the user. > >> +void event_source_release(EventGSource *src) > >> +{ > >> + g_source_destroy(&src->source); > > > > Leaks src. > > > All of the mem used by EventGSource are allocated by g_source_new, so > g_source_destroy can reclaim all of them. Okay, then the bug is events_source_release() which calls g_free(src) after g_source_destroy(&src->source). > >> +EventsGSource *events_source_new(GSourceFuncs *funcs, GSourceFunc dispatch_cb, void *opaque) > >> +{ > >> + EventsGSource *src = (EventsGSource *)g_source_new(funcs, sizeof(EventsGSource)); > >> + > >> + /* 8bits size at initial */ > >> + src->bmp_sz = 8; > >> + src->alloc_bmp = g_malloc0(src->bmp_sz >> 3); > > > > This is unportable. alloc_bmp is unsigned long, you are allocating just > > one byte! > > > I had thought that resorting to bmp_sz to guarantee the bit-ops on > alloc_bmp. And if EventsGSource->pollfds is allocated with 64 instance > at initialize, it cost too much. I can fix it with more fine code > when alloc_bmp's size growing. > > > Please drop the bitmap approach and use a doubly-linked list or another > > glib container type of your choice. It needs 3 operations: add, remove, > > and iterate. > > > But as the case for slirp, owning to network's connection and > disconnection, the slirp's sockets can be dynamically changed quickly. > The bitmap approach is something like slab, while glib container > type lacks such support (maybe using two GArray inuse[], free[]). Doubly-linked list insertion and removal are O(1). The linked list can be allocated with g_slice_alloc() which is efficient. Iterating linked lists isn't cache-friendly but this is premature optimization. I bet the userspace TCP - pulling packets apart - is more of a CPU bottleneck than a doubly-linked list of fds. Please use existing data structures instead of writing them from scratch unless there is a real need (e.g. profiling shows it matters).
On Fri, Apr 19, 2013 at 7:59 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote: > On Fri, Apr 19, 2013 at 02:52:08PM +0800, liu ping fan wrote: >> On Thu, Apr 18, 2013 at 10:01 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: >> > On Wed, Apr 17, 2013 at 04:39:10PM +0800, Liu Ping Fan wrote: >> >> +static gboolean prepare(GSource *src, gint *time) >> >> +{ >> >> + EventGSource *nsrc = (EventGSource *)src; >> >> + int events = 0; >> >> + >> >> + if (!nsrc->readable && !nsrc->writable) { >> >> + return false; >> >> + } >> >> + if (nsrc->readable && nsrc->readable(nsrc->opaque)) { >> >> + events |= G_IO_IN; >> >> + } >> >> + if ((nsrc->writable) && nsrc->writable(nsrc->opaque)) { >> >> + events |= G_IO_OUT; >> >> + } >> > >> > G_IO_ERR, G_IO_HUP, G_IO_PRI? >> > >> > Here is the select(2) to GCondition mapping: >> > rfds -> G_IO_IN | G_IO_HUP | G_IO_ERR >> > wfds -> G_IO_OUT | G_IO_ERR >> > xfds -> G_IO_PRI >> > >> Does G_IO_PRI only happen on read-in direction? > > Yes. > >> > In other words, we're missing events by just using G_IO_IN and G_IO_OUT. >> > Whether that matters depends on EventGSource users. For sockets it can >> > matter. >> > >> I think you mean just prepare all of them, and let the dispatch decide >> how to handle them, right? > > The user must decide which events to monitor. Otherwise the event loop > may run at 100% CPU due to events that are monitored but not handled by > the user. > >> >> +void event_source_release(EventGSource *src) >> >> +{ >> >> + g_source_destroy(&src->source); >> > >> > Leaks src. >> > >> All of the mem used by EventGSource are allocated by g_source_new, so >> g_source_destroy can reclaim all of them. > > Okay, then the bug is events_source_release() which calls g_free(src) > after g_source_destroy(&src->source). > >> >> +EventsGSource *events_source_new(GSourceFuncs *funcs, GSourceFunc dispatch_cb, void *opaque) >> >> +{ >> >> + EventsGSource *src = (EventsGSource *)g_source_new(funcs, sizeof(EventsGSource)); >> >> + >> >> + /* 8bits size at initial */ >> >> + src->bmp_sz = 8; >> >> + src->alloc_bmp = g_malloc0(src->bmp_sz >> 3); >> > >> > This is unportable. alloc_bmp is unsigned long, you are allocating just >> > one byte! >> > >> I had thought that resorting to bmp_sz to guarantee the bit-ops on >> alloc_bmp. And if EventsGSource->pollfds is allocated with 64 instance >> at initialize, it cost too much. I can fix it with more fine code >> when alloc_bmp's size growing. >> >> > Please drop the bitmap approach and use a doubly-linked list or another >> > glib container type of your choice. It needs 3 operations: add, remove, >> > and iterate. >> > >> But as the case for slirp, owning to network's connection and >> disconnection, the slirp's sockets can be dynamically changed quickly. >> The bitmap approach is something like slab, while glib container >> type lacks such support (maybe using two GArray inuse[], free[]). > > Doubly-linked list insertion and removal are O(1). > > The linked list can be allocated with g_slice_alloc() which is > efficient. > > Iterating linked lists isn't cache-friendly but this is premature > optimization. I bet the userspace TCP - pulling packets apart - is more > of a CPU bottleneck than a doubly-linked list of fds. > > Please use existing data structures instead of writing them from scratch > unless there is a real need (e.g. profiling shows it matters). Ok, thanks for the detail explaining. Regards, Pingfan
diff --git a/util/Makefile.objs b/util/Makefile.objs index 495a178..a676d7d 100644 --- a/util/Makefile.objs +++ b/util/Makefile.objs @@ -8,3 +8,4 @@ util-obj-y += error.o qemu-error.o util-obj-$(CONFIG_POSIX) += compatfd.o util-obj-y += iov.o aes.o qemu-config.o qemu-sockets.o uri.o notify.o util-obj-y += qemu-option.o qemu-progress.o +util-obj-y += event_gsource.o diff --git a/util/event_gsource.c b/util/event_gsource.c new file mode 100644 index 0000000..b255c47 --- /dev/null +++ b/util/event_gsource.c @@ -0,0 +1,169 @@ +/* + * Copyright (C) 2013 IBM + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; under version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + */ + +#include "event_gsource.h" +#include "qemu/bitops.h" + +static gboolean prepare(GSource *src, gint *time) +{ + EventGSource *nsrc = (EventGSource *)src; + int events = 0; + + if (!nsrc->readable && !nsrc->writable) { + return false; + } + if (nsrc->readable && nsrc->readable(nsrc->opaque)) { + events |= G_IO_IN; + } + if ((nsrc->writable) && nsrc->writable(nsrc->opaque)) { + events |= G_IO_OUT; + } + nsrc->gfd.events = events; + + return false; +} + +static gboolean check(GSource *src) +{ + EventGSource *nsrc = (EventGSource *)src; + + if (nsrc->gfd.revents & nsrc->gfd.events) { + return true; + } + return false; +} + +static gboolean dispatch(GSource *src, GSourceFunc cb, gpointer data) +{ + gboolean ret = false; + + if (cb) { + ret = cb(data); + } + return ret; +} + +static GSourceFuncs net_gsource_funcs = { + prepare, + check, + dispatch, + NULL +}; + +EventGSource *event_source_new(int fd, GSourceFunc dispatch_cb, void *opaque) +{ + EventGSource *nsrc = (EventGSource *)g_source_new(&net_gsource_funcs, + sizeof(EventGSource)); + nsrc->gfd.fd = fd; + nsrc->opaque = opaque; + g_source_set_callback(&nsrc->source, dispatch_cb, nsrc, NULL); + g_source_add_poll(&nsrc->source, &nsrc->gfd); + + return nsrc; +} + +void event_source_release(EventGSource *src) +{ + g_source_destroy(&src->source); +} + +GPollFD *events_source_get_gfd(EventsGSource *src, int fd) +{ + GPollFD *retfd; + unsigned long idx; + + idx = find_first_zero_bit(src->alloc_bmp, src->bmp_sz); + if (idx == src->bmp_sz) { + //idx = src->bmp_sz; + src->bmp_sz += 8; + src->alloc_bmp = g_realloc(src->alloc_bmp, src->bmp_sz >> 3); + src->pollfds = g_realloc(src->pollfds, src->bmp_sz); + } + set_bit(idx, src->alloc_bmp); + + retfd = src->pollfds + idx; + retfd->events = 0; + retfd->fd = fd; + if (fd > 0) { + g_source_add_poll(&src->source, retfd); + } + + return retfd; +} + +void events_source_close_gfd(EventsGSource *src, GPollFD *pollfd) +{ + unsigned long idx; + + idx = pollfd - src->pollfds; + clear_bit(idx, src->alloc_bmp); + g_source_remove_poll(&src->source, pollfd); +} + +gboolean events_source_check(GSource *src) +{ + unsigned long idx = 0; + EventsGSource *nsrc = (EventsGSource *)src; + unsigned long sz = nsrc->bmp_sz; + GPollFD *gfd; + + do { + idx = find_next_bit(nsrc->alloc_bmp, sz, idx); + if (idx < sz) { + gfd = nsrc->pollfds + idx; + if (gfd->revents & gfd->events) { + return true; + } + idx++; + continue; + } else { + return false; + } + } while (true); +} + +gboolean events_source_dispatch(GSource *src, GSourceFunc cb, gpointer data) +{ + gboolean ret = false; + + if (cb) { + ret = cb(data); + } + return ret; +} + +EventsGSource *events_source_new(GSourceFuncs *funcs, GSourceFunc dispatch_cb, void *opaque) +{ + EventsGSource *src = (EventsGSource *)g_source_new(funcs, sizeof(EventsGSource)); + + /* 8bits size at initial */ + src->bmp_sz = 8; + src->alloc_bmp = g_malloc0(src->bmp_sz >> 3); + src->pollfds = g_malloc0(8 * sizeof(GPollFD)); + src->opaque = opaque; + g_source_set_callback(&src->source, dispatch_cb, src, NULL); + + return src; +} + +void events_source_release(EventsGSource *src) +{ + g_free(src->alloc_bmp); + g_free(src->pollfds); + g_source_destroy(&src->source); + g_free(src); +} + diff --git a/util/event_gsource.h b/util/event_gsource.h new file mode 100644 index 0000000..fd07e6d --- /dev/null +++ b/util/event_gsource.h @@ -0,0 +1,54 @@ +/* + * Copyright (C) 2013 IBM + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; under version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + */ + +#ifndef EVENT_GSOURCE_H +#define EVENT_GSOURCE_H +#include "qemu-common.h" + +typedef bool (*Pollable)(void *opaque); + +/* single fd drive gsource */ +typedef struct EventGSource { + GSource source; + GPollFD gfd; + Pollable readable; + Pollable writable; + void *opaque; +} EventGSource; + +EventGSource *event_source_new(int fd, GSourceFunc dispatch_cb, void *opaque); +void event_source_release(EventGSource *src); + +/* multi fd drive gsource*/ +typedef struct EventsGSource { + GSource source; + /* 8 for initial, stand for 8 pollfds */ + unsigned int bmp_sz; + unsigned long *alloc_bmp; + GPollFD *pollfds; + void *opaque; +} EventsGSource; + +EventsGSource *events_source_new(GSourceFuncs *funcs, GSourceFunc dispatch_cb, void *opaque); +void events_source_release(EventsGSource *src); +gboolean events_source_check(GSource *src); +gboolean events_source_dispatch(GSource *src, GSourceFunc cb, gpointer data); +GPollFD *events_source_get_gfd(EventsGSource *src, int fd); +void events_source_close_gfd(EventsGSource *src, GPollFD *pollfd); + + + +#endif