[PATCH net] virtio_net: move netdev_tx_reset_queue() call before RX napi enable

[PATCH net] virtio_net: move netdev_tx_reset_queue() call before RX napi enable

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

During suspend/resume the following BUG was hit:
------------[ cut here ]------------
kernel BUG at lib/dynamic_queue_limits.c:99!
Internal error: Oops - BUG: 0 [#1] SMP ARM
Modules linked in: bluetooth ecdh_generic ecc libaes
CPU: 1 PID: 1282 Comm: rtcwake Not tainted
6.10.0-rc3-00732-gc8bd1f7f3e61 #15240
Hardware name: Generic DT based system
PC is at dql_completed+0x270/0x2cc
LR is at __free_old_xmit+0x120/0x198
pc : [<c07ffa54>]    lr : [<c0c42bf4>]    psr: 80000013
...
Flags: Nzcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: 43a4406a  DAC: 00000051
...
Process rtcwake (pid: 1282, stack limit = 0xfbc21278)
Stack: (0xe0805e80 to 0xe0806000)
...
Call trace:
  dql_completed from __free_old_xmit+0x120/0x198
  __free_old_xmit from free_old_xmit+0x44/0xe4
  free_old_xmit from virtnet_poll_tx+0x88/0x1b4
  virtnet_poll_tx from __napi_poll+0x2c/0x1d4
  __napi_poll from net_rx_action+0x140/0x2b4
  net_rx_action from handle_softirqs+0x11c/0x350
  handle_softirqs from call_with_stack+0x18/0x20
  call_with_stack from do_softirq+0x48/0x50
  do_softirq from __local_bh_enable_ip+0xa0/0xa4
  __local_bh_enable_ip from virtnet_open+0xd4/0x21c
  virtnet_open from virtnet_restore+0x94/0x120
  virtnet_restore from virtio_device_restore+0x110/0x1f4
  virtio_device_restore from dpm_run_callback+0x3c/0x100
  dpm_run_callback from device_resume+0x12c/0x2a8
  device_resume from dpm_resume+0x12c/0x1e0
  dpm_resume from dpm_resume_end+0xc/0x18
  dpm_resume_end from suspend_devices_and_enter+0x1f0/0x72c
  suspend_devices_and_enter from pm_suspend+0x270/0x2a0
  pm_suspend from state_store+0x68/0xc8
  state_store from kernfs_fop_write_iter+0x10c/0x1cc
  kernfs_fop_write_iter from vfs_write+0x2b0/0x3dc
  vfs_write from ksys_write+0x5c/0xd4
  ksys_write from ret_fast_syscall+0x0/0x54
Exception stack(0xe8bf1fa8 to 0xe8bf1ff0)
...
---[ end trace 0000000000000000 ]---

After virtnet_napi_enable() is called, the following path is hit:
  __napi_poll()
    -> virtnet_poll()
      -> virtnet_poll_cleantx()
        -> netif_tx_wake_queue()

That wakes the TX queue and allows skbs to be submitted and accounted by
BQL counters.

Then netdev_tx_reset_queue() is called that resets BQL counters and
eventually leads to the BUG in dql_completed().

Move virtnet_napi_tx_enable() what does BQL counters reset before RX
napi enable to avoid the issue.

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Closes: https://lore.kernel.org/netdev/e632e378-d019-4de7-8f13-07c572ab37a9@samsung.com/
Fixes: c8bd1f7f3e61 ("virtio_net: add support for Byte Queue Limits")
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/net/virtio_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 3f10c72743e9..c6af18948092 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2867,8 +2867,8 @@ static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
 	if (err < 0)
 		goto err_xdp_reg_mem_model;
 
-	virtnet_napi_enable(vi->rq[qp_index].vq, &vi->rq[qp_index].napi);
 	netdev_tx_reset_queue(netdev_get_tx_queue(vi->dev, qp_index));
+	virtnet_napi_enable(vi->rq[qp_index].vq, &vi->rq[qp_index].napi);
 	virtnet_napi_tx_enable(vi, vi->sq[qp_index].vq, &vi->sq[qp_index].napi);
 
 	return 0;
-- 
2.45.2

Re: [PATCH net] virtio_net: move netdev_tx_reset_queue() call before RX napi enable

[Michael S. Tsirkin]

On Wed, Aug 14, 2024 at 02:25:00PM +0200, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> During suspend/resume the following BUG was hit:
> ------------[ cut here ]------------
> kernel BUG at lib/dynamic_queue_limits.c:99!
> Internal error: Oops - BUG: 0 [#1] SMP ARM
> Modules linked in: bluetooth ecdh_generic ecc libaes
> CPU: 1 PID: 1282 Comm: rtcwake Not tainted
> 6.10.0-rc3-00732-gc8bd1f7f3e61 #15240
> Hardware name: Generic DT based system
> PC is at dql_completed+0x270/0x2cc
> LR is at __free_old_xmit+0x120/0x198
> pc : [<c07ffa54>]    lr : [<c0c42bf4>]    psr: 80000013
> ...
> Flags: Nzcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> Control: 10c5387d  Table: 43a4406a  DAC: 00000051
> ...
> Process rtcwake (pid: 1282, stack limit = 0xfbc21278)
> Stack: (0xe0805e80 to 0xe0806000)
> ...
> Call trace:
>   dql_completed from __free_old_xmit+0x120/0x198
>   __free_old_xmit from free_old_xmit+0x44/0xe4
>   free_old_xmit from virtnet_poll_tx+0x88/0x1b4
>   virtnet_poll_tx from __napi_poll+0x2c/0x1d4
>   __napi_poll from net_rx_action+0x140/0x2b4
>   net_rx_action from handle_softirqs+0x11c/0x350
>   handle_softirqs from call_with_stack+0x18/0x20
>   call_with_stack from do_softirq+0x48/0x50
>   do_softirq from __local_bh_enable_ip+0xa0/0xa4
>   __local_bh_enable_ip from virtnet_open+0xd4/0x21c
>   virtnet_open from virtnet_restore+0x94/0x120
>   virtnet_restore from virtio_device_restore+0x110/0x1f4
>   virtio_device_restore from dpm_run_callback+0x3c/0x100
>   dpm_run_callback from device_resume+0x12c/0x2a8
>   device_resume from dpm_resume+0x12c/0x1e0
>   dpm_resume from dpm_resume_end+0xc/0x18
>   dpm_resume_end from suspend_devices_and_enter+0x1f0/0x72c
>   suspend_devices_and_enter from pm_suspend+0x270/0x2a0
>   pm_suspend from state_store+0x68/0xc8
>   state_store from kernfs_fop_write_iter+0x10c/0x1cc
>   kernfs_fop_write_iter from vfs_write+0x2b0/0x3dc
>   vfs_write from ksys_write+0x5c/0xd4
>   ksys_write from ret_fast_syscall+0x0/0x54
> Exception stack(0xe8bf1fa8 to 0xe8bf1ff0)
> ...
> ---[ end trace 0000000000000000 ]---
> 
> After virtnet_napi_enable() is called, the following path is hit:
>   __napi_poll()
>     -> virtnet_poll()
>       -> virtnet_poll_cleantx()
>         -> netif_tx_wake_queue()
> 
> That wakes the TX queue and allows skbs to be submitted and accounted by
> BQL counters.
> 
> Then netdev_tx_reset_queue() is called that resets BQL counters and
> eventually leads to the BUG in dql_completed().
> 
> Move virtnet_napi_tx_enable() what does BQL counters reset before RX
> napi enable to avoid the issue.
> 
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Closes: https://lore.kernel.org/netdev/e632e378-d019-4de7-8f13-07c572ab37a9@samsung.com/
> Fixes: c8bd1f7f3e61 ("virtio_net: add support for Byte Queue Limits")
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  drivers/net/virtio_net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 3f10c72743e9..c6af18948092 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2867,8 +2867,8 @@ static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
>  	if (err < 0)
>  		goto err_xdp_reg_mem_model;
>  
> -	virtnet_napi_enable(vi->rq[qp_index].vq, &vi->rq[qp_index].napi);
>  	netdev_tx_reset_queue(netdev_get_tx_queue(vi->dev, qp_index));
> +	virtnet_napi_enable(vi->rq[qp_index].vq, &vi->rq[qp_index].napi);
>  	virtnet_napi_tx_enable(vi, vi->sq[qp_index].vq, &vi->sq[qp_index].napi);
>  
>  	return 0;
> -- 
> 2.45.2

Re: [PATCH net] virtio_net: move netdev_tx_reset_queue() call before RX napi enable

[Jason Wang]

On Wed, Aug 14, 2024 at 8:29 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Aug 14, 2024 at 02:25:00PM +0200, Jiri Pirko wrote:
> > From: Jiri Pirko <jiri@nvidia.com>
> >
> > During suspend/resume the following BUG was hit:
> > ------------[ cut here ]------------
> > kernel BUG at lib/dynamic_queue_limits.c:99!
> > Internal error: Oops - BUG: 0 [#1] SMP ARM
> > Modules linked in: bluetooth ecdh_generic ecc libaes
> > CPU: 1 PID: 1282 Comm: rtcwake Not tainted
> > 6.10.0-rc3-00732-gc8bd1f7f3e61 #15240
> > Hardware name: Generic DT based system
> > PC is at dql_completed+0x270/0x2cc
> > LR is at __free_old_xmit+0x120/0x198
> > pc : [<c07ffa54>]    lr : [<c0c42bf4>]    psr: 80000013
> > ...
> > Flags: Nzcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> > Control: 10c5387d  Table: 43a4406a  DAC: 00000051
> > ...
> > Process rtcwake (pid: 1282, stack limit = 0xfbc21278)
> > Stack: (0xe0805e80 to 0xe0806000)
> > ...
> > Call trace:
> >   dql_completed from __free_old_xmit+0x120/0x198
> >   __free_old_xmit from free_old_xmit+0x44/0xe4
> >   free_old_xmit from virtnet_poll_tx+0x88/0x1b4
> >   virtnet_poll_tx from __napi_poll+0x2c/0x1d4
> >   __napi_poll from net_rx_action+0x140/0x2b4
> >   net_rx_action from handle_softirqs+0x11c/0x350
> >   handle_softirqs from call_with_stack+0x18/0x20
> >   call_with_stack from do_softirq+0x48/0x50
> >   do_softirq from __local_bh_enable_ip+0xa0/0xa4
> >   __local_bh_enable_ip from virtnet_open+0xd4/0x21c
> >   virtnet_open from virtnet_restore+0x94/0x120
> >   virtnet_restore from virtio_device_restore+0x110/0x1f4
> >   virtio_device_restore from dpm_run_callback+0x3c/0x100
> >   dpm_run_callback from device_resume+0x12c/0x2a8
> >   device_resume from dpm_resume+0x12c/0x1e0
> >   dpm_resume from dpm_resume_end+0xc/0x18
> >   dpm_resume_end from suspend_devices_and_enter+0x1f0/0x72c
> >   suspend_devices_and_enter from pm_suspend+0x270/0x2a0
> >   pm_suspend from state_store+0x68/0xc8
> >   state_store from kernfs_fop_write_iter+0x10c/0x1cc
> >   kernfs_fop_write_iter from vfs_write+0x2b0/0x3dc
> >   vfs_write from ksys_write+0x5c/0xd4
> >   ksys_write from ret_fast_syscall+0x0/0x54
> > Exception stack(0xe8bf1fa8 to 0xe8bf1ff0)
> > ...
> > ---[ end trace 0000000000000000 ]---
> >
> > After virtnet_napi_enable() is called, the following path is hit:
> >   __napi_poll()
> >     -> virtnet_poll()
> >       -> virtnet_poll_cleantx()
> >         -> netif_tx_wake_queue()
> >
> > That wakes the TX queue and allows skbs to be submitted and accounted by
> > BQL counters.
> >
> > Then netdev_tx_reset_queue() is called that resets BQL counters and
> > eventually leads to the BUG in dql_completed().
> >
> > Move virtnet_napi_tx_enable() what does BQL counters reset before RX
> > napi enable to avoid the issue.
> >
> > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Closes: https://lore.kernel.org/netdev/e632e378-d019-4de7-8f13-07c572ab37a9@samsung.com/
> > Fixes: c8bd1f7f3e61 ("virtio_net: add support for Byte Queue Limits")
> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

Re: [PATCH net] virtio_net: move netdev_tx_reset_queue() call before RX napi enable

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 14 Aug 2024 14:25:00 +0200 you wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> During suspend/resume the following BUG was hit:
> ------------[ cut here ]------------
> kernel BUG at lib/dynamic_queue_limits.c:99!
> Internal error: Oops - BUG: 0 [#1] SMP ARM
> Modules linked in: bluetooth ecdh_generic ecc libaes
> CPU: 1 PID: 1282 Comm: rtcwake Not tainted
> 6.10.0-rc3-00732-gc8bd1f7f3e61 #15240
> Hardware name: Generic DT based system
> PC is at dql_completed+0x270/0x2cc
> LR is at __free_old_xmit+0x120/0x198
> pc : [<c07ffa54>]    lr : [<c0c42bf4>]    psr: 80000013
> ...
> Flags: Nzcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> Control: 10c5387d  Table: 43a4406a  DAC: 00000051
> ...
> Process rtcwake (pid: 1282, stack limit = 0xfbc21278)
> Stack: (0xe0805e80 to 0xe0806000)
> ...
> Call trace:
>   dql_completed from __free_old_xmit+0x120/0x198
>   __free_old_xmit from free_old_xmit+0x44/0xe4
>   free_old_xmit from virtnet_poll_tx+0x88/0x1b4
>   virtnet_poll_tx from __napi_poll+0x2c/0x1d4
>   __napi_poll from net_rx_action+0x140/0x2b4
>   net_rx_action from handle_softirqs+0x11c/0x350
>   handle_softirqs from call_with_stack+0x18/0x20
>   call_with_stack from do_softirq+0x48/0x50
>   do_softirq from __local_bh_enable_ip+0xa0/0xa4
>   __local_bh_enable_ip from virtnet_open+0xd4/0x21c
>   virtnet_open from virtnet_restore+0x94/0x120
>   virtnet_restore from virtio_device_restore+0x110/0x1f4
>   virtio_device_restore from dpm_run_callback+0x3c/0x100
>   dpm_run_callback from device_resume+0x12c/0x2a8
>   device_resume from dpm_resume+0x12c/0x1e0
>   dpm_resume from dpm_resume_end+0xc/0x18
>   dpm_resume_end from suspend_devices_and_enter+0x1f0/0x72c
>   suspend_devices_and_enter from pm_suspend+0x270/0x2a0
>   pm_suspend from state_store+0x68/0xc8
>   state_store from kernfs_fop_write_iter+0x10c/0x1cc
>   kernfs_fop_write_iter from vfs_write+0x2b0/0x3dc
>   vfs_write from ksys_write+0x5c/0xd4
>   ksys_write from ret_fast_syscall+0x0/0x54
> Exception stack(0xe8bf1fa8 to 0xe8bf1ff0)
> ...
> 
> [...]

Here is the summary with links:
  - [net] virtio_net: move netdev_tx_reset_queue() call before RX napi enable
    https://git.kernel.org/netdev/net/c/b96ed2c97c79

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html