Re: [Qemu-devel] [PATCH 2/3] net: introduce lock to protect NetClientState's send_queue

[liu ping fan]

On Mon, Mar 4, 2013 at 10:49 PM, Stefan Hajnoczi <address@hidden> wrote:
> On Sun, Mar 03, 2013 at 09:21:21PM +0800, Liu Ping Fan wrote:
>> From: Liu Ping Fan <address@hidden>
>>
>> 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 <address@hidden>
>> ---
>>  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