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);
Willem de Bruijn
2021-May-28 22:25 UTC
[PATCH v3 1/4] virtio_net: move tx vq operation under tx queue lock
On Wed, May 26, 2021 at 11:41 PM Jason Wang <jasowang at redhat.com> wrote:> > > ? 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.This patch also disables callbacks during free_old_xmit_skbs processing on tx interrupt. Should that be a separate commit, with its own explanation?> > > > 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))) {Should this also be inside the lock, as it operates on vq? Is there anything that is not allowed to run with the lock held?> > + 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> > Thanks > > > > > > if (sq->vq->num_free >= 2 + MAX_SKB_FRAGS) > > netif_tx_wake_queue(txq); >
Michael S. Tsirkin
2021-Jun-09 22:03 UTC
[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 <jasowang at redhat.com> wrote: > > > > > > ? 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. > > This patch also disables callbacks during free_old_xmit_skbs > processing on tx interrupt. Should that be a separate commit, with its > own explanation? > > > > > > 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))) { > > 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? > > > + 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 wellYes I thought I saw something here that can't be called with tx lock held but I no longer see it. Will do.> > > > Thanks > > > > > > > > > > if (sq->vq->num_free >= 2 + MAX_SKB_FRAGS) > > > netif_tx_wake_queue(txq); > >