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
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);
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
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);
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; }
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 !! ?
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)
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).
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?
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?
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?
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()
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?
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.
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.
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...
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)
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)
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
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)
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?
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.
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!
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:
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
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;
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
linux-kselftest-mirror@lists.linaro.org
-
Jakub Kicinski -
Mina Almasry -
Stanislav Fomichev -
Taehee Yoo