* [Qemu-devel] [RFC PATCH v4 01/15] util: introduce gsource event abstration
2013-04-17 8:39 [Qemu-devel] [RFC PATCH v4 00/15] port network layer onto glib Liu Ping Fan
@ 2013-04-17 8:39 ` Liu Ping Fan
2013-04-18 14:01 ` Stefan Hajnoczi
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 02/15] net: introduce bind_ctx to NetClientInfo Liu Ping Fan
` (13 subsequent siblings)
14 siblings, 1 reply; 35+ messages in thread
From: Liu Ping Fan @ 2013-04-17 8:39 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, Paolo Bonzini, Stefan Hajnoczi, Anthony Liguori,
Jan Kiszka
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
Introduce two structs EventGSource, EventsGSource
EventGSource is used to abstract the event with single backend file.
EventsGSource is used to abstract the event with dynamicly changed
backend file, ex, slirp.
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
util/Makefile.objs | 1 +
util/event_gsource.c | 169 ++++++++++++++++++++++++++++++++++++++++++++++++++
util/event_gsource.h | 54 ++++++++++++++++
3 files changed, 224 insertions(+), 0 deletions(-)
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 495a178..a676d7d 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -8,3 +8,4 @@ util-obj-y += error.o qemu-error.o
util-obj-$(CONFIG_POSIX) += compatfd.o
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 += event_gsource.o
diff --git a/util/event_gsource.c b/util/event_gsource.c
new file mode 100644
index 0000000..b255c47
--- /dev/null
+++ b/util/event_gsource.c
@@ -0,0 +1,169 @@
+/*
+ * 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"
+
+static gboolean prepare(GSource *src, gint *time)
+{
+ EventGSource *nsrc = (EventGSource *)src;
+ int events = 0;
+
+ if (!nsrc->readable && !nsrc->writable) {
+ return false;
+ }
+ if (nsrc->readable && nsrc->readable(nsrc->opaque)) {
+ events |= G_IO_IN;
+ }
+ if ((nsrc->writable) && nsrc->writable(nsrc->opaque)) {
+ events |= G_IO_OUT;
+ }
+ nsrc->gfd.events = events;
+
+ return false;
+}
+
+static gboolean check(GSource *src)
+{
+ EventGSource *nsrc = (EventGSource *)src;
+
+ if (nsrc->gfd.revents & nsrc->gfd.events) {
+ return true;
+ }
+ return false;
+}
+
+static gboolean dispatch(GSource *src, GSourceFunc cb, gpointer data)
+{
+ gboolean ret = false;
+
+ if (cb) {
+ ret = cb(data);
+ }
+ return ret;
+}
+
+static GSourceFuncs net_gsource_funcs = {
+ prepare,
+ check,
+ dispatch,
+ NULL
+};
+
+EventGSource *event_source_new(int fd, GSourceFunc dispatch_cb, void *opaque)
+{
+ EventGSource *nsrc = (EventGSource *)g_source_new(&net_gsource_funcs,
+ sizeof(EventGSource));
+ nsrc->gfd.fd = fd;
+ nsrc->opaque = opaque;
+ g_source_set_callback(&nsrc->source, dispatch_cb, nsrc, NULL);
+ g_source_add_poll(&nsrc->source, &nsrc->gfd);
+
+ return nsrc;
+}
+
+void event_source_release(EventGSource *src)
+{
+ g_source_destroy(&src->source);
+}
+
+GPollFD *events_source_get_gfd(EventsGSource *src, int fd)
+{
+ GPollFD *retfd;
+ unsigned long idx;
+
+ idx = find_first_zero_bit(src->alloc_bmp, src->bmp_sz);
+ if (idx == src->bmp_sz) {
+ //idx = src->bmp_sz;
+ src->bmp_sz += 8;
+ src->alloc_bmp = g_realloc(src->alloc_bmp, src->bmp_sz >> 3);
+ src->pollfds = g_realloc(src->pollfds, src->bmp_sz);
+ }
+ set_bit(idx, src->alloc_bmp);
+
+ retfd = src->pollfds + idx;
+ retfd->events = 0;
+ retfd->fd = fd;
+ if (fd > 0) {
+ g_source_add_poll(&src->source, retfd);
+ }
+
+ return retfd;
+}
+
+void events_source_close_gfd(EventsGSource *src, GPollFD *pollfd)
+{
+ unsigned long idx;
+
+ idx = pollfd - src->pollfds;
+ clear_bit(idx, src->alloc_bmp);
+ g_source_remove_poll(&src->source, pollfd);
+}
+
+gboolean events_source_check(GSource *src)
+{
+ unsigned long idx = 0;
+ EventsGSource *nsrc = (EventsGSource *)src;
+ unsigned long sz = nsrc->bmp_sz;
+ GPollFD *gfd;
+
+ do {
+ idx = find_next_bit(nsrc->alloc_bmp, sz, idx);
+ if (idx < sz) {
+ gfd = nsrc->pollfds + idx;
+ if (gfd->revents & gfd->events) {
+ return true;
+ }
+ idx++;
+ continue;
+ } else {
+ return false;
+ }
+ } while (true);
+}
+
+gboolean events_source_dispatch(GSource *src, GSourceFunc cb, gpointer data)
+{
+ gboolean ret = false;
+
+ if (cb) {
+ ret = cb(data);
+ }
+ return ret;
+}
+
+EventsGSource *events_source_new(GSourceFuncs *funcs, GSourceFunc dispatch_cb, void *opaque)
+{
+ EventsGSource *src = (EventsGSource *)g_source_new(funcs, sizeof(EventsGSource));
+
+ /* 8bits size at initial */
+ src->bmp_sz = 8;
+ src->alloc_bmp = g_malloc0(src->bmp_sz >> 3);
+ src->pollfds = g_malloc0(8 * sizeof(GPollFD));
+ src->opaque = opaque;
+ g_source_set_callback(&src->source, dispatch_cb, src, NULL);
+
+ return src;
+}
+
+void events_source_release(EventsGSource *src)
+{
+ g_free(src->alloc_bmp);
+ g_free(src->pollfds);
+ g_source_destroy(&src->source);
+ g_free(src);
+}
+
diff --git a/util/event_gsource.h b/util/event_gsource.h
new file mode 100644
index 0000000..fd07e6d
--- /dev/null
+++ b/util/event_gsource.h
@@ -0,0 +1,54 @@
+/*
+ * 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 bool (*Pollable)(void *opaque);
+
+/* single fd drive gsource */
+typedef struct EventGSource {
+ GSource source;
+ GPollFD gfd;
+ Pollable readable;
+ Pollable writable;
+ void *opaque;
+} EventGSource;
+
+EventGSource *event_source_new(int fd, GSourceFunc dispatch_cb, void *opaque);
+void event_source_release(EventGSource *src);
+
+/* multi fd drive gsource*/
+typedef struct EventsGSource {
+ GSource source;
+ /* 8 for initial, stand for 8 pollfds */
+ unsigned int bmp_sz;
+ unsigned long *alloc_bmp;
+ GPollFD *pollfds;
+ void *opaque;
+} EventsGSource;
+
+EventsGSource *events_source_new(GSourceFuncs *funcs, GSourceFunc dispatch_cb, void *opaque);
+void events_source_release(EventsGSource *src);
+gboolean events_source_check(GSource *src);
+gboolean events_source_dispatch(GSource *src, GSourceFunc cb, gpointer data);
+GPollFD *events_source_get_gfd(EventsGSource *src, int fd);
+void events_source_close_gfd(EventsGSource *src, GPollFD *pollfd);
+
+
+
+#endif
--
1.7.4.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [RFC PATCH v4 01/15] util: introduce gsource event abstration
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 01/15] util: introduce gsource event abstration Liu Ping Fan
@ 2013-04-18 14:01 ` Stefan Hajnoczi
2013-04-19 6:52 ` liu ping fan
0 siblings, 1 reply; 35+ messages in thread
From: Stefan Hajnoczi @ 2013-04-18 14:01 UTC (permalink / raw)
To: Liu Ping Fan
Cc: Stefan Hajnoczi, Jan Kiszka, qemu-devel, mdroth, Anthony Liguori,
Paolo Bonzini
On Wed, Apr 17, 2013 at 04:39:10PM +0800, Liu Ping Fan wrote:
> +static gboolean prepare(GSource *src, gint *time)
> +{
> + EventGSource *nsrc = (EventGSource *)src;
> + int events = 0;
> +
> + if (!nsrc->readable && !nsrc->writable) {
> + return false;
> + }
> + if (nsrc->readable && nsrc->readable(nsrc->opaque)) {
> + events |= G_IO_IN;
> + }
> + if ((nsrc->writable) && nsrc->writable(nsrc->opaque)) {
> + events |= G_IO_OUT;
> + }
G_IO_ERR, G_IO_HUP, G_IO_PRI?
Here is the select(2) to GCondition mapping:
rfds -> G_IO_IN | G_IO_HUP | G_IO_ERR
wfds -> G_IO_OUT | G_IO_ERR
xfds -> G_IO_PRI
In other words, we're missing events by just using G_IO_IN and G_IO_OUT.
Whether that matters depends on EventGSource users. For sockets it can
matter.
> +void event_source_release(EventGSource *src)
> +{
> + g_source_destroy(&src->source);
Leaks src.
> +}
> +
> +GPollFD *events_source_get_gfd(EventsGSource *src, int fd)
events_source_add_fd() seems like a better name since this function
always allocates a new GPollFD, it never "gets" an existing one.
> +{
> + GPollFD *retfd;
> + unsigned long idx;
> +
> + idx = find_first_zero_bit(src->alloc_bmp, src->bmp_sz);
> + if (idx == src->bmp_sz) {
> + //idx = src->bmp_sz;
Commented out line.
> +void events_source_close_gfd(EventsGSource *src, GPollFD *pollfd)
"close" usually means close(2). I suggest "remove" instead.
> +EventsGSource *events_source_new(GSourceFuncs *funcs, GSourceFunc dispatch_cb, void *opaque)
> +{
> + EventsGSource *src = (EventsGSource *)g_source_new(funcs, sizeof(EventsGSource));
> +
> + /* 8bits size at initial */
> + src->bmp_sz = 8;
> + src->alloc_bmp = g_malloc0(src->bmp_sz >> 3);
This is unportable. alloc_bmp is unsigned long, you are allocating just
one byte!
Please drop the bitmap approach and use a doubly-linked list or another
glib container type of your choice. It needs 3 operations: add, remove,
and iterate.
> +/* multi fd drive gsource*/
> +typedef struct EventsGSource {
> + GSource source;
> + /* 8 for initial, stand for 8 pollfds */
> + unsigned int bmp_sz;
> + unsigned long *alloc_bmp;
> + GPollFD *pollfds;
> + void *opaque;
> +} EventsGSource;
> +
> +EventsGSource *events_source_new(GSourceFuncs *funcs, GSourceFunc dispatch_cb, void *opaque);
> +void events_source_release(EventsGSource *src);
> +gboolean events_source_check(GSource *src);
> +gboolean events_source_dispatch(GSource *src, GSourceFunc cb, gpointer data);
> +GPollFD *events_source_get_gfd(EventsGSource *src, int fd);
> +void events_source_close_gfd(EventsGSource *src, GPollFD *pollfd);
Why are check/dispatch public? Perhaps events_source_new() just needs a
prepare() argument instead of exposing GSourceFuncs.
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [RFC PATCH v4 01/15] util: introduce gsource event abstration
2013-04-18 14:01 ` Stefan Hajnoczi
@ 2013-04-19 6:52 ` liu ping fan
2013-04-19 11:59 ` Stefan Hajnoczi
0 siblings, 1 reply; 35+ messages in thread
From: liu ping fan @ 2013-04-19 6:52 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Stefan Hajnoczi, Jan Kiszka, qemu-devel, mdroth, Anthony Liguori,
Paolo Bonzini
On Thu, Apr 18, 2013 at 10:01 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Wed, Apr 17, 2013 at 04:39:10PM +0800, Liu Ping Fan wrote:
>> +static gboolean prepare(GSource *src, gint *time)
>> +{
>> + EventGSource *nsrc = (EventGSource *)src;
>> + int events = 0;
>> +
>> + if (!nsrc->readable && !nsrc->writable) {
>> + return false;
>> + }
>> + if (nsrc->readable && nsrc->readable(nsrc->opaque)) {
>> + events |= G_IO_IN;
>> + }
>> + if ((nsrc->writable) && nsrc->writable(nsrc->opaque)) {
>> + events |= G_IO_OUT;
>> + }
>
> G_IO_ERR, G_IO_HUP, G_IO_PRI?
>
> Here is the select(2) to GCondition mapping:
> rfds -> G_IO_IN | G_IO_HUP | G_IO_ERR
> wfds -> G_IO_OUT | G_IO_ERR
> xfds -> G_IO_PRI
>
Does G_IO_PRI only happen on read-in direction?
> In other words, we're missing events by just using G_IO_IN and G_IO_OUT.
> Whether that matters depends on EventGSource users. For sockets it can
> matter.
>
I think you mean just prepare all of them, and let the dispatch decide
how to handle them, right?
>> +void event_source_release(EventGSource *src)
>> +{
>> + g_source_destroy(&src->source);
>
> Leaks src.
>
All of the mem used by EventGSource are allocated by g_source_new, so
g_source_destroy can reclaim all of them.
>> +}
>> +
>> +GPollFD *events_source_get_gfd(EventsGSource *src, int fd)
>
> events_source_add_fd() seems like a better name since this function
> always allocates a new GPollFD, it never "gets" an existing one.
>
Thanks for pointing out. But see the reply to "unportable alloc_bmp" below.
>> +{
>> + GPollFD *retfd;
>> + unsigned long idx;
>> +
>> + idx = find_first_zero_bit(src->alloc_bmp, src->bmp_sz);
>> + if (idx == src->bmp_sz) {
>> + //idx = src->bmp_sz;
>
> Commented out line.
>
Apply,
>> +void events_source_close_gfd(EventsGSource *src, GPollFD *pollfd)
>
> "close" usually means close(2). I suggest "remove" instead.
>
>> +EventsGSource *events_source_new(GSourceFuncs *funcs, GSourceFunc dispatch_cb, void *opaque)
>> +{
>> + EventsGSource *src = (EventsGSource *)g_source_new(funcs, sizeof(EventsGSource));
>> +
>> + /* 8bits size at initial */
>> + src->bmp_sz = 8;
>> + src->alloc_bmp = g_malloc0(src->bmp_sz >> 3);
>
> This is unportable. alloc_bmp is unsigned long, you are allocating just
> one byte!
>
I had thought that resorting to bmp_sz to guarantee the bit-ops on
alloc_bmp. And if EventsGSource->pollfds is allocated with 64 instance
at initialize, it cost too much. I can fix it with more fine code
when alloc_bmp's size growing.
> Please drop the bitmap approach and use a doubly-linked list or another
> glib container type of your choice. It needs 3 operations: add, remove,
> and iterate.
>
But as the case for slirp, owning to network's connection and
disconnection, the slirp's sockets can be dynamically changed quickly.
The bitmap approach is something like slab, while glib container
type lacks such support (maybe using two GArray inuse[], free[]).
>> +/* multi fd drive gsource*/
>> +typedef struct EventsGSource {
>> + GSource source;
>> + /* 8 for initial, stand for 8 pollfds */
>> + unsigned int bmp_sz;
>> + unsigned long *alloc_bmp;
>> + GPollFD *pollfds;
>> + void *opaque;
>> +} EventsGSource;
>> +
>> +EventsGSource *events_source_new(GSourceFuncs *funcs, GSourceFunc dispatch_cb, void *opaque);
>> +void events_source_release(EventsGSource *src);
>> +gboolean events_source_check(GSource *src);
>> +gboolean events_source_dispatch(GSource *src, GSourceFunc cb, gpointer data);
>> +GPollFD *events_source_get_gfd(EventsGSource *src, int fd);
>> +void events_source_close_gfd(EventsGSource *src, GPollFD *pollfd);
>
> Why are check/dispatch public? Perhaps events_source_new() just needs a
> prepare() argument instead of exposing GSourceFuncs.
Ok, that is more closed and reasonable.
Thanks, Pingfan
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [RFC PATCH v4 01/15] util: introduce gsource event abstration
2013-04-19 6:52 ` liu ping fan
@ 2013-04-19 11:59 ` Stefan Hajnoczi
2013-04-22 7:50 ` liu ping fan
0 siblings, 1 reply; 35+ messages in thread
From: Stefan Hajnoczi @ 2013-04-19 11:59 UTC (permalink / raw)
To: liu ping fan
Cc: Stefan Hajnoczi, qemu-devel, mdroth, Anthony Liguori, Jan Kiszka,
Paolo Bonzini
On Fri, Apr 19, 2013 at 02:52:08PM +0800, liu ping fan wrote:
> On Thu, Apr 18, 2013 at 10:01 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Wed, Apr 17, 2013 at 04:39:10PM +0800, Liu Ping Fan wrote:
> >> +static gboolean prepare(GSource *src, gint *time)
> >> +{
> >> + EventGSource *nsrc = (EventGSource *)src;
> >> + int events = 0;
> >> +
> >> + if (!nsrc->readable && !nsrc->writable) {
> >> + return false;
> >> + }
> >> + if (nsrc->readable && nsrc->readable(nsrc->opaque)) {
> >> + events |= G_IO_IN;
> >> + }
> >> + if ((nsrc->writable) && nsrc->writable(nsrc->opaque)) {
> >> + events |= G_IO_OUT;
> >> + }
> >
> > G_IO_ERR, G_IO_HUP, G_IO_PRI?
> >
> > Here is the select(2) to GCondition mapping:
> > rfds -> G_IO_IN | G_IO_HUP | G_IO_ERR
> > wfds -> G_IO_OUT | G_IO_ERR
> > xfds -> G_IO_PRI
> >
> Does G_IO_PRI only happen on read-in direction?
Yes.
> > In other words, we're missing events by just using G_IO_IN and G_IO_OUT.
> > Whether that matters depends on EventGSource users. For sockets it can
> > matter.
> >
> I think you mean just prepare all of them, and let the dispatch decide
> how to handle them, right?
The user must decide which events to monitor. Otherwise the event loop
may run at 100% CPU due to events that are monitored but not handled by
the user.
> >> +void event_source_release(EventGSource *src)
> >> +{
> >> + g_source_destroy(&src->source);
> >
> > Leaks src.
> >
> All of the mem used by EventGSource are allocated by g_source_new, so
> g_source_destroy can reclaim all of them.
Okay, then the bug is events_source_release() which calls g_free(src)
after g_source_destroy(&src->source).
> >> +EventsGSource *events_source_new(GSourceFuncs *funcs, GSourceFunc dispatch_cb, void *opaque)
> >> +{
> >> + EventsGSource *src = (EventsGSource *)g_source_new(funcs, sizeof(EventsGSource));
> >> +
> >> + /* 8bits size at initial */
> >> + src->bmp_sz = 8;
> >> + src->alloc_bmp = g_malloc0(src->bmp_sz >> 3);
> >
> > This is unportable. alloc_bmp is unsigned long, you are allocating just
> > one byte!
> >
> I had thought that resorting to bmp_sz to guarantee the bit-ops on
> alloc_bmp. And if EventsGSource->pollfds is allocated with 64 instance
> at initialize, it cost too much. I can fix it with more fine code
> when alloc_bmp's size growing.
>
> > Please drop the bitmap approach and use a doubly-linked list or another
> > glib container type of your choice. It needs 3 operations: add, remove,
> > and iterate.
> >
> But as the case for slirp, owning to network's connection and
> disconnection, the slirp's sockets can be dynamically changed quickly.
> The bitmap approach is something like slab, while glib container
> type lacks such support (maybe using two GArray inuse[], free[]).
Doubly-linked list insertion and removal are O(1).
The linked list can be allocated with g_slice_alloc() which is
efficient.
Iterating linked lists isn't cache-friendly but this is premature
optimization. I bet the userspace TCP - pulling packets apart - is more
of a CPU bottleneck than a doubly-linked list of fds.
Please use existing data structures instead of writing them from scratch
unless there is a real need (e.g. profiling shows it matters).
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [RFC PATCH v4 01/15] util: introduce gsource event abstration
2013-04-19 11:59 ` Stefan Hajnoczi
@ 2013-04-22 7:50 ` liu ping fan
0 siblings, 0 replies; 35+ messages in thread
From: liu ping fan @ 2013-04-22 7:50 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Stefan Hajnoczi, qemu-devel, mdroth, Anthony Liguori, Jan Kiszka,
Paolo Bonzini
On Fri, Apr 19, 2013 at 7:59 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Fri, Apr 19, 2013 at 02:52:08PM +0800, liu ping fan wrote:
>> On Thu, Apr 18, 2013 at 10:01 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> > On Wed, Apr 17, 2013 at 04:39:10PM +0800, Liu Ping Fan wrote:
>> >> +static gboolean prepare(GSource *src, gint *time)
>> >> +{
>> >> + EventGSource *nsrc = (EventGSource *)src;
>> >> + int events = 0;
>> >> +
>> >> + if (!nsrc->readable && !nsrc->writable) {
>> >> + return false;
>> >> + }
>> >> + if (nsrc->readable && nsrc->readable(nsrc->opaque)) {
>> >> + events |= G_IO_IN;
>> >> + }
>> >> + if ((nsrc->writable) && nsrc->writable(nsrc->opaque)) {
>> >> + events |= G_IO_OUT;
>> >> + }
>> >
>> > G_IO_ERR, G_IO_HUP, G_IO_PRI?
>> >
>> > Here is the select(2) to GCondition mapping:
>> > rfds -> G_IO_IN | G_IO_HUP | G_IO_ERR
>> > wfds -> G_IO_OUT | G_IO_ERR
>> > xfds -> G_IO_PRI
>> >
>> Does G_IO_PRI only happen on read-in direction?
>
> Yes.
>
>> > In other words, we're missing events by just using G_IO_IN and G_IO_OUT.
>> > Whether that matters depends on EventGSource users. For sockets it can
>> > matter.
>> >
>> I think you mean just prepare all of them, and let the dispatch decide
>> how to handle them, right?
>
> The user must decide which events to monitor. Otherwise the event loop
> may run at 100% CPU due to events that are monitored but not handled by
> the user.
>
>> >> +void event_source_release(EventGSource *src)
>> >> +{
>> >> + g_source_destroy(&src->source);
>> >
>> > Leaks src.
>> >
>> All of the mem used by EventGSource are allocated by g_source_new, so
>> g_source_destroy can reclaim all of them.
>
> Okay, then the bug is events_source_release() which calls g_free(src)
> after g_source_destroy(&src->source).
>
>> >> +EventsGSource *events_source_new(GSourceFuncs *funcs, GSourceFunc dispatch_cb, void *opaque)
>> >> +{
>> >> + EventsGSource *src = (EventsGSource *)g_source_new(funcs, sizeof(EventsGSource));
>> >> +
>> >> + /* 8bits size at initial */
>> >> + src->bmp_sz = 8;
>> >> + src->alloc_bmp = g_malloc0(src->bmp_sz >> 3);
>> >
>> > This is unportable. alloc_bmp is unsigned long, you are allocating just
>> > one byte!
>> >
>> I had thought that resorting to bmp_sz to guarantee the bit-ops on
>> alloc_bmp. And if EventsGSource->pollfds is allocated with 64 instance
>> at initialize, it cost too much. I can fix it with more fine code
>> when alloc_bmp's size growing.
>>
>> > Please drop the bitmap approach and use a doubly-linked list or another
>> > glib container type of your choice. It needs 3 operations: add, remove,
>> > and iterate.
>> >
>> But as the case for slirp, owning to network's connection and
>> disconnection, the slirp's sockets can be dynamically changed quickly.
>> The bitmap approach is something like slab, while glib container
>> type lacks such support (maybe using two GArray inuse[], free[]).
>
> Doubly-linked list insertion and removal are O(1).
>
> The linked list can be allocated with g_slice_alloc() which is
> efficient.
>
> Iterating linked lists isn't cache-friendly but this is premature
> optimization. I bet the userspace TCP - pulling packets apart - is more
> of a CPU bottleneck than a doubly-linked list of fds.
>
> Please use existing data structures instead of writing them from scratch
> unless there is a real need (e.g. profiling shows it matters).
Ok, thanks for the detail explaining.
Regards,
Pingfan
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [RFC PATCH v4 02/15] net: introduce bind_ctx to NetClientInfo
2013-04-17 8:39 [Qemu-devel] [RFC PATCH v4 00/15] port network layer onto glib Liu Ping Fan
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 01/15] util: introduce gsource event abstration Liu Ping Fan
@ 2013-04-17 8:39 ` Liu Ping Fan
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 03/15] net: port tap onto GSource Liu Ping Fan
` (12 subsequent siblings)
14 siblings, 0 replies; 35+ messages in thread
From: Liu Ping Fan @ 2013-04-17 8:39 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, Paolo Bonzini, Stefan Hajnoczi, Anthony Liguori,
Jan Kiszka
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
Introduce bind_ctx interface for NetClientState. It will help to
bind NetClientState with a GSource. Currently, these GSource attached
with default context, but in future, after resolving all the race
condition in network layer, NetClientStates can run on different
threads
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
include/net/net.h | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/include/net/net.h b/include/net/net.h
index cb049a1..88332d2 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -44,6 +44,7 @@ typedef ssize_t (NetReceiveIOV)(NetClientState *, const struct iovec *, int);
typedef void (NetCleanup) (NetClientState *);
typedef void (LinkStatusChanged)(NetClientState *);
typedef void (NetClientDestructor)(NetClientState *);
+typedef void (NetClientBindCtx)(NetClientState *, GMainContext *);
typedef struct NetClientInfo {
NetClientOptionsKind type;
@@ -55,6 +56,7 @@ typedef struct NetClientInfo {
NetCleanup *cleanup;
LinkStatusChanged *link_status_changed;
NetPoll *poll;
+ NetClientBindCtx *bind_ctx;
} NetClientInfo;
struct NetClientState {
--
1.7.4.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [RFC PATCH v4 03/15] net: port tap onto GSource
2013-04-17 8:39 [Qemu-devel] [RFC PATCH v4 00/15] port network layer onto glib Liu Ping Fan
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 01/15] util: introduce gsource event abstration Liu Ping Fan
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 02/15] net: introduce bind_ctx to NetClientInfo Liu Ping Fan
@ 2013-04-17 8:39 ` Liu Ping Fan
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 04/15] net: resolve race of tap backend and its peer Liu Ping Fan
` (11 subsequent siblings)
14 siblings, 0 replies; 35+ messages in thread
From: Liu Ping Fan @ 2013-04-17 8:39 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, Paolo Bonzini, Stefan Hajnoczi, Anthony Liguori,
Jan Kiszka
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
net/tap.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++---------
1 files changed, 52 insertions(+), 10 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index daab350..35cbb6e 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -62,6 +62,7 @@ typedef struct TAPState {
bool enabled;
VHostNetState *vhost_net;
unsigned host_vnet_hdr_len;
+ EventGSource *nsrc;
} TAPState;
static int launch_script(const char *setup_script, const char *ifname, int fd);
@@ -70,25 +71,48 @@ static int tap_can_send(void *opaque);
static void tap_send(void *opaque);
static void tap_writable(void *opaque);
-static void tap_update_fd_handler(TAPState *s)
+static bool readable(void *opaque)
{
- qemu_set_fd_handler2(s->fd,
- s->read_poll && s->enabled ? tap_can_send : NULL,
- s->read_poll && s->enabled ? tap_send : NULL,
- s->write_poll && s->enabled ? tap_writable : NULL,
- s);
+ TAPState *s = opaque;
+
+ if (s->enabled && s->read_poll &&
+ tap_can_send(s)) {
+ return true;
+ }
+ return false;
+}
+
+static bool writable(void *opaque)
+{
+ TAPState *s = opaque;
+
+ if (s->enabled && s->write_poll) {
+ return true;
+ }
+ return false;
+}
+
+static gboolean tap_handler(gpointer data)
+{
+ EventGSource *nsrc = data;
+
+ if (nsrc->gfd.revents & G_IO_IN) {
+ tap_send(nsrc->opaque);
+ }
+ if (nsrc->gfd.revents & G_IO_OUT) {
+ tap_writable(nsrc->opaque);
+ }
+ return true;
}
static void tap_read_poll(TAPState *s, bool enable)
{
s->read_poll = enable;
- tap_update_fd_handler(s);
}
static void tap_write_poll(TAPState *s, bool enable)
{
s->write_poll = enable;
- tap_update_fd_handler(s);
}
static void tap_writable(void *opaque)
@@ -291,6 +315,7 @@ static void tap_cleanup(NetClientState *nc)
tap_read_poll(s, false);
tap_write_poll(s, false);
+ event_source_release(s->nsrc);
close(s->fd);
s->fd = -1;
}
@@ -298,8 +323,10 @@ static void tap_cleanup(NetClientState *nc)
static void tap_poll(NetClientState *nc, bool enable)
{
TAPState *s = DO_UPCAST(TAPState, nc, nc);
+ /* fixme, when tap backend on another thread, the disable should be sync */
tap_read_poll(s, enable);
tap_write_poll(s, enable);
+
}
int tap_get_fd(NetClientState *nc)
@@ -309,6 +336,13 @@ int tap_get_fd(NetClientState *nc)
return s->fd;
}
+static void tap_bind_ctx(NetClientState *nc, GMainContext *ctx)
+{
+ TAPState *s = DO_UPCAST(TAPState, nc, nc);
+
+ g_source_attach(&s->nsrc->source, ctx);
+}
+
/* fd support */
static NetClientInfo net_tap_info = {
@@ -319,6 +353,7 @@ static NetClientInfo net_tap_info = {
.receive_iov = tap_receive_iov,
.poll = tap_poll,
.cleanup = tap_cleanup,
+ .bind_ctx = tap_bind_ctx,
};
static TAPState *net_tap_fd_init(NetClientState *peer,
@@ -596,6 +631,7 @@ static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
int vnet_hdr, int fd)
{
TAPState *s;
+ EventGSource *nsrc;
s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
if (!s) {
@@ -606,6 +642,12 @@ static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
if (tap_set_sndbuf(s->fd, tap) < 0) {
return -1;
}
+ nsrc = event_source_new(s->fd, tap_handler, s);
+ nsrc->gfd.events = G_IO_IN|G_IO_OUT;
+ nsrc->readable = readable;
+ nsrc->writable = writable;
+ s->nsrc = nsrc;
+ s->nc.info->bind_ctx(&s->nc, NULL);
if (tap->has_fd || tap->has_fds) {
snprintf(s->nc.info_str, sizeof(s->nc.info_str), "fd=%d", fd);
@@ -843,8 +885,8 @@ int tap_enable(NetClientState *nc)
} else {
ret = tap_fd_enable(s->fd);
if (ret == 0) {
+ /*fixme, will be sync to ensure handler not be called */
s->enabled = true;
- tap_update_fd_handler(s);
}
return ret;
}
@@ -861,8 +903,8 @@ int tap_disable(NetClientState *nc)
ret = tap_fd_disable(s->fd);
if (ret == 0) {
qemu_purge_queued_packets(nc);
+ /*fixme, will be sync to ensure handler not be called */
s->enabled = false;
- tap_update_fd_handler(s);
}
return ret;
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [RFC PATCH v4 04/15] net: resolve race of tap backend and its peer
2013-04-17 8:39 [Qemu-devel] [RFC PATCH v4 00/15] port network layer onto glib Liu Ping Fan
` (2 preceding siblings ...)
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 03/15] net: port tap onto GSource Liu Ping Fan
@ 2013-04-17 8:39 ` Liu Ping Fan
2013-04-18 14:11 ` Stefan Hajnoczi
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 05/15] net: port vde onto GSource Liu Ping Fan
` (10 subsequent siblings)
14 siblings, 1 reply; 35+ messages in thread
From: Liu Ping Fan @ 2013-04-17 8:39 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, Paolo Bonzini, Stefan Hajnoczi, Anthony Liguori,
Jan Kiszka
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
When vhost net enabled, we should be sure that the user space
fd handler is not in flight
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
net/tap.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index 35cbb6e..b5629e3 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -41,6 +41,7 @@
#include "qemu/error-report.h"
#include "net/tap.h"
+#include "util/event_gsource.h"
#include "hw/vhost_net.h"
@@ -327,6 +328,10 @@ static void tap_poll(NetClientState *nc, bool enable)
tap_read_poll(s, enable);
tap_write_poll(s, enable);
+ if (!enable) {
+ /* need sync so vhost can take over polling */
+ g_source_remove_poll(&s->nsrc->source, &s->nsrc->gfd);
+ }
}
int tap_get_fd(NetClientState *nc)
--
1.7.4.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [RFC PATCH v4 04/15] net: resolve race of tap backend and its peer
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 04/15] net: resolve race of tap backend and its peer Liu Ping Fan
@ 2013-04-18 14:11 ` Stefan Hajnoczi
2013-04-19 5:43 ` liu ping fan
0 siblings, 1 reply; 35+ messages in thread
From: Stefan Hajnoczi @ 2013-04-18 14:11 UTC (permalink / raw)
To: Liu Ping Fan
Cc: Stefan Hajnoczi, Jan Kiszka, qemu-devel, mdroth, Anthony Liguori,
Paolo Bonzini
On Wed, Apr 17, 2013 at 04:39:13PM +0800, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>
> When vhost net enabled, we should be sure that the user space
> fd handler is not in flight
>
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
> net/tap.c | 5 +++++
> 1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/net/tap.c b/net/tap.c
> index 35cbb6e..b5629e3 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -41,6 +41,7 @@
> #include "qemu/error-report.h"
>
> #include "net/tap.h"
> +#include "util/event_gsource.h"
>
> #include "hw/vhost_net.h"
>
> @@ -327,6 +328,10 @@ static void tap_poll(NetClientState *nc, bool enable)
> tap_read_poll(s, enable);
> tap_write_poll(s, enable);
>
> + if (!enable) {
> + /* need sync so vhost can take over polling */
> + g_source_remove_poll(&s->nsrc->source, &s->nsrc->gfd);
> + }
We must also re-enable it when .poll(nc, true) is called.
Please drop the comments the previous patch added. In fact, this patch
can be squashed.
I think there was another vhost sync comment which you haven't addressed
here. See the previous patch.
Stefan
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [RFC PATCH v4 04/15] net: resolve race of tap backend and its peer
2013-04-18 14:11 ` Stefan Hajnoczi
@ 2013-04-19 5:43 ` liu ping fan
0 siblings, 0 replies; 35+ messages in thread
From: liu ping fan @ 2013-04-19 5:43 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Stefan Hajnoczi, Jan Kiszka, qemu-devel, mdroth, Anthony Liguori,
Paolo Bonzini
On Thu, Apr 18, 2013 at 10:11 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Wed, Apr 17, 2013 at 04:39:13PM +0800, Liu Ping Fan wrote:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> When vhost net enabled, we should be sure that the user space
>> fd handler is not in flight
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>> net/tap.c | 5 +++++
>> 1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/tap.c b/net/tap.c
>> index 35cbb6e..b5629e3 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -41,6 +41,7 @@
>> #include "qemu/error-report.h"
>>
>> #include "net/tap.h"
>> +#include "util/event_gsource.h"
>>
>> #include "hw/vhost_net.h"
>>
>> @@ -327,6 +328,10 @@ static void tap_poll(NetClientState *nc, bool enable)
>> tap_read_poll(s, enable);
>> tap_write_poll(s, enable);
>>
>> + if (!enable) {
>> + /* need sync so vhost can take over polling */
>> + g_source_remove_poll(&s->nsrc->source, &s->nsrc->gfd);
>> + }
>
> We must also re-enable it when .poll(nc, true) is called.
>
Yes, will fix it.
> Please drop the comments the previous patch added. In fact, this patch
> can be squashed.
>
Ok.
> I think there was another vhost sync comment which you haven't addressed
> here. See the previous patch.
>
It is related with multi-queue, and I check the code, it can be async.
I will drop the related comment in previous patch
Thanks, Pingfan
> Stefan
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [RFC PATCH v4 05/15] net: port vde onto GSource
2013-04-17 8:39 [Qemu-devel] [RFC PATCH v4 00/15] port network layer onto glib Liu Ping Fan
` (3 preceding siblings ...)
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 04/15] net: resolve race of tap backend and its peer Liu Ping Fan
@ 2013-04-17 8:39 ` Liu Ping Fan
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 06/15] net: port socket to GSource Liu Ping Fan
` (9 subsequent siblings)
14 siblings, 0 replies; 35+ messages in thread
From: Liu Ping Fan @ 2013-04-17 8:39 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, Paolo Bonzini, Stefan Hajnoczi, Anthony Liguori,
Jan Kiszka
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
net/vde.c | 28 ++++++++++++++++++++++++++--
1 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/net/vde.c b/net/vde.c
index 4dea32d..bb7016b 100644
--- a/net/vde.c
+++ b/net/vde.c
@@ -30,10 +30,12 @@
#include "qemu-common.h"
#include "qemu/option.h"
#include "qemu/main-loop.h"
+#include "util/event_gsource.h"
typedef struct VDEState {
NetClientState nc;
VDECONN *vde;
+ EventGSource *nsrc;
} VDEState;
static void vde_to_qemu(void *opaque)
@@ -60,18 +62,36 @@ static ssize_t vde_receive(NetClientState *nc, const uint8_t *buf, size_t size)
return ret;
}
+static gboolean vde_handler(gpointer data)
+{
+ EventGSource *nsrc = (EventGSource *)data;
+
+ if (nsrc->gfd.revents & G_IO_IN) {
+ vde_to_qemu(nsrc->opaque);
+ }
+ return true;
+}
+
static void vde_cleanup(NetClientState *nc)
{
VDEState *s = DO_UPCAST(VDEState, nc, nc);
- qemu_set_fd_handler(vde_datafd(s->vde), NULL, NULL, NULL);
+ event_source_release(s->nsrc);
vde_close(s->vde);
}
+static void vde_bind_ctx(NetClientState *nc, GMainContext *ctx)
+{
+ VDEState *s = DO_UPCAST(VDEState, nc, nc);
+
+ g_source_attach(&s->nsrc->source, ctx);
+}
+
static NetClientInfo net_vde_info = {
.type = NET_CLIENT_OPTIONS_KIND_VDE,
.size = sizeof(VDEState),
.receive = vde_receive,
.cleanup = vde_cleanup,
+ .bind_ctx = vde_bind_ctx,
};
static int net_vde_init(NetClientState *peer, const char *model,
@@ -83,6 +103,7 @@ static int net_vde_init(NetClientState *peer, const char *model,
VDECONN *vde;
char *init_group = (char *)group;
char *init_sock = (char *)sock;
+ EventGSource *nsrc;
struct vde_open_args args = {
.port = port,
@@ -104,7 +125,10 @@ static int net_vde_init(NetClientState *peer, const char *model,
s->vde = vde;
- qemu_set_fd_handler(vde_datafd(s->vde), vde_to_qemu, NULL, s);
+ nsrc = event_source_new(vde_datafd(vde), vde_handler, s);
+ nsrc->gfd.events = G_IO_IN;
+ s->nsrc = nsrc;
+ nc->info->bind_ctx(nc, NULL);
return 0;
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [RFC PATCH v4 06/15] net: port socket to GSource
2013-04-17 8:39 [Qemu-devel] [RFC PATCH v4 00/15] port network layer onto glib Liu Ping Fan
` (4 preceding siblings ...)
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 05/15] net: port vde onto GSource Liu Ping Fan
@ 2013-04-17 8:39 ` Liu Ping Fan
2013-04-18 14:34 ` Stefan Hajnoczi
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 07/15] net: port tap-win32 onto GSource Liu Ping Fan
` (8 subsequent siblings)
14 siblings, 1 reply; 35+ messages in thread
From: Liu Ping Fan @ 2013-04-17 8:39 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, Paolo Bonzini, Stefan Hajnoczi, Anthony Liguori,
Jan Kiszka
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
Port NetSocketState onto NetClientSource. The only thing specail is that
owning to the socket's state machine changes, we need to change the handler.
We implement that by destroy the old NetClientSource and attach a new one
with NetSocketState.
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
net/socket.c | 158 ++++++++++++++++++++++++++++++++++++++++++++-------------
1 files changed, 122 insertions(+), 36 deletions(-)
diff --git a/net/socket.c b/net/socket.c
index 396dc8c..bdd5dc0 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -31,6 +31,8 @@
#include "qemu/option.h"
#include "qemu/sockets.h"
#include "qemu/iov.h"
+#include "util/event_gsource.h"
+
typedef struct NetSocketState {
NetClientState nc;
@@ -42,13 +44,15 @@ typedef struct NetSocketState {
unsigned int send_index; /* number of bytes sent (only SOCK_STREAM) */
uint8_t buf[4096];
struct sockaddr_in dgram_dst; /* contains inet host and port destination iff connectionless (SOCK_DGRAM) */
- IOHandler *send_fn; /* differs between SOCK_STREAM/SOCK_DGRAM */
bool read_poll; /* waiting to receive data? */
bool write_poll; /* waiting to transmit data? */
+ EventGSource *nsrc;
} NetSocketState;
-static void net_socket_accept(void *opaque);
static void net_socket_writable(void *opaque);
+static gboolean net_socket_listen_handler(gpointer data);
+static gboolean net_socket_establish_handler(gpointer data);
+
/* Only read packets from socket when peer can receive them */
static int net_socket_can_send(void *opaque)
@@ -58,25 +62,14 @@ static int net_socket_can_send(void *opaque)
return qemu_can_send_packet(&s->nc);
}
-static void net_socket_update_fd_handler(NetSocketState *s)
-{
- qemu_set_fd_handler2(s->fd,
- s->read_poll ? net_socket_can_send : NULL,
- s->read_poll ? s->send_fn : NULL,
- s->write_poll ? net_socket_writable : NULL,
- s);
-}
-
static void net_socket_read_poll(NetSocketState *s, bool enable)
{
s->read_poll = enable;
- net_socket_update_fd_handler(s);
}
static void net_socket_write_poll(NetSocketState *s, bool enable)
{
s->write_poll = enable;
- net_socket_update_fd_handler(s);
}
static void net_socket_writable(void *opaque)
@@ -148,6 +141,7 @@ static void net_socket_send(void *opaque)
unsigned l;
uint8_t buf1[4096];
const uint8_t *buf;
+ EventGSource *new_nsrc, *nsrc;
size = qemu_recv(s->fd, buf1, sizeof(buf1), 0);
if (size < 0) {
@@ -160,7 +154,13 @@ static void net_socket_send(void *opaque)
net_socket_read_poll(s, false);
net_socket_write_poll(s, false);
if (s->listen_fd != -1) {
- qemu_set_fd_handler(s->listen_fd, net_socket_accept, NULL, s);
+ nsrc = s->nsrc;
+ new_nsrc = event_source_new(s->listen_fd, net_socket_listen_handler,
+ s);
+ s->nsrc = new_nsrc;
+ new_nsrc->gfd.events = G_IO_IN;
+ g_source_destroy(&nsrc->source);
+ s->nc.info->bind_ctx(&s->nc, NULL);
}
closesocket(s->fd);
@@ -331,6 +331,14 @@ static void net_socket_cleanup(NetClientState *nc)
closesocket(s->listen_fd);
s->listen_fd = -1;
}
+ event_source_release(s->nsrc);
+}
+
+static void net_socket_bind_ctx(NetClientState *nc, GMainContext *ctx)
+{
+ NetSocketState *s = DO_UPCAST(NetSocketState, nc, nc);
+
+ g_source_attach(&s->nsrc->source, ctx);
}
static NetClientInfo net_dgram_socket_info = {
@@ -338,8 +346,22 @@ static NetClientInfo net_dgram_socket_info = {
.size = sizeof(NetSocketState),
.receive = net_socket_receive_dgram,
.cleanup = net_socket_cleanup,
+ .bind_ctx = net_socket_bind_ctx,
};
+static gboolean net_socket_dgram_handler(gpointer data)
+{
+ EventGSource *nsrc = (EventGSource *)data;
+ NetSocketState *s = nsrc->opaque;
+
+ if (nsrc->gfd.revents & G_IO_IN) {
+ net_socket_send_dgram(s);
+ } else {
+ net_socket_writable(s);
+ }
+ return true;
+}
+
static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
const char *model,
const char *name,
@@ -350,6 +372,7 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
socklen_t saddr_len;
NetClientState *nc;
NetSocketState *s;
+ EventGSource *nsrc;
/* fd passed: multicast: "learn" dgram_dst address from bound address and save it
* Because this may be "shared" socket from a "master" process, datagrams would be recv()
@@ -393,7 +416,10 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
s->fd = fd;
s->listen_fd = -1;
- s->send_fn = net_socket_send_dgram;
+ nsrc = event_source_new(fd, net_socket_dgram_handler, s);
+ s->nsrc = nsrc;
+ nsrc->gfd.events = G_IO_IN|G_IO_OUT;
+ nc->info->bind_ctx(nc, NULL);
net_socket_read_poll(s, true);
/* mcast: save bound address as dst */
@@ -408,20 +434,28 @@ err:
return NULL;
}
-static void net_socket_connect(void *opaque)
-{
- NetSocketState *s = opaque;
- s->send_fn = net_socket_send;
- net_socket_read_poll(s, true);
-}
-
static NetClientInfo net_socket_info = {
.type = NET_CLIENT_OPTIONS_KIND_SOCKET,
.size = sizeof(NetSocketState),
.receive = net_socket_receive,
.cleanup = net_socket_cleanup,
+ .bind_ctx = net_socket_bind_ctx,
};
+static gboolean net_socket_connect_handler(gpointer data)
+{
+ EventGSource *new_nsrc, *nsrc = data;
+ NetSocketState *s = nsrc->opaque;
+
+ new_nsrc = event_source_new(s->fd, net_socket_establish_handler, s);
+ s->nsrc = new_nsrc;
+ new_nsrc->gfd.events = G_IO_IN|G_IO_OUT;
+ g_source_destroy(&nsrc->source);
+ s->nc.info->bind_ctx(&s->nc, NULL);
+
+ return true;
+}
+
static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
const char *model,
const char *name,
@@ -429,6 +463,7 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
{
NetClientState *nc;
NetSocketState *s;
+ EventGSource *nsrc;
nc = qemu_new_net_client(&net_socket_info, peer, model, name);
@@ -440,9 +475,16 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
s->listen_fd = -1;
if (is_connected) {
- net_socket_connect(s);
+ nsrc = event_source_new(fd, net_socket_establish_handler, s);
+ s->nsrc = nsrc;
+ nsrc->gfd.events = G_IO_IN|G_IO_OUT;
+ nc->info->bind_ctx(nc, NULL);
} else {
- qemu_set_fd_handler(s->fd, NULL, net_socket_connect, s);
+ nsrc = event_source_new(fd, net_socket_connect_handler, s);
+ s->nsrc = nsrc;
+ nsrc->gfd.events = G_IO_IN;
+ nc->info->bind_ctx(nc, NULL);
+
}
return s;
}
@@ -473,30 +515,69 @@ static NetSocketState *net_socket_fd_init(NetClientState *peer,
return NULL;
}
-static void net_socket_accept(void *opaque)
+static gboolean net_socket_establish_handler(gpointer data)
+{
+ EventGSource *nsrc = (EventGSource *)data;
+ NetSocketState *s = nsrc->opaque;
+
+ if (nsrc->gfd.revents & G_IO_IN) {
+ net_socket_send(s);
+ } else {
+ net_socket_writable(s);
+ }
+ return true;
+}
+
+static bool readable(void *opaque)
{
NetSocketState *s = opaque;
+
+ if (s->read_poll && net_socket_can_send(s)) {
+ return true;
+ }
+ return false;
+}
+
+static bool writable(void *opaque)
+{
+ NetSocketState *s = opaque;
+
+ if (s->write_poll) {
+ return true;
+ }
+ return false;
+}
+
+static gboolean net_socket_listen_handler(gpointer data)
+{
+ EventGSource *new_nsrc, *nsrc = data;
+ NetSocketState *s = nsrc->opaque;
struct sockaddr_in saddr;
socklen_t len;
int fd;
- for(;;) {
- len = sizeof(saddr);
- fd = qemu_accept(s->listen_fd, (struct sockaddr *)&saddr, &len);
- if (fd < 0 && errno != EINTR) {
- return;
- } else if (fd >= 0) {
- qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL);
- break;
- }
+ len = sizeof(saddr);
+ fd = qemu_accept(s->listen_fd, (struct sockaddr *)&saddr, &len);
+ if (fd < 0 && errno != EINTR) {
+ return false;
}
s->fd = fd;
s->nc.link_down = false;
- net_socket_connect(s);
+ new_nsrc = event_source_new(fd, net_socket_establish_handler, s);
+ s->nsrc = new_nsrc;
+ new_nsrc->gfd.events = G_IO_IN|G_IO_OUT;
+ new_nsrc->readable = readable;
+ new_nsrc->writable = writable;
+ /* prevent more than one connect req */
+ g_source_destroy(&nsrc->source);
+ s->nc.info->bind_ctx(&s->nc, NULL);
+ net_socket_read_poll(s, true);
snprintf(s->nc.info_str, sizeof(s->nc.info_str),
"socket: connection from %s:%d",
inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));
+
+ return true;
}
static int net_socket_listen_init(NetClientState *peer,
@@ -508,6 +589,7 @@ static int net_socket_listen_init(NetClientState *peer,
NetSocketState *s;
struct sockaddr_in saddr;
int fd, val, ret;
+ EventGSource *nsrc;
if (parse_host_port(&saddr, host_str) < 0)
return -1;
@@ -542,7 +624,11 @@ static int net_socket_listen_init(NetClientState *peer,
s->listen_fd = fd;
s->nc.link_down = true;
- qemu_set_fd_handler(s->listen_fd, net_socket_accept, NULL, s);
+ nsrc = event_source_new(fd, net_socket_listen_handler, s);
+ s->nsrc = nsrc;
+ nsrc->gfd.events = G_IO_IN;
+ nc->info->bind_ctx(nc, NULL);
+
return 0;
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [RFC PATCH v4 06/15] net: port socket to GSource
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 06/15] net: port socket to GSource Liu Ping Fan
@ 2013-04-18 14:34 ` Stefan Hajnoczi
2013-04-19 5:58 ` liu ping fan
0 siblings, 1 reply; 35+ messages in thread
From: Stefan Hajnoczi @ 2013-04-18 14:34 UTC (permalink / raw)
To: Liu Ping Fan
Cc: Stefan Hajnoczi, Jan Kiszka, qemu-devel, mdroth, Anthony Liguori,
Paolo Bonzini
On Wed, Apr 17, 2013 at 04:39:15PM +0800, Liu Ping Fan wrote:
> @@ -160,7 +154,13 @@ static void net_socket_send(void *opaque)
> net_socket_read_poll(s, false);
> net_socket_write_poll(s, false);
> if (s->listen_fd != -1) {
> - qemu_set_fd_handler(s->listen_fd, net_socket_accept, NULL, s);
> + nsrc = s->nsrc;
> + new_nsrc = event_source_new(s->listen_fd, net_socket_listen_handler,
> + s);
> + s->nsrc = new_nsrc;
> + new_nsrc->gfd.events = G_IO_IN;
> + g_source_destroy(&nsrc->source);
> + s->nc.info->bind_ctx(&s->nc, NULL);
The following is equivalent:
event_source_release(s->nsrc);
s->nsrc = event_source_new(s->listen_fd, net_socket_listen_handler, s);
s->nc.info->bind_ctx(&s->nc, NULL);
Then new_nsrc/nsrc can be dropped and the nsrc memory leak is avoided.
Note that gfd.events = G_IO_IN does not get used since prepare()
overwrites gfd.events. Please drop and make sure read_poll == true.
I'm a little worried that we're lacking G_IO_HUP | G_IO_ERR. Perhaps
disconnect and network errors will be ignored.
> }
> closesocket(s->fd);
>
> @@ -331,6 +331,14 @@ static void net_socket_cleanup(NetClientState *nc)
> closesocket(s->listen_fd);
> s->listen_fd = -1;
> }
> + event_source_release(s->nsrc);
> +}
> +
> +static void net_socket_bind_ctx(NetClientState *nc, GMainContext *ctx)
> +{
> + NetSocketState *s = DO_UPCAST(NetSocketState, nc, nc);
> +
> + g_source_attach(&s->nsrc->source, ctx);
> }
>
> static NetClientInfo net_dgram_socket_info = {
> @@ -338,8 +346,22 @@ static NetClientInfo net_dgram_socket_info = {
> .size = sizeof(NetSocketState),
> .receive = net_socket_receive_dgram,
> .cleanup = net_socket_cleanup,
> + .bind_ctx = net_socket_bind_ctx,
> };
>
> +static gboolean net_socket_dgram_handler(gpointer data)
> +{
> + EventGSource *nsrc = (EventGSource *)data;
> + NetSocketState *s = nsrc->opaque;
> +
> + if (nsrc->gfd.revents & G_IO_IN) {
> + net_socket_send_dgram(s);
> + } else {
> + net_socket_writable(s);
> + }
> + return true;
> +}
> +
> static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
> const char *model,
> const char *name,
> @@ -350,6 +372,7 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
> socklen_t saddr_len;
> NetClientState *nc;
> NetSocketState *s;
> + EventGSource *nsrc;
>
> /* fd passed: multicast: "learn" dgram_dst address from bound address and save it
> * Because this may be "shared" socket from a "master" process, datagrams would be recv()
> @@ -393,7 +416,10 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
>
> s->fd = fd;
> s->listen_fd = -1;
> - s->send_fn = net_socket_send_dgram;
> + nsrc = event_source_new(fd, net_socket_dgram_handler, s);
> + s->nsrc = nsrc;
> + nsrc->gfd.events = G_IO_IN|G_IO_OUT;
Please drop.
> + nc->info->bind_ctx(nc, NULL);
> net_socket_read_poll(s, true);
>
> /* mcast: save bound address as dst */
> @@ -408,20 +434,28 @@ err:
> return NULL;
> }
>
> -static void net_socket_connect(void *opaque)
> -{
> - NetSocketState *s = opaque;
> - s->send_fn = net_socket_send;
> - net_socket_read_poll(s, true);
> -}
> -
> static NetClientInfo net_socket_info = {
> .type = NET_CLIENT_OPTIONS_KIND_SOCKET,
> .size = sizeof(NetSocketState),
> .receive = net_socket_receive,
> .cleanup = net_socket_cleanup,
> + .bind_ctx = net_socket_bind_ctx,
> };
>
> +static gboolean net_socket_connect_handler(gpointer data)
> +{
> + EventGSource *new_nsrc, *nsrc = data;
> + NetSocketState *s = nsrc->opaque;
> +
> + new_nsrc = event_source_new(s->fd, net_socket_establish_handler, s);
> + s->nsrc = new_nsrc;
> + new_nsrc->gfd.events = G_IO_IN|G_IO_OUT;
Please drop.
> + g_source_destroy(&nsrc->source);
> + s->nc.info->bind_ctx(&s->nc, NULL);
> +
> + return true;
> +}
> +
> static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
> const char *model,
> const char *name,
> @@ -429,6 +463,7 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
> {
> NetClientState *nc;
> NetSocketState *s;
> + EventGSource *nsrc;
>
> nc = qemu_new_net_client(&net_socket_info, peer, model, name);
>
> @@ -440,9 +475,16 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
> s->listen_fd = -1;
>
> if (is_connected) {
> - net_socket_connect(s);
> + nsrc = event_source_new(fd, net_socket_establish_handler, s);
> + s->nsrc = nsrc;
> + nsrc->gfd.events = G_IO_IN|G_IO_OUT;
Please drop.
> + nc->info->bind_ctx(nc, NULL);
> } else {
> - qemu_set_fd_handler(s->fd, NULL, net_socket_connect, s);
> + nsrc = event_source_new(fd, net_socket_connect_handler, s);
> + s->nsrc = nsrc;
> + nsrc->gfd.events = G_IO_IN;
Please drop.
> + nc->info->bind_ctx(nc, NULL);
> +
> }
> return s;
> }
> @@ -473,30 +515,69 @@ static NetSocketState *net_socket_fd_init(NetClientState *peer,
> return NULL;
> }
>
> -static void net_socket_accept(void *opaque)
> +static gboolean net_socket_establish_handler(gpointer data)
> +{
> + EventGSource *nsrc = (EventGSource *)data;
> + NetSocketState *s = nsrc->opaque;
> +
> + if (nsrc->gfd.revents & G_IO_IN) {
> + net_socket_send(s);
> + } else {
> + net_socket_writable(s);
> + }
> + return true;
> +}
> +
> +static bool readable(void *opaque)
> {
> NetSocketState *s = opaque;
> +
> + if (s->read_poll && net_socket_can_send(s)) {
> + return true;
> + }
> + return false;
> +}
> +
> +static bool writable(void *opaque)
> +{
> + NetSocketState *s = opaque;
> +
> + if (s->write_poll) {
> + return true;
> + }
> + return false;
> +}
> +
> +static gboolean net_socket_listen_handler(gpointer data)
> +{
> + EventGSource *new_nsrc, *nsrc = data;
> + NetSocketState *s = nsrc->opaque;
> struct sockaddr_in saddr;
> socklen_t len;
> int fd;
>
> - for(;;) {
> - len = sizeof(saddr);
> - fd = qemu_accept(s->listen_fd, (struct sockaddr *)&saddr, &len);
> - if (fd < 0 && errno != EINTR) {
> - return;
> - } else if (fd >= 0) {
> - qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL);
> - break;
> - }
> + len = sizeof(saddr);
> + fd = qemu_accept(s->listen_fd, (struct sockaddr *)&saddr, &len);
> + if (fd < 0 && errno != EINTR) {
> + return false;
> }
>
> s->fd = fd;
> s->nc.link_down = false;
> - net_socket_connect(s);
> + new_nsrc = event_source_new(fd, net_socket_establish_handler, s);
> + s->nsrc = new_nsrc;
> + new_nsrc->gfd.events = G_IO_IN|G_IO_OUT;
Please drop.
> + new_nsrc->readable = readable;
> + new_nsrc->writable = writable;
> + /* prevent more than one connect req */
> + g_source_destroy(&nsrc->source);
> + s->nc.info->bind_ctx(&s->nc, NULL);
> + net_socket_read_poll(s, true);
> snprintf(s->nc.info_str, sizeof(s->nc.info_str),
> "socket: connection from %s:%d",
> inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));
> +
> + return true;
> }
>
> static int net_socket_listen_init(NetClientState *peer,
> @@ -508,6 +589,7 @@ static int net_socket_listen_init(NetClientState *peer,
> NetSocketState *s;
> struct sockaddr_in saddr;
> int fd, val, ret;
> + EventGSource *nsrc;
>
> if (parse_host_port(&saddr, host_str) < 0)
> return -1;
> @@ -542,7 +624,11 @@ static int net_socket_listen_init(NetClientState *peer,
> s->listen_fd = fd;
> s->nc.link_down = true;
>
> - qemu_set_fd_handler(s->listen_fd, net_socket_accept, NULL, s);
> + nsrc = event_source_new(fd, net_socket_listen_handler, s);
> + s->nsrc = nsrc;
> + nsrc->gfd.events = G_IO_IN;
Please drop.
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [RFC PATCH v4 06/15] net: port socket to GSource
2013-04-18 14:34 ` Stefan Hajnoczi
@ 2013-04-19 5:58 ` liu ping fan
2013-04-19 12:03 ` Stefan Hajnoczi
0 siblings, 1 reply; 35+ messages in thread
From: liu ping fan @ 2013-04-19 5:58 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Stefan Hajnoczi, Jan Kiszka, qemu-devel, mdroth, Anthony Liguori,
Paolo Bonzini
On Thu, Apr 18, 2013 at 10:34 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Wed, Apr 17, 2013 at 04:39:15PM +0800, Liu Ping Fan wrote:
>> @@ -160,7 +154,13 @@ static void net_socket_send(void *opaque)
>> net_socket_read_poll(s, false);
>> net_socket_write_poll(s, false);
>> if (s->listen_fd != -1) {
>> - qemu_set_fd_handler(s->listen_fd, net_socket_accept, NULL, s);
>> + nsrc = s->nsrc;
>> + new_nsrc = event_source_new(s->listen_fd, net_socket_listen_handler,
>> + s);
>> + s->nsrc = new_nsrc;
>> + new_nsrc->gfd.events = G_IO_IN;
>> + g_source_destroy(&nsrc->source);
>> + s->nc.info->bind_ctx(&s->nc, NULL);
>
> The following is equivalent:
>
> event_source_release(s->nsrc);
> s->nsrc = event_source_new(s->listen_fd, net_socket_listen_handler, s);
> s->nc.info->bind_ctx(&s->nc, NULL);
>
> Then new_nsrc/nsrc can be dropped and the nsrc memory leak is avoided.
>
Apply here and the following same issues.
> Note that gfd.events = G_IO_IN does not get used since prepare()
> overwrites gfd.events. Please drop and make sure read_poll == true.
>
Apply,
> I'm a little worried that we're lacking G_IO_HUP | G_IO_ERR. Perhaps
> disconnect and network errors will be ignored.
>
NetSocketState can do limited things about these situation, perhaps,
implement net_socket_can_receive() to export such message to frontend
?
Pingfan
>> }
>> closesocket(s->fd);
>>
>> @@ -331,6 +331,14 @@ static void net_socket_cleanup(NetClientState *nc)
>> closesocket(s->listen_fd);
>> s->listen_fd = -1;
>> }
>> + event_source_release(s->nsrc);
>> +}
>> +
>> +static void net_socket_bind_ctx(NetClientState *nc, GMainContext *ctx)
>> +{
>> + NetSocketState *s = DO_UPCAST(NetSocketState, nc, nc);
>> +
>> + g_source_attach(&s->nsrc->source, ctx);
>> }
>>
>> static NetClientInfo net_dgram_socket_info = {
>> @@ -338,8 +346,22 @@ static NetClientInfo net_dgram_socket_info = {
>> .size = sizeof(NetSocketState),
>> .receive = net_socket_receive_dgram,
>> .cleanup = net_socket_cleanup,
>> + .bind_ctx = net_socket_bind_ctx,
>> };
>>
>> +static gboolean net_socket_dgram_handler(gpointer data)
>> +{
>> + EventGSource *nsrc = (EventGSource *)data;
>> + NetSocketState *s = nsrc->opaque;
>> +
>> + if (nsrc->gfd.revents & G_IO_IN) {
>> + net_socket_send_dgram(s);
>> + } else {
>> + net_socket_writable(s);
>> + }
>> + return true;
>> +}
>> +
>> static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
>> const char *model,
>> const char *name,
>> @@ -350,6 +372,7 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
>> socklen_t saddr_len;
>> NetClientState *nc;
>> NetSocketState *s;
>> + EventGSource *nsrc;
>>
>> /* fd passed: multicast: "learn" dgram_dst address from bound address and save it
>> * Because this may be "shared" socket from a "master" process, datagrams would be recv()
>> @@ -393,7 +416,10 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
>>
>> s->fd = fd;
>> s->listen_fd = -1;
>> - s->send_fn = net_socket_send_dgram;
>> + nsrc = event_source_new(fd, net_socket_dgram_handler, s);
>> + s->nsrc = nsrc;
>> + nsrc->gfd.events = G_IO_IN|G_IO_OUT;
>
> Please drop.
>
>> + nc->info->bind_ctx(nc, NULL);
>> net_socket_read_poll(s, true);
>>
>> /* mcast: save bound address as dst */
>> @@ -408,20 +434,28 @@ err:
>> return NULL;
>> }
>>
>> -static void net_socket_connect(void *opaque)
>> -{
>> - NetSocketState *s = opaque;
>> - s->send_fn = net_socket_send;
>> - net_socket_read_poll(s, true);
>> -}
>> -
>> static NetClientInfo net_socket_info = {
>> .type = NET_CLIENT_OPTIONS_KIND_SOCKET,
>> .size = sizeof(NetSocketState),
>> .receive = net_socket_receive,
>> .cleanup = net_socket_cleanup,
>> + .bind_ctx = net_socket_bind_ctx,
>> };
>>
>> +static gboolean net_socket_connect_handler(gpointer data)
>> +{
>> + EventGSource *new_nsrc, *nsrc = data;
>> + NetSocketState *s = nsrc->opaque;
>> +
>> + new_nsrc = event_source_new(s->fd, net_socket_establish_handler, s);
>> + s->nsrc = new_nsrc;
>> + new_nsrc->gfd.events = G_IO_IN|G_IO_OUT;
>
> Please drop.
>
>> + g_source_destroy(&nsrc->source);
>> + s->nc.info->bind_ctx(&s->nc, NULL);
>> +
>> + return true;
>> +}
>> +
>> static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
>> const char *model,
>> const char *name,
>> @@ -429,6 +463,7 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
>> {
>> NetClientState *nc;
>> NetSocketState *s;
>> + EventGSource *nsrc;
>>
>> nc = qemu_new_net_client(&net_socket_info, peer, model, name);
>>
>> @@ -440,9 +475,16 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
>> s->listen_fd = -1;
>>
>> if (is_connected) {
>> - net_socket_connect(s);
>> + nsrc = event_source_new(fd, net_socket_establish_handler, s);
>> + s->nsrc = nsrc;
>> + nsrc->gfd.events = G_IO_IN|G_IO_OUT;
>
> Please drop.
>
>> + nc->info->bind_ctx(nc, NULL);
>> } else {
>> - qemu_set_fd_handler(s->fd, NULL, net_socket_connect, s);
>> + nsrc = event_source_new(fd, net_socket_connect_handler, s);
>> + s->nsrc = nsrc;
>> + nsrc->gfd.events = G_IO_IN;
>
> Please drop.
>
>> + nc->info->bind_ctx(nc, NULL);
>> +
>> }
>> return s;
>> }
>> @@ -473,30 +515,69 @@ static NetSocketState *net_socket_fd_init(NetClientState *peer,
>> return NULL;
>> }
>>
>> -static void net_socket_accept(void *opaque)
>> +static gboolean net_socket_establish_handler(gpointer data)
>> +{
>> + EventGSource *nsrc = (EventGSource *)data;
>> + NetSocketState *s = nsrc->opaque;
>> +
>> + if (nsrc->gfd.revents & G_IO_IN) {
>> + net_socket_send(s);
>> + } else {
>> + net_socket_writable(s);
>> + }
>> + return true;
>> +}
>> +
>> +static bool readable(void *opaque)
>> {
>> NetSocketState *s = opaque;
>> +
>> + if (s->read_poll && net_socket_can_send(s)) {
>> + return true;
>> + }
>> + return false;
>> +}
>> +
>> +static bool writable(void *opaque)
>> +{
>> + NetSocketState *s = opaque;
>> +
>> + if (s->write_poll) {
>> + return true;
>> + }
>> + return false;
>> +}
>> +
>> +static gboolean net_socket_listen_handler(gpointer data)
>> +{
>> + EventGSource *new_nsrc, *nsrc = data;
>> + NetSocketState *s = nsrc->opaque;
>> struct sockaddr_in saddr;
>> socklen_t len;
>> int fd;
>>
>> - for(;;) {
>> - len = sizeof(saddr);
>> - fd = qemu_accept(s->listen_fd, (struct sockaddr *)&saddr, &len);
>> - if (fd < 0 && errno != EINTR) {
>> - return;
>> - } else if (fd >= 0) {
>> - qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL);
>> - break;
>> - }
>> + len = sizeof(saddr);
>> + fd = qemu_accept(s->listen_fd, (struct sockaddr *)&saddr, &len);
>> + if (fd < 0 && errno != EINTR) {
>> + return false;
>> }
>>
>> s->fd = fd;
>> s->nc.link_down = false;
>> - net_socket_connect(s);
>> + new_nsrc = event_source_new(fd, net_socket_establish_handler, s);
>> + s->nsrc = new_nsrc;
>> + new_nsrc->gfd.events = G_IO_IN|G_IO_OUT;
>
> Please drop.
>
>> + new_nsrc->readable = readable;
>> + new_nsrc->writable = writable;
>> + /* prevent more than one connect req */
>> + g_source_destroy(&nsrc->source);
>> + s->nc.info->bind_ctx(&s->nc, NULL);
>> + net_socket_read_poll(s, true);
>> snprintf(s->nc.info_str, sizeof(s->nc.info_str),
>> "socket: connection from %s:%d",
>> inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));
>> +
>> + return true;
>> }
>>
>> static int net_socket_listen_init(NetClientState *peer,
>> @@ -508,6 +589,7 @@ static int net_socket_listen_init(NetClientState *peer,
>> NetSocketState *s;
>> struct sockaddr_in saddr;
>> int fd, val, ret;
>> + EventGSource *nsrc;
>>
>> if (parse_host_port(&saddr, host_str) < 0)
>> return -1;
>> @@ -542,7 +624,11 @@ static int net_socket_listen_init(NetClientState *peer,
>> s->listen_fd = fd;
>> s->nc.link_down = true;
>>
>> - qemu_set_fd_handler(s->listen_fd, net_socket_accept, NULL, s);
>> + nsrc = event_source_new(fd, net_socket_listen_handler, s);
>> + s->nsrc = nsrc;
>> + nsrc->gfd.events = G_IO_IN;
>
> Please drop.
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [RFC PATCH v4 06/15] net: port socket to GSource
2013-04-19 5:58 ` liu ping fan
@ 2013-04-19 12:03 ` Stefan Hajnoczi
2013-04-22 7:52 ` liu ping fan
0 siblings, 1 reply; 35+ messages in thread
From: Stefan Hajnoczi @ 2013-04-19 12:03 UTC (permalink / raw)
To: liu ping fan
Cc: Stefan Hajnoczi, qemu-devel, mdroth, Anthony Liguori, Jan Kiszka,
Paolo Bonzini
On Fri, Apr 19, 2013 at 01:58:40PM +0800, liu ping fan wrote:
> On Thu, Apr 18, 2013 at 10:34 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Wed, Apr 17, 2013 at 04:39:15PM +0800, Liu Ping Fan wrote:
> > I'm a little worried that we're lacking G_IO_HUP | G_IO_ERR. Perhaps
> > disconnect and network errors will be ignored.
> >
> NetSocketState can do limited things about these situation, perhaps,
> implement net_socket_can_receive() to export such message to frontend
> ?
net/socket.c uses G_IO_HUP | G_IO_ERR today, see
iohandler.c:qemu_iohandler_fill().
This patch *stops* using them. My question is: why stop and is it
correct?
Stefan
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v4 06/15] net: port socket to GSource
2013-04-19 12:03 ` Stefan Hajnoczi
@ 2013-04-22 7:52 ` liu ping fan
0 siblings, 0 replies; 35+ messages in thread
From: liu ping fan @ 2013-04-22 7:52 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Stefan Hajnoczi, qemu-devel, mdroth, Anthony Liguori, Jan Kiszka,
Paolo Bonzini
On Fri, Apr 19, 2013 at 8:03 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Fri, Apr 19, 2013 at 01:58:40PM +0800, liu ping fan wrote:
>> On Thu, Apr 18, 2013 at 10:34 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> > On Wed, Apr 17, 2013 at 04:39:15PM +0800, Liu Ping Fan wrote:
>> > I'm a little worried that we're lacking G_IO_HUP | G_IO_ERR. Perhaps
>> > disconnect and network errors will be ignored.
>> >
>> NetSocketState can do limited things about these situation, perhaps,
>> implement net_socket_can_receive() to export such message to frontend
>> ?
>
> net/socket.c uses G_IO_HUP | G_IO_ERR today, see
> iohandler.c:qemu_iohandler_fill().
>
> This patch *stops* using them. My question is: why stop and is it
> correct?
>
No, it is not. It leaves dead file descriptors unhandled. Will fix
up in next version
> Stefan
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [RFC PATCH v4 07/15] net: port tap-win32 onto GSource
2013-04-17 8:39 [Qemu-devel] [RFC PATCH v4 00/15] port network layer onto glib Liu Ping Fan
` (5 preceding siblings ...)
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 06/15] net: port socket to GSource Liu Ping Fan
@ 2013-04-17 8:39 ` Liu Ping Fan
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 08/15] net: hub use lock to protect ports list Liu Ping Fan
` (7 subsequent siblings)
14 siblings, 0 replies; 35+ messages in thread
From: Liu Ping Fan @ 2013-04-17 8:39 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, Paolo Bonzini, Stefan Hajnoczi, Anthony Liguori,
Jan Kiszka
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
net/tap-win32.c | 28 ++++++++++++++++++++++++++--
1 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/net/tap-win32.c b/net/tap-win32.c
index 91e9e84..e66edf1 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -635,13 +635,14 @@ static int tap_win32_open(tap_win32_overlapped_t **phandle,
typedef struct TAPState {
NetClientState nc;
tap_win32_overlapped_t *handle;
+ EventGSource *nsrc;
} TAPState;
static void tap_cleanup(NetClientState *nc)
{
TAPState *s = DO_UPCAST(TAPState, nc, nc);
- qemu_del_wait_object(s->handle->tap_semaphore, NULL, NULL);
+ event_source_release(s->nsrc);
/* FIXME: need to kill thread and close file handle:
tap_win32_close(s);
@@ -669,19 +670,39 @@ static void tap_win32_send(void *opaque)
}
}
+static void tap_bind_ctx(NetClientState *nc, GMainContext *ctx)
+{
+ TAPState *s = DO_UPCAST(TAPState, nc, nc);
+
+ g_source_attach(&s->nsrc->source, ctx);
+}
+
static NetClientInfo net_tap_win32_info = {
.type = NET_CLIENT_OPTIONS_KIND_TAP,
.size = sizeof(TAPState),
.receive = tap_receive,
.cleanup = tap_cleanup,
+ .bind_ctx = tap_bind_ctx,
};
+static gboolean tap_win32_handler(gpointer data)
+{
+ EventGSource *nsrc = data;
+ TAPState *s = nsrc->opaque;
+
+ if (nsrc->gfd.revents & G_IO_IN) {
+ tap_win32_send(s);
+ }
+ return true;
+}
+
static int tap_win32_init(NetClientState *peer, const char *model,
const char *name, const char *ifname)
{
NetClientState *nc;
TAPState *s;
tap_win32_overlapped_t *handle;
+ EventGSource *nsrc;
if (tap_win32_open(&handle, ifname) < 0) {
printf("tap: Could not open '%s'\n", ifname);
@@ -697,7 +718,10 @@ static int tap_win32_init(NetClientState *peer, const char *model,
s->handle = handle;
- qemu_add_wait_object(s->handle->tap_semaphore, tap_win32_send, s);
+ nsrc = event_source_new(s->handle->tap_semaphore, tap_win32_handler, s);
+ nsrc->gfd.events = G_IO_IN;
+ s->nsrc = nsrc;
+ nc->info->bind_ctx(&s->nc, NULL);
return 0;
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [RFC PATCH v4 08/15] net: hub use lock to protect ports list
2013-04-17 8:39 [Qemu-devel] [RFC PATCH v4 00/15] port network layer onto glib Liu Ping Fan
` (6 preceding siblings ...)
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 07/15] net: port tap-win32 onto GSource Liu Ping Fan
@ 2013-04-17 8:39 ` Liu Ping Fan
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 09/15] net: introduce lock to protect NetQueue Liu Ping Fan
` (6 subsequent siblings)
14 siblings, 0 replies; 35+ messages in thread
From: Liu Ping Fan @ 2013-04-17 8:39 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, Paolo Bonzini, Stefan Hajnoczi, Anthony Liguori,
Jan Kiszka
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
Hub ports will run on multi-threads, so use lock to protect them.
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
net/hub.c | 25 ++++++++++++++++++++++++-
1 files changed, 24 insertions(+), 1 deletions(-)
diff --git a/net/hub.c b/net/hub.c
index df32074..812a6dc 100644
--- a/net/hub.c
+++ b/net/hub.c
@@ -37,6 +37,7 @@ struct NetHub {
int id;
QLIST_ENTRY(NetHub) next;
int num_ports;
+ QemuMutex ports_lock;
QLIST_HEAD(, NetHubPort) ports;
};
@@ -47,6 +48,7 @@ static ssize_t net_hub_receive(NetHub *hub, NetHubPort *source_port,
{
NetHubPort *port;
+ qemu_mutex_lock(&hub->ports_lock);
QLIST_FOREACH(port, &hub->ports, next) {
if (port == source_port) {
continue;
@@ -54,6 +56,7 @@ static ssize_t net_hub_receive(NetHub *hub, NetHubPort *source_port,
qemu_send_packet(&port->nc, buf, len);
}
+ qemu_mutex_unlock(&hub->ports_lock);
return len;
}
@@ -63,6 +66,7 @@ static ssize_t net_hub_receive_iov(NetHub *hub, NetHubPort *source_port,
NetHubPort *port;
ssize_t len = iov_size(iov, iovcnt);
+ qemu_mutex_lock(&hub->ports_lock);
QLIST_FOREACH(port, &hub->ports, next) {
if (port == source_port) {
continue;
@@ -70,6 +74,7 @@ static ssize_t net_hub_receive_iov(NetHub *hub, NetHubPort *source_port,
qemu_sendv_packet(&port->nc, iov, iovcnt);
}
+ qemu_mutex_unlock(&hub->ports_lock);
return len;
}
@@ -80,6 +85,7 @@ static NetHub *net_hub_new(int id)
hub = g_malloc(sizeof(*hub));
hub->id = id;
hub->num_ports = 0;
+ qemu_mutex_init(&hub->ports_lock);
QLIST_INIT(&hub->ports);
QLIST_INSERT_HEAD(&hubs, hub, next);
@@ -93,16 +99,19 @@ static int net_hub_port_can_receive(NetClientState *nc)
NetHubPort *src_port = DO_UPCAST(NetHubPort, nc, nc);
NetHub *hub = src_port->hub;
+ qemu_mutex_lock(&hub->ports_lock);
QLIST_FOREACH(port, &hub->ports, next) {
if (port == src_port) {
continue;
}
if (qemu_can_send_packet(&port->nc)) {
+ qemu_mutex_unlock(&hub->ports_lock);
return 1;
}
}
+ qemu_mutex_unlock(&hub->ports_lock);
return 0;
}
@@ -155,8 +164,9 @@ static NetHubPort *net_hub_port_new(NetHub *hub, const char *name)
port = DO_UPCAST(NetHubPort, nc, nc);
port->id = id;
port->hub = hub;
-
+ qemu_mutex_lock(&hub->ports_lock);
QLIST_INSERT_HEAD(&hub->ports, port, next);
+ qemu_mutex_unlock(&hub->ports_lock);
return port;
}
@@ -197,13 +207,16 @@ NetClientState *net_hub_find_client_by_name(int hub_id, const char *name)
QLIST_FOREACH(hub, &hubs, next) {
if (hub->id == hub_id) {
+ qemu_mutex_lock(&hub->ports_lock);
QLIST_FOREACH(port, &hub->ports, next) {
peer = port->nc.peer;
if (peer && strcmp(peer->name, name) == 0) {
+ qemu_mutex_unlock(&hub->ports_lock);
return peer;
}
}
+ qemu_mutex_unlock(&hub->ports_lock);
}
}
return NULL;
@@ -220,12 +233,15 @@ NetClientState *net_hub_port_find(int hub_id)
QLIST_FOREACH(hub, &hubs, next) {
if (hub->id == hub_id) {
+ qemu_mutex_lock(&hub->ports_lock);
QLIST_FOREACH(port, &hub->ports, next) {
nc = port->nc.peer;
if (!nc) {
+ qemu_mutex_unlock(&hub->ports_lock);
return &(port->nc);
}
}
+ qemu_mutex_unlock(&hub->ports_lock);
break;
}
}
@@ -244,12 +260,14 @@ void net_hub_info(Monitor *mon)
QLIST_FOREACH(hub, &hubs, next) {
monitor_printf(mon, "hub %d\n", hub->id);
+ qemu_mutex_lock(&hub->ports_lock);
QLIST_FOREACH(port, &hub->ports, next) {
if (port->nc.peer) {
monitor_printf(mon, " \\ ");
print_net_client(mon, port->nc.peer);
}
}
+ qemu_mutex_unlock(&hub->ports_lock);
}
}
@@ -306,6 +324,7 @@ void net_hub_check_clients(void)
QLIST_FOREACH(hub, &hubs, next) {
int has_nic = 0, has_host_dev = 0;
+ qemu_mutex_lock(&hub->ports_lock);
QLIST_FOREACH(port, &hub->ports, next) {
peer = port->nc.peer;
if (!peer) {
@@ -328,6 +347,7 @@ void net_hub_check_clients(void)
break;
}
}
+ qemu_mutex_unlock(&hub->ports_lock);
if (has_host_dev && !has_nic) {
fprintf(stderr, "Warning: vlan %d with no nics\n", hub->id);
}
@@ -343,12 +363,15 @@ bool net_hub_flush(NetClientState *nc)
{
NetHubPort *port;
NetHubPort *source_port = DO_UPCAST(NetHubPort, nc, nc);
+ NetHub *hub = source_port->hub;
int ret = 0;
+ qemu_mutex_lock(&hub->ports_lock);
QLIST_FOREACH(port, &source_port->hub->ports, next) {
if (port != source_port) {
ret += qemu_net_queue_flush(port->nc.send_queue);
}
}
+ qemu_mutex_unlock(&hub->ports_lock);
return ret ? true : false;
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [RFC PATCH v4 09/15] net: introduce lock to protect NetQueue
2013-04-17 8:39 [Qemu-devel] [RFC PATCH v4 00/15] port network layer onto glib Liu Ping Fan
` (7 preceding siblings ...)
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 08/15] net: hub use lock to protect ports list Liu Ping Fan
@ 2013-04-17 8:39 ` Liu Ping Fan
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 10/15] net: introduce lock to protect NetClientState's peer's access Liu Ping Fan
` (5 subsequent siblings)
14 siblings, 0 replies; 35+ messages in thread
From: Liu Ping Fan @ 2013-04-17 8:39 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, Paolo Bonzini, Stefan Hajnoczi, Anthony Liguori,
Jan Kiszka
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
NetQueue will be accessed by nc and its peers at the same time,
need lock to protect it.
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
net/queue.c | 11 +++++++++++
1 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/net/queue.c b/net/queue.c
index 859d02a..2856c1d 100644
--- a/net/queue.c
+++ b/net/queue.c
@@ -53,6 +53,7 @@ struct NetQueue {
uint32_t nq_maxlen;
uint32_t nq_count;
+ QemuMutex lock;
QTAILQ_HEAD(packets, NetPacket) packets;
unsigned delivering : 1;
@@ -68,6 +69,7 @@ NetQueue *qemu_new_net_queue(void *opaque)
queue->nq_maxlen = 10000;
queue->nq_count = 0;
+ qemu_mutex_init(&queue->lock);
QTAILQ_INIT(&queue->packets);
queue->delivering = 0;
@@ -107,7 +109,9 @@ static void qemu_net_queue_append(NetQueue *queue,
memcpy(packet->data, buf, size);
queue->nq_count++;
+ qemu_mutex_lock(&queue->lock);
QTAILQ_INSERT_TAIL(&queue->packets, packet, entry);
+ qemu_mutex_unlock(&queue->lock);
}
static void qemu_net_queue_append_iov(NetQueue *queue,
@@ -142,7 +146,9 @@ static void qemu_net_queue_append_iov(NetQueue *queue,
}
queue->nq_count++;
+ qemu_mutex_lock(&queue->lock);
QTAILQ_INSERT_TAIL(&queue->packets, packet, entry);
+ qemu_mutex_unlock(&queue->lock);
}
static ssize_t qemu_net_queue_deliver(NetQueue *queue,
@@ -229,6 +235,7 @@ void qemu_net_queue_purge(NetQueue *queue, NetClientState *from)
{
NetPacket *packet, *next;
+ qemu_mutex_lock(&queue->lock);
QTAILQ_FOREACH_SAFE(packet, &queue->packets, entry, next) {
if (packet->sender == from) {
QTAILQ_REMOVE(&queue->packets, packet, entry);
@@ -236,10 +243,12 @@ void qemu_net_queue_purge(NetQueue *queue, NetClientState *from)
g_free(packet);
}
}
+ qemu_mutex_unlock(&queue->lock);
}
bool qemu_net_queue_flush(NetQueue *queue)
{
+ qemu_mutex_lock(&queue->lock);
while (!QTAILQ_EMPTY(&queue->packets)) {
NetPacket *packet;
int ret;
@@ -256,6 +265,7 @@ bool qemu_net_queue_flush(NetQueue *queue)
if (ret == 0) {
queue->nq_count++;
QTAILQ_INSERT_HEAD(&queue->packets, packet, entry);
+ qemu_mutex_unlock(&queue->lock);
return false;
}
@@ -265,5 +275,6 @@ bool qemu_net_queue_flush(NetQueue *queue)
g_free(packet);
}
+ qemu_mutex_unlock(&queue->lock);
return true;
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [RFC PATCH v4 10/15] net: introduce lock to protect NetClientState's peer's access
2013-04-17 8:39 [Qemu-devel] [RFC PATCH v4 00/15] port network layer onto glib Liu Ping Fan
` (8 preceding siblings ...)
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 09/15] net: introduce lock to protect NetQueue Liu Ping Fan
@ 2013-04-17 8:39 ` Liu Ping Fan
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 11/15] net: make netclient re-entrant with refcnt Liu Ping Fan
` (4 subsequent siblings)
14 siblings, 0 replies; 35+ messages in thread
From: Liu Ping Fan @ 2013-04-17 8:39 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, Paolo Bonzini, Stefan Hajnoczi, Anthony Liguori,
Jan Kiszka
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
Introduce nc->peer_lock to shield off the race of nc->peer's reader and
deleter. With it, after deleter finish, no new qemu_send_packet_xx()
will append packet to peer->send_queue, therefore no new reference from
packet->sender to nc will exist in nc->peer->send_queue.
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
include/net/net.h | 7 +++++
net/net.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++---
net/queue.c | 4 +-
3 files changed, 84 insertions(+), 6 deletions(-)
diff --git a/include/net/net.h b/include/net/net.h
index 88332d2..54f91ea 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -5,6 +5,7 @@
#include "qemu-common.h"
#include "qapi/qmp/qdict.h"
#include "qemu/option.h"
+#include "qemu/thread.h"
#include "net/queue.h"
#include "migration/vmstate.h"
#include "qapi-types.h"
@@ -63,6 +64,10 @@ struct NetClientState {
NetClientInfo *info;
int link_down;
QTAILQ_ENTRY(NetClientState) next;
+ /* protect the race access of peer only between reader and writer.
+ * to resolve the writer's race condition, resort on biglock.
+ */
+ QemuMutex peer_lock;
NetClientState *peer;
NetQueue *send_queue;
char *model;
@@ -75,6 +80,7 @@ struct NetClientState {
typedef struct NICState {
NetClientState *ncs;
+ NetClientState **pending_peer;
NICConf *conf;
void *opaque;
bool peer_deleted;
@@ -102,6 +108,7 @@ NetClientState *qemu_find_vlan_client_by_name(Monitor *mon, int vlan_id,
const char *client_str);
typedef void (*qemu_nic_foreach)(NICState *nic, void *opaque);
void qemu_foreach_nic(qemu_nic_foreach func, void *opaque);
+int qemu_can_send_packet_nolock(NetClientState *sender);
int qemu_can_send_packet(NetClientState *nc);
ssize_t qemu_sendv_packet(NetClientState *nc, const struct iovec *iov,
int iovcnt);
diff --git a/net/net.c b/net/net.c
index f3d67f8..7619762 100644
--- a/net/net.c
+++ b/net/net.c
@@ -207,6 +207,7 @@ static void qemu_net_client_setup(NetClientState *nc,
nc->peer = peer;
peer->peer = nc;
}
+ qemu_mutex_init(&nc->peer_lock);
QTAILQ_INSERT_TAIL(&net_clients, nc, next);
nc->send_queue = qemu_new_net_queue(nc);
@@ -246,6 +247,7 @@ NICState *qemu_new_nic(NetClientInfo *info,
nic->ncs = (void *)nic + info->size;
nic->conf = conf;
nic->opaque = opaque;
+ nic->pending_peer = g_malloc0(sizeof(NetClientState *) * queues);
for (i = 0; i < queues; i++) {
qemu_net_client_setup(&nic->ncs[i], info, peers[i], model, name,
@@ -304,6 +306,38 @@ static void qemu_free_net_client(NetClientState *nc)
}
}
+/* elimate the reference and sync with exit of rx/tx action.
+ * And flush out peer's queue.
+ */
+static void qemu_net_client_detach_flush(NetClientState *nc)
+{
+ NetClientState *peer;
+
+ /* reader of self's peer field , fixme? the deleters are not concurrent,
+ * so this pair lock can save.
+ */
+ qemu_mutex_lock(&nc->peer_lock);
+ peer = nc->peer;
+ qemu_mutex_unlock(&nc->peer_lock);
+
+ /* writer of peer's peer field*/
+ if (peer) {
+ /* exclude the race with tx to @nc */
+ qemu_mutex_lock(&peer->peer_lock);
+ peer->peer = NULL;
+ qemu_mutex_unlock(&peer->peer_lock);
+ }
+
+ /* writer of self's peer field*/
+ /* exclude the race with tx from @nc */
+ qemu_mutex_lock(&nc->peer_lock);
+ nc->peer = NULL;
+ if (peer) {
+ qemu_net_queue_purge(peer->send_queue, nc);
+ }
+ qemu_mutex_unlock(&nc->peer_lock);
+}
+
void qemu_del_net_client(NetClientState *nc)
{
NetClientState *ncs[MAX_QUEUE_NUM];
@@ -334,7 +368,9 @@ void qemu_del_net_client(NetClientState *nc)
}
for (i = 0; i < queues; i++) {
+ qemu_net_client_detach_flush(ncs[i]);
qemu_cleanup_net_client(ncs[i]);
+ nic->pending_peer[i] = ncs[i];
}
return;
@@ -343,6 +379,7 @@ void qemu_del_net_client(NetClientState *nc)
assert(nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC);
for (i = 0; i < queues; i++) {
+ qemu_net_client_detach_flush(ncs[i]);
qemu_cleanup_net_client(ncs[i]);
qemu_free_net_client(ncs[i]);
}
@@ -355,17 +392,19 @@ void qemu_del_nic(NICState *nic)
/* If this is a peer NIC and peer has already been deleted, free it now. */
if (nic->peer_deleted) {
for (i = 0; i < queues; i++) {
- qemu_free_net_client(qemu_get_subqueue(nic, i)->peer);
+ qemu_free_net_client(nic->pending_peer[i]);
}
}
for (i = queues - 1; i >= 0; i--) {
NetClientState *nc = qemu_get_subqueue(nic, i);
+ qemu_net_client_detach_flush(nc);
qemu_cleanup_net_client(nc);
qemu_free_net_client(nc);
}
+ g_free(nic->pending_peer);
g_free(nic);
}
@@ -382,7 +421,7 @@ void qemu_foreach_nic(qemu_nic_foreach func, void *opaque)
}
}
-int qemu_can_send_packet(NetClientState *sender)
+int qemu_can_send_packet_nolock(NetClientState *sender)
{
if (!sender->peer) {
return 1;
@@ -397,6 +436,28 @@ int qemu_can_send_packet(NetClientState *sender)
return 1;
}
+int qemu_can_send_packet(NetClientState *sender)
+{
+ int ret = 1;
+
+ qemu_mutex_lock(&sender->peer_lock);
+ if (!sender->peer) {
+ goto unlock;
+ }
+
+ if (sender->peer->receive_disabled) {
+ ret = 0;
+ goto unlock;
+ } else if (sender->peer->info->can_receive &&
+ !sender->peer->info->can_receive(sender->peer)) {
+ ret = 0;
+ goto unlock;
+ }
+unlock:
+ qemu_mutex_unlock(&sender->peer_lock);
+ return ret;
+}
+
ssize_t qemu_deliver_packet(NetClientState *sender,
unsigned flags,
const uint8_t *data,
@@ -460,19 +521,24 @@ static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender,
NetPacketSent *sent_cb)
{
NetQueue *queue;
+ ssize_t sz;
#ifdef DEBUG_NET
printf("qemu_send_packet_async:\n");
hex_dump(stdout, buf, size);
#endif
+ qemu_mutex_lock(&sender->peer_lock);
if (sender->link_down || !sender->peer) {
+ qemu_mutex_unlock(&sender->peer_lock);
return size;
}
queue = sender->peer->send_queue;
- return qemu_net_queue_send(queue, sender, flags, buf, size, sent_cb);
+ sz = qemu_net_queue_send(queue, sender, flags, buf, size, sent_cb);
+ qemu_mutex_unlock(&sender->peer_lock);
+ return sz;
}
ssize_t qemu_send_packet_async(NetClientState *sender,
@@ -540,16 +606,21 @@ ssize_t qemu_sendv_packet_async(NetClientState *sender,
NetPacketSent *sent_cb)
{
NetQueue *queue;
+ ssize_t sz;
+ qemu_mutex_lock(&sender->peer_lock);
if (sender->link_down || !sender->peer) {
+ qemu_mutex_unlock(&sender->peer_lock);
return iov_size(iov, iovcnt);
}
queue = sender->peer->send_queue;
- return qemu_net_queue_send_iov(queue, sender,
+ sz = qemu_net_queue_send_iov(queue, sender,
QEMU_NET_PACKET_FLAG_NONE,
iov, iovcnt, sent_cb);
+ qemu_mutex_unlock(&sender->peer_lock);
+ return sz;
}
ssize_t
diff --git a/net/queue.c b/net/queue.c
index 2856c1d..123c338 100644
--- a/net/queue.c
+++ b/net/queue.c
@@ -190,7 +190,7 @@ ssize_t qemu_net_queue_send(NetQueue *queue,
{
ssize_t ret;
- if (queue->delivering || !qemu_can_send_packet(sender)) {
+ if (queue->delivering || !qemu_can_send_packet_nolock(sender)) {
qemu_net_queue_append(queue, sender, flags, data, size, sent_cb);
return 0;
}
@@ -215,7 +215,7 @@ ssize_t qemu_net_queue_send_iov(NetQueue *queue,
{
ssize_t ret;
- if (queue->delivering || !qemu_can_send_packet(sender)) {
+ if (queue->delivering || !qemu_can_send_packet_nolock(sender)) {
qemu_net_queue_append_iov(queue, sender, flags, iov, iovcnt, sent_cb);
return 0;
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [RFC PATCH v4 11/15] net: make netclient re-entrant with refcnt
2013-04-17 8:39 [Qemu-devel] [RFC PATCH v4 00/15] port network layer onto glib Liu Ping Fan
` (9 preceding siblings ...)
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 10/15] net: introduce lock to protect NetClientState's peer's access Liu Ping Fan
@ 2013-04-17 8:39 ` Liu Ping Fan
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 12/15] slirp: make timeout local Liu Ping Fan
` (3 subsequent siblings)
14 siblings, 0 replies; 35+ messages in thread
From: Liu Ping Fan @ 2013-04-17 8:39 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, Paolo Bonzini, Stefan Hajnoczi, Anthony Liguori,
Jan Kiszka
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
With refcnt, NetClientState's user can run agaist deleter.
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
hw/qdev-properties-system.c | 14 +++++++++++++
include/net/net.h | 3 ++
net/hub.c | 3 ++
net/net.c | 46 ++++++++++++++++++++++++++++++++++++++++--
net/slirp.c | 3 +-
5 files changed, 65 insertions(+), 4 deletions(-)
diff --git a/hw/qdev-properties-system.c b/hw/qdev-properties-system.c
index ce3af22..14c6d49 100644
--- a/hw/qdev-properties-system.c
+++ b/hw/qdev-properties-system.c
@@ -301,6 +301,7 @@ static void set_vlan(Object *obj, Visitor *v, void *opaque,
return;
}
+ /* inc ref, released when unset property */
hubport = net_hub_port_find(id);
if (!hubport) {
error_set(errp, QERR_INVALID_PARAMETER_VALUE,
@@ -310,11 +311,24 @@ static void set_vlan(Object *obj, Visitor *v, void *opaque,
*ptr = hubport;
}
+static void release_vlan(Object *obj, const char *name, void *opaque)
+{
+ DeviceState *dev = DEVICE(obj);
+ Property *prop = opaque;
+ NICPeers *peers_ptr = qdev_get_prop_ptr(dev, prop);
+ NetClientState **ptr = &peers_ptr->ncs[0];
+
+ if (*ptr) {
+ netclient_unref(*ptr);
+ }
+}
+
PropertyInfo qdev_prop_vlan = {
.name = "vlan",
.print = print_vlan,
.get = get_vlan,
.set = set_vlan,
+ .release = release_vlan,
};
int qdev_prop_set_drive(DeviceState *dev, const char *name,
diff --git a/include/net/net.h b/include/net/net.h
index 54f91ea..ef4137d 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -61,6 +61,7 @@ typedef struct NetClientInfo {
} NetClientInfo;
struct NetClientState {
+ int ref;
NetClientInfo *info;
int link_down;
QTAILQ_ENTRY(NetClientState) next;
@@ -89,6 +90,8 @@ typedef struct NICState {
NetClientState *qemu_find_netdev(const char *id);
int qemu_find_net_clients_except(const char *id, NetClientState **ncs,
NetClientOptionsKind type, int max);
+void netclient_ref(NetClientState *nc);
+void netclient_unref(NetClientState *nc);
NetClientState *qemu_new_net_client(NetClientInfo *info,
NetClientState *peer,
const char *model,
diff --git a/net/hub.c b/net/hub.c
index 812a6dc..2970f8e 100644
--- a/net/hub.c
+++ b/net/hub.c
@@ -212,6 +212,7 @@ NetClientState *net_hub_find_client_by_name(int hub_id, const char *name)
peer = port->nc.peer;
if (peer && strcmp(peer->name, name) == 0) {
+ netclient_ref(peer);
qemu_mutex_unlock(&hub->ports_lock);
return peer;
}
@@ -237,6 +238,7 @@ NetClientState *net_hub_port_find(int hub_id)
QLIST_FOREACH(port, &hub->ports, next) {
nc = port->nc.peer;
if (!nc) {
+ netclient_ref(&port->nc);
qemu_mutex_unlock(&hub->ports_lock);
return &(port->nc);
}
@@ -247,6 +249,7 @@ NetClientState *net_hub_port_find(int hub_id)
}
nc = net_hub_add_port(hub_id, NULL);
+ netclient_ref(nc);
return nc;
}
diff --git a/net/net.c b/net/net.c
index 7619762..ac859ff 100644
--- a/net/net.c
+++ b/net/net.c
@@ -45,6 +45,7 @@
# define CONFIG_NET_BRIDGE
#endif
+static QemuMutex net_clients_lock;
static QTAILQ_HEAD(, NetClientState) net_clients;
int default_net = 1;
@@ -166,6 +167,7 @@ static char *assign_name(NetClientState *nc1, const char *model)
char buf[256];
int id = 0;
+ qemu_mutex_lock(&net_clients_lock);
QTAILQ_FOREACH(nc, &net_clients, next) {
if (nc == nc1) {
continue;
@@ -176,6 +178,7 @@ static char *assign_name(NetClientState *nc1, const char *model)
id++;
}
}
+ qemu_mutex_unlock(&net_clients_lock);
snprintf(buf, sizeof(buf), "%s.%d", model, id);
@@ -206,9 +209,13 @@ static void qemu_net_client_setup(NetClientState *nc,
assert(!peer->peer);
nc->peer = peer;
peer->peer = nc;
+ netclient_ref(peer);
+ netclient_ref(nc);
}
qemu_mutex_init(&nc->peer_lock);
+ qemu_mutex_lock(&net_clients_lock);
QTAILQ_INSERT_TAIL(&net_clients, nc, next);
+ qemu_mutex_unlock(&net_clients_lock);
nc->send_queue = qemu_new_net_queue(nc);
nc->destructor = destructor;
@@ -224,6 +231,7 @@ NetClientState *qemu_new_net_client(NetClientInfo *info,
assert(info->size >= sizeof(NetClientState));
nc = g_malloc0(info->size);
+ netclient_ref(nc);
qemu_net_client_setup(nc, info, peer, model, name,
qemu_net_client_destructor);
@@ -284,7 +292,9 @@ void *qemu_get_nic_opaque(NetClientState *nc)
static void qemu_cleanup_net_client(NetClientState *nc)
{
+ qemu_mutex_lock(&net_clients_lock);
QTAILQ_REMOVE(&net_clients, nc, next);
+ qemu_mutex_unlock(&net_clients_lock);
if (nc->info->cleanup) {
nc->info->cleanup(nc);
@@ -306,6 +316,18 @@ static void qemu_free_net_client(NetClientState *nc)
}
}
+void netclient_ref(NetClientState *nc)
+{
+ __sync_add_and_fetch(&nc->ref, 1);
+}
+
+void netclient_unref(NetClientState *nc)
+{
+ if (__sync_sub_and_fetch(&nc->ref, 1) == 0) {
+ qemu_free_net_client(nc);
+ }
+}
+
/* elimate the reference and sync with exit of rx/tx action.
* And flush out peer's queue.
*/
@@ -334,8 +356,10 @@ static void qemu_net_client_detach_flush(NetClientState *nc)
nc->peer = NULL;
if (peer) {
qemu_net_queue_purge(peer->send_queue, nc);
+ netclient_unref(peer);
}
qemu_mutex_unlock(&nc->peer_lock);
+ netclient_unref(nc);
}
void qemu_del_net_client(NetClientState *nc)
@@ -381,7 +405,7 @@ void qemu_del_net_client(NetClientState *nc)
for (i = 0; i < queues; i++) {
qemu_net_client_detach_flush(ncs[i]);
qemu_cleanup_net_client(ncs[i]);
- qemu_free_net_client(ncs[i]);
+ netclient_unref(ncs[i]);
}
}
@@ -392,7 +416,7 @@ void qemu_del_nic(NICState *nic)
/* If this is a peer NIC and peer has already been deleted, free it now. */
if (nic->peer_deleted) {
for (i = 0; i < queues; i++) {
- qemu_free_net_client(nic->pending_peer[i]);
+ netclient_unref(nic->pending_peer[i]);
}
}
@@ -401,7 +425,7 @@ void qemu_del_nic(NICState *nic)
qemu_net_client_detach_flush(nc);
qemu_cleanup_net_client(nc);
- qemu_free_net_client(nc);
+ netclient_unref(nc);
}
g_free(nic->pending_peer);
@@ -412,6 +436,7 @@ void qemu_foreach_nic(qemu_nic_foreach func, void *opaque)
{
NetClientState *nc;
+ qemu_mutex_lock(&net_clients_lock);
QTAILQ_FOREACH(nc, &net_clients, next) {
if (nc->info->type == NET_CLIENT_OPTIONS_KIND_NIC) {
if (nc->queue_index == 0) {
@@ -419,6 +444,7 @@ void qemu_foreach_nic(qemu_nic_foreach func, void *opaque)
}
}
}
+ qemu_mutex_unlock(&net_clients_lock);
}
int qemu_can_send_packet_nolock(NetClientState *sender)
@@ -633,13 +659,17 @@ NetClientState *qemu_find_netdev(const char *id)
{
NetClientState *nc;
+ qemu_mutex_lock(&net_clients_lock);
QTAILQ_FOREACH(nc, &net_clients, next) {
if (nc->info->type == NET_CLIENT_OPTIONS_KIND_NIC)
continue;
if (!strcmp(nc->name, id)) {
+ netclient_ref(nc);
+ qemu_mutex_unlock(&net_clients_lock);
return nc;
}
}
+ qemu_mutex_unlock(&net_clients_lock);
return NULL;
}
@@ -650,6 +680,7 @@ int qemu_find_net_clients_except(const char *id, NetClientState **ncs,
NetClientState *nc;
int ret = 0;
+ qemu_mutex_lock(&net_clients_lock);
QTAILQ_FOREACH(nc, &net_clients, next) {
if (nc->info->type == type) {
continue;
@@ -661,6 +692,7 @@ int qemu_find_net_clients_except(const char *id, NetClientState **ncs,
ret++;
}
}
+ qemu_mutex_unlock(&net_clients_lock);
return ret;
}
@@ -969,6 +1001,7 @@ void net_host_device_remove(Monitor *mon, const QDict *qdict)
return;
}
qemu_del_net_client(nc);
+ netclient_unref(nc);
}
void netdev_add(QemuOpts *opts, Error **errp)
@@ -1024,6 +1057,7 @@ void qmp_netdev_del(const char *id, Error **errp)
}
qemu_del_net_client(nc);
+ netclient_unref(nc);
qemu_opts_del(opts);
}
@@ -1042,6 +1076,7 @@ void do_info_network(Monitor *mon, const QDict *qdict)
net_hub_info(mon);
+ qemu_mutex_lock(&net_clients_lock);
QTAILQ_FOREACH(nc, &net_clients, next) {
peer = nc->peer;
type = nc->info->type;
@@ -1059,6 +1094,7 @@ void do_info_network(Monitor *mon, const QDict *qdict)
print_net_client(mon, peer);
}
}
+ qemu_mutex_unlock(&net_clients_lock);
}
void qmp_set_link(const char *name, bool up, Error **errp)
@@ -1112,6 +1148,7 @@ void net_cleanup(void)
qemu_del_net_client(nc);
}
}
+ qemu_mutex_destroy(&net_clients_lock);
}
void net_check_clients(void)
@@ -1133,6 +1170,7 @@ void net_check_clients(void)
net_hub_check_clients();
+ qemu_mutex_lock(&net_clients_lock);
QTAILQ_FOREACH(nc, &net_clients, next) {
if (!nc->peer) {
fprintf(stderr, "Warning: %s %s has no peer\n",
@@ -1140,6 +1178,7 @@ void net_check_clients(void)
"nic" : "netdev", nc->name);
}
}
+ qemu_mutex_unlock(&net_clients_lock);
/* Check that all NICs requested via -net nic actually got created.
* NICs created via -device don't need to be checked here because
@@ -1197,6 +1236,7 @@ int net_init_clients(void)
#endif
}
+ qemu_mutex_init(&net_clients_lock);
QTAILQ_INIT(&net_clients);
if (qemu_opts_foreach(qemu_find_opts("netdev"), net_init_netdev, NULL, 1) == -1)
diff --git a/net/slirp.c b/net/slirp.c
index 4df550f..a6116d5 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -346,7 +346,7 @@ void net_slirp_hostfwd_remove(Monitor *mon, const QDict *qdict)
err = slirp_remove_hostfwd(QTAILQ_FIRST(&slirp_stacks)->slirp, is_udp,
host_addr, host_port);
-
+ netclient_unref(&s->nc);
monitor_printf(mon, "host forwarding rule for %s %s\n", src_str,
err ? "not found" : "removed");
return;
@@ -437,6 +437,7 @@ void net_slirp_hostfwd_add(Monitor *mon, const QDict *qdict)
}
if (s) {
slirp_hostfwd(s, redir_str, 0);
+ netclient_unref(&s->nc);
}
}
--
1.7.4.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [RFC PATCH v4 12/15] slirp: make timeout local
2013-04-17 8:39 [Qemu-devel] [RFC PATCH v4 00/15] port network layer onto glib Liu Ping Fan
` (10 preceding siblings ...)
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 11/15] net: make netclient re-entrant with refcnt Liu Ping Fan
@ 2013-04-17 8:39 ` Liu Ping Fan
2013-04-18 14:22 ` Paolo Bonzini
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 13/15] slirp: make slirp event dispatch based on slirp instance, not global Liu Ping Fan
` (2 subsequent siblings)
14 siblings, 1 reply; 35+ messages in thread
From: Liu Ping Fan @ 2013-04-17 8:39 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, Paolo Bonzini, Stefan Hajnoczi, Anthony Liguori,
Jan Kiszka
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
Each slirp has its own time to caculate timeout.
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
slirp/slirp.c | 22 ++++++++++------------
slirp/slirp.h | 3 +++
2 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/slirp/slirp.c b/slirp/slirp.c
index bd9b7cb..5f1c5e8 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -40,8 +40,6 @@ static const uint8_t special_ethaddr[ETH_ALEN] = {
static const uint8_t zero_ethaddr[ETH_ALEN] = { 0, 0, 0, 0, 0, 0 };
u_int curtime;
-static u_int time_fasttimo, last_slowtimo;
-static int do_slowtimo;
static QTAILQ_HEAD(slirp_instances, Slirp) slirp_instances =
QTAILQ_HEAD_INITIALIZER(slirp_instances);
@@ -278,14 +276,14 @@ void slirp_pollfds_fill(GArray *pollfds)
/*
* First, TCP sockets
*/
- do_slowtimo = 0;
QTAILQ_FOREACH(slirp, &slirp_instances, entry) {
/*
* *_slowtimo needs calling if there are IP fragments
* in the fragment queue, or there are TCP connections active
*/
- do_slowtimo |= ((slirp->tcb.so_next != &slirp->tcb) ||
+ slirp->do_slowtimo = 0;
+ slirp->do_slowtimo |= ((slirp->tcb.so_next != &slirp->tcb) ||
(&slirp->ipq.ip_link != slirp->ipq.ip_link.next));
for (so = slirp->tcb.so_next; so != &slirp->tcb;
@@ -299,8 +297,8 @@ void slirp_pollfds_fill(GArray *pollfds)
/*
* See if we need a tcp_fasttimo
*/
- if (time_fasttimo == 0 && so->so_tcpcb->t_flags & TF_DELACK) {
- time_fasttimo = curtime; /* Flag when we want a fasttimo */
+ if (slirp->time_fasttimo == 0 && so->so_tcpcb->t_flags & TF_DELACK) {
+ slirp->time_fasttimo = curtime; /* Flag when we want a fasttimo */
}
/*
@@ -381,7 +379,7 @@ void slirp_pollfds_fill(GArray *pollfds)
udp_detach(so);
continue;
} else {
- do_slowtimo = 1; /* Let socket expire */
+ slirp->do_slowtimo = 1; /* Let socket expire */
}
}
@@ -422,7 +420,7 @@ void slirp_pollfds_fill(GArray *pollfds)
icmp_detach(so);
continue;
} else {
- do_slowtimo = 1; /* Let socket expire */
+ slirp->do_slowtimo = 1; /* Let socket expire */
}
}
@@ -454,14 +452,14 @@ void slirp_pollfds_poll(GArray *pollfds, int select_error)
/*
* See if anything has timed out
*/
- if (time_fasttimo && ((curtime - time_fasttimo) >= 2)) {
+ if (slirp->time_fasttimo && ((curtime - slirp->time_fasttimo) >= 2)) {
tcp_fasttimo(slirp);
- time_fasttimo = 0;
+ slirp->time_fasttimo = 0;
}
- if (do_slowtimo && ((curtime - last_slowtimo) >= 499)) {
+ if (slirp->do_slowtimo && ((curtime - slirp->last_slowtimo) >= 499)) {
ip_slowtimo(slirp);
tcp_slowtimo(slirp);
- last_slowtimo = curtime;
+ slirp->last_slowtimo = curtime;
}
/*
diff --git a/slirp/slirp.h b/slirp/slirp.h
index fe0e65d..008360e 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -203,6 +203,9 @@ bool arp_table_search(Slirp *slirp, uint32_t ip_addr,
struct Slirp {
QTAILQ_ENTRY(Slirp) entry;
+ u_int time_fasttimo;
+ u_int last_slowtimo;
+ int do_slowtimo;
/* virtual network configuration */
struct in_addr vnetwork_addr;
--
1.7.4.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [RFC PATCH v4 12/15] slirp: make timeout local
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 12/15] slirp: make timeout local Liu Ping Fan
@ 2013-04-18 14:22 ` Paolo Bonzini
0 siblings, 0 replies; 35+ messages in thread
From: Paolo Bonzini @ 2013-04-18 14:22 UTC (permalink / raw)
To: Liu Ping Fan
Cc: Jan Kiszka, Stefan Hajnoczi, qemu-devel, Anthony Liguori, mdroth
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>
> Each slirp has its own time to caculate timeout.
Nice cleanup to have anyway upstream.
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
> slirp/slirp.c | 22 ++++++++++------------
> slirp/slirp.h | 3 +++
> 2 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index bd9b7cb..5f1c5e8 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -40,8 +40,6 @@ static const uint8_t special_ethaddr[ETH_ALEN] = {
> static const uint8_t zero_ethaddr[ETH_ALEN] = { 0, 0, 0, 0, 0, 0 };
>
> u_int curtime;
> -static u_int time_fasttimo, last_slowtimo;
> -static int do_slowtimo;
>
> static QTAILQ_HEAD(slirp_instances, Slirp) slirp_instances =
> QTAILQ_HEAD_INITIALIZER(slirp_instances);
> @@ -278,14 +276,14 @@ void slirp_pollfds_fill(GArray *pollfds)
> /*
> * First, TCP sockets
> */
> - do_slowtimo = 0;
>
> QTAILQ_FOREACH(slirp, &slirp_instances, entry) {
> /*
> * *_slowtimo needs calling if there are IP fragments
> * in the fragment queue, or there are TCP connections active
> */
> - do_slowtimo |= ((slirp->tcb.so_next != &slirp->tcb) ||
> + slirp->do_slowtimo = 0;
> + slirp->do_slowtimo |= ((slirp->tcb.so_next != &slirp->tcb) ||
> (&slirp->ipq.ip_link != slirp->ipq.ip_link.next));
No need to do = 0 here.
Paolo
>
> for (so = slirp->tcb.so_next; so != &slirp->tcb;
> @@ -299,8 +297,8 @@ void slirp_pollfds_fill(GArray *pollfds)
> /*
> * See if we need a tcp_fasttimo
> */
> - if (time_fasttimo == 0 && so->so_tcpcb->t_flags & TF_DELACK) {
> - time_fasttimo = curtime; /* Flag when we want a fasttimo */
> + if (slirp->time_fasttimo == 0 && so->so_tcpcb->t_flags & TF_DELACK) {
> + slirp->time_fasttimo = curtime; /* Flag when we want a fasttimo */
> }
>
> /*
> @@ -381,7 +379,7 @@ void slirp_pollfds_fill(GArray *pollfds)
> udp_detach(so);
> continue;
> } else {
> - do_slowtimo = 1; /* Let socket expire */
> + slirp->do_slowtimo = 1; /* Let socket expire */
> }
> }
>
> @@ -422,7 +420,7 @@ void slirp_pollfds_fill(GArray *pollfds)
> icmp_detach(so);
> continue;
> } else {
> - do_slowtimo = 1; /* Let socket expire */
> + slirp->do_slowtimo = 1; /* Let socket expire */
> }
> }
>
> @@ -454,14 +452,14 @@ void slirp_pollfds_poll(GArray *pollfds, int
> select_error)
> /*
> * See if anything has timed out
> */
> - if (time_fasttimo && ((curtime - time_fasttimo) >= 2)) {
> + if (slirp->time_fasttimo && ((curtime - slirp->time_fasttimo) >= 2))
> {
> tcp_fasttimo(slirp);
> - time_fasttimo = 0;
> + slirp->time_fasttimo = 0;
> }
> - if (do_slowtimo && ((curtime - last_slowtimo) >= 499)) {
> + if (slirp->do_slowtimo && ((curtime - slirp->last_slowtimo) >= 499))
> {
> ip_slowtimo(slirp);
> tcp_slowtimo(slirp);
> - last_slowtimo = curtime;
> + slirp->last_slowtimo = curtime;
> }
>
> /*
> diff --git a/slirp/slirp.h b/slirp/slirp.h
> index fe0e65d..008360e 100644
> --- a/slirp/slirp.h
> +++ b/slirp/slirp.h
> @@ -203,6 +203,9 @@ bool arp_table_search(Slirp *slirp, uint32_t ip_addr,
>
> struct Slirp {
> QTAILQ_ENTRY(Slirp) entry;
> + u_int time_fasttimo;
> + u_int last_slowtimo;
> + int do_slowtimo;
>
> /* virtual network configuration */
> struct in_addr vnetwork_addr;
> --
> 1.7.4.4
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [RFC PATCH v4 13/15] slirp: make slirp event dispatch based on slirp instance, not global
2013-04-17 8:39 [Qemu-devel] [RFC PATCH v4 00/15] port network layer onto glib Liu Ping Fan
` (11 preceding siblings ...)
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 12/15] slirp: make timeout local Liu Ping Fan
@ 2013-04-17 8:39 ` Liu Ping Fan
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 14/15] slirp: handle race condition Liu Ping Fan
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 15/15] slirp: use lock to protect the slirp_instances Liu Ping Fan
14 siblings, 0 replies; 35+ messages in thread
From: Liu Ping Fan @ 2013-04-17 8:39 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, Paolo Bonzini, Stefan Hajnoczi, Anthony Liguori,
Jan Kiszka
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
Split slirp_pollfds_fill/poll actions into each slirp, so that SlirpState
can run on dedicated context.
Each slirp socket will corresponds to a GPollFD, and its SlirpState stands
for a GSource(EventsGSource). Finally different SlirpState can run on different
context.
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
net: port slirp to glib
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
main-loop.c | 4 -
net/slirp.c | 44 ++++
slirp/libslirp.h | 7 +-
slirp/slirp.c | 587 +++++++++++++++++++++++++-----------------------------
slirp/socket.c | 2 +
slirp/socket.h | 1 +
stubs/slirp.c | 8 -
7 files changed, 321 insertions(+), 332 deletions(-)
diff --git a/main-loop.c b/main-loop.c
index 8c9b58c..970f25d 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -432,14 +432,10 @@ int main_loop_wait(int nonblocking)
/* XXX: separate device handlers from system ones */
#ifdef CONFIG_SLIRP
slirp_update_timeout(&timeout);
- slirp_pollfds_fill(gpollfds);
#endif
qemu_iohandler_fill(gpollfds);
ret = os_host_main_loop_wait(timeout);
qemu_iohandler_poll(gpollfds, ret);
-#ifdef CONFIG_SLIRP
- slirp_pollfds_poll(gpollfds, (ret < 0));
-#endif
qemu_run_all_timers();
diff --git a/net/slirp.c b/net/slirp.c
index a6116d5..ece98f0 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -36,6 +36,7 @@
#include "qemu/sockets.h"
#include "slirp/libslirp.h"
#include "char/char.h"
+#include "util/event_gsource.h"
static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
{
@@ -76,6 +77,7 @@ typedef struct SlirpState {
#ifndef _WIN32
char smb_dir[128];
#endif
+ EventsGSource *slirp_src;
} SlirpState;
static struct slirp_config_str *slirp_configs;
@@ -120,17 +122,56 @@ static void net_slirp_cleanup(NetClientState *nc)
SlirpState *s = DO_UPCAST(SlirpState, nc, nc);
slirp_cleanup(s->slirp);
+ events_source_release(s->slirp_src);
slirp_smb_cleanup(s);
QTAILQ_REMOVE(&slirp_stacks, s, entry);
}
+static void net_slirp_bind_ctx(NetClientState *nc, GMainContext *ctx)
+{
+ SlirpState *s = DO_UPCAST(SlirpState, nc, nc);
+
+ g_source_attach(&s->slirp_src->source, ctx);
+}
+
static NetClientInfo net_slirp_info = {
.type = NET_CLIENT_OPTIONS_KIND_USER,
.size = sizeof(SlirpState),
.receive = net_slirp_receive,
.cleanup = net_slirp_cleanup,
+ .bind_ctx = net_slirp_bind_ctx,
};
+static GSourceFuncs slirp_gfuncs = {
+ .prepare = slirp_prepare,
+ .check = events_source_check,
+ .dispatch = events_source_dispatch,
+};
+
+static EventsGSource *slirp_source_new(GSourceFunc dispatch_cb, void *opaque)
+{
+ return events_source_new(&slirp_gfuncs, dispatch_cb, opaque);
+}
+
+GPollFD *slirp_gsource_get_gfd(void *opaque, int fd)
+{
+ GPollFD *retfd;
+ SlirpState *s = opaque;
+ EventsGSource *src = s->slirp_src;
+
+ retfd = events_source_get_gfd(src, fd);
+
+ return retfd;
+}
+
+void slirp_gsource_close_gfd(void *opaque, GPollFD *pollfd)
+{
+ SlirpState *s = opaque;
+ EventsGSource *src = s->slirp_src;
+
+ events_source_close_gfd(src, pollfd);
+}
+
static int net_slirp_init(NetClientState *peer, const char *model,
const char *name, int restricted,
const char *vnetwork, const char *vhost,
@@ -244,6 +285,8 @@ static int net_slirp_init(NetClientState *peer, const char *model,
s->slirp = slirp_init(restricted, net, mask, host, vhostname,
tftp_export, bootfile, dhcp, dns, dnssearch, s);
+ s->slirp_src = slirp_source_new(slirp_handler, s->slirp);
+
QTAILQ_INSERT_TAIL(&slirp_stacks, s, entry);
for (config = slirp_configs; config; config = config->next) {
@@ -266,6 +309,7 @@ static int net_slirp_init(NetClientState *peer, const char *model,
goto error;
}
#endif
+ s->nc.info->bind_ctx(&s->nc, NULL);
return 0;
diff --git a/slirp/libslirp.h b/slirp/libslirp.h
index ceabff8..1aad5a4 100644
--- a/slirp/libslirp.h
+++ b/slirp/libslirp.h
@@ -17,11 +17,10 @@ Slirp *slirp_init(int restricted, struct in_addr vnetwork,
void slirp_cleanup(Slirp *slirp);
void slirp_update_timeout(uint32_t *timeout);
-void slirp_pollfds_fill(GArray *pollfds);
-
-void slirp_pollfds_poll(GArray *pollfds, int select_error);
void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len);
+gboolean slirp_prepare(GSource *source, gint *time);
+gboolean slirp_handler(gpointer data);
/* you must provide the following functions: */
void slirp_output(void *opaque, const uint8_t *pkt, int pkt_len);
@@ -40,5 +39,7 @@ void slirp_socket_recv(Slirp *slirp, struct in_addr guest_addr,
int guest_port, const uint8_t *buf, int size);
size_t slirp_socket_can_recv(Slirp *slirp, struct in_addr guest_addr,
int guest_port);
+GPollFD *slirp_gsource_get_gfd(void *opaque, int fd);
+void slirp_gsource_close_gfd(void *opaque, GPollFD *pollfd);
#endif
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 5f1c5e8..883b7bd 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -26,6 +26,7 @@
#include "char/char.h"
#include "slirp.h"
#include "hw/hw.h"
+#include "util/event_gsource.h"
/* host loopback address */
struct in_addr loopback_addr;
@@ -262,386 +263,338 @@ void slirp_update_timeout(uint32_t *timeout)
if (!QTAILQ_EMPTY(&slirp_instances)) {
*timeout = MIN(1000, *timeout);
}
+ curtime = qemu_get_clock_ms(rt_clock);
}
-void slirp_pollfds_fill(GArray *pollfds)
+gboolean slirp_prepare(GSource *source, gint *time)
{
- Slirp *slirp;
+ EventsGSource *slirp_src = (EventsGSource *)source;
+ Slirp *slirp = slirp_src->opaque;
struct socket *so, *so_next;
-
- if (QTAILQ_EMPTY(&slirp_instances)) {
- return;
- }
+ int events = 0;
/*
- * First, TCP sockets
+ * *_slowtimo needs calling if there are IP fragments
+ * in the fragment queue, or there are TCP connections active
*/
+ slirp->do_slowtimo = 0;
+ slirp->do_slowtimo |= ((slirp->tcb.so_next != &slirp->tcb) ||
+ (&slirp->ipq.ip_link != slirp->ipq.ip_link.next));
+
+ for (so = slirp->tcb.so_next; so != &slirp->tcb;
+ so = so_next) {
- QTAILQ_FOREACH(slirp, &slirp_instances, entry) {
+ so_next = so->so_next;
+ if (so->pollfd->fd == -1 && so->s != -1) {
+ so->pollfd->fd = so->s;
+ g_source_add_poll(source, so->pollfd);
+ }
/*
- * *_slowtimo needs calling if there are IP fragments
- * in the fragment queue, or there are TCP connections active
+ * See if we need a tcp_fasttimo
*/
- slirp->do_slowtimo = 0;
- slirp->do_slowtimo |= ((slirp->tcb.so_next != &slirp->tcb) ||
- (&slirp->ipq.ip_link != slirp->ipq.ip_link.next));
-
- for (so = slirp->tcb.so_next; so != &slirp->tcb;
- so = so_next) {
- int events = 0;
-
- so_next = so->so_next;
-
- so->pollfds_idx = -1;
-
- /*
- * See if we need a tcp_fasttimo
- */
- if (slirp->time_fasttimo == 0 && so->so_tcpcb->t_flags & TF_DELACK) {
- slirp->time_fasttimo = curtime; /* Flag when we want a fasttimo */
- }
-
- /*
- * NOFDREF can include still connecting to local-host,
- * newly socreated() sockets etc. Don't want to select these.
- */
- if (so->so_state & SS_NOFDREF || so->s == -1) {
- continue;
- }
-
- /*
- * Set for reading sockets which are accepting
- */
- if (so->so_state & SS_FACCEPTCONN) {
- GPollFD pfd = {
- .fd = so->s,
- .events = G_IO_IN | G_IO_HUP | G_IO_ERR,
- };
- so->pollfds_idx = pollfds->len;
- g_array_append_val(pollfds, pfd);
- continue;
- }
+ if (slirp->time_fasttimo == 0 &&
+ so->so_tcpcb->t_flags & TF_DELACK) {
+ slirp->time_fasttimo = curtime; /* Flag when want a fasttimo */
+ }
- /*
- * Set for writing sockets which are connecting
- */
- if (so->so_state & SS_ISFCONNECTING) {
- GPollFD pfd = {
- .fd = so->s,
- .events = G_IO_OUT | G_IO_ERR,
- };
- so->pollfds_idx = pollfds->len;
- g_array_append_val(pollfds, pfd);
- continue;
- }
+ /*
+ * NOFDREF can include still connecting to local-host,
+ * newly socreated() sockets etc. Don't want to select these.
+ */
+ if (so->so_state & SS_NOFDREF || so->s == -1) {
+ continue;
+ }
- /*
- * Set for writing if we are connected, can send more, and
- * we have something to send
- */
- if (CONN_CANFSEND(so) && so->so_rcv.sb_cc) {
- events |= G_IO_OUT | G_IO_ERR;
- }
+ /*
+ * Set for reading sockets which are accepting
+ */
+ if (so->so_state & SS_FACCEPTCONN) {
+ so->pollfd->events = G_IO_IN | G_IO_HUP | G_IO_ERR;
+ continue;
+ }
- /*
- * Set for reading (and urgent data) if we are connected, can
- * receive more, and we have room for it XXX /2 ?
- */
- if (CONN_CANFRCV(so) &&
- (so->so_snd.sb_cc < (so->so_snd.sb_datalen/2))) {
- events |= G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_PRI;
- }
+ /*
+ * Set for writing sockets which are connecting
+ */
+ if (so->so_state & SS_ISFCONNECTING) {
+ so->pollfd->events = G_IO_OUT | G_IO_ERR;
+ continue;
+ }
- if (events) {
- GPollFD pfd = {
- .fd = so->s,
- .events = events,
- };
- so->pollfds_idx = pollfds->len;
- g_array_append_val(pollfds, pfd);
- }
+ /*
+ * Set for writing if we are connected, can send more, and
+ * we have something to send
+ */
+ if (CONN_CANFSEND(so) && so->so_rcv.sb_cc) {
+ events |= G_IO_OUT | G_IO_ERR;
}
/*
- * UDP sockets
+ * Set for reading (and urgent data) if we are connected, can
+ * receive more, and we have room for it XXX /2 ?
*/
- for (so = slirp->udb.so_next; so != &slirp->udb;
- so = so_next) {
- so_next = so->so_next;
+ if (CONN_CANFRCV(so) &&
+ (so->so_snd.sb_cc < (so->so_snd.sb_datalen/2))) {
+ events |= G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_PRI;
+ }
- so->pollfds_idx = -1;
+ if (events) {
+ so->pollfd->events = events;
+ }
+ }
- /*
- * See if it's timed out
- */
- if (so->so_expire) {
- if (so->so_expire <= curtime) {
- udp_detach(so);
- continue;
- } else {
- slirp->do_slowtimo = 1; /* Let socket expire */
- }
- }
+ /*
+ * UDP sockets
+ */
+ for (so = slirp->udb.so_next; so != &slirp->udb;
+ so = so_next) {
+ so_next = so->so_next;
- /*
- * When UDP packets are received from over the
- * link, they're sendto()'d straight away, so
- * no need for setting for writing
- * Limit the number of packets queued by this session
- * to 4. Note that even though we try and limit this
- * to 4 packets, the session could have more queued
- * if the packets needed to be fragmented
- * (XXX <= 4 ?)
- */
- if ((so->so_state & SS_ISFCONNECTED) && so->so_queued <= 4) {
- GPollFD pfd = {
- .fd = so->s,
- .events = G_IO_IN | G_IO_HUP | G_IO_ERR,
- };
- so->pollfds_idx = pollfds->len;
- g_array_append_val(pollfds, pfd);
+ /*
+ * See if it's timed out
+ */
+ if (so->so_expire) {
+ if (so->so_expire <= curtime) {
+ udp_detach(so);
+ continue;
+ } else {
+ slirp->do_slowtimo = 1; /* Let socket expire */
}
}
/*
- * ICMP sockets
+ * When UDP packets are received from over the
+ * link, they're sendto()'d straight away, so
+ * no need for setting for writing
+ * Limit the number of packets queued by this session
+ * to 4. Note that even though we try and limit this
+ * to 4 packets, the session could have more queued
+ * if the packets needed to be fragmented
+ * (XXX <= 4 ?)
*/
- for (so = slirp->icmp.so_next; so != &slirp->icmp;
- so = so_next) {
- so_next = so->so_next;
+ if ((so->so_state & SS_ISFCONNECTED) && so->so_queued <= 4) {
+ so->pollfd->events = G_IO_IN | G_IO_HUP | G_IO_ERR;
+ }
+ }
- so->pollfds_idx = -1;
+ /*
+ * ICMP sockets
+ */
+ for (so = slirp->icmp.so_next; so != &slirp->icmp;
+ so = so_next) {
+ so_next = so->so_next;
- /*
- * See if it's timed out
- */
- if (so->so_expire) {
- if (so->so_expire <= curtime) {
- icmp_detach(so);
- continue;
- } else {
- slirp->do_slowtimo = 1; /* Let socket expire */
- }
+ /*
+ * See if it's timed out
+ */
+ if (so->so_expire) {
+ if (so->so_expire <= curtime) {
+ icmp_detach(so);
+ continue;
+ } else {
+ slirp->do_slowtimo = 1; /* Let socket expire */
}
+ }
- if (so->so_state & SS_ISFCONNECTED) {
- GPollFD pfd = {
- .fd = so->s,
- .events = G_IO_IN | G_IO_HUP | G_IO_ERR,
- };
- so->pollfds_idx = pollfds->len;
- g_array_append_val(pollfds, pfd);
- }
+ if (so->so_state & SS_ISFCONNECTED) {
+ so->pollfd->events = G_IO_IN | G_IO_HUP | G_IO_ERR;
}
}
+
+ return false;
}
-void slirp_pollfds_poll(GArray *pollfds, int select_error)
+gboolean slirp_handler(gpointer data)
{
- Slirp *slirp;
+ EventsGSource *src = data;
+ Slirp *slirp = src->opaque;
struct socket *so, *so_next;
int ret;
- if (QTAILQ_EMPTY(&slirp_instances)) {
- return;
+ /*
+ * See if anything has timed out
+ */
+ if (slirp->time_fasttimo && ((curtime - slirp->time_fasttimo) >= 2)) {
+ tcp_fasttimo(slirp);
+ slirp->time_fasttimo = 0;
+ }
+ if (slirp->do_slowtimo && ((curtime - slirp->last_slowtimo) >= 499)) {
+ ip_slowtimo(slirp);
+ tcp_slowtimo(slirp);
+ slirp->last_slowtimo = curtime;
}
- curtime = qemu_get_clock_ms(rt_clock);
+ /*
+ * Check TCP sockets
+ */
+ for (so = slirp->tcb.so_next; so != &slirp->tcb;
+ so = so_next) {
+ int revents;
- QTAILQ_FOREACH(slirp, &slirp_instances, entry) {
- /*
- * See if anything has timed out
- */
- if (slirp->time_fasttimo && ((curtime - slirp->time_fasttimo) >= 2)) {
- tcp_fasttimo(slirp);
- slirp->time_fasttimo = 0;
+ so_next = so->so_next;
+
+ revents = 0;
+ if (so->pollfd) {
+ revents = so->pollfd->revents;
}
- if (slirp->do_slowtimo && ((curtime - slirp->last_slowtimo) >= 499)) {
- ip_slowtimo(slirp);
- tcp_slowtimo(slirp);
- slirp->last_slowtimo = curtime;
+ if (so->so_state & SS_NOFDREF || so->s == -1) {
+ continue;
}
/*
- * Check sockets
+ * Check for URG data
+ * This will soread as well, so no need to
+ * test for G_IO_IN below if this succeeds
*/
- if (!select_error) {
+ if (revents & G_IO_PRI) {
+ sorecvoob(so);
+ }
+ /*
+ * Check sockets for reading
+ */
+ else if (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR)) {
/*
- * Check TCP sockets
+ * Check for incoming connections
*/
- for (so = slirp->tcb.so_next; so != &slirp->tcb;
- so = so_next) {
- int revents;
-
- so_next = so->so_next;
-
- revents = 0;
- if (so->pollfds_idx != -1) {
- revents = g_array_index(pollfds, GPollFD,
- so->pollfds_idx).revents;
- }
+ if (so->so_state & SS_FACCEPTCONN) {
+ tcp_connect(so);
+ continue;
+ } /* else */
+ ret = soread(so);
- if (so->so_state & SS_NOFDREF || so->s == -1) {
- continue;
- }
+ /* Output it if we read something */
+ if (ret > 0) {
+ tcp_output(sototcpcb(so));
+ }
+ }
- /*
- * Check for URG data
- * This will soread as well, so no need to
- * test for G_IO_IN below if this succeeds
- */
- if (revents & G_IO_PRI) {
- sorecvoob(so);
- }
- /*
- * Check sockets for reading
- */
- else if (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR)) {
- /*
- * Check for incoming connections
- */
- if (so->so_state & SS_FACCEPTCONN) {
- tcp_connect(so);
+ /*
+ * Check sockets for writing
+ */
+ if (!(so->so_state & SS_NOFDREF) &&
+ (revents & (G_IO_OUT | G_IO_ERR))) {
+ /*
+ * Check for non-blocking, still-connecting sockets
+ */
+ if (so->so_state & SS_ISFCONNECTING) {
+ /* Connected */
+ so->so_state &= ~SS_ISFCONNECTING;
+
+ ret = send(so->s, (const void *) &ret, 0, 0);
+ if (ret < 0) {
+ /* XXXXX Must fix, zero bytes is a NOP */
+ if (errno == EAGAIN || errno == EWOULDBLOCK ||
+ errno == EINPROGRESS || errno == ENOTCONN) {
continue;
- } /* else */
- ret = soread(so);
-
- /* Output it if we read something */
- if (ret > 0) {
- tcp_output(sototcpcb(so));
}
- }
- /*
- * Check sockets for writing
- */
- if (!(so->so_state & SS_NOFDREF) &&
- (revents & (G_IO_OUT | G_IO_ERR))) {
- /*
- * Check for non-blocking, still-connecting sockets
- */
- if (so->so_state & SS_ISFCONNECTING) {
- /* Connected */
- so->so_state &= ~SS_ISFCONNECTING;
-
- ret = send(so->s, (const void *) &ret, 0, 0);
- if (ret < 0) {
- /* XXXXX Must fix, zero bytes is a NOP */
- if (errno == EAGAIN || errno == EWOULDBLOCK ||
- errno == EINPROGRESS || errno == ENOTCONN) {
- continue;
- }
-
- /* else failed */
- so->so_state &= SS_PERSISTENT_MASK;
- so->so_state |= SS_NOFDREF;
- }
- /* else so->so_state &= ~SS_ISFCONNECTING; */
-
- /*
- * Continue tcp_input
- */
- tcp_input((struct mbuf *)NULL, sizeof(struct ip), so);
- /* continue; */
- } else {
- ret = sowrite(so);
- }
- /*
- * XXXXX If we wrote something (a lot), there
- * could be a need for a window update.
- * In the worst case, the remote will send
- * a window probe to get things going again
- */
+ /* else failed */
+ so->so_state &= SS_PERSISTENT_MASK;
+ so->so_state |= SS_NOFDREF;
}
+ /* else so->so_state &= ~SS_ISFCONNECTING; */
/*
- * Probe a still-connecting, non-blocking socket
- * to check if it's still alive
+ * Continue tcp_input
*/
-#ifdef PROBE_CONN
- if (so->so_state & SS_ISFCONNECTING) {
- ret = qemu_recv(so->s, &ret, 0, 0);
-
- if (ret < 0) {
- /* XXX */
- if (errno == EAGAIN || errno == EWOULDBLOCK ||
- errno == EINPROGRESS || errno == ENOTCONN) {
- continue; /* Still connecting, continue */
- }
-
- /* else failed */
- so->so_state &= SS_PERSISTENT_MASK;
- so->so_state |= SS_NOFDREF;
-
- /* tcp_input will take care of it */
- } else {
- ret = send(so->s, &ret, 0, 0);
- if (ret < 0) {
- /* XXX */
- if (errno == EAGAIN || errno == EWOULDBLOCK ||
- errno == EINPROGRESS || errno == ENOTCONN) {
- continue;
- }
- /* else failed */
- so->so_state &= SS_PERSISTENT_MASK;
- so->so_state |= SS_NOFDREF;
- } else {
- so->so_state &= ~SS_ISFCONNECTING;
- }
-
- }
- tcp_input((struct mbuf *)NULL, sizeof(struct ip), so);
- } /* SS_ISFCONNECTING */
-#endif
- }
-
- /*
- * Now UDP sockets.
- * Incoming packets are sent straight away, they're not buffered.
- * Incoming UDP data isn't buffered either.
- */
- for (so = slirp->udb.so_next; so != &slirp->udb;
- so = so_next) {
- int revents;
-
- so_next = so->so_next;
-
- revents = 0;
- if (so->pollfds_idx != -1) {
- revents = g_array_index(pollfds, GPollFD,
- so->pollfds_idx).revents;
- }
-
- if (so->s != -1 &&
- (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR))) {
- sorecvfrom(so);
- }
+ tcp_input((struct mbuf *)NULL, sizeof(struct ip), so);
+ /* continue; */
+ } else {
+ ret = sowrite(so);
}
-
/*
- * Check incoming ICMP relies.
+ * XXXXX If we wrote something (a lot), there
+ * could be a need for a window update.
+ * In the worst case, the remote will send
+ * a window probe to get things going again
*/
- for (so = slirp->icmp.so_next; so != &slirp->icmp;
- so = so_next) {
- int revents;
-
- so_next = so->so_next;
-
- revents = 0;
- if (so->pollfds_idx != -1) {
- revents = g_array_index(pollfds, GPollFD,
- so->pollfds_idx).revents;
- }
-
- if (so->s != -1 &&
- (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR))) {
- icmp_receive(so);
- }
- }
}
- if_start(slirp);
+ /*
+ * Probe a still-connecting, non-blocking socket
+ * to check if it's still alive
+ */
+#ifdef PROBE_CONN
+ if (so->so_state & SS_ISFCONNECTING) {
+ ret = qemu_recv(so->s, &ret, 0, 0);
+
+ if (ret < 0) {
+ /* XXX */
+ if (errno == EAGAIN || errno == EWOULDBLOCK ||
+ errno == EINPROGRESS || errno == ENOTCONN) {
+ continue; /* Still connecting, continue */
+ }
+
+ /* else failed */
+ so->so_state &= SS_PERSISTENT_MASK;
+ so->so_state |= SS_NOFDREF;
+
+ /* tcp_input will take care of it */
+ } else {
+ ret = send(so->s, &ret, 0, 0);
+ if (ret < 0) {
+ /* XXX */
+ if (errno == EAGAIN || errno == EWOULDBLOCK ||
+ errno == EINPROGRESS || errno == ENOTCONN) {
+ continue;
+ }
+ /* else failed */
+ so->so_state &= SS_PERSISTENT_MASK;
+ so->so_state |= SS_NOFDREF;
+ } else {
+ so->so_state &= ~SS_ISFCONNECTING;
+ }
+
+ }
+ tcp_input((struct mbuf *)NULL, sizeof(struct ip), so);
+ } /* SS_ISFCONNECTING */
+#endif
+ }
+
+ /*
+ * Now UDP sockets.
+ * Incoming packets are sent straight away, they're not buffered.
+ * Incoming UDP data isn't buffered either.
+ */
+ for (so = slirp->udb.so_next; so != &slirp->udb;
+ so = so_next) {
+ int revents;
+
+ so_next = so->so_next;
+
+ revents = 0;
+ if (so->pollfd) {
+ revents = so->pollfd->revents;
+ }
+
+ if (so->s != -1 &&
+ (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR))) {
+ sorecvfrom(so);
+ }
+ }
+
+ /*
+ * Check incoming ICMP relies.
+ */
+ for (so = slirp->icmp.so_next; so != &slirp->icmp;
+ so = so_next) {
+ int revents;
+
+ so_next = so->so_next;
+
+ revents = 0;
+ if (so->pollfd) {
+ revents = so->pollfd->revents;
+ }
+
+ if (so->s != -1 &&
+ (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR))) {
+ icmp_receive(so);
+ }
}
+
+ if_start(slirp);
+ return true;
}
static void arp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
diff --git a/slirp/socket.c b/slirp/socket.c
index bb639ae..058d2e3 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -52,6 +52,7 @@ socreate(Slirp *slirp)
so->s = -1;
so->slirp = slirp;
so->pollfds_idx = -1;
+ so->pollfd = slirp_gsource_get_gfd(slirp->opaque, so->s);
}
return(so);
}
@@ -64,6 +65,7 @@ sofree(struct socket *so)
{
Slirp *slirp = so->slirp;
+ slirp_gsource_close_gfd(slirp->opaque, so->pollfd);
if (so->so_emu==EMU_RSH && so->extra) {
sofree(so->extra);
so->extra=NULL;
diff --git a/slirp/socket.h b/slirp/socket.h
index 57e0407..522c5f0 100644
--- a/slirp/socket.h
+++ b/slirp/socket.h
@@ -21,6 +21,7 @@ struct socket {
int s; /* The actual socket */
int pollfds_idx; /* GPollFD GArray index */
+ GPollFD *pollfd;
Slirp *slirp; /* managing slirp instance */
diff --git a/stubs/slirp.c b/stubs/slirp.c
index f1fc833..c343364 100644
--- a/stubs/slirp.c
+++ b/stubs/slirp.c
@@ -5,11 +5,3 @@ void slirp_update_timeout(uint32_t *timeout)
{
}
-void slirp_pollfds_fill(GArray *pollfds)
-{
-}
-
-void slirp_pollfds_poll(GArray *pollfds, int select_error)
-{
-}
-
--
1.7.4.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* [Qemu-devel] [RFC PATCH v4 14/15] slirp: handle race condition
2013-04-17 8:39 [Qemu-devel] [RFC PATCH v4 00/15] port network layer onto glib Liu Ping Fan
` (12 preceding siblings ...)
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 13/15] slirp: make slirp event dispatch based on slirp instance, not global Liu Ping Fan
@ 2013-04-17 8:39 ` Liu Ping Fan
2013-04-18 7:13 ` Jan Kiszka
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 15/15] slirp: use lock to protect the slirp_instances Liu Ping Fan
14 siblings, 1 reply; 35+ messages in thread
From: Liu Ping Fan @ 2013-04-17 8:39 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, Paolo Bonzini, Stefan Hajnoczi, Anthony Liguori,
Jan Kiszka
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
Slirp and its peer can run on different context at the same time.
Using lock to protect
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
slirp/slirp.c | 16 ++++++++++++++--
slirp/slirp.h | 3 +++
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 883b7bd..6bfcc67 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -206,6 +206,7 @@ Slirp *slirp_init(int restricted, struct in_addr vnetwork,
slirp_init_once();
+ qemu_mutex_init(&slirp->lock);
slirp->restricted = restricted;
if_init(slirp);
@@ -248,6 +249,7 @@ void slirp_cleanup(Slirp *slirp)
ip_cleanup(slirp);
m_cleanup(slirp);
+ qemu_mutex_destroy(&slirp->lock);
g_free(slirp->vdnssearch);
g_free(slirp->tftp_prefix);
@@ -411,6 +413,7 @@ gboolean slirp_handler(gpointer data)
struct socket *so, *so_next;
int ret;
+ qemu_mutex_lock(&slirp->lock);
/*
* See if anything has timed out
*/
@@ -594,6 +597,7 @@ gboolean slirp_handler(gpointer data)
}
if_start(slirp);
+ qemu_mutex_unlock(&slirp->lock);
return true;
}
@@ -665,6 +669,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
return;
proto = ntohs(*(uint16_t *)(pkt + 12));
+ qemu_mutex_lock(&slirp->lock);
switch(proto) {
case ETH_P_ARP:
arp_input(slirp, pkt, pkt_len);
@@ -688,6 +693,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
default:
break;
}
+ qemu_mutex_unlock(&slirp->lock);
}
/* Output the IP packet to the ethernet device. Returns 0 if the packet must be
@@ -860,15 +866,21 @@ void slirp_socket_recv(Slirp *slirp, struct in_addr guest_addr, int guest_port,
const uint8_t *buf, int size)
{
int ret;
- struct socket *so = slirp_find_ctl_socket(slirp, guest_addr, guest_port);
+ struct socket *so;
+
+ qemu_mutex_lock(&slirp->lock);
+ so = slirp_find_ctl_socket(slirp, guest_addr, guest_port);
- if (!so)
+ if (!so) {
+ qemu_mutex_unlock(&slirp->lock);
return;
+ }
ret = soreadbuf(so, (const char *)buf, size);
if (ret > 0)
tcp_output(sototcpcb(so));
+ qemu_mutex_unlock(&slirp->lock);
}
static void slirp_tcp_save(QEMUFile *f, struct tcpcb *tp)
diff --git a/slirp/slirp.h b/slirp/slirp.h
index 008360e..7ab0c70 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -135,6 +135,7 @@ void free(void *ptr);
#include "qemu/queue.h"
#include "qemu/sockets.h"
+#include "qemu/thread.h"
#include "libslirp.h"
#include "ip.h"
@@ -207,6 +208,8 @@ struct Slirp {
u_int last_slowtimo;
int do_slowtimo;
+ /* lock to protect slirp running both on frontend or SlirpState context */
+ QemuMutex lock;
/* virtual network configuration */
struct in_addr vnetwork_addr;
struct in_addr vnetwork_mask;
--
1.7.4.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [RFC PATCH v4 14/15] slirp: handle race condition
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 14/15] slirp: handle race condition Liu Ping Fan
@ 2013-04-18 7:13 ` Jan Kiszka
2013-04-19 0:18 ` liu ping fan
0 siblings, 1 reply; 35+ messages in thread
From: Jan Kiszka @ 2013-04-18 7:13 UTC (permalink / raw)
To: Liu Ping Fan
Cc: Paolo Bonzini, Stefan Hajnoczi, qemu-devel@nongnu.org,
Anthony Liguori, mdroth
On 2013-04-17 10:39, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>
> Slirp and its peer can run on different context at the same time.
> Using lock to protect
What are the usage rules for this lock, what precisely is it protecting?
Is it ensured that we do not take the BQL while holding this one?
Jan
>
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
> slirp/slirp.c | 16 ++++++++++++++--
> slirp/slirp.h | 3 +++
> 2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 883b7bd..6bfcc67 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -206,6 +206,7 @@ Slirp *slirp_init(int restricted, struct in_addr vnetwork,
>
> slirp_init_once();
>
> + qemu_mutex_init(&slirp->lock);
> slirp->restricted = restricted;
>
> if_init(slirp);
> @@ -248,6 +249,7 @@ void slirp_cleanup(Slirp *slirp)
>
> ip_cleanup(slirp);
> m_cleanup(slirp);
> + qemu_mutex_destroy(&slirp->lock);
>
> g_free(slirp->vdnssearch);
> g_free(slirp->tftp_prefix);
> @@ -411,6 +413,7 @@ gboolean slirp_handler(gpointer data)
> struct socket *so, *so_next;
> int ret;
>
> + qemu_mutex_lock(&slirp->lock);
> /*
> * See if anything has timed out
> */
> @@ -594,6 +597,7 @@ gboolean slirp_handler(gpointer data)
> }
>
> if_start(slirp);
> + qemu_mutex_unlock(&slirp->lock);
> return true;
> }
>
> @@ -665,6 +669,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
> return;
>
> proto = ntohs(*(uint16_t *)(pkt + 12));
> + qemu_mutex_lock(&slirp->lock);
> switch(proto) {
> case ETH_P_ARP:
> arp_input(slirp, pkt, pkt_len);
> @@ -688,6 +693,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
> default:
> break;
> }
> + qemu_mutex_unlock(&slirp->lock);
> }
>
> /* Output the IP packet to the ethernet device. Returns 0 if the packet must be
> @@ -860,15 +866,21 @@ void slirp_socket_recv(Slirp *slirp, struct in_addr guest_addr, int guest_port,
> const uint8_t *buf, int size)
> {
> int ret;
> - struct socket *so = slirp_find_ctl_socket(slirp, guest_addr, guest_port);
> + struct socket *so;
> +
> + qemu_mutex_lock(&slirp->lock);
> + so = slirp_find_ctl_socket(slirp, guest_addr, guest_port);
>
> - if (!so)
> + if (!so) {
> + qemu_mutex_unlock(&slirp->lock);
> return;
> + }
>
> ret = soreadbuf(so, (const char *)buf, size);
>
> if (ret > 0)
> tcp_output(sototcpcb(so));
> + qemu_mutex_unlock(&slirp->lock);
> }
>
> static void slirp_tcp_save(QEMUFile *f, struct tcpcb *tp)
> diff --git a/slirp/slirp.h b/slirp/slirp.h
> index 008360e..7ab0c70 100644
> --- a/slirp/slirp.h
> +++ b/slirp/slirp.h
> @@ -135,6 +135,7 @@ void free(void *ptr);
>
> #include "qemu/queue.h"
> #include "qemu/sockets.h"
> +#include "qemu/thread.h"
>
> #include "libslirp.h"
> #include "ip.h"
> @@ -207,6 +208,8 @@ struct Slirp {
> u_int last_slowtimo;
> int do_slowtimo;
>
> + /* lock to protect slirp running both on frontend or SlirpState context */
> + QemuMutex lock;
> /* virtual network configuration */
> struct in_addr vnetwork_addr;
> struct in_addr vnetwork_mask;
>
--
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [RFC PATCH v4 14/15] slirp: handle race condition
2013-04-18 7:13 ` Jan Kiszka
@ 2013-04-19 0:18 ` liu ping fan
2013-04-19 8:21 ` Jan Kiszka
0 siblings, 1 reply; 35+ messages in thread
From: liu ping fan @ 2013-04-19 0:18 UTC (permalink / raw)
To: Jan Kiszka
Cc: Paolo Bonzini, Stefan Hajnoczi, qemu-devel@nongnu.org,
Anthony Liguori, mdroth
On Thu, Apr 18, 2013 at 3:13 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2013-04-17 10:39, Liu Ping Fan wrote:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> Slirp and its peer can run on different context at the same time.
>> Using lock to protect
>
> What are the usage rules for this lock, what precisely is it protecting?
> Is it ensured that we do not take the BQL while holding this one?
>
It protect the slirp state, since slirp can be touched by slrip_input
called by frontend(ex, e1000), also it can be touched by its event
handler. With this lock, we do not need BQL
Regards,
Pingfan
> Jan
>
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>> slirp/slirp.c | 16 ++++++++++++++--
>> slirp/slirp.h | 3 +++
>> 2 files changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/slirp/slirp.c b/slirp/slirp.c
>> index 883b7bd..6bfcc67 100644
>> --- a/slirp/slirp.c
>> +++ b/slirp/slirp.c
>> @@ -206,6 +206,7 @@ Slirp *slirp_init(int restricted, struct in_addr vnetwork,
>>
>> slirp_init_once();
>>
>> + qemu_mutex_init(&slirp->lock);
>> slirp->restricted = restricted;
>>
>> if_init(slirp);
>> @@ -248,6 +249,7 @@ void slirp_cleanup(Slirp *slirp)
>>
>> ip_cleanup(slirp);
>> m_cleanup(slirp);
>> + qemu_mutex_destroy(&slirp->lock);
>>
>> g_free(slirp->vdnssearch);
>> g_free(slirp->tftp_prefix);
>> @@ -411,6 +413,7 @@ gboolean slirp_handler(gpointer data)
>> struct socket *so, *so_next;
>> int ret;
>>
>> + qemu_mutex_lock(&slirp->lock);
>> /*
>> * See if anything has timed out
>> */
>> @@ -594,6 +597,7 @@ gboolean slirp_handler(gpointer data)
>> }
>>
>> if_start(slirp);
>> + qemu_mutex_unlock(&slirp->lock);
>> return true;
>> }
>>
>> @@ -665,6 +669,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
>> return;
>>
>> proto = ntohs(*(uint16_t *)(pkt + 12));
>> + qemu_mutex_lock(&slirp->lock);
>> switch(proto) {
>> case ETH_P_ARP:
>> arp_input(slirp, pkt, pkt_len);
>> @@ -688,6 +693,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
>> default:
>> break;
>> }
>> + qemu_mutex_unlock(&slirp->lock);
>> }
>>
>> /* Output the IP packet to the ethernet device. Returns 0 if the packet must be
>> @@ -860,15 +866,21 @@ void slirp_socket_recv(Slirp *slirp, struct in_addr guest_addr, int guest_port,
>> const uint8_t *buf, int size)
>> {
>> int ret;
>> - struct socket *so = slirp_find_ctl_socket(slirp, guest_addr, guest_port);
>> + struct socket *so;
>> +
>> + qemu_mutex_lock(&slirp->lock);
>> + so = slirp_find_ctl_socket(slirp, guest_addr, guest_port);
>>
>> - if (!so)
>> + if (!so) {
>> + qemu_mutex_unlock(&slirp->lock);
>> return;
>> + }
>>
>> ret = soreadbuf(so, (const char *)buf, size);
>>
>> if (ret > 0)
>> tcp_output(sototcpcb(so));
>> + qemu_mutex_unlock(&slirp->lock);
>> }
>>
>> static void slirp_tcp_save(QEMUFile *f, struct tcpcb *tp)
>> diff --git a/slirp/slirp.h b/slirp/slirp.h
>> index 008360e..7ab0c70 100644
>> --- a/slirp/slirp.h
>> +++ b/slirp/slirp.h
>> @@ -135,6 +135,7 @@ void free(void *ptr);
>>
>> #include "qemu/queue.h"
>> #include "qemu/sockets.h"
>> +#include "qemu/thread.h"
>>
>> #include "libslirp.h"
>> #include "ip.h"
>> @@ -207,6 +208,8 @@ struct Slirp {
>> u_int last_slowtimo;
>> int do_slowtimo;
>>
>> + /* lock to protect slirp running both on frontend or SlirpState context */
>> + QemuMutex lock;
>> /* virtual network configuration */
>> struct in_addr vnetwork_addr;
>> struct in_addr vnetwork_mask;
>>
>
> --
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [RFC PATCH v4 14/15] slirp: handle race condition
2013-04-19 0:18 ` liu ping fan
@ 2013-04-19 8:21 ` Jan Kiszka
2013-04-22 5:55 ` liu ping fan
0 siblings, 1 reply; 35+ messages in thread
From: Jan Kiszka @ 2013-04-19 8:21 UTC (permalink / raw)
To: liu ping fan
Cc: Paolo Bonzini, Stefan Hajnoczi, qemu-devel@nongnu.org,
Anthony Liguori, mdroth
On 2013-04-19 02:18, liu ping fan wrote:
> On Thu, Apr 18, 2013 at 3:13 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2013-04-17 10:39, Liu Ping Fan wrote:
>>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>
>>> Slirp and its peer can run on different context at the same time.
>>> Using lock to protect
>>
>> What are the usage rules for this lock, what precisely is it protecting?
>> Is it ensured that we do not take the BQL while holding this one?
>>
> It protect the slirp state, since slirp can be touched by slrip_input
> called by frontend(ex, e1000), also it can be touched by its event
> handler. With this lock, we do not need BQL
...but the BQL will, at least initially, remain to be everywhere. Every
non-converted device model will hold it while calling into Slirp. So we
have the ordering "BQL before Slirp lock" already. And we must ensure
that there is no "BQL after Slirp lock". Can you guarantee this?
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v4 14/15] slirp: handle race condition
2013-04-19 8:21 ` Jan Kiszka
@ 2013-04-22 5:55 ` liu ping fan
2013-04-23 7:20 ` liu ping fan
0 siblings, 1 reply; 35+ messages in thread
From: liu ping fan @ 2013-04-22 5:55 UTC (permalink / raw)
To: Jan Kiszka
Cc: Paolo Bonzini, Stefan Hajnoczi, qemu-devel@nongnu.org,
Anthony Liguori, mdroth
On Fri, Apr 19, 2013 at 4:21 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2013-04-19 02:18, liu ping fan wrote:
>> On Thu, Apr 18, 2013 at 3:13 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On 2013-04-17 10:39, Liu Ping Fan wrote:
>>>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>>
>>>> Slirp and its peer can run on different context at the same time.
>>>> Using lock to protect
>>>
>>> What are the usage rules for this lock, what precisely is it protecting?
>>> Is it ensured that we do not take the BQL while holding this one?
>>>
>> It protect the slirp state, since slirp can be touched by slrip_input
>> called by frontend(ex, e1000), also it can be touched by its event
>> handler. With this lock, we do not need BQL
>
> ...but the BQL will, at least initially, remain to be everywhere. Every
> non-converted device model will hold it while calling into Slirp. So we
> have the ordering "BQL before Slirp lock" already. And we must ensure
> that there is no "BQL after Slirp lock". Can you guarantee this?
>
Oh, yes, there is a potential ABBA lock problem. Especially for slirp
backend, the NetClientState's receive() can be nested, the scene:
e1000 send packet -> net_slirp_receive() ...->arp_input()..->
->slirp_output() ->e1000_receive()
Although currently, there is no extra lock required by
e1000_receive(), but the nested model does cause the potential of ABBA
lock problem, when beginning to covert the frontend(e1000).
What about introducing slirp->lockstate and slirp->lock only used to
protect it. So the users of slirp will protect agaist each other by
slirp->lockstate, meanwhile, we can advoid the potential dead lock.
Regards,
Pingfan
> Jan
>
> --
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v4 14/15] slirp: handle race condition
2013-04-22 5:55 ` liu ping fan
@ 2013-04-23 7:20 ` liu ping fan
0 siblings, 0 replies; 35+ messages in thread
From: liu ping fan @ 2013-04-23 7:20 UTC (permalink / raw)
To: Jan Kiszka
Cc: Paolo Bonzini, Stefan Hajnoczi, qemu-devel@nongnu.org,
Anthony Liguori, mdroth
On Mon, Apr 22, 2013 at 1:55 PM, liu ping fan <qemulist@gmail.com> wrote:
> On Fri, Apr 19, 2013 at 4:21 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2013-04-19 02:18, liu ping fan wrote:
>>> On Thu, Apr 18, 2013 at 3:13 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> On 2013-04-17 10:39, Liu Ping Fan wrote:
>>>>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>>>
>>>>> Slirp and its peer can run on different context at the same time.
>>>>> Using lock to protect
>>>>
>>>> What are the usage rules for this lock, what precisely is it protecting?
>>>> Is it ensured that we do not take the BQL while holding this one?
>>>>
>>> It protect the slirp state, since slirp can be touched by slrip_input
>>> called by frontend(ex, e1000), also it can be touched by its event
>>> handler. With this lock, we do not need BQL
>>
>> ...but the BQL will, at least initially, remain to be everywhere. Every
>> non-converted device model will hold it while calling into Slirp. So we
>> have the ordering "BQL before Slirp lock" already. And we must ensure
>> that there is no "BQL after Slirp lock". Can you guarantee this?
>>
> Oh, yes, there is a potential ABBA lock problem. Especially for slirp
> backend, the NetClientState's receive() can be nested, the scene:
> e1000 send packet -> net_slirp_receive() ...->arp_input()..->
> ->slirp_output() ->e1000_receive()
> Although currently, there is no extra lock required by
> e1000_receive(), but the nested model does cause the potential of ABBA
> lock problem, when beginning to covert the frontend(e1000).
> What about introducing slirp->lockstate and slirp->lock only used to
> protect it. So the users of slirp will protect agaist each other by
> slirp->lockstate, meanwhile, we can advoid the potential dead lock.
>
Drop the bad idea. The slirp->lock should be dropped before calling
out direction method. And if_start() is need to fix
> Regards,
> Pingfan
>> Jan
>>
>> --
>> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
>> Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [RFC PATCH v4 15/15] slirp: use lock to protect the slirp_instances
2013-04-17 8:39 [Qemu-devel] [RFC PATCH v4 00/15] port network layer onto glib Liu Ping Fan
` (13 preceding siblings ...)
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 14/15] slirp: handle race condition Liu Ping Fan
@ 2013-04-17 8:39 ` Liu Ping Fan
2013-04-18 7:20 ` Jan Kiszka
14 siblings, 1 reply; 35+ messages in thread
From: Liu Ping Fan @ 2013-04-17 8:39 UTC (permalink / raw)
To: qemu-devel
Cc: mdroth, Paolo Bonzini, Stefan Hajnoczi, Anthony Liguori,
Jan Kiszka
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
slirps will run on dedicated thread, so need to protect the global
list.
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
include/qemu/module.h | 2 ++
slirp/slirp.c | 20 ++++++++++++++++++++
2 files changed, 22 insertions(+), 0 deletions(-)
diff --git a/include/qemu/module.h b/include/qemu/module.h
index c4ccd57..2720943 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -22,6 +22,7 @@ static void __attribute__((constructor)) do_qemu_init_ ## function(void) { \
typedef enum {
MODULE_INIT_BLOCK,
+ MODULE_INIT_SLIRP,
MODULE_INIT_MACHINE,
MODULE_INIT_QAPI,
MODULE_INIT_QOM,
@@ -29,6 +30,7 @@ typedef enum {
} module_init_type;
#define block_init(function) module_init(function, MODULE_INIT_BLOCK)
+#define slirplayer_init(function) module_init(function, MODULE_INIT_SLIRP)
#define machine_init(function) module_init(function, MODULE_INIT_MACHINE)
#define qapi_init(function) module_init(function, MODULE_INIT_QAPI)
#define type_init(function) module_init(function, MODULE_INIT_QOM)
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 6bfcc67..4cbf04d 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -42,6 +42,7 @@ static const uint8_t zero_ethaddr[ETH_ALEN] = { 0, 0, 0, 0, 0, 0 };
u_int curtime;
+static QemuMutex slirp_instances_lock;
static QTAILQ_HEAD(slirp_instances, Slirp) slirp_instances =
QTAILQ_HEAD_INITIALIZER(slirp_instances);
@@ -236,14 +237,18 @@ Slirp *slirp_init(int restricted, struct in_addr vnetwork,
register_savevm(NULL, "slirp", 0, 3,
slirp_state_save, slirp_state_load, slirp);
+ qemu_mutex_lock(&slirp_instances_lock);
QTAILQ_INSERT_TAIL(&slirp_instances, slirp, entry);
+ qemu_mutex_unlock(&slirp_instances_lock);
return slirp;
}
void slirp_cleanup(Slirp *slirp)
{
+ qemu_mutex_lock(&slirp_instances_lock);
QTAILQ_REMOVE(&slirp_instances, slirp, entry);
+ qemu_mutex_unlock(&slirp_instances_lock);
unregister_savevm(NULL, "slirp", slirp);
@@ -262,9 +267,12 @@ void slirp_cleanup(Slirp *slirp)
void slirp_update_timeout(uint32_t *timeout)
{
+ qemu_mutex_lock(&slirp_instances_lock);
if (!QTAILQ_EMPTY(&slirp_instances)) {
*timeout = MIN(1000, *timeout);
}
+ qemu_mutex_unlock(&slirp_instances_lock);
+
curtime = qemu_get_clock_ms(rt_clock);
}
@@ -1140,3 +1148,15 @@ static int slirp_state_load(QEMUFile *f, void *opaque, int version_id)
return 0;
}
+
+static void slirplayer_cleanup(void)
+{
+ qemu_mutex_destroy(&slirp_instances_lock);
+}
+
+static void slirplayer_bootup(void)
+{
+ qemu_mutex_init(&slirp_instances_lock);
+ atexit(&slirplayer_cleanup);
+}
+slirplayer_init(slirplayer_bootup)
--
1.7.4.4
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [RFC PATCH v4 15/15] slirp: use lock to protect the slirp_instances
2013-04-17 8:39 ` [Qemu-devel] [RFC PATCH v4 15/15] slirp: use lock to protect the slirp_instances Liu Ping Fan
@ 2013-04-18 7:20 ` Jan Kiszka
2013-04-18 14:16 ` Paolo Bonzini
0 siblings, 1 reply; 35+ messages in thread
From: Jan Kiszka @ 2013-04-18 7:20 UTC (permalink / raw)
To: Liu Ping Fan
Cc: Paolo Bonzini, Stefan Hajnoczi, qemu-devel@nongnu.org,
Anthony Liguori, mdroth
On 2013-04-17 10:39, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>
> slirps will run on dedicated thread, so need to protect the global
> list.
>
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
> include/qemu/module.h | 2 ++
> slirp/slirp.c | 20 ++++++++++++++++++++
> 2 files changed, 22 insertions(+), 0 deletions(-)
>
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index c4ccd57..2720943 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -22,6 +22,7 @@ static void __attribute__((constructor)) do_qemu_init_ ## function(void) { \
>
> typedef enum {
> MODULE_INIT_BLOCK,
> + MODULE_INIT_SLIRP,
> MODULE_INIT_MACHINE,
> MODULE_INIT_QAPI,
> MODULE_INIT_QOM,
> @@ -29,6 +30,7 @@ typedef enum {
> } module_init_type;
>
> #define block_init(function) module_init(function, MODULE_INIT_BLOCK)
> +#define slirplayer_init(function) module_init(function, MODULE_INIT_SLIRP)
> #define machine_init(function) module_init(function, MODULE_INIT_MACHINE)
> #define qapi_init(function) module_init(function, MODULE_INIT_QAPI)
> #define type_init(function) module_init(function, MODULE_INIT_QOM)
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 6bfcc67..4cbf04d 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -42,6 +42,7 @@ static const uint8_t zero_ethaddr[ETH_ALEN] = { 0, 0, 0, 0, 0, 0 };
>
> u_int curtime;
>
> +static QemuMutex slirp_instances_lock;
> static QTAILQ_HEAD(slirp_instances, Slirp) slirp_instances =
> QTAILQ_HEAD_INITIALIZER(slirp_instances);
>
> @@ -236,14 +237,18 @@ Slirp *slirp_init(int restricted, struct in_addr vnetwork,
> register_savevm(NULL, "slirp", 0, 3,
> slirp_state_save, slirp_state_load, slirp);
>
> + qemu_mutex_lock(&slirp_instances_lock);
> QTAILQ_INSERT_TAIL(&slirp_instances, slirp, entry);
> + qemu_mutex_unlock(&slirp_instances_lock);
>
> return slirp;
> }
>
> void slirp_cleanup(Slirp *slirp)
> {
> + qemu_mutex_lock(&slirp_instances_lock);
> QTAILQ_REMOVE(&slirp_instances, slirp, entry);
> + qemu_mutex_unlock(&slirp_instances_lock);
>
> unregister_savevm(NULL, "slirp", slirp);
>
> @@ -262,9 +267,12 @@ void slirp_cleanup(Slirp *slirp)
>
> void slirp_update_timeout(uint32_t *timeout)
> {
> + qemu_mutex_lock(&slirp_instances_lock);
> if (!QTAILQ_EMPTY(&slirp_instances)) {
> *timeout = MIN(1000, *timeout);
> }
> + qemu_mutex_unlock(&slirp_instances_lock);
> +
> curtime = qemu_get_clock_ms(rt_clock);
> }
>
> @@ -1140,3 +1148,15 @@ static int slirp_state_load(QEMUFile *f, void *opaque, int version_id)
>
> return 0;
> }
> +
> +static void slirplayer_cleanup(void)
> +{
> + qemu_mutex_destroy(&slirp_instances_lock);
> +}
> +
> +static void slirplayer_bootup(void)
> +{
> + qemu_mutex_init(&slirp_instances_lock);
> + atexit(&slirplayer_cleanup);
> +}
> +slirplayer_init(slirplayer_bootup)
>
grep'ing for slirp_instances points to more spots that work with that
list (QTAILQ_FOREACH, QTAILQ_EMPTY, ...). So the same question here:
What are the usage rules? When do I _not_ need it when touching the list
of instances, and why?
Well, I started reading at the top, but there are more lock-adding
patches in this series. And the more locks we have, the higher the
probability of ABBA gets. Therefore, please document from the beginning
the lock order rules that shall prevent it (which may also be "never
take other locks while holding this one" or "never hold other locks when
taking this one").
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [Qemu-devel] [RFC PATCH v4 15/15] slirp: use lock to protect the slirp_instances
2013-04-18 7:20 ` Jan Kiszka
@ 2013-04-18 14:16 ` Paolo Bonzini
2013-04-19 6:13 ` liu ping fan
0 siblings, 1 reply; 35+ messages in thread
From: Paolo Bonzini @ 2013-04-18 14:16 UTC (permalink / raw)
To: Jan Kiszka
Cc: mdroth, Stefan Hajnoczi, Liu Ping Fan, Anthony Liguori,
qemu-devel
> grep'ing for slirp_instances points to more spots that work with that
> list (QTAILQ_FOREACH, QTAILQ_EMPTY, ...). So the same question here:
> What are the usage rules? When do I _not_ need it when touching the list
> of instances, and why?
>
> Well, I started reading at the top, but there are more lock-adding
> patches in this series. And the more locks we have, the higher the
> probability of ABBA gets. Therefore, please document from the beginning
> the lock order rules that shall prevent it (which may also be "never
> take other locks while holding this one" or "never hold other locks when
> taking this one").
Yeah, the only sane ordering rules should be "hold nothing or just
the BQL when taking this one". Everything else needs a very good
justification...
Paolo
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v4 15/15] slirp: use lock to protect the slirp_instances
2013-04-18 14:16 ` Paolo Bonzini
@ 2013-04-19 6:13 ` liu ping fan
0 siblings, 0 replies; 35+ messages in thread
From: liu ping fan @ 2013-04-19 6:13 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Jan Kiszka, Stefan Hajnoczi, qemu-devel, Anthony Liguori, mdroth
On Thu, Apr 18, 2013 at 10:16 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>> grep'ing for slirp_instances points to more spots that work with that
>> list (QTAILQ_FOREACH, QTAILQ_EMPTY, ...). So the same question here:
>> What are the usage rules? When do I _not_ need it when touching the list
>> of instances, and why?
>>
>> Well, I started reading at the top, but there are more lock-adding
>> patches in this series. And the more locks we have, the higher the
>> probability of ABBA gets. Therefore, please document from the beginning
>> the lock order rules that shall prevent it (which may also be "never
>> take other locks while holding this one" or "never hold other locks when
>> taking this one").
>
slrip->lock is used to protect the frontend and backend to touch
slirp's content at the same time. While slirp_instances_lock is used
to protected the list ops only. For example, when the SlirpState's
refcnt comes to zero(this can not happen with holding slirp->lock),
finalize is called, and finally, slirp_cleanup is called to remove
slrip from the list. The two locks have no overlap.
Regards, Pingfan
> Yeah, the only sane ordering rules should be "hold nothing or just
> the BQL when taking this one". Everything else needs a very good
> justification...
>
Ok, will notice.
Thanks, Pingfan
> Paolo
^ permalink raw reply [flat|nested] 35+ messages in thread