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

[Michael Roth]

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 <address@hidden>
> > ---
> >  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.