Re: [Qemu-devel] [PATCH v2 4/5] net: introduce lock to protect NetClientState's peer's access

[liu ping fan]

On Wed, Mar 13, 2013 at 6:39 PM, Paolo Bonzini <address@hidden> wrote:
> Il 13/03/2013 02:26, liu ping fan ha scritto:
>> On Tue, Mar 12, 2013 at 4:55 PM, Paolo Bonzini <address@hidden> wrote:
>>> Il 07/03/2013 03:53, Liu Ping Fan ha scritto:
>>>> From: Liu Ping Fan <address@hidden>
>>>>
>>>> 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 <address@hidden>
>>>> ---
>>>>  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