On Wed, Mar 13, 2013 at 6:39 PM, Paolo Bonzini <pbonz...@redhat.com> wrote: > Il 13/03/2013 02:26, liu ping fan ha scritto: >> On Tue, Mar 12, 2013 at 4:55 PM, Paolo Bonzini <pbonz...@redhat.com> wrote: >>> Il 07/03/2013 03:53, Liu Ping Fan ha scritto: >>>> From: Liu Ping Fan <pingf...@linux.vnet.ibm.com> >>>> >>>> Introduce nc->send_lock, it shield off the race of nc->peer's reader and >>>> deleter. With it, after deleter finish, no new qemu_send_packet_xx() >>>> can reach ->send_queue, so no new reference(packet->sender) to nc will >>>> be appended to nc->peer->send_queue. >>>> >>>> Signed-off-by: Liu Ping Fan <pingf...@linux.vnet.ibm.com> >>>> --- >>>> include/net/net.h | 4 +++ >>>> net/hub.c | 18 ++++++++++++ >>>> net/net.c | 77 >>>> ++++++++++++++++++++++++++++++++++++++++++++++++++--- >>>> net/queue.c | 4 +- >>>> 4 files changed, 97 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/include/net/net.h b/include/net/net.h >>>> index 9c2b357..45779d2 100644 >>>> --- a/include/net/net.h >>>> +++ b/include/net/net.h >>>> @@ -66,6 +66,8 @@ struct NetClientState { >>>> NetClientInfo *info; >>>> int link_down; >>>> QTAILQ_ENTRY(NetClientState) next; >>>> + /* protect the race access of peer between deleter and sender */ >>>> + QemuMutex send_lock; >>>> NetClientState *peer; >>>> NetQueue *send_queue; >>>> char *model; >>>> @@ -78,6 +80,7 @@ struct NetClientState { >>>> >>>> typedef struct NICState { >>>> NetClientState *ncs; >>>> + NetClientState **pending_peer; >>>> 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/net/hub.c b/net/hub.c >>>> index 47fe72c..d953a99 100644 >>>> --- a/net/hub.c >>>> +++ b/net/hub.c >>>> @@ -57,8 +57,14 @@ static ssize_t net_hub_receive(NetHub *hub, NetHubPort >>>> *source_port, >>>> continue; >>>> } >>>> >>>> + qemu_mutex_lock(&port->nc.send_lock); >>>> + if (!port->nc.peer) { >>>> + qemu_mutex_unlock(&port->nc.send_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.send_lock); >>> >>> Do you really need to lock everything? Can you just wrap the peer with >>> a ref/unref, like >>> >>> NetClientState *net_client_get_peer(NetClientState *nc) >>> { >>> NetClientState *peer; >>> qemu_mutex_lock(&nc->send_lock); >>> peer = nc->peer; >>> if (peer) { >>> net_client_ref(peer); >>> } >>> qemu_mutex_unlock(&nc->send_lock); >>> return peer; >>> } >>> >>> and then >>> >>> - qemu_net_queue_append(port->nc.peer->send_queue, &port->nc, >>> + peer = net_client_get_peer(&port->nc); >>> + if (!peer) { >>> + continue; >>> + } >>> + qemu_net_queue_append(peer->send_queue, &port->nc, >>> QEMU_NET_PACKET_FLAG_NONE, buf, len, NULL); >>> + net_client_unref(peer); >>> >> Oh, seems that I do not explain very clearly in the commit log. The >> lock does not only protect against the reclaimer( and this can be >> achieved by refcnt as your codes), but also sync between remover and >> sender. If the NetClientState being removed, the remover will be >> like: >> nc->peer = NULL; >> ----------> Here opens the gap for in-flight sender, and >> refcnt can not work >> flush out reference from its peer's send_queue; > > What's the problem if the dying peer is still used momentarily? The > next unref will drop the last reference and qemu_del_net_queue will free > the packet that was just appended. > The deletion of NetClientState-A does mean that its peer will be delete, and so the peer's qemu_del_net_queue. And each packet referring to A still holds a ref, and finally A will not be reclaimed.
Regards, Pingfan > Paolo