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

[Paolo Bonzini <pbonzini@redhat.com>]

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