| Message ID | 1362316883-7948-3-git-send-email-qemulist@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Sun, Mar 03, 2013 at 09:21:21PM +0800, Liu Ping Fan wrote: > From: Liu Ping Fan <pingfank@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. > deleter and senders will sync on this lock, so we can also survive across > unplug. > > Signed-off-by: Liu Ping Fan <pingfank@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. > 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? > + 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. > 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. > +{ > + 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.
Il 04/03/2013 15:49, Stefan Hajnoczi ha scritto: > > 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. In fact, it's a bit strange to use nc->something_lock to lock something in nc->peer. Please add the lock to NetQueue and include/net/queue.h. nc->peer shouldn't need a lock. It is immutable, isn't it? Paolo
On Mon, Mar 4, 2013 at 11:04 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 04/03/2013 15:49, Stefan Hajnoczi ha scritto: >> > 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. > > In fact, it's a bit strange to use nc->something_lock to lock something > in nc->peer. Please add the lock to NetQueue and include/net/queue.h. > The lock also servers as exclusion between NetClientState's sender and remover. So if we apply lock on NetQueue, it can not achieve the goal. > nc->peer shouldn't need a lock. It is immutable, isn't it? > The sender should run against hot-uplug. Thanks and regards, Pingfan > Paolo
On Mon, Mar 4, 2013 at 10:49 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Sun, Mar 03, 2013 at 09:21:21PM +0800, Liu Ping Fan wrote: >> From: Liu Ping Fan <pingfank@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 <pingfank@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
On Tue, Mar 5, 2013 at 10:45 AM, liu ping fan <qemulist@gmail.com> wrote: > On Mon, Mar 4, 2013 at 10:49 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: >> On Sun, Mar 03, 2013 at 09:21:21PM +0800, Liu Ping Fan wrote: >>> From: Liu Ping Fan <pingfank@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 <pingfank@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. > The race between removers is excluded by big lock in hot-unplug path. > > Thanks and regards, > Pingfan
Il 05/03/2013 03:45, liu ping fan ha scritto: > > In fact, it's a bit strange to use nc->something_lock to lock something > > in nc->peer. Please add the lock to NetQueue and include/net/queue.h. > > The lock also servers as exclusion between NetClientState's sender and > remover. So if we apply lock on NetQueue, it can not achieve the > goal. Ok, but please document it. Paolo
On Tue, Mar 05, 2013 at 11:04:28AM +0800, liu ping fan wrote: > On Tue, Mar 5, 2013 at 10:45 AM, liu ping fan <qemulist@gmail.com> wrote: > > On Mon, Mar 4, 2013 at 10:49 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > >> On Sun, Mar 03, 2013 at 09:21:21PM +0800, Liu Ping Fan wrote: > >>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com> > >>> @@ -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. > > > The race between removers is excluded by big lock in hot-unplug path. Please use the locks consistently instead of relying on the global mutex or other assumptions. The assumptions can change (plus you haven't documented them) so these patches make the net subsystem very brittle. The next person wishing to modify net.c would have to reverse-engineer all the assumptions you've made and doesn't have clear examples of which locks to use. Stefan
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]; 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) { + 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 */ 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) +{ + 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); + } + + /* sync on send from this nc */ + 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); +} + void qemu_del_net_client(NetClientState *nc) { NetClientState *ncs[MAX_QUEUE_NUM]; @@ -337,7 +361,9 @@ 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]); + nic->pending_peer[i] = ncs[i]; } return; @@ -346,6 +372,7 @@ void qemu_del_net_client(NetClientState *nc) assert(nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC); for (i = 0; i < queues; i++) { + qemu_flushout_net_client(ncs[i]); qemu_cleanup_net_client(ncs[i]); qemu_free_net_client(ncs[i]); } @@ -358,16 +385,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_flushout_net_client(nc); qemu_cleanup_net_client(nc); qemu_free_net_client(nc); } + } void qemu_foreach_nic(qemu_nic_foreach func, void *opaque) @@ -383,7 +413,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; @@ -398,6 +428,29 @@ int qemu_can_send_packet(NetClientState *sender) return 1; } +int qemu_can_send_packet(NetClientState *sender) +{ + int ret = 1; + + qemu_mutex_lock(&sender->transfer_lock); + if (!sender->peer) { + ret = 1; + 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->transfer_lock); + return ret; +} + ssize_t qemu_deliver_packet(NetClientState *sender, unsigned flags, const uint8_t *data, @@ -455,19 +508,24 @@ static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender, NetPacketSent *sent_cb) { NetQueue *queue; + ssize_t rslt; #ifdef DEBUG_NET printf("qemu_send_packet_async:\n"); hex_dump(stdout, buf, size); #endif + qemu_mutex_lock(&sender->transfer_lock); if (sender->link_down || !sender->peer) { + qemu_mutex_lock(&sender->transfer_lock); return size; } queue = sender->peer->send_queue; - return qemu_net_queue_send(queue, sender, flags, buf, size, sent_cb); + rslt = qemu_net_queue_send(queue, sender, flags, buf, size, sent_cb); + qemu_mutex_unlock(&sender->transfer_lock); + return rslt; } ssize_t qemu_send_packet_async(NetClientState *sender, @@ -535,16 +593,21 @@ ssize_t qemu_sendv_packet_async(NetClientState *sender, NetPacketSent *sent_cb) { NetQueue *queue; + ssize_t rslt; + qemu_mutex_lock(&sender->transfer_lock); if (sender->link_down || !sender->peer) { + qemu_mutex_unlock(&sender->transfer_lock); return iov_size(iov, iovcnt); } queue = sender->peer->send_queue; - return qemu_net_queue_send_iov(queue, sender, + rslt = qemu_net_queue_send_iov(queue, sender, QEMU_NET_PACKET_FLAG_NONE, iov, iovcnt, sent_cb); + qemu_mutex_unlock(&sender->transfer_lock); + return rslt; } ssize_t @@ -984,6 +1047,7 @@ void do_info_network(Monitor *mon, const QDict *qdict) print_net_client(mon, peer); } } + } void qmp_set_link(const char *name, bool up, Error **errp) diff --git a/net/queue.c b/net/queue.c index d4fb965..ad65523 100644 --- a/net/queue.c +++ b/net/queue.c @@ -24,6 +24,7 @@ #include "net/queue.h" #include "qemu/queue.h" #include "net/net.h" +#include "qom/object.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 @@ -172,7 +173,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; } @@ -197,7 +198,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; } @@ -225,6 +226,16 @@ void qemu_net_queue_purge(NetQueue *queue, NetClientState *from) } } +void qemu_net_queue_purge_all(NetQueue *queue) +{ + NetPacket *packet, *next; + + QTAILQ_FOREACH_SAFE(packet, &queue->packets, entry, next) { + QTAILQ_REMOVE(&queue->packets, packet, entry); + g_free(packet); + } +} + bool qemu_net_queue_flush(NetQueue *queue) { while (!QTAILQ_EMPTY(&queue->packets)) {