Re: [Qemu-devel] [PATCH v1 1/5] util: introduce gsource event abstraction

[Michael Roth <mdroth@linux.vnet.ibm.com>]

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 <pingfank@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.

Here's the c) approach I was referring to:

https://github.com/mdroth/qemu/blob/9a749a2a1ae93ac1b7d6a1216edaf0ca96ff1edb/iohandler.c#L110

It was actually quite a bit more straightforward: we set a flag during dispatch
that tells registration/de-registration that they cannot modify the event list
until the dispatch_complete condition is issued by GSource's dispatch function
unless that thread owns the GMainContext (which we can easily check via
g_main_context_is_owner() due to the requirement that callers of
g_main_context_dispatch must call g_main_context_acquire)

As a result, we can drop the GSource's mutex prior to walking the list of event
callbacks, and don't even need to build up a second list. The special
consideration is use the Q*_FOREACH__SAFE macros while walking, since callbacks
might modify it while we do so.

Anthony wasn't too enthusiastic about it and after talking with him a bit I
decided to look into a lockless approach for fd-based events, but hopefully it
at least provides a reference for a possible approach to the issue if
lockless isn't a viable option for the GSource, or we don't care about
lock contention due to rapid de/re-registration of events/pfds/fds for a
particular GSource.

> 
> > +    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