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.
Paolo
