On Mon, Mar 4, 2013 at 10:49 PM, Stefan Hajnoczi <stefa...@gmail.com> wrote: > On Sun, Mar 03, 2013 at 09:21:21PM +0800, Liu Ping Fan wrote: >> From: Liu Ping Fan <pingf...@linux.vnet.ibm.com> >> >> Use nc->transfer_lock to protect the nc->peer->send_queue. All of the > > Please use consistent names: the lock protects ->send_queue so it's best > called send_queue_lock or send_lock. > OK. >> deleter and senders will sync on this lock, so we can also survive across >> unplug. >> >> Signed-off-by: Liu Ping Fan <pingf...@linux.vnet.ibm.com> >> --- >> include/net/net.h | 4 +++ >> include/net/queue.h | 1 + >> net/hub.c | 21 +++++++++++++- >> net/net.c | 72 >> ++++++++++++++++++++++++++++++++++++++++++++++++--- >> net/queue.c | 15 +++++++++- >> 5 files changed, 105 insertions(+), 8 deletions(-) >> >> diff --git a/include/net/net.h b/include/net/net.h >> index 24563ef..3e4b9df 100644 >> --- a/include/net/net.h >> +++ b/include/net/net.h >> @@ -63,6 +63,8 @@ typedef struct NetClientInfo { >> } NetClientInfo; >> >> struct NetClientState { >> + /* protect peer's send_queue */ >> + QemuMutex transfer_lock; >> NetClientInfo *info; >> int link_down; >> QTAILQ_ENTRY(NetClientState) next; >> @@ -78,6 +80,7 @@ struct NetClientState { >> >> typedef struct NICState { >> NetClientState ncs[MAX_QUEUE_NUM]; >> + NetClientState *pending_peer[MAX_QUEUE_NUM]; > > Please rebase onto github.com/stefanha/qemu.git net. ncs[] is no longer > statically sized to MAX_QUEUE_NUM. > OK >> NICConf *conf; >> void *opaque; >> bool peer_deleted; >> @@ -105,6 +108,7 @@ NetClientState *qemu_find_vlan_client_by_name(Monitor >> *mon, int vlan_id, >> const char *client_str); >> typedef void (*qemu_nic_foreach)(NICState *nic, void *opaque); >> void qemu_foreach_nic(qemu_nic_foreach func, void *opaque); >> +int qemu_can_send_packet_nolock(NetClientState *sender); >> int qemu_can_send_packet(NetClientState *nc); >> ssize_t qemu_sendv_packet(NetClientState *nc, const struct iovec *iov, >> int iovcnt); >> diff --git a/include/net/queue.h b/include/net/queue.h >> index f60e57f..0ecd23b 100644 >> --- a/include/net/queue.h >> +++ b/include/net/queue.h >> @@ -67,6 +67,7 @@ ssize_t qemu_net_queue_send_iov(NetQueue *queue, >> NetPacketSent *sent_cb); >> >> void qemu_net_queue_purge(NetQueue *queue, NetClientState *from); >> +void qemu_net_queue_purge_all(NetQueue *queue); >> bool qemu_net_queue_flush(NetQueue *queue); >> >> #endif /* QEMU_NET_QUEUE_H */ >> diff --git a/net/hub.c b/net/hub.c >> index 81d2a04..97c3ac3 100644 >> --- a/net/hub.c >> +++ b/net/hub.c >> @@ -53,9 +53,14 @@ static ssize_t net_hub_receive(NetHub *hub, NetHubPort >> *source_port, >> if (port == source_port) { >> continue; >> } >> - >> + qemu_mutex_lock(&port->nc.transfer_lock); >> + if (!port->nc.peer) { > > .peer is protected by transfer_lock too? This was not documented above > and I think it's not necessary to protect .peer? > The transfer_lock has two aims: to protect the send path against remove path. (lock for nc->peer) to protect among the senders (lock for nc->peer->send_queue) >> + qemu_mutex_unlock(&port->nc.transfer_lock); >> + continue; >> + } >> qemu_net_queue_append(port->nc.peer->send_queue, &port->nc, >> QEMU_NET_PACKET_FLAG_NONE, buf, len, NULL); >> + qemu_mutex_unlock(&port->nc.transfer_lock); >> event_notifier_set(&port->e); >> } >> return len; >> @@ -65,7 +70,13 @@ static void hub_port_deliver_packet(void *opaque) >> { >> NetHubPort *port = (NetHubPort *)opaque; >> >> + qemu_mutex_lock(&port->nc.transfer_lock); >> + if (!port->nc.peer) { >> + qemu_mutex_unlock(&port->nc.transfer_lock); >> + return; >> + } >> qemu_net_queue_flush(port->nc.peer->send_queue); >> + qemu_mutex_unlock(&port->nc.transfer_lock); >> } >> >> static ssize_t net_hub_receive_iov(NetHub *hub, NetHubPort *source_port, >> @@ -78,10 +89,16 @@ static ssize_t net_hub_receive_iov(NetHub *hub, >> NetHubPort *source_port, >> if (port == source_port) { >> continue; >> } >> - >> + qemu_mutex_lock(&port->nc.transfer_lock); >> + if (!port->nc.peer) { >> + qemu_mutex_unlock(&port->nc.transfer_lock); >> + continue; >> + } >> qemu_net_queue_append_iov(port->nc.peer->send_queue, &port->nc, >> QEMU_NET_PACKET_FLAG_NONE, iov, iovcnt, NULL); >> + qemu_mutex_unlock(&port->nc.transfer_lock); >> event_notifier_set(&port->e); >> + >> } >> return len; >> } >> diff --git a/net/net.c b/net/net.c >> index 544542b..0acb933 100644 >> --- a/net/net.c >> +++ b/net/net.c >> @@ -207,6 +207,7 @@ static void qemu_net_client_setup(NetClientState *nc, >> nc->peer = peer; >> peer->peer = nc; >> } >> + qemu_mutex_init(&nc->transfer_lock); >> QTAILQ_INSERT_TAIL(&net_clients, nc, next); >> >> nc->send_queue = qemu_new_net_queue(nc); >> @@ -285,6 +286,7 @@ void *qemu_get_nic_opaque(NetClientState *nc) >> >> static void qemu_cleanup_net_client(NetClientState *nc) >> { >> + /* This is the place where may be out of big lock, when dev finalized */ > > I don't understand this comment. > Will remove. I had recorded it to remind myself that extra lock is needed here. >> QTAILQ_REMOVE(&net_clients, nc, next); >> >> if (nc->info->cleanup) { >> @@ -307,6 +309,28 @@ static void qemu_free_net_client(NetClientState *nc) >> } >> } >> >> +/* exclude race with rx/tx path, flush out peer's queue */ >> +static void qemu_flushout_net_client(NetClientState *nc) > > This function detaches the peer, the name should reflect that. > OK. >> +{ >> + NetClientState *peer; >> + >> + /* sync on receive path */ >> + peer = nc->peer; >> + if (peer) { >> + qemu_mutex_lock(&peer->transfer_lock); >> + peer->peer = NULL; >> + qemu_mutex_unlock(&peer->transfer_lock); >> + } > > This is weird. You don't lock to read nc->peer but you do lock to write > peer->peer. If you use a lock it must be used consistently. Because removal is the only code path to assign nc->peer = NULL, so the reader and updater is serial here. But as for peer->peer, it must run against sender.
Thanks and regards, Pingfan