[PATCH net-next v2 00/12] selftests: ncdevmem: Add ncdevmem to ksft

[PATCH net-next v2 00/12] selftests: ncdevmem: Add ncdevmem to ksft

[Stanislav Fomichev]

The goal of the series is to simplify and make it possible to use
ncdevmem in an automated way from the ksft python wrapper.

ncdevmem is slowly mutated into a state where it uses stdout
to print the payload and the python wrapper is added to
make sure the arrived payload matches the expected one.

v2:
- don't remove validation (Mina)
- keep 5-tuple flow steering but use it only when -c is provided (Mina)
- remove separate flag for probing (Mina)
- move ncdevmem under drivers/net/hw, not drivers/net (Jakub)

Cc: Mina Almasry <almasrymina@google.com>

Stanislav Fomichev (12):
  selftests: ncdevmem: Redirect all non-payload output to stderr
  selftests: ncdevmem: Separate out dmabuf provider
  selftests: ncdevmem: Unify error handling
  selftests: ncdevmem: Make client_ip optional
  selftests: ncdevmem: Remove default arguments
  selftests: ncdevmem: Switch to AF_INET6
  selftests: ncdevmem: Properly reset flow steering
  selftests: ncdevmem: Use YNL to enable TCP header split
  selftests: ncdevmem: Remove hard-coded queue numbers
  selftests: ncdevmem: Run selftest when none of the -s or -c has been
    provided
  selftests: ncdevmem: Move ncdevmem under drivers/net/hw
  selftests: ncdevmem: Add automated test

 .../selftests/drivers/net/hw/.gitignore       |   1 +
 .../testing/selftests/drivers/net/hw/Makefile |  10 +
 .../selftests/drivers/net/hw/devmem.py        |  46 ++
 .../selftests/drivers/net/hw/ncdevmem.c       | 734 ++++++++++++++++++
 tools/testing/selftests/net/.gitignore        |   1 -
 tools/testing/selftests/net/Makefile          |   9 -
 tools/testing/selftests/net/ncdevmem.c        | 570 --------------
 7 files changed, 791 insertions(+), 580 deletions(-)
 create mode 100644 tools/testing/selftests/drivers/net/hw/.gitignore
 create mode 100755 tools/testing/selftests/drivers/net/hw/devmem.py
 create mode 100644 tools/testing/selftests/drivers/net/hw/ncdevmem.c
 delete mode 100644 tools/testing/selftests/net/ncdevmem.c

-- 
2.46.0

[PATCH net-next v2 01/12] selftests: ncdevmem: Redirect all non-payload output to stderr

[Stanislav Fomichev]

That should make it possible to do expected payload validation on
the caller side.

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/ncdevmem.c | 61 +++++++++++++-------------
 1 file changed, 30 insertions(+), 31 deletions(-)

diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
index 64d6805381c5..9245d3f158dd 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -88,7 +88,6 @@ void print_nonzero_bytes(void *ptr, size_t size)
 
 	for (i = 0; i < size; i++)
 		putchar(p[i]);
-	printf("\n");
 }
 
 void validate_buffer(void *line, size_t size)
@@ -120,7 +119,7 @@ void validate_buffer(void *line, size_t size)
 		char command[256];                                      \
 		memset(command, 0, sizeof(command));                    \
 		snprintf(command, sizeof(command), cmd, ##__VA_ARGS__); \
-		printf("Running: %s\n", command);                       \
+		fprintf(stderr, "Running: %s\n", command);                       \
 		system(command);                                        \
 	})
 
@@ -128,22 +127,22 @@ static int reset_flow_steering(void)
 {
 	int ret = 0;
 
-	ret = run_command("sudo ethtool -K %s ntuple off", ifname);
+	ret = run_command("sudo ethtool -K %s ntuple off >&2", ifname);
 	if (ret)
 		return ret;
 
-	return run_command("sudo ethtool -K %s ntuple on", ifname);
+	return run_command("sudo ethtool -K %s ntuple on >&2", ifname);
 }
 
 static int configure_headersplit(bool on)
 {
-	return run_command("sudo ethtool -G %s tcp-data-split %s", ifname,
+	return run_command("sudo ethtool -G %s tcp-data-split %s >&2", ifname,
 			   on ? "on" : "off");
 }
 
 static int configure_rss(void)
 {
-	return run_command("sudo ethtool -X %s equal %d", ifname, start_queue);
+	return run_command("sudo ethtool -X %s equal %d >&2", ifname, start_queue);
 }
 
 static int configure_channels(unsigned int rx, unsigned int tx)
@@ -153,7 +152,7 @@ static int configure_channels(unsigned int rx, unsigned int tx)
 
 static int configure_flow_steering(void)
 {
-	return run_command("sudo ethtool -N %s flow-type tcp4 src-ip %s dst-ip %s src-port %s dst-port %s queue %d",
+	return run_command("sudo ethtool -N %s flow-type tcp4 src-ip %s dst-ip %s src-port %s dst-port %s queue %d >&2",
 			   ifname, client_ip, server_ip, port, port, start_queue);
 }
 
@@ -187,7 +186,7 @@ static int bind_rx_queue(unsigned int ifindex, unsigned int dmabuf_fd,
 		goto err_close;
 	}
 
-	printf("got dmabuf id=%d\n", rsp->id);
+	fprintf(stderr, "got dmabuf id=%d\n", rsp->id);
 	dmabuf_id = rsp->id;
 
 	netdev_bind_rx_req_free(req);
@@ -314,8 +313,8 @@ int do_server(void)
 	if (ret)
 		error(errno, errno, "%s: [FAIL, set sock opt]\n", TEST_PREFIX);
 
-	printf("binding to address %s:%d\n", server_ip,
-	       ntohs(server_sin.sin_port));
+	fprintf(stderr, "binding to address %s:%d\n", server_ip,
+		ntohs(server_sin.sin_port));
 
 	ret = bind(socket_fd, &server_sin, sizeof(server_sin));
 	if (ret)
@@ -329,14 +328,14 @@ int do_server(void)
 
 	inet_ntop(server_sin.sin_family, &server_sin.sin_addr, buffer,
 		  sizeof(buffer));
-	printf("Waiting or connection on %s:%d\n", buffer,
-	       ntohs(server_sin.sin_port));
+	fprintf(stderr, "Waiting or connection on %s:%d\n", buffer,
+		ntohs(server_sin.sin_port));
 	client_fd = accept(socket_fd, &client_addr, &client_addr_len);
 
 	inet_ntop(client_addr.sin_family, &client_addr.sin_addr, buffer,
 		  sizeof(buffer));
-	printf("Got connection from %s:%d\n", buffer,
-	       ntohs(client_addr.sin_port));
+	fprintf(stderr, "Got connection from %s:%d\n", buffer,
+		ntohs(client_addr.sin_port));
 
 	while (1) {
 		struct iovec iov = { .iov_base = iobuf,
@@ -349,14 +348,13 @@ int do_server(void)
 		ssize_t ret;
 
 		is_devmem = false;
-		printf("\n\n");
 
 		msg.msg_iov = &iov;
 		msg.msg_iovlen = 1;
 		msg.msg_control = ctrl_data;
 		msg.msg_controllen = sizeof(ctrl_data);
 		ret = recvmsg(client_fd, &msg, MSG_SOCK_DEVMEM);
-		printf("recvmsg ret=%ld\n", ret);
+		fprintf(stderr, "recvmsg ret=%ld\n", ret);
 		if (ret < 0 && (errno == EAGAIN || errno == EWOULDBLOCK))
 			continue;
 		if (ret < 0) {
@@ -364,7 +362,7 @@ int do_server(void)
 			continue;
 		}
 		if (ret == 0) {
-			printf("client exited\n");
+			fprintf(stderr, "client exited\n");
 			goto cleanup;
 		}
 
@@ -373,7 +371,7 @@ int do_server(void)
 			if (cm->cmsg_level != SOL_SOCKET ||
 			    (cm->cmsg_type != SCM_DEVMEM_DMABUF &&
 			     cm->cmsg_type != SCM_DEVMEM_LINEAR)) {
-				fprintf(stdout, "skipping non-devmem cmsg\n");
+				fprintf(stderr, "skipping non-devmem cmsg\n");
 				continue;
 			}
 
@@ -384,7 +382,7 @@ int do_server(void)
 				/* TODO: process data copied from skb's linear
 				 * buffer.
 				 */
-				fprintf(stdout,
+				fprintf(stderr,
 					"SCM_DEVMEM_LINEAR. dmabuf_cmsg->frag_size=%u\n",
 					dmabuf_cmsg->frag_size);
 
@@ -395,12 +393,13 @@ int do_server(void)
 			token.token_count = 1;
 
 			total_received += dmabuf_cmsg->frag_size;
-			printf("received frag_page=%llu, in_page_offset=%llu, frag_offset=%llu, frag_size=%u, token=%u, total_received=%lu, dmabuf_id=%u\n",
-			       dmabuf_cmsg->frag_offset >> PAGE_SHIFT,
-			       dmabuf_cmsg->frag_offset % getpagesize(),
-			       dmabuf_cmsg->frag_offset, dmabuf_cmsg->frag_size,
-			       dmabuf_cmsg->frag_token, total_received,
-			       dmabuf_cmsg->dmabuf_id);
+			fprintf(stderr,
+				"received frag_page=%llu, in_page_offset=%llu, frag_offset=%llu, frag_size=%u, token=%u, total_received=%lu, dmabuf_id=%u\n",
+				dmabuf_cmsg->frag_offset >> PAGE_SHIFT,
+				dmabuf_cmsg->frag_offset % getpagesize(),
+				dmabuf_cmsg->frag_offset,
+				dmabuf_cmsg->frag_size, dmabuf_cmsg->frag_token,
+				total_received, dmabuf_cmsg->dmabuf_id);
 
 			if (dmabuf_cmsg->dmabuf_id != dmabuf_id)
 				error(1, 0,
@@ -438,15 +437,15 @@ int do_server(void)
 		if (!is_devmem)
 			error(1, 0, "flow steering error\n");
 
-		printf("total_received=%lu\n", total_received);
+		fprintf(stderr, "total_received=%lu\n", total_received);
 	}
 
-	fprintf(stdout, "%s: ok\n", TEST_PREFIX);
+	fprintf(stderr, "%s: ok\n", TEST_PREFIX);
 
-	fprintf(stdout, "page_aligned_frags=%lu, non_page_aligned_frags=%lu\n",
+	fprintf(stderr, "page_aligned_frags=%lu, non_page_aligned_frags=%lu\n",
 		page_aligned_frags, non_page_aligned_frags);
 
-	fprintf(stdout, "page_aligned_frags=%lu, non_page_aligned_frags=%lu\n",
+	fprintf(stderr, "page_aligned_frags=%lu, non_page_aligned_frags=%lu\n",
 		page_aligned_frags, non_page_aligned_frags);
 
 cleanup:
@@ -551,7 +550,7 @@ int main(int argc, char *argv[])
 			ifname = optarg;
 			break;
 		case '?':
-			printf("unknown option: %c\n", optopt);
+			fprintf(stderr, "unknown option: %c\n", optopt);
 			break;
 		}
 	}
@@ -559,7 +558,7 @@ int main(int argc, char *argv[])
 	ifindex = if_nametoindex(ifname);
 
 	for (; optind < argc; optind++)
-		printf("extra arguments: %s\n", argv[optind]);
+		fprintf(stderr, "extra arguments: %s\n", argv[optind]);
 
 	run_devmem_tests();
 
-- 
2.46.0

[PATCH net-next v2 02/12] selftests: ncdevmem: Separate out dmabuf provider

[Stanislav Fomichev]

So we can plug the other ones in the future if needed.

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/ncdevmem.c | 215 +++++++++++++++----------
 1 file changed, 134 insertions(+), 81 deletions(-)

diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
index 9245d3f158dd..557175c3bf02 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -71,17 +71,118 @@ static char *ifname = "eth1";
 static unsigned int ifindex;
 static unsigned int dmabuf_id;
 
-void print_bytes(void *ptr, size_t size)
+struct memory_buffer {
+	int fd;
+	size_t size;
+
+	int devfd;
+	int memfd;
+	char *buf_mem;
+};
+
+struct memory_provider {
+	struct memory_buffer *(*alloc)(size_t size);
+	void (*free)(struct memory_buffer *ctx);
+	void (*memcpy_to_device)(struct memory_buffer *dst, size_t off,
+				 void *src, int n);
+	void (*memcpy_from_device)(void *dst, struct memory_buffer *src,
+				   size_t off, int n);
+};
+
+static struct memory_buffer *udmabuf_alloc(size_t size)
 {
-	unsigned char *p = ptr;
-	int i;
+	struct udmabuf_create create;
+	struct memory_buffer *ctx;
+	int ret;
 
-	for (i = 0; i < size; i++)
-		printf("%02hhX ", p[i]);
-	printf("\n");
+	ctx = malloc(sizeof(*ctx));
+	if (!ctx)
+		error(1, ENOMEM, "malloc failed");
+
+	ctx->size = size;
+
+	ctx->devfd = open("/dev/udmabuf", O_RDWR);
+	if (ctx->devfd < 0)
+		error(1, errno,
+		      "%s: [skip,no-udmabuf: Unable to access DMA buffer device file]\n",
+		      TEST_PREFIX);
+
+	ctx->memfd = memfd_create("udmabuf-test", MFD_ALLOW_SEALING);
+	if (ctx->memfd < 0)
+		error(1, errno, "%s: [skip,no-memfd]\n", TEST_PREFIX);
+
+	ret = fcntl(ctx->memfd, F_ADD_SEALS, F_SEAL_SHRINK);
+	if (ret < 0)
+		error(1, errno, "%s: [skip,fcntl-add-seals]\n", TEST_PREFIX);
+
+	ret = ftruncate(ctx->memfd, size);
+	if (ret == -1)
+		error(1, errno, "%s: [FAIL,memfd-truncate]\n", TEST_PREFIX);
+
+	memset(&create, 0, sizeof(create));
+
+	create.memfd = ctx->memfd;
+	create.offset = 0;
+	create.size = size;
+	ctx->fd = ioctl(ctx->devfd, UDMABUF_CREATE, &create);
+	if (ctx->fd < 0)
+		error(1, errno, "%s: [FAIL, create udmabuf]\n", TEST_PREFIX);
+
+	ctx->buf_mem = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED,
+			    ctx->fd, 0);
+	if (ctx->buf_mem == MAP_FAILED)
+		error(1, errno, "%s: [FAIL, map udmabuf]\n", TEST_PREFIX);
+
+	return ctx;
+}
+
+static void udmabuf_free(struct memory_buffer *ctx)
+{
+	munmap(ctx->buf_mem, ctx->size);
+	close(ctx->fd);
+	close(ctx->memfd);
+	close(ctx->devfd);
+	free(ctx);
+}
+
+static void udmabuf_memcpy_to_device(struct memory_buffer *dst, size_t off,
+				     void *src, int n)
+{
+	struct dma_buf_sync sync = {};
+
+	sync.flags = DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE;
+	ioctl(dst->fd, DMA_BUF_IOCTL_SYNC, &sync);
+
+	memcpy(dst->buf_mem + off, src, n);
+
+	sync.flags = DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE;
+	ioctl(dst->fd, DMA_BUF_IOCTL_SYNC, &sync);
+}
+
+static void udmabuf_memcpy_from_device(void *dst, struct memory_buffer *src,
+				       size_t off, int n)
+{
+	struct dma_buf_sync sync = {};
+
+	sync.flags = DMA_BUF_SYNC_START;
+	ioctl(src->fd, DMA_BUF_IOCTL_SYNC, &sync);
+
+	memcpy(dst, src->buf_mem + off, n);
+
+	sync.flags = DMA_BUF_SYNC_END;
+	ioctl(src->fd, DMA_BUF_IOCTL_SYNC, &sync);
 }
 
-void print_nonzero_bytes(void *ptr, size_t size)
+static struct memory_provider udmabuf_memory_provider = {
+	.alloc = udmabuf_alloc,
+	.free = udmabuf_free,
+	.memcpy_to_device = udmabuf_memcpy_to_device,
+	.memcpy_from_device = udmabuf_memcpy_from_device,
+};
+
+static struct memory_provider *provider = &udmabuf_memory_provider;
+
+static void print_nonzero_bytes(void *ptr, size_t size)
 {
 	unsigned char *p = ptr;
 	unsigned int i;
@@ -201,42 +302,7 @@ static int bind_rx_queue(unsigned int ifindex, unsigned int dmabuf_fd,
 	return -1;
 }
 
-static void create_udmabuf(int *devfd, int *memfd, int *buf, size_t dmabuf_size)
-{
-	struct udmabuf_create create;
-	int ret;
-
-	*devfd = open("/dev/udmabuf", O_RDWR);
-	if (*devfd < 0) {
-		error(70, 0,
-		      "%s: [skip,no-udmabuf: Unable to access DMA buffer device file]\n",
-		      TEST_PREFIX);
-	}
-
-	*memfd = memfd_create("udmabuf-test", MFD_ALLOW_SEALING);
-	if (*memfd < 0)
-		error(70, 0, "%s: [skip,no-memfd]\n", TEST_PREFIX);
-
-	/* Required for udmabuf */
-	ret = fcntl(*memfd, F_ADD_SEALS, F_SEAL_SHRINK);
-	if (ret < 0)
-		error(73, 0, "%s: [skip,fcntl-add-seals]\n", TEST_PREFIX);
-
-	ret = ftruncate(*memfd, dmabuf_size);
-	if (ret == -1)
-		error(74, 0, "%s: [FAIL,memfd-truncate]\n", TEST_PREFIX);
-
-	memset(&create, 0, sizeof(create));
-
-	create.memfd = *memfd;
-	create.offset = 0;
-	create.size = dmabuf_size;
-	*buf = ioctl(*devfd, UDMABUF_CREATE, &create);
-	if (*buf < 0)
-		error(75, 0, "%s: [FAIL, create udmabuf]\n", TEST_PREFIX);
-}
-
-int do_server(void)
+int do_server(struct memory_buffer *mem)
 {
 	char ctrl_data[sizeof(int) * 20000];
 	struct netdev_queue_id *queues;
@@ -244,23 +310,18 @@ int do_server(void)
 	struct sockaddr_in client_addr;
 	struct sockaddr_in server_sin;
 	size_t page_aligned_frags = 0;
-	int devfd, memfd, buf, ret;
 	size_t total_received = 0;
 	socklen_t client_addr_len;
 	bool is_devmem = false;
-	char *buf_mem = NULL;
+	char *tmp_mem = NULL;
 	struct ynl_sock *ys;
-	size_t dmabuf_size;
 	char iobuf[819200];
 	char buffer[256];
 	int socket_fd;
 	int client_fd;
 	size_t i = 0;
 	int opt = 1;
-
-	dmabuf_size = getpagesize() * NUM_PAGES;
-
-	create_udmabuf(&devfd, &memfd, &buf, dmabuf_size);
+	int ret;
 
 	if (reset_flow_steering())
 		error(1, 0, "Failed to reset flow steering\n");
@@ -284,13 +345,12 @@ int do_server(void)
 		queues[i].id = start_queue + i;
 	}
 
-	if (bind_rx_queue(ifindex, buf, queues, num_queues, &ys))
+	if (bind_rx_queue(ifindex, mem->fd, queues, num_queues, &ys))
 		error(1, 0, "Failed to bind\n");
 
-	buf_mem = mmap(NULL, dmabuf_size, PROT_READ | PROT_WRITE, MAP_SHARED,
-		       buf, 0);
-	if (buf_mem == MAP_FAILED)
-		error(1, 0, "mmap()");
+	tmp_mem = malloc(mem->size);
+	if (!tmp_mem)
+		error(1, ENOMEM, "malloc failed");
 
 	server_sin.sin_family = AF_INET;
 	server_sin.sin_port = htons(atoi(port));
@@ -341,7 +401,6 @@ int do_server(void)
 		struct iovec iov = { .iov_base = iobuf,
 				     .iov_len = sizeof(iobuf) };
 		struct dmabuf_cmsg *dmabuf_cmsg = NULL;
-		struct dma_buf_sync sync = { 0 };
 		struct cmsghdr *cm = NULL;
 		struct msghdr msg = { 0 };
 		struct dmabuf_token token;
@@ -410,22 +469,17 @@ int do_server(void)
 			else
 				page_aligned_frags++;
 
-			sync.flags = DMA_BUF_SYNC_READ | DMA_BUF_SYNC_START;
-			ioctl(buf, DMA_BUF_IOCTL_SYNC, &sync);
+			provider->memcpy_from_device(tmp_mem, mem,
+						     dmabuf_cmsg->frag_offset,
+						     dmabuf_cmsg->frag_size);
 
 			if (do_validation)
 				validate_buffer(
-					((unsigned char *)buf_mem) +
+					((unsigned char *)tmp_mem) +
 						dmabuf_cmsg->frag_offset,
 					dmabuf_cmsg->frag_size);
 			else
-				print_nonzero_bytes(
-					((unsigned char *)buf_mem) +
-						dmabuf_cmsg->frag_offset,
-					dmabuf_cmsg->frag_size);
-
-			sync.flags = DMA_BUF_SYNC_READ | DMA_BUF_SYNC_END;
-			ioctl(buf, DMA_BUF_IOCTL_SYNC, &sync);
+				print_nonzero_bytes(tmp_mem, dmabuf_cmsg->frag_size);
 
 			ret = setsockopt(client_fd, SOL_SOCKET,
 					 SO_DEVMEM_DONTNEED, &token,
@@ -450,12 +504,9 @@ int do_server(void)
 
 cleanup:
 
-	munmap(buf_mem, dmabuf_size);
+	free(tmp_mem);
 	close(client_fd);
 	close(socket_fd);
-	close(buf);
-	close(memfd);
-	close(devfd);
 	ynl_sock_destroy(ys);
 
 	return 0;
@@ -464,14 +515,11 @@ int do_server(void)
 void run_devmem_tests(void)
 {
 	struct netdev_queue_id *queues;
-	int devfd, memfd, buf;
+	struct memory_buffer *mem;
 	struct ynl_sock *ys;
-	size_t dmabuf_size;
 	size_t i = 0;
 
-	dmabuf_size = getpagesize() * NUM_PAGES;
-
-	create_udmabuf(&devfd, &memfd, &buf, dmabuf_size);
+	mem = provider->alloc(getpagesize() * NUM_PAGES);
 
 	/* Configure RSS to divert all traffic from our devmem queues */
 	if (configure_rss())
@@ -482,7 +530,7 @@ void run_devmem_tests(void)
 	if (configure_headersplit(1))
 		error(1, 0, "Failed to configure header split\n");
 
-	if (!bind_rx_queue(ifindex, buf, queues, num_queues, &ys))
+	if (!bind_rx_queue(ifindex, mem->fd, queues, num_queues, &ys))
 		error(1, 0, "Binding empty queues array should have failed\n");
 
 	for (i = 0; i < num_queues; i++) {
@@ -495,7 +543,7 @@ void run_devmem_tests(void)
 	if (configure_headersplit(0))
 		error(1, 0, "Failed to configure header split\n");
 
-	if (!bind_rx_queue(ifindex, buf, queues, num_queues, &ys))
+	if (!bind_rx_queue(ifindex, mem->fd, queues, num_queues, &ys))
 		error(1, 0, "Configure dmabuf with header split off should have failed\n");
 
 	if (configure_headersplit(1))
@@ -508,7 +556,7 @@ void run_devmem_tests(void)
 		queues[i].id = start_queue + i;
 	}
 
-	if (bind_rx_queue(ifindex, buf, queues, num_queues, &ys))
+	if (bind_rx_queue(ifindex, mem->fd, queues, num_queues, &ys))
 		error(1, 0, "Failed to bind\n");
 
 	/* Deactivating a bound queue should not be legal */
@@ -517,11 +565,15 @@ void run_devmem_tests(void)
 
 	/* Closing the netlink socket does an implicit unbind */
 	ynl_sock_destroy(ys);
+
+	provider->free(mem);
 }
 
 int main(int argc, char *argv[])
 {
+	struct memory_buffer *mem;
 	int is_server = 0, opt;
+	int ret;
 
 	while ((opt = getopt(argc, argv, "ls:c:p:v:q:t:f:")) != -1) {
 		switch (opt) {
@@ -562,8 +614,9 @@ int main(int argc, char *argv[])
 
 	run_devmem_tests();
 
-	if (is_server)
-		return do_server();
+	mem = provider->alloc(getpagesize() * NUM_PAGES);
+	ret = is_server ? do_server(mem) : 1;
+	provider->free(mem);
 
-	return 0;
+	return ret;
 }
-- 
2.46.0

[PATCH net-next v2 03/12] selftests: ncdevmem: Unify error handling

[Stanislav Fomichev]

There is a bunch of places where error() calls look out of place.
Use the same error(1, errno, ...) pattern everywhere.

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/ncdevmem.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
index 557175c3bf02..7b56b13708d4 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -357,32 +357,32 @@ int do_server(struct memory_buffer *mem)
 
 	ret = inet_pton(server_sin.sin_family, server_ip, &server_sin.sin_addr);
 	if (socket < 0)
-		error(79, 0, "%s: [FAIL, create socket]\n", TEST_PREFIX);
+		error(1, 0, "%s: [FAIL, create socket]\n", TEST_PREFIX);
 
 	socket_fd = socket(server_sin.sin_family, SOCK_STREAM, 0);
 	if (socket < 0)
-		error(errno, errno, "%s: [FAIL, create socket]\n", TEST_PREFIX);
+		error(1, errno, "%s: [FAIL, create socket]\n", TEST_PREFIX);
 
 	ret = setsockopt(socket_fd, SOL_SOCKET, SO_REUSEPORT, &opt,
 			 sizeof(opt));
 	if (ret)
-		error(errno, errno, "%s: [FAIL, set sock opt]\n", TEST_PREFIX);
+		error(1, errno, "%s: [FAIL, set sock opt]\n", TEST_PREFIX);
 
 	ret = setsockopt(socket_fd, SOL_SOCKET, SO_REUSEADDR, &opt,
 			 sizeof(opt));
 	if (ret)
-		error(errno, errno, "%s: [FAIL, set sock opt]\n", TEST_PREFIX);
+		error(1, errno, "%s: [FAIL, set sock opt]\n", TEST_PREFIX);
 
 	fprintf(stderr, "binding to address %s:%d\n", server_ip,
 		ntohs(server_sin.sin_port));
 
 	ret = bind(socket_fd, &server_sin, sizeof(server_sin));
 	if (ret)
-		error(errno, errno, "%s: [FAIL, bind]\n", TEST_PREFIX);
+		error(1, errno, "%s: [FAIL, bind]\n", TEST_PREFIX);
 
 	ret = listen(socket_fd, 1);
 	if (ret)
-		error(errno, errno, "%s: [FAIL, listen]\n", TEST_PREFIX);
+		error(1, errno, "%s: [FAIL, listen]\n", TEST_PREFIX);
 
 	client_addr_len = sizeof(client_addr);
 
-- 
2.46.0

[PATCH net-next v2 04/12] selftests: ncdevmem: Make client_ip optional

[Stanislav Fomichev]

Support 3-tuple filtering by making client_ip optional. When -c is
not passed, don't specify src-ip/src-port in the filter.

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/ncdevmem.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
index 7b56b13708d4..699692fdfd7d 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -62,7 +62,7 @@
  */
 
 static char *server_ip = "192.168.1.4";
-static char *client_ip = "192.168.1.2";
+static char *client_ip;
 static char *port = "5201";
 static size_t do_validation;
 static int start_queue = 8;
@@ -253,8 +253,14 @@ static int configure_channels(unsigned int rx, unsigned int tx)
 
 static int configure_flow_steering(void)
 {
-	return run_command("sudo ethtool -N %s flow-type tcp4 src-ip %s dst-ip %s src-port %s dst-port %s queue %d >&2",
-			   ifname, client_ip, server_ip, port, port, start_queue);
+	return run_command("sudo ethtool -N %s flow-type tcp4 %s %s dst-ip %s %s %s dst-port %s queue %d >&2",
+			   ifname,
+			   client_ip ? "src-ip" : "",
+			   client_ip ?: "",
+			   server_ip,
+			   client_ip ? "src-port" : "",
+			   client_ip ? port : "",
+			   port, start_queue);
 }
 
 static int bind_rx_queue(unsigned int ifindex, unsigned int dmabuf_fd,
-- 
2.46.0

[PATCH net-next v2 05/12] selftests: ncdevmem: Remove default arguments

[Stanislav Fomichev]

To make it clear what's required and what's not. Also, some of the
values don't seem like a good defaults; for example eth1.

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/ncdevmem.c | 34 +++++++++-----------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
index 699692fdfd7d..bf446d74a4f0 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -42,32 +42,13 @@
 #define MSG_SOCK_DEVMEM 0x2000000
 #endif
 
-/*
- * tcpdevmem netcat. Works similarly to netcat but does device memory TCP
- * instead of regular TCP. Uses udmabuf to mock a dmabuf provider.
- *
- * Usage:
- *
- *	On server:
- *	ncdevmem -s <server IP> -c <client IP> -f eth1 -l -p 5201 -v 7
- *
- *	On client:
- *	yes $(echo -e \\x01\\x02\\x03\\x04\\x05\\x06) | \
- *		tr \\n \\0 | \
- *		head -c 5G | \
- *		nc <server IP> 5201 -p 5201
- *
- * Note this is compatible with regular netcat. i.e. the sender or receiver can
- * be replaced with regular netcat to test the RX or TX path in isolation.
- */
-
-static char *server_ip = "192.168.1.4";
+static char *server_ip;
 static char *client_ip;
-static char *port = "5201";
+static char *port;
 static size_t do_validation;
 static int start_queue = 8;
 static int num_queues = 8;
-static char *ifname = "eth1";
+static char *ifname;
 static unsigned int ifindex;
 static unsigned int dmabuf_id;
 
@@ -613,6 +594,15 @@ int main(int argc, char *argv[])
 		}
 	}
 
+	if (!server_ip)
+		error(1, 0, "Missing -s argument\n");
+
+	if (!port)
+		error(1, 0, "Missing -p argument\n");
+
+	if (!ifname)
+		error(1, 0, "Missing -f argument\n");
+
 	ifindex = if_nametoindex(ifname);
 
 	for (; optind < argc; optind++)
-- 
2.46.0

[PATCH net-next v2 06/12] selftests: ncdevmem: Switch to AF_INET6

[Stanislav Fomichev]

Use dualstack socket to support both v4 and v6. v4-mapped-v6 address
can be used to do v4.

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/ncdevmem.c | 85 +++++++++++++++++---------
 1 file changed, 57 insertions(+), 28 deletions(-)

diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
index bf446d74a4f0..47458a13eff5 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -232,13 +232,22 @@ static int configure_channels(unsigned int rx, unsigned int tx)
 	return run_command("sudo ethtool -L %s rx %u tx %u", ifname, rx, tx);
 }
 
-static int configure_flow_steering(void)
+static int configure_flow_steering(struct sockaddr_in6 *server_sin)
 {
-	return run_command("sudo ethtool -N %s flow-type tcp4 %s %s dst-ip %s %s %s dst-port %s queue %d >&2",
+	const char *server_addr = server_ip;
+	const char *type = "tcp6";
+
+	if (IN6_IS_ADDR_V4MAPPED(&server_sin->sin6_addr)) {
+		type = "tcp4";
+		server_addr = strrchr(server_ip, ':') + 1;
+	}
+
+	return run_command("sudo ethtool -N %s flow-type %s %s %s dst-ip %s %s %s dst-port %s queue %d >&2",
 			   ifname,
+			   type,
 			   client_ip ? "src-ip" : "",
 			   client_ip ?: "",
-			   server_ip,
+			   server_addr,
 			   client_ip ? "src-port" : "",
 			   client_ip ? port : "",
 			   port, start_queue);
@@ -289,13 +298,43 @@ static int bind_rx_queue(unsigned int ifindex, unsigned int dmabuf_fd,
 	return -1;
 }
 
+static int enable_reuseaddr(int fd)
+{
+	int opt = 1;
+	int ret;
+
+	ret = setsockopt(fd, SOL_SOCKET, SO_REUSEPORT, &opt, sizeof(opt));
+	if (ret)
+		return -errno;
+
+	ret = setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt));
+	if (ret)
+		return -errno;
+
+	return 0;
+}
+
+static int parse_address(const char *str, int port, struct sockaddr_in6 *sin6)
+{
+	int ret;
+
+	sin6->sin6_family = AF_INET6;
+	sin6->sin6_port = htons(port);
+
+	ret = inet_pton(sin6->sin6_family, str, &sin6->sin6_addr);
+	if (ret < 0)
+		return -1;
+
+	return 0;
+}
+
 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_in client_addr;
-	struct sockaddr_in server_sin;
+	struct sockaddr_in6 client_addr;
+	struct sockaddr_in6 server_sin;
 	size_t page_aligned_frags = 0;
 	size_t total_received = 0;
 	socklen_t client_addr_len;
@@ -307,9 +346,12 @@ int do_server(struct memory_buffer *mem)
 	int socket_fd;
 	int client_fd;
 	size_t i = 0;
-	int opt = 1;
 	int ret;
 
+	ret = parse_address(server_ip, atoi(port), &server_sin);
+	if (ret < 0)
+		error(1, 0, "parse server address");
+
 	if (reset_flow_steering())
 		error(1, 0, "Failed to reset flow steering\n");
 
@@ -318,7 +360,7 @@ int do_server(struct memory_buffer *mem)
 		error(1, 0, "Failed to configure rss\n");
 
 	/* Flow steer our devmem flows to start_queue */
-	if (configure_flow_steering())
+	if (configure_flow_steering(&server_sin))
 		error(1, 0, "Failed to configure flow steering\n");
 
 	sleep(1);
@@ -339,29 +381,16 @@ int do_server(struct memory_buffer *mem)
 	if (!tmp_mem)
 		error(1, ENOMEM, "malloc failed");
 
-	server_sin.sin_family = AF_INET;
-	server_sin.sin_port = htons(atoi(port));
-
-	ret = inet_pton(server_sin.sin_family, server_ip, &server_sin.sin_addr);
-	if (socket < 0)
-		error(1, 0, "%s: [FAIL, create socket]\n", TEST_PREFIX);
-
-	socket_fd = socket(server_sin.sin_family, SOCK_STREAM, 0);
+	socket_fd = socket(AF_INET6, SOCK_STREAM, 0);
 	if (socket < 0)
 		error(1, errno, "%s: [FAIL, create socket]\n", TEST_PREFIX);
 
-	ret = setsockopt(socket_fd, SOL_SOCKET, SO_REUSEPORT, &opt,
-			 sizeof(opt));
-	if (ret)
-		error(1, errno, "%s: [FAIL, set sock opt]\n", TEST_PREFIX);
-
-	ret = setsockopt(socket_fd, SOL_SOCKET, SO_REUSEADDR, &opt,
-			 sizeof(opt));
+	ret = enable_reuseaddr(socket_fd);
 	if (ret)
-		error(1, errno, "%s: [FAIL, set sock opt]\n", TEST_PREFIX);
+		error(1, errno, "%s: [FAIL, reuseaddr]\n", TEST_PREFIX);
 
 	fprintf(stderr, "binding to address %s:%d\n", server_ip,
-		ntohs(server_sin.sin_port));
+		ntohs(server_sin.sin6_port));
 
 	ret = bind(socket_fd, &server_sin, sizeof(server_sin));
 	if (ret)
@@ -373,16 +402,16 @@ int do_server(struct memory_buffer *mem)
 
 	client_addr_len = sizeof(client_addr);
 
-	inet_ntop(server_sin.sin_family, &server_sin.sin_addr, buffer,
+	inet_ntop(AF_INET6, &server_sin.sin6_addr, buffer,
 		  sizeof(buffer));
 	fprintf(stderr, "Waiting or connection on %s:%d\n", buffer,
-		ntohs(server_sin.sin_port));
+		ntohs(server_sin.sin6_port));
 	client_fd = accept(socket_fd, &client_addr, &client_addr_len);
 
-	inet_ntop(client_addr.sin_family, &client_addr.sin_addr, buffer,
+	inet_ntop(AF_INET6, &client_addr.sin6_addr, buffer,
 		  sizeof(buffer));
 	fprintf(stderr, "Got connection from %s:%d\n", buffer,
-		ntohs(client_addr.sin_port));
+		ntohs(client_addr.sin6_port));
 
 	while (1) {
 		struct iovec iov = { .iov_base = iobuf,
-- 
2.46.0

[PATCH net-next v2 07/12] selftests: ncdevmem: Properly reset flow steering

[Stanislav Fomichev]

ntuple off/on might be not enough to do it on all NICs.
Add a bunch of shell crap to explicitly remove the rules.

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/ncdevmem.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
index 47458a13eff5..48cbf057fde7 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -207,13 +207,12 @@ void validate_buffer(void *line, size_t size)
 
 static int reset_flow_steering(void)
 {
-	int ret = 0;
-
-	ret = run_command("sudo ethtool -K %s ntuple off >&2", ifname);
-	if (ret)
-		return ret;
-
-	return run_command("sudo ethtool -K %s ntuple on >&2", ifname);
+	run_command("sudo ethtool -K %s ntuple off >&2", ifname);
+	run_command("sudo ethtool -K %s ntuple on >&2", ifname);
+	run_command(
+		"sudo ethtool -n %s | grep 'Filter:' | awk '{print $2}' | xargs -n1 ethtool -N %s delete >&2",
+		ifname, ifname);
+	return 0;
 }
 
 static int configure_headersplit(bool on)
-- 
2.46.0

[PATCH net-next v2 08/12] selftests: ncdevmem: Use YNL to enable TCP header split

[Stanislav Fomichev]

In the next patch the hard-coded queue numbers are gonna be removed.
So introduce some initial support for ethtool YNL and use
it to enable header split.

Also, tcp-data-split requires latest ethtool which is unlikely
to be present in the distros right now.

(ideally, we should not shell out to ethtool at all).

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/Makefile   |  2 +-
 tools/testing/selftests/net/ncdevmem.c | 43 ++++++++++++++++++++++++--
 2 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 649f1fe0dc46..9c970e96ed33 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -112,7 +112,7 @@ TEST_INCLUDES := forwarding/lib.sh
 include ../lib.mk
 
 # YNL build
-YNL_GENS := netdev
+YNL_GENS := ethtool netdev
 include ynl.mk
 
 $(OUTPUT)/epoll_busy_poll: LDLIBS += -lcap
diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
index 48cbf057fde7..a1fa818c8229 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -28,10 +28,12 @@
 #include <linux/netlink.h>
 #include <linux/genetlink.h>
 #include <linux/netdev.h>
+#include <linux/ethtool_netlink.h>
 #include <time.h>
 #include <net/if.h>
 
 #include "netdev-user.h"
+#include "ethtool-user.h"
 #include <ynl.h>
 
 #define PAGE_SHIFT 12
@@ -217,8 +219,42 @@ static int reset_flow_steering(void)
 
 static int configure_headersplit(bool on)
 {
-	return run_command("sudo ethtool -G %s tcp-data-split %s >&2", ifname,
-			   on ? "on" : "off");
+	struct ethtool_rings_set_req *req;
+	struct ynl_error yerr;
+	struct ynl_sock *ys;
+	int ret;
+
+	ys = ynl_sock_create(&ynl_ethtool_family, &yerr);
+	if (!ys) {
+		fprintf(stderr, "YNL: %s\n", yerr.msg);
+		return -1;
+	}
+
+	req = ethtool_rings_set_req_alloc();
+	ethtool_rings_set_req_set_header_dev_index(req, ifindex);
+	ethtool_rings_set_req_set_tcp_data_split(req, on ? 2 : 0);
+	ret = ethtool_rings_set(ys, req);
+	if (ret < 0)
+		fprintf(stderr, "YNL failed: %s\n", ys->err.msg);
+	ethtool_rings_set_req_free(req);
+
+	{
+		struct ethtool_rings_get_req *req;
+		struct ethtool_rings_get_rsp *rsp;
+
+		req = ethtool_rings_get_req_alloc();
+		ethtool_rings_get_req_set_header_dev_index(req, ifindex);
+		rsp = ethtool_rings_get(ys, req);
+		ethtool_rings_get_req_free(req);
+		if (rsp)
+			fprintf(stderr, "TCP header split: %d\n",
+				rsp->tcp_data_split);
+		ethtool_rings_get_rsp_free(rsp);
+	}
+
+	ynl_sock_destroy(ys);
+
+	return ret;
 }
 
 static int configure_rss(void)
@@ -354,6 +390,9 @@ int do_server(struct memory_buffer *mem)
 	if (reset_flow_steering())
 		error(1, 0, "Failed to reset flow steering\n");
 
+	if (configure_headersplit(1))
+		error(1, 0, "Failed to enable TCP header split\n");
+
 	/* Configure RSS to divert all traffic from our devmem queues */
 	if (configure_rss())
 		error(1, 0, "Failed to configure rss\n");
-- 
2.46.0

[PATCH net-next v2 09/12] selftests: ncdevmem: Remove hard-coded queue numbers

[Stanislav Fomichev]

Use single last queue of the device and probe it dynamically.

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/ncdevmem.c | 40 ++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
index a1fa818c8229..900a661a61af 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -48,8 +48,8 @@ static char *server_ip;
 static char *client_ip;
 static char *port;
 static size_t do_validation;
-static int start_queue = 8;
-static int num_queues = 8;
+static int start_queue = -1;
+static int num_queues = 1;
 static char *ifname;
 static unsigned int ifindex;
 static unsigned int dmabuf_id;
@@ -198,6 +198,33 @@ void validate_buffer(void *line, size_t size)
 	fprintf(stdout, "Validated buffer\n");
 }
 
+static int rxq_num(int ifindex)
+{
+	struct ethtool_channels_get_req *req;
+	struct ethtool_channels_get_rsp *rsp;
+	struct ynl_error yerr;
+	struct ynl_sock *ys;
+	int num = -1;
+
+	ys = ynl_sock_create(&ynl_ethtool_family, &yerr);
+	if (!ys) {
+		fprintf(stderr, "YNL: %s\n", yerr.msg);
+		return -1;
+	}
+
+	req = ethtool_channels_get_req_alloc();
+	ethtool_channels_get_req_set_header_dev_index(req, ifindex);
+	rsp = ethtool_channels_get(ys, req);
+	if (rsp)
+		num = rsp->rx_count + rsp->combined_count;
+	ethtool_channels_get_req_free(req);
+	ethtool_channels_get_rsp_free(rsp);
+
+	ynl_sock_destroy(ys);
+
+	return num;
+}
+
 #define run_command(cmd, ...)                                           \
 	({                                                              \
 		char command[256];                                      \
@@ -672,6 +699,15 @@ int main(int argc, char *argv[])
 
 	ifindex = if_nametoindex(ifname);
 
+	if (start_queue < 0) {
+		start_queue = rxq_num(ifindex) - 1;
+
+		if (start_queue < 0)
+			error(1, 0, "couldn't detect number of queues\n");
+
+		fprintf(stderr, "using queues %d..%d\n", start_queue, start_queue + num_queues);
+	}
+
 	for (; optind < argc; optind++)
 		fprintf(stderr, "extra arguments: %s\n", argv[optind]);
 
-- 
2.46.0

[PATCH net-next v2 10/12] selftests: ncdevmem: Run selftest when none of the -s or -c has been provided

[Stanislav Fomichev]

This will be used as a 'probe' mode in the selftest to check whether
the device supports the devmem or not. Use hard-coded queue layout
(two last queues) and prevent user from passing custom -q and/or -t.

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/ncdevmem.c | 27 +++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
index 900a661a61af..9b0a81b12eac 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -688,17 +688,26 @@ int main(int argc, char *argv[])
 		}
 	}
 
-	if (!server_ip)
-		error(1, 0, "Missing -s argument\n");
-
-	if (!port)
-		error(1, 0, "Missing -p argument\n");
-
 	if (!ifname)
 		error(1, 0, "Missing -f argument\n");
 
 	ifindex = if_nametoindex(ifname);
 
+	if (!server_ip && !client_ip) {
+		if (start_queue != -1)
+			error(1, 0, "don't support custom start queue for probing\n");
+		if (num_queues != 1)
+			error(1, 0, "don't support custom number of queues for probing\n");
+
+		start_queue = rxq_num(ifindex) - 2;
+		if (start_queue < 0)
+			error(1, 0, "couldn't detect number of queues\n");
+		num_queues = 2; /* make sure can bind to multiple queues */
+
+		run_devmem_tests();
+		return 0;
+	}
+
 	if (start_queue < 0) {
 		start_queue = rxq_num(ifindex) - 1;
 
@@ -711,7 +720,11 @@ int main(int argc, char *argv[])
 	for (; optind < argc; optind++)
 		fprintf(stderr, "extra arguments: %s\n", argv[optind]);
 
-	run_devmem_tests();
+	if (!server_ip)
+		error(1, 0, "Missing -s argument\n");
+
+	if (!port)
+		error(1, 0, "Missing -p argument\n");
 
 	mem = provider->alloc(getpagesize() * NUM_PAGES);
 	ret = is_server ? do_server(mem) : 1;
-- 
2.46.0

[PATCH net-next v2 11/12] selftests: ncdevmem: Move ncdevmem under drivers/net/hw

[Stanislav Fomichev]

This is where all the tests that depend on the HW functionality live in
and this is where the automated test is gonna be added in the next
patch.

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/drivers/net/hw/.gitignore        | 1 +
 tools/testing/selftests/drivers/net/hw/Makefile          | 9 +++++++++
 .../testing/selftests/{net => drivers/net/hw}/ncdevmem.c | 0
 tools/testing/selftests/net/.gitignore                   | 1 -
 tools/testing/selftests/net/Makefile                     | 9 ---------
 5 files changed, 10 insertions(+), 10 deletions(-)
 create mode 100644 tools/testing/selftests/drivers/net/hw/.gitignore
 rename tools/testing/selftests/{net => drivers/net/hw}/ncdevmem.c (100%)

diff --git a/tools/testing/selftests/drivers/net/hw/.gitignore b/tools/testing/selftests/drivers/net/hw/.gitignore
new file mode 100644
index 000000000000..e9fe6ede681a
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/hw/.gitignore
@@ -0,0 +1 @@
+ncdevmem
diff --git a/tools/testing/selftests/drivers/net/hw/Makefile b/tools/testing/selftests/drivers/net/hw/Makefile
index c9f2f48fc30f..7bce46817953 100644
--- a/tools/testing/selftests/drivers/net/hw/Makefile
+++ b/tools/testing/selftests/drivers/net/hw/Makefile
@@ -26,4 +26,13 @@ TEST_INCLUDES := \
 	../../../net/forwarding/tc_common.sh \
 	#
 
+# YNL files, must be before "include ..lib.mk"
+EXTRA_CLEAN += $(OUTPUT)/libynl.a $(OUTPUT)/ncdevmem
+YNL_GEN_FILES := ncdevmem
+TEST_GEN_FILES += $(YNL_GEN_FILES)
+
 include ../../../lib.mk
+
+# YNL build
+YNL_GENS := ethtool netdev
+include ../../../net/ynl.mk
diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/drivers/net/hw/ncdevmem.c
similarity index 100%
rename from tools/testing/selftests/net/ncdevmem.c
rename to tools/testing/selftests/drivers/net/hw/ncdevmem.c
diff --git a/tools/testing/selftests/net/.gitignore b/tools/testing/selftests/net/.gitignore
index 1c04c780db66..923bf098e2eb 100644
--- a/tools/testing/selftests/net/.gitignore
+++ b/tools/testing/selftests/net/.gitignore
@@ -17,7 +17,6 @@ ipv6_flowlabel
 ipv6_flowlabel_mgr
 log.txt
 msg_zerocopy
-ncdevmem
 nettest
 psock_fanout
 psock_snd
diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 9c970e96ed33..22a5d6a7c3f3 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -97,11 +97,6 @@ TEST_PROGS += fq_band_pktlimit.sh
 TEST_PROGS += vlan_hw_filter.sh
 TEST_PROGS += bpf_offload.py
 
-# YNL files, must be before "include ..lib.mk"
-EXTRA_CLEAN += $(OUTPUT)/libynl.a
-YNL_GEN_FILES := ncdevmem
-TEST_GEN_FILES += $(YNL_GEN_FILES)
-
 TEST_FILES := settings
 TEST_FILES += in_netns.sh lib.sh net_helper.sh setup_loopback.sh setup_veth.sh
 
@@ -111,10 +106,6 @@ TEST_INCLUDES := forwarding/lib.sh
 
 include ../lib.mk
 
-# YNL build
-YNL_GENS := ethtool netdev
-include ynl.mk
-
 $(OUTPUT)/epoll_busy_poll: LDLIBS += -lcap
 $(OUTPUT)/reuseport_bpf_numa: LDLIBS += -lnuma
 $(OUTPUT)/tcp_mmap: LDLIBS += -lpthread -lcrypto
-- 
2.46.0

[PATCH net-next v2 12/12] selftests: ncdevmem: Add automated test

[Stanislav Fomichev]

Only RX side for now and small message to test the setup.
In the future, we can extend it to TX side and to testing
both sides with a couple of megs of data.

  make \
  	-C tools/testing/selftests \
  	TARGETS="drivers/hw/net" \
  	install INSTALL_PATH=~/tmp/ksft

  scp ~/tmp/ksft ${HOST}:
  scp ~/tmp/ksft ${PEER}:

  cfg+="NETIF=${DEV}\n"
  cfg+="LOCAL_V6=${HOST_IP}\n"
  cfg+="REMOTE_V6=${PEER_IP}\n"
  cfg+="REMOTE_TYPE=ssh\n"
  cfg+="REMOTE_ARGS=root@${PEER}\n"

  echo -e "$cfg" | ssh root@${HOST} "cat > ksft/drivers/net/net.config"
  ssh root@${HOST} "cd ksft && ./run_kselftest.sh -t drivers/net:devmem.py"

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 .../testing/selftests/drivers/net/hw/Makefile |  1 +
 .../selftests/drivers/net/hw/devmem.py        | 46 +++++++++++++++++++
 2 files changed, 47 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/net/hw/devmem.py

diff --git a/tools/testing/selftests/drivers/net/hw/Makefile b/tools/testing/selftests/drivers/net/hw/Makefile
index 7bce46817953..a582b1bb3ae1 100644
--- a/tools/testing/selftests/drivers/net/hw/Makefile
+++ b/tools/testing/selftests/drivers/net/hw/Makefile
@@ -3,6 +3,7 @@
 TEST_PROGS = \
 	csum.py \
 	devlink_port_split.py \
+	devmem.py \
 	ethtool.sh \
 	ethtool_extended_state.sh \
 	ethtool_mm.sh \
diff --git a/tools/testing/selftests/drivers/net/hw/devmem.py b/tools/testing/selftests/drivers/net/hw/devmem.py
new file mode 100755
index 000000000000..29085591616b
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/hw/devmem.py
@@ -0,0 +1,46 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: GPL-2.0
+
+import errno
+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):
+    if not hasattr(cfg, "_devmem_probed"):
+        port = rand_port()
+        probe_command = f"./ncdevmem -f {cfg.ifname}"
+        cfg._devmem_supported = cmd(probe_command, fail=False, shell=True).ret == 0
+        cfg._devmem_probed = True
+
+    if not cfg._devmem_supported:
+        raise KsftSkipEx("Test requires devmem support")
+
+
+@ksft_disruptive
+def check_rx(cfg) -> None:
+    cfg.require_v6()
+    require_devmem(cfg)
+
+    port = rand_port()
+    listen_cmd = f"./ncdevmem -l -f {cfg.ifname} -s {cfg.v6} -p {port}"
+
+    with bkg(listen_cmd) as nc:
+        wait_port_listen(port)
+        cmd(f"echo -e \"hello\\nworld\"| nc {cfg.v6} {port}", host=cfg.remote, shell=True)
+
+    ksft_eq(nc.stdout.strip(), "hello\nworld")
+
+
+def main() -> None:
+    with NetDrvEpEnv(__file__) as cfg:
+        ksft_run([check_rx],
+                 args=(cfg, ))
+    ksft_exit()
+
+
+if __name__ == "__main__":
+    main()
-- 
2.46.0

Re: [PATCH net-next v2 04/12] selftests: ncdevmem: Make client_ip optional

[Mina Almasry]

On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> Support 3-tuple filtering by making client_ip optional. When -c is
> not passed, don't specify src-ip/src-port in the filter.
>
> Cc: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>

Sorry for the late review. Been out sick this week. I'll take a look
at some of the low hanging fruit out of this and hopefully finish the
rest before the end of the week.

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

-- 
Thanks,
Mina

Re: [PATCH net-next v2 03/12] selftests: ncdevmem: Unify error handling

[Mina Almasry]

On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> There is a bunch of places where error() calls look out of place.
> Use the same error(1, errno, ...) pattern everywhere.
>
> Cc: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>

Useful looking cleanup, the return code of ncdevmem is not checked and
not set correctly really. Returning 1 everywhere seems a bit cleaner.

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

Re: [PATCH net-next v2 05/12] selftests: ncdevmem: Remove default arguments

[Mina Almasry]

On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> To make it clear what's required and what's not. Also, some of the
> values don't seem like a good defaults; for example eth1.
>
> Cc: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> ---
>  tools/testing/selftests/net/ncdevmem.c | 34 +++++++++-----------------
>  1 file changed, 12 insertions(+), 22 deletions(-)
>
> diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> index 699692fdfd7d..bf446d74a4f0 100644
> --- a/tools/testing/selftests/net/ncdevmem.c
> +++ b/tools/testing/selftests/net/ncdevmem.c
> @@ -42,32 +42,13 @@
>  #define MSG_SOCK_DEVMEM 0x2000000
>  #endif
>
> -/*
> - * tcpdevmem netcat. Works similarly to netcat but does device memory TCP
> - * instead of regular TCP. Uses udmabuf to mock a dmabuf provider.
> - *
> - * Usage:
> - *
> - *     On server:
> - *     ncdevmem -s <server IP> -c <client IP> -f eth1 -l -p 5201 -v 7
> - *

No need to remove this documentation I think. This is useful since we
don't have a proper docs anywhere.

Please instead update the args in the above line, if they need
updating, but looks like it's already correct even after this change.

> - *     On client:
> - *     yes $(echo -e \\x01\\x02\\x03\\x04\\x05\\x06) | \
> - *             tr \\n \\0 | \
> - *             head -c 5G | \
> - *             nc <server IP> 5201 -p 5201
> - *
> - * Note this is compatible with regular netcat. i.e. the sender or receiver can
> - * be replaced with regular netcat to test the RX or TX path in isolation.
> - */
> -
> -static char *server_ip = "192.168.1.4";
> +static char *server_ip;
>  static char *client_ip;
> -static char *port = "5201";
> +static char *port;
>  static size_t do_validation;
>  static int start_queue = 8;
>  static int num_queues = 8;
> -static char *ifname = "eth1";
> +static char *ifname;
>  static unsigned int ifindex;
>  static unsigned int dmabuf_id;
>
> @@ -613,6 +594,15 @@ int main(int argc, char *argv[])
>                 }
>         }
>
> +       if (!server_ip)
> +               error(1, 0, "Missing -s argument\n");
> +
> +       if (!port)
> +               error(1, 0, "Missing -p argument\n");
> +
> +       if (!ifname)
> +               error(1, 0, "Missing -f argument\n");
> +
>         ifindex = if_nametoindex(ifname);
>
>         for (; optind < argc; optind++)
> --
> 2.46.0
>


-- 
Thanks,
Mina

Re: [PATCH net-next v2 07/12] selftests: ncdevmem: Properly reset flow steering

[Mina Almasry]

On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> ntuple off/on might be not enough to do it on all NICs.
> Add a bunch of shell crap to explicitly remove the rules.
>
> Cc: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> ---
>  tools/testing/selftests/net/ncdevmem.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> index 47458a13eff5..48cbf057fde7 100644
> --- a/tools/testing/selftests/net/ncdevmem.c
> +++ b/tools/testing/selftests/net/ncdevmem.c
> @@ -207,13 +207,12 @@ void validate_buffer(void *line, size_t size)
>
>  static int reset_flow_steering(void)
>  {
> -       int ret = 0;
> -
> -       ret = run_command("sudo ethtool -K %s ntuple off >&2", ifname);
> -       if (ret)
> -               return ret;
> -
> -       return run_command("sudo ethtool -K %s ntuple on >&2", ifname);
> +       run_command("sudo ethtool -K %s ntuple off >&2", ifname);
> +       run_command("sudo ethtool -K %s ntuple on >&2", ifname);
> +       run_command(
> +               "sudo ethtool -n %s | grep 'Filter:' | awk '{print $2}' | xargs -n1 ethtool -N %s delete >&2",
> +               ifname, ifname);
> +       return 0;

Any reason to remove the checking of the return codes? Silent failures
can waste time if the test fails and someone has to spend time finding
out its the flow steering reset that failed (it may not be very
obvious without the checking of the return code.

-- 
Thanks,
Mina

Re: [PATCH net-next v2 06/12] selftests: ncdevmem: Switch to AF_INET6

[Mina Almasry]

On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> Use dualstack socket to support both v4 and v6. v4-mapped-v6 address
> can be used to do v4.
>

The network on test machines I can run ncdevmem on are ipv4 only, and
ipv6 support is not possible for my test setup at all. Probably a mega
noob question, but are these changes going to regress such networks?
Or does the dualstack support handle ipv4 networks seamlessly?

If such regression is there, maybe we can add a -6 flag that enables
ipv6 support similar to how other binaries like nc do it?

Re: [PATCH net-next v2 09/12] selftests: ncdevmem: Remove hard-coded queue numbers

[Mina Almasry]

On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> Use single last queue of the device and probe it dynamically.
>

Sorry I know there was a pending discussion in the last iteration that
I didn't respond to. Been a rough week with me out sick a bit.

For this, the issue I see is that by default only 1 queue binding will
be tested, but I feel like test coverage for the multiple queues case
by default is very nice because I actually ran into some issues making
multi-queue binding work.

Can we change this so that, by default, it binds to the last rxq_num/2
queues of the device?

> Cc: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> ---
>  tools/testing/selftests/net/ncdevmem.c | 40 ++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> index a1fa818c8229..900a661a61af 100644
> --- a/tools/testing/selftests/net/ncdevmem.c
> +++ b/tools/testing/selftests/net/ncdevmem.c
> @@ -48,8 +48,8 @@ static char *server_ip;
>  static char *client_ip;
>  static char *port;
>  static size_t do_validation;
> -static int start_queue = 8;
> -static int num_queues = 8;
> +static int start_queue = -1;
> +static int num_queues = 1;
>  static char *ifname;
>  static unsigned int ifindex;
>  static unsigned int dmabuf_id;
> @@ -198,6 +198,33 @@ void validate_buffer(void *line, size_t size)
>         fprintf(stdout, "Validated buffer\n");
>  }
>
> +static int rxq_num(int ifindex)
> +{
> +       struct ethtool_channels_get_req *req;
> +       struct ethtool_channels_get_rsp *rsp;
> +       struct ynl_error yerr;
> +       struct ynl_sock *ys;
> +       int num = -1;
> +
> +       ys = ynl_sock_create(&ynl_ethtool_family, &yerr);
> +       if (!ys) {
> +               fprintf(stderr, "YNL: %s\n", yerr.msg);
> +               return -1;
> +       }
> +
> +       req = ethtool_channels_get_req_alloc();
> +       ethtool_channels_get_req_set_header_dev_index(req, ifindex);
> +       rsp = ethtool_channels_get(ys, req);
> +       if (rsp)
> +               num = rsp->rx_count + rsp->combined_count;
> +       ethtool_channels_get_req_free(req);
> +       ethtool_channels_get_rsp_free(rsp);
> +
> +       ynl_sock_destroy(ys);
> +
> +       return num;
> +}
> +
>  #define run_command(cmd, ...)                                           \
>         ({                                                              \
>                 char command[256];                                      \
> @@ -672,6 +699,15 @@ int main(int argc, char *argv[])
>
>         ifindex = if_nametoindex(ifname);
>
> +       if (start_queue < 0) {
> +               start_queue = rxq_num(ifindex) - 1;

I think the only changes needed are:

start_queue = rxq_num(ifindex) / 2;
num_queues = rxq_num(ifindex) - start_queue;

(I may have an off-by-1 error somewhere with this math).


-- 
Thanks,
Mina

Re: [PATCH net-next v2 08/12] selftests: ncdevmem: Use YNL to enable TCP header split

[Mina Almasry]

On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> In the next patch the hard-coded queue numbers are gonna be removed.
> So introduce some initial support for ethtool YNL and use
> it to enable header split.
>
> Also, tcp-data-split requires latest ethtool which is unlikely
> to be present in the distros right now.
>
> (ideally, we should not shell out to ethtool at all).
>
> Cc: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> ---
>  tools/testing/selftests/net/Makefile   |  2 +-
>  tools/testing/selftests/net/ncdevmem.c | 43 ++++++++++++++++++++++++--
>  2 files changed, 42 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
> index 649f1fe0dc46..9c970e96ed33 100644
> --- a/tools/testing/selftests/net/Makefile
> +++ b/tools/testing/selftests/net/Makefile
> @@ -112,7 +112,7 @@ TEST_INCLUDES := forwarding/lib.sh
>  include ../lib.mk
>
>  # YNL build
> -YNL_GENS := netdev
> +YNL_GENS := ethtool netdev
>  include ynl.mk
>
>  $(OUTPUT)/epoll_busy_poll: LDLIBS += -lcap
> diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> index 48cbf057fde7..a1fa818c8229 100644
> --- a/tools/testing/selftests/net/ncdevmem.c
> +++ b/tools/testing/selftests/net/ncdevmem.c
> @@ -28,10 +28,12 @@
>  #include <linux/netlink.h>
>  #include <linux/genetlink.h>
>  #include <linux/netdev.h>
> +#include <linux/ethtool_netlink.h>
>  #include <time.h>
>  #include <net/if.h>
>
>  #include "netdev-user.h"
> +#include "ethtool-user.h"
>  #include <ynl.h>
>
>  #define PAGE_SHIFT 12
> @@ -217,8 +219,42 @@ static int reset_flow_steering(void)
>
>  static int configure_headersplit(bool on)
>  {
> -       return run_command("sudo ethtool -G %s tcp-data-split %s >&2", ifname,
> -                          on ? "on" : "off");
> +       struct ethtool_rings_set_req *req;
> +       struct ynl_error yerr;
> +       struct ynl_sock *ys;
> +       int ret;
> +
> +       ys = ynl_sock_create(&ynl_ethtool_family, &yerr);
> +       if (!ys) {
> +               fprintf(stderr, "YNL: %s\n", yerr.msg);
> +               return -1;
> +       }
> +
> +       req = ethtool_rings_set_req_alloc();
> +       ethtool_rings_set_req_set_header_dev_index(req, ifindex);
> +       ethtool_rings_set_req_set_tcp_data_split(req, on ? 2 : 0);

I'm guessing 2 is explicit on? 1 being auto probably? A comment would
be nice, but that's just a nit.

> +       ret = ethtool_rings_set(ys, req);
> +       if (ret < 0)
> +               fprintf(stderr, "YNL failed: %s\n", ys->err.msg);

Don't you wanna return ret; here on error?

> +       ethtool_rings_set_req_free(req);
> +
> +       {
> +               struct ethtool_rings_get_req *req;
> +               struct ethtool_rings_get_rsp *rsp;
> +

I'm guessing you're creating a new scope to re-declare req/rsp? To be
honest it's a bit weird style I haven't seen anywhere else. I would
prefer get_req and get_rsp instead, but this is a nit.

> +               req = ethtool_rings_get_req_alloc();
> +               ethtool_rings_get_req_set_header_dev_index(req, ifindex);
> +               rsp = ethtool_rings_get(ys, req);
> +               ethtool_rings_get_req_free(req);
> +               if (rsp)
> +                       fprintf(stderr, "TCP header split: %d\n",
> +                               rsp->tcp_data_split);

I would prefer to cehck that rsp->tcp_data_split == 2 for 'on' and ==
0 for 'off' instead of just printing and relying on the user to notice
the mismatch in the logs.

Consider addressing the feedback, but mostly nits/improvements that
can be done later, so:

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

-- 
Thanks,
Mina

Re: [PATCH net-next v2 10/12] selftests: ncdevmem: Run selftest when none of the -s or -c has been provided

[Mina Almasry]

On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> This will be used as a 'probe' mode in the selftest to check whether
> the device supports the devmem or not.

It's not really 'probing'. Running ncdevmem with -s or -c does the
data path tests. Running ncdevmem without does all the control path
tests in run_devmem_tests(). It's not just probing driver for support,
it's mean to catch bugs. Maybe rename to 'control path tests' or
'config tests' instead of probing.


> Use hard-coded queue layout
> (two last queues) and prevent user from passing custom -q and/or -t.
>
> Cc: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> ---
>  tools/testing/selftests/net/ncdevmem.c | 27 +++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> index 900a661a61af..9b0a81b12eac 100644
> --- a/tools/testing/selftests/net/ncdevmem.c
> +++ b/tools/testing/selftests/net/ncdevmem.c
> @@ -688,17 +688,26 @@ int main(int argc, char *argv[])
>                 }
>         }
>
> -       if (!server_ip)
> -               error(1, 0, "Missing -s argument\n");
> -
> -       if (!port)
> -               error(1, 0, "Missing -p argument\n");
> -
>         if (!ifname)
>                 error(1, 0, "Missing -f argument\n");
>
>         ifindex = if_nametoindex(ifname);
>
> +       if (!server_ip && !client_ip) {
> +               if (start_queue != -1)
> +                       error(1, 0, "don't support custom start queue for probing\n");
> +               if (num_queues != 1)
> +                       error(1, 0, "don't support custom number of queues for probing\n");
> +

Remove the start_queue + num_queues check here please. I would like to
be able to run the control path tests binding dmabuf to all the queues
or 1 of the queues or some of the queues.

-- 
Thanks,
Mina

Re: [PATCH net-next v2 11/12] selftests: ncdevmem: Move ncdevmem under drivers/net/hw

[Mina Almasry]

On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> This is where all the tests that depend on the HW functionality live in
> and this is where the automated test is gonna be added in the next
> patch.
>

Tbh I don't like this very much. I wanted to take ncdevmem in the
opposite direction: to make at least the control path tests runnable
on netdevsim or something like that and have it not require any HW
support at all.

But I see in the cover letter that Jakub himself asked for the move,
so if there is some strong reason to make this in hw, sure.

Does it being under HW preclude future improvements to making it a
non-HW dependent test?

-- 
Thanks,
Mina

Re: [PATCH net-next v2 12/12] selftests: ncdevmem: Add automated test

[Mina Almasry]

On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> Only RX side for now and small message to test the setup.
> In the future, we can extend it to TX side and to testing
> both sides with a couple of megs of data.
>

This is really awesome. Thank you.

>   make \
>         -C tools/testing/selftests \
>         TARGETS="drivers/hw/net" \
>         install INSTALL_PATH=~/tmp/ksft
>
>   scp ~/tmp/ksft ${HOST}:
>   scp ~/tmp/ksft ${PEER}:
>
>   cfg+="NETIF=${DEV}\n"
>   cfg+="LOCAL_V6=${HOST_IP}\n"
>   cfg+="REMOTE_V6=${PEER_IP}\n"

Not a review comment but noob question: does NIPA not support ipv4? Or
is ipv6 preferred here?

>   cfg+="REMOTE_TYPE=ssh\n"
>   cfg+="REMOTE_ARGS=root@${PEER}\n"
>
>   echo -e "$cfg" | ssh root@${HOST} "cat > ksft/drivers/net/net.config"
>   ssh root@${HOST} "cd ksft && ./run_kselftest.sh -t drivers/net:devmem.py"
>
> Cc: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> ---
>  .../testing/selftests/drivers/net/hw/Makefile |  1 +
>  .../selftests/drivers/net/hw/devmem.py        | 46 +++++++++++++++++++
>  2 files changed, 47 insertions(+)
>  create mode 100755 tools/testing/selftests/drivers/net/hw/devmem.py
>
> diff --git a/tools/testing/selftests/drivers/net/hw/Makefile b/tools/testing/selftests/drivers/net/hw/Makefile
> index 7bce46817953..a582b1bb3ae1 100644
> --- a/tools/testing/selftests/drivers/net/hw/Makefile
> +++ b/tools/testing/selftests/drivers/net/hw/Makefile
> @@ -3,6 +3,7 @@
>  TEST_PROGS = \
>         csum.py \
>         devlink_port_split.py \
> +       devmem.py \
>         ethtool.sh \
>         ethtool_extended_state.sh \
>         ethtool_mm.sh \
> diff --git a/tools/testing/selftests/drivers/net/hw/devmem.py b/tools/testing/selftests/drivers/net/hw/devmem.py
> new file mode 100755
> index 000000000000..29085591616b
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/devmem.py
> @@ -0,0 +1,46 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0
> +
> +import errno
> +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):
> +    if not hasattr(cfg, "_devmem_probed"):
> +        port = rand_port()
> +        probe_command = f"./ncdevmem -f {cfg.ifname}"
> +        cfg._devmem_supported = cmd(probe_command, fail=False, shell=True).ret == 0
> +        cfg._devmem_probed = True
> +
> +    if not cfg._devmem_supported:
> +        raise KsftSkipEx("Test requires devmem support")
> +
> +
> +@ksft_disruptive
> +def check_rx(cfg) -> None:
> +    cfg.require_v6()
> +    require_devmem(cfg)
> +
> +    port = rand_port()
> +    listen_cmd = f"./ncdevmem -l -f {cfg.ifname} -s {cfg.v6} -p {port}"

So AFAICT adding validation to this test is simple. What you would do
is change the above line to:

listen_cmd = f"./ncdevmem -l -f {cfg.ifname} -s {cfg.v6} -p {port} -v 7"

then, below...

> +
> +    with bkg(listen_cmd) as nc:
> +        wait_port_listen(port)
> +        cmd(f"echo -e \"hello\\nworld\"| nc {cfg.v6} {port}", host=cfg.remote, shell=True)
> +

...change this to the equivalent of 'yes $(echo -e
\\x01\\x02\\x03\\x04\\x05\\x06) | tr \\n \\0 | head -c 1G"

> +    ksft_eq(nc.stdout.strip(), "hello\nworld")
> +

...then remove this ksft_eq().

But this is just a suggestion, I think you were leaving this to future
work for me, which is fine.

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


-- 
Thanks,
Mina

Re: [PATCH net-next v2 05/12] selftests: ncdevmem: Remove default arguments

[Stanislav Fomichev]

On 10/02, Mina Almasry wrote:
> On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > To make it clear what's required and what's not. Also, some of the
> > values don't seem like a good defaults; for example eth1.
> >
> > Cc: Mina Almasry <almasrymina@google.com>
> > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > ---
> >  tools/testing/selftests/net/ncdevmem.c | 34 +++++++++-----------------
> >  1 file changed, 12 insertions(+), 22 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> > index 699692fdfd7d..bf446d74a4f0 100644
> > --- a/tools/testing/selftests/net/ncdevmem.c
> > +++ b/tools/testing/selftests/net/ncdevmem.c
> > @@ -42,32 +42,13 @@
> >  #define MSG_SOCK_DEVMEM 0x2000000
> >  #endif
> >
> > -/*
> > - * tcpdevmem netcat. Works similarly to netcat but does device memory TCP
> > - * instead of regular TCP. Uses udmabuf to mock a dmabuf provider.
> > - *
> > - * Usage:
> > - *
> > - *     On server:
> > - *     ncdevmem -s <server IP> -c <client IP> -f eth1 -l -p 5201 -v 7
> > - *
> 
> No need to remove this documentation I think. This is useful since we
> don't have a proper docs anywhere.
> 
> Please instead update the args in the above line, if they need
> updating, but looks like it's already correct even after this change.

The client needs '-s' part. That's why I removed it - most likely
will tend to go stale and we now have the invocation example in the
selftest. But if you want to keep it, how about I move it to the
top of the file and cleanup a bit? Will do for the next iteration..

Re: [PATCH net-next v2 07/12] selftests: ncdevmem: Properly reset flow steering

[Stanislav Fomichev]

On 10/03, Mina Almasry wrote:
> On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > ntuple off/on might be not enough to do it on all NICs.
> > Add a bunch of shell crap to explicitly remove the rules.
> >
> > Cc: Mina Almasry <almasrymina@google.com>
> > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > ---
> >  tools/testing/selftests/net/ncdevmem.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> > index 47458a13eff5..48cbf057fde7 100644
> > --- a/tools/testing/selftests/net/ncdevmem.c
> > +++ b/tools/testing/selftests/net/ncdevmem.c
> > @@ -207,13 +207,12 @@ void validate_buffer(void *line, size_t size)
> >
> >  static int reset_flow_steering(void)
> >  {
> > -       int ret = 0;
> > -
> > -       ret = run_command("sudo ethtool -K %s ntuple off >&2", ifname);
> > -       if (ret)
> > -               return ret;
> > -
> > -       return run_command("sudo ethtool -K %s ntuple on >&2", ifname);
> > +       run_command("sudo ethtool -K %s ntuple off >&2", ifname);
> > +       run_command("sudo ethtool -K %s ntuple on >&2", ifname);
> > +       run_command(
> > +               "sudo ethtool -n %s | grep 'Filter:' | awk '{print $2}' | xargs -n1 ethtool -N %s delete >&2",
> > +               ifname, ifname);
> > +       return 0;
> 
> Any reason to remove the checking of the return codes? Silent failures
> can waste time if the test fails and someone has to spend time finding
> out its the flow steering reset that failed (it may not be very
> obvious without the checking of the return code.

IIRC, for me the 'ntuple off' part fails because the NIC doesn't let me
turn it of. And the new "ethtool .. | grep 'Filter: ' | ..." also fails
when there are no existing filters. 

I will add a comment to clarify..

Re: [PATCH net-next v2 06/12] selftests: ncdevmem: Switch to AF_INET6

[Stanislav Fomichev]

On 10/03, Mina Almasry wrote:
> On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > Use dualstack socket to support both v4 and v6. v4-mapped-v6 address
> > can be used to do v4.
> >
> 
> The network on test machines I can run ncdevmem on are ipv4 only, and
> ipv6 support is not possible for my test setup at all. Probably a mega
> noob question, but are these changes going to regress such networks?
> Or does the dualstack support handle ipv4 networks seamlessly?
> 
> If such regression is there, maybe we can add a -6 flag that enables
> ipv6 support similar to how other binaries like nc do it?

As long as your kernel is compiled with IPv6 (which all kernel in the
past 10+ years do) and you don't toggle net.ipv6.bindv6only sysctl to 1
(which defaults to 0 and afaik, none of the distros change it to 1),
AF_INET6 should properly support both v4 and v6 even if you don't have any
v6 environment setup.

Re: [PATCH net-next v2 08/12] selftests: ncdevmem: Use YNL to enable TCP header split

[Stanislav Fomichev]

On 10/03, Mina Almasry wrote:
> On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > In the next patch the hard-coded queue numbers are gonna be removed.
> > So introduce some initial support for ethtool YNL and use
> > it to enable header split.
> >
> > Also, tcp-data-split requires latest ethtool which is unlikely
> > to be present in the distros right now.
> >
> > (ideally, we should not shell out to ethtool at all).
> >
> > Cc: Mina Almasry <almasrymina@google.com>
> > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > ---
> >  tools/testing/selftests/net/Makefile   |  2 +-
> >  tools/testing/selftests/net/ncdevmem.c | 43 ++++++++++++++++++++++++--
> >  2 files changed, 42 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
> > index 649f1fe0dc46..9c970e96ed33 100644
> > --- a/tools/testing/selftests/net/Makefile
> > +++ b/tools/testing/selftests/net/Makefile
> > @@ -112,7 +112,7 @@ TEST_INCLUDES := forwarding/lib.sh
> >  include ../lib.mk
> >
> >  # YNL build
> > -YNL_GENS := netdev
> > +YNL_GENS := ethtool netdev
> >  include ynl.mk
> >
> >  $(OUTPUT)/epoll_busy_poll: LDLIBS += -lcap
> > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> > index 48cbf057fde7..a1fa818c8229 100644
> > --- a/tools/testing/selftests/net/ncdevmem.c
> > +++ b/tools/testing/selftests/net/ncdevmem.c
> > @@ -28,10 +28,12 @@
> >  #include <linux/netlink.h>
> >  #include <linux/genetlink.h>
> >  #include <linux/netdev.h>
> > +#include <linux/ethtool_netlink.h>
> >  #include <time.h>
> >  #include <net/if.h>
> >
> >  #include "netdev-user.h"
> > +#include "ethtool-user.h"
> >  #include <ynl.h>
> >
> >  #define PAGE_SHIFT 12
> > @@ -217,8 +219,42 @@ static int reset_flow_steering(void)
> >
> >  static int configure_headersplit(bool on)
> >  {
> > -       return run_command("sudo ethtool -G %s tcp-data-split %s >&2", ifname,
> > -                          on ? "on" : "off");
> > +       struct ethtool_rings_set_req *req;
> > +       struct ynl_error yerr;
> > +       struct ynl_sock *ys;
> > +       int ret;
> > +
> > +       ys = ynl_sock_create(&ynl_ethtool_family, &yerr);
> > +       if (!ys) {
> > +               fprintf(stderr, "YNL: %s\n", yerr.msg);
> > +               return -1;
> > +       }
> > +
> > +       req = ethtool_rings_set_req_alloc();
> > +       ethtool_rings_set_req_set_header_dev_index(req, ifindex);
> > +       ethtool_rings_set_req_set_tcp_data_split(req, on ? 2 : 0);
> 
> I'm guessing 2 is explicit on? 1 being auto probably? A comment would
> be nice, but that's just a nit.

Sure, will add.

> > +       ret = ethtool_rings_set(ys, req);
> > +       if (ret < 0)
> > +               fprintf(stderr, "YNL failed: %s\n", ys->err.msg);
> 
> Don't you wanna return ret; here on error?

Good point. Will add 'if (ret == 0)' to the get request.

> > +       ethtool_rings_set_req_free(req);
> > +
> > +       {
> > +               struct ethtool_rings_get_req *req;
> > +               struct ethtool_rings_get_rsp *rsp;
> > +
> 
> I'm guessing you're creating a new scope to re-declare req/rsp? To be
> honest it's a bit weird style I haven't seen anywhere else. I would
> prefer get_req and get_rsp instead, but this is a nit.

SG.

> > +               req = ethtool_rings_get_req_alloc();
> > +               ethtool_rings_get_req_set_header_dev_index(req, ifindex);
> > +               rsp = ethtool_rings_get(ys, req);
> > +               ethtool_rings_get_req_free(req);
> > +               if (rsp)
> > +                       fprintf(stderr, "TCP header split: %d\n",
> > +                               rsp->tcp_data_split);
> 
> I would prefer to cehck that rsp->tcp_data_split == 2 for 'on' and ==
> 0 for 'off' instead of just printing and relying on the user to notice
> the mismatch in the logs.

Will do, thanks!
 
> Consider addressing the feedback, but mostly nits/improvements that
> can be done later, so:
> 
> Reviewed-by: Mina Almasry <almasrymina@google.com>

Re: [PATCH net-next v2 09/12] selftests: ncdevmem: Remove hard-coded queue numbers

[Stanislav Fomichev]

On 10/03, Mina Almasry wrote:
> On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > Use single last queue of the device and probe it dynamically.
> >
> 
> Sorry I know there was a pending discussion in the last iteration that
> I didn't respond to. Been a rough week with me out sick a bit.
> 
> For this, the issue I see is that by default only 1 queue binding will
> be tested, but I feel like test coverage for the multiple queues case
> by default is very nice because I actually ran into some issues making
> multi-queue binding work.
> 
> Can we change this so that, by default, it binds to the last rxq_num/2
> queues of the device?

I'm probably missing something, but why do you think exercising this from
the probe/selftest mode is not enough? It might be confusing for the readers
to understand why we bind to half of the queues and flow steer into them
when in reality there is only single tcp flow.

IOW, can we keep these two modes:
1. server / client - use single queue
2. selftest / probe - use more than 1 queue by default (and I'll remove the
   checks that enforce the number of queues for this mode to let the
   users override)

Re: [PATCH net-next v2 06/12] selftests: ncdevmem: Switch to AF_INET6

[Mina Almasry]

On Thu, Oct 3, 2024 at 9:47 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 10/03, Mina Almasry wrote:
> > On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > >
> > > Use dualstack socket to support both v4 and v6. v4-mapped-v6 address
> > > can be used to do v4.
> > >
> >
> > The network on test machines I can run ncdevmem on are ipv4 only, and
> > ipv6 support is not possible for my test setup at all. Probably a mega
> > noob question, but are these changes going to regress such networks?
> > Or does the dualstack support handle ipv4 networks seamlessly?
> >
> > If such regression is there, maybe we can add a -6 flag that enables
> > ipv6 support similar to how other binaries like nc do it?
>
> As long as your kernel is compiled with IPv6 (which all kernel in the
> past 10+ years do) and you don't toggle net.ipv6.bindv6only sysctl to 1
> (which defaults to 0 and afaik, none of the distros change it to 1),
> AF_INET6 should properly support both v4 and v6 even if you don't have any
> v6 environment setup.

Thank you for the details!

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

-- 
Thanks,
Mina

Re: [PATCH net-next v2 10/12] selftests: ncdevmem: Run selftest when none of the -s or -c has been provided

[Stanislav Fomichev]

On 10/03, Mina Almasry wrote:
> On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > This will be used as a 'probe' mode in the selftest to check whether
> > the device supports the devmem or not.
> 
> It's not really 'probing'. Running ncdevmem with -s or -c does the
> data path tests. Running ncdevmem without does all the control path
> tests in run_devmem_tests(). It's not just probing driver for support,
> it's mean to catch bugs. Maybe rename to 'control path tests' or
> 'config tests' instead of probing.

Re 'probing' vs 'control path tests': I need something I can call
in the python selftest to tell me whether the nic supports devmem
or not (to skip the tests on the unsupported nics), so I'm reusing this
'control path tests' mode for this. I do agree that there might be an
issue where the nic supports devmem, but has some bug which causes
'control path tests' to fail which leads to skipping the data plane tests...

We can try to separate these two in the future. (and I'll keep the word
'probing' for now since it's only in the commit message to describe the
intent)

> > Use hard-coded queue layout
> > (two last queues) and prevent user from passing custom -q and/or -t.
> >
> > Cc: Mina Almasry <almasrymina@google.com>
> > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > ---
> >  tools/testing/selftests/net/ncdevmem.c | 27 +++++++++++++++++++-------
> >  1 file changed, 20 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> > index 900a661a61af..9b0a81b12eac 100644
> > --- a/tools/testing/selftests/net/ncdevmem.c
> > +++ b/tools/testing/selftests/net/ncdevmem.c
> > @@ -688,17 +688,26 @@ int main(int argc, char *argv[])
> >                 }
> >         }
> >
> > -       if (!server_ip)
> > -               error(1, 0, "Missing -s argument\n");
> > -
> > -       if (!port)
> > -               error(1, 0, "Missing -p argument\n");
> > -
> >         if (!ifname)
> >                 error(1, 0, "Missing -f argument\n");
> >
> >         ifindex = if_nametoindex(ifname);
> >
> > +       if (!server_ip && !client_ip) {
> > +               if (start_queue != -1)
> > +                       error(1, 0, "don't support custom start queue for probing\n");
> > +               if (num_queues != 1)
> > +                       error(1, 0, "don't support custom number of queues for probing\n");
> > +
> 
> Remove the start_queue + num_queues check here please. I would like to
> be able to run the control path tests binding dmabuf to all the queues
> or 1 of the queues or some of the queues.

Will remove, but let's continue discussing this in the other thread.

Re: [PATCH net-next v2 11/12] selftests: ncdevmem: Move ncdevmem under drivers/net/hw

[Stanislav Fomichev]

On 10/03, Mina Almasry wrote:
> On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > This is where all the tests that depend on the HW functionality live in
> > and this is where the automated test is gonna be added in the next
> > patch.
> >
> 
> Tbh I don't like this very much. I wanted to take ncdevmem in the
> opposite direction: to make at least the control path tests runnable
> on netdevsim or something like that and have it not require any HW
> support at all.
> 
> But I see in the cover letter that Jakub himself asked for the move,
> so if there is some strong reason to make this in hw, sure.
> 
> Does it being under HW preclude future improvements to making it a
> non-HW dependent test?

I'm moving it under drivers/net/hw only because I want ncdevmem to end
up as a TEST_GEN_FILES dependency (drivers/net/hw is the directory
that the vendors will eventually run against their HW so this is
where the HW-dependent tests are gonna stay for now). And I'm not sure I
can do a cross directory dependency in TEST_GEN_FILES. But maybe I
haven't looked hard enough?

Let's not marry too much to the location? When you get ncdevmem
working with netdevsim, we can move the file to a different location.

Re: [PATCH net-next v2 01/12] selftests: ncdevmem: Redirect all non-payload output to stderr

[Mina Almasry]

On Mon, Sep 30, 2024 at 10:17 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> That should make it possible to do expected payload validation on
> the caller side.
>
> Cc: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>

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

> ---
>  tools/testing/selftests/net/ncdevmem.c | 61 +++++++++++++-------------
>  1 file changed, 30 insertions(+), 31 deletions(-)
>
> diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> index 64d6805381c5..9245d3f158dd 100644
> --- a/tools/testing/selftests/net/ncdevmem.c
> +++ b/tools/testing/selftests/net/ncdevmem.c
> @@ -88,7 +88,6 @@ void print_nonzero_bytes(void *ptr, size_t size)
>
>         for (i = 0; i < size; i++)
>                 putchar(p[i]);
> -       printf("\n");
>  }
>
>  void validate_buffer(void *line, size_t size)
> @@ -120,7 +119,7 @@ void validate_buffer(void *line, size_t size)
>                 char command[256];                                      \
>                 memset(command, 0, sizeof(command));                    \
>                 snprintf(command, sizeof(command), cmd, ##__VA_ARGS__); \
> -               printf("Running: %s\n", command);                       \
> +               fprintf(stderr, "Running: %s\n", command);                       \
>                 system(command);                                        \
>         })
>
> @@ -128,22 +127,22 @@ static int reset_flow_steering(void)
>  {
>         int ret = 0;
>
> -       ret = run_command("sudo ethtool -K %s ntuple off", ifname);
> +       ret = run_command("sudo ethtool -K %s ntuple off >&2", ifname);
>         if (ret)
>                 return ret;
>
> -       return run_command("sudo ethtool -K %s ntuple on", ifname);
> +       return run_command("sudo ethtool -K %s ntuple on >&2", ifname);
>  }
>
>  static int configure_headersplit(bool on)
>  {
> -       return run_command("sudo ethtool -G %s tcp-data-split %s", ifname,
> +       return run_command("sudo ethtool -G %s tcp-data-split %s >&2", ifname,
>                            on ? "on" : "off");
>  }
>
>  static int configure_rss(void)
>  {
> -       return run_command("sudo ethtool -X %s equal %d", ifname, start_queue);
> +       return run_command("sudo ethtool -X %s equal %d >&2", ifname, start_queue);
>  }
>
>  static int configure_channels(unsigned int rx, unsigned int tx)
> @@ -153,7 +152,7 @@ static int configure_channels(unsigned int rx, unsigned int tx)
>
>  static int configure_flow_steering(void)
>  {
> -       return run_command("sudo ethtool -N %s flow-type tcp4 src-ip %s dst-ip %s src-port %s dst-port %s queue %d",
> +       return run_command("sudo ethtool -N %s flow-type tcp4 src-ip %s dst-ip %s src-port %s dst-port %s queue %d >&2",
>                            ifname, client_ip, server_ip, port, port, start_queue);
>  }
>
> @@ -187,7 +186,7 @@ static int bind_rx_queue(unsigned int ifindex, unsigned int dmabuf_fd,
>                 goto err_close;
>         }
>
> -       printf("got dmabuf id=%d\n", rsp->id);
> +       fprintf(stderr, "got dmabuf id=%d\n", rsp->id);
>         dmabuf_id = rsp->id;
>
>         netdev_bind_rx_req_free(req);
> @@ -314,8 +313,8 @@ int do_server(void)
>         if (ret)
>                 error(errno, errno, "%s: [FAIL, set sock opt]\n", TEST_PREFIX);
>
> -       printf("binding to address %s:%d\n", server_ip,
> -              ntohs(server_sin.sin_port));
> +       fprintf(stderr, "binding to address %s:%d\n", server_ip,
> +               ntohs(server_sin.sin_port));
>
>         ret = bind(socket_fd, &server_sin, sizeof(server_sin));
>         if (ret)
> @@ -329,14 +328,14 @@ int do_server(void)
>
>         inet_ntop(server_sin.sin_family, &server_sin.sin_addr, buffer,
>                   sizeof(buffer));
> -       printf("Waiting or connection on %s:%d\n", buffer,
> -              ntohs(server_sin.sin_port));
> +       fprintf(stderr, "Waiting or connection on %s:%d\n", buffer,
> +               ntohs(server_sin.sin_port));
>         client_fd = accept(socket_fd, &client_addr, &client_addr_len);
>
>         inet_ntop(client_addr.sin_family, &client_addr.sin_addr, buffer,
>                   sizeof(buffer));
> -       printf("Got connection from %s:%d\n", buffer,
> -              ntohs(client_addr.sin_port));
> +       fprintf(stderr, "Got connection from %s:%d\n", buffer,
> +               ntohs(client_addr.sin_port));
>
>         while (1) {
>                 struct iovec iov = { .iov_base = iobuf,
> @@ -349,14 +348,13 @@ int do_server(void)
>                 ssize_t ret;
>
>                 is_devmem = false;
> -               printf("\n\n");
>
>                 msg.msg_iov = &iov;
>                 msg.msg_iovlen = 1;
>                 msg.msg_control = ctrl_data;
>                 msg.msg_controllen = sizeof(ctrl_data);
>                 ret = recvmsg(client_fd, &msg, MSG_SOCK_DEVMEM);
> -               printf("recvmsg ret=%ld\n", ret);
> +               fprintf(stderr, "recvmsg ret=%ld\n", ret);
>                 if (ret < 0 && (errno == EAGAIN || errno == EWOULDBLOCK))
>                         continue;
>                 if (ret < 0) {
> @@ -364,7 +362,7 @@ int do_server(void)
>                         continue;
>                 }
>                 if (ret == 0) {
> -                       printf("client exited\n");
> +                       fprintf(stderr, "client exited\n");
>                         goto cleanup;
>                 }
>
> @@ -373,7 +371,7 @@ int do_server(void)
>                         if (cm->cmsg_level != SOL_SOCKET ||
>                             (cm->cmsg_type != SCM_DEVMEM_DMABUF &&
>                              cm->cmsg_type != SCM_DEVMEM_LINEAR)) {
> -                               fprintf(stdout, "skipping non-devmem cmsg\n");
> +                               fprintf(stderr, "skipping non-devmem cmsg\n");
>                                 continue;
>                         }
>
> @@ -384,7 +382,7 @@ int do_server(void)
>                                 /* TODO: process data copied from skb's linear
>                                  * buffer.
>                                  */
> -                               fprintf(stdout,
> +                               fprintf(stderr,
>                                         "SCM_DEVMEM_LINEAR. dmabuf_cmsg->frag_size=%u\n",
>                                         dmabuf_cmsg->frag_size);
>
> @@ -395,12 +393,13 @@ int do_server(void)
>                         token.token_count = 1;
>
>                         total_received += dmabuf_cmsg->frag_size;
> -                       printf("received frag_page=%llu, in_page_offset=%llu, frag_offset=%llu, frag_size=%u, token=%u, total_received=%lu, dmabuf_id=%u\n",
> -                              dmabuf_cmsg->frag_offset >> PAGE_SHIFT,
> -                              dmabuf_cmsg->frag_offset % getpagesize(),
> -                              dmabuf_cmsg->frag_offset, dmabuf_cmsg->frag_size,
> -                              dmabuf_cmsg->frag_token, total_received,
> -                              dmabuf_cmsg->dmabuf_id);
> +                       fprintf(stderr,
> +                               "received frag_page=%llu, in_page_offset=%llu, frag_offset=%llu, frag_size=%u, token=%u, total_received=%lu, dmabuf_id=%u\n",
> +                               dmabuf_cmsg->frag_offset >> PAGE_SHIFT,
> +                               dmabuf_cmsg->frag_offset % getpagesize(),
> +                               dmabuf_cmsg->frag_offset,
> +                               dmabuf_cmsg->frag_size, dmabuf_cmsg->frag_token,
> +                               total_received, dmabuf_cmsg->dmabuf_id);
>
>                         if (dmabuf_cmsg->dmabuf_id != dmabuf_id)
>                                 error(1, 0,
> @@ -438,15 +437,15 @@ int do_server(void)
>                 if (!is_devmem)
>                         error(1, 0, "flow steering error\n");
>
> -               printf("total_received=%lu\n", total_received);
> +               fprintf(stderr, "total_received=%lu\n", total_received);
>         }
>
> -       fprintf(stdout, "%s: ok\n", TEST_PREFIX);
> +       fprintf(stderr, "%s: ok\n", TEST_PREFIX);
>
> -       fprintf(stdout, "page_aligned_frags=%lu, non_page_aligned_frags=%lu\n",
> +       fprintf(stderr, "page_aligned_frags=%lu, non_page_aligned_frags=%lu\n",
>                 page_aligned_frags, non_page_aligned_frags);
>
> -       fprintf(stdout, "page_aligned_frags=%lu, non_page_aligned_frags=%lu\n",
> +       fprintf(stderr, "page_aligned_frags=%lu, non_page_aligned_frags=%lu\n",
>                 page_aligned_frags, non_page_aligned_frags);
>
>  cleanup:
> @@ -551,7 +550,7 @@ int main(int argc, char *argv[])
>                         ifname = optarg;
>                         break;
>                 case '?':
> -                       printf("unknown option: %c\n", optopt);
> +                       fprintf(stderr, "unknown option: %c\n", optopt);
>                         break;
>                 }
>         }
> @@ -559,7 +558,7 @@ int main(int argc, char *argv[])
>         ifindex = if_nametoindex(ifname);
>
>         for (; optind < argc; optind++)
> -               printf("extra arguments: %s\n", argv[optind]);
> +               fprintf(stderr, "extra arguments: %s\n", argv[optind]);
>
>         run_devmem_tests();
>
> --
> 2.46.0
>


-- 
Thanks,
Mina

Re: [PATCH net-next v2 12/12] selftests: ncdevmem: Add automated test

[Stanislav Fomichev]

On 10/03, Mina Almasry wrote:
> On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > Only RX side for now and small message to test the setup.
> > In the future, we can extend it to TX side and to testing
> > both sides with a couple of megs of data.
> >
> 
> This is really awesome. Thank you.
> 
> >   make \
> >         -C tools/testing/selftests \
> >         TARGETS="drivers/hw/net" \
> >         install INSTALL_PATH=~/tmp/ksft
> >
> >   scp ~/tmp/ksft ${HOST}:
> >   scp ~/tmp/ksft ${PEER}:
> >
> >   cfg+="NETIF=${DEV}\n"
> >   cfg+="LOCAL_V6=${HOST_IP}\n"
> >   cfg+="REMOTE_V6=${PEER_IP}\n"
> 
> Not a review comment but noob question: does NIPA not support ipv4? Or
> is ipv6 preferred here?

Yes, absolutely, you can pass it LOCAL/REMOTE_V4 but you'll have to make
some changes to the selftest itself to use the v4 addresses. Things like
'-s {cfg.v6}' will have to be changed to '-s ::ffff:{cfg.v4}'.

I wonder whether it might be a good idea to have some new config method that
falls back to v4-mapped-v6 (::ffff:{cfg.v4}) to support both v4 and v6
transparently for the selftests that don't care about particular transport?

I'll leave it for you to explore...

> >   cfg+="REMOTE_TYPE=ssh\n"
> >   cfg+="REMOTE_ARGS=root@${PEER}\n"
> >
> >   echo -e "$cfg" | ssh root@${HOST} "cat > ksft/drivers/net/net.config"
> >   ssh root@${HOST} "cd ksft && ./run_kselftest.sh -t drivers/net:devmem.py"
> >
> > Cc: Mina Almasry <almasrymina@google.com>
> > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > ---
> >  .../testing/selftests/drivers/net/hw/Makefile |  1 +
> >  .../selftests/drivers/net/hw/devmem.py        | 46 +++++++++++++++++++
> >  2 files changed, 47 insertions(+)
> >  create mode 100755 tools/testing/selftests/drivers/net/hw/devmem.py
> >
> > diff --git a/tools/testing/selftests/drivers/net/hw/Makefile b/tools/testing/selftests/drivers/net/hw/Makefile
> > index 7bce46817953..a582b1bb3ae1 100644
> > --- a/tools/testing/selftests/drivers/net/hw/Makefile
> > +++ b/tools/testing/selftests/drivers/net/hw/Makefile
> > @@ -3,6 +3,7 @@
> >  TEST_PROGS = \
> >         csum.py \
> >         devlink_port_split.py \
> > +       devmem.py \
> >         ethtool.sh \
> >         ethtool_extended_state.sh \
> >         ethtool_mm.sh \
> > diff --git a/tools/testing/selftests/drivers/net/hw/devmem.py b/tools/testing/selftests/drivers/net/hw/devmem.py
> > new file mode 100755
> > index 000000000000..29085591616b
> > --- /dev/null
> > +++ b/tools/testing/selftests/drivers/net/hw/devmem.py
> > @@ -0,0 +1,46 @@
> > +#!/usr/bin/env python3
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +import errno
> > +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):
> > +    if not hasattr(cfg, "_devmem_probed"):
> > +        port = rand_port()
> > +        probe_command = f"./ncdevmem -f {cfg.ifname}"
> > +        cfg._devmem_supported = cmd(probe_command, fail=False, shell=True).ret == 0
> > +        cfg._devmem_probed = True
> > +
> > +    if not cfg._devmem_supported:
> > +        raise KsftSkipEx("Test requires devmem support")
> > +
> > +
> > +@ksft_disruptive
> > +def check_rx(cfg) -> None:
> > +    cfg.require_v6()
> > +    require_devmem(cfg)
> > +
> > +    port = rand_port()
> > +    listen_cmd = f"./ncdevmem -l -f {cfg.ifname} -s {cfg.v6} -p {port}"
> 
> So AFAICT adding validation to this test is simple. What you would do
> is change the above line to:
> 
> listen_cmd = f"./ncdevmem -l -f {cfg.ifname} -s {cfg.v6} -p {port} -v 7"
> 
> then, below...
> 
> > +
> > +    with bkg(listen_cmd) as nc:
> > +        wait_port_listen(port)
> > +        cmd(f"echo -e \"hello\\nworld\"| nc {cfg.v6} {port}", host=cfg.remote, shell=True)
> > +
> 
> ...change this to the equivalent of 'yes $(echo -e
> \\x01\\x02\\x03\\x04\\x05\\x06) | tr \\n \\0 | head -c 1G"
> 
> > +    ksft_eq(nc.stdout.strip(), "hello\nworld")
> > +
> 
> ...then remove this ksft_eq().
> 
> But this is just a suggestion, I think you were leaving this to future
> work for me, which is fine.
> 
> Reviewed-by: Mina Almasry <almasrymina@google.com>

Let me try. Worst case I leave it as is and you'll follow up with
the conversion once you get the TX side going..

Re: [PATCH net-next v2 02/12] selftests: ncdevmem: Separate out dmabuf provider

[Mina Almasry]

On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> So we can plug the other ones in the future if needed.
>
> Cc: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>

Feedback is minor, so with or without it:

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

> ---
>  tools/testing/selftests/net/ncdevmem.c | 215 +++++++++++++++----------
>  1 file changed, 134 insertions(+), 81 deletions(-)
>
> diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> index 9245d3f158dd..557175c3bf02 100644
> --- a/tools/testing/selftests/net/ncdevmem.c
> +++ b/tools/testing/selftests/net/ncdevmem.c
> @@ -71,17 +71,118 @@ static char *ifname = "eth1";
>  static unsigned int ifindex;
>  static unsigned int dmabuf_id;
>
> -void print_bytes(void *ptr, size_t size)
> +struct memory_buffer {
> +       int fd;
> +       size_t size;
> +
> +       int devfd;
> +       int memfd;
> +       char *buf_mem;
> +};
> +
> +struct memory_provider {
> +       struct memory_buffer *(*alloc)(size_t size);
> +       void (*free)(struct memory_buffer *ctx);
> +       void (*memcpy_to_device)(struct memory_buffer *dst, size_t off,
> +                                void *src, int n);

AFAICT all this code in this api and its implementation is unused. I'm
guessing this is for the future TX path testing?

If you get the chance to slice these changes out of this patch and
punting them for when the TX changes are ready, please do. Just to
minimize dead code until TX path is added, but since these are tests,
dead code is not a huge deal.

> +       void (*memcpy_from_device)(void *dst, struct memory_buffer *src,
> +                                  size_t off, int n);
> +};
> +
> +static struct memory_buffer *udmabuf_alloc(size_t size)
>  {
> -       unsigned char *p = ptr;
> -       int i;
> +       struct udmabuf_create create;
> +       struct memory_buffer *ctx;
> +       int ret;
>
> -       for (i = 0; i < size; i++)
> -               printf("%02hhX ", p[i]);
> -       printf("\n");
> +       ctx = malloc(sizeof(*ctx));
> +       if (!ctx)
> +               error(1, ENOMEM, "malloc failed");
> +
> +       ctx->size = size;
> +
> +       ctx->devfd = open("/dev/udmabuf", O_RDWR);
> +       if (ctx->devfd < 0)
> +               error(1, errno,
> +                     "%s: [skip,no-udmabuf: Unable to access DMA buffer device file]\n",
> +                     TEST_PREFIX);
> +
> +       ctx->memfd = memfd_create("udmabuf-test", MFD_ALLOW_SEALING);
> +       if (ctx->memfd < 0)
> +               error(1, errno, "%s: [skip,no-memfd]\n", TEST_PREFIX);
> +
> +       ret = fcntl(ctx->memfd, F_ADD_SEALS, F_SEAL_SHRINK);
> +       if (ret < 0)
> +               error(1, errno, "%s: [skip,fcntl-add-seals]\n", TEST_PREFIX);
> +
> +       ret = ftruncate(ctx->memfd, size);
> +       if (ret == -1)
> +               error(1, errno, "%s: [FAIL,memfd-truncate]\n", TEST_PREFIX);
> +
> +       memset(&create, 0, sizeof(create));
> +
> +       create.memfd = ctx->memfd;
> +       create.offset = 0;
> +       create.size = size;
> +       ctx->fd = ioctl(ctx->devfd, UDMABUF_CREATE, &create);
> +       if (ctx->fd < 0)
> +               error(1, errno, "%s: [FAIL, create udmabuf]\n", TEST_PREFIX);
> +
> +       ctx->buf_mem = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED,
> +                           ctx->fd, 0);
> +       if (ctx->buf_mem == MAP_FAILED)
> +               error(1, errno, "%s: [FAIL, map udmabuf]\n", TEST_PREFIX);
> +
> +       return ctx;
> +}
> +
> +static void udmabuf_free(struct memory_buffer *ctx)
> +{
> +       munmap(ctx->buf_mem, ctx->size);
> +       close(ctx->fd);
> +       close(ctx->memfd);
> +       close(ctx->devfd);
> +       free(ctx);
> +}
> +
> +static void udmabuf_memcpy_to_device(struct memory_buffer *dst, size_t off,
> +                                    void *src, int n)
> +{
> +       struct dma_buf_sync sync = {};
> +
> +       sync.flags = DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE;
> +       ioctl(dst->fd, DMA_BUF_IOCTL_SYNC, &sync);
> +
> +       memcpy(dst->buf_mem + off, src, n);
> +
> +       sync.flags = DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE;
> +       ioctl(dst->fd, DMA_BUF_IOCTL_SYNC, &sync);
> +}
> +
> +static void udmabuf_memcpy_from_device(void *dst, struct memory_buffer *src,
> +                                      size_t off, int n)
> +{
> +       struct dma_buf_sync sync = {};
> +
> +       sync.flags = DMA_BUF_SYNC_START;
> +       ioctl(src->fd, DMA_BUF_IOCTL_SYNC, &sync);
> +
> +       memcpy(dst, src->buf_mem + off, n);
> +
> +       sync.flags = DMA_BUF_SYNC_END;
> +       ioctl(src->fd, DMA_BUF_IOCTL_SYNC, &sync);
>  }
>
> -void print_nonzero_bytes(void *ptr, size_t size)
> +static struct memory_provider udmabuf_memory_provider = {
> +       .alloc = udmabuf_alloc,
> +       .free = udmabuf_free,
> +       .memcpy_to_device = udmabuf_memcpy_to_device,
> +       .memcpy_from_device = udmabuf_memcpy_from_device,
> +};
> +
> +static struct memory_provider *provider = &udmabuf_memory_provider;
> +
> +static void print_nonzero_bytes(void *ptr, size_t size)
>  {
>         unsigned char *p = ptr;
>         unsigned int i;
> @@ -201,42 +302,7 @@ static int bind_rx_queue(unsigned int ifindex, unsigned int dmabuf_fd,
>         return -1;
>  }
>
> -static void create_udmabuf(int *devfd, int *memfd, int *buf, size_t dmabuf_size)
> -{
> -       struct udmabuf_create create;
> -       int ret;
> -
> -       *devfd = open("/dev/udmabuf", O_RDWR);
> -       if (*devfd < 0) {
> -               error(70, 0,
> -                     "%s: [skip,no-udmabuf: Unable to access DMA buffer device file]\n",
> -                     TEST_PREFIX);
> -       }
> -
> -       *memfd = memfd_create("udmabuf-test", MFD_ALLOW_SEALING);
> -       if (*memfd < 0)
> -               error(70, 0, "%s: [skip,no-memfd]\n", TEST_PREFIX);
> -
> -       /* Required for udmabuf */
> -       ret = fcntl(*memfd, F_ADD_SEALS, F_SEAL_SHRINK);
> -       if (ret < 0)
> -               error(73, 0, "%s: [skip,fcntl-add-seals]\n", TEST_PREFIX);
> -
> -       ret = ftruncate(*memfd, dmabuf_size);
> -       if (ret == -1)
> -               error(74, 0, "%s: [FAIL,memfd-truncate]\n", TEST_PREFIX);
> -
> -       memset(&create, 0, sizeof(create));
> -
> -       create.memfd = *memfd;
> -       create.offset = 0;
> -       create.size = dmabuf_size;
> -       *buf = ioctl(*devfd, UDMABUF_CREATE, &create);
> -       if (*buf < 0)
> -               error(75, 0, "%s: [FAIL, create udmabuf]\n", TEST_PREFIX);
> -}
> -
> -int do_server(void)
> +int do_server(struct memory_buffer *mem)
>  {
>         char ctrl_data[sizeof(int) * 20000];
>         struct netdev_queue_id *queues;
> @@ -244,23 +310,18 @@ int do_server(void)
>         struct sockaddr_in client_addr;
>         struct sockaddr_in server_sin;
>         size_t page_aligned_frags = 0;
> -       int devfd, memfd, buf, ret;
>         size_t total_received = 0;
>         socklen_t client_addr_len;
>         bool is_devmem = false;
> -       char *buf_mem = NULL;
> +       char *tmp_mem = NULL;
>         struct ynl_sock *ys;
> -       size_t dmabuf_size;
>         char iobuf[819200];
>         char buffer[256];
>         int socket_fd;
>         int client_fd;
>         size_t i = 0;
>         int opt = 1;
> -
> -       dmabuf_size = getpagesize() * NUM_PAGES;
> -
> -       create_udmabuf(&devfd, &memfd, &buf, dmabuf_size);
> +       int ret;
>
>         if (reset_flow_steering())
>                 error(1, 0, "Failed to reset flow steering\n");
> @@ -284,13 +345,12 @@ int do_server(void)
>                 queues[i].id = start_queue + i;
>         }
>
> -       if (bind_rx_queue(ifindex, buf, queues, num_queues, &ys))
> +       if (bind_rx_queue(ifindex, mem->fd, queues, num_queues, &ys))
>                 error(1, 0, "Failed to bind\n");
>
> -       buf_mem = mmap(NULL, dmabuf_size, PROT_READ | PROT_WRITE, MAP_SHARED,
> -                      buf, 0);
> -       if (buf_mem == MAP_FAILED)
> -               error(1, 0, "mmap()");
> +       tmp_mem = malloc(mem->size);
> +       if (!tmp_mem)
> +               error(1, ENOMEM, "malloc failed");
>
>         server_sin.sin_family = AF_INET;
>         server_sin.sin_port = htons(atoi(port));
> @@ -341,7 +401,6 @@ int do_server(void)
>                 struct iovec iov = { .iov_base = iobuf,
>                                      .iov_len = sizeof(iobuf) };
>                 struct dmabuf_cmsg *dmabuf_cmsg = NULL;
> -               struct dma_buf_sync sync = { 0 };
>                 struct cmsghdr *cm = NULL;
>                 struct msghdr msg = { 0 };
>                 struct dmabuf_token token;
> @@ -410,22 +469,17 @@ int do_server(void)
>                         else
>                                 page_aligned_frags++;
>
> -                       sync.flags = DMA_BUF_SYNC_READ | DMA_BUF_SYNC_START;
> -                       ioctl(buf, DMA_BUF_IOCTL_SYNC, &sync);
> +                       provider->memcpy_from_device(tmp_mem, mem,
> +                                                    dmabuf_cmsg->frag_offset,
> +                                                    dmabuf_cmsg->frag_size);
>
>                         if (do_validation)
>                                 validate_buffer(
> -                                       ((unsigned char *)buf_mem) +
> +                                       ((unsigned char *)tmp_mem) +
>                                                 dmabuf_cmsg->frag_offset,
>                                         dmabuf_cmsg->frag_size);
>                         else
> -                               print_nonzero_bytes(
> -                                       ((unsigned char *)buf_mem) +
> -                                               dmabuf_cmsg->frag_offset,
> -                                       dmabuf_cmsg->frag_size);
> -
> -                       sync.flags = DMA_BUF_SYNC_READ | DMA_BUF_SYNC_END;
> -                       ioctl(buf, DMA_BUF_IOCTL_SYNC, &sync);
> +                               print_nonzero_bytes(tmp_mem, dmabuf_cmsg->frag_size);
>
>                         ret = setsockopt(client_fd, SOL_SOCKET,
>                                          SO_DEVMEM_DONTNEED, &token,
> @@ -450,12 +504,9 @@ int do_server(void)
>
>  cleanup:
>
> -       munmap(buf_mem, dmabuf_size);
> +       free(tmp_mem);
>         close(client_fd);
>         close(socket_fd);
> -       close(buf);
> -       close(memfd);
> -       close(devfd);
>         ynl_sock_destroy(ys);
>
>         return 0;
> @@ -464,14 +515,11 @@ int do_server(void)
>  void run_devmem_tests(void)
>  {
>         struct netdev_queue_id *queues;
> -       int devfd, memfd, buf;
> +       struct memory_buffer *mem;
>         struct ynl_sock *ys;
> -       size_t dmabuf_size;
>         size_t i = 0;
>
> -       dmabuf_size = getpagesize() * NUM_PAGES;
> -
> -       create_udmabuf(&devfd, &memfd, &buf, dmabuf_size);
> +       mem = provider->alloc(getpagesize() * NUM_PAGES);
>

There is some weirdness here where run_devmem_tests() allocates its
own provider, but do_server() uses a provider allocated in main(). Any
reason these are not symmetric? I would marginally prefer do_server()
to allocate its own provider just like run_devmem_tests(), or at least
make them both match, if possible.


-- 
Thanks,
Mina

Re: [PATCH net-next v2 05/12] selftests: ncdevmem: Remove default arguments

[Mina Almasry]

On Thu, Oct 3, 2024 at 9:36 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 10/02, Mina Almasry wrote:
> > On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > >
> > > To make it clear what's required and what's not. Also, some of the
> > > values don't seem like a good defaults; for example eth1.
> > >
> > > Cc: Mina Almasry <almasrymina@google.com>
> > > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > > ---
> > >  tools/testing/selftests/net/ncdevmem.c | 34 +++++++++-----------------
> > >  1 file changed, 12 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> > > index 699692fdfd7d..bf446d74a4f0 100644
> > > --- a/tools/testing/selftests/net/ncdevmem.c
> > > +++ b/tools/testing/selftests/net/ncdevmem.c
> > > @@ -42,32 +42,13 @@
> > >  #define MSG_SOCK_DEVMEM 0x2000000
> > >  #endif
> > >
> > > -/*
> > > - * tcpdevmem netcat. Works similarly to netcat but does device memory TCP
> > > - * instead of regular TCP. Uses udmabuf to mock a dmabuf provider.
> > > - *
> > > - * Usage:
> > > - *
> > > - *     On server:
> > > - *     ncdevmem -s <server IP> -c <client IP> -f eth1 -l -p 5201 -v 7
> > > - *
> >
> > No need to remove this documentation I think. This is useful since we
> > don't have a proper docs anywhere.
> >
> > Please instead update the args in the above line, if they need
> > updating, but looks like it's already correct even after this change.
>
> The client needs '-s' part. That's why I removed it - most likely
> will tend to go stale and we now have the invocation example in the
> selftest. But if you want to keep it, how about I move it to the
> top of the file and cleanup a bit? Will do for the next iteration..

Yeah, the 'docs' will need to be updated for the TX path, but I hope,
not removed. We don't really have any other clues on how to run this
thing. The docs will become less important when the kselftest is
properly up and running because it is self documenting, but just in
case anyone wants to run ncdevmem manually the docs are nice. Any
cleanup and movement for clarity is welcome indeed.

-- 
Thanks,
Mina

Re: [PATCH net-next v2 07/12] selftests: ncdevmem: Properly reset flow steering

[Mina Almasry]

On Thu, Oct 3, 2024 at 9:42 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 10/03, Mina Almasry wrote:
> > On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > >
> > > ntuple off/on might be not enough to do it on all NICs.
> > > Add a bunch of shell crap to explicitly remove the rules.
> > >
> > > Cc: Mina Almasry <almasrymina@google.com>
> > > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > > ---
> > >  tools/testing/selftests/net/ncdevmem.c | 13 ++++++-------
> > >  1 file changed, 6 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> > > index 47458a13eff5..48cbf057fde7 100644
> > > --- a/tools/testing/selftests/net/ncdevmem.c
> > > +++ b/tools/testing/selftests/net/ncdevmem.c
> > > @@ -207,13 +207,12 @@ void validate_buffer(void *line, size_t size)
> > >
> > >  static int reset_flow_steering(void)
> > >  {
> > > -       int ret = 0;
> > > -
> > > -       ret = run_command("sudo ethtool -K %s ntuple off >&2", ifname);
> > > -       if (ret)
> > > -               return ret;
> > > -
> > > -       return run_command("sudo ethtool -K %s ntuple on >&2", ifname);
> > > +       run_command("sudo ethtool -K %s ntuple off >&2", ifname);
> > > +       run_command("sudo ethtool -K %s ntuple on >&2", ifname);
> > > +       run_command(
> > > +               "sudo ethtool -n %s | grep 'Filter:' | awk '{print $2}' | xargs -n1 ethtool -N %s delete >&2",
> > > +               ifname, ifname);
> > > +       return 0;
> >
> > Any reason to remove the checking of the return codes? Silent failures
> > can waste time if the test fails and someone has to spend time finding
> > out its the flow steering reset that failed (it may not be very
> > obvious without the checking of the return code.
>
> IIRC, for me the 'ntuple off' part fails because the NIC doesn't let me
> turn it of. And the new "ethtool .. | grep 'Filter: ' | ..." also fails
> when there are no existing filters.
>
> I will add a comment to clarify..

Ah, understood. Seems this area is fraught with subtleties.

If you have time, maybe to counter these subtleties we can do a get of
ntuple filters and confirm they're 0 somehow at the end of the
function. That would offset not checking the return code.

But, I don't think it's extremely likely to run into errors here? So,
this is probably good and can easily be improved later if we run into
issues:

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

-- 
Thanks,
Mina

Re: [PATCH net-next v2 09/12] selftests: ncdevmem: Remove hard-coded queue numbers

[Mina Almasry]

On Thu, Oct 3, 2024 at 10:02 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 10/03, Mina Almasry wrote:
> > On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > >
> > > Use single last queue of the device and probe it dynamically.
> > >
> >
> > Sorry I know there was a pending discussion in the last iteration that
> > I didn't respond to. Been a rough week with me out sick a bit.
> >
> > For this, the issue I see is that by default only 1 queue binding will
> > be tested, but I feel like test coverage for the multiple queues case
> > by default is very nice because I actually ran into some issues making
> > multi-queue binding work.
> >
> > Can we change this so that, by default, it binds to the last rxq_num/2
> > queues of the device?
>
> I'm probably missing something, but why do you think exercising this from
> the probe/selftest mode is not enough? It might be confusing for the readers
> to understand why we bind to half of the queues and flow steer into them
> when in reality there is only single tcp flow.
>
> IOW, can we keep these two modes:
> 1. server / client - use single queue
> 2. selftest / probe - use more than 1 queue by default (and I'll remove the
>    checks that enforce the number of queues for this mode to let the
>    users override)

Ah, I see. Thanks for the explanation.

My paranoia here is that we don't notice multi-queue binding
regressions because the tests are often run in data path mode and we
don't use or notice failures in the probe mode.

I will concede my paranoia is just that and this is not very likely to
happen, but also if it is confusing to bind multi-queues and then just
use one, then we could remedy that with a comment and keep the
accidental test coverage. It also makes the test simpler to always
bind the same # of queues rather than special case data and control
path tests.

But your 2 mode approach sounds fine as well. But to implement that
you need more than to remove the checks that enforce the number of
queues, right? In probe mode num_queues should be rxq_num/2, and in
server mode num_queues should be 1, yes?

-- 
Thanks,
Mina

Re: [PATCH net-next v2 10/12] selftests: ncdevmem: Run selftest when none of the -s or -c has been provided

[Mina Almasry]

On Thu, Oct 3, 2024 at 10:18 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 10/03, Mina Almasry wrote:
> > On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > >
> > > This will be used as a 'probe' mode in the selftest to check whether
> > > the device supports the devmem or not.
> >
> > It's not really 'probing'. Running ncdevmem with -s or -c does the
> > data path tests. Running ncdevmem without does all the control path
> > tests in run_devmem_tests(). It's not just probing driver for support,
> > it's mean to catch bugs. Maybe rename to 'control path tests' or
> > 'config tests' instead of probing.
>
> Re 'probing' vs 'control path tests': I need something I can call
> in the python selftest to tell me whether the nic supports devmem
> or not (to skip the tests on the unsupported nics), so I'm reusing this
> 'control path tests' mode for this. I do agree that there might be an
> issue where the nic supports devmem, but has some bug which causes
> 'control path tests' to fail which leads to skipping the data plane tests...
>
> We can try to separate these two in the future. (and I'll keep the word
> 'probing' for now since it's only in the commit message to describe the
> intent)

Ah, got it. Thanks for the explanation.

Complete probing should call the binding API and assert that the
return is EOPNOTSUPP or something like that. Other errors in the
control path tests should be considered failures and not test skips.
But probing is necessary to run this test and this is a good way to do
it for now. We can improve later if necessary. Thanks!

-- 
Thanks,
Mina

Re: [PATCH net-next v2 11/12] selftests: ncdevmem: Move ncdevmem under drivers/net/hw

[Mina Almasry]

On Thu, Oct 3, 2024 at 10:26 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 10/03, Mina Almasry wrote:
> > On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > >
> > > This is where all the tests that depend on the HW functionality live in
> > > and this is where the automated test is gonna be added in the next
> > > patch.
> > >
> >
> > Tbh I don't like this very much. I wanted to take ncdevmem in the
> > opposite direction: to make at least the control path tests runnable
> > on netdevsim or something like that and have it not require any HW
> > support at all.
> >
> > But I see in the cover letter that Jakub himself asked for the move,
> > so if there is some strong reason to make this in hw, sure.
> >
> > Does it being under HW preclude future improvements to making it a
> > non-HW dependent test?
>
> I'm moving it under drivers/net/hw only because I want ncdevmem to end
> up as a TEST_GEN_FILES dependency (drivers/net/hw is the directory
> that the vendors will eventually run against their HW so this is
> where the HW-dependent tests are gonna stay for now).

Ah, OK. Makes sense then.

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

Thanks!

-- 
Thanks,
Mina

Re: [PATCH net-next v2 02/12] selftests: ncdevmem: Separate out dmabuf provider

[Stanislav Fomichev]

On 10/03, Mina Almasry wrote:
> On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > So we can plug the other ones in the future if needed.
> >
> > Cc: Mina Almasry <almasrymina@google.com>
> > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> 
> Feedback is minor, so with or without it:
> 
> Reviewed-by: Mina Almasry <almasrymina@google.com>
> 
> > ---
> >  tools/testing/selftests/net/ncdevmem.c | 215 +++++++++++++++----------
> >  1 file changed, 134 insertions(+), 81 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> > index 9245d3f158dd..557175c3bf02 100644
> > --- a/tools/testing/selftests/net/ncdevmem.c
> > +++ b/tools/testing/selftests/net/ncdevmem.c
> > @@ -71,17 +71,118 @@ static char *ifname = "eth1";
> >  static unsigned int ifindex;
> >  static unsigned int dmabuf_id;
> >
> > -void print_bytes(void *ptr, size_t size)
> > +struct memory_buffer {
> > +       int fd;
> > +       size_t size;
> > +
> > +       int devfd;
> > +       int memfd;
> > +       char *buf_mem;
> > +};
> > +
> > +struct memory_provider {
> > +       struct memory_buffer *(*alloc)(size_t size);
> > +       void (*free)(struct memory_buffer *ctx);
> > +       void (*memcpy_to_device)(struct memory_buffer *dst, size_t off,
> > +                                void *src, int n);
> 
> AFAICT all this code in this api and its implementation is unused. I'm
> guessing this is for the future TX path testing?
> 
> If you get the chance to slice these changes out of this patch and
> punting them for when the TX changes are ready, please do. Just to
> minimize dead code until TX path is added, but since these are tests,
> dead code is not a huge deal.

Ah, yes, I'll remove it for now.

> > +       void (*memcpy_from_device)(void *dst, struct memory_buffer *src,
> > +                                  size_t off, int n);
> > +};
> > +
> > +static struct memory_buffer *udmabuf_alloc(size_t size)
> >  {
> > -       unsigned char *p = ptr;
> > -       int i;
> > +       struct udmabuf_create create;
> > +       struct memory_buffer *ctx;
> > +       int ret;
> >
> > -       for (i = 0; i < size; i++)
> > -               printf("%02hhX ", p[i]);
> > -       printf("\n");
> > +       ctx = malloc(sizeof(*ctx));
> > +       if (!ctx)
> > +               error(1, ENOMEM, "malloc failed");
> > +
> > +       ctx->size = size;
> > +
> > +       ctx->devfd = open("/dev/udmabuf", O_RDWR);
> > +       if (ctx->devfd < 0)
> > +               error(1, errno,
> > +                     "%s: [skip,no-udmabuf: Unable to access DMA buffer device file]\n",
> > +                     TEST_PREFIX);
> > +
> > +       ctx->memfd = memfd_create("udmabuf-test", MFD_ALLOW_SEALING);
> > +       if (ctx->memfd < 0)
> > +               error(1, errno, "%s: [skip,no-memfd]\n", TEST_PREFIX);
> > +
> > +       ret = fcntl(ctx->memfd, F_ADD_SEALS, F_SEAL_SHRINK);
> > +       if (ret < 0)
> > +               error(1, errno, "%s: [skip,fcntl-add-seals]\n", TEST_PREFIX);
> > +
> > +       ret = ftruncate(ctx->memfd, size);
> > +       if (ret == -1)
> > +               error(1, errno, "%s: [FAIL,memfd-truncate]\n", TEST_PREFIX);
> > +
> > +       memset(&create, 0, sizeof(create));
> > +
> > +       create.memfd = ctx->memfd;
> > +       create.offset = 0;
> > +       create.size = size;
> > +       ctx->fd = ioctl(ctx->devfd, UDMABUF_CREATE, &create);
> > +       if (ctx->fd < 0)
> > +               error(1, errno, "%s: [FAIL, create udmabuf]\n", TEST_PREFIX);
> > +
> > +       ctx->buf_mem = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED,
> > +                           ctx->fd, 0);
> > +       if (ctx->buf_mem == MAP_FAILED)
> > +               error(1, errno, "%s: [FAIL, map udmabuf]\n", TEST_PREFIX);
> > +
> > +       return ctx;
> > +}
> > +
> > +static void udmabuf_free(struct memory_buffer *ctx)
> > +{
> > +       munmap(ctx->buf_mem, ctx->size);
> > +       close(ctx->fd);
> > +       close(ctx->memfd);
> > +       close(ctx->devfd);
> > +       free(ctx);
> > +}
> > +
> > +static void udmabuf_memcpy_to_device(struct memory_buffer *dst, size_t off,
> > +                                    void *src, int n)
> > +{
> > +       struct dma_buf_sync sync = {};
> > +
> > +       sync.flags = DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE;
> > +       ioctl(dst->fd, DMA_BUF_IOCTL_SYNC, &sync);
> > +
> > +       memcpy(dst->buf_mem + off, src, n);
> > +
> > +       sync.flags = DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE;
> > +       ioctl(dst->fd, DMA_BUF_IOCTL_SYNC, &sync);
> > +}
> > +
> > +static void udmabuf_memcpy_from_device(void *dst, struct memory_buffer *src,
> > +                                      size_t off, int n)
> > +{
> > +       struct dma_buf_sync sync = {};
> > +
> > +       sync.flags = DMA_BUF_SYNC_START;
> > +       ioctl(src->fd, DMA_BUF_IOCTL_SYNC, &sync);
> > +
> > +       memcpy(dst, src->buf_mem + off, n);
> > +
> > +       sync.flags = DMA_BUF_SYNC_END;
> > +       ioctl(src->fd, DMA_BUF_IOCTL_SYNC, &sync);
> >  }
> >
> > -void print_nonzero_bytes(void *ptr, size_t size)
> > +static struct memory_provider udmabuf_memory_provider = {
> > +       .alloc = udmabuf_alloc,
> > +       .free = udmabuf_free,
> > +       .memcpy_to_device = udmabuf_memcpy_to_device,
> > +       .memcpy_from_device = udmabuf_memcpy_from_device,
> > +};
> > +
> > +static struct memory_provider *provider = &udmabuf_memory_provider;
> > +
> > +static void print_nonzero_bytes(void *ptr, size_t size)
> >  {
> >         unsigned char *p = ptr;
> >         unsigned int i;
> > @@ -201,42 +302,7 @@ static int bind_rx_queue(unsigned int ifindex, unsigned int dmabuf_fd,
> >         return -1;
> >  }
> >
> > -static void create_udmabuf(int *devfd, int *memfd, int *buf, size_t dmabuf_size)
> > -{
> > -       struct udmabuf_create create;
> > -       int ret;
> > -
> > -       *devfd = open("/dev/udmabuf", O_RDWR);
> > -       if (*devfd < 0) {
> > -               error(70, 0,
> > -                     "%s: [skip,no-udmabuf: Unable to access DMA buffer device file]\n",
> > -                     TEST_PREFIX);
> > -       }
> > -
> > -       *memfd = memfd_create("udmabuf-test", MFD_ALLOW_SEALING);
> > -       if (*memfd < 0)
> > -               error(70, 0, "%s: [skip,no-memfd]\n", TEST_PREFIX);
> > -
> > -       /* Required for udmabuf */
> > -       ret = fcntl(*memfd, F_ADD_SEALS, F_SEAL_SHRINK);
> > -       if (ret < 0)
> > -               error(73, 0, "%s: [skip,fcntl-add-seals]\n", TEST_PREFIX);
> > -
> > -       ret = ftruncate(*memfd, dmabuf_size);
> > -       if (ret == -1)
> > -               error(74, 0, "%s: [FAIL,memfd-truncate]\n", TEST_PREFIX);
> > -
> > -       memset(&create, 0, sizeof(create));
> > -
> > -       create.memfd = *memfd;
> > -       create.offset = 0;
> > -       create.size = dmabuf_size;
> > -       *buf = ioctl(*devfd, UDMABUF_CREATE, &create);
> > -       if (*buf < 0)
> > -               error(75, 0, "%s: [FAIL, create udmabuf]\n", TEST_PREFIX);
> > -}
> > -
> > -int do_server(void)
> > +int do_server(struct memory_buffer *mem)
> >  {
> >         char ctrl_data[sizeof(int) * 20000];
> >         struct netdev_queue_id *queues;
> > @@ -244,23 +310,18 @@ int do_server(void)
> >         struct sockaddr_in client_addr;
> >         struct sockaddr_in server_sin;
> >         size_t page_aligned_frags = 0;
> > -       int devfd, memfd, buf, ret;
> >         size_t total_received = 0;
> >         socklen_t client_addr_len;
> >         bool is_devmem = false;
> > -       char *buf_mem = NULL;
> > +       char *tmp_mem = NULL;
> >         struct ynl_sock *ys;
> > -       size_t dmabuf_size;
> >         char iobuf[819200];
> >         char buffer[256];
> >         int socket_fd;
> >         int client_fd;
> >         size_t i = 0;
> >         int opt = 1;
> > -
> > -       dmabuf_size = getpagesize() * NUM_PAGES;
> > -
> > -       create_udmabuf(&devfd, &memfd, &buf, dmabuf_size);
> > +       int ret;
> >
> >         if (reset_flow_steering())
> >                 error(1, 0, "Failed to reset flow steering\n");
> > @@ -284,13 +345,12 @@ int do_server(void)
> >                 queues[i].id = start_queue + i;
> >         }
> >
> > -       if (bind_rx_queue(ifindex, buf, queues, num_queues, &ys))
> > +       if (bind_rx_queue(ifindex, mem->fd, queues, num_queues, &ys))
> >                 error(1, 0, "Failed to bind\n");
> >
> > -       buf_mem = mmap(NULL, dmabuf_size, PROT_READ | PROT_WRITE, MAP_SHARED,
> > -                      buf, 0);
> > -       if (buf_mem == MAP_FAILED)
> > -               error(1, 0, "mmap()");
> > +       tmp_mem = malloc(mem->size);
> > +       if (!tmp_mem)
> > +               error(1, ENOMEM, "malloc failed");
> >
> >         server_sin.sin_family = AF_INET;
> >         server_sin.sin_port = htons(atoi(port));
> > @@ -341,7 +401,6 @@ int do_server(void)
> >                 struct iovec iov = { .iov_base = iobuf,
> >                                      .iov_len = sizeof(iobuf) };
> >                 struct dmabuf_cmsg *dmabuf_cmsg = NULL;
> > -               struct dma_buf_sync sync = { 0 };
> >                 struct cmsghdr *cm = NULL;
> >                 struct msghdr msg = { 0 };
> >                 struct dmabuf_token token;
> > @@ -410,22 +469,17 @@ int do_server(void)
> >                         else
> >                                 page_aligned_frags++;
> >
> > -                       sync.flags = DMA_BUF_SYNC_READ | DMA_BUF_SYNC_START;
> > -                       ioctl(buf, DMA_BUF_IOCTL_SYNC, &sync);
> > +                       provider->memcpy_from_device(tmp_mem, mem,
> > +                                                    dmabuf_cmsg->frag_offset,
> > +                                                    dmabuf_cmsg->frag_size);
> >
> >                         if (do_validation)
> >                                 validate_buffer(
> > -                                       ((unsigned char *)buf_mem) +
> > +                                       ((unsigned char *)tmp_mem) +
> >                                                 dmabuf_cmsg->frag_offset,
> >                                         dmabuf_cmsg->frag_size);
> >                         else
> > -                               print_nonzero_bytes(
> > -                                       ((unsigned char *)buf_mem) +
> > -                                               dmabuf_cmsg->frag_offset,
> > -                                       dmabuf_cmsg->frag_size);
> > -
> > -                       sync.flags = DMA_BUF_SYNC_READ | DMA_BUF_SYNC_END;
> > -                       ioctl(buf, DMA_BUF_IOCTL_SYNC, &sync);
> > +                               print_nonzero_bytes(tmp_mem, dmabuf_cmsg->frag_size);
> >
> >                         ret = setsockopt(client_fd, SOL_SOCKET,
> >                                          SO_DEVMEM_DONTNEED, &token,
> > @@ -450,12 +504,9 @@ int do_server(void)
> >
> >  cleanup:
> >
> > -       munmap(buf_mem, dmabuf_size);
> > +       free(tmp_mem);
> >         close(client_fd);
> >         close(socket_fd);
> > -       close(buf);
> > -       close(memfd);
> > -       close(devfd);
> >         ynl_sock_destroy(ys);
> >
> >         return 0;
> > @@ -464,14 +515,11 @@ int do_server(void)
> >  void run_devmem_tests(void)
> >  {
> >         struct netdev_queue_id *queues;
> > -       int devfd, memfd, buf;
> > +       struct memory_buffer *mem;
> >         struct ynl_sock *ys;
> > -       size_t dmabuf_size;
> >         size_t i = 0;
> >
> > -       dmabuf_size = getpagesize() * NUM_PAGES;
> > -
> > -       create_udmabuf(&devfd, &memfd, &buf, dmabuf_size);
> > +       mem = provider->alloc(getpagesize() * NUM_PAGES);
> >
> 
> There is some weirdness here where run_devmem_tests() allocates its
> own provider, but do_server() uses a provider allocated in main(). Any
> reason these are not symmetric? I would marginally prefer do_server()
> to allocate its own provider just like run_devmem_tests(), or at least
> make them both match, if possible.

I wanted to keep them separate in case we end up adding more to
the selftest part. For example, not sure what would happen now if we pass
a udmabuf with just one page? Do we need some test for the drivers
to make sure they handle this case?

Re: [PATCH net-next v2 07/12] selftests: ncdevmem: Properly reset flow steering

[Stanislav Fomichev]

On 10/03, Mina Almasry wrote:
> On Thu, Oct 3, 2024 at 9:42 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
> >
> > On 10/03, Mina Almasry wrote:
> > > On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > > >
> > > > ntuple off/on might be not enough to do it on all NICs.
> > > > Add a bunch of shell crap to explicitly remove the rules.
> > > >
> > > > Cc: Mina Almasry <almasrymina@google.com>
> > > > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > > > ---
> > > >  tools/testing/selftests/net/ncdevmem.c | 13 ++++++-------
> > > >  1 file changed, 6 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> > > > index 47458a13eff5..48cbf057fde7 100644
> > > > --- a/tools/testing/selftests/net/ncdevmem.c
> > > > +++ b/tools/testing/selftests/net/ncdevmem.c
> > > > @@ -207,13 +207,12 @@ void validate_buffer(void *line, size_t size)
> > > >
> > > >  static int reset_flow_steering(void)
> > > >  {
> > > > -       int ret = 0;
> > > > -
> > > > -       ret = run_command("sudo ethtool -K %s ntuple off >&2", ifname);
> > > > -       if (ret)
> > > > -               return ret;
> > > > -
> > > > -       return run_command("sudo ethtool -K %s ntuple on >&2", ifname);
> > > > +       run_command("sudo ethtool -K %s ntuple off >&2", ifname);
> > > > +       run_command("sudo ethtool -K %s ntuple on >&2", ifname);
> > > > +       run_command(
> > > > +               "sudo ethtool -n %s | grep 'Filter:' | awk '{print $2}' | xargs -n1 ethtool -N %s delete >&2",
> > > > +               ifname, ifname);
> > > > +       return 0;
> > >
> > > Any reason to remove the checking of the return codes? Silent failures
> > > can waste time if the test fails and someone has to spend time finding
> > > out its the flow steering reset that failed (it may not be very
> > > obvious without the checking of the return code.
> >
> > IIRC, for me the 'ntuple off' part fails because the NIC doesn't let me
> > turn it of. And the new "ethtool .. | grep 'Filter: ' | ..." also fails
> > when there are no existing filters.
> >
> > I will add a comment to clarify..
> 
> Ah, understood. Seems this area is fraught with subtleties.
> 
> If you have time, maybe to counter these subtleties we can do a get of
> ntuple filters and confirm they're 0 somehow at the end of the
> function. That would offset not checking the return code.
> 
> But, I don't think it's extremely likely to run into errors here? So,
> this is probably good and can easily be improved later if we run into
> issues:
> 
> Reviewed-by: Mina Almasry <almasrymina@google.com>

Ack, I'll keep it as is with a comment. Ideally we should do proper
ethtool netlink/ioctl instead of shelling out, but I don' think
ntuple API is exposed to netlink and I'm too lazy to dive into how
the old ioctl-based ntuple API works :-D

Re: [PATCH net-next v2 09/12] selftests: ncdevmem: Remove hard-coded queue numbers

[Stanislav Fomichev]

On 10/03, Mina Almasry wrote:
> On Thu, Oct 3, 2024 at 10:02 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
> >
> > On 10/03, Mina Almasry wrote:
> > > On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > > >
> > > > Use single last queue of the device and probe it dynamically.
> > > >
> > >
> > > Sorry I know there was a pending discussion in the last iteration that
> > > I didn't respond to. Been a rough week with me out sick a bit.
> > >
> > > For this, the issue I see is that by default only 1 queue binding will
> > > be tested, but I feel like test coverage for the multiple queues case
> > > by default is very nice because I actually ran into some issues making
> > > multi-queue binding work.
> > >
> > > Can we change this so that, by default, it binds to the last rxq_num/2
> > > queues of the device?
> >
> > I'm probably missing something, but why do you think exercising this from
> > the probe/selftest mode is not enough? It might be confusing for the readers
> > to understand why we bind to half of the queues and flow steer into them
> > when in reality there is only single tcp flow.
> >
> > IOW, can we keep these two modes:
> > 1. server / client - use single queue
> > 2. selftest / probe - use more than 1 queue by default (and I'll remove the
> >    checks that enforce the number of queues for this mode to let the
> >    users override)
> 
> Ah, I see. Thanks for the explanation.
> 
> My paranoia here is that we don't notice multi-queue binding
> regressions because the tests are often run in data path mode and we
> don't use or notice failures in the probe mode.
> 
> I will concede my paranoia is just that and this is not very likely to
> happen, but also if it is confusing to bind multi-queues and then just
> use one, then we could remedy that with a comment and keep the
> accidental test coverage. It also makes the test simpler to always
> bind the same # of queues rather than special case data and control
> path tests.
> 
> But your 2 mode approach sounds fine as well. But to implement that
> you need more than to remove the checks that enforce the number of
> queues, right? In probe mode num_queues should be rxq_num/2, and in
> server mode num_queues should be 1, yes?

Yes, I'll follow your suggestion with `start_queues = num_queues / 2`
for the selftest part.

Tentatively (default to 1/2 queues, if want to override - provide both
-t an -q):

index 90aacfb3433f..3a456c058241 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -64,7 +64,7 @@ static char *client_ip;
 static char *port;
 static size_t do_validation;
 static int start_queue = -1;
-static int num_queues = 1;
+static int num_queues = -1;
 static char *ifname;
 static unsigned int ifindex;
 static unsigned int dmabuf_id;
@@ -706,19 +706,31 @@ int main(int argc, char *argv[])
                }
        }

-       if (!server_ip)
-               error(1, 0, "Missing -s argument\n");
-
-       if (!port)
-               error(1, 0, "Missing -p argument\n");
-
        if (!ifname)
                error(1, 0, "Missing -f argument\n");

        ifindex = if_nametoindex(ifname);

-       if (start_queue < 0) {
-               start_queue = rxq_num(ifindex) - 1;
+       if (!server_ip && !client_ip) {
+               if (start_queue < 0 && num_queues < 0) {
+                       num_queues = rxq_num(ifindex);
+                       if (num_queues < 0)
+                               error(1, 0, "couldn't detect number of queues\n");
+                       /* make sure can bind to multiple queues */
+                       start_queues = num_queues / 2;
+                       num_queues /= 2;
+               }
+
+               if (start_queue < 0 || num_queues < 0)
+                       error(1, 0, "Both -t and -q are requred\n");
+
+               run_devmem_tests();
+               return 0;
+       }
+
+       if (start_queue < 0 && num_queues < 0) {
+               num_queues = 1;
+               start_queue = rxq_num(ifindex) - num_queues;

Re: [PATCH net-next v2 02/12] selftests: ncdevmem: Separate out dmabuf provider

[Mina Almasry]

On Thu, Oct 3, 2024 at 3:08 PM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> > > @@ -464,14 +515,11 @@ int do_server(void)
> > >  void run_devmem_tests(void)
> > >  {
> > >         struct netdev_queue_id *queues;
> > > -       int devfd, memfd, buf;
> > > +       struct memory_buffer *mem;
> > >         struct ynl_sock *ys;
> > > -       size_t dmabuf_size;
> > >         size_t i = 0;
> > >
> > > -       dmabuf_size = getpagesize() * NUM_PAGES;
> > > -
> > > -       create_udmabuf(&devfd, &memfd, &buf, dmabuf_size);
> > > +       mem = provider->alloc(getpagesize() * NUM_PAGES);
> > >
> >
> > There is some weirdness here where run_devmem_tests() allocates its
> > own provider, but do_server() uses a provider allocated in main(). Any
> > reason these are not symmetric? I would marginally prefer do_server()
> > to allocate its own provider just like run_devmem_tests(), or at least
> > make them both match, if possible.
>
> I wanted to keep them separate in case we end up adding more to
> the selftest part. For example, not sure what would happen now if we pass
> a udmabuf with just one page? Do we need some test for the drivers
> to make sure they handle this case?

The size of the udmabuf (or more generically, any dmabuf) is the
address space for the page pool; if the dmabuf is too small, like 1
page, the rest of the pp allocations will return -ENOMEM.

-- 
Thanks,
Mina