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

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

[Liu Ping Fan]

Currently, I drop slirp and frontend related code (I guess I can reuse some
code by mdroth's QContext later). And this series only resolve the net core
re-entrant problem.

v1->v2:
  introduce netqueue re-entrant detection and defer it to BH

Liu Ping Fan (6):
  net: introduce lock to protect NetQueue
  net: introduce lock to protect NetClientState's peer's access
  net: make netclient re-entrant with refcnt
  net: force NetQue opaque to be NetClientState
  net: defer nested call to BH
  net: hub use lock to protect ports list

 hw/core/qdev-properties-system.c |  14 +++++
 include/net/net.h                |  10 ++++
 include/net/queue.h              |   2 +-
 net/hub.c                        |  28 ++++++++-
 net/net.c                        | 124 +++++++++++++++++++++++++++++++++++++--
 net/queue.c                      |  57 ++++++++++++++++--
 net/slirp.c                      |   3 +-
 7 files changed, 225 insertions(+), 13 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH v2 1/6] net: introduce lock to protect NetQueue

[Liu Ping Fan]

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>
---
 include/net/net.h |  1 +
 net/queue.c       | 13 +++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/net/net.h b/include/net/net.h
index 43d85a1..2f72b26 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"
diff --git a/net/queue.c b/net/queue.c
index 859d02a..c6d4241 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;
@@ -96,7 +98,9 @@ static void qemu_net_queue_append(NetQueue *queue,
 {
     NetPacket *packet;
 
+    qemu_mutex_lock(&queue->lock);
     if (queue->nq_count >= queue->nq_maxlen && !sent_cb) {
+        qemu_mutex_unlock(&queue->lock);
         return; /* drop if queue full and no callback */
     }
     packet = g_malloc(sizeof(NetPacket) + size);
@@ -108,6 +112,7 @@ static void qemu_net_queue_append(NetQueue *queue,
 
     queue->nq_count++;
     QTAILQ_INSERT_TAIL(&queue->packets, packet, entry);
+    qemu_mutex_unlock(&queue->lock);
 }
 
 static void qemu_net_queue_append_iov(NetQueue *queue,
@@ -121,7 +126,9 @@ static void qemu_net_queue_append_iov(NetQueue *queue,
     size_t max_len = 0;
     int i;
 
+    qemu_mutex_lock(&queue->lock);
     if (queue->nq_count >= queue->nq_maxlen && !sent_cb) {
+        qemu_mutex_unlock(&queue->lock);
         return; /* drop if queue full and no callback */
     }
     for (i = 0; i < iovcnt; i++) {
@@ -143,6 +150,7 @@ static void qemu_net_queue_append_iov(NetQueue *queue,
 
     queue->nq_count++;
     QTAILQ_INSERT_TAIL(&queue->packets, packet, entry);
+    qemu_mutex_unlock(&queue->lock);
 }
 
 static ssize_t qemu_net_queue_deliver(NetQueue *queue,
@@ -229,6 +237,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 +245,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 +267,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 +277,6 @@ bool qemu_net_queue_flush(NetQueue *queue)
 
         g_free(packet);
     }
+    qemu_mutex_unlock(&queue->lock);
     return true;
 }
-- 
1.8.1.4

[Qemu-devel] [PATCH v2 2/6] net: introduce lock to protect NetClientState's peer's access

[Liu Ping Fan]

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 |  6 +++++
 net/net.c         | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 net/queue.c       |  4 +--
 3 files changed, 83 insertions(+), 6 deletions(-)

diff --git a/include/net/net.h b/include/net/net.h
index 2f72b26..ea46f13 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -67,6 +67,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;
@@ -79,6 +83,7 @@ struct NetClientState {
 
 typedef struct NICState {
     NetClientState *ncs;
+    NetClientState **pending_peer;
     NICConf *conf;
     void *opaque;
     bool peer_deleted;
@@ -106,6 +111,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 43a74e4..717db12 100644
--- a/net/net.c
+++ b/net/net.c
@@ -204,6 +204,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);
@@ -243,6 +244,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,
@@ -301,6 +303,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];
@@ -331,7 +365,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;
@@ -340,6 +376,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]);
     }
@@ -352,17 +389,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);
 }
 
@@ -379,7 +418,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;
@@ -394,6 +433,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,
@@ -457,19 +518,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,
@@ -537,16 +603,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 c6d4241..7d6c52e 100644
--- a/net/queue.c
+++ b/net/queue.c
@@ -192,7 +192,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;
     }
@@ -217,7 +217,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.8.1.4

[Qemu-devel] [PATCH v2 3/6] net: make netclient re-entrant with refcnt

[Liu Ping Fan]

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/core/qdev-properties-system.c | 14 ++++++++++++
 include/net/net.h                |  3 +++
 net/hub.c                        |  3 +++
 net/net.c                        | 47 +++++++++++++++++++++++++++++++++++++---
 net/slirp.c                      |  3 ++-
 5 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 0eada32..41cc7e6 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -302,6 +302,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,
@@ -311,11 +312,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 ea46f13..1a31d1b 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -64,6 +64,7 @@ typedef struct NetClientInfo {
 } NetClientInfo;
 
 struct NetClientState {
+    int ref;
     NetClientInfo *info;
     int link_down;
     QTAILQ_ENTRY(NetClientState) next;
@@ -92,6 +93,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 df32074..9c6c559 100644
--- a/net/hub.c
+++ b/net/hub.c
@@ -201,6 +201,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);
                     return peer;
                 }
             }
@@ -223,6 +224,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);
                     return &(port->nc);
                 }
             }
@@ -231,6 +233,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 717db12..478a719 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;
@@ -165,6 +166,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;
@@ -173,6 +175,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);
 
@@ -203,9 +206,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;
@@ -221,6 +228,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);
 
@@ -281,7 +289,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);
@@ -303,6 +313,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.
  */
@@ -331,8 +353,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)
@@ -378,7 +402,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]);
     }
 }
 
@@ -389,7 +413,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]);
         }
     }
 
@@ -398,7 +422,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);
@@ -409,6 +433,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) {
@@ -416,6 +441,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)
@@ -630,13 +656,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;
 }
@@ -647,6 +677,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;
@@ -658,6 +689,7 @@ int qemu_find_net_clients_except(const char *id, NetClientState **ncs,
             ret++;
         }
     }
+    qemu_mutex_unlock(&net_clients_lock);
 
     return ret;
 }
@@ -963,9 +995,11 @@ void net_host_device_remove(Monitor *mon, const QDict *qdict)
     }
     if (!net_host_check_device(nc->model)) {
         monitor_printf(mon, "invalid host network device %s\n", device);
+        netclient_unref(nc);
         return;
     }
     qemu_del_net_client(nc);
+    netclient_unref(nc);
 }
 
 void netdev_add(QemuOpts *opts, Error **errp)
@@ -1021,6 +1055,7 @@ void qmp_netdev_del(const char *id, Error **errp)
     }
 
     qemu_del_net_client(nc);
+    netclient_unref(nc);
     qemu_opts_del(opts);
 }
 
@@ -1039,6 +1074,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;
@@ -1056,6 +1092,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)
@@ -1109,6 +1146,7 @@ void net_cleanup(void)
             qemu_del_net_client(nc);
         }
     }
+    qemu_mutex_destroy(&net_clients_lock);
 }
 
 void net_check_clients(void)
@@ -1130,6 +1168,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",
@@ -1137,6 +1176,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
@@ -1194,6 +1234,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 b3f35d5..e541548 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.8.1.4

[Qemu-devel] [PATCH v2 4/6] net: force NetQue opaque to be NetClientState

[Liu Ping Fan]

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

qemu_net_client_setup() is the only user of qemu_new_net_queue(), which
will pass in NetClientState. By forcing it be a NetClientState, we
can ref/unref NetQueue's owner

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 include/net/queue.h | 2 +-
 net/queue.c         | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/net/queue.h b/include/net/queue.h
index fc02b33..ddb6d98 100644
--- a/include/net/queue.h
+++ b/include/net/queue.h
@@ -34,7 +34,7 @@ typedef void (NetPacketSent) (NetClientState *sender, ssize_t ret);
 #define QEMU_NET_PACKET_FLAG_NONE  0
 #define QEMU_NET_PACKET_FLAG_RAW  (1<<0)
 
-NetQueue *qemu_new_net_queue(void *opaque);
+NetQueue *qemu_new_net_queue(NetClientState *opaque);
 
 void qemu_del_net_queue(NetQueue *queue);
 
diff --git a/net/queue.c b/net/queue.c
index 7d6c52e..58222b0 100644
--- a/net/queue.c
+++ b/net/queue.c
@@ -49,7 +49,7 @@ struct NetPacket {
 };
 
 struct NetQueue {
-    void *opaque;
+    NetClientState *opaque;
     uint32_t nq_maxlen;
     uint32_t nq_count;
 
@@ -59,7 +59,7 @@ struct NetQueue {
     unsigned delivering : 1;
 };
 
-NetQueue *qemu_new_net_queue(void *opaque)
+NetQueue *qemu_new_net_queue(NetClientState *opaque)
 {
     NetQueue *queue;
 
-- 
1.8.1.4

[Qemu-devel] [PATCH v2 5/6] net: defer nested call to BH

[Liu Ping Fan]

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

Nested call caused by ->receive() will raise issue like deadlock,
so postphone it to BH.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 net/queue.c | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/net/queue.c b/net/queue.c
index 58222b0..9c343ab 100644
--- a/net/queue.c
+++ b/net/queue.c
@@ -24,6 +24,8 @@
 #include "net/queue.h"
 #include "qemu/queue.h"
 #include "net/net.h"
+#include "block/aio.h"
+#include "qemu/main-loop.h"
 
 /* The delivery handler may only return zero if it will call
  * qemu_net_queue_flush() when it determines that it is once again able
@@ -183,6 +185,22 @@ static ssize_t qemu_net_queue_deliver_iov(NetQueue *queue,
     return ret;
 }
 
+typedef struct NetQueBH {
+    QEMUBH *bh;
+    NetClientState *nc;
+} NetQueBH;
+
+static void qemu_net_queue_send_bh(void *opaque)
+{
+    NetQueBH *q_bh = opaque;
+    NetQueue *queue = q_bh->nc->send_queue;
+
+    qemu_net_queue_flush(queue);
+    netclient_unref(q_bh->nc);
+    qemu_bh_delete(q_bh->bh);
+    g_slice_free(NetQueBH, q_bh);
+}
+
 ssize_t qemu_net_queue_send(NetQueue *queue,
                             NetClientState *sender,
                             unsigned flags,
@@ -192,8 +210,17 @@ ssize_t qemu_net_queue_send(NetQueue *queue,
 {
     ssize_t ret;
 
-    if (queue->delivering || !qemu_can_send_packet_nolock(sender)) {
+    if (queue->delivering || !qemu_can_send_packet_nolock(sender)
+        || sender->send_queue->delivering) {
         qemu_net_queue_append(queue, sender, flags, data, size, sent_cb);
+        /* Nested call will be deferred to BH */
+        if (sender->send_queue->delivering) {
+            NetQueBH *que_bh = g_slice_new(NetQueBH);
+            que_bh->bh = qemu_bh_new(qemu_net_queue_send_bh, que_bh);
+            que_bh->nc = queue->opaque;
+            netclient_ref(queue->opaque);
+            qemu_bh_schedule(que_bh->bh);
+        }
         return 0;
     }
 
@@ -217,8 +244,17 @@ ssize_t qemu_net_queue_send_iov(NetQueue *queue,
 {
     ssize_t ret;
 
-    if (queue->delivering || !qemu_can_send_packet_nolock(sender)) {
+    if (queue->delivering || !qemu_can_send_packet_nolock(sender)
+        || sender->send_queue->delivering) {
         qemu_net_queue_append_iov(queue, sender, flags, iov, iovcnt, sent_cb);
+        /* Nested call will be deferred to BH */
+        if (sender->send_queue->delivering) {
+            NetQueBH *que_bh = g_slice_new(NetQueBH);
+            que_bh->bh = qemu_bh_new(qemu_net_queue_send_bh, que_bh);
+            que_bh->nc = queue->opaque;
+            netclient_ref(queue->opaque);
+            qemu_bh_schedule(que_bh->bh);
+        }
         return 0;
     }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH v2 6/6] net: hub use lock to protect ports list

[Liu Ping Fan]

From: Liu Ping Fan <pingfanl@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 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/net/hub.c b/net/hub.c
index 9c6c559..2970f8e 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,14 +207,17 @@ 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) {
                     netclient_ref(peer);
+                    qemu_mutex_unlock(&hub->ports_lock);
                     return peer;
                 }
             }
+            qemu_mutex_unlock(&hub->ports_lock);
         }
     }
     return NULL;
@@ -221,13 +234,16 @@ 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) {
                     netclient_ref(&port->nc);
+                    qemu_mutex_unlock(&hub->ports_lock);
                     return &(port->nc);
                 }
             }
+            qemu_mutex_unlock(&hub->ports_lock);
             break;
         }
     }
@@ -247,12 +263,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);
     }
 }
 
@@ -309,6 +327,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) {
@@ -331,6 +350,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);
         }
@@ -346,12 +366,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.8.1.4

Re: [Qemu-devel] [PATCH v2 2/6] net: introduce lock to protect NetClientState's peer's access

[Stefan Hajnoczi]

On Thu, Jun 13, 2013 at 05:03:02PM +0800, Liu Ping Fan wrote:
> @@ -67,6 +67,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.
> +         */

Indentation

> @@ -301,6 +303,38 @@ static void qemu_free_net_client(NetClientState *nc)
>      }
>  }
>  
> +/* elimate the reference and sync with exit of rx/tx action.

s/elimate/Eliminate/

> + * 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.
> +         */

Indentation, also please resolve the fixme.

> @@ -394,6 +433,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;
> +    }

Just call qemu_can_send_packet_nolock() instead of duplicating code?

Re: [Qemu-devel] [PATCH v2 3/6] net: make netclient re-entrant with refcnt

[Stefan Hajnoczi]

On Thu, Jun 13, 2013 at 05:03:03PM +0800, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> With refcnt, NetClientState's user can run agaist deleter.

Please split this into two patches:

1. net_clients lock
2. NetClientState refcount

> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/core/qdev-properties-system.c | 14 ++++++++++++
>  include/net/net.h                |  3 +++
>  net/hub.c                        |  3 +++
>  net/net.c                        | 47 +++++++++++++++++++++++++++++++++++++---
>  net/slirp.c                      |  3 ++-
>  5 files changed, 66 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index 0eada32..41cc7e6 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -302,6 +302,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,
> @@ -311,11 +312,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,

What about the netdev property?  I don't see any refcount code there.

> @@ -1109,6 +1146,7 @@ void net_cleanup(void)
>              qemu_del_net_client(nc);
>          }
>      }
> +    qemu_mutex_destroy(&net_clients_lock);

Why is it okay to iterate over net_clients here without the lock?

Re: [Qemu-devel] [PATCH v2 4/6] net: force NetQue opaque to be NetClientState

[Stefan Hajnoczi]

On Thu, Jun 13, 2013 at 05:03:04PM +0800, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfanl@linux.vnet.ibm.com>
> 
> qemu_net_client_setup() is the only user of qemu_new_net_queue(), which
> will pass in NetClientState. By forcing it be a NetClientState, we
> can ref/unref NetQueue's owner

Please s/opaque/nc/ in net/queue.[hc] since it's no longer "opaque" :).

Also, qemu_deliver_packet()/qemu_deliver_packet_iov() can take an
NetClientState *nc instead of void *opaque.

> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

pingfank here and pingfanl in the From: header.  Are both okay and which
do you prefer to use?

Re: [Qemu-devel] [PATCH v2 5/6] net: defer nested call to BH

[Stefan Hajnoczi]

On Thu, Jun 13, 2013 at 05:03:05PM +0800, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfanl@linux.vnet.ibm.com>
> 
> Nested call caused by ->receive() will raise issue like deadlock,
> so postphone it to BH.
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  net/queue.c | 40 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)

Does this patch belong before the netqueue lock patch?  The commit
history should be bisectable without temporary failures/deadlocks.

> diff --git a/net/queue.c b/net/queue.c
> index 58222b0..9c343ab 100644
> --- a/net/queue.c
> +++ b/net/queue.c
> @@ -24,6 +24,8 @@
>  #include "net/queue.h"
>  #include "qemu/queue.h"
>  #include "net/net.h"
> +#include "block/aio.h"
> +#include "qemu/main-loop.h"
>  
>  /* The delivery handler may only return zero if it will call
>   * qemu_net_queue_flush() when it determines that it is once again able
> @@ -183,6 +185,22 @@ static ssize_t qemu_net_queue_deliver_iov(NetQueue *queue,
>      return ret;
>  }
>  
> +typedef struct NetQueBH {

This file uses "Queue" consistently, please don't add "Que" here.

> @@ -192,8 +210,17 @@ ssize_t qemu_net_queue_send(NetQueue *queue,
>  {
>      ssize_t ret;
>  
> -    if (queue->delivering || !qemu_can_send_packet_nolock(sender)) {
> +    if (queue->delivering || !qemu_can_send_packet_nolock(sender)
> +        || sender->send_queue->delivering) {

Not sure this is safe, we're only holding one NetClientState->peer_lock
and one NetQueue->lock.  How can we access both queue->delivering and
sender->send_queue->delivering safely?

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

[Stefan Hajnoczi]

On Thu, Jun 13, 2013 at 05:03:00PM +0800, Liu Ping Fan wrote:
> Currently, I drop slirp and frontend related code (I guess I can reuse some
> code by mdroth's QContext later). And this series only resolve the net core
> re-entrant problem.
> 
> v1->v2:
>   introduce netqueue re-entrant detection and defer it to BH
> 
> Liu Ping Fan (6):
>   net: introduce lock to protect NetQueue
>   net: introduce lock to protect NetClientState's peer's access
>   net: make netclient re-entrant with refcnt
>   net: force NetQue opaque to be NetClientState
>   net: defer nested call to BH
>   net: hub use lock to protect ports list
> 
>  hw/core/qdev-properties-system.c |  14 +++++
>  include/net/net.h                |  10 ++++
>  include/net/queue.h              |   2 +-
>  net/hub.c                        |  28 ++++++++-
>  net/net.c                        | 124 +++++++++++++++++++++++++++++++++++++--
>  net/queue.c                      |  57 ++++++++++++++++--
>  net/slirp.c                      |   3 +-
>  7 files changed, 225 insertions(+), 13 deletions(-)

I've replied with comments for the obvious issues I noticed.  For the
next revision I'll apply the patches and do a global review of the
rx/tx/add/del paths because it's hard to really review this
patch-by-patch.

Re: [Qemu-devel] [PATCH v2 2/6] net: introduce lock to protect NetClientState's peer's access

[liu ping fan]

On Tue, Jun 18, 2013 at 8:25 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Jun 13, 2013 at 05:03:02PM +0800, Liu Ping Fan wrote:
>> @@ -67,6 +67,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.
>> +         */
>
> Indentation
>
Will fix.
>> @@ -301,6 +303,38 @@ static void qemu_free_net_client(NetClientState *nc)
>>      }
>>  }
>>
>> +/* elimate the reference and sync with exit of rx/tx action.
>
> s/elimate/Eliminate/
>
Will fix
>> + * 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.
>> +         */
>
> Indentation, also please resolve the fixme.
>
So, here can I take the assumption that the deleters are serialized by
biglock, and remove the lock following this comment?

>> @@ -394,6 +433,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;
>> +    }
>
> Just call qemu_can_send_packet_nolock() instead of duplicating code?

Yes.

Thx & regards,
pingfan

Re: [Qemu-devel] [PATCH v2 4/6] net: force NetQue opaque to be NetClientState

[liu ping fan]

On Tue, Jun 18, 2013 at 8:47 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Jun 13, 2013 at 05:03:04PM +0800, Liu Ping Fan wrote:
>> From: Liu Ping Fan <pingfanl@linux.vnet.ibm.com>
>>
>> qemu_net_client_setup() is the only user of qemu_new_net_queue(), which
>> will pass in NetClientState. By forcing it be a NetClientState, we
>> can ref/unref NetQueue's owner
>
> Please s/opaque/nc/ in net/queue.[hc] since it's no longer "opaque" :).
>
Ok, I will.
> Also, qemu_deliver_packet()/qemu_deliver_packet_iov() can take an
> NetClientState *nc instead of void *opaque.
>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>
> pingfank here and pingfanl in the From: header.  Are both okay and which
> do you prefer to use?

Change my disk, totally re-install my system, and mix up with internal
mail address. Will fix it.

Thx & regards,
Pingfan

Re: [Qemu-devel] [PATCH v2 5/6] net: defer nested call to BH

[liu ping fan]

On Tue, Jun 18, 2013 at 8:57 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Jun 13, 2013 at 05:03:05PM +0800, Liu Ping Fan wrote:
>> From: Liu Ping Fan <pingfanl@linux.vnet.ibm.com>
>>
>> Nested call caused by ->receive() will raise issue like deadlock,
>> so postphone it to BH.
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  net/queue.c | 40 ++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 38 insertions(+), 2 deletions(-)
>
> Does this patch belong before the netqueue lock patch?  The commit
> history should be bisectable without temporary failures/deadlocks.
>
Ok.
>> diff --git a/net/queue.c b/net/queue.c
>> index 58222b0..9c343ab 100644
>> --- a/net/queue.c
>> +++ b/net/queue.c
>> @@ -24,6 +24,8 @@
>>  #include "net/queue.h"
>>  #include "qemu/queue.h"
>>  #include "net/net.h"
>> +#include "block/aio.h"
>> +#include "qemu/main-loop.h"
>>
>>  /* The delivery handler may only return zero if it will call
>>   * qemu_net_queue_flush() when it determines that it is once again able
>> @@ -183,6 +185,22 @@ static ssize_t qemu_net_queue_deliver_iov(NetQueue *queue,
>>      return ret;
>>  }
>>
>> +typedef struct NetQueBH {
>
> This file uses "Queue" consistently, please don't add "Que" here.
>
>> @@ -192,8 +210,17 @@ ssize_t qemu_net_queue_send(NetQueue *queue,
>>  {
>>      ssize_t ret;
>>
>> -    if (queue->delivering || !qemu_can_send_packet_nolock(sender)) {
>> +    if (queue->delivering || !qemu_can_send_packet_nolock(sender)
>> +        || sender->send_queue->delivering) {
>
> Not sure this is safe, we're only holding one NetClientState->peer_lock
> and one NetQueue->lock.  How can we access both queue->delivering and
> sender->send_queue->delivering safely?

Yes, you are right, it is not safely. The queue->delivering is
protected by peer_lock and we do not take the verse direction lock .
So finally the above code can not tell out the nested calling
"A-->B-->A"  from  "A-->B,  B-->A" (where A, B stands for a
NetClientState).
What about using TLS to trace the nested calling?  With it, we can
avoid AB-BA lock problem.

Thx & regards,
Pingfan

Re: [Qemu-devel] [PATCH v2 2/6] net: introduce lock to protect NetClientState's peer's access

[Stefan Hajnoczi]

On Thu, Jun 20, 2013 at 02:30:30PM +0800, liu ping fan wrote:
> On Tue, Jun 18, 2013 at 8:25 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Thu, Jun 13, 2013 at 05:03:02PM +0800, Liu Ping Fan wrote:
> >> + * 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.
> >> +         */
> >
> > Indentation, also please resolve the fixme.
> >
> So, here can I take the assumption that the deleters are serialized by
> biglock, and remove the lock following this comment?

Ah, I understand the comment now.  Is there any advantage to dropping
the lock?  IMO it's clearer to take the lock consistently instead of
"optimizing" cases we think only get called from the main loop.

Re: [Qemu-devel] [PATCH v2 5/6] net: defer nested call to BH

[Stefan Hajnoczi]

On Thu, Jun 20, 2013 at 02:30:56PM +0800, liu ping fan wrote:
> On Tue, Jun 18, 2013 at 8:57 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Thu, Jun 13, 2013 at 05:03:05PM +0800, Liu Ping Fan wrote:
> >> From: Liu Ping Fan <pingfanl@linux.vnet.ibm.com>
> >>
> >> Nested call caused by ->receive() will raise issue like deadlock,
> >> so postphone it to BH.
> >>
> >> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> >> ---
> >>  net/queue.c | 40 ++++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 38 insertions(+), 2 deletions(-)
> >
> > Does this patch belong before the netqueue lock patch?  The commit
> > history should be bisectable without temporary failures/deadlocks.
> >
> Ok.
> >> diff --git a/net/queue.c b/net/queue.c
> >> index 58222b0..9c343ab 100644
> >> --- a/net/queue.c
> >> +++ b/net/queue.c
> >> @@ -24,6 +24,8 @@
> >>  #include "net/queue.h"
> >>  #include "qemu/queue.h"
> >>  #include "net/net.h"
> >> +#include "block/aio.h"
> >> +#include "qemu/main-loop.h"
> >>
> >>  /* The delivery handler may only return zero if it will call
> >>   * qemu_net_queue_flush() when it determines that it is once again able
> >> @@ -183,6 +185,22 @@ static ssize_t qemu_net_queue_deliver_iov(NetQueue *queue,
> >>      return ret;
> >>  }
> >>
> >> +typedef struct NetQueBH {
> >
> > This file uses "Queue" consistently, please don't add "Que" here.
> >
> >> @@ -192,8 +210,17 @@ ssize_t qemu_net_queue_send(NetQueue *queue,
> >>  {
> >>      ssize_t ret;
> >>
> >> -    if (queue->delivering || !qemu_can_send_packet_nolock(sender)) {
> >> +    if (queue->delivering || !qemu_can_send_packet_nolock(sender)
> >> +        || sender->send_queue->delivering) {
> >
> > Not sure this is safe, we're only holding one NetClientState->peer_lock
> > and one NetQueue->lock.  How can we access both queue->delivering and
> > sender->send_queue->delivering safely?
> 
> Yes, you are right, it is not safely. The queue->delivering is
> protected by peer_lock and we do not take the verse direction lock .
> So finally the above code can not tell out the nested calling
> "A-->B-->A"  from  "A-->B,  B-->A" (where A, B stands for a
> NetClientState).
> What about using TLS to trace the nested calling?  With it, we can
> avoid AB-BA lock problem.

I would take a step back and see if there's a way to avoid reaching into
inspect sender->send_queue->delivering here.

Stefan

Re: [Qemu-devel] [PATCH v2 3/6] net: make netclient re-entrant with refcnt

[liu ping fan]

On Tue, Jun 18, 2013 at 8:41 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Jun 13, 2013 at 05:03:03PM +0800, Liu Ping Fan wrote:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> With refcnt, NetClientState's user can run agaist deleter.
>
> Please split this into two patches:
>
> 1. net_clients lock
> 2. NetClientState refcount
>
Ok.
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  hw/core/qdev-properties-system.c | 14 ++++++++++++
>>  include/net/net.h                |  3 +++
>>  net/hub.c                        |  3 +++
>>  net/net.c                        | 47 +++++++++++++++++++++++++++++++++++++---
>>  net/slirp.c                      |  3 ++-
>>  5 files changed, 66 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
>> index 0eada32..41cc7e6 100644
>> --- a/hw/core/qdev-properties-system.c
>> +++ b/hw/core/qdev-properties-system.c
>> @@ -302,6 +302,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,
>> @@ -311,11 +312,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,
>
> What about the netdev property?  I don't see any refcount code there.
>
Yes, the release of netdev and vlan property should all free its
backend. Will add the code.
>> @@ -1109,6 +1146,7 @@ void net_cleanup(void)
>>              qemu_del_net_client(nc);
>>          }
>>      }
>> +    qemu_mutex_destroy(&net_clients_lock);
>
> Why is it okay to iterate over net_clients here without the lock?

 atexit(&net_cleanup); So no other racers exist.

Thx & Regards,
Pingfan

Re: [Qemu-devel] [PATCH v2 2/6] net: introduce lock to protect NetClientState's peer's access

[liu ping fan]

On Thu, Jun 20, 2013 at 3:46 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Thu, Jun 20, 2013 at 02:30:30PM +0800, liu ping fan wrote:
>> On Tue, Jun 18, 2013 at 8:25 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> > On Thu, Jun 13, 2013 at 05:03:02PM +0800, Liu Ping Fan wrote:
>> >> + * 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.
>> >> +         */
>> >
>> > Indentation, also please resolve the fixme.
>> >
>> So, here can I take the assumption that the deleters are serialized by
>> biglock, and remove the lock following this comment?
>
> Ah, I understand the comment now.  Is there any advantage to dropping

:), only two atomic instruction in rare path.
> the lock?  IMO it's clearer to take the lock consistently instead of
> "optimizing" cases we think only get called from the main loop.

Reasonable, will keep them.

Re: [Qemu-devel] [PATCH v2 3/6] net: make netclient re-entrant with refcnt

[Stefan Hajnoczi]

On Thu, Jun 20, 2013 at 05:14:56PM +0800, liu ping fan wrote:
> On Tue, Jun 18, 2013 at 8:41 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Thu, Jun 13, 2013 at 05:03:03PM +0800, Liu Ping Fan wrote:
> >> @@ -1109,6 +1146,7 @@ void net_cleanup(void)
> >>              qemu_del_net_client(nc);
> >>          }
> >>      }
> >> +    qemu_mutex_destroy(&net_clients_lock);
> >
> > Why is it okay to iterate over net_clients here without the lock?
> 
>  atexit(&net_cleanup); So no other racers exist.

What about dataplane?  The device may not be reset when net_cleanup runs.

It's best not to make assumptions, taking the lock is easy.

Stefan

Re: [Qemu-devel] [PATCH v2 3/6] net: make netclient re-entrant with refcnt

[liu ping fan]

On Mon, Jul 1, 2013 at 7:50 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Jun 20, 2013 at 05:14:56PM +0800, liu ping fan wrote:
>> On Tue, Jun 18, 2013 at 8:41 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> > On Thu, Jun 13, 2013 at 05:03:03PM +0800, Liu Ping Fan wrote:
>> >> @@ -1109,6 +1146,7 @@ void net_cleanup(void)
>> >>              qemu_del_net_client(nc);
>> >>          }
>> >>      }
>> >> +    qemu_mutex_destroy(&net_clients_lock);
>> >
>> > Why is it okay to iterate over net_clients here without the lock?
>>
>>  atexit(&net_cleanup); So no other racers exist.
>
> What about dataplane?  The device may not be reset when net_cleanup runs.
>
Does the func registered by atexit run after all of the other threads terminate?
> It's best not to make assumptions, taking the lock is easy.
>
Yes, assumptions are not reliable. I will take the lock for the next version.

Thx & regards,
Pingfan
> Stefan

Re: [Qemu-devel] [PATCH v2 5/6] net: defer nested call to BH

[Paolo Bonzini]

Il 13/06/2013 11:03, Liu Ping Fan ha scritto:
> From: Liu Ping Fan <pingfanl@linux.vnet.ibm.com>
> 
> Nested call caused by ->receive() will raise issue like deadlock,
> so postphone it to BH.

When I read this patch, I am completely puzzled.

All I see is that  a qemu_net_queue_flush is done in a bottom half.  I
have no clue of how you get a nested call of ->receive, and I have no
clue of what this patch does to prevent it (since the bottom half is
asynchronous).

A bottom half is not a synchronization primitive.  If you use a lock, or
a condition variable, or RCU, you can usually assume that the reader
understands how you're using it (though not if you're doing fancy
tricks).  If you are introducing infrastructure, you can use long
comments or docs/ and expect the reviewer to read those.

But if you're doing tricky stuff (such as if you have global/local
locks, or you have parts of the code that run lockless, or if you are
modifying the behavior of an existing subsystem to prevent deadlocks),
you need to document _every single step that you do_.

For example, let's take the patches you had for hostmem.c.  You had a
single patch doing all the work:

   hostmem: make hostmem global and RAM hotunplg safe

   The hostmem listener will translate gpa to hva quickly. Make it
   global, so other component can use it.
   Also this patch adopt MemoryRegionOps's ref/unref interface to
   make it survive the RAM hotunplug.

No reference whatsoever to the reference counting of the hostmem itself,
and how you are using the locks.  The "also" in the commit message is a
big warning sign, too (though I'm guilty of adding many "also"s in the
commit messages).

When I took your ideas and applied them to memory.c, here is how I
explained it:

   memory: use a new FlatView pointer on every topology update

   This is the first step towards converting as->current_map to
   RCU-style updates, where the FlatView updates run concurrently
   with uses of an old FlatView.
   ---
   memory: add reference counting to FlatView

   With this change, a FlatView can be used even after a concurrent
   update has replaced it.  Because we do not have RCU, we use a
   mutex to protect the small critical sections that read/write the
   as->current_map pointer.  Accesses to the FlatView can be done
   outside the mutex.

And in the code:

   /* flat_view_mutex is taken around reading as->current_map; the
    * critical section is extremely short, so I'm using a single mutex
    * for every AS.  We could also RCU for the read-side.
    *
    * The BQL is taken around transaction commits, hence both locks are
    * taken while writing to as->current_map (with the BQL taken
    * outside).
    */

It is still quite concise, but it explains both the concurrency to
expect, and the locking policies.

This patch is changing the behavior of existing code in a complex
scenario and in a tricky way.  If you want your patches accepted (or
even reviewed), you _must_ learn how to explain the scenarios and your
fixes.

And believe me, it is not a language problem; people with much worse
English than yours manage this all the time.  It is more of an attitude
problem, and I've explained it many times.

Paolo

> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  net/queue.c | 40 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/net/queue.c b/net/queue.c
> index 58222b0..9c343ab 100644
> --- a/net/queue.c
> +++ b/net/queue.c
> @@ -24,6 +24,8 @@
>  #include "net/queue.h"
>  #include "qemu/queue.h"
>  #include "net/net.h"
> +#include "block/aio.h"
> +#include "qemu/main-loop.h"
>  
>  /* The delivery handler may only return zero if it will call
>   * qemu_net_queue_flush() when it determines that it is once again able
> @@ -183,6 +185,22 @@ static ssize_t qemu_net_queue_deliver_iov(NetQueue *queue,
>      return ret;
>  }
>  
> +typedef struct NetQueBH {
> +    QEMUBH *bh;
> +    NetClientState *nc;
> +} NetQueBH;
> +
> +static void qemu_net_queue_send_bh(void *opaque)
> +{
> +    NetQueBH *q_bh = opaque;
> +    NetQueue *queue = q_bh->nc->send_queue;
> +
> +    qemu_net_queue_flush(queue);
> +    netclient_unref(q_bh->nc);
> +    qemu_bh_delete(q_bh->bh);
> +    g_slice_free(NetQueBH, q_bh);
> +}
> +
>  ssize_t qemu_net_queue_send(NetQueue *queue,
>                              NetClientState *sender,
>                              unsigned flags,
> @@ -192,8 +210,17 @@ ssize_t qemu_net_queue_send(NetQueue *queue,
>  {
>      ssize_t ret;
>  
> -    if (queue->delivering || !qemu_can_send_packet_nolock(sender)) {
> +    if (queue->delivering || !qemu_can_send_packet_nolock(sender)
> +        || sender->send_queue->delivering) {
>          qemu_net_queue_append(queue, sender, flags, data, size, sent_cb);
> +        /* Nested call will be deferred to BH */
> +        if (sender->send_queue->delivering) {
> +            NetQueBH *que_bh = g_slice_new(NetQueBH);
> +            que_bh->bh = qemu_bh_new(qemu_net_queue_send_bh, que_bh);
> +            que_bh->nc = queue->opaque;
> +            netclient_ref(queue->opaque);
> +            qemu_bh_schedule(que_bh->bh);
> +        }
>          return 0;
>      }
>  
> @@ -217,8 +244,17 @@ ssize_t qemu_net_queue_send_iov(NetQueue *queue,
>  {
>      ssize_t ret;
>  
> -    if (queue->delivering || !qemu_can_send_packet_nolock(sender)) {
> +    if (queue->delivering || !qemu_can_send_packet_nolock(sender)
> +        || sender->send_queue->delivering) {
>          qemu_net_queue_append_iov(queue, sender, flags, iov, iovcnt, sent_cb);
> +        /* Nested call will be deferred to BH */
> +        if (sender->send_queue->delivering) {
> +            NetQueBH *que_bh = g_slice_new(NetQueBH);
> +            que_bh->bh = qemu_bh_new(qemu_net_queue_send_bh, que_bh);
> +            que_bh->nc = queue->opaque;
> +            netclient_ref(queue->opaque);
> +            qemu_bh_schedule(que_bh->bh);
> +        }
>          return 0;
>      }
>  
> 

Re: [Qemu-devel] [PATCH v2 3/6] net: make netclient re-entrant with refcnt

[Stefan Hajnoczi]

On Wed, Jul 03, 2013 at 11:41:19AM +0800, liu ping fan wrote:
> On Mon, Jul 1, 2013 at 7:50 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Thu, Jun 20, 2013 at 05:14:56PM +0800, liu ping fan wrote:
> >> On Tue, Jun 18, 2013 at 8:41 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >> > On Thu, Jun 13, 2013 at 05:03:03PM +0800, Liu Ping Fan wrote:
> >> >> @@ -1109,6 +1146,7 @@ void net_cleanup(void)
> >> >>              qemu_del_net_client(nc);
> >> >>          }
> >> >>      }
> >> >> +    qemu_mutex_destroy(&net_clients_lock);
> >> >
> >> > Why is it okay to iterate over net_clients here without the lock?
> >>
> >>  atexit(&net_cleanup); So no other racers exist.
> >
> > What about dataplane?  The device may not be reset when net_cleanup runs.
> >
> Does the func registered by atexit run after all of the other threads terminate?

I imagine that atexit(3) runs while detached threads are still alive,
but I'm not sure about the exact rules.  The pthread specification links
I found online didn't state the rules.

Stefan

Re: [Qemu-devel] [PATCH v2 3/6] net: make netclient re-entrant with refcnt

[liu ping fan]

On Wed, Jul 3, 2013 at 3:49 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Wed, Jul 03, 2013 at 11:41:19AM +0800, liu ping fan wrote:
>> On Mon, Jul 1, 2013 at 7:50 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> > On Thu, Jun 20, 2013 at 05:14:56PM +0800, liu ping fan wrote:
>> >> On Tue, Jun 18, 2013 at 8:41 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> >> > On Thu, Jun 13, 2013 at 05:03:03PM +0800, Liu Ping Fan wrote:
>> >> >> @@ -1109,6 +1146,7 @@ void net_cleanup(void)
>> >> >>              qemu_del_net_client(nc);
>> >> >>          }
>> >> >>      }
>> >> >> +    qemu_mutex_destroy(&net_clients_lock);
>> >> >
>> >> > Why is it okay to iterate over net_clients here without the lock?
>> >>
>> >>  atexit(&net_cleanup); So no other racers exist.
>> >
>> > What about dataplane?  The device may not be reset when net_cleanup runs.
>> >
>> Does the func registered by atexit run after all of the other threads terminate?
>
> I imagine that atexit(3) runs while detached threads are still alive,
> but I'm not sure about the exact rules.  The pthread specification links
> I found online didn't state the rules.
>
Haha, finally, got some hint for this.  pthread_exit(3) says:
       After  the  last  thread  in  a  process terminates, the
process terminates as by calling exit(3) with an exit status of zero;
thus, process-shared
       resources are released and functions registered using atexit(3)
are called.

Regards,
Pingfan

> Stefan

Re: [Qemu-devel] [PATCH v2 3/6] net: make netclient re-entrant with refcnt

[Stefan Hajnoczi]

On Wed, Jul 03, 2013 at 03:54:44PM +0800, liu ping fan wrote:
> On Wed, Jul 3, 2013 at 3:49 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Wed, Jul 03, 2013 at 11:41:19AM +0800, liu ping fan wrote:
> >> On Mon, Jul 1, 2013 at 7:50 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >> > On Thu, Jun 20, 2013 at 05:14:56PM +0800, liu ping fan wrote:
> >> >> On Tue, Jun 18, 2013 at 8:41 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >> >> > On Thu, Jun 13, 2013 at 05:03:03PM +0800, Liu Ping Fan wrote:
> >> >> >> @@ -1109,6 +1146,7 @@ void net_cleanup(void)
> >> >> >>              qemu_del_net_client(nc);
> >> >> >>          }
> >> >> >>      }
> >> >> >> +    qemu_mutex_destroy(&net_clients_lock);
> >> >> >
> >> >> > Why is it okay to iterate over net_clients here without the lock?
> >> >>
> >> >>  atexit(&net_cleanup); So no other racers exist.
> >> >
> >> > What about dataplane?  The device may not be reset when net_cleanup runs.
> >> >
> >> Does the func registered by atexit run after all of the other threads terminate?
> >
> > I imagine that atexit(3) runs while detached threads are still alive,
> > but I'm not sure about the exact rules.  The pthread specification links
> > I found online didn't state the rules.
> >
> Haha, finally, got some hint for this.  pthread_exit(3) says:
>        After  the  last  thread  in  a  process terminates, the
> process terminates as by calling exit(3) with an exit status of zero;
> thus, process-shared
>        resources are released and functions registered using atexit(3)
> are called.

That's only true for non-detached threads.  A program can exit while
detached threads are running.

Stefan