[PATCH net-next v1 0/9] Devmem TCP minor cleanups and ksft improvements

[PATCH net-next v1 0/9] Devmem TCP minor cleanups and ksft improvements

[Mina Almasry]

Minor cleanups to the devmem tcp code, and not-so-minor improvements to
the ksft.

For the cleanups:
- Address comment from Paolo post-merge.
- Fix whitespace.
- Add improvement dropped from Taehee's fix patch.

For the ksft:
- Add support for ipv4 environment.
- Add support for drivers that are limited to 5-tuple flow steering.
- Improve test by sending 1K data instead of just "hello\nworld"

Cc: sdf@fomichev.me
Cc: ap420073@gmail.com
Cc: praan@google.com
Cc: shivajikant@google.com


Mina Almasry (9):
  net: devmem: move list_add to net_devmem_bind_dmabuf.
  page_pool: fix ugly page_pool formatting
  net: devmem: preserve sockc_err
  net: devmem: ksft: remove ksft_disruptive
  net: devmem: ksft: add ipv4 support
  net: devmem: ksft: add exit_wait to make rx test pass
  net: devmem: ksft: add 5 tuple FS support
  net: devmem: ksft: upgrade rx test to send 1K data
  net: devmem: ncdevmem: remove unused variable

 net/core/devmem.c                             |  5 +-
 net/core/devmem.h                             |  5 +-
 net/core/netdev-genl.c                        |  8 +--
 net/core/page_pool.c                          |  4 +-
 net/ipv4/tcp.c                                | 24 ++++-----
 .../selftests/drivers/net/hw/devmem.py        | 52 +++++++++++++------
 .../selftests/drivers/net/hw/ncdevmem.c       |  1 -
 7 files changed, 59 insertions(+), 40 deletions(-)


base-commit: b8fa067c4a76e9a28f2003a50ff9b60f00b11168
-- 
2.49.0.1101.gccaa498523-goog

[PATCH net-next v1 1/9] net: devmem: move list_add to net_devmem_bind_dmabuf.

[Mina Almasry]

It's annoying for the list_add to be outside net_devmem_bind_dmabuf, but
the list_del is in net_devmem_unbind_dmabuf. Make it consistent by
having both the list_add/del be inside the net_devmem_[un]bind_dmabuf.

Cc: ap420073@gmail.com
Signed-off-by: Mina Almasry <almasrymina@google.com>

---
 net/core/devmem.c      | 5 ++++-
 net/core/devmem.h      | 5 ++++-
 net/core/netdev-genl.c | 8 ++------
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/net/core/devmem.c b/net/core/devmem.c
index 0dba26baae18..b3a62ca0df65 100644
--- a/net/core/devmem.c
+++ b/net/core/devmem.c
@@ -178,7 +178,8 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
 struct net_devmem_dmabuf_binding *
 net_devmem_bind_dmabuf(struct net_device *dev,
 		       enum dma_data_direction direction,
-		       unsigned int dmabuf_fd, struct netlink_ext_ack *extack)
+		       unsigned int dmabuf_fd, struct netdev_nl_sock *priv,
+		       struct netlink_ext_ack *extack)
 {
 	struct net_devmem_dmabuf_binding *binding;
 	static u32 id_alloc_next;
@@ -299,6 +300,8 @@ net_devmem_bind_dmabuf(struct net_device *dev,
 	if (err < 0)
 		goto err_free_chunks;
 
+	list_add(&binding->list, &priv->bindings);
+
 	return binding;
 
 err_free_chunks:
diff --git a/net/core/devmem.h b/net/core/devmem.h
index 58d8d3c1b945..e7ba77050b8f 100644
--- a/net/core/devmem.h
+++ b/net/core/devmem.h
@@ -11,6 +11,7 @@
 #define _NET_DEVMEM_H
 
 #include <net/netmem.h>
+#include <net/netdev_netlink.h>
 
 struct netlink_ext_ack;
 
@@ -82,7 +83,8 @@ void __net_devmem_dmabuf_binding_free(struct work_struct *wq);
 struct net_devmem_dmabuf_binding *
 net_devmem_bind_dmabuf(struct net_device *dev,
 		       enum dma_data_direction direction,
-		       unsigned int dmabuf_fd, struct netlink_ext_ack *extack);
+		       unsigned int dmabuf_fd, struct netdev_nl_sock *priv,
+		       struct netlink_ext_ack *extack);
 struct net_devmem_dmabuf_binding *net_devmem_lookup_dmabuf(u32 id);
 void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding);
 int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
@@ -170,6 +172,7 @@ static inline void __net_devmem_dmabuf_binding_free(struct work_struct *wq)
 static inline struct net_devmem_dmabuf_binding *
 net_devmem_bind_dmabuf(struct net_device *dev, unsigned int dmabuf_fd,
 		       enum dma_data_direction direction,
+		       struct netdev_nl_sock *priv,
 		       struct netlink_ext_ack *extack)
 {
 	return ERR_PTR(-EOPNOTSUPP);
diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index 762570dcda61..2afa7b2141aa 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -908,7 +908,7 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
 	}
 
 	binding = net_devmem_bind_dmabuf(netdev, DMA_FROM_DEVICE, dmabuf_fd,
-					 info->extack);
+					 priv, info->extack);
 	if (IS_ERR(binding)) {
 		err = PTR_ERR(binding);
 		goto err_unlock;
@@ -943,8 +943,6 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
 			goto err_unbind;
 	}
 
-	list_add(&binding->list, &priv->bindings);
-
 	nla_put_u32(rsp, NETDEV_A_DMABUF_ID, binding->id);
 	genlmsg_end(rsp, hdr);
 
@@ -1020,15 +1018,13 @@ int netdev_nl_bind_tx_doit(struct sk_buff *skb, struct genl_info *info)
 		goto err_unlock_netdev;
 	}
 
-	binding = net_devmem_bind_dmabuf(netdev, DMA_TO_DEVICE, dmabuf_fd,
+	binding = net_devmem_bind_dmabuf(netdev, DMA_TO_DEVICE, dmabuf_fd, priv,
 					 info->extack);
 	if (IS_ERR(binding)) {
 		err = PTR_ERR(binding);
 		goto err_unlock_netdev;
 	}
 
-	list_add(&binding->list, &priv->bindings);
-
 	nla_put_u32(rsp, NETDEV_A_DMABUF_ID, binding->id);
 	genlmsg_end(rsp, hdr);
 
-- 
2.49.0.1101.gccaa498523-goog

[PATCH net-next v1 2/9] page_pool: fix ugly page_pool formatting

[Mina Almasry]

Minor cleanup; this line is badly formatted.

Signed-off-by: Mina Almasry <almasrymina@google.com>

---
 net/core/page_pool.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 974f3eef2efa..4011eb305cee 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -867,8 +867,8 @@ void page_pool_put_unrefed_netmem(struct page_pool *pool, netmem_ref netmem,
 	if (!allow_direct)
 		allow_direct = page_pool_napi_local(pool);
 
-	netmem =
-		__page_pool_put_page(pool, netmem, dma_sync_size, allow_direct);
+	netmem = __page_pool_put_page(pool, netmem, dma_sync_size,
+				      allow_direct);
 	if (netmem && !page_pool_recycle_in_ring(pool, netmem)) {
 		/* Cache full, fallback to free pages */
 		recycle_stat_inc(pool, ring_full);
-- 
2.49.0.1101.gccaa498523-goog

[PATCH net-next v1 3/9] net: devmem: preserve sockc_err

[Mina Almasry]

Preserve the error code returned by sock_cmsg_send and return that on
err.

Signed-off-by: Mina Almasry <almasrymina@google.com>
---
 net/ipv4/tcp.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index b7b6ab41b496..45abe5772157 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1067,7 +1067,7 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
 	int flags, err, copied = 0;
 	int mss_now = 0, size_goal, copied_syn = 0;
 	int process_backlog = 0;
-	bool sockc_valid = true;
+	int sockc_err = 0;
 	int zc = 0;
 	long timeo;
 
@@ -1075,13 +1075,10 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
 
 	sockc = (struct sockcm_cookie){ .tsflags = READ_ONCE(sk->sk_tsflags) };
 	if (msg->msg_controllen) {
-		err = sock_cmsg_send(sk, msg, &sockc);
-		if (unlikely(err))
-			/* Don't return error until MSG_FASTOPEN has been
-			 * processed; that may succeed even if the cmsg is
-			 * invalid.
-			 */
-			sockc_valid = false;
+		sockc_err = sock_cmsg_send(sk, msg, &sockc);
+		/* Don't return error until MSG_FASTOPEN has been processed;
+		 * that may succeed even if the cmsg is invalid.
+		 */
 	}
 
 	if ((flags & MSG_ZEROCOPY) && size) {
@@ -1092,7 +1089,7 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
 		} else if (sock_flag(sk, SOCK_ZEROCOPY)) {
 			skb = tcp_write_queue_tail(sk);
 			uarg = msg_zerocopy_realloc(sk, size, skb_zcopy(skb),
-						    sockc_valid && !!sockc.dmabuf_id);
+						    !sockc_err && !!sockc.dmabuf_id);
 			if (!uarg) {
 				err = -ENOBUFS;
 				goto out_err;
@@ -1102,7 +1099,7 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
 			else
 				uarg_to_msgzc(uarg)->zerocopy = 0;
 
-			if (sockc_valid && sockc.dmabuf_id) {
+			if (!sockc_err && sockc.dmabuf_id) {
 				binding = net_devmem_get_binding(sk, sockc.dmabuf_id);
 				if (IS_ERR(binding)) {
 					err = PTR_ERR(binding);
@@ -1116,7 +1113,7 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
 			zc = MSG_SPLICE_PAGES;
 	}
 
-	if (sockc_valid && sockc.dmabuf_id &&
+	if (!sockc_err && sockc.dmabuf_id &&
 	    (!(flags & MSG_ZEROCOPY) || !sock_flag(sk, SOCK_ZEROCOPY))) {
 		err = -EINVAL;
 		goto out_err;
@@ -1160,9 +1157,8 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
 		/* 'common' sending to sendq */
 	}
 
-	if (!sockc_valid) {
-		if (!err)
-			err = -EINVAL;
+	if (!!sockc_err) {
+		err = sockc_err;
 		goto out_err;
 	}
 
-- 
2.49.0.1101.gccaa498523-goog

[PATCH net-next v1 4/9] net: devmem: ksft: remove ksft_disruptive

[Mina Almasry]

As far as I can tell the ksft_disruptive here is unnecessary. These
tests are largerly independent, and when one test fails, it's nice to
know the results from all the other test cases.

Signed-off-by: Mina Almasry <almasrymina@google.com>

---
 tools/testing/selftests/drivers/net/hw/devmem.py | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/hw/devmem.py b/tools/testing/selftests/drivers/net/hw/devmem.py
index 7fc686cf47a2..f5d7809400ea 100755
--- a/tools/testing/selftests/drivers/net/hw/devmem.py
+++ b/tools/testing/selftests/drivers/net/hw/devmem.py
@@ -6,7 +6,6 @@ from lib.py import ksft_run, ksft_exit
 from lib.py import ksft_eq, KsftSkipEx
 from lib.py import NetDrvEpEnv
 from lib.py import bkg, cmd, rand_port, wait_port_listen
-from lib.py import ksft_disruptive
 
 
 def require_devmem(cfg):
@@ -19,7 +18,6 @@ def require_devmem(cfg):
         raise KsftSkipEx("Test requires devmem support")
 
 
-@ksft_disruptive
 def check_rx(cfg) -> None:
     cfg.require_ipver("6")
     require_devmem(cfg)
@@ -34,7 +32,6 @@ def check_rx(cfg) -> None:
     ksft_eq(socat.stdout.strip(), "hello\nworld")
 
 
-@ksft_disruptive
 def check_tx(cfg) -> None:
     cfg.require_ipver("6")
     require_devmem(cfg)
-- 
2.49.0.1101.gccaa498523-goog

[PATCH net-next v1 5/9] net: devmem: ksft: add ipv4 support

[Mina Almasry]

ncdevmem supports both ipv4 and ipv6, but the ksft is currently
ipv6-only. Propagate the ipv4 support to the ksft, so that folks that
are limited to these networks can also test.

Signed-off-by: Mina Almasry <almasrymina@google.com>

---
 .../selftests/drivers/net/hw/devmem.py        | 33 ++++++++++++-------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/hw/devmem.py b/tools/testing/selftests/drivers/net/hw/devmem.py
index f5d7809400ea..850381e14d9e 100755
--- a/tools/testing/selftests/drivers/net/hw/devmem.py
+++ b/tools/testing/selftests/drivers/net/hw/devmem.py
@@ -18,30 +18,36 @@ def require_devmem(cfg):
         raise KsftSkipEx("Test requires devmem support")
 
 
-def check_rx(cfg) -> None:
-    cfg.require_ipver("6")
+def check_rx(cfg, ipver) -> None:
     require_devmem(cfg)
 
+    addr = cfg.addr_v[ipver]
+    if ipver == "6":
+        addr = "[" + addr + "]"
+
+    socat = f"socat -u - TCP{ipver}:{addr}:{port}"
+
     port = rand_port()
     listen_cmd = f"{cfg.bin_local} -l -f {cfg.ifname} -s {cfg.addr_v['6']} -p {port}"
 
-    with bkg(listen_cmd) as socat:
+    with bkg(listen_cmd) as ncdevmem:
         wait_port_listen(port)
-        cmd(f"echo -e \"hello\\nworld\"| socat -u - TCP6:[{cfg.addr_v['6']}]:{port}", host=cfg.remote, shell=True)
+        cmd(f"echo -e \"hello\\nworld\"| {socat}", host=cfg.remote, shell=True)
 
-    ksft_eq(socat.stdout.strip(), "hello\nworld")
+    ksft_eq(ncdevmem.stdout.strip(), "hello\nworld")
 
 
-def check_tx(cfg) -> None:
-    cfg.require_ipver("6")
+def check_tx(cfg, ipver) -> None:
     require_devmem(cfg)
 
     port = rand_port()
-    listen_cmd = f"socat -U - TCP6-LISTEN:{port}"
+    listen_cmd = f"socat -U - TCP{ipver}-LISTEN:{port}"
 
-    with bkg(listen_cmd, exit_wait=True) as socat:
+    addr = cfg.addr_v[ipver]
+
+    with bkg(listen_cmd) as socat:
         wait_port_listen(port)
-        cmd(f"echo -e \"hello\\nworld\"| {cfg.bin_remote} -f {cfg.ifname} -s {cfg.addr_v['6']} -p {port}", host=cfg.remote, shell=True)
+        cmd(f"echo -e \"hello\\nworld\"| {cfg.bin_remote} -f {cfg.ifname} -s {addr} -p {port}", host=cfg.remote, shell=True)
 
     ksft_eq(socat.stdout.strip(), "hello\nworld")
 
@@ -51,8 +57,13 @@ def main() -> None:
         cfg.bin_local = path.abspath(path.dirname(__file__) + "/ncdevmem")
         cfg.bin_remote = cfg.remote.deploy(cfg.bin_local)
 
+        if "4" in cfg.addr_v:
+            ipver = "4"
+        else:
+            ipver = "6"
+
         ksft_run([check_rx, check_tx],
-                 args=(cfg, ))
+                 args=(cfg, ipver))
     ksft_exit()
 
 
-- 
2.49.0.1101.gccaa498523-goog

[PATCH net-next v1 6/9] net: devmem: ksft: add exit_wait to make rx test pass

[Mina Almasry]

This exit_wait seems necessary to make the rx side test pass for me.
I think this is just missed from the original test add patch. Add it now.

Signed-off-by: Mina Almasry <almasrymina@google.com>

---
 tools/testing/selftests/drivers/net/hw/devmem.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/drivers/net/hw/devmem.py b/tools/testing/selftests/drivers/net/hw/devmem.py
index 850381e14d9e..39b5241463aa 100755
--- a/tools/testing/selftests/drivers/net/hw/devmem.py
+++ b/tools/testing/selftests/drivers/net/hw/devmem.py
@@ -30,7 +30,7 @@ def check_rx(cfg, ipver) -> None:
     port = rand_port()
     listen_cmd = f"{cfg.bin_local} -l -f {cfg.ifname} -s {cfg.addr_v['6']} -p {port}"
 
-    with bkg(listen_cmd) as ncdevmem:
+    with bkg(listen_cmd, exit_wait=True) as ncdevmem:
         wait_port_listen(port)
         cmd(f"echo -e \"hello\\nworld\"| {socat}", host=cfg.remote, shell=True)
 
-- 
2.49.0.1101.gccaa498523-goog

[PATCH net-next v1 7/9] net: devmem: ksft: add 5 tuple FS support

[Mina Almasry]

ncdevmem supports drivers that are limited to either 3-tuple or 5-tuple
FS support, but the ksft is currently 3-tuple only. Support drivers that
have 5-tuple FS supported by adding a ksft arg.

Signed-off-by: Mina Almasry <almasrymina@google.com>

---
 .../testing/selftests/drivers/net/hw/devmem.py  | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/hw/devmem.py b/tools/testing/selftests/drivers/net/hw/devmem.py
index 39b5241463aa..40fe5b525d51 100755
--- a/tools/testing/selftests/drivers/net/hw/devmem.py
+++ b/tools/testing/selftests/drivers/net/hw/devmem.py
@@ -21,14 +21,27 @@ def require_devmem(cfg):
 def check_rx(cfg, ipver) -> None:
     require_devmem(cfg)
 
+    fs_5_tuple = False
+    if "FLOW_STEERING_5_TUPLE" in cfg.env:
+        fs_5_tuple = cfg.env["FLOW_STEERING_5_TUPLE"]
+
     addr = cfg.addr_v[ipver]
+    remote_addr = cfg.remote_addr_v[ipver]
+    port = rand_port()
+
     if ipver == "6":
         addr = "[" + addr + "]"
+        remote_addr = "[" + remote_addr + "]"
 
     socat = f"socat -u - TCP{ipver}:{addr}:{port}"
 
-    port = rand_port()
-    listen_cmd = f"{cfg.bin_local} -l -f {cfg.ifname} -s {cfg.addr_v['6']} -p {port}"
+    if fs_5_tuple:
+        socat += f",bind={remote_addr}:{port}"
+
+    listen_cmd = f"{cfg.bin_local} -l -f {cfg.ifname} -s {addr} -p {port}"
+
+    if fs_5_tuple:
+        listen_cmd += f" -c {remote_addr}"
 
     with bkg(listen_cmd, exit_wait=True) as ncdevmem:
         wait_port_listen(port)
-- 
2.49.0.1101.gccaa498523-goog

[PATCH net-next v1 8/9] net: devmem: ksft: upgrade rx test to send 1K data

[Mina Almasry]

The current test just sends "hello\nworld" and verifies that is the
string received on the RX side. That is fine, but improve the test a bit
by sending 1K data. The test should be improved further to send more
data, but for now this should be a welcome improvement.

The test will send a repeating pattern of 0x01, 0x02, ... 0x06. The
ncdevmem `-v 7` flag will verify this pattern. ncdevmem will provide
useful debugging info when the test fails, such as the frags received
and verified fine, and which frag exactly failed, what was the expected
byte pattern, and what is the actual byte pattern received. All this
debug information will be useful when the test fails.

Signed-off-by: Mina Almasry <almasrymina@google.com>

---
 tools/testing/selftests/drivers/net/hw/devmem.py | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/hw/devmem.py b/tools/testing/selftests/drivers/net/hw/devmem.py
index 40fe5b525d51..10ffd8a8f72b 100755
--- a/tools/testing/selftests/drivers/net/hw/devmem.py
+++ b/tools/testing/selftests/drivers/net/hw/devmem.py
@@ -38,16 +38,17 @@ def check_rx(cfg, ipver) -> None:
     if fs_5_tuple:
         socat += f",bind={remote_addr}:{port}"
 
-    listen_cmd = f"{cfg.bin_local} -l -f {cfg.ifname} -s {addr} -p {port}"
+    listen_cmd = f"{cfg.bin_local} -l -f {cfg.ifname} -s {addr} -p {port} -v 7"
 
     if fs_5_tuple:
         listen_cmd += f" -c {remote_addr}"
 
     with bkg(listen_cmd, exit_wait=True) as ncdevmem:
         wait_port_listen(port)
-        cmd(f"echo -e \"hello\\nworld\"| {socat}", host=cfg.remote, shell=True)
+        cmd(f"yes $(echo -e \x01\x02\x03\x04\x05\x06) | \
+            head -c 1K | {socat}", host=cfg.remote, shell=True)
 
-    ksft_eq(ncdevmem.stdout.strip(), "hello\nworld")
+    ksft_eq(ncdevmem.ret, 0)
 
 
 def check_tx(cfg, ipver) -> None:
-- 
2.49.0.1101.gccaa498523-goog

[PATCH net-next v1 9/9] net: devmem: ncdevmem: remove unused variable

[Mina Almasry]

This variable is unused and can be removed.

Signed-off-by: Mina Almasry <almasrymina@google.com>
---
 tools/testing/selftests/drivers/net/hw/ncdevmem.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/testing/selftests/drivers/net/hw/ncdevmem.c b/tools/testing/selftests/drivers/net/hw/ncdevmem.c
index ca723722a810..a86e4ce5e65d 100644
--- a/tools/testing/selftests/drivers/net/hw/ncdevmem.c
+++ b/tools/testing/selftests/drivers/net/hw/ncdevmem.c
@@ -526,7 +526,6 @@ static struct netdev_queue_id *create_queues(void)
 static int do_server(struct memory_buffer *mem)
 {
 	char ctrl_data[sizeof(int) * 20000];
-	struct netdev_queue_id *queues;
 	size_t non_page_aligned_frags = 0;
 	struct sockaddr_in6 client_addr;
 	struct sockaddr_in6 server_sin;
-- 
2.49.0.1101.gccaa498523-goog

Re: [PATCH net-next v1 1/9] net: devmem: move list_add to net_devmem_bind_dmabuf.

[Taehee Yoo]

On Mon, May 19, 2025 at 11:35 AM Mina Almasry <almasrymina@google.com> wrote:
>
> It's annoying for the list_add to be outside net_devmem_bind_dmabuf, but
> the list_del is in net_devmem_unbind_dmabuf. Make it consistent by
> having both the list_add/del be inside the net_devmem_[un]bind_dmabuf.
>
> Cc: ap420073@gmail.com
> Signed-off-by: Mina Almasry <almasrymina@google.com>

Hi Mina,
Thanks a lot for this work!

I tested it and it works well.

Tested-by: Taehee Yoo <ap420073@gmail.com>

Thanks!
Taehee Yoo

>
> ---
>  net/core/devmem.c      | 5 ++++-
>  net/core/devmem.h      | 5 ++++-
>  net/core/netdev-genl.c | 8 ++------
>  3 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index 0dba26baae18..b3a62ca0df65 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c
> @@ -178,7 +178,8 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
>  struct net_devmem_dmabuf_binding *
>  net_devmem_bind_dmabuf(struct net_device *dev,
>                        enum dma_data_direction direction,
> -                      unsigned int dmabuf_fd, struct netlink_ext_ack *extack)
> +                      unsigned int dmabuf_fd, struct netdev_nl_sock *priv,
> +                      struct netlink_ext_ack *extack)
>  {
>         struct net_devmem_dmabuf_binding *binding;
>         static u32 id_alloc_next;
> @@ -299,6 +300,8 @@ net_devmem_bind_dmabuf(struct net_device *dev,
>         if (err < 0)
>                 goto err_free_chunks;
>
> +       list_add(&binding->list, &priv->bindings);
> +
>         return binding;
>
>  err_free_chunks:
> diff --git a/net/core/devmem.h b/net/core/devmem.h
> index 58d8d3c1b945..e7ba77050b8f 100644
> --- a/net/core/devmem.h
> +++ b/net/core/devmem.h
> @@ -11,6 +11,7 @@
>  #define _NET_DEVMEM_H
>
>  #include <net/netmem.h>
> +#include <net/netdev_netlink.h>
>
>  struct netlink_ext_ack;
>
> @@ -82,7 +83,8 @@ void __net_devmem_dmabuf_binding_free(struct work_struct *wq);
>  struct net_devmem_dmabuf_binding *
>  net_devmem_bind_dmabuf(struct net_device *dev,
>                        enum dma_data_direction direction,
> -                      unsigned int dmabuf_fd, struct netlink_ext_ack *extack);
> +                      unsigned int dmabuf_fd, struct netdev_nl_sock *priv,
> +                      struct netlink_ext_ack *extack);
>  struct net_devmem_dmabuf_binding *net_devmem_lookup_dmabuf(u32 id);
>  void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding);
>  int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
> @@ -170,6 +172,7 @@ static inline void __net_devmem_dmabuf_binding_free(struct work_struct *wq)
>  static inline struct net_devmem_dmabuf_binding *
>  net_devmem_bind_dmabuf(struct net_device *dev, unsigned int dmabuf_fd,
>                        enum dma_data_direction direction,
> +                      struct netdev_nl_sock *priv,
>                        struct netlink_ext_ack *extack)
>  {
>         return ERR_PTR(-EOPNOTSUPP);
> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> index 762570dcda61..2afa7b2141aa 100644
> --- a/net/core/netdev-genl.c
> +++ b/net/core/netdev-genl.c
> @@ -908,7 +908,7 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
>         }
>
>         binding = net_devmem_bind_dmabuf(netdev, DMA_FROM_DEVICE, dmabuf_fd,
> -                                        info->extack);
> +                                        priv, info->extack);
>         if (IS_ERR(binding)) {
>                 err = PTR_ERR(binding);
>                 goto err_unlock;
> @@ -943,8 +943,6 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
>                         goto err_unbind;
>         }
>
> -       list_add(&binding->list, &priv->bindings);
> -
>         nla_put_u32(rsp, NETDEV_A_DMABUF_ID, binding->id);
>         genlmsg_end(rsp, hdr);
>
> @@ -1020,15 +1018,13 @@ int netdev_nl_bind_tx_doit(struct sk_buff *skb, struct genl_info *info)
>                 goto err_unlock_netdev;
>         }
>
> -       binding = net_devmem_bind_dmabuf(netdev, DMA_TO_DEVICE, dmabuf_fd,
> +       binding = net_devmem_bind_dmabuf(netdev, DMA_TO_DEVICE, dmabuf_fd, priv,
>                                          info->extack);
>         if (IS_ERR(binding)) {
>                 err = PTR_ERR(binding);
>                 goto err_unlock_netdev;
>         }
>
> -       list_add(&binding->list, &priv->bindings);
> -
>         nla_put_u32(rsp, NETDEV_A_DMABUF_ID, binding->id);
>         genlmsg_end(rsp, hdr);
>
> --
> 2.49.0.1101.gccaa498523-goog
>

Re: [PATCH net-next v1 3/9] net: devmem: preserve sockc_err

[Stanislav Fomichev]

On 05/19, Mina Almasry wrote:
> Preserve the error code returned by sock_cmsg_send and return that on
> err.
> 
> Signed-off-by: Mina Almasry <almasrymina@google.com>
> ---
>  net/ipv4/tcp.c | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index b7b6ab41b496..45abe5772157 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -1067,7 +1067,7 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
>  	int flags, err, copied = 0;
>  	int mss_now = 0, size_goal, copied_syn = 0;
>  	int process_backlog = 0;
> -	bool sockc_valid = true;
> +	int sockc_err = 0;
>  	int zc = 0;
>  	long timeo;
>  
> @@ -1075,13 +1075,10 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
>  
>  	sockc = (struct sockcm_cookie){ .tsflags = READ_ONCE(sk->sk_tsflags) };
>  	if (msg->msg_controllen) {
> -		err = sock_cmsg_send(sk, msg, &sockc);
> -		if (unlikely(err))
> -			/* Don't return error until MSG_FASTOPEN has been
> -			 * processed; that may succeed even if the cmsg is
> -			 * invalid.
> -			 */
> -			sockc_valid = false;
> +		sockc_err = sock_cmsg_send(sk, msg, &sockc);
> +		/* Don't return error until MSG_FASTOPEN has been processed;
> +		 * that may succeed even if the cmsg is invalid.
> +		 */
>  	}
>  
>  	if ((flags & MSG_ZEROCOPY) && size) {
> @@ -1092,7 +1089,7 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
>  		} else if (sock_flag(sk, SOCK_ZEROCOPY)) {
>  			skb = tcp_write_queue_tail(sk);
>  			uarg = msg_zerocopy_realloc(sk, size, skb_zcopy(skb),
> -						    sockc_valid && !!sockc.dmabuf_id);
> +						    !sockc_err && !!sockc.dmabuf_id);

Why have these extra !! here? Other places below simply do '&& sockc.dmabuf_id',
why not the same here? 

>  			if (!uarg) {
>  				err = -ENOBUFS;
>  				goto out_err;
> @@ -1102,7 +1099,7 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
>  			else
>  				uarg_to_msgzc(uarg)->zerocopy = 0;
>  
> -			if (sockc_valid && sockc.dmabuf_id) {
> +			if (!sockc_err && sockc.dmabuf_id) {
>  				binding = net_devmem_get_binding(sk, sockc.dmabuf_id);
>  				if (IS_ERR(binding)) {
>  					err = PTR_ERR(binding);
> @@ -1116,7 +1113,7 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
>  			zc = MSG_SPLICE_PAGES;
>  	}
>  
> -	if (sockc_valid && sockc.dmabuf_id &&
> +	if (!sockc_err && sockc.dmabuf_id &&
>  	    (!(flags & MSG_ZEROCOPY) || !sock_flag(sk, SOCK_ZEROCOPY))) {
>  		err = -EINVAL;
>  		goto out_err;
> @@ -1160,9 +1157,8 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
>  		/* 'common' sending to sendq */
>  	}
>  
> -	if (!sockc_valid) {
> -		if (!err)
> -			err = -EINVAL;
> +	if (!!sockc_err) {

Same here, I don't think we need these extra !! ?

Re: [PATCH net-next v1 4/9] net: devmem: ksft: remove ksft_disruptive

[Stanislav Fomichev]

On 05/19, Mina Almasry wrote:
> As far as I can tell the ksft_disruptive here is unnecessary. These
> tests are largerly independent, and when one test fails, it's nice to
> know the results from all the other test cases.

We currently don't do anything special for disruptive tests. I'm assuming
anything that changes nic configuration is disruptive and was thinking of
an option to run all disruptive tests at the end of the run. But so far we
haven't had any problem with mixing disruptive and non-disruptive tests,
so it's all moot. I'd prefer to keep everything as is for now (or remove
this whole disruptive category).

Re: [PATCH net-next v1 5/9] net: devmem: ksft: add ipv4 support

[Stanislav Fomichev]

On 05/19, Mina Almasry wrote:
> ncdevmem supports both ipv4 and ipv6, but the ksft is currently
> ipv6-only. Propagate the ipv4 support to the ksft, so that folks that
> are limited to these networks can also test.
> 
> Signed-off-by: Mina Almasry <almasrymina@google.com>
> 
> ---
>  .../selftests/drivers/net/hw/devmem.py        | 33 ++++++++++++-------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/testing/selftests/drivers/net/hw/devmem.py b/tools/testing/selftests/drivers/net/hw/devmem.py
> index f5d7809400ea..850381e14d9e 100755
> --- a/tools/testing/selftests/drivers/net/hw/devmem.py
> +++ b/tools/testing/selftests/drivers/net/hw/devmem.py
> @@ -18,30 +18,36 @@ def require_devmem(cfg):
>          raise KsftSkipEx("Test requires devmem support")
>  
>  
> -def check_rx(cfg) -> None:
> -    cfg.require_ipver("6")
> +def check_rx(cfg, ipver) -> None:
>      require_devmem(cfg)
>  
> +    addr = cfg.addr_v[ipver]
> +    if ipver == "6":
> +        addr = "[" + addr + "]"

I think you can add [] unconditionally, no need to special case v6.

> +
> +    socat = f"socat -u - TCP{ipver}:{addr}:{port}"
> +
>      port = rand_port()
>      listen_cmd = f"{cfg.bin_local} -l -f {cfg.ifname} -s {cfg.addr_v['6']} -p {port}"
>  
> -    with bkg(listen_cmd) as socat:
> +    with bkg(listen_cmd) as ncdevmem:
>          wait_port_listen(port)
> -        cmd(f"echo -e \"hello\\nworld\"| socat -u - TCP6:[{cfg.addr_v['6']}]:{port}", host=cfg.remote, shell=True)
> +        cmd(f"echo -e \"hello\\nworld\"| {socat}", host=cfg.remote, shell=True)
>  
> -    ksft_eq(socat.stdout.strip(), "hello\nworld")
> +    ksft_eq(ncdevmem.stdout.strip(), "hello\nworld")
>  
>  
> -def check_tx(cfg) -> None:
> -    cfg.require_ipver("6")
> +def check_tx(cfg, ipver) -> None:
>      require_devmem(cfg)
>  
>      port = rand_port()
> -    listen_cmd = f"socat -U - TCP6-LISTEN:{port}"
> +    listen_cmd = f"socat -U - TCP{ipver}-LISTEN:{port}"
>  
> -    with bkg(listen_cmd, exit_wait=True) as socat:
> +    addr = cfg.addr_v[ipver]
> +
> +    with bkg(listen_cmd) as socat:
>          wait_port_listen(port)
> -        cmd(f"echo -e \"hello\\nworld\"| {cfg.bin_remote} -f {cfg.ifname} -s {cfg.addr_v['6']} -p {port}", host=cfg.remote, shell=True)
> +        cmd(f"echo -e \"hello\\nworld\"| {cfg.bin_remote} -f {cfg.ifname} -s {addr} -p {port}", host=cfg.remote, shell=True)
>  
>      ksft_eq(socat.stdout.strip(), "hello\nworld")
>  
> @@ -51,8 +57,13 @@ def main() -> None:
>          cfg.bin_local = path.abspath(path.dirname(__file__) + "/ncdevmem")
>          cfg.bin_remote = cfg.remote.deploy(cfg.bin_local)
>  
> +        if "4" in cfg.addr_v:
> +            ipver = "4"
> +        else:
> +            ipver = "6"

If we have both, we prefer v4, can we do the opposite? 

Re: [PATCH net-next v1 6/9] net: devmem: ksft: add exit_wait to make rx test pass

[Stanislav Fomichev]

On 05/19, Mina Almasry wrote:
> This exit_wait seems necessary to make the rx side test pass for me.
> I think this is just missed from the original test add patch. Add it now.
> 
> Signed-off-by: Mina Almasry <almasrymina@google.com>

Acked-by: Stanislav Fomichev <sdf@fomichev.me>

Re: [PATCH net-next v1 7/9] net: devmem: ksft: add 5 tuple FS support

[Stanislav Fomichev]

On 05/19, Mina Almasry wrote:
> ncdevmem supports drivers that are limited to either 3-tuple or 5-tuple
> FS support, but the ksft is currently 3-tuple only. Support drivers that
> have 5-tuple FS supported by adding a ksft arg.
> 
> Signed-off-by: Mina Almasry <almasrymina@google.com>
> 
> ---
>  .../testing/selftests/drivers/net/hw/devmem.py  | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/drivers/net/hw/devmem.py b/tools/testing/selftests/drivers/net/hw/devmem.py
> index 39b5241463aa..40fe5b525d51 100755
> --- a/tools/testing/selftests/drivers/net/hw/devmem.py
> +++ b/tools/testing/selftests/drivers/net/hw/devmem.py
> @@ -21,14 +21,27 @@ def require_devmem(cfg):
>  def check_rx(cfg, ipver) -> None:
>      require_devmem(cfg)
>  
> +    fs_5_tuple = False
> +    if "FLOW_STEERING_5_TUPLE" in cfg.env:
> +        fs_5_tuple = cfg.env["FLOW_STEERING_5_TUPLE"]

I wonder if we can transparently handle it in ncdevmem: if -c is passed,
try installing 3-tuple rule, and if it fails, try 5-tuple one. This
should work without any user input / extra environment variable.

>      addr = cfg.addr_v[ipver]
> +    remote_addr = cfg.remote_addr_v[ipver]
> +    port = rand_port()
> +
>      if ipver == "6":
>          addr = "[" + addr + "]"
> +        remote_addr = "[" + remote_addr + "]"
>  
>      socat = f"socat -u - TCP{ipver}:{addr}:{port}"
>  
> -    port = rand_port()
> -    listen_cmd = f"{cfg.bin_local} -l -f {cfg.ifname} -s {cfg.addr_v['6']} -p {port}"
> +    if fs_5_tuple:
> +        socat += f",bind={remote_addr}:{port}"

This we can always do regardless of 3 or 5 tuple?

Re: [PATCH net-next v1 8/9] net: devmem: ksft: upgrade rx test to send 1K data

[Stanislav Fomichev]

On 05/19, Mina Almasry wrote:
> The current test just sends "hello\nworld" and verifies that is the
> string received on the RX side. That is fine, but improve the test a bit
> by sending 1K data. The test should be improved further to send more
> data, but for now this should be a welcome improvement.
> 
> The test will send a repeating pattern of 0x01, 0x02, ... 0x06. The
> ncdevmem `-v 7` flag will verify this pattern. ncdevmem will provide
> useful debugging info when the test fails, such as the frags received
> and verified fine, and which frag exactly failed, what was the expected
> byte pattern, and what is the actual byte pattern received. All this
> debug information will be useful when the test fails.
> 
> Signed-off-by: Mina Almasry <almasrymina@google.com>

Acked-by: Stanislav Fomichev <sdf@fomichev.me>

Re: [PATCH net-next v1 9/9] net: devmem: ncdevmem: remove unused variable

[Stanislav Fomichev]

On 05/19, Mina Almasry wrote:
> This variable is unused and can be removed.
> 
> Signed-off-by: Mina Almasry <almasrymina@google.com>

Acked-by: Stanislav Fomichev <sdf@fomichev.me>

Re: [PATCH net-next v1 4/9] net: devmem: ksft: remove ksft_disruptive

[Mina Almasry]

On Mon, May 19, 2025 at 8:25 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 05/19, Mina Almasry wrote:
> > As far as I can tell the ksft_disruptive here is unnecessary. These
> > tests are largerly independent, and when one test fails, it's nice to
> > know the results from all the other test cases.
>
> We currently don't do anything special for disruptive tests. I'm assuming
> anything that changes nic configuration is disruptive and was thinking of
> an option to run all disruptive tests at the end of the run. But so far we
> haven't had any problem with mixing disruptive and non-disruptive tests,
> so it's all moot. I'd prefer to keep everything as is for now (or remove
> this whole disruptive category).

I've noticed that if all the tests are marked disruptive, and one test
fails, the others don't run at all, which seems unnecessary. I'd like
to see if the rx test passed if the tx one failed and vice versa for
example. Removing the disruptive tag seems to resolve that.

dmabuf bind is automatically unbound when ncdevmem exits, so i don't
think these tests leave the nic in a bad state or anything that
warrants blocking running the other tests?

-- 
Thanks,
Mina

Re: [PATCH net-next v1 5/9] net: devmem: ksft: add ipv4 support

[Mina Almasry]

On Mon, May 19, 2025 at 8:32 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 05/19, Mina Almasry wrote:
> > ncdevmem supports both ipv4 and ipv6, but the ksft is currently
> > ipv6-only. Propagate the ipv4 support to the ksft, so that folks that
> > are limited to these networks can also test.
> >
> > Signed-off-by: Mina Almasry <almasrymina@google.com>
> >
> > ---
> >  .../selftests/drivers/net/hw/devmem.py        | 33 ++++++++++++-------
> >  1 file changed, 22 insertions(+), 11 deletions(-)
> >
> > diff --git a/tools/testing/selftests/drivers/net/hw/devmem.py b/tools/testing/selftests/drivers/net/hw/devmem.py
> > index f5d7809400ea..850381e14d9e 100755
> > --- a/tools/testing/selftests/drivers/net/hw/devmem.py
> > +++ b/tools/testing/selftests/drivers/net/hw/devmem.py
> > @@ -18,30 +18,36 @@ def require_devmem(cfg):
> >          raise KsftSkipEx("Test requires devmem support")
> >
> >
> > -def check_rx(cfg) -> None:
> > -    cfg.require_ipver("6")
> > +def check_rx(cfg, ipver) -> None:
> >      require_devmem(cfg)
> >
> > +    addr = cfg.addr_v[ipver]
> > +    if ipver == "6":
> > +        addr = "[" + addr + "]"
>
> I think you can add [] unconditionally, no need to special case v6.
>

I'll double check, but IIRC the [] were v6-only.

> > +
> > +    socat = f"socat -u - TCP{ipver}:{addr}:{port}"
> > +
> >      port = rand_port()
> >      listen_cmd = f"{cfg.bin_local} -l -f {cfg.ifname} -s {cfg.addr_v['6']} -p {port}"
> >
> > -    with bkg(listen_cmd) as socat:
> > +    with bkg(listen_cmd) as ncdevmem:
> >          wait_port_listen(port)
> > -        cmd(f"echo -e \"hello\\nworld\"| socat -u - TCP6:[{cfg.addr_v['6']}]:{port}", host=cfg.remote, shell=True)
> > +        cmd(f"echo -e \"hello\\nworld\"| {socat}", host=cfg.remote, shell=True)
> >
> > -    ksft_eq(socat.stdout.strip(), "hello\nworld")
> > +    ksft_eq(ncdevmem.stdout.strip(), "hello\nworld")
> >
> >
> > -def check_tx(cfg) -> None:
> > -    cfg.require_ipver("6")
> > +def check_tx(cfg, ipver) -> None:
> >      require_devmem(cfg)
> >
> >      port = rand_port()
> > -    listen_cmd = f"socat -U - TCP6-LISTEN:{port}"
> > +    listen_cmd = f"socat -U - TCP{ipver}-LISTEN:{port}"
> >
> > -    with bkg(listen_cmd, exit_wait=True) as socat:
> > +    addr = cfg.addr_v[ipver]
> > +
> > +    with bkg(listen_cmd) as socat:
> >          wait_port_listen(port)
> > -        cmd(f"echo -e \"hello\\nworld\"| {cfg.bin_remote} -f {cfg.ifname} -s {cfg.addr_v['6']} -p {port}", host=cfg.remote, shell=True)
> > +        cmd(f"echo -e \"hello\\nworld\"| {cfg.bin_remote} -f {cfg.ifname} -s {addr} -p {port}", host=cfg.remote, shell=True)
> >
> >      ksft_eq(socat.stdout.strip(), "hello\nworld")
> >
> > @@ -51,8 +57,13 @@ def main() -> None:
> >          cfg.bin_local = path.abspath(path.dirname(__file__) + "/ncdevmem")
> >          cfg.bin_remote = cfg.remote.deploy(cfg.bin_local)
> >
> > +        if "4" in cfg.addr_v:
> > +            ipver = "4"
> > +        else:
> > +            ipver = "6"
>
> If we have both, we prefer v4, can we do the opposite?

Sure, but why? Just curious.

-- 
Thanks,
Mina

Re: [PATCH net-next v1 7/9] net: devmem: ksft: add 5 tuple FS support

[Mina Almasry]

On Mon, May 19, 2025 at 8:37 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 05/19, Mina Almasry wrote:
> > ncdevmem supports drivers that are limited to either 3-tuple or 5-tuple
> > FS support, but the ksft is currently 3-tuple only. Support drivers that
> > have 5-tuple FS supported by adding a ksft arg.
> >
> > Signed-off-by: Mina Almasry <almasrymina@google.com>
> >
> > ---
> >  .../testing/selftests/drivers/net/hw/devmem.py  | 17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/drivers/net/hw/devmem.py b/tools/testing/selftests/drivers/net/hw/devmem.py
> > index 39b5241463aa..40fe5b525d51 100755
> > --- a/tools/testing/selftests/drivers/net/hw/devmem.py
> > +++ b/tools/testing/selftests/drivers/net/hw/devmem.py
> > @@ -21,14 +21,27 @@ def require_devmem(cfg):
> >  def check_rx(cfg, ipver) -> None:
> >      require_devmem(cfg)
> >
> > +    fs_5_tuple = False
> > +    if "FLOW_STEERING_5_TUPLE" in cfg.env:
> > +        fs_5_tuple = cfg.env["FLOW_STEERING_5_TUPLE"]
>
> I wonder if we can transparently handle it in ncdevmem: if -c is passed,
> try installing 3-tuple rule, and if it fails, try 5-tuple one. This
> should work without any user input / extra environment variable.
>

This seems like a good idea, yes, but I think install a 5-tuple one
first, and if that fails, try a 3-tuple one? 5-tuple rules are more
specific and should take precedence when the driver supports both. It
doesn't really matter but the 3-tuple one can match unintended flows.

-- 
Thanks,
Mina

Re: [PATCH net-next v1 4/9] net: devmem: ksft: remove ksft_disruptive

[Stanislav Fomichev]

On 05/19, Mina Almasry wrote:
> On Mon, May 19, 2025 at 8:25 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
> >
> > On 05/19, Mina Almasry wrote:
> > > As far as I can tell the ksft_disruptive here is unnecessary. These
> > > tests are largerly independent, and when one test fails, it's nice to
> > > know the results from all the other test cases.
> >
> > We currently don't do anything special for disruptive tests. I'm assuming
> > anything that changes nic configuration is disruptive and was thinking of
> > an option to run all disruptive tests at the end of the run. But so far we
> > haven't had any problem with mixing disruptive and non-disruptive tests,
> > so it's all moot. I'd prefer to keep everything as is for now (or remove
> > this whole disruptive category).
> 
> I've noticed that if all the tests are marked disruptive, and one test
> fails, the others don't run at all, which seems unnecessary. I'd like
> to see if the rx test passed if the tx one failed and vice versa for
> example. Removing the disruptive tag seems to resolve that.

I don't think that's the expected behavior. Disruptive should not
have any effect on other tests if any one fails. Any idea why it happens?

Re: [PATCH net-next v1 5/9] net: devmem: ksft: add ipv4 support

[Stanislav Fomichev]

On 05/19, Mina Almasry wrote:
> On Mon, May 19, 2025 at 8:32 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
> >
> > On 05/19, Mina Almasry wrote:
> > > ncdevmem supports both ipv4 and ipv6, but the ksft is currently
> > > ipv6-only. Propagate the ipv4 support to the ksft, so that folks that
> > > are limited to these networks can also test.
> > >
> > > Signed-off-by: Mina Almasry <almasrymina@google.com>
> > >
> > > ---
> > >  .../selftests/drivers/net/hw/devmem.py        | 33 ++++++++++++-------
> > >  1 file changed, 22 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/drivers/net/hw/devmem.py b/tools/testing/selftests/drivers/net/hw/devmem.py
> > > index f5d7809400ea..850381e14d9e 100755
> > > --- a/tools/testing/selftests/drivers/net/hw/devmem.py
> > > +++ b/tools/testing/selftests/drivers/net/hw/devmem.py
> > > @@ -18,30 +18,36 @@ def require_devmem(cfg):
> > >          raise KsftSkipEx("Test requires devmem support")
> > >
> > >
> > > -def check_rx(cfg) -> None:
> > > -    cfg.require_ipver("6")
> > > +def check_rx(cfg, ipver) -> None:
> > >      require_devmem(cfg)
> > >
> > > +    addr = cfg.addr_v[ipver]
> > > +    if ipver == "6":
> > > +        addr = "[" + addr + "]"
> >
> > I think you can add [] unconditionally, no need to special case v6.
> >
> 
> I'll double check, but IIRC the [] were v6-only.
> 
> > > +
> > > +    socat = f"socat -u - TCP{ipver}:{addr}:{port}"
> > > +
> > >      port = rand_port()
> > >      listen_cmd = f"{cfg.bin_local} -l -f {cfg.ifname} -s {cfg.addr_v['6']} -p {port}"
> > >
> > > -    with bkg(listen_cmd) as socat:
> > > +    with bkg(listen_cmd) as ncdevmem:
> > >          wait_port_listen(port)
> > > -        cmd(f"echo -e \"hello\\nworld\"| socat -u - TCP6:[{cfg.addr_v['6']}]:{port}", host=cfg.remote, shell=True)
> > > +        cmd(f"echo -e \"hello\\nworld\"| {socat}", host=cfg.remote, shell=True)
> > >
> > > -    ksft_eq(socat.stdout.strip(), "hello\nworld")
> > > +    ksft_eq(ncdevmem.stdout.strip(), "hello\nworld")
> > >
> > >
> > > -def check_tx(cfg) -> None:
> > > -    cfg.require_ipver("6")
> > > +def check_tx(cfg, ipver) -> None:
> > >      require_devmem(cfg)
> > >
> > >      port = rand_port()
> > > -    listen_cmd = f"socat -U - TCP6-LISTEN:{port}"
> > > +    listen_cmd = f"socat -U - TCP{ipver}-LISTEN:{port}"
> > >
> > > -    with bkg(listen_cmd, exit_wait=True) as socat:
> > > +    addr = cfg.addr_v[ipver]
> > > +
> > > +    with bkg(listen_cmd) as socat:
> > >          wait_port_listen(port)
> > > -        cmd(f"echo -e \"hello\\nworld\"| {cfg.bin_remote} -f {cfg.ifname} -s {cfg.addr_v['6']} -p {port}", host=cfg.remote, shell=True)
> > > +        cmd(f"echo -e \"hello\\nworld\"| {cfg.bin_remote} -f {cfg.ifname} -s {addr} -p {port}", host=cfg.remote, shell=True)
> > >
> > >      ksft_eq(socat.stdout.strip(), "hello\nworld")
> > >
> > > @@ -51,8 +57,13 @@ def main() -> None:
> > >          cfg.bin_local = path.abspath(path.dirname(__file__) + "/ncdevmem")
> > >          cfg.bin_remote = cfg.remote.deploy(cfg.bin_local)
> > >
> > > +        if "4" in cfg.addr_v:
> > > +            ipver = "4"
> > > +        else:
> > > +            ipver = "6"
> >
> > If we have both, we prefer v4, can we do the opposite?
> 
> Sure, but why? Just curious.

We want to be in the v6-only world at some point (unlikely to get there
though), and having dualstack deployments prefer v6 is the way to go.

Re: [PATCH net-next v1 7/9] net: devmem: ksft: add 5 tuple FS support

[Stanislav Fomichev]

On 05/19, Mina Almasry wrote:
> On Mon, May 19, 2025 at 8:37 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
> >
> > On 05/19, Mina Almasry wrote:
> > > ncdevmem supports drivers that are limited to either 3-tuple or 5-tuple
> > > FS support, but the ksft is currently 3-tuple only. Support drivers that
> > > have 5-tuple FS supported by adding a ksft arg.
> > >
> > > Signed-off-by: Mina Almasry <almasrymina@google.com>
> > >
> > > ---
> > >  .../testing/selftests/drivers/net/hw/devmem.py  | 17 +++++++++++++++--
> > >  1 file changed, 15 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/drivers/net/hw/devmem.py b/tools/testing/selftests/drivers/net/hw/devmem.py
> > > index 39b5241463aa..40fe5b525d51 100755
> > > --- a/tools/testing/selftests/drivers/net/hw/devmem.py
> > > +++ b/tools/testing/selftests/drivers/net/hw/devmem.py
> > > @@ -21,14 +21,27 @@ def require_devmem(cfg):
> > >  def check_rx(cfg, ipver) -> None:
> > >      require_devmem(cfg)
> > >
> > > +    fs_5_tuple = False
> > > +    if "FLOW_STEERING_5_TUPLE" in cfg.env:
> > > +        fs_5_tuple = cfg.env["FLOW_STEERING_5_TUPLE"]
> >
> > I wonder if we can transparently handle it in ncdevmem: if -c is passed,
> > try installing 3-tuple rule, and if it fails, try 5-tuple one. This
> > should work without any user input / extra environment variable.
> >
> 
> This seems like a good idea, yes, but I think install a 5-tuple one
> first, and if that fails, try a 3-tuple one? 5-tuple rules are more
> specific and should take precedence when the driver supports both. It
> doesn't really matter but the 3-tuple one can match unintended flows.

SG!

Re: [PATCH net-next v1 4/9] net: devmem: ksft: remove ksft_disruptive

[Jakub Kicinski]

On Mon, 19 May 2025 13:18:24 -0700 Stanislav Fomichev wrote:
> On 05/19, Mina Almasry wrote:
> > On Mon, May 19, 2025 at 8:25 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:  
> > >
> > > On 05/19, Mina Almasry wrote:  
> > > > As far as I can tell the ksft_disruptive here is unnecessary. These
> > > > tests are largerly independent, and when one test fails, it's nice to
> > > > know the results from all the other test cases.  
> > >
> > > We currently don't do anything special for disruptive tests. I'm assuming
> > > anything that changes nic configuration is disruptive and was thinking of
> > > an option to run all disruptive tests at the end of the run. But so far we
> > > haven't had any problem with mixing disruptive and non-disruptive tests,
> > > so it's all moot. I'd prefer to keep everything as is for now (or remove
> > > this whole disruptive category).  
> > 
> > I've noticed that if all the tests are marked disruptive, and one test
> > fails, the others don't run at all, which seems unnecessary. I'd like
> > to see if the rx test passed if the tx one failed and vice versa for
> > example. Removing the disruptive tag seems to resolve that.  
> 
> I don't think that's the expected behavior. Disruptive should not
> have any effect on other tests if any one fails. Any idea why it happens?

Right, this sounds odd and needs investigating.

FWIW, in my mind disruptive tests were supposed to be the tests which
can cut a single NIC machine off the network. IOW the use case is that
the NIC under test is the same NIC over which we're SSH'ing into the
machine to run the test. So we shouldn't do things like take the link
down or flush the IP addresses because it may kill our SSH connection.

Obviously every test may be somewhat disruptive, but most shouldn't
break ssh?

Re: [PATCH net-next v1 5/9] net: devmem: ksft: add ipv4 support

[Jakub Kicinski]

On Mon, 19 May 2025 02:35:13 +0000 Mina Almasry wrote:
> +    addr = cfg.addr_v[ipver]
> +    if ipver == "6":
> +        addr = "[" + addr + "]"

You want baddr ?

https://web.git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/tools/testing/selftests/drivers/net/lib/py/env.py#n155

In general you should use cfg.addr, cfg.addr_remote and self.addr_ipver 
if you don't care about what IP version env provides.

If you want to test specifically v4 or v6 they should be separate test
cases (doesn't sound like that's your intention here tho)