Quoting Michael Roth (2013-08-08 16:03:30) > 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.
I take that back, would still need to drop EventsGSource mutex, in EventsGSource->dispatch prior calling the dispatch cb, but this is trivial unless the dispatch cb or associated user_data can be modified, which isn't the case with this interface.
