[v1,1/5] util: introduce gsource event abstraction

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

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)

Small nit, but if the class is EventsGSource, the methods should
use the events_gsource_* prefix. Or we can just call it EventsSource.

> +{
> +    GPollFD *retfd;
> +
> +    retfd = g_slice_alloc(sizeof(GPollFD));
> +    retfd->events = 0;
> +    retfd->fd = fd;
> +    src->pollfds_list = g_list_append(src->pollfds_list, retfd);
> +    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;

I'm not sure how useful this EventsGSource abstraction is if it requires users
to implement a custom prepare() function that accesses EventsGSource fields
directly.

Either we should just make this SlirpGSource until another user comes
along where we can look at pulling out common bits, or we should pass in a
prepare() function that operates on the opaque data instead of the
underlying EventsGSource.

If you take the latter approach, you might consider having
events_source_add_pollfd take a GPollFD* arg instead of an fd, then storing
a pointer to the same GPollFD* in your opaque/Slirp object so you can do things
like set the event masks for all the GPollFDs in the prepare cb prior to
completing the GSource's prepare function (which could then do something generic
like return true if any GPollFDs have a non-zero event mask)

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

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.

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

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.

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.

On Fri, Aug 9, 2013 at 12:29 AM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> 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)
>
> Small nit, but if the class is EventsGSource, the methods should
> use the events_gsource_* prefix. Or we can just call it EventsSource.
>
Will fix.
[...]

>> +
>> +EventsGSource *events_source_new(GPrepare prepare, GSourceFunc dispatch_cb,
>> +    void *opaque)
>> +{
>> +    EventsGSource *src;
>> +    GSourceFuncs *gfuncs = g_new0(GSourceFuncs, 1);
>> +    gfuncs->prepare = prepare;
>
> I'm not sure how useful this EventsGSource abstraction is if it requires users
> to implement a custom prepare() function that accesses EventsGSource fields
> directly.
>
> Either we should just make this SlirpGSource until another user comes
> along where we can look at pulling out common bits, or we should pass in a
> prepare() function that operates on the opaque data instead of the
> underlying EventsGSource.
>
Maybe SlirpGSource, since the prepare of slirp is too complicated.

> If you take the latter approach, you might consider having
> events_source_add_pollfd take a GPollFD* arg instead of an fd, then storing
> a pointer to the same GPollFD* in your opaque/Slirp object so you can do things

The GPollFD* is stored in slirp's socket (each slirp-socket has a sock-fd ).
> like set the event masks for all the GPollFDs in the prepare cb prior to
> completing the GSource's prepare function (which could then do something generic
> like return true if any GPollFDs have a non-zero event mask)
>
What is the aim of "masks for all the GPollFDs"?  We just poll the
GPollFD when so->state ask us to do that. Otherwise they are kept
clean.

Thanks and regards,
Pingfan
>> +    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

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