[Qemu-devel] [RFC PATCH v2 0/4] port network layer onto glib

[Qemu-devel] [RFC PATCH v2 0/4] port network layer onto glib

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

These series aim to make the whole network re-entrant, here only apply backend and frontend,
and for the netcore, separated patches have been sent out.  All of these will prepare us for
moving towards making network layer mutlit-thread.
Finally it would be omething like
   qemu -object io-thread,id=thread0 \
     -device virtio-net,rx[0]=thread0,tx[0]=thread0

The brief of the whole aim and plan is documented on
  http://wiki.qemu.org/Features/network_reentrant

The main issue is about GSource or AioContext,
  http://marc.info/?t=136315453300002&r=1&w=3
And I sumary the main points:
  disadvantage for current AioContext
   1st. need to define and expand interface for other fd events, while glib open this interface for user *
   2nd. need to add support for IOCanReadHandler, while gsource provide prepare, check method to allow more flexible control
   3rd. block layer's AioContext will block other AioContexts on the same thread.
   4th. need more document
 disadvantage for glib
   1st. if more than one fds on the same GSource, need re-implement something like aio_set_file_handler

Since I have successed to port frontend on glib, there is no obstale to use glib.


v1->v2:
  1.NetClientState can associate with up to 2 GSource, for virtio net, one for tx, one for rx, 
    so vq can run on different threads.
  2.make network front-end onto glib, currently virtio net dataplane


Liu Ping Fan (4):
  net: port tap onto glib
  net: resolve race of tap backend and its peer
  net: port hub onto glib
  net: port virtio net onto glib

 hw/qdev-properties-system.c |    1 +
 hw/virtio-net.c             |  165 +++++++++++++++++++++++++++++++++++++++++++
 hw/virtio.c                 |    6 ++
 hw/virtio.h                 |    2 +
 include/net/net.h           |   27 +++++++
 include/net/queue.h         |   14 ++++
 net/hub.c                   |   34 ++++++++-
 net/net.c                   |   97 +++++++++++++++++++++++++
 net/queue.c                 |    4 +-
 net/tap.c                   |   62 +++++++++++++---
 10 files changed, 397 insertions(+), 15 deletions(-)

-- 
1.7.4.4

[Qemu-devel] [RFC PATCH v2 1/4] net: port tap onto glib

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

Bind each NetClientState with a GSource(ie,NetClientSource). Currently,
these GSource attached with default context, but in future, after
resolving the race between handlers and the interface exposed by NetClientInfo
and other re-entrant issue,  we can run NetClientState on different threads

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 include/net/net.h |   27 +++++++++++++++
 net/net.c         |   96 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 net/tap.c         |   57 +++++++++++++++++++++++++------
 3 files changed, 169 insertions(+), 11 deletions(-)

diff --git a/include/net/net.h b/include/net/net.h
index cb049a1..8fdb7eb 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,8 +56,25 @@ typedef struct NetClientInfo {
     NetCleanup *cleanup;
     LinkStatusChanged *link_status_changed;
     NetPoll *poll;
+    NetClientBindCtx *bind_ctx;
 } NetClientInfo;
 
+typedef bool (*Pollable)(void *opaque);
+
+typedef struct NetClientSource {
+    GSource source;
+    GPollFD gfd;
+    GMainContext *ctx;
+
+    Pollable readable;
+    Pollable writeable;
+    /* status returned by pollable last time */
+    bool last_rd_sts;
+    bool last_wr_sts;
+
+    void *opaque;
+} NetClientSource;
+
 struct NetClientState {
     NetClientInfo *info;
     int link_down;
@@ -69,6 +87,10 @@ struct NetClientState {
     unsigned receive_disabled : 1;
     NetClientDestructor *destructor;
     unsigned int queue_index;
+    /* For virtio net, rx on [0], tx on [1], so the virtque
+     * can run on different threads.
+     */
+    NetClientSource * nsrc[2];
 };
 
 typedef struct NICState {
@@ -155,6 +177,9 @@ extern int default_net;
 extern const char *legacy_tftp_prefix;
 extern const char *legacy_bootp_filename;
 
+void net_init_gsource(NetClientState *nc, int idx, NetClientSource *nsrc,
+    int fd, int events, Pollable rd, Pollable wr, GSourceFunc f, void *opaque);
+
 int net_client_init(QemuOpts *opts, int is_netdev, Error **errp);
 int net_client_parse(QemuOptsList *opts_list, const char *str);
 int net_init_clients(void);
@@ -190,4 +215,6 @@ unsigned compute_mcast_idx(const uint8_t *ep);
     .offset     = vmstate_offset_macaddr(_state, _field),            \
 }
 
+extern GSourceFuncs net_gsource_funcs;
+
 #endif
diff --git a/net/net.c b/net/net.c
index f3d67f8..abe8fef 100644
--- a/net/net.c
+++ b/net/net.c
@@ -299,6 +299,12 @@ static void qemu_free_net_client(NetClientState *nc)
     }
     g_free(nc->name);
     g_free(nc->model);
+    if (nc->nsrc[0]) {
+        g_source_remove(g_source_get_id(&nc->nsrc[0]->source));
+    }
+    if (nc->nsrc[1]) {
+        g_source_remove(g_source_get_id(&nc->nsrc[1]->source));
+    }
     if (nc->destructor) {
         nc->destructor(nc);
     }
@@ -558,6 +564,96 @@ qemu_sendv_packet(NetClientState *nc, const struct iovec *iov, int iovcnt)
     return qemu_sendv_packet_async(nc, iov, iovcnt, NULL);
 }
 
+static gboolean prepare(GSource *src, gint *time)
+{
+    NetClientSource *nsrc = (NetClientSource *)src;
+    bool rd, wr, change = false;
+
+    if (!nsrc->readable && !nsrc->writeable) {
+        return false;
+    }
+    if (nsrc->readable) {
+        rd = nsrc->readable(nsrc->opaque);
+        if (nsrc->last_rd_sts != rd) {
+            nsrc->last_rd_sts = rd;
+            change = true;
+        }
+    }
+    if (nsrc->writeable) {
+        wr = nsrc->writeable(nsrc->opaque);
+        if (nsrc->last_wr_sts != wr) {
+            nsrc->last_wr_sts = wr;
+            change = true;
+        }
+    }
+    if (!change) {
+        return false;
+    }
+
+    g_source_remove_poll(src, &nsrc->gfd);
+    if (rd) {
+        nsrc->gfd.events |= G_IO_IN;
+    } else {
+        nsrc->gfd.events &= ~G_IO_IN;
+    }
+    if (wr) {
+        nsrc->gfd.events |= G_IO_OUT;
+    } else {
+        nsrc->gfd.events &= ~G_IO_OUT;
+    }
+    g_source_add_poll(src, &nsrc->gfd);
+    return false;
+}
+
+static gboolean check(GSource *src)
+{
+    NetClientSource *nsrc = (NetClientSource *)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;
+}
+
+GSourceFuncs net_gsource_funcs = {
+    prepare,
+    check,
+    dispatch,
+    NULL
+};
+
+void net_init_gsource(NetClientState *nc, int idx, NetClientSource *nsrc,
+    int fd, int events, Pollable rd, Pollable wr, GSourceFunc f, void *opaque)
+{
+    nsrc->gfd.fd = fd;
+    nsrc->gfd.events = events;
+    nsrc->opaque = opaque;
+    nsrc->readable = rd;
+    nsrc->writeable = wr;
+    nsrc->last_rd_sts = false;
+    nsrc->last_wr_sts = false;
+    if (idx == 0) {
+        nc->nsrc[0] = nsrc;
+    } else {
+        nc->nsrc[1] = nsrc;
+    }
+    /* add PollFD if it wont change, otherwise left for GSource prepare */
+    if (!rd && !wr) {
+        g_source_add_poll(&nsrc->source, &nsrc->gfd);
+    }
+    g_source_set_callback(&nsrc->source, f, nsrc, NULL);
+}
+
 NetClientState *qemu_find_netdev(const char *id)
 {
     NetClientState *nc;
diff --git a/net/tap.c b/net/tap.c
index daab350..0b663d1 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -70,25 +70,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 = (TAPState *)opaque;
+
+    if (s->enabled && s->read_poll &&
+        tap_can_send(s)) {
+        return true;
+    }
+    return false;
+}
+
+static bool writeable(void *opaque)
+{
+    TAPState *s = (TAPState *)opaque;
+
+    if (s->enabled && s->write_poll) {
+        return true;
+    }
+    return false;
+}
+
+static gboolean tap_handler(gpointer data)
+{
+    NetClientSource *nsrc = (NetClientSource *)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)
@@ -298,6 +321,7 @@ 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);
 }
@@ -309,6 +333,11 @@ int tap_get_fd(NetClientState *nc)
     return s->fd;
 }
 
+static void tap_bind_ctx(NetClientState *nc, GMainContext *ctx)
+{
+    g_source_attach(&nc->nsrc[0]->source, ctx);
+}
+
 /* fd support */
 
 static NetClientInfo net_tap_info = {
@@ -319,6 +348,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,13 +626,18 @@ static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
                             int vnet_hdr, int fd)
 {
     TAPState *s;
+    NetClientSource *nsrc;
 
     s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
     if (!s) {
         close(fd);
         return -1;
     }
-
+    nsrc = (NetClientSource *)g_source_new(&net_gsource_funcs,
+            sizeof(NetClientSource));
+    net_init_gsource(&s->nc, 0, nsrc, s->fd, G_IO_IN|G_IO_OUT,
+            readable, writeable, tap_handler, s);
+    s->nc.info->bind_ctx(&s->nc, NULL);
     if (tap_set_sndbuf(s->fd, tap) < 0) {
         return -1;
     }
@@ -843,8 +878,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 +896,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

[Qemu-devel] [RFC PATCH v2 2/4] net: resolve race of tap backend and its peer

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 net/tap.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index 0b663d1..401161c 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -321,9 +321,14 @@ 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);
+
+    if (!enable) {
+        /* need sync so vhost can take over polling */
+        g_source_remove_poll(&nc->nsrc->source, &nc->nsrc->gfd);
+    }
 }
 
 int tap_get_fd(NetClientState *nc)
-- 
1.7.4.4

[Qemu-devel] [RFC PATCH v2 3/4] net: port hub onto glib

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

Attach each hubport with a GSource. Currently the GSource is attached
to default main context, and the hub still run on iothread, but in
future, after making the whole layer thread-safe, we will admit ports
to run on different thread.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/qdev-properties-system.c |    1 +
 include/net/queue.h         |   14 ++++++++++++++
 net/hub.c                   |   34 ++++++++++++++++++++++++++++++++--
 net/net.c                   |    1 +
 net/queue.c                 |    4 ++--
 5 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/hw/qdev-properties-system.c b/hw/qdev-properties-system.c
index ce3af22..bb2448d 100644
--- a/hw/qdev-properties-system.c
+++ b/hw/qdev-properties-system.c
@@ -307,6 +307,7 @@ static void set_vlan(Object *obj, Visitor *v, void *opaque,
                   name, prop->info->name);
         return;
     }
+    hubport->info->bind_ctx(hubport, NULL);
     *ptr = hubport;
 }
 
diff --git a/include/net/queue.h b/include/net/queue.h
index fc02b33..f60e57f 100644
--- a/include/net/queue.h
+++ b/include/net/queue.h
@@ -38,6 +38,20 @@ NetQueue *qemu_new_net_queue(void *opaque);
 
 void qemu_del_net_queue(NetQueue *queue);
 
+void qemu_net_queue_append(NetQueue *queue,
+                                  NetClientState *sender,
+                                  unsigned flags,
+                                  const uint8_t *buf,
+                                  size_t size,
+                                  NetPacketSent *sent_cb);
+
+void qemu_net_queue_append_iov(NetQueue *queue,
+                                      NetClientState *sender,
+                                      unsigned flags,
+                                      const struct iovec *iov,
+                                      int iovcnt,
+                                      NetPacketSent *sent_cb);
+
 ssize_t qemu_net_queue_send(NetQueue *queue,
                             NetClientState *sender,
                             unsigned flags,
diff --git a/net/hub.c b/net/hub.c
index df32074..594fab7 100644
--- a/net/hub.c
+++ b/net/hub.c
@@ -31,6 +31,7 @@ typedef struct NetHubPort {
     QLIST_ENTRY(NetHubPort) next;
     NetHub *hub;
     int id;
+    EventNotifier e;
 } NetHubPort;
 
 struct NetHub {
@@ -52,7 +53,9 @@ static ssize_t net_hub_receive(NetHub *hub, NetHubPort *source_port,
             continue;
         }
 
-        qemu_send_packet(&port->nc, buf, len);
+        qemu_net_queue_append(port->nc.peer->send_queue, &port->nc,
+                        QEMU_NET_PACKET_FLAG_NONE, buf, len, NULL);
+        event_notifier_set(&port->e);
     }
     return len;
 }
@@ -68,7 +71,9 @@ static ssize_t net_hub_receive_iov(NetHub *hub, NetHubPort *source_port,
             continue;
         }
 
-        qemu_sendv_packet(&port->nc, iov, iovcnt);
+        qemu_net_queue_append_iov(port->nc.peer->send_queue, &port->nc,
+                        QEMU_NET_PACKET_FLAG_NONE, iov, iovcnt, NULL);
+        event_notifier_set(&port->e);
     }
     return len;
 }
@@ -129,6 +134,11 @@ static void net_hub_port_cleanup(NetClientState *nc)
     QLIST_REMOVE(port, next);
 }
 
+static void net_hub_port_bind(NetClientState *nc, GMainContext *ctx)
+{
+    g_source_attach(&nc->nsrc[0]->source, ctx);
+}
+
 static NetClientInfo net_hub_port_info = {
     .type = NET_CLIENT_OPTIONS_KIND_HUBPORT,
     .size = sizeof(NetHubPort),
@@ -136,14 +146,29 @@ static NetClientInfo net_hub_port_info = {
     .receive = net_hub_port_receive,
     .receive_iov = net_hub_port_receive_iov,
     .cleanup = net_hub_port_cleanup,
+    .bind_ctx = net_hub_port_bind,
 };
 
+static gboolean hub_port_handler(gpointer data)
+{
+    NetClientSource *nsrc = (NetClientSource *)data;
+    NetHubPort *port = (NetHubPort *)nsrc->opaque;
+
+    if (nsrc->gfd.revents & G_IO_IN) {
+        event_notifier_test_and_clear(&port->e);
+        qemu_net_queue_flush(port->nc.peer->send_queue);
+        return true;
+    }
+    return false;
+}
+
 static NetHubPort *net_hub_port_new(NetHub *hub, const char *name)
 {
     NetClientState *nc;
     NetHubPort *port;
     int id = hub->num_ports++;
     char default_name[128];
+    NetClientSource *nsrc;
 
     if (!name) {
         snprintf(default_name, sizeof(default_name),
@@ -155,6 +180,11 @@ static NetHubPort *net_hub_port_new(NetHub *hub, const char *name)
     port = DO_UPCAST(NetHubPort, nc, nc);
     port->id = id;
     port->hub = hub;
+    event_notifier_init(&port->e, 0);
+    nsrc = (NetClientSource *)g_source_new(&net_gsource_funcs,
+                sizeof(NetClientSource));
+    net_init_gsource(nc, 0, nsrc, event_notifier_get_fd(&port->e), G_IO_IN,
+                NULL, NULL, hub_port_handler, port);
 
     QLIST_INSERT_HEAD(&hub->ports, port, next);
 
diff --git a/net/net.c b/net/net.c
index abe8fef..b5caa39 100644
--- a/net/net.c
+++ b/net/net.c
@@ -877,6 +877,7 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)
             (opts->kind != NET_CLIENT_OPTIONS_KIND_NIC ||
              !opts->nic->has_netdev)) {
             peer = net_hub_add_port(u.net->has_vlan ? u.net->vlan : 0, NULL);
+            peer->info->bind_ctx(peer, NULL);
         }
 
         if (net_client_init_fun[opts->kind](opts, name, peer) < 0) {
diff --git a/net/queue.c b/net/queue.c
index 859d02a..67959f8 100644
--- a/net/queue.c
+++ b/net/queue.c
@@ -87,7 +87,7 @@ void qemu_del_net_queue(NetQueue *queue)
     g_free(queue);
 }
 
-static void qemu_net_queue_append(NetQueue *queue,
+void qemu_net_queue_append(NetQueue *queue,
                                   NetClientState *sender,
                                   unsigned flags,
                                   const uint8_t *buf,
@@ -110,7 +110,7 @@ static void qemu_net_queue_append(NetQueue *queue,
     QTAILQ_INSERT_TAIL(&queue->packets, packet, entry);
 }
 
-static void qemu_net_queue_append_iov(NetQueue *queue,
+void qemu_net_queue_append_iov(NetQueue *queue,
                                       NetClientState *sender,
                                       unsigned flags,
                                       const struct iovec *iov,
-- 
1.7.4.4

[Qemu-devel] [RFC PATCH v2 4/4] net: port virtio net onto glib

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/virtio-net.c |  165 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio.c     |    6 ++
 hw/virtio.h     |    2 +
 3 files changed, 173 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index bb2c26c..c6ebd68 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -61,6 +61,7 @@ typedef struct VirtIONet
     uint8_t nouni;
     uint8_t nobcast;
     uint8_t vhost_started;
+    uint8_t dataplane_started;
     struct {
         int in_use;
         int first_multi;
@@ -98,6 +99,9 @@ static VirtIOFeature feature_sizes[] = {
     {}
 };
 
+static void virtio_net_data_plane_start(VirtIONet *n);
+static void virtio_net_data_plane_stop(VirtIONet *n);
+
 static VirtIONetQueue *virtio_net_get_subqueue(NetClientState *nc)
 {
     VirtIONet *n = qemu_get_nic_opaque(nc);
@@ -150,6 +154,12 @@ static bool virtio_net_started(VirtIONet *n, uint8_t status)
         (n->status & VIRTIO_NET_S_LINK_UP) && n->vdev.vm_running;
 }
 
+#if 0
+static void virtio_net_data_plane_status(VirtIONet *n, uint8_t status)
+{
+}
+#endif
+
 static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
 {
     NetClientState *nc = qemu_get_queue(n->nic);
@@ -197,6 +207,19 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
 
     virtio_net_vhost_status(n, status);
 
+    if (virtio_net_started(n, status) && !n->vhost_started) {
+        /* use data plane */
+        if (!n->dataplane_started) {
+            virtio_net_data_plane_start(n);
+            n->dataplane_started = 1;
+            return;
+        } else {
+            virtio_net_data_plane_stop(n);
+            n->dataplane_started = 0;
+            return;
+        }
+    }
+
     for (i = 0; i < n->max_queues; i++) {
         q = &n->vqs[i];
 
@@ -1008,6 +1031,141 @@ static void virtio_net_handle_tx_timer(VirtIODevice *vdev, VirtQueue *vq)
     }
 }
 
+/* In esseatial, this is ioevent handler */
+static gboolean virtio_net_tx_handler(gpointer user_data)
+{
+    NetClientSource *nsrc = (NetClientSource *)user_data;
+    VirtQueue *vq = (VirtQueue *)nsrc->opaque;
+    VirtIODevice *vdev =  virtio_queue_get_vdev(vq);
+    VirtIONet *n = to_virtio_net(vdev);
+    VirtIONetQueue *q = &n->vqs[vq2q(virtio_get_queue_index(vq))];
+    int32_t ret;
+
+    event_notifier_test_and_clear(virtio_queue_get_host_notifier(vq));
+
+    /* This happens when device was stopped but VCPU wasn't. */
+    if (!n->vdev.vm_running) {
+        return false;
+    }
+    virtio_queue_set_notification(vq, 0);
+    ret = virtio_net_flush_tx(q);
+    if (ret >= n->tx_burst) {
+        event_notifier_set(virtio_queue_get_host_notifier(vq));
+    } else if (ret >= 0) {
+        /* ret == -EBUSY, will rely on cb to re-enable */
+        virtio_queue_set_notification(vq, 1);
+    }
+
+    return true;
+}
+
+static gboolean virtio_net_rx_handler(gpointer user_data)
+{
+    NetClientSource *nsrc = (NetClientSource *)user_data;
+    VirtQueue *vq = (VirtQueue *)nsrc->opaque;
+    VirtIODevice *vdev =  virtio_queue_get_vdev(vq);
+
+    event_notifier_test_and_clear(virtio_queue_get_host_notifier(vq));
+    virtio_net_handle_rx(vdev, vq);
+
+    return true;
+}
+
+/* Detach the org ioeventfd handler */
+static void queue_host_notifier_rebind(VirtQueue *vq)
+{
+    VirtIODevice *vdev =  virtio_queue_get_vdev(vq);
+    VirtIONet *n = to_virtio_net(vdev);
+    VirtIONetQueue *q;
+    NetClientState *nc;
+    NetClientSource *nsrc;
+    nc = &n->nic->ncs[vq2q(virtio_get_queue_index(vq))];
+    q = &n->vqs[vq2q(virtio_get_queue_index(vq))];
+
+    if (vq == q->rx_vq) {
+        nsrc = nc->nsrc[0];
+    } else {
+        nsrc = nc->nsrc[1];
+    }
+    event_notifier_set_handler(virtio_queue_get_host_notifier(vq), NULL);
+    g_source_attach(&nsrc->source, nsrc->ctx);
+}
+
+static void virtio_net_host_notifiers_rebind(VirtIONet *n)
+{
+    VirtIONetQueue *q;
+    int i;
+
+    for (i = 0; i < n->curr_queues; i++) {
+        q = &n->vqs[i];
+        queue_host_notifier_rebind(q->rx_vq);
+        queue_host_notifier_rebind(q->tx_vq);
+    }
+}
+
+static void virtio_net_gsource_init(VirtIONet *n)
+{
+    VirtIONetQueue *q;
+    NetClientState *nc;
+    NetClientSource *nsrc;
+    VirtIODevice *vdev = &n->vdev;
+    int fd;
+    int i;
+
+    for (i = 0; i < n->curr_queues; i++) {
+
+        q = &n->vqs[i];
+        /* rx */
+        nsrc = (NetClientSource *)g_source_new(&net_gsource_funcs,
+                    sizeof(NetClientSource));
+        vdev->binding->set_host_notifier(vdev->binding_opaque, i, true);
+        fd = event_notifier_get_fd(virtio_queue_get_host_notifier(q->rx_vq));
+        nc = &n->nic->ncs[i];
+        net_init_gsource(nc, 0, nsrc, fd, G_IO_IN, NULL, NULL,
+            virtio_net_rx_handler, q->rx_vq);
+        /* tx */
+        nsrc = (NetClientSource *)g_source_new(&net_gsource_funcs,
+                    sizeof(NetClientSource));
+        vdev->binding->set_host_notifier(vdev->binding_opaque, 2*i+1, true);
+        fd = event_notifier_get_fd(virtio_queue_get_host_notifier(q->tx_vq));
+        net_init_gsource(nc, 1, nsrc, fd, G_IO_IN, NULL, NULL,
+            virtio_net_tx_handler, q->tx_vq);
+    }
+}
+
+static void virtio_net_data_plane_start(VirtIONet *n)
+{
+    virtio_net_gsource_init(n);
+    virtio_net_host_notifiers_rebind(n);
+}
+
+static void queue_data_plane_stop(VirtIONetQueue *q)
+{
+    VirtIONet *n = q->n;
+    VirtIODevice *vdev = &n->vdev;
+    int idx;
+    NetClientSource *nsrc;
+    NetClientState *nc;
+    idx = vq2q(virtio_get_queue_index(q->rx_vq));
+    nc = &n->nic->ncs[idx];
+
+    vdev->binding->set_host_notifier(vdev->binding_opaque, 2*idx, false);
+    nsrc = nc->nsrc[0];
+    g_source_destroy(&nsrc->source);
+    vdev->binding->set_host_notifier(vdev->binding_opaque, 2*idx+1, false);
+    nsrc = nc->nsrc[1];
+    g_source_destroy(&nsrc->source);
+}
+
+static void virtio_net_data_plane_stop(VirtIONet *n)
+{
+    int i;
+
+    for (i = 0; i < n->curr_queues; i++) {
+        queue_data_plane_stop(&n->vqs[i]);
+    }
+}
+
 static void virtio_net_handle_tx_bh(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIONet *n = to_virtio_net(vdev);
@@ -1274,6 +1432,12 @@ static void virtio_net_cleanup(NetClientState *nc)
     n->nic = NULL;
 }
 
+static void virtio_net_bind_ctx(NetClientState *nc, GMainContext *ctx)
+{
+    g_source_attach(&nc->nsrc[0]->source, ctx);
+    g_source_attach(&nc->nsrc[1]->source, ctx);
+}
+
 static NetClientInfo net_virtio_info = {
     .type = NET_CLIENT_OPTIONS_KIND_NIC,
     .size = sizeof(NICState),
@@ -1281,6 +1445,7 @@ static NetClientInfo net_virtio_info = {
     .receive = virtio_net_receive,
         .cleanup = virtio_net_cleanup,
     .link_status_changed = virtio_net_set_link_status,
+    .bind_ctx = virtio_net_bind_ctx,
 };
 
 static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
diff --git a/hw/virtio.c b/hw/virtio.c
index e259348..d21c4f2 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -658,6 +658,12 @@ int virtio_queue_get_id(VirtQueue *vq)
     return vq - &vdev->vq[0];
 }
 
+VirtIODevice *virtio_queue_get_vdev(VirtQueue *vq)
+{
+    VirtIODevice *vdev = vq->vdev;
+    return vdev;
+}
+
 void virtio_queue_notify_vq(VirtQueue *vq)
 {
     if (vq->vring.desc) {
diff --git a/hw/virtio.h b/hw/virtio.h
index 1e206b8..afe4cf4 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -283,6 +283,8 @@ void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n, uint16_t idx);
 VirtQueue *virtio_get_queue(VirtIODevice *vdev, int n);
 uint16_t virtio_get_queue_index(VirtQueue *vq);
 int virtio_queue_get_id(VirtQueue *vq);
+VirtIODevice *virtio_queue_get_vdev(VirtQueue *vq);
+
 EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq);
 void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign,
                                                 bool with_irqfd);
-- 
1.7.4.4

Re: [Qemu-devel] [RFC PATCH v2 0/4] port network layer onto glib

[Paolo Bonzini]

Il 28/03/2013 08:55, Liu Ping Fan ha scritto:
>   disadvantage for current AioContext
>    1st. need to define and expand interface for other fd events, while glib open this interface for user *

True.

>    2nd. need to add support for IOCanReadHandler, while gsource provide prepare, check method to allow more flexible control

The AioFlushHandler is basically the same thing as the IOCanReadHandler.

>    3rd. block layer's AioContext will block other AioContexts on the same thread.

I cannot understand this.

>    4th. need more document

I really don't think so.

>  disadvantage for glib
>    1st. if more than one fds on the same GSource, need re-implement something like aio_set_file_handler

Re: [Qemu-devel] [RFC PATCH v2 0/4] port network layer onto glib

[Stefan Hajnoczi]

On Thu, Mar 28, 2013 at 09:42:47AM +0100, Paolo Bonzini wrote:
> Il 28/03/2013 08:55, Liu Ping Fan ha scritto:
> >    3rd. block layer's AioContext will block other AioContexts on the same thread.
> 
> I cannot understand this.

The plan is for BlockDriverState to be bound to an AioContext.  That
means each thread is set up with one AioContext.  BlockDriverStates that
are used in that thread will first be bound to its AioContext.

It's not very useful to have multiple AioContext in the same thread.

Stefan

Re: [Qemu-devel] [RFC PATCH v2 1/4] net: port tap onto glib

[Stefan Hajnoczi]

On Thu, Mar 28, 2013 at 03:55:52PM +0800, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> Bind each NetClientState with a GSource(ie,NetClientSource). Currently,
> these GSource attached with default context, but in future, after
> resolving the race between handlers and the interface exposed by NetClientInfo
> and other re-entrant issue,  we can run NetClientState on different threads
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  include/net/net.h |   27 +++++++++++++++
>  net/net.c         |   96 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  net/tap.c         |   57 +++++++++++++++++++++++++------
>  3 files changed, 169 insertions(+), 11 deletions(-)

Please split this into two patches:

1. NetClientSource
2. Convert tap to NetClientSource

Once you do that it turns out that NetClientSource has nothing to do
with the net subsystem, it's a generic file descriptor GSource (weird
that glib doesn't already provide this abstraction).

Each net client needs to reimplement .bind_ctx() anyway, so I don't see
much point in having NetClientSource.nsrc[].  We might as well let net
clients have that field themselves and destroy the GSource in their
destructor function.

> diff --git a/include/net/net.h b/include/net/net.h
> index cb049a1..8fdb7eb 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,8 +56,25 @@ typedef struct NetClientInfo {
>      NetCleanup *cleanup;
>      LinkStatusChanged *link_status_changed;
>      NetPoll *poll;
> +    NetClientBindCtx *bind_ctx;
>  } NetClientInfo;
>  
> +typedef bool (*Pollable)(void *opaque);
> +
> +typedef struct NetClientSource {
> +    GSource source;
> +    GPollFD gfd;
> +    GMainContext *ctx;
> +
> +    Pollable readable;
> +    Pollable writeable;
> +    /* status returned by pollable last time */
> +    bool last_rd_sts;
> +    bool last_wr_sts;

Please use full names, then you can also drop the comment:

bool last_readable_status;
bool last_writeable_status;

> +
> +    void *opaque;
> +} NetClientSource;
> +
>  struct NetClientState {
>      NetClientInfo *info;
>      int link_down;
> @@ -69,6 +87,10 @@ struct NetClientState {
>      unsigned receive_disabled : 1;
>      NetClientDestructor *destructor;
>      unsigned int queue_index;
> +    /* For virtio net, rx on [0], tx on [1], so the virtque

s/virtque/virtqueue/

> @@ -558,6 +564,96 @@ qemu_sendv_packet(NetClientState *nc, const struct iovec *iov, int iovcnt)
>      return qemu_sendv_packet_async(nc, iov, iovcnt, NULL);
>  }
>  
> +static gboolean prepare(GSource *src, gint *time)
> +{
> +    NetClientSource *nsrc = (NetClientSource *)src;
> +    bool rd, wr, change = false;
> +
> +    if (!nsrc->readable && !nsrc->writeable) {
> +        return false;
> +    }
> +    if (nsrc->readable) {
> +        rd = nsrc->readable(nsrc->opaque);
> +        if (nsrc->last_rd_sts != rd) {
> +            nsrc->last_rd_sts = rd;
> +            change = true;
> +        }
> +    }
> +    if (nsrc->writeable) {
> +        wr = nsrc->writeable(nsrc->opaque);
> +        if (nsrc->last_wr_sts != wr) {
> +            nsrc->last_wr_sts = wr;
> +            change = true;
> +        }
> +    }
> +    if (!change) {
> +        return false;
> +    }
> +
> +    g_source_remove_poll(src, &nsrc->gfd);
> +    if (rd) {
> +        nsrc->gfd.events |= G_IO_IN;
> +    } else {
> +        nsrc->gfd.events &= ~G_IO_IN;
> +    }
> +    if (wr) {
> +        nsrc->gfd.events |= G_IO_OUT;
> +    } else {
> +        nsrc->gfd.events &= ~G_IO_OUT;
> +    }
> +    g_source_add_poll(src, &nsrc->gfd);
> +    return false;

This seems equivalent to:

int events = 0;

if (nsrc->readable && nsrc->readable(nsrc->opaque)) {
    events |= G_IO_IN;
}
if (nsrc->writeable && nsrc->writeable(nsrc->opaque)) {
    events |= G_IO_OUT;
}
if (events != nsrc->gfd.events) {
    g_source_remove_poll(src, &nsrc->gfd);
    nsrc->gfd.events = events;
    g_source_add_poll(src, &nsrc->gfd);
}
return FALSE;

This way you can drop last_wr_sts/last_rd_sts.

Are you sure you need to remove and re-add the GPollFD when .events is
changed?

> +void net_init_gsource(NetClientState *nc, int idx, NetClientSource *nsrc,
> +    int fd, int events, Pollable rd, Pollable wr, GSourceFunc f, void *opaque)
> +{
> +    nsrc->gfd.fd = fd;
> +    nsrc->gfd.events = events;
> +    nsrc->opaque = opaque;
> +    nsrc->readable = rd;
> +    nsrc->writeable = wr;
> +    nsrc->last_rd_sts = false;
> +    nsrc->last_wr_sts = false;
> +    if (idx == 0) {
> +        nc->nsrc[0] = nsrc;
> +    } else {
> +        nc->nsrc[1] = nsrc;
> +    }
> +    /* add PollFD if it wont change, otherwise left for GSource prepare */
> +    if (!rd && !wr) {
> +        g_source_add_poll(&nsrc->source, &nsrc->gfd);
> +    }
> +    g_source_set_callback(&nsrc->source, f, nsrc, NULL);
> +}

Simpler version:

NetClientSource *net_source_new(size_t size, int fd,
                                GSourceFunc f, void *opaque)
{
    NetClientSource *nsrc = (NetClientSource *)g_source_new(&net_gsource_funcs,
                                                            size);
    memset(nsrc, 0, sizeof(*nsrc)); /* docs don't say whether memory is 0 */
    nsrc->gfd.fd = fd;
    nsrc->opaque = opaque;
    g_source_set_callback(&nsrc->source, f, nsrc, NULL);
    return nsrc;
}

net_gsource_funcs can be static, callers don't need to know about it.

We don't need to mess with readable/writeable or g_source_add_poll().
Let the caller do nsrc->readable = foo and let prepare() take care of
g_source_add_poll().

> diff --git a/net/tap.c b/net/tap.c
> index daab350..0b663d1 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -70,25 +70,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 = (TAPState *)opaque;

No cast necessary from void* pointer.

> +
> +    if (s->enabled && s->read_poll &&
> +        tap_can_send(s)) {
> +        return true;
> +    }
> +    return false;
> +}
> +
> +static bool writeable(void *opaque)
> +{
> +    TAPState *s = (TAPState *)opaque;

No cast necessary from void* pointer.

> +
> +    if (s->enabled && s->write_poll) {
> +        return true;
> +    }
> +    return false;
> +}
> +
> +static gboolean tap_handler(gpointer data)
> +{
> +    NetClientSource *nsrc = (NetClientSource *)data;

No cast necessary from gpointer.

> +
> +    if (nsrc->gfd.revents & G_IO_IN) {
> +        tap_send(nsrc->opaque);
> +    }
> +    if (nsrc->gfd.revents & G_IO_OUT) {
> +        tap_writable(nsrc->opaque);

Since tap already uses "writable" please use it instead of "writeable"
in your patch.  I grepped to check that "writeable" is not used in the
net subsystem.

Re: [Qemu-devel] [RFC PATCH v2 2/4] net: resolve race of tap backend and its peer

[Stefan Hajnoczi]

On Thu, Mar 28, 2013 at 03:55:53PM +0800, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  net/tap.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/net/tap.c b/net/tap.c
> index 0b663d1..401161c 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -321,9 +321,14 @@ 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);
> +
> +    if (!enable) {
> +        /* need sync so vhost can take over polling */
> +        g_source_remove_poll(&nc->nsrc->source, &nc->nsrc->gfd);
> +    }

Is this necessary?

Since we're in tap_poll() we are currently not in poll(2) waiting on the
fd.  Therefore it's safe for the caller to use the fd, the prepare
function will remove it from the fd set before glib calls poll(2) again.

Stefan

Re: [Qemu-devel] [RFC PATCH v2 3/4] net: port hub onto glib

[Stefan Hajnoczi]

On Thu, Mar 28, 2013 at 03:55:54PM +0800, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> Attach each hubport with a GSource. Currently the GSource is attached
> to default main context, and the hub still run on iothread, but in
> future, after making the whole layer thread-safe, we will admit ports
> to run on different thread.
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/qdev-properties-system.c |    1 +
>  include/net/queue.h         |   14 ++++++++++++++
>  net/hub.c                   |   34 ++++++++++++++++++++++++++++++++--
>  net/net.c                   |    1 +
>  net/queue.c                 |    4 ++--
>  5 files changed, 50 insertions(+), 4 deletions(-)

There are two possibilities when a hub is used:

1. All net clients on the hub run in the same event loop.

We don't need any code changes to make this work.

2. Net clients run in different event loops.

I don't think this case matters, let's forbid it.  It's not worth adding
more complexity to the net subsystem in order to support it.

Can you think of a use case that is well-served by net clients in
multiple event loops?

Stefan

Re: [Qemu-devel] [RFC PATCH v2 0/4] port network layer onto glib

[Stefan Hajnoczi]

On Thu, Mar 28, 2013 at 03:55:51PM +0800, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> These series aim to make the whole network re-entrant, here only apply backend and frontend,
> and for the netcore, separated patches have been sent out.  All of these will prepare us for
> moving towards making network layer mutlit-thread.
> Finally it would be omething like
>    qemu -object io-thread,id=thread0 \
>      -device virtio-net,rx[0]=thread0,tx[0]=thread0
> 
> The brief of the whole aim and plan is documented on
>   http://wiki.qemu.org/Features/network_reentrant
> 
> The main issue is about GSource or AioContext,
>   http://marc.info/?t=136315453300002&r=1&w=3
> And I sumary the main points:
>   disadvantage for current AioContext
>    1st. need to define and expand interface for other fd events, while glib open this interface for user *
>    2nd. need to add support for IOCanReadHandler, while gsource provide prepare, check method to allow more flexible control
>    3rd. block layer's AioContext will block other AioContexts on the same thread.
>    4th. need more document
>  disadvantage for glib
>    1st. if more than one fds on the same GSource, need re-implement something like aio_set_file_handler
> 
> Since I have successed to port frontend on glib, there is no obstale to use glib.
> 
> 
> v1->v2:
>   1.NetClientState can associate with up to 2 GSource, for virtio net, one for tx, one for rx, 
>     so vq can run on different threads.
>   2.make network front-end onto glib, currently virtio net dataplane
> 
> 
> Liu Ping Fan (4):
>   net: port tap onto glib
>   net: resolve race of tap backend and its peer
>   net: port hub onto glib
>   net: port virtio net onto glib
> 
>  hw/qdev-properties-system.c |    1 +
>  hw/virtio-net.c             |  165 +++++++++++++++++++++++++++++++++++++++++++
>  hw/virtio.c                 |    6 ++
>  hw/virtio.h                 |    2 +
>  include/net/net.h           |   27 +++++++
>  include/net/queue.h         |   14 ++++
>  net/hub.c                   |   34 ++++++++-
>  net/net.c                   |   97 +++++++++++++++++++++++++
>  net/queue.c                 |    4 +-
>  net/tap.c                   |   62 +++++++++++++---
>  10 files changed, 397 insertions(+), 15 deletions(-)

It seems the AioContext vs glib issue hasn't been settled yet.  My take
is that glib is preferrable *if* we don't need to write too many
helpers/wrappers on top (then we're basically back to rolling our own,
AioContext).

I was surprised by the amount of code required to listen on a file
descriptor.  Are you sure there isn't a glib way of doing this that
avoids rolling our own GSource?

In the next series, please drop the hub re-entrancy stuff and virtio-net
data plane.  Instead just focus on systematically moving existing net
clients onto the event loop (net/*.c and NICs).  The only controversial
issue there is AioContext vs glib, and once that's settled we can merge
the patches.

Please avoid layering violations - for example a comment about
virtio-net in net.h, a comment about vhost in tap, or putting
net_source_funcs in net.h.  I think converting all existing net clients
will help make the code changes appropriate and eliminate these kinds of
hacks which are because you're focussing just on virtio, tap, and hub
here.

Stefan

Re: [Qemu-devel] [RFC PATCH v2 0/4] port network layer onto glib

[mdroth]

On Thu, Mar 28, 2013 at 03:55:52PM +0100, Stefan Hajnoczi wrote:
> On Thu, Mar 28, 2013 at 03:55:51PM +0800, Liu Ping Fan wrote:
> > From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> > 
> > These series aim to make the whole network re-entrant, here only apply backend and frontend,
> > and for the netcore, separated patches have been sent out.  All of these will prepare us for
> > moving towards making network layer mutlit-thread.
> > Finally it would be omething like
> >    qemu -object io-thread,id=thread0 \
> >      -device virtio-net,rx[0]=thread0,tx[0]=thread0
> > 
> > The brief of the whole aim and plan is documented on
> >   http://wiki.qemu.org/Features/network_reentrant
> > 
> > The main issue is about GSource or AioContext,
> >   http://marc.info/?t=136315453300002&r=1&w=3
> > And I sumary the main points:
> >   disadvantage for current AioContext
> >    1st. need to define and expand interface for other fd events, while glib open this interface for user *
> >    2nd. need to add support for IOCanReadHandler, while gsource provide prepare, check method to allow more flexible control
> >    3rd. block layer's AioContext will block other AioContexts on the same thread.
> >    4th. need more document
> >  disadvantage for glib
> >    1st. if more than one fds on the same GSource, need re-implement something like aio_set_file_handler
> > 
> > Since I have successed to port frontend on glib, there is no obstale to use glib.
> > 
> > 
> > v1->v2:
> >   1.NetClientState can associate with up to 2 GSource, for virtio net, one for tx, one for rx, 
> >     so vq can run on different threads.
> >   2.make network front-end onto glib, currently virtio net dataplane
> > 
> > 
> > Liu Ping Fan (4):
> >   net: port tap onto glib
> >   net: resolve race of tap backend and its peer
> >   net: port hub onto glib
> >   net: port virtio net onto glib
> > 
> >  hw/qdev-properties-system.c |    1 +
> >  hw/virtio-net.c             |  165 +++++++++++++++++++++++++++++++++++++++++++
> >  hw/virtio.c                 |    6 ++
> >  hw/virtio.h                 |    2 +
> >  include/net/net.h           |   27 +++++++
> >  include/net/queue.h         |   14 ++++
> >  net/hub.c                   |   34 ++++++++-
> >  net/net.c                   |   97 +++++++++++++++++++++++++
> >  net/queue.c                 |    4 +-
> >  net/tap.c                   |   62 +++++++++++++---
> >  10 files changed, 397 insertions(+), 15 deletions(-)
> 
> It seems the AioContext vs glib issue hasn't been settled yet.  My take
> is that glib is preferrable *if* we don't need to write too many
> helpers/wrappers on top (then we're basically back to rolling our own,
> AioContext).
> 
> I was surprised by the amount of code required to listen on a file
> descriptor.  Are you sure there isn't a glib way of doing this that
> avoids rolling our own GSource?

GIOChannel provides a pre-baked GSource you can pass around, but this
might add more confusion to the mix when you consider things like
Slirp which will likely require a custom GSource. It also assumes a
different signature than GSourceFunc for the callback which further adds
to the confusion.

Keeping the interfaces centered around normal GSources I think would help
to avoid the proliferation of more event-handling registration
mechanisms in the future, and make conversions easier if we do need to
change things.

A generic GSource for handling FDs that we can re-use for basic use
cases would help curtail some of the boilerplate later for common fd
handlers. Probably doesn't make sense to generalize until we reach a
decision on glib vs. aiocontext though.

> 
> In the next series, please drop the hub re-entrancy stuff and virtio-net
> data plane.  Instead just focus on systematically moving existing net
> clients onto the event loop (net/*.c and NICs).  The only controversial
> issue there is AioContext vs glib, and once that's settled we can merge
> the patches.
> 
> Please avoid layering violations - for example a comment about
> virtio-net in net.h, a comment about vhost in tap, or putting
> net_source_funcs in net.h.  I think converting all existing net clients
> will help make the code changes appropriate and eliminate these kinds of
> hacks which are because you're focussing just on virtio, tap, and hub
> here.
> 
> Stefan
> 

Re: [Qemu-devel] [RFC PATCH v2 2/4] net: resolve race of tap backend and its peer

[liu ping fan]

On Thu, Mar 28, 2013 at 10:34 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Mar 28, 2013 at 03:55:53PM +0800, Liu Ping Fan wrote:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  net/tap.c |    7 ++++++-
>>  1 files changed, 6 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/tap.c b/net/tap.c
>> index 0b663d1..401161c 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -321,9 +321,14 @@ 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);
>> +
>> +    if (!enable) {
>> +        /* need sync so vhost can take over polling */
>> +        g_source_remove_poll(&nc->nsrc->source, &nc->nsrc->gfd);
>> +    }
>
> Is this necessary?
>
> Since we're in tap_poll() we are currently not in poll(2) waiting on the
> fd.  Therefore it's safe for the caller to use the fd, the prepare
> function will remove it from the fd set before glib calls poll(2) again.
>
The main purpose is to make sure that the GSource->dispatch is not
in-flight, in which case, both vhost handler and ->dispatch() will
work on it.

Regards
Pingfan

> Stefan

Re: [Qemu-devel] [RFC PATCH v2 3/4] net: port hub onto glib

[liu ping fan]

On Thu, Mar 28, 2013 at 10:47 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Mar 28, 2013 at 03:55:54PM +0800, Liu Ping Fan wrote:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> Attach each hubport with a GSource. Currently the GSource is attached
>> to default main context, and the hub still run on iothread, but in
>> future, after making the whole layer thread-safe, we will admit ports
>> to run on different thread.
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  hw/qdev-properties-system.c |    1 +
>>  include/net/queue.h         |   14 ++++++++++++++
>>  net/hub.c                   |   34 ++++++++++++++++++++++++++++++++--
>>  net/net.c                   |    1 +
>>  net/queue.c                 |    4 ++--
>>  5 files changed, 50 insertions(+), 4 deletions(-)
>
> There are two possibilities when a hub is used:
>
> 1. All net clients on the hub run in the same event loop.
>
> We don't need any code changes to make this work.
>
> 2. Net clients run in different event loops.
>
> I don't think this case matters, let's forbid it.  It's not worth adding

Oh great,  if we can forbid it.
> more complexity to the net subsystem in order to support it.
>
> Can you think of a use case that is well-served by net clients in
> multiple event loops?
>
No

Thanks
Pingfan

> Stefan

Re: [Qemu-devel] [RFC PATCH v2 2/4] net: resolve race of tap backend and its peer

[Stefan Hajnoczi]

On Fri, Mar 29, 2013 at 8:21 AM, liu ping fan <qemulist@gmail.com> wrote:
> On Thu, Mar 28, 2013 at 10:34 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Thu, Mar 28, 2013 at 03:55:53PM +0800, Liu Ping Fan wrote:
>>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>
>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>> ---
>>>  net/tap.c |    7 ++++++-
>>>  1 files changed, 6 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/net/tap.c b/net/tap.c
>>> index 0b663d1..401161c 100644
>>> --- a/net/tap.c
>>> +++ b/net/tap.c
>>> @@ -321,9 +321,14 @@ 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);
>>> +
>>> +    if (!enable) {
>>> +        /* need sync so vhost can take over polling */
>>> +        g_source_remove_poll(&nc->nsrc->source, &nc->nsrc->gfd);
>>> +    }
>>
>> Is this necessary?
>>
>> Since we're in tap_poll() we are currently not in poll(2) waiting on the
>> fd.  Therefore it's safe for the caller to use the fd, the prepare
>> function will remove it from the fd set before glib calls poll(2) again.
>>
> The main purpose is to make sure that the GSource->dispatch is not
> in-flight, in which case, both vhost handler and ->dispatch() will
> work on it.

Okay, I see.  hw/vhost_net.c will give the fd to the vhost kernel
module.  We need to be sure the fd is no longer in use - and we don't
want tap readable/writable handlers to be called.

Stefan

Re: [Qemu-devel] [RFC PATCH v2 0/4] port network layer onto glib

[liu ping fan]

On Thu, Mar 28, 2013 at 10:55 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> On Thu, Mar 28, 2013 at 03:55:51PM +0800, Liu Ping Fan wrote:
> > From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> >
> > These series aim to make the whole network re-entrant, here only apply backend and frontend,
> > and for the netcore, separated patches have been sent out.  All of these will prepare us for
> > moving towards making network layer mutlit-thread.
> > Finally it would be omething like
> >    qemu -object io-thread,id=thread0 \
> >      -device virtio-net,rx[0]=thread0,tx[0]=thread0
> >
> > The brief of the whole aim and plan is documented on
> >   http://wiki.qemu.org/Features/network_reentrant
> >
> > The main issue is about GSource or AioContext,
> >   http://marc.info/?t=136315453300002&r=1&w=3
> > And I sumary the main points:
> >   disadvantage for current AioContext
> >    1st. need to define and expand interface for other fd events, while glib open this interface for user *
> >    2nd. need to add support for IOCanReadHandler, while gsource provide prepare, check method to allow more flexible control
> >    3rd. block layer's AioContext will block other AioContexts on the same thread.
> >    4th. need more document
> >  disadvantage for glib
> >    1st. if more than one fds on the same GSource, need re-implement something like aio_set_file_handler
> >
> > Since I have successed to port frontend on glib, there is no obstale to use glib.
> >
> >
> > v1->v2:
> >   1.NetClientState can associate with up to 2 GSource, for virtio net, one for tx, one for rx,
> >     so vq can run on different threads.
> >   2.make network front-end onto glib, currently virtio net dataplane
> >
> >
> > Liu Ping Fan (4):
> >   net: port tap onto glib
> >   net: resolve race of tap backend and its peer
> >   net: port hub onto glib
> >   net: port virtio net onto glib
> >
> >  hw/qdev-properties-system.c |    1 +
> >  hw/virtio-net.c             |  165 +++++++++++++++++++++++++++++++++++++++++++
> >  hw/virtio.c                 |    6 ++
> >  hw/virtio.h                 |    2 +
> >  include/net/net.h           |   27 +++++++
> >  include/net/queue.h         |   14 ++++
> >  net/hub.c                   |   34 ++++++++-
> >  net/net.c                   |   97 +++++++++++++++++++++++++
> >  net/queue.c                 |    4 +-
> >  net/tap.c                   |   62 +++++++++++++---
> >  10 files changed, 397 insertions(+), 15 deletions(-)
>
> It seems the AioContext vs glib issue hasn't been settled yet.  My take
> is that glib is preferrable *if* we don't need to write too many
> helpers/wrappers on top (then we're basically back to rolling our own,
> AioContext).
>
> I was surprised by the amount of code required to listen on a file
> descriptor.  Are you sure there isn't a glib way of doing this that
> avoids rolling our own GSource?
>
Will diving more into the code, as mdroth's suggestion. Currently the
main issue is that glib seems no support for readalbe or writable.

> In the next series, please drop the hub re-entrancy stuff and virtio-net
> data plane.  Instead just focus on systematically moving existing net
> clients onto the event loop (net/*.c and NICs).  The only controversial
> issue there is AioContext vs glib, and once that's settled we can merge
> the patches.
>
What about the core (queue.c, net.c)?  Need I send them out at the
same time or after the backend(clients) convert finished?

> Please avoid layering violations - for example a comment about
> virtio-net in net.h, a comment about vhost in tap, or putting
> net_source_funcs in net.h.  I think converting all existing net clients
> will help make the code changes appropriate and eliminate these kinds of
> hacks which are because you're focussing just on virtio, tap, and hub
> here.

Ok, will convert all.

Regards,
Ping Fan
>
> Stefan

Re: [Qemu-devel] [RFC PATCH v2 0/4] port network layer onto glib

[liu ping fan]

On Thu, Mar 28, 2013 at 9:40 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Mar 28, 2013 at 09:42:47AM +0100, Paolo Bonzini wrote:
>> Il 28/03/2013 08:55, Liu Ping Fan ha scritto:
>> >    3rd. block layer's AioContext will block other AioContexts on the same thread.
>>
>> I cannot understand this.
>
> The plan is for BlockDriverState to be bound to an AioContext.  That
> means each thread is set up with one AioContext.  BlockDriverStates that
> are used in that thread will first be bound to its AioContext.
>
> It's not very useful to have multiple AioContext in the same thread.
>
But it can be the case that we detach and re-attach the different
device( AioContext) to the same thread.   I think that the design of
io_flush is to sync, but for NetClientState, we need something else.
So if we use AioContext, is it proper to extend readable/writeable
interface for qemu_aio_set_fd_handler()?

Thanks,
Pingfan

> Stefan

Re: [Qemu-devel] [RFC PATCH v2 1/4] net: port tap onto glib

[liu ping fan]

On Thu, Mar 28, 2013 at 10:32 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Mar 28, 2013 at 03:55:52PM +0800, Liu Ping Fan wrote:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> Bind each NetClientState with a GSource(ie,NetClientSource). Currently,
>> these GSource attached with default context, but in future, after
>> resolving the race between handlers and the interface exposed by NetClientInfo
>> and other re-entrant issue,  we can run NetClientState on different threads
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  include/net/net.h |   27 +++++++++++++++
>>  net/net.c         |   96 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  net/tap.c         |   57 +++++++++++++++++++++++++------
>>  3 files changed, 169 insertions(+), 11 deletions(-)
>
> Please split this into two patches:
>
> 1. NetClientSource
> 2. Convert tap to NetClientSource
>
> Once you do that it turns out that NetClientSource has nothing to do
> with the net subsystem, it's a generic file descriptor GSource (weird
> that glib doesn't already provide this abstraction).
>
> Each net client needs to reimplement .bind_ctx() anyway, so I don't see
> much point in having NetClientSource.nsrc[].  We might as well let net
> clients have that field themselves and destroy the GSource in their
> destructor function.
>
The only way to detach the GSource from GMainContext is
g_source_destroy, so if we want to re-bind nc from threadA to threadB,
 we should destroy the old one and create a new. Is that meaningful?

>> diff --git a/include/net/net.h b/include/net/net.h
>> index cb049a1..8fdb7eb 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,8 +56,25 @@ typedef struct NetClientInfo {
>>      NetCleanup *cleanup;
>>      LinkStatusChanged *link_status_changed;
>>      NetPoll *poll;
>> +    NetClientBindCtx *bind_ctx;
>>  } NetClientInfo;
>>
>> +typedef bool (*Pollable)(void *opaque);
>> +
>> +typedef struct NetClientSource {
>> +    GSource source;
>> +    GPollFD gfd;
>> +    GMainContext *ctx;
>> +
>> +    Pollable readable;
>> +    Pollable writeable;
>> +    /* status returned by pollable last time */
>> +    bool last_rd_sts;
>> +    bool last_wr_sts;
>
> Please use full names, then you can also drop the comment:
>
> bool last_readable_status;
> bool last_writeable_status;
>
>> +
>> +    void *opaque;
>> +} NetClientSource;
>> +
>>  struct NetClientState {
>>      NetClientInfo *info;
>>      int link_down;
>> @@ -69,6 +87,10 @@ struct NetClientState {
>>      unsigned receive_disabled : 1;
>>      NetClientDestructor *destructor;
>>      unsigned int queue_index;
>> +    /* For virtio net, rx on [0], tx on [1], so the virtque
>
> s/virtque/virtqueue/
>
>> @@ -558,6 +564,96 @@ qemu_sendv_packet(NetClientState *nc, const struct iovec *iov, int iovcnt)
>>      return qemu_sendv_packet_async(nc, iov, iovcnt, NULL);
>>  }
>>
>> +static gboolean prepare(GSource *src, gint *time)
>> +{
>> +    NetClientSource *nsrc = (NetClientSource *)src;
>> +    bool rd, wr, change = false;
>> +
>> +    if (!nsrc->readable && !nsrc->writeable) {
>> +        return false;
>> +    }
>> +    if (nsrc->readable) {
>> +        rd = nsrc->readable(nsrc->opaque);
>> +        if (nsrc->last_rd_sts != rd) {
>> +            nsrc->last_rd_sts = rd;
>> +            change = true;
>> +        }
>> +    }
>> +    if (nsrc->writeable) {
>> +        wr = nsrc->writeable(nsrc->opaque);
>> +        if (nsrc->last_wr_sts != wr) {
>> +            nsrc->last_wr_sts = wr;
>> +            change = true;
>> +        }
>> +    }
>> +    if (!change) {
>> +        return false;
>> +    }
>> +
>> +    g_source_remove_poll(src, &nsrc->gfd);
>> +    if (rd) {
>> +        nsrc->gfd.events |= G_IO_IN;
>> +    } else {
>> +        nsrc->gfd.events &= ~G_IO_IN;
>> +    }
>> +    if (wr) {
>> +        nsrc->gfd.events |= G_IO_OUT;
>> +    } else {
>> +        nsrc->gfd.events &= ~G_IO_OUT;
>> +    }
>> +    g_source_add_poll(src, &nsrc->gfd);
>> +    return false;
>
> This seems equivalent to:
>
> int events = 0;
>
> if (nsrc->readable && nsrc->readable(nsrc->opaque)) {
>     events |= G_IO_IN;
> }
> if (nsrc->writeable && nsrc->writeable(nsrc->opaque)) {
>     events |= G_IO_OUT;
> }
> if (events != nsrc->gfd.events) {
>     g_source_remove_poll(src, &nsrc->gfd);
>     nsrc->gfd.events = events;
>     g_source_add_poll(src, &nsrc->gfd);
> }
> return FALSE;
>
> This way you can drop last_wr_sts/last_rd_sts.
>
Thanks for the simpler.
> Are you sure you need to remove and re-add the GPollFD when .events is
> changed?
>
Re-see the glib source code, and find it is not necessary.
>> +void net_init_gsource(NetClientState *nc, int idx, NetClientSource *nsrc,
>> +    int fd, int events, Pollable rd, Pollable wr, GSourceFunc f, void *opaque)
>> +{
>> +    nsrc->gfd.fd = fd;
>> +    nsrc->gfd.events = events;
>> +    nsrc->opaque = opaque;
>> +    nsrc->readable = rd;
>> +    nsrc->writeable = wr;
>> +    nsrc->last_rd_sts = false;
>> +    nsrc->last_wr_sts = false;
>> +    if (idx == 0) {
>> +        nc->nsrc[0] = nsrc;
>> +    } else {
>> +        nc->nsrc[1] = nsrc;
>> +    }
>> +    /* add PollFD if it wont change, otherwise left for GSource prepare */
>> +    if (!rd && !wr) {
>> +        g_source_add_poll(&nsrc->source, &nsrc->gfd);
>> +    }
>> +    g_source_set_callback(&nsrc->source, f, nsrc, NULL);
>> +}
>
> Simpler version:
>
> NetClientSource *net_source_new(size_t size, int fd,
>                                 GSourceFunc f, void *opaque)
> {
>     NetClientSource *nsrc = (NetClientSource *)g_source_new(&net_gsource_funcs,
>                                                             size);
>     memset(nsrc, 0, sizeof(*nsrc)); /* docs don't say whether memory is 0 */
>     nsrc->gfd.fd = fd;
>     nsrc->opaque = opaque;
>     g_source_set_callback(&nsrc->source, f, nsrc, NULL);
>     return nsrc;
> }
>
> net_gsource_funcs can be static, callers don't need to know about it.
>
> We don't need to mess with readable/writeable or g_source_add_poll().
> Let the caller do nsrc->readable = foo and let prepare() take care of
> g_source_add_poll().
>
I will follow this.
>> diff --git a/net/tap.c b/net/tap.c
>> index daab350..0b663d1 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -70,25 +70,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 = (TAPState *)opaque;
>
> No cast necessary from void* pointer.
>
Oh, will fix all this issue
>> +
>> +    if (s->enabled && s->read_poll &&
>> +        tap_can_send(s)) {
>> +        return true;
>> +    }
>> +    return false;
>> +}
>> +
>> +static bool writeable(void *opaque)
>> +{
>> +    TAPState *s = (TAPState *)opaque;
>
> No cast necessary from void* pointer.
>
>> +
>> +    if (s->enabled && s->write_poll) {
>> +        return true;
>> +    }
>> +    return false;
>> +}
>> +
>> +static gboolean tap_handler(gpointer data)
>> +{
>> +    NetClientSource *nsrc = (NetClientSource *)data;
>
> No cast necessary from gpointer.
>
>> +
>> +    if (nsrc->gfd.revents & G_IO_IN) {
>> +        tap_send(nsrc->opaque);
>> +    }
>> +    if (nsrc->gfd.revents & G_IO_OUT) {
>> +        tap_writable(nsrc->opaque);
>
> Since tap already uses "writable" please use it instead of "writeable"
> in your patch.  I grepped to check that "writeable" is not used in the
> net subsystem.
Ok,

Thanks for your detail review

Pingfan

Re: [Qemu-devel] [RFC PATCH v2 1/4] net: port tap onto glib

[Stefan Hajnoczi]

On Wed, Apr 03, 2013 at 05:28:39PM +0800, liu ping fan wrote:
> On Thu, Mar 28, 2013 at 10:32 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Thu, Mar 28, 2013 at 03:55:52PM +0800, Liu Ping Fan wrote:
> >> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> >>
> >> Bind each NetClientState with a GSource(ie,NetClientSource). Currently,
> >> these GSource attached with default context, but in future, after
> >> resolving the race between handlers and the interface exposed by NetClientInfo
> >> and other re-entrant issue,  we can run NetClientState on different threads
> >>
> >> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> >> ---
> >>  include/net/net.h |   27 +++++++++++++++
> >>  net/net.c         |   96 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  net/tap.c         |   57 +++++++++++++++++++++++++------
> >>  3 files changed, 169 insertions(+), 11 deletions(-)
> >
> > Please split this into two patches:
> >
> > 1. NetClientSource
> > 2. Convert tap to NetClientSource
> >
> > Once you do that it turns out that NetClientSource has nothing to do
> > with the net subsystem, it's a generic file descriptor GSource (weird
> > that glib doesn't already provide this abstraction).
> >
> > Each net client needs to reimplement .bind_ctx() anyway, so I don't see
> > much point in having NetClientSource.nsrc[].  We might as well let net
> > clients have that field themselves and destroy the GSource in their
> > destructor function.
> >
> The only way to detach the GSource from GMainContext is
> g_source_destroy, so if we want to re-bind nc from threadA to threadB,
>  we should destroy the old one and create a new. Is that meaningful?

I guess that can be done.

What I was really thinking when I suggested getting rid of nsrc[] is
that it's a little ugly to have the array with 2 GSources.  Different
net clients have different numbers of GSources - 0 for NICs, 1 for most
backends, 2 for ioeventfd for virtio-net data plane with separate rx/tx.

So my thought was to leave the number of GSources in the layer that uses
them - each specific net client.

Stefan

Re: [Qemu-devel] [RFC PATCH v2 0/4] port network layer onto glib

[Stefan Hajnoczi]

On Tue, Apr 02, 2013 at 05:49:57PM +0800, liu ping fan wrote:
> On Thu, Mar 28, 2013 at 9:40 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Thu, Mar 28, 2013 at 09:42:47AM +0100, Paolo Bonzini wrote:
> >> Il 28/03/2013 08:55, Liu Ping Fan ha scritto:
> >> >    3rd. block layer's AioContext will block other AioContexts on the same thread.
> >>
> >> I cannot understand this.
> >
> > The plan is for BlockDriverState to be bound to an AioContext.  That
> > means each thread is set up with one AioContext.  BlockDriverStates that
> > are used in that thread will first be bound to its AioContext.
> >
> > It's not very useful to have multiple AioContext in the same thread.
> >
> But it can be the case that we detach and re-attach the different
> device( AioContext) to the same thread.   I think that the design of
> io_flush is to sync, but for NetClientState, we need something else.
> So if we use AioContext, is it proper to extend readable/writeable
> interface for qemu_aio_set_fd_handler()?

Devices don't have AioContexts, threads do.  When you bind a device to
an AioContext the AioContext already exists independent of the device.

Unfortunately I don't understand your question about io_flush and
readable/writeable qemu_aio_set_fd_handler().

Stefan

Re: [Qemu-devel] [RFC PATCH v2 0/4] port network layer onto glib

[Stefan Hajnoczi]

On Mon, Apr 01, 2013 at 04:15:06PM +0800, liu ping fan wrote:
> On Thu, Mar 28, 2013 at 10:55 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >
> > On Thu, Mar 28, 2013 at 03:55:51PM +0800, Liu Ping Fan wrote:
> > It seems the AioContext vs glib issue hasn't been settled yet.  My take
> > is that glib is preferrable *if* we don't need to write too many
> > helpers/wrappers on top (then we're basically back to rolling our own,
> > AioContext).
> >
> > I was surprised by the amount of code required to listen on a file
> > descriptor.  Are you sure there isn't a glib way of doing this that
> > avoids rolling our own GSource?
> >
> Will diving more into the code, as mdroth's suggestion. Currently the
> main issue is that glib seems no support for readalbe or writable.

I mentioned this in another reply.  I'm not sure what you mean by glib
does not support readable or writeable.  It has G_IO_IN and G_IO_OUT for
readable/writeable events.

> > In the next series, please drop the hub re-entrancy stuff and virtio-net
> > data plane.  Instead just focus on systematically moving existing net
> > clients onto the event loop (net/*.c and NICs).  The only controversial
> > issue there is AioContext vs glib, and once that's settled we can merge
> > the patches.
> >
> What about the core (queue.c, net.c)?  Need I send them out at the
> same time or after the backend(clients) convert finished?

I think the core changes are necessary to build the converted net
clients, so they should be part of the series.

Stefan

Re: [Qemu-devel] [RFC PATCH v2 0/4] port network layer onto glib

[liu ping fan]

On Mon, Apr 8, 2013 at 7:49 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Apr 01, 2013 at 04:15:06PM +0800, liu ping fan wrote:
>> On Thu, Mar 28, 2013 at 10:55 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> >
>> > On Thu, Mar 28, 2013 at 03:55:51PM +0800, Liu Ping Fan wrote:
>> > It seems the AioContext vs glib issue hasn't been settled yet.  My take
>> > is that glib is preferrable *if* we don't need to write too many
>> > helpers/wrappers on top (then we're basically back to rolling our own,
>> > AioContext).
>> >
>> > I was surprised by the amount of code required to listen on a file
>> > descriptor.  Are you sure there isn't a glib way of doing this that
>> > avoids rolling our own GSource?
>> >
>> Will diving more into the code, as mdroth's suggestion. Currently the
>> main issue is that glib seems no support for readalbe or writable.
>
> I mentioned this in another reply.  I'm not sure what you mean by glib
> does not support readable or writeable.  It has G_IO_IN and G_IO_OUT for
> readable/writeable events.
>
Answer it in another reply.
>> > In the next series, please drop the hub re-entrancy stuff and virtio-net
>> > data plane.  Instead just focus on systematically moving existing net
>> > clients onto the event loop (net/*.c and NICs).  The only controversial
>> > issue there is AioContext vs glib, and once that's settled we can merge
>> > the patches.
>> >
>> What about the core (queue.c, net.c)?  Need I send them out at the
>> same time or after the backend(clients) convert finished?
>
> I think the core changes are necessary to build the converted net
> clients, so they should be part of the series.
>
OK, I will send out all of them in v4.

> Stefan

Re: [Qemu-devel] [RFC PATCH v2 0/4] port network layer onto glib

[liu ping fan]

On Mon, Apr 8, 2013 at 7:46 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Apr 02, 2013 at 05:49:57PM +0800, liu ping fan wrote:
>> On Thu, Mar 28, 2013 at 9:40 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> > On Thu, Mar 28, 2013 at 09:42:47AM +0100, Paolo Bonzini wrote:
>> >> Il 28/03/2013 08:55, Liu Ping Fan ha scritto:
>> >> >    3rd. block layer's AioContext will block other AioContexts on the same thread.
>> >>
>> >> I cannot understand this.
>> >
>> > The plan is for BlockDriverState to be bound to an AioContext.  That
>> > means each thread is set up with one AioContext.  BlockDriverStates that
>> > are used in that thread will first be bound to its AioContext.
>> >
>> > It's not very useful to have multiple AioContext in the same thread.
>> >
>> But it can be the case that we detach and re-attach the different
>> device( AioContext) to the same thread.   I think that the design of
>> io_flush is to sync, but for NetClientState, we need something else.
>> So if we use AioContext, is it proper to extend readable/writeable
>> interface for qemu_aio_set_fd_handler()?
>
> Devices don't have AioContexts, threads do.  When you bind a device to
> an AioContext the AioContext already exists independent of the device.
>
Oh, yes.  So let me say in this way,   switch the devices among
different thread. Then if NetClientState happens to exist on the same
thread with BlockDriverState, it will not be responsive until the
BlockDriverState has finished the flying job.

> Unfortunately I don't understand your question about io_flush and
> readable/writeable qemu_aio_set_fd_handler().
>
As for readable/writable, I mean something like IOCanReadHandler. If
NetClientState is unreadable, it just does not poll for G_IO_IN event,
but not blocks.  But as for io_flush, it will block for pending AIO
operations. These behaviors are different, so I suggest to expand
readable/writeable for qemu_aio_set_fd_handler()

Regards,
Pingfan

> Stefan

Re: [Qemu-devel] [RFC PATCH v2 1/4] net: port tap onto glib

[liu ping fan]

On Mon, Apr 8, 2013 at 7:44 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Wed, Apr 03, 2013 at 05:28:39PM +0800, liu ping fan wrote:
>> On Thu, Mar 28, 2013 at 10:32 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> > On Thu, Mar 28, 2013 at 03:55:52PM +0800, Liu Ping Fan wrote:
>> >> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> >>
>> >> Bind each NetClientState with a GSource(ie,NetClientSource). Currently,
>> >> these GSource attached with default context, but in future, after
>> >> resolving the race between handlers and the interface exposed by NetClientInfo
>> >> and other re-entrant issue,  we can run NetClientState on different threads
>> >>
>> >> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> >> ---
>> >>  include/net/net.h |   27 +++++++++++++++
>> >>  net/net.c         |   96 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >>  net/tap.c         |   57 +++++++++++++++++++++++++------
>> >>  3 files changed, 169 insertions(+), 11 deletions(-)
>> >
>> > Please split this into two patches:
>> >
>> > 1. NetClientSource
>> > 2. Convert tap to NetClientSource
>> >
>> > Once you do that it turns out that NetClientSource has nothing to do
>> > with the net subsystem, it's a generic file descriptor GSource (weird
>> > that glib doesn't already provide this abstraction).
>> >
>> > Each net client needs to reimplement .bind_ctx() anyway, so I don't see
>> > much point in having NetClientSource.nsrc[].  We might as well let net
>> > clients have that field themselves and destroy the GSource in their
>> > destructor function.
>> >
>> The only way to detach the GSource from GMainContext is
>> g_source_destroy, so if we want to re-bind nc from threadA to threadB,
>>  we should destroy the old one and create a new. Is that meaningful?
>
> I guess that can be done.
>
> What I was really thinking when I suggested getting rid of nsrc[] is
> that it's a little ugly to have the array with 2 GSources.  Different
> net clients have different numbers of GSources - 0 for NICs, 1 for most
> backends, 2 for ioeventfd for virtio-net data plane with separate rx/tx.
>
> So my thought was to leave the number of GSources in the layer that uses
> them - each specific net client.
>
OK, see. What about nsrc[0] at the end of struct or **nsrc ?

> Stefan

Re: [Qemu-devel] [RFC PATCH v2 1/4] net: port tap onto glib

[Stefan Hajnoczi]

On Tue, Apr 09, 2013 at 01:12:39PM +0800, liu ping fan wrote:
> On Mon, Apr 8, 2013 at 7:44 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Wed, Apr 03, 2013 at 05:28:39PM +0800, liu ping fan wrote:
> >> On Thu, Mar 28, 2013 at 10:32 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >> > On Thu, Mar 28, 2013 at 03:55:52PM +0800, Liu Ping Fan wrote:
> >> >> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> >> >>
> >> >> Bind each NetClientState with a GSource(ie,NetClientSource). Currently,
> >> >> these GSource attached with default context, but in future, after
> >> >> resolving the race between handlers and the interface exposed by NetClientInfo
> >> >> and other re-entrant issue,  we can run NetClientState on different threads
> >> >>
> >> >> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> >> >> ---
> >> >>  include/net/net.h |   27 +++++++++++++++
> >> >>  net/net.c         |   96 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> >>  net/tap.c         |   57 +++++++++++++++++++++++++------
> >> >>  3 files changed, 169 insertions(+), 11 deletions(-)
> >> >
> >> > Please split this into two patches:
> >> >
> >> > 1. NetClientSource
> >> > 2. Convert tap to NetClientSource
> >> >
> >> > Once you do that it turns out that NetClientSource has nothing to do
> >> > with the net subsystem, it's a generic file descriptor GSource (weird
> >> > that glib doesn't already provide this abstraction).
> >> >
> >> > Each net client needs to reimplement .bind_ctx() anyway, so I don't see
> >> > much point in having NetClientSource.nsrc[].  We might as well let net
> >> > clients have that field themselves and destroy the GSource in their
> >> > destructor function.
> >> >
> >> The only way to detach the GSource from GMainContext is
> >> g_source_destroy, so if we want to re-bind nc from threadA to threadB,
> >>  we should destroy the old one and create a new. Is that meaningful?
> >
> > I guess that can be done.
> >
> > What I was really thinking when I suggested getting rid of nsrc[] is
> > that it's a little ugly to have the array with 2 GSources.  Different
> > net clients have different numbers of GSources - 0 for NICs, 1 for most
> > backends, 2 for ioeventfd for virtio-net data plane with separate rx/tx.
> >
> > So my thought was to leave the number of GSources in the layer that uses
> > them - each specific net client.
> >
> OK, see. What about nsrc[0] at the end of struct or **nsrc ?

The net core code doesn't need to know about nsrc at all.  I think
nsrc[] should be dropped completely.  The net backends should manage
their GSource (if they need one).

Stefan

Re: [Qemu-devel] [RFC PATCH v2 0/4] port network layer onto glib

[Stefan Hajnoczi]

On Tue, Apr 09, 2013 at 01:10:29PM +0800, liu ping fan wrote:
> On Mon, Apr 8, 2013 at 7:46 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Tue, Apr 02, 2013 at 05:49:57PM +0800, liu ping fan wrote:
> >> On Thu, Mar 28, 2013 at 9:40 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >> > On Thu, Mar 28, 2013 at 09:42:47AM +0100, Paolo Bonzini wrote:
> >> >> Il 28/03/2013 08:55, Liu Ping Fan ha scritto:
> >> >> >    3rd. block layer's AioContext will block other AioContexts on the same thread.
> >> >>
> >> >> I cannot understand this.
> >> >
> >> > The plan is for BlockDriverState to be bound to an AioContext.  That
> >> > means each thread is set up with one AioContext.  BlockDriverStates that
> >> > are used in that thread will first be bound to its AioContext.
> >> >
> >> > It's not very useful to have multiple AioContext in the same thread.
> >> >
> >> But it can be the case that we detach and re-attach the different
> >> device( AioContext) to the same thread.   I think that the design of
> >> io_flush is to sync, but for NetClientState, we need something else.
> >> So if we use AioContext, is it proper to extend readable/writeable
> >> interface for qemu_aio_set_fd_handler()?
> >
> > Devices don't have AioContexts, threads do.  When you bind a device to
> > an AioContext the AioContext already exists independent of the device.
> >
> Oh, yes.  So let me say in this way,   switch the devices among
> different thread. Then if NetClientState happens to exist on the same
> thread with BlockDriverState, it will not be responsive until the
> BlockDriverState has finished the flying job.

It's partially true that devices sharing an event loop may be less
responsive.  That's why we have the option of a 1:1 device-to-thread
mapping.

But remember that QEMU code is (meant to be) designed for event loops.
Therefore, it must not block and should return back to the event loop as
quickly as possible.  So a block and net device in the same event loop
shouldn't inconvenience each other dramatically if the device-to-thread
mappings are reasonable given the host machine, workload, etc.

> > Unfortunately I don't understand your question about io_flush and
> > readable/writeable qemu_aio_set_fd_handler().
> >
> As for readable/writable, I mean something like IOCanReadHandler. If
> NetClientState is unreadable, it just does not poll for G_IO_IN event,
> but not blocks.  But as for io_flush, it will block for pending AIO
> operations. These behaviors are different, so I suggest to expand
> readable/writeable for qemu_aio_set_fd_handler()

I see, thanks for explaining.

In another thread Kevin suggested a solution:

Basically, io_flush() and qemu_aio_wait() should be removed.  Instead
we'll push the synchronous wait into the block layer, which is the only
user.

We can do that by introducing a .bdrv_drain() function which is similar
to io_flush().  Now the qemu_drain_all() which uses qemu_aio_wait() can
change to calling .bdrv_drain() and then executing an event loop
iteration.

In other words, the event loop shouldn't know about io_flush().

I will try to send patches for this today.

Stefan