Jason Wang
2021-Feb-03 05:33 UTC
[PATCH net] virtio-net: suppress bad irq warning for tx napi
On 2021/2/2 ??10:37, Willem de Bruijn wrote:> On Mon, Feb 1, 2021 at 10:09 PM Jason Wang <jasowang at redhat.com> wrote: >> >> On 2021/1/29 ??8:21, Wei Wang wrote: >>> With the implementation of napi-tx in virtio driver, we clean tx >>> descriptors from rx napi handler, for the purpose of reducing tx >>> complete interrupts. But this could introduce a race where tx complete >>> interrupt has been raised, but the handler found there is no work to do >>> because we have done the work in the previous rx interrupt handler. >>> This could lead to the following warning msg: >>> [ 3588.010778] irq 38: nobody cared (try booting with the >>> "irqpoll" option) >>> [ 3588.017938] CPU: 4 PID: 0 Comm: swapper/4 Not tainted >>> 5.3.0-19-generic #20~18.04.2-Ubuntu >>> [ 3588.017940] Call Trace: >>> [ 3588.017942] <IRQ> >>> [ 3588.017951] dump_stack+0x63/0x85 >>> [ 3588.017953] __report_bad_irq+0x35/0xc0 >>> [ 3588.017955] note_interrupt+0x24b/0x2a0 >>> [ 3588.017956] handle_irq_event_percpu+0x54/0x80 >>> [ 3588.017957] handle_irq_event+0x3b/0x60 >>> [ 3588.017958] handle_edge_irq+0x83/0x1a0 >>> [ 3588.017961] handle_irq+0x20/0x30 >>> [ 3588.017964] do_IRQ+0x50/0xe0 >>> [ 3588.017966] common_interrupt+0xf/0xf >>> [ 3588.017966] </IRQ> >>> [ 3588.017989] handlers: >>> [ 3588.020374] [<000000001b9f1da8>] vring_interrupt >>> [ 3588.025099] Disabling IRQ #38 >>> >>> This patch adds a new param to struct vring_virtqueue, and we set it for >>> tx virtqueues if napi-tx is enabled, to suppress the warning in such >>> case. >>> >>> Fixes: 7b0411ef4aa6 ("virtio-net: clean tx descriptors from rx napi") >>> Reported-by: Rick Jones <jonesrick at google.com> >>> Signed-off-by: Wei Wang <weiwan at google.com> >>> Signed-off-by: Willem de Bruijn <willemb at google.com> >> >> Please use get_maintainer.pl to make sure Michael and me were cced. > Will do. Sorry about that. I suggested just the virtualization list, my bad. > >>> --- >>> drivers/net/virtio_net.c | 19 ++++++++++++++----- >>> drivers/virtio/virtio_ring.c | 16 ++++++++++++++++ >>> include/linux/virtio.h | 2 ++ >>> 3 files changed, 32 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >>> index 508408fbe78f..e9a3f30864e8 100644 >>> --- a/drivers/net/virtio_net.c >>> +++ b/drivers/net/virtio_net.c >>> @@ -1303,13 +1303,22 @@ static void virtnet_napi_tx_enable(struct virtnet_info *vi, >>> return; >>> } >>> >>> + /* With napi_tx enabled, free_old_xmit_skbs() could be called from >>> + * rx napi handler. Set work_steal to suppress bad irq warning for >>> + * IRQ_NONE case from tx complete interrupt handler. >>> + */ >>> + virtqueue_set_work_steal(vq, true); >>> + >>> return virtnet_napi_enable(vq, napi); >> >> Do we need to force the ordering between steal set and napi enable? > The warning only occurs after one hundred spurious interrupts, so not > really.Ok, so it looks like a hint. Then I wonder how much value do we need to introduce helper like virtqueue_set_work_steal() that allows the caller to toggle. How about disable the check forever during virtqueue initialization?> >>> } >>> >>> -static void virtnet_napi_tx_disable(struct napi_struct *napi) >>> +static void virtnet_napi_tx_disable(struct virtqueue *vq, >>> + struct napi_struct *napi) >>> { >>> - if (napi->weight) >>> + if (napi->weight) { >>> napi_disable(napi); >>> + virtqueue_set_work_steal(vq, false); >>> + } >>> } >>> >>> static void refill_work(struct work_struct *work) >>> @@ -1835,7 +1844,7 @@ static int virtnet_close(struct net_device *dev) >>> for (i = 0; i < vi->max_queue_pairs; i++) { >>> xdp_rxq_info_unreg(&vi->rq[i].xdp_rxq); >>> napi_disable(&vi->rq[i].napi); >>> - virtnet_napi_tx_disable(&vi->sq[i].napi); >>> + virtnet_napi_tx_disable(vi->sq[i].vq, &vi->sq[i].napi); >>> } >>> >>> return 0; >>> @@ -2315,7 +2324,7 @@ static void virtnet_freeze_down(struct virtio_device *vdev) >>> if (netif_running(vi->dev)) { >>> for (i = 0; i < vi->max_queue_pairs; i++) { >>> napi_disable(&vi->rq[i].napi); >>> - virtnet_napi_tx_disable(&vi->sq[i].napi); >>> + virtnet_napi_tx_disable(vi->sq[i].vq, &vi->sq[i].napi); >>> } >>> } >>> } >>> @@ -2440,7 +2449,7 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog, >>> if (netif_running(dev)) { >>> for (i = 0; i < vi->max_queue_pairs; i++) { >>> napi_disable(&vi->rq[i].napi); >>> - virtnet_napi_tx_disable(&vi->sq[i].napi); >>> + virtnet_napi_tx_disable(vi->sq[i].vq, &vi->sq[i].napi); >>> } >>> } >>> >>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c >>> index 71e16b53e9c1..f7c5d697c302 100644 >>> --- a/drivers/virtio/virtio_ring.c >>> +++ b/drivers/virtio/virtio_ring.c >>> @@ -105,6 +105,9 @@ struct vring_virtqueue { >>> /* Host publishes avail event idx */ >>> bool event; >>> >>> + /* Tx side napi work could be done from rx side. */ >>> + bool work_steal; >> >> So vring_vritqueue is a general structure, let's avoid mentioning >> network specific stuffs here. And we need a better name like >> "no_interrupt_check"? >> >> And we need a separate patch for virtio core changes. > Ack. Will change. > >>> + >>> /* Head of free buffer list. */ >>> unsigned int free_head; >>> /* Number we've added since last sync. */ >>> @@ -1604,6 +1607,7 @@ static struct virtqueue *vring_create_virtqueue_packed( >>> vq->notify = notify; >>> vq->weak_barriers = weak_barriers; >>> vq->broken = false; >>> + vq->work_steal = false; >>> vq->last_used_idx = 0; >>> vq->num_added = 0; >>> vq->packed_ring = true; >>> @@ -2038,6 +2042,9 @@ irqreturn_t vring_interrupt(int irq, void *_vq) >>> >>> if (!more_used(vq)) { >>> pr_debug("virtqueue interrupt with no work for %p\n", vq); >> >> Do we still need to keep this warning? > Come to think of it, I would say no, in this case. > >> >>> + if (vq->work_steal) >>> + return IRQ_HANDLED; >> >> So I wonder instead of doing trick like this, maybe it's time to unify >> TX/RX NAPI with the help of[1] (virtio-net use queue pairs). >> >> Thanks >> >> [1] https://lkml.org/lkml/2014/12/25/169 > Interesting idea. It does sound like a good fit for this model. The > patch in the Fixes line proved effective at suppressing unnecessary TX > interrupts when processing in RX interrupt handler. So not sure how > much will help in practice. Might be a nice project to evaluate > separate for net-next at some point.Right, basically the idea is that if an irq is shared among several virtqueues, there's no need to check for more_used() there. Yes, we can try sometime in the future. (Or e.g we have more than 128 queue pairs). Thanks> > Thanks for the review! >
Willem de Bruijn
2021-Feb-03 18:28 UTC
[PATCH net] virtio-net: suppress bad irq warning for tx napi
On Wed, Feb 3, 2021 at 12:33 AM Jason Wang <jasowang at redhat.com> wrote:> > > On 2021/2/2 ??10:37, Willem de Bruijn wrote: > > On Mon, Feb 1, 2021 at 10:09 PM Jason Wang <jasowang at redhat.com> wrote: > >> > >> On 2021/1/29 ??8:21, Wei Wang wrote: > >>> With the implementation of napi-tx in virtio driver, we clean tx > >>> descriptors from rx napi handler, for the purpose of reducing tx > >>> complete interrupts. But this could introduce a race where tx complete > >>> interrupt has been raised, but the handler found there is no work to do > >>> because we have done the work in the previous rx interrupt handler. > >>> This could lead to the following warning msg: > >>> [ 3588.010778] irq 38: nobody cared (try booting with the > >>> "irqpoll" option) > >>> [ 3588.017938] CPU: 4 PID: 0 Comm: swapper/4 Not tainted > >>> 5.3.0-19-generic #20~18.04.2-Ubuntu > >>> [ 3588.017940] Call Trace: > >>> [ 3588.017942] <IRQ> > >>> [ 3588.017951] dump_stack+0x63/0x85 > >>> [ 3588.017953] __report_bad_irq+0x35/0xc0 > >>> [ 3588.017955] note_interrupt+0x24b/0x2a0 > >>> [ 3588.017956] handle_irq_event_percpu+0x54/0x80 > >>> [ 3588.017957] handle_irq_event+0x3b/0x60 > >>> [ 3588.017958] handle_edge_irq+0x83/0x1a0 > >>> [ 3588.017961] handle_irq+0x20/0x30 > >>> [ 3588.017964] do_IRQ+0x50/0xe0 > >>> [ 3588.017966] common_interrupt+0xf/0xf > >>> [ 3588.017966] </IRQ> > >>> [ 3588.017989] handlers: > >>> [ 3588.020374] [<000000001b9f1da8>] vring_interrupt > >>> [ 3588.025099] Disabling IRQ #38 > >>> > >>> This patch adds a new param to struct vring_virtqueue, and we set it for > >>> tx virtqueues if napi-tx is enabled, to suppress the warning in such > >>> case. > >>> > >>> Fixes: 7b0411ef4aa6 ("virtio-net: clean tx descriptors from rx napi") > >>> Reported-by: Rick Jones <jonesrick at google.com> > >>> Signed-off-by: Wei Wang <weiwan at google.com> > >>> Signed-off-by: Willem de Bruijn <willemb at google.com> > >> > >> Please use get_maintainer.pl to make sure Michael and me were cced. > > Will do. Sorry about that. I suggested just the virtualization list, my bad. > > > >>> --- > >>> drivers/net/virtio_net.c | 19 ++++++++++++++----- > >>> drivers/virtio/virtio_ring.c | 16 ++++++++++++++++ > >>> include/linux/virtio.h | 2 ++ > >>> 3 files changed, 32 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > >>> index 508408fbe78f..e9a3f30864e8 100644 > >>> --- a/drivers/net/virtio_net.c > >>> +++ b/drivers/net/virtio_net.c > >>> @@ -1303,13 +1303,22 @@ static void virtnet_napi_tx_enable(struct virtnet_info *vi, > >>> return; > >>> } > >>> > >>> + /* With napi_tx enabled, free_old_xmit_skbs() could be called from > >>> + * rx napi handler. Set work_steal to suppress bad irq warning for > >>> + * IRQ_NONE case from tx complete interrupt handler. > >>> + */ > >>> + virtqueue_set_work_steal(vq, true); > >>> + > >>> return virtnet_napi_enable(vq, napi); > >> > >> Do we need to force the ordering between steal set and napi enable? > > The warning only occurs after one hundred spurious interrupts, so not > > really. > > > Ok, so it looks like a hint. Then I wonder how much value do we need to > introduce helper like virtqueue_set_work_steal() that allows the caller > to toggle. How about disable the check forever during virtqueue > initialization?Yes, that is even simpler. We still need the helper, as the internal variables of vring_virtqueue are not accessible from virtio-net. An earlier patch added the variable to virtqueue itself, but I think it belongs in vring_virtqueue. And the helper is not a lot of code.