From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> To: "Michael S. Tsirkin" <mst@redhat.com> Cc: linux-kernel <linux-kernel@vger.kernel.org>, Jakub Kicinski <kuba@kernel.org>, Wei Wang <weiwan@google.com>, David Miller <davem@davemloft.net>, Network Development <netdev@vger.kernel.org>, virtualization <virtualization@lists.linux-foundation.org> Subject: Re: [PATCH v3 0/4] virtio net: spurious interrupt related fixes Date: Wed, 26 May 2021 11:34:58 -0400 [thread overview] Message-ID: <CA+FuTScp-OhBnVzkXcsCBWxmq51VO6+8UGpSU5i3AJQV84eTLg@mail.gmail.com> (raw) In-Reply-To: <20210526082423.47837-1-mst@redhat.com> On Wed, May 26, 2021 at 4:24 AM Michael S. Tsirkin <mst@redhat.com> 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 introduces a race where tx complete > interrupt has been raised, but the handler finds there is no work to do > because we have done the work in the previous rx interrupt handler. > A similar issue exists with polling from start_xmit, it is however > less common because of the delayed cb optimization of the split ring - > but will likely affect the packed ring once that is more common. > > In particular, this was reported to 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 patchset attempts to fix this by cleaning up a bunch of races > related to the handling of sq callbacks (aka tx interrupts). > Somewhat tested but I couldn't reproduce the original issues > reported, sending out for help with testing. > > Wei, does this address the spurious interrupt issue you are > observing? Could you confirm please? Thanks for working on this, Michael. Wei is on leave. I'll try to reproduce. My main concern is whether the cost of the fix may be greater than the race, if the additional locking may significantly impact efficiency/throughput/latency. We lack that performance data right now. The race had not been reported for years, and caused no real concerns in the initial report we did get, either. That said, it may be more problematic in specific scenarios, such as the packed rings you pointed out. One (additional) short term mitigation could be to further restrict tx_napi default-on to exclude such scenarios. Let me take a closer look at the individual patches. > > Thanks! > > changes from v2: > Fixed a race condition in start_xmit: enable_cb_delayed was > done as an optimization (to push out event index for > split ring) so we did not have to care about it > returning false (recheck). Now that we actually disable the cb > we have to do test the return value and do the actual recheck. > > > Michael S. Tsirkin (4): > virtio_net: move tx vq operation under tx queue lock > virtio_net: move txq wakeups under tx q lock > virtio: fix up virtio_disable_cb > virtio_net: disable cb aggressively > > drivers/net/virtio_net.c | 49 ++++++++++++++++++++++++++++-------- > drivers/virtio/virtio_ring.c | 26 ++++++++++++++++++- > 2 files changed, 64 insertions(+), 11 deletions(-) > > -- > MST >
WARNING: multiple messages have this Message-ID (diff)
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> To: "Michael S. Tsirkin" <mst@redhat.com> Cc: Network Development <netdev@vger.kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>, virtualization <virtualization@lists.linux-foundation.org>, Jakub Kicinski <kuba@kernel.org>, Wei Wang <weiwan@google.com>, David Miller <davem@davemloft.net> Subject: Re: [PATCH v3 0/4] virtio net: spurious interrupt related fixes Date: Wed, 26 May 2021 11:34:58 -0400 [thread overview] Message-ID: <CA+FuTScp-OhBnVzkXcsCBWxmq51VO6+8UGpSU5i3AJQV84eTLg@mail.gmail.com> (raw) In-Reply-To: <20210526082423.47837-1-mst@redhat.com> On Wed, May 26, 2021 at 4:24 AM Michael S. Tsirkin <mst@redhat.com> 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 introduces a race where tx complete > interrupt has been raised, but the handler finds there is no work to do > because we have done the work in the previous rx interrupt handler. > A similar issue exists with polling from start_xmit, it is however > less common because of the delayed cb optimization of the split ring - > but will likely affect the packed ring once that is more common. > > In particular, this was reported to 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 patchset attempts to fix this by cleaning up a bunch of races > related to the handling of sq callbacks (aka tx interrupts). > Somewhat tested but I couldn't reproduce the original issues > reported, sending out for help with testing. > > Wei, does this address the spurious interrupt issue you are > observing? Could you confirm please? Thanks for working on this, Michael. Wei is on leave. I'll try to reproduce. My main concern is whether the cost of the fix may be greater than the race, if the additional locking may significantly impact efficiency/throughput/latency. We lack that performance data right now. The race had not been reported for years, and caused no real concerns in the initial report we did get, either. That said, it may be more problematic in specific scenarios, such as the packed rings you pointed out. One (additional) short term mitigation could be to further restrict tx_napi default-on to exclude such scenarios. Let me take a closer look at the individual patches. > > Thanks! > > changes from v2: > Fixed a race condition in start_xmit: enable_cb_delayed was > done as an optimization (to push out event index for > split ring) so we did not have to care about it > returning false (recheck). Now that we actually disable the cb > we have to do test the return value and do the actual recheck. > > > Michael S. Tsirkin (4): > virtio_net: move tx vq operation under tx queue lock > virtio_net: move txq wakeups under tx q lock > virtio: fix up virtio_disable_cb > virtio_net: disable cb aggressively > > drivers/net/virtio_net.c | 49 ++++++++++++++++++++++++++++-------- > drivers/virtio/virtio_ring.c | 26 ++++++++++++++++++- > 2 files changed, 64 insertions(+), 11 deletions(-) > > -- > MST > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2021-05-26 15:35 UTC|newest] Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-26 8:24 [PATCH v3 0/4] virtio net: spurious interrupt related fixes Michael S. Tsirkin 2021-05-26 8:24 ` Michael S. Tsirkin 2021-05-26 8:24 ` [PATCH v3 1/4] virtio_net: move tx vq operation under tx queue lock Michael S. Tsirkin 2021-05-26 8:24 ` Michael S. Tsirkin 2021-05-27 3:41 ` Jason Wang 2021-05-27 3:41 ` Jason Wang 2021-05-28 22:25 ` Willem de Bruijn 2021-05-28 22:25 ` Willem de Bruijn 2021-06-09 22:03 ` Michael S. Tsirkin 2021-06-09 22:03 ` Michael S. Tsirkin 2021-05-26 8:24 ` [PATCH v3 2/4] virtio_net: move txq wakeups under tx q lock Michael S. Tsirkin 2021-05-26 8:24 ` Michael S. Tsirkin 2021-05-27 3:48 ` Jason Wang 2021-05-27 3:48 ` Jason Wang 2021-05-26 8:24 ` [PATCH v3 3/4] virtio: fix up virtio_disable_cb Michael S. Tsirkin 2021-05-26 8:24 ` Michael S. Tsirkin 2021-05-27 4:01 ` Jason Wang 2021-05-27 4:01 ` Jason Wang 2023-03-30 6:07 ` Xuan Zhuo 2023-03-30 6:07 ` Xuan Zhuo 2023-03-30 6:44 ` Michael S. Tsirkin 2023-03-30 6:44 ` Michael S. Tsirkin 2023-03-30 6:54 ` Xuan Zhuo 2023-03-30 6:54 ` Xuan Zhuo 2023-03-30 14:04 ` Michael S. Tsirkin 2023-03-30 14:04 ` Michael S. Tsirkin 2023-03-31 3:38 ` Xuan Zhuo 2023-03-31 3:38 ` Xuan Zhuo 2021-05-26 8:24 ` [PATCH v3 4/4] virtio_net: disable cb aggressively Michael S. Tsirkin 2021-05-26 8:24 ` Michael S. Tsirkin 2021-05-26 15:15 ` Eric Dumazet 2021-05-26 15:15 ` Eric Dumazet 2021-05-26 21:22 ` Willem de Bruijn 2021-05-26 21:22 ` Willem de Bruijn 2021-05-26 19:39 ` Jakub Kicinski 2021-05-27 4:09 ` Jason Wang 2021-05-27 4:09 ` Jason Wang 2023-01-16 13:41 ` Laurent Vivier 2023-01-16 13:41 ` Laurent Vivier 2023-01-17 3:48 ` Jason Wang 2023-01-17 3:48 ` Jason Wang 2021-05-26 15:34 ` Willem de Bruijn [this message] 2021-05-26 15:34 ` [PATCH v3 0/4] virtio net: spurious interrupt related fixes Willem de Bruijn 2021-06-01 2:53 ` Willem de Bruijn 2021-06-01 2:53 ` Willem de Bruijn 2021-06-09 21:36 ` Willem de Bruijn 2021-06-09 21:36 ` Willem de Bruijn 2021-06-09 22:59 ` Willem de Bruijn 2021-06-09 22:59 ` Willem de Bruijn
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CA+FuTScp-OhBnVzkXcsCBWxmq51VO6+8UGpSU5i3AJQV84eTLg@mail.gmail.com \ --to=willemdebruijn.kernel@gmail.com \ --cc=davem@davemloft.net \ --cc=kuba@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mst@redhat.com \ --cc=netdev@vger.kernel.org \ --cc=virtualization@lists.linux-foundation.org \ --cc=weiwan@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.