Michael S. Tsirkin
2021-May-26 08:24 UTC
[PATCH v3 1/4] virtio_net: move tx vq operation under tx queue lock
It's unsafe to operate a vq from multiple threads. Unfortunately this is exactly what we do when invoking clean tx poll from rx napi. Same happens with napi-tx even without the opportunistic cleaning from the receive interrupt: that races with processing the vq in start_xmit. As a fix move everything that deals with the vq to under tx lock. Fixes: b92f1e6751a6 ("virtio-net: transmit napi") Signed-off-by: Michael S. Tsirkin <mst at redhat.com> --- drivers/net/virtio_net.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index ac0c143f97b4..12512d1002ec 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -1508,6 +1508,8 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget) struct virtnet_info *vi = sq->vq->vdev->priv; unsigned int index = vq2txq(sq->vq); struct netdev_queue *txq; + int opaque; + bool done; if (unlikely(is_xdp_raw_buffer_queue(vi, index))) { /* We don't need to enable cb for XDP */ @@ -1517,10 +1519,28 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget) txq = netdev_get_tx_queue(vi->dev, index); __netif_tx_lock(txq, raw_smp_processor_id()); + virtqueue_disable_cb(sq->vq); free_old_xmit_skbs(sq, true); + + opaque = virtqueue_enable_cb_prepare(sq->vq); + + done = napi_complete_done(napi, 0); + + if (!done) + virtqueue_disable_cb(sq->vq); + __netif_tx_unlock(txq); - virtqueue_napi_complete(napi, sq->vq, 0); + if (done) { + if (unlikely(virtqueue_poll(sq->vq, opaque))) { + if (napi_schedule_prep(napi)) { + __netif_tx_lock(txq, raw_smp_processor_id()); + virtqueue_disable_cb(sq->vq); + __netif_tx_unlock(txq); + __napi_schedule(napi); + } + } + } if (sq->vq->num_free >= 2 + MAX_SKB_FRAGS) netif_tx_wake_queue(txq); -- MST
Jason Wang
2021-May-27 03:41 UTC
[PATCH v3 1/4] virtio_net: move tx vq operation under tx queue lock
? 2021/5/26 ??4:24, Michael S. Tsirkin ??:> It's unsafe to operate a vq from multiple threads. > Unfortunately this is exactly what we do when invoking > clean tx poll from rx napi. > Same happens with napi-tx even without the > opportunistic cleaning from the receive interrupt: that races > with processing the vq in start_xmit. > > As a fix move everything that deals with the vq to under tx lock. > > Fixes: b92f1e6751a6 ("virtio-net: transmit napi") > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > --- > drivers/net/virtio_net.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index ac0c143f97b4..12512d1002ec 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -1508,6 +1508,8 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget) > struct virtnet_info *vi = sq->vq->vdev->priv; > unsigned int index = vq2txq(sq->vq); > struct netdev_queue *txq; > + int opaque; > + bool done; > > if (unlikely(is_xdp_raw_buffer_queue(vi, index))) { > /* We don't need to enable cb for XDP */ > @@ -1517,10 +1519,28 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget) > > txq = netdev_get_tx_queue(vi->dev, index); > __netif_tx_lock(txq, raw_smp_processor_id()); > + virtqueue_disable_cb(sq->vq); > free_old_xmit_skbs(sq, true); > + > + opaque = virtqueue_enable_cb_prepare(sq->vq); > + > + done = napi_complete_done(napi, 0); > + > + if (!done) > + virtqueue_disable_cb(sq->vq); > + > __netif_tx_unlock(txq); > > - virtqueue_napi_complete(napi, sq->vq, 0); > + if (done) { > + if (unlikely(virtqueue_poll(sq->vq, opaque))) { > + if (napi_schedule_prep(napi)) { > + __netif_tx_lock(txq, raw_smp_processor_id()); > + virtqueue_disable_cb(sq->vq); > + __netif_tx_unlock(txq); > + __napi_schedule(napi); > + } > + } > + }Interesting, this looks like somehwo a open-coded version of virtqueue_napi_complete(). I wonder if we can simply keep using virtqueue_napi_complete() by simply moving the __netif_tx_unlock() after that: netif_tx_lock(txq); free_old_xmit_skbs(sq, true); virtqueue_napi_complete(napi, sq->vq, 0); __netif_tx_unlock(txq); Thanks> > if (sq->vq->num_free >= 2 + MAX_SKB_FRAGS) > netif_tx_wake_queue(txq);