Quoting Liu Ping Fan (2013-08-08 01:26:07)
> Introduce struct EventsGSource. It will ease the usage of GSource
> associated with a group of files, which are dynamically allocated
> and release, ex, slirp.
>
> Signed-off-by: Liu Ping Fan <pingf...@linux.vnet.ibm.com>
> ---
> util/Makefile.objs | 1 +
> util/event_gsource.c | 94
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
> util/event_gsource.h | 37 +++++++++++++++++++++
> 3 files changed, 132 insertions(+)
> create mode 100644 util/event_gsource.c
> create mode 100644 util/event_gsource.h
>
> diff --git a/util/Makefile.objs b/util/Makefile.objs
> index dc72ab0..eec55bd 100644
> --- a/util/Makefile.objs
> +++ b/util/Makefile.objs
> @@ -11,3 +11,4 @@ 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 += hexdump.o
> util-obj-y += crc32c.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..4b9fa89
> --- /dev/null
> +++ b/util/event_gsource.c
> @@ -0,0 +1,94 @@
> +/*
> + * 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"
> +
> +GPollFD *events_source_add_pollfd(EventsGSource *src, int fd)
> +{
> + GPollFD *retfd;
> +
> + retfd = g_slice_alloc(sizeof(GPollFD));
> + retfd->events = 0;
> + retfd->fd = fd;
> + src->pollfds_list = g_list_append(src->pollfds_list, retfd);
I think moving to a GSource to simplify our mainloop implementation is
worthwhile even if we still rely on the global mutex and don't initially
support running those GSources outside the main iothread. But since being
able to eventually offload NetClient backends to seperate events loops to
support things like virtio-net dataplane is (I assume) still one of the
eventual goals, we should consider how to deal with concurrent
access to EventsGSource members.
For instance, In the case of slirp your dispatch callback may modify
src->pollfds_lists via
slirp_handler->tcp_input->socreate->events_source_add_pollfd(), while
another thread attempts to call socreate() via something like
net_slirp_hostfwd_add from the monitor (that's being driven by a separate
main loop).
So events_source_add_pollfd() and the various prepare/check/dispatch
functions should take a lock on pollfds_lists.
Dispatch is tricky though, since dispatch() invoke callbacks that may in
turn try to call events_source_add_pollfd(), as is the case above, in which
case you can deadlock.
I've been looking at the situation with regard to moving
qemu_set_fd_handler* to a GSource.
GLib has to deal with the same issue with regard to calls to
g_source_attach() while it's iterating through it's list of GSources. It
uses the GMainContext lock protect that list, and actually doesn't disallow
use of functions like g_source_attach() within a dispatch function. In
g_main_context_dispatch(), to work around the potential deadlock issue, it
actually builds up a separate list of dispatch cb functions and callback data,
then drops the GMainContext lock before iterating through that list and
calling the dispatch cb functions for all the GSources that fired.
This new list it builds up is safe from concurrent modification since
only the main loop thread can access it.
AFAIK there's 3 ways to deal with this type of concurrency:
a) Move to a 1-to-1 mapping between GPollFDs and EventGSources: we can then
let GLib handle managing our list of GPollFDs for us. We may still need a
mutex for other members of EventsGSource, but that lock can be taken from
within our prepare/check/dispatch functions and held for the duration of
the calls without any strange deadlock issues.
The major downside here is potentially performance. Currently we do an
O(n) lookup for stuff like qemu_set_fd_handler, where n is the number of
IOHandlers. If we move to 1-to-1 fd-to-GSource mapping, it's O(m), where
m is all GSources attached to the GMainContext. I'm not sure what the
performance penalty would be, but it will get worse as the number of
GSources increases. Not sure if this penalty is applicable for slirp,
as it doesn't seem like we need to do any sort of per-socket/fd lookup,
since we have a direct pointer to the GPollFD (if you take the approach
I mentioned above where you pass a GPollFD* to event_source_add_pollfd())
b) Stick with this many-to-1 mapping between GPollFDs and EventGSources: we
can then introduce variants of events_source_{add,remove}_pollfd
that don't take the EventGSource mutex so you can call them inside the
dispatch function, which is nasty, because in the case of slirp you'll then
end up with similar variants for things like socreate(), or:
c) Stick with the many-to-1 mapping, but do what glib does and build a list
of dispatch callbacks, then drop the EventGSource lock before calling them.
(I know for EventGSource we currently have 1 cb for all the FDs, but the
requirements are the same, you're just pushing synchronization concerns
higher up the stack to
slirp_event_source_add_pollfd/slirp_prepare/slirp_handler, and will run
into the same recursive locking issue in slirp_handler as a result. I think
it's better to handle it all in EventGSource so non-slirp users don't need
to implement the same trick, but the approach should be applicable either
way)
One concern here is that we might remove an event via
sofree()->slirp_event_source_remove_pollfd() just after
EventGSource->dispatch() drops EventGSource->mutex, so it might still end up
dispatching cb for that pfd even though we've deleted it. I think we can
have EventGSource set a flag in this case indicating it's in the middle of
dispatch, so that event_source_{add,remove}_pfd can wait on a condition
variable like EventGSource->cond_event_dispatch_complete before completing.
I'll be looking at c) WRT to the qemu_set_fd_handler stuff. Perhaps we can
re-use the same GSourceFuncs here as well, but don't let that bottleneck you,
just wanted to bring it up for discussion.
> + if (fd >= 0) {
> + g_source_add_poll(&src->source, retfd);
> + }
> +
> + return retfd;
> +}
> +
> +void events_source_remove_pollfd(EventsGSource *src, GPollFD *pollfd)
> +{
> + g_source_remove_poll(&src->source, pollfd);
> + src->pollfds_list = g_list_remove(src->pollfds_list, pollfd);
> + g_slice_free(GPollFD, pollfd);
> +}
> +
> +static gboolean events_source_check(GSource *src)
> +{
> + EventsGSource *nsrc = (EventsGSource *)src;
> + GList *cur;
> + GPollFD *gfd;
> +
> + cur = nsrc->pollfds_list;
> + while (cur) {
> + gfd = cur->data;
> + if (gfd->fd >= 0 && (gfd->revents & gfd->events)) {
> + return true;
> + }
> + cur = g_list_next(cur);
> + }
> +
> + return false;
> +}
> +
> +static gboolean events_source_dispatch(GSource *src, GSourceFunc cb,
> + gpointer data)
> +{
> + gboolean ret = false;
> +
> + if (cb) {
> + ret = cb(data);
> + }
> + return ret;
> +}
> +
> +EventsGSource *events_source_new(GPrepare prepare, GSourceFunc dispatch_cb,
> + void *opaque)
> +{
> + EventsGSource *src;
> + GSourceFuncs *gfuncs = g_new0(GSourceFuncs, 1);
> + gfuncs->prepare = prepare;
> + gfuncs->check = events_source_check,
> + gfuncs->dispatch = events_source_dispatch,
> +
> + src = (EventsGSource *)g_source_new(gfuncs, sizeof(EventsGSource));
> + src->gfuncs = gfuncs;
> + src->pollfds_list = NULL;
> + src->opaque = opaque;
> + g_source_set_callback(&src->source, dispatch_cb, src, NULL);
> +
> + return src;
> +}
> +
> +void events_source_release(EventsGSource *src)
> +{
> + assert(!src->pollfds_list);
> + g_free(src->gfuncs);
> + g_source_destroy(&src->source);
> +}
> diff --git a/util/event_gsource.h b/util/event_gsource.h
> new file mode 100644
> index 0000000..8755952
> --- /dev/null
> +++ b/util/event_gsource.h
> @@ -0,0 +1,37 @@
> +/*
> + * 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 gboolean (*GPrepare)(GSource *source, gint *timeout_);
> +
> +/* multi fd drive GSource*/
> +typedef struct EventsGSource {
> + GSource source;
> + /* a group of GPollFD which dynamically join or leave the GSource */
> + GList *pollfds_list;
> + GSourceFuncs *gfuncs;
> + void *opaque;
> +} EventsGSource;
> +
> +EventsGSource *events_source_new(GPrepare prepare, GSourceFunc dispatch_cb,
> + void *opaque);
> +void events_source_release(EventsGSource *src);
> +GPollFD *events_source_add_pollfd(EventsGSource *src, int fd);
> +void events_source_remove_pollfd(EventsGSource *src, GPollFD *pollfd);
> +#endif
> --
> 1.8.1.4
