[PATCH v4 0/4] virtio-net: disable delayed refill when pausing rx

[PATCH v4 0/4] virtio-net: disable delayed refill when pausing rx

[Bui Quang Minh]

Hi everyone,

This series tries to fix a deadlock in virtio-net when binding/unbinding
XDP program, XDP socket or resizing the rx queue.

When pausing rx (e.g. set up xdp, xsk pool, rx resize), we call
napi_disable() on the receive queue's napi. In delayed refill_work, it
also calls napi_disable() on the receive queue's napi. When
napi_disable() is called on an already disabled napi, it will sleep in
napi_disable_locked while still holding the netdev_lock. As a result,
later napi_enable gets stuck too as it cannot acquire the netdev_lock.
This leads to refill_work and the pause-then-resume tx are stuck
altogether.

This scenario can be reproducible by binding a XDP socket to virtio-net
interface without setting up the fill ring. As a result, try_fill_recv
will fail until the fill ring is set up and refill_work is scheduled.

This fix adds virtnet_rx_(pause/resume)_all helpers and fixes up the
virtnet_rx_resume to disable future and cancel all inflights delayed
refill_work before calling napi_disable() to pause the rx.

Version 4 changes:
- Add force zerocopy mode to xdp_helper
- Make virtio_net selftest use force zerocopy mode
- Move virtio_net selftest to drivers/net/hw

Version 3 changes:
- Patch 1: refactor to avoid code duplication

Version 2 changes:
- Add selftest for deadlock scenario

Thanks,
Quang Minh.

Bui Quang Minh (4):
  virtio-net: disable delayed refill when pausing rx
  selftests: net: move xdp_helper to net/lib
  selftests: net: add flag to force zerocopy mode in xdp_helper
  selftests: net: add a virtio_net deadlock selftest

 drivers/net/virtio_net.c                      | 69 +++++++++++++++----
 tools/testing/selftests/drivers/net/Makefile  |  2 -
 .../testing/selftests/drivers/net/hw/Makefile |  1 +
 .../selftests/drivers/net/hw/virtio_net.py    | 65 +++++++++++++++++
 tools/testing/selftests/drivers/net/queues.py |  4 +-
 tools/testing/selftests/net/lib/.gitignore    |  1 +
 tools/testing/selftests/net/lib/Makefile      |  1 +
 .../{drivers/net => net/lib}/xdp_helper.c     | 13 +++-
 8 files changed, 138 insertions(+), 18 deletions(-)
 create mode 100755 tools/testing/selftests/drivers/net/hw/virtio_net.py
 rename tools/testing/selftests/{drivers/net => net/lib}/xdp_helper.c (90%)

-- 
2.43.0

[PATCH v4 1/4] virtio-net: disable delayed refill when pausing rx

[Bui Quang Minh]

When pausing rx (e.g. set up xdp, xsk pool, rx resize), we call
napi_disable() on the receive queue's napi. In delayed refill_work, it
also calls napi_disable() on the receive queue's napi.  When
napi_disable() is called on an already disabled napi, it will sleep in
napi_disable_locked while still holding the netdev_lock. As a result,
later napi_enable gets stuck too as it cannot acquire the netdev_lock.
This leads to refill_work and the pause-then-resume tx are stuck
altogether.

This scenario can be reproducible by binding a XDP socket to virtio-net
interface without setting up the fill ring. As a result, try_fill_recv
will fail until the fill ring is set up and refill_work is scheduled.

This commit adds virtnet_rx_(pause/resume)_all helpers and fixes up the
virtnet_rx_resume to disable future and cancel all inflights delayed
refill_work before calling napi_disable() to pause the rx.

Fixes: 413f0271f396 ("net: protect NAPI enablement with netdev_lock()")
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 drivers/net/virtio_net.c | 69 +++++++++++++++++++++++++++++++++-------
 1 file changed, 57 insertions(+), 12 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7e4617216a4b..848fab51dfa1 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3342,7 +3342,8 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 	return NETDEV_TX_OK;
 }
 
-static void virtnet_rx_pause(struct virtnet_info *vi, struct receive_queue *rq)
+static void __virtnet_rx_pause(struct virtnet_info *vi,
+			       struct receive_queue *rq)
 {
 	bool running = netif_running(vi->dev);
 
@@ -3352,17 +3353,63 @@ static void virtnet_rx_pause(struct virtnet_info *vi, struct receive_queue *rq)
 	}
 }
 
-static void virtnet_rx_resume(struct virtnet_info *vi, struct receive_queue *rq)
+static void virtnet_rx_pause_all(struct virtnet_info *vi)
+{
+	int i;
+
+	/*
+	 * Make sure refill_work does not run concurrently to
+	 * avoid napi_disable race which leads to deadlock.
+	 */
+	disable_delayed_refill(vi);
+	cancel_delayed_work_sync(&vi->refill);
+	for (i = 0; i < vi->max_queue_pairs; i++)
+		__virtnet_rx_pause(vi, &vi->rq[i]);
+}
+
+static void virtnet_rx_pause(struct virtnet_info *vi, struct receive_queue *rq)
+{
+	/*
+	 * Make sure refill_work does not run concurrently to
+	 * avoid napi_disable race which leads to deadlock.
+	 */
+	disable_delayed_refill(vi);
+	cancel_delayed_work_sync(&vi->refill);
+	__virtnet_rx_pause(vi, rq);
+}
+
+static void __virtnet_rx_resume(struct virtnet_info *vi,
+				struct receive_queue *rq,
+				bool refill)
 {
 	bool running = netif_running(vi->dev);
 
-	if (!try_fill_recv(vi, rq, GFP_KERNEL))
+	if (refill && !try_fill_recv(vi, rq, GFP_KERNEL))
 		schedule_delayed_work(&vi->refill, 0);
 
 	if (running)
 		virtnet_napi_enable(rq);
 }
 
+static void virtnet_rx_resume_all(struct virtnet_info *vi)
+{
+	int i;
+
+	enable_delayed_refill(vi);
+	for (i = 0; i < vi->max_queue_pairs; i++) {
+		if (i < vi->curr_queue_pairs)
+			__virtnet_rx_resume(vi, &vi->rq[i], true);
+		else
+			__virtnet_rx_resume(vi, &vi->rq[i], false);
+	}
+}
+
+static void virtnet_rx_resume(struct virtnet_info *vi, struct receive_queue *rq)
+{
+	enable_delayed_refill(vi);
+	__virtnet_rx_resume(vi, rq, true);
+}
+
 static int virtnet_rx_resize(struct virtnet_info *vi,
 			     struct receive_queue *rq, u32 ring_num)
 {
@@ -5959,12 +6006,12 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 	if (prog)
 		bpf_prog_add(prog, vi->max_queue_pairs - 1);
 
+	virtnet_rx_pause_all(vi);
+
 	/* Make sure NAPI is not using any XDP TX queues for RX. */
 	if (netif_running(dev)) {
-		for (i = 0; i < vi->max_queue_pairs; i++) {
-			virtnet_napi_disable(&vi->rq[i]);
+		for (i = 0; i < vi->max_queue_pairs; i++)
 			virtnet_napi_tx_disable(&vi->sq[i]);
-		}
 	}
 
 	if (!prog) {
@@ -5996,13 +6043,12 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 		vi->xdp_enabled = false;
 	}
 
+	virtnet_rx_resume_all(vi);
 	for (i = 0; i < vi->max_queue_pairs; i++) {
 		if (old_prog)
 			bpf_prog_put(old_prog);
-		if (netif_running(dev)) {
-			virtnet_napi_enable(&vi->rq[i]);
+		if (netif_running(dev))
 			virtnet_napi_tx_enable(&vi->sq[i]);
-		}
 	}
 
 	return 0;
@@ -6014,11 +6060,10 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 			rcu_assign_pointer(vi->rq[i].xdp_prog, old_prog);
 	}
 
+	virtnet_rx_resume_all(vi);
 	if (netif_running(dev)) {
-		for (i = 0; i < vi->max_queue_pairs; i++) {
-			virtnet_napi_enable(&vi->rq[i]);
+		for (i = 0; i < vi->max_queue_pairs; i++)
 			virtnet_napi_tx_enable(&vi->sq[i]);
-		}
 	}
 	if (prog)
 		bpf_prog_sub(prog, vi->max_queue_pairs - 1);
-- 
2.43.0

Re: [PATCH v4 1/4] virtio-net: disable delayed refill when pausing rx

[Jason Wang]

On Thu, Apr 17, 2025 at 3:29 PM Bui Quang Minh <minhquangbui99@gmail.com> wrote:
>
> When pausing rx (e.g. set up xdp, xsk pool, rx resize), we call
> napi_disable() on the receive queue's napi. In delayed refill_work, it
> also calls napi_disable() on the receive queue's napi.  When
> napi_disable() is called on an already disabled napi, it will sleep in
> napi_disable_locked while still holding the netdev_lock. As a result,
> later napi_enable gets stuck too as it cannot acquire the netdev_lock.
> This leads to refill_work and the pause-then-resume tx are stuck
> altogether.
>
> This scenario can be reproducible by binding a XDP socket to virtio-net
> interface without setting up the fill ring. As a result, try_fill_recv
> will fail until the fill ring is set up and refill_work is scheduled.
>
> This commit adds virtnet_rx_(pause/resume)_all helpers and fixes up the
> virtnet_rx_resume to disable future and cancel all inflights delayed
> refill_work before calling napi_disable() to pause the rx.
>
> Fixes: 413f0271f396 ("net: protect NAPI enablement with netdev_lock()")
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>

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

(In the future, we may consider switch to per virtqueue refill work instead)

Thanks

[PATCH v4 2/4] selftests: net: move xdp_helper to net/lib

[Bui Quang Minh]

Move xdp_helper to net/lib to make it easier for other selftests to use
the helper.

Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 tools/testing/selftests/drivers/net/Makefile                  | 2 --
 tools/testing/selftests/drivers/net/queues.py                 | 4 ++--
 tools/testing/selftests/net/lib/.gitignore                    | 1 +
 tools/testing/selftests/net/lib/Makefile                      | 1 +
 tools/testing/selftests/{drivers/net => net/lib}/xdp_helper.c | 0
 5 files changed, 4 insertions(+), 4 deletions(-)
 rename tools/testing/selftests/{drivers/net => net/lib}/xdp_helper.c (100%)

diff --git a/tools/testing/selftests/drivers/net/Makefile b/tools/testing/selftests/drivers/net/Makefile
index 0c95bd944d56..cd74f1eb3193 100644
--- a/tools/testing/selftests/drivers/net/Makefile
+++ b/tools/testing/selftests/drivers/net/Makefile
@@ -6,8 +6,6 @@ TEST_INCLUDES := $(wildcard lib/py/*.py) \
 		 ../../net/net_helper.sh \
 		 ../../net/lib.sh \
 
-TEST_GEN_FILES := xdp_helper
-
 TEST_PROGS := \
 	netcons_basic.sh \
 	netcons_fragmented_msg.sh \
diff --git a/tools/testing/selftests/drivers/net/queues.py b/tools/testing/selftests/drivers/net/queues.py
index 06abd3f233e1..236005290a33 100755
--- a/tools/testing/selftests/drivers/net/queues.py
+++ b/tools/testing/selftests/drivers/net/queues.py
@@ -26,13 +26,13 @@ def nl_get_queues(cfg, nl, qtype='rx'):
 
 def check_xsk(cfg, nl, xdp_queue_id=0) -> None:
     # Probe for support
-    xdp = cmd(f'{cfg.test_dir / "xdp_helper"} - -', fail=False)
+    xdp = cmd(f'{cfg.net_lib_dir / "xdp_helper"} - -', fail=False)
     if xdp.ret == 255:
         raise KsftSkipEx('AF_XDP unsupported')
     elif xdp.ret > 0:
         raise KsftFailEx('unable to create AF_XDP socket')
 
-    with bkg(f'{cfg.test_dir / "xdp_helper"} {cfg.ifindex} {xdp_queue_id}',
+    with bkg(f'{cfg.net_lib_dir / "xdp_helper"} {cfg.ifindex} {xdp_queue_id}',
              ksft_wait=3):
 
         rx = tx = False
diff --git a/tools/testing/selftests/net/lib/.gitignore b/tools/testing/selftests/net/lib/.gitignore
index 1ebc6187f421..bbc97d6bf556 100644
--- a/tools/testing/selftests/net/lib/.gitignore
+++ b/tools/testing/selftests/net/lib/.gitignore
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-only
 csum
+xdp_helper
diff --git a/tools/testing/selftests/net/lib/Makefile b/tools/testing/selftests/net/lib/Makefile
index c22623b9a2a5..88c4bc461459 100644
--- a/tools/testing/selftests/net/lib/Makefile
+++ b/tools/testing/selftests/net/lib/Makefile
@@ -10,6 +10,7 @@ TEST_FILES += ../../../../net/ynl
 
 TEST_GEN_FILES += csum
 TEST_GEN_FILES += $(patsubst %.c,%.o,$(wildcard *.bpf.c))
+TEST_GEN_FILES += xdp_helper
 
 TEST_INCLUDES := $(wildcard py/*.py sh/*.sh)
 
diff --git a/tools/testing/selftests/drivers/net/xdp_helper.c b/tools/testing/selftests/net/lib/xdp_helper.c
similarity index 100%
rename from tools/testing/selftests/drivers/net/xdp_helper.c
rename to tools/testing/selftests/net/lib/xdp_helper.c
-- 
2.43.0

[PATCH v4 3/4] selftests: net: add flag to force zerocopy mode in xdp_helper

[Bui Quang Minh]

This commit adds an optional -z flag to xdp_helper. When this flag is
provided, the XDP socket binding is forced to be in zerocopy mode.

Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 tools/testing/selftests/net/lib/xdp_helper.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/lib/xdp_helper.c b/tools/testing/selftests/net/lib/xdp_helper.c
index aeed25914104..6afd77bfbe8b 100644
--- a/tools/testing/selftests/net/lib/xdp_helper.c
+++ b/tools/testing/selftests/net/lib/xdp_helper.c
@@ -81,8 +81,9 @@ int main(int argc, char **argv)
 	int sock_fd;
 	int queue;
 
-	if (argc != 3) {
-		fprintf(stderr, "Usage: %s ifindex queue_id\n", argv[0]);
+	if (argc != 3 && argc != 4) {
+		fprintf(stderr, "Usage: %s ifindex queue_id [-z]\n\n"
+			"where:\n\t-z: force zerocopy mode", argv[0]);
 		return 1;
 	}
 
@@ -132,6 +133,14 @@ int main(int argc, char **argv)
 	sxdp.sxdp_queue_id = queue;
 	sxdp.sxdp_flags = 0;
 
+	if (argc == 4 && strcmp(argv[3], "-z")) {
+		fprintf(stderr, "Usage: %s ifindex queue_id [-z]\n\n"
+			"where:\n\t-z: force zerocopy mode\n", argv[0]);
+		return 1;
+	} else if (argc == 4) {
+		sxdp.sxdp_flags = XDP_ZEROCOPY;
+	}
+
 	if (bind(sock_fd, (struct sockaddr *)&sxdp, sizeof(sxdp)) != 0) {
 		munmap(umem_area, UMEM_SZ);
 		perror("bind failed");
-- 
2.43.0

Re: [PATCH v4 3/4] selftests: net: add flag to force zerocopy mode in xdp_helper

[Jakub Kicinski]

On Thu, 17 Apr 2025 14:28:05 +0700 Bui Quang Minh wrote:
> +	if (argc == 4 && strcmp(argv[3], "-z")) {
> +		fprintf(stderr, "Usage: %s ifindex queue_id [-z]\n\n"
> +			"where:\n\t-z: force zerocopy mode\n", argv[0]);
> +		return 1;
> +	} else if (argc == 4) {
> +		sxdp.sxdp_flags = XDP_ZEROCOPY;
> +	}

I've applied the fix, the tests need a couple touch ups, so please
repost just the tests for net-next?

Here I think it'd be cleaner to write the change as:

static void print_usage(const char *bin)
{
	fprintf(stderr, "Usage: %s ifindex queue_id [-z]\n\n"
		"where:\n\t-z: force zerocopy mode\n", bin);	
}

[...]

	if (argc > 3) {
		if (!strcmp(argv[3], "-z")) {
			sxdp.sxdp_flags |= XDP_ZEROCOPY;
		} else {
			print_usage(argv[0]);
			return 1;
		}
	}

[PATCH v4 4/4] selftests: net: add a virtio_net deadlock selftest

[Bui Quang Minh]

The selftest reproduces the deadlock scenario when binding/unbinding XDP
program, XDP socket, rx ring resize on virtio_net interface.

Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 .../testing/selftests/drivers/net/hw/Makefile |  1 +
 .../selftests/drivers/net/hw/virtio_net.py    | 65 +++++++++++++++++++
 2 files changed, 66 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/net/hw/virtio_net.py

diff --git a/tools/testing/selftests/drivers/net/hw/Makefile b/tools/testing/selftests/drivers/net/hw/Makefile
index 07cddb19ba35..b5af7c1412bf 100644
--- a/tools/testing/selftests/drivers/net/hw/Makefile
+++ b/tools/testing/selftests/drivers/net/hw/Makefile
@@ -21,6 +21,7 @@ TEST_PROGS = \
 	rss_ctx.py \
 	rss_input_xfrm.py \
 	tso.py \
+	virtio_net.py \
 	#
 
 TEST_FILES := \
diff --git a/tools/testing/selftests/drivers/net/hw/virtio_net.py b/tools/testing/selftests/drivers/net/hw/virtio_net.py
new file mode 100755
index 000000000000..7cad7ab98635
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/hw/virtio_net.py
@@ -0,0 +1,65 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: GPL-2.0
+
+# This is intended to be run on a virtio-net guest interface.
+# The test binds the XDP socket to the interface without setting
+# the fill ring to trigger delayed refill_work. This helps to
+# make it easier to reproduce the deadlock when XDP program,
+# XDP socket bind/unbind, rx ring resize race with refill_work on
+# the buggy kernel.
+#
+# The Qemu command to setup virtio-net
+# -netdev tap,id=hostnet1,vhost=on,script=no,downscript=no
+# -device virtio-net-pci,netdev=hostnet1,iommu_platform=on,disable-legacy=on
+
+from lib.py import ksft_exit, ksft_run
+from lib.py import KsftSkipEx, KsftFailEx
+from lib.py import NetDrvEnv
+from lib.py import bkg, ip, cmd, ethtool
+import re
+
+def _get_rx_ring_entries(cfg):
+    output = ethtool(f"-g {cfg.ifname}").stdout
+    values = re.findall(r'RX:\s+(\d+)', output)
+    return int(values[1])
+
+def setup_xsk(cfg, xdp_queue_id = 0) -> bkg:
+    # Probe for support
+    xdp = cmd(f'{cfg.net_lib_dir / "xdp_helper"} - -', fail=False)
+    if xdp.ret == 255:
+        raise KsftSkipEx('AF_XDP unsupported')
+    elif xdp.ret > 0:
+        raise KsftFailEx('unable to create AF_XDP socket')
+
+    try:
+        xsk_bkg = bkg(f'{cfg.net_lib_dir / "xdp_helper"} {cfg.ifindex} ' \
+                      '{xdp_queue_id} -z', ksft_wait=3)
+        return xsk_bkg
+    except:
+        raise KsftSkipEx('Failed to bind XDP socket in zerocopy. ' \
+                         'Please consider adding iommu_platform=on ' \
+                         'when setting up virtio-net-pci')
+
+def check_xdp_bind(cfg):
+    ip(f"link set dev %s xdp obj %s sec xdp" %
+       (cfg.ifname, cfg.net_lib_dir / "xdp_dummy.bpf.o"))
+    ip(f"link set dev %s xdp off" % cfg.ifname)
+
+def check_rx_resize(cfg, queue_size = 128):
+    rx_ring = _get_rx_ring_entries(cfg)
+    ethtool(f"-G %s rx %d" % (cfg.ifname, queue_size))
+    ethtool(f"-G %s rx %d" % (cfg.ifname, rx_ring))
+
+def main():
+    with NetDrvEnv(__file__, nsim_test=False) as cfg:
+        try:
+            xsk_bkg = setup_xsk(cfg)
+        except KsftSkipEx as e:
+            print(f"WARN: xsk pool is not set up, err: {e}")
+
+        ksft_run([check_xdp_bind, check_rx_resize],
+                 args=(cfg, ))
+    ksft_exit()
+
+if __name__ == "__main__":
+    main()
-- 
2.43.0

Re: [PATCH v4 4/4] selftests: net: add a virtio_net deadlock selftest

[Jakub Kicinski]

On Thu, 17 Apr 2025 14:28:06 +0700 Bui Quang Minh wrote:
> The selftest reproduces the deadlock scenario when binding/unbinding XDP
> program, XDP socket, rx ring resize on virtio_net interface.
> 
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> ---
>  .../testing/selftests/drivers/net/hw/Makefile |  1 +
>  .../selftests/drivers/net/hw/virtio_net.py    | 65 +++++++++++++++++++
>  2 files changed, 66 insertions(+)
>  create mode 100755 tools/testing/selftests/drivers/net/hw/virtio_net.py
> 
> diff --git a/tools/testing/selftests/drivers/net/hw/Makefile b/tools/testing/selftests/drivers/net/hw/Makefile
> index 07cddb19ba35..b5af7c1412bf 100644
> --- a/tools/testing/selftests/drivers/net/hw/Makefile
> +++ b/tools/testing/selftests/drivers/net/hw/Makefile
> @@ -21,6 +21,7 @@ TEST_PROGS = \
>  	rss_ctx.py \
>  	rss_input_xfrm.py \
>  	tso.py \
> +	virtio_net.py \

Maybe xsk_reconfig.py ? Other drivers will benefit from this test, too,
and that's a more descriptive name.

>  	#
>  
>  TEST_FILES := \
> diff --git a/tools/testing/selftests/drivers/net/hw/virtio_net.py b/tools/testing/selftests/drivers/net/hw/virtio_net.py
> new file mode 100755
> index 000000000000..7cad7ab98635
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/virtio_net.py
> @@ -0,0 +1,65 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0
> +
> +# This is intended to be run on a virtio-net guest interface.
> +# The test binds the XDP socket to the interface without setting
> +# the fill ring to trigger delayed refill_work. This helps to
> +# make it easier to reproduce the deadlock when XDP program,
> +# XDP socket bind/unbind, rx ring resize race with refill_work on
> +# the buggy kernel.
> +#
> +# The Qemu command to setup virtio-net
> +# -netdev tap,id=hostnet1,vhost=on,script=no,downscript=no
> +# -device virtio-net-pci,netdev=hostnet1,iommu_platform=on,disable-legacy=on
> +
> +from lib.py import ksft_exit, ksft_run
> +from lib.py import KsftSkipEx, KsftFailEx
> +from lib.py import NetDrvEnv
> +from lib.py import bkg, ip, cmd, ethtool
> +import re
> +
> +def _get_rx_ring_entries(cfg):
> +    output = ethtool(f"-g {cfg.ifname}").stdout
> +    values = re.findall(r'RX:\s+(\d+)', output)

no need for the regexps, ethtool -g supports json formatting:

	output = ethtool(f"-g {cfg.ifname}", json=True)[0]
	return output["rx"]

?

> +    return int(values[1])
> +
> +def setup_xsk(cfg, xdp_queue_id = 0) -> bkg:
> +    # Probe for support
> +    xdp = cmd(f'{cfg.net_lib_dir / "xdp_helper"} - -', fail=False)
> +    if xdp.ret == 255:
> +        raise KsftSkipEx('AF_XDP unsupported')
> +    elif xdp.ret > 0:
> +        raise KsftFailEx('unable to create AF_XDP socket')
> +
> +    try:
> +        xsk_bkg = bkg(f'{cfg.net_lib_dir / "xdp_helper"} {cfg.ifindex} ' \
> +                      '{xdp_queue_id} -z', ksft_wait=3)

This process will time out after 3 seconds but the test really
shouldn't leave things running after it exits. Don't worry about
the couple of seconds of execution time. Wrap each test in

	with bkg(f"... the exec info ... "):
		# test code here

The bkg() class has an __exit__() handle once the test finishes
and leaves the with block it will terminate.

> +        return xsk_bkg
> +    except:
> +        raise KsftSkipEx('Failed to bind XDP socket in zerocopy. ' \
> +                         'Please consider adding iommu_platform=on ' \
> +                         'when setting up virtio-net-pci')
> +
> +def check_xdp_bind(cfg):
> +    ip(f"link set dev %s xdp obj %s sec xdp" %
> +       (cfg.ifname, cfg.net_lib_dir / "xdp_dummy.bpf.o"))
> +    ip(f"link set dev %s xdp off" % cfg.ifname)
> +
> +def check_rx_resize(cfg, queue_size = 128):
> +    rx_ring = _get_rx_ring_entries(cfg)
> +    ethtool(f"-G %s rx %d" % (cfg.ifname, queue_size))
> +    ethtool(f"-G %s rx %d" % (cfg.ifname, rx_ring))

Why guess the ring size? What if it's already 128? I usually do:

	rx_ring = _get_rx_ring_entries(cfg)
	ethtool(f"-G %s rx %d" % (cfg.ifname, rx_ring / 2))
	ethtool(f"-G %s rx %d" % (cfg.ifname, rx_ring))

IOW flip between half or double and current.

> +def main():
> +    with NetDrvEnv(__file__, nsim_test=False) as cfg:
> +        try:
> +            xsk_bkg = setup_xsk(cfg)
> +        except KsftSkipEx as e:
> +            print(f"WARN: xsk pool is not set up, err: {e}")
> +
> +        ksft_run([check_xdp_bind, check_rx_resize],
> +                 args=(cfg, ))
> +    ksft_exit()
> +
> +if __name__ == "__main__":
> +    main()

Re: [PATCH v4 4/4] selftests: net: add a virtio_net deadlock selftest

[Bui Quang Minh]

On 4/23/25 08:41, Jakub Kicinski wrote:
> On Thu, 17 Apr 2025 14:28:06 +0700 Bui Quang Minh wrote:
>> The selftest reproduces the deadlock scenario when binding/unbinding XDP
>> program, XDP socket, rx ring resize on virtio_net interface.
>>
>> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
>> ---
>>   .../testing/selftests/drivers/net/hw/Makefile |  1 +
>>   .../selftests/drivers/net/hw/virtio_net.py    | 65 +++++++++++++++++++
>>   2 files changed, 66 insertions(+)
>>   create mode 100755 tools/testing/selftests/drivers/net/hw/virtio_net.py
>>
>> diff --git a/tools/testing/selftests/drivers/net/hw/Makefile b/tools/testing/selftests/drivers/net/hw/Makefile
>> index 07cddb19ba35..b5af7c1412bf 100644
>> --- a/tools/testing/selftests/drivers/net/hw/Makefile
>> +++ b/tools/testing/selftests/drivers/net/hw/Makefile
>> @@ -21,6 +21,7 @@ TEST_PROGS = \
>>   	rss_ctx.py \
>>   	rss_input_xfrm.py \
>>   	tso.py \
>> +	virtio_net.py \
> Maybe xsk_reconfig.py ? Other drivers will benefit from this test, too,
> and that's a more descriptive name.
>
>>   	#
>>   
>>   TEST_FILES := \
>> diff --git a/tools/testing/selftests/drivers/net/hw/virtio_net.py b/tools/testing/selftests/drivers/net/hw/virtio_net.py
>> new file mode 100755
>> index 000000000000..7cad7ab98635
>> --- /dev/null
>> +++ b/tools/testing/selftests/drivers/net/hw/virtio_net.py
>> @@ -0,0 +1,65 @@
>> +#!/usr/bin/env python3
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +# This is intended to be run on a virtio-net guest interface.
>> +# The test binds the XDP socket to the interface without setting
>> +# the fill ring to trigger delayed refill_work. This helps to
>> +# make it easier to reproduce the deadlock when XDP program,
>> +# XDP socket bind/unbind, rx ring resize race with refill_work on
>> +# the buggy kernel.
>> +#
>> +# The Qemu command to setup virtio-net
>> +# -netdev tap,id=hostnet1,vhost=on,script=no,downscript=no
>> +# -device virtio-net-pci,netdev=hostnet1,iommu_platform=on,disable-legacy=on
>> +
>> +from lib.py import ksft_exit, ksft_run
>> +from lib.py import KsftSkipEx, KsftFailEx
>> +from lib.py import NetDrvEnv
>> +from lib.py import bkg, ip, cmd, ethtool
>> +import re
>> +
>> +def _get_rx_ring_entries(cfg):
>> +    output = ethtool(f"-g {cfg.ifname}").stdout
>> +    values = re.findall(r'RX:\s+(\d+)', output)
> no need for the regexps, ethtool -g supports json formatting:
>
> 	output = ethtool(f"-g {cfg.ifname}", json=True)[0]
> 	return output["rx"]
>
> ?
>
>> +    return int(values[1])
>> +
>> +def setup_xsk(cfg, xdp_queue_id = 0) -> bkg:
>> +    # Probe for support
>> +    xdp = cmd(f'{cfg.net_lib_dir / "xdp_helper"} - -', fail=False)
>> +    if xdp.ret == 255:
>> +        raise KsftSkipEx('AF_XDP unsupported')
>> +    elif xdp.ret > 0:
>> +        raise KsftFailEx('unable to create AF_XDP socket')
>> +
>> +    try:
>> +        xsk_bkg = bkg(f'{cfg.net_lib_dir / "xdp_helper"} {cfg.ifindex} ' \
>> +                      '{xdp_queue_id} -z', ksft_wait=3)
> This process will time out after 3 seconds but the test really
> shouldn't leave things running after it exits. Don't worry about
> the couple of seconds of execution time. Wrap each test in
>
> 	with bkg(f"... the exec info ... "):
> 		# test code here
>
> The bkg() class has an __exit__() handle once the test finishes
> and leaves the with block it will terminate.

I've tried to make the setup_xsk into each test. However, I've an issue 
that the XDP socket destruct waits for an RCU grace period as I see this 
sock's flag SOCK_RCU_FREE is set. So if we start the next test right 
away, we can have the error when setting up XDP socket again because 
previous XDP socket has not unbound the network interface's queue yet. I 
can resolve the issue by putting the sleep(1) after closing the socket 
in xdp_helper:

diff --git a/tools/testing/selftests/net/lib/xdp_helper.c 
b/tools/testing/selftests/net/lib/xdp_helper.c
index f21536ab95ba..e882bb22877f 100644
--- a/tools/testing/selftests/net/lib/xdp_helper.c
+++ b/tools/testing/selftests/net/lib/xdp_helper.c
@@ -162,5 +162,6 @@ int main(int argc, char **argv)
          */

         close(sock_fd);
+       sleep(1);
         return 0;
  }

Do you think it's enough or do you have a better suggestion here?

Thanks,
Quang Minh.

>
>> +        return xsk_bkg
>> +    except:
>> +        raise KsftSkipEx('Failed to bind XDP socket in zerocopy. ' \
>> +                         'Please consider adding iommu_platform=on ' \
>> +                         'when setting up virtio-net-pci')
>> +
>> +def check_xdp_bind(cfg):
>> +    ip(f"link set dev %s xdp obj %s sec xdp" %
>> +       (cfg.ifname, cfg.net_lib_dir / "xdp_dummy.bpf.o"))
>> +    ip(f"link set dev %s xdp off" % cfg.ifname)
>> +
>> +def check_rx_resize(cfg, queue_size = 128):
>> +    rx_ring = _get_rx_ring_entries(cfg)
>> +    ethtool(f"-G %s rx %d" % (cfg.ifname, queue_size))
>> +    ethtool(f"-G %s rx %d" % (cfg.ifname, rx_ring))
> Why guess the ring size? What if it's already 128? I usually do:
>
> 	rx_ring = _get_rx_ring_entries(cfg)
> 	ethtool(f"-G %s rx %d" % (cfg.ifname, rx_ring / 2))
> 	ethtool(f"-G %s rx %d" % (cfg.ifname, rx_ring))
>
> IOW flip between half or double and current.
>
>> +def main():
>> +    with NetDrvEnv(__file__, nsim_test=False) as cfg:
>> +        try:
>> +            xsk_bkg = setup_xsk(cfg)
>> +        except KsftSkipEx as e:
>> +            print(f"WARN: xsk pool is not set up, err: {e}")
>> +
>> +        ksft_run([check_xdp_bind, check_rx_resize],
>> +                 args=(cfg, ))
>> +    ksft_exit()
>> +
>> +if __name__ == "__main__":
>> +    main()

Re: [PATCH v4 4/4] selftests: net: add a virtio_net deadlock selftest

[Jakub Kicinski]

On Wed, 23 Apr 2025 22:20:41 +0700 Bui Quang Minh wrote:
> I've tried to make the setup_xsk into each test. However, I've an issue 
> that the XDP socket destruct waits for an RCU grace period as I see this 
> sock's flag SOCK_RCU_FREE is set. So if we start the next test right 
> away, we can have the error when setting up XDP socket again because 
> previous XDP socket has not unbound the network interface's queue yet. I 
> can resolve the issue by putting the sleep(1) after closing the socket 
> in xdp_helper:
> 
> diff --git a/tools/testing/selftests/net/lib/xdp_helper.c 
> b/tools/testing/selftests/net/lib/xdp_helper.c
> index f21536ab95ba..e882bb22877f 100644
> --- a/tools/testing/selftests/net/lib/xdp_helper.c
> +++ b/tools/testing/selftests/net/lib/xdp_helper.c
> @@ -162,5 +162,6 @@ int main(int argc, char **argv)
>           */
> 
>          close(sock_fd);
> +       sleep(1);
>          return 0;
>   }
> 
> Do you think it's enough or do you have a better suggestion here?

Interesting :S What errno does the kernel return? EBUSY?
Perhaps we could loop for a second retrying the bind()
if kernel returns EBUSY in case it's just a socket waiting
to be cleaned up?

Re: [PATCH v4 4/4] selftests: net: add a virtio_net deadlock selftest

[Bui Quang Minh]

On 4/24/25 05:23, Jakub Kicinski wrote:
> On Wed, 23 Apr 2025 22:20:41 +0700 Bui Quang Minh wrote:
>> I've tried to make the setup_xsk into each test. However, I've an issue
>> that the XDP socket destruct waits for an RCU grace period as I see this
>> sock's flag SOCK_RCU_FREE is set. So if we start the next test right
>> away, we can have the error when setting up XDP socket again because
>> previous XDP socket has not unbound the network interface's queue yet. I
>> can resolve the issue by putting the sleep(1) after closing the socket
>> in xdp_helper:
>>
>> diff --git a/tools/testing/selftests/net/lib/xdp_helper.c
>> b/tools/testing/selftests/net/lib/xdp_helper.c
>> index f21536ab95ba..e882bb22877f 100644
>> --- a/tools/testing/selftests/net/lib/xdp_helper.c
>> +++ b/tools/testing/selftests/net/lib/xdp_helper.c
>> @@ -162,5 +162,6 @@ int main(int argc, char **argv)
>>            */
>>
>>           close(sock_fd);
>> +       sleep(1);
>>           return 0;
>>    }
>>
>> Do you think it's enough or do you have a better suggestion here?
> Interesting :S What errno does the kernel return? EBUSY?
> Perhaps we could loop for a second retrying the bind()
> if kernel returns EBUSY in case it's just a socket waiting
> to be cleaned up?

Yes, the kernel returns EBUSY. Loop and retry sounds good to me but it's 
not easy to get the return code when using bkg(). So for simplicity, 
I'll retry with sleep(1) 3 times when the xdp_helper fails.

Thanks,
Quang Minh.

Re: [PATCH v4 4/4] selftests: net: add a virtio_net deadlock selftest

[Jakub Kicinski]

On Thu, 24 Apr 2025 17:33:49 +0700 Bui Quang Minh wrote:
> Yes, the kernel returns EBUSY. Loop and retry sounds good to me but it's 
> not easy to get the return code when using bkg(). So for simplicity, 
> I'll retry with sleep(1) 3 times when the xdp_helper fails.

I meant retry _inside_ the xdp_helper :)
That should be pretty easy?

Re: [PATCH v4 0/4] virtio-net: disable delayed refill when pausing rx

[patchwork-bot+netdevbpf]

Hello:

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

On Thu, 17 Apr 2025 14:28:02 +0700 you wrote:
> Hi everyone,
> 
> This series tries to fix a deadlock in virtio-net when binding/unbinding
> XDP program, XDP socket or resizing the rx queue.
> 
> When pausing rx (e.g. set up xdp, xsk pool, rx resize), we call
> napi_disable() on the receive queue's napi. In delayed refill_work, it
> also calls napi_disable() on the receive queue's napi. When
> napi_disable() is called on an already disabled napi, it will sleep in
> napi_disable_locked while still holding the netdev_lock. As a result,
> later napi_enable gets stuck too as it cannot acquire the netdev_lock.
> This leads to refill_work and the pause-then-resume tx are stuck
> altogether.
> 
> [...]

Here is the summary with links:
  - [v4,1/4] virtio-net: disable delayed refill when pausing rx
    https://git.kernel.org/netdev/net/c/4bc12818b363
  - [v4,2/4] selftests: net: move xdp_helper to net/lib
    (no matching commit)
  - [v4,3/4] selftests: net: add flag to force zerocopy mode in xdp_helper
    (no matching commit)
  - [v4,4/4] selftests: net: add a virtio_net deadlock selftest
    (no matching commit)

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