| Message ID | 20210526082423.47837-2-mst@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | virtio net: spurious interrupt related fixes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 37 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
在 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@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);
On Wed, May 26, 2021 at 11:41 PM Jason Wang <jasowang@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@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); >
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@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@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 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. > > > > Thanks > > > > > > > > > > if (sq->vq->num_free >= 2 + MAX_SKB_FRAGS) > > > netif_tx_wake_queue(txq); > >
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);
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@redhat.com> --- drivers/net/virtio_net.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)