Patchwork [3/3] net: make netclient re-entrant with refcnt

login
register
mail settings
Submitter pingfan liu
Date March 3, 2013, 1:21 p.m.
Message ID <1362316883-7948-4-git-send-email-qemulist@gmail.com>
Download mbox | patch
Permalink /patch/224552/
State New
Headers show

Comments

pingfan liu - March 3, 2013, 1:21 p.m.
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

With refcnt, NetClientState's caller can run agaist reclaimer.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/qdev-properties-system.c |   14 ++++++++++++++
 include/net/net.h           |    3 +++
 net/hub.c                   |   29 ++++++++++++++++++++++++++++-
 net/net.c                   |   40 +++++++++++++++++++++++++++++++---------
 net/slirp.c                 |    3 ++-
 5 files changed, 78 insertions(+), 11 deletions(-)
Stefan Hajnoczi - March 4, 2013, 3:19 p.m.
On Sun, Mar 03, 2013 at 09:21:22PM +0800, Liu Ping Fan wrote:
> diff --git a/net/hub.c b/net/hub.c
> index 97c3ac3..ab4448e 100644
> --- a/net/hub.c
> +++ b/net/hub.c
> @@ -36,6 +36,7 @@ typedef struct NetHubPort {
>  } NetHubPort;
>  
>  struct NetHub {
> +    QemuMutex lock;

Please document what this lock protects.  I think it only protects the
ports list.

> diff --git a/net/net.c b/net/net.c
> index 0acb933..5a8bb6a 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;

I'm not sure what net_clients_lock protects since this lock is not taken
by all net_clients users.

The lock should be destroyed in net_cleanup().

This should be a separate patch that fully explains the purpose of the
lock and uses it consistently in net.c.
pingfan liu - March 5, 2013, 2:48 a.m.
On Mon, Mar 4, 2013 at 11:19 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Sun, Mar 03, 2013 at 09:21:22PM +0800, Liu Ping Fan wrote:
>> diff --git a/net/hub.c b/net/hub.c
>> index 97c3ac3..ab4448e 100644
>> --- a/net/hub.c
>> +++ b/net/hub.c
>> @@ -36,6 +36,7 @@ typedef struct NetHubPort {
>>  } NetHubPort;
>>
>>  struct NetHub {
>> +    QemuMutex lock;
>
> Please document what this lock protects.  I think it only protects the
> ports list.
>
OK
>> diff --git a/net/net.c b/net/net.c
>> index 0acb933..5a8bb6a 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;
>
> I'm not sure what net_clients_lock protects since this lock is not taken
> by all net_clients users.
>
Oh, some made mistake when rebasing. Will fix it.
> The lock should be destroyed in net_cleanup().
>
OK
> This should be a separate patch that fully explains the purpose of the
> lock and uses it consistently in net.c.
OK

Thanks and regards,
Pingfan

Patch

diff --git a/hw/qdev-properties-system.c b/hw/qdev-properties-system.c
index 88f0acf..b27cc16 100644
--- a/hw/qdev-properties-system.c
+++ b/hw/qdev-properties-system.c
@@ -301,6 +301,7 @@  static void set_vlan(Object *obj, Visitor *v, void *opaque,
         return;
     }
 
+    /* inc ref, released when unset property */
     hubport = net_hub_port_find(id);
     if (!hubport) {
         error_set(errp, QERR_INVALID_PARAMETER_VALUE,
@@ -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 3e4b9df..fd1eda6 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -63,6 +63,7 @@  typedef struct NetClientInfo {
 } NetClientInfo;
 
 struct NetClientState {
+    int ref;
     /* protect peer's send_queue */
     QemuMutex transfer_lock;
     NetClientInfo *info;
@@ -89,6 +90,8 @@  typedef struct NICState {
 NetClientState *qemu_find_netdev(const char *id);
 int qemu_find_net_clients_except(const char *id, NetClientState **ncs,
                                  NetClientOptionsKind type, int max);
+void netclient_ref(NetClientState *nc);
+void netclient_unref(NetClientState *nc);
 NetClientState *qemu_new_net_client(NetClientInfo *info,
                                     NetClientState *peer,
                                     const char *model,
diff --git a/net/hub.c b/net/hub.c
index 97c3ac3..ab4448e 100644
--- a/net/hub.c
+++ b/net/hub.c
@@ -36,6 +36,7 @@  typedef struct NetHubPort {
 } NetHubPort;
 
 struct NetHub {
+    QemuMutex lock;
     int id;
     QLIST_ENTRY(NetHub) next;
     int num_ports;
@@ -49,6 +50,7 @@  static ssize_t net_hub_receive(NetHub *hub, NetHubPort *source_port,
 {
     NetHubPort *port;
 
+    qemu_mutex_lock(&hub->lock);
     QLIST_FOREACH(port, &hub->ports, next) {
         if (port == source_port) {
             continue;
@@ -63,6 +65,7 @@  static ssize_t net_hub_receive(NetHub *hub, NetHubPort *source_port,
         qemu_mutex_unlock(&port->nc.transfer_lock);
         event_notifier_set(&port->e);
     }
+    qemu_mutex_unlock(&hub->lock);
     return len;
 }
 
@@ -85,6 +88,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->lock);
     QLIST_FOREACH(port, &hub->ports, next) {
         if (port == source_port) {
             continue;
@@ -100,6 +104,8 @@  static ssize_t net_hub_receive_iov(NetHub *hub, NetHubPort *source_port,
         event_notifier_set(&port->e);
 
     }
+    qemu_mutex_unlock(&hub->lock);
+
     return len;
 }
 
@@ -111,7 +117,7 @@  static NetHub *net_hub_new(int id)
     hub->id = id;
     hub->num_ports = 0;
     QLIST_INIT(&hub->ports);
-
+    qemu_mutex_init(&hub->lock);
     QLIST_INSERT_HEAD(&hubs, hub, next);
 
     return hub;
@@ -123,15 +129,18 @@  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->lock);
     QLIST_FOREACH(port, &hub->ports, next) {
         if (port == src_port) {
             continue;
         }
 
         if (qemu_can_send_packet(&port->nc)) {
+            qemu_mutex_unlock(&hub->lock);
             return 1;
         }
     }
+    qemu_mutex_unlock(&hub->lock);
 
     return 0;
 }
@@ -160,7 +169,9 @@  static void net_hub_port_cleanup(NetClientState *nc)
         aio_set_fd_handler(port->ctx, event_notifier_get_fd(&port->e),
                         NULL, NULL, NULL, NULL);
     }
+    qemu_mutex_lock(&port->hub->lock);
     QLIST_REMOVE(port, next);
+    qemu_mutex_unlock(&port->hub->lock);
 }
 
 static void net_hub_port_reside(NetClientState *nc, AioContext *ctx)
@@ -200,7 +211,9 @@  static NetHubPort *net_hub_port_new(NetHub *hub, const char *name)
     port->id = id;
     port->hub = hub;
     event_notifier_init(&port->e, 0);
+    qemu_mutex_lock(&hub->lock);
     QLIST_INSERT_HEAD(&hub->ports, port, next);
+    qemu_mutex_unlock(&hub->lock);
 
     return port;
 }
@@ -241,13 +254,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->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->lock);
                     return peer;
                 }
             }
+            qemu_mutex_unlock(&hub->lock);
         }
     }
     return NULL;
@@ -264,17 +281,22 @@  NetClientState *net_hub_port_find(int hub_id)
 
     QLIST_FOREACH(hub, &hubs, next) {
         if (hub->id == hub_id) {
+            qemu_mutex_lock(&hub->lock);
             QLIST_FOREACH(port, &hub->ports, next) {
                 nc = port->nc.peer;
                 if (!nc) {
+                    netclient_ref(&port->nc);
+                    qemu_mutex_unlock(&hub->lock);
                     return &(port->nc);
                 }
             }
+            qemu_mutex_unlock(&hub->lock);
             break;
         }
     }
 
     nc = net_hub_add_port(hub_id, NULL);
+    netclient_ref(nc);
     return nc;
 }
 
@@ -288,12 +310,14 @@  void net_hub_info(Monitor *mon)
 
     QLIST_FOREACH(hub, &hubs, next) {
         monitor_printf(mon, "hub %d\n", hub->id);
+        qemu_mutex_lock(&hub->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->lock);
     }
 }
 
@@ -350,6 +374,7 @@  void net_hub_check_clients(void)
     QLIST_FOREACH(hub, &hubs, next) {
         int has_nic = 0, has_host_dev = 0;
 
+        qemu_mutex_lock(&hub->lock);
         QLIST_FOREACH(port, &hub->ports, next) {
             peer = port->nc.peer;
             if (!peer) {
@@ -372,6 +397,8 @@  void net_hub_check_clients(void)
                 break;
             }
         }
+        qemu_mutex_unlock(&hub->lock);
+
         if (has_host_dev && !has_nic) {
             fprintf(stderr, "Warning: vlan %d with no nics\n", hub->id);
         }
diff --git a/net/net.c b/net/net.c
index 0acb933..5a8bb6a 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;
@@ -206,12 +207,16 @@  static void qemu_net_client_setup(NetClientState *nc,
         assert(!peer->peer);
         nc->peer = peer;
         peer->peer = nc;
+        netclient_ref(nc);
+        netclient_ref(peer);
     }
     qemu_mutex_init(&nc->transfer_lock);
-    QTAILQ_INSERT_TAIL(&net_clients, nc, next);
-
     nc->send_queue = qemu_new_net_queue(nc);
     nc->destructor = destructor;
+
+    qemu_mutex_lock(&net_clients_lock);
+    QTAILQ_INSERT_TAIL(&net_clients, nc, next);
+    qemu_mutex_unlock(&net_clients_lock);
 }
 
 NetClientState *qemu_new_net_client(NetClientInfo *info,
@@ -224,6 +229,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);
 
@@ -299,9 +305,6 @@  static void qemu_free_net_client(NetClientState *nc)
     if (nc->send_queue) {
         qemu_del_net_queue(nc->send_queue);
     }
-    if (nc->peer) {
-        nc->peer->peer = NULL;
-    }
     g_free(nc->name);
     g_free(nc->model);
     if (nc->destructor) {
@@ -309,6 +312,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);
+    }
+}
+
 /* exclude race with rx/tx path, flush out peer's queue */
 static void qemu_flushout_net_client(NetClientState *nc)
 {
@@ -320,15 +335,17 @@  static void qemu_flushout_net_client(NetClientState *nc)
         qemu_mutex_lock(&peer->transfer_lock);
         peer->peer = NULL;
         qemu_mutex_unlock(&peer->transfer_lock);
+        netclient_unref(nc);
     }
 
-    /* sync on send from this nc */
+    /* sync on send, flush out packets on peer's queue */
     qemu_mutex_lock(&nc->transfer_lock);
     nc->peer = NULL;
     if (peer) {
         qemu_net_queue_purge(peer->send_queue, nc);
     }
     qemu_mutex_unlock(&nc->transfer_lock);
+    netclient_unref(peer);
 }
 
 void qemu_del_net_client(NetClientState *nc)
@@ -374,7 +391,7 @@  void qemu_del_net_client(NetClientState *nc)
     for (i = 0; i < queues; i++) {
         qemu_flushout_net_client(ncs[i]);
         qemu_cleanup_net_client(ncs[i]);
-        qemu_free_net_client(ncs[i]);
+        netclient_unref(ncs[i]);
     }
 }
 
@@ -385,7 +402,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]);
         }
     }
 
@@ -395,7 +412,7 @@  void qemu_del_nic(NICState *nic)
 
         qemu_flushout_net_client(nc);
         qemu_cleanup_net_client(nc);
-        qemu_free_net_client(nc);
+        netclient_unref(nc);
     }
 
 }
@@ -624,6 +641,8 @@  NetClientState *qemu_find_netdev(const char *id)
         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;
         }
     }
@@ -957,6 +976,7 @@  void net_host_device_remove(Monitor *mon, const QDict *qdict)
         return;
     }
     qemu_del_net_client(nc);
+    netclient_unref(nc);
 }
 
 void netdev_add(QemuOpts *opts, Error **errp)
@@ -1012,6 +1032,7 @@  void qmp_netdev_del(const char *id, Error **errp)
     }
 
     qemu_del_net_client(nc);
+    netclient_unref(nc);
     qemu_opts_del(opts);
 }
 
@@ -1186,6 +1207,7 @@  int net_init_clients(void)
 #endif
     }
 
+    qemu_mutex_init(&net_clients_lock);
     QTAILQ_INIT(&net_clients);
 
     if (qemu_opts_foreach(qemu_find_opts("netdev"), net_init_netdev, NULL, 1) == -1)
diff --git a/net/slirp.c b/net/slirp.c
index 4df550f..a6116d5 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -346,7 +346,7 @@  void net_slirp_hostfwd_remove(Monitor *mon, const QDict *qdict)
 
     err = slirp_remove_hostfwd(QTAILQ_FIRST(&slirp_stacks)->slirp, is_udp,
                                host_addr, host_port);
-
+    netclient_unref(&s->nc);
     monitor_printf(mon, "host forwarding rule for %s %s\n", src_str,
                    err ? "not found" : "removed");
     return;
@@ -437,6 +437,7 @@  void net_slirp_hostfwd_add(Monitor *mon, const QDict *qdict)
     }
     if (s) {
         slirp_hostfwd(s, redir_str, 0);
+        netclient_unref(&s->nc);
     }
 
 }