Re: [PATCH v3 1/4] virtio_net: move tx vq operation under tx queue lock

On Fri, May 28, 2021 at 06:25:11PM -0400, Willem de Bruijn wrote:

On Wed, May 26, 2021 at 11:41 PM Jason Wang [off-list ref] wrote:

quoted

在 2021/5/26 下午4:24, Michael S. Tsirkin 写道:

quoted

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.

This patch also disables callbacks during free_old_xmit_skbs
processing on tx interrupt. Should that be a separate commit, with its
own explanation?

quoted

quoted

Fixes: b92f1e6751a6 ("virtio-net: transmit napi")
Signed-off-by: Michael S. Tsirkin <mst@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))) {

Should this also be inside the lock, as it operates on vq?

No vq poll is ok outside of locks, it's atomic.

Is there anything that is not allowed to run with the lock held?

quoted

quoted

+                     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);

Agreed. And subsequent block

       if (sq->vq->num_free >= 2 + MAX_SKB_FRAGS)
               netif_tx_wake_queue(txq);

as well

Yes I thought I saw something here that can't be called with tx lock
held but I no longer see it. Will do.

quoted

Thanks

quoted

      if (sq->vq->num_free >= 2 + MAX_SKB_FRAGS)
              netif_tx_wake_queue(txq);

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization