[RFC PATCH v1 0/3] test/vsock: update two tests and add new tool

[RFC PATCH v1 0/3] test/vsock: update two tests and add new tool

[Arseniy Krasnov]

Since there is work on several significant updates for vsock(virtio/
vsock especially): skbuff, DGRAM, zerocopy rx/tx, so I think that this
patchset will be useful.

This patchset updates vsock tests and tools a little bit. First of all
it updates test suite: two new tests are added. One test is reworked
message bound test. Now it is more complex. Instead of sending 1 byte
messages with one MSG_EOR bit, it sends messages of random length(one
half of messages are smaller than page size, second half are bigger)
with random number of MSG_EOR bits set. Receiver also don't know total
number of messages. Message bounds control is maintained by hash sum
of messages length calculation. Second test is for SOCK_SEQPACKET - it
tries to send message with length more than allowed. I think both tests
will be useful for DGRAM support also.

Third thing that this patchset adds is small utility to test vsock
performance for both rx and tx. I think this util could be useful as
'iperf', because:
1) It is small comparing to 'iperf()', so it very easy to add new
   mode or feature to it(especially vsock specific).
2) It is located in kernel source tree, so it could be updated by the
   same patchset which changes related kernel functionality in vsock.

I used this util very often to check performance of my rx zerocopy
support(this tool has rx zerocopy support, but not in this patchset).

Patchset was rebased and tested on skbuff patch from Bobby Eshleman:
https://lore.kernel.org/lkml/20221110171723.24263-1-bobby.eshleman@bytedance.com/

All 3 patches are from my previous big patchset for virtio/vsock rx
zerocopy - they are not related to zerocopy, so Stefano suggested to
exclude them from patchset.

Arseniy Krasnov(3):
 test/vsock: rework message bound test
 test/vsock: add big message test
 test/vsock: vsock_perf utility

 tools/testing/vsock/Makefile     |   1 +
 tools/testing/vsock/vsock_perf.c | 386 +++++++++++++++++++++++++++++++++++++++
 tools/testing/vsock/vsock_test.c |  62 +++++++
 3 files changed, 449 insertions(+)

-- 
2.25.1

[RFC PATCH v1 1/3] test/vsock: rework message bound test

[Arseniy Krasnov]

This updates message bound test making it more complex. Instead of
sending 1 bytes messages with one MSG_EOR bit, it sends messages of
random length(one half of messages are smaller than page size, second
half are bigger) with random number of MSG_EOR bits set. Receiver
also don't know total number of messages.

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 tools/testing/vsock/control.c    |  34 +++++++++
 tools/testing/vsock/control.h    |   2 +
 tools/testing/vsock/util.c       |  13 ++++
 tools/testing/vsock/util.h       |   1 +
 tools/testing/vsock/vsock_test.c | 115 +++++++++++++++++++++++++++----
 5 files changed, 152 insertions(+), 13 deletions(-)

diff --git a/tools/testing/vsock/control.c b/tools/testing/vsock/control.c
index 4874872fc5a3..bed1649bdf3d 100644
--- a/tools/testing/vsock/control.c
+++ b/tools/testing/vsock/control.c
@@ -141,6 +141,40 @@ void control_writeln(const char *str)
 	timeout_end();
 }
 
+void control_writeulong(unsigned long value)
+{
+	char str[32];
+
+	if (snprintf(str, sizeof(str), "%lu", value) >= sizeof(str)) {
+		perror("snprintf");
+		exit(EXIT_FAILURE);
+	}
+
+	control_writeln(str);
+}
+
+unsigned long control_readulong(bool *ok)
+{
+	unsigned long value;
+	char *str;
+
+	if (ok)
+		*ok = false;
+
+	str = control_readln();
+
+	if (str == NULL)
+		return 0;
+
+	value = strtoul(str, NULL, 10);
+	free(str);
+
+	if (ok)
+		*ok = true;
+
+	return value;
+}
+
 /* Return the next line from the control socket (without the trailing newline).
  *
  * The program terminates if a timeout occurs.
diff --git a/tools/testing/vsock/control.h b/tools/testing/vsock/control.h
index 51814b4f9ac1..cdd922dfea68 100644
--- a/tools/testing/vsock/control.h
+++ b/tools/testing/vsock/control.h
@@ -9,7 +9,9 @@ void control_init(const char *control_host, const char *control_port,
 void control_cleanup(void);
 void control_writeln(const char *str);
 char *control_readln(void);
+unsigned long control_readulong(bool *ok);
 void control_expectln(const char *str);
 bool control_cmpln(char *line, const char *str, bool fail);
+void control_writeulong(unsigned long value);
 
 #endif /* CONTROL_H */
diff --git a/tools/testing/vsock/util.c b/tools/testing/vsock/util.c
index 2acbb7703c6a..351903836774 100644
--- a/tools/testing/vsock/util.c
+++ b/tools/testing/vsock/util.c
@@ -395,3 +395,16 @@ void skip_test(struct test_case *test_cases, size_t test_cases_len,
 
 	test_cases[test_id].skip = true;
 }
+
+unsigned long djb2(const void *data, size_t len)
+{
+	unsigned long hash = 5381;
+	int i = 0;
+
+	while (i < len) {
+		hash = ((hash << 5) + hash) + ((unsigned char *)data)[i];
+		i++;
+	}
+
+	return hash;
+}
diff --git a/tools/testing/vsock/util.h b/tools/testing/vsock/util.h
index a3375ad2fb7f..988cc69a4642 100644
--- a/tools/testing/vsock/util.h
+++ b/tools/testing/vsock/util.h
@@ -49,4 +49,5 @@ void run_tests(const struct test_case *test_cases,
 void list_tests(const struct test_case *test_cases);
 void skip_test(struct test_case *test_cases, size_t test_cases_len,
 	       const char *test_id_str);
+unsigned long djb2(const void *data, size_t len);
 #endif /* UTIL_H */
diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index bb6d691cb30d..107c11165887 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -284,10 +284,14 @@ static void test_stream_msg_peek_server(const struct test_opts *opts)
 	close(fd);
 }
 
-#define MESSAGES_CNT 7
-#define MSG_EOR_IDX (MESSAGES_CNT / 2)
+#define SOCK_BUF_SIZE (2 * 1024 * 1024)
+#define MAX_MSG_SIZE (32 * 1024)
+
 static void test_seqpacket_msg_bounds_client(const struct test_opts *opts)
 {
+	unsigned long curr_hash;
+	int page_size;
+	int msg_count;
 	int fd;
 
 	fd = vsock_seqpacket_connect(opts->peer_cid, 1234);
@@ -296,18 +300,69 @@ static void test_seqpacket_msg_bounds_client(const struct test_opts *opts)
 		exit(EXIT_FAILURE);
 	}
 
-	/* Send several messages, one with MSG_EOR flag */
-	for (int i = 0; i < MESSAGES_CNT; i++)
-		send_byte(fd, 1, (i == MSG_EOR_IDX) ? MSG_EOR : 0);
+	/* Wait, until receiver sets buffer size. */
+	control_expectln("SRVREADY");
+
+	curr_hash = 0;
+	page_size = getpagesize();
+	msg_count = SOCK_BUF_SIZE / MAX_MSG_SIZE;
+
+	for (int i = 0; i < msg_count; i++) {
+		ssize_t send_size;
+		size_t buf_size;
+		int flags;
+		void *buf;
+
+		/* Use "small" buffers and "big" buffers. */
+		if (i & 1)
+			buf_size = page_size +
+					(rand() % (MAX_MSG_SIZE - page_size));
+		else
+			buf_size = 1 + (rand() % page_size);
+
+		buf = malloc(buf_size);
+
+		if (!buf) {
+			perror("malloc");
+			exit(EXIT_FAILURE);
+		}
+
+		/* Set at least one MSG_EOR + some random. */
+		if (i == (msg_count / 2) || (rand() & 1)) {
+			flags = MSG_EOR;
+			curr_hash++;
+		} else {
+			flags = 0;
+		}
+
+		send_size = send(fd, buf, buf_size, flags);
+
+		if (send_size < 0) {
+			perror("send");
+			exit(EXIT_FAILURE);
+		}
+
+		if (send_size != buf_size) {
+			fprintf(stderr, "Invalid send size\n");
+			exit(EXIT_FAILURE);
+		}
+
+		curr_hash += send_size;
+		curr_hash = djb2(&curr_hash, sizeof(curr_hash));
+	}
 
 	control_writeln("SENDDONE");
+	control_writeulong(curr_hash);
 	close(fd);
 }
 
 static void test_seqpacket_msg_bounds_server(const struct test_opts *opts)
 {
+	unsigned long sock_buf_size;
+	unsigned long remote_hash;
+	unsigned long curr_hash;
 	int fd;
-	char buf[16];
+	char buf[MAX_MSG_SIZE];
 	struct msghdr msg = {0};
 	struct iovec iov = {0};
 
@@ -317,25 +372,58 @@ static void test_seqpacket_msg_bounds_server(const struct test_opts *opts)
 		exit(EXIT_FAILURE);
 	}
 
+	sock_buf_size = SOCK_BUF_SIZE;
+
+	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_MAX_SIZE,
+		       &sock_buf_size, sizeof(sock_buf_size))) {
+		perror("getsockopt");
+		exit(EXIT_FAILURE);
+	}
+
+	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
+		       &sock_buf_size, sizeof(sock_buf_size))) {
+		perror("getsockopt");
+		exit(EXIT_FAILURE);
+	}
+
+	/* Ready to receive data. */
+	control_writeln("SRVREADY");
+	/* Wait, until peer sends whole data. */
 	control_expectln("SENDDONE");
 	iov.iov_base = buf;
 	iov.iov_len = sizeof(buf);
 	msg.msg_iov = &iov;
 	msg.msg_iovlen = 1;
 
-	for (int i = 0; i < MESSAGES_CNT; i++) {
-		if (recvmsg(fd, &msg, 0) != 1) {
-			perror("message bound violated");
-			exit(EXIT_FAILURE);
-		}
+	curr_hash = 0;
 
-		if ((i == MSG_EOR_IDX) ^ !!(msg.msg_flags & MSG_EOR)) {
-			perror("MSG_EOR");
+	while (1) {
+		ssize_t recv_size;
+
+		recv_size = recvmsg(fd, &msg, 0);
+
+		if (!recv_size)
+			break;
+
+		if (recv_size < 0) {
+			perror("recvmsg");
 			exit(EXIT_FAILURE);
 		}
+
+		if (msg.msg_flags & MSG_EOR)
+			curr_hash++;
+
+		curr_hash += recv_size;
+		curr_hash = djb2(&curr_hash, sizeof(curr_hash));
 	}
 
 	close(fd);
+	remote_hash = control_readulong(NULL);
+
+	if (curr_hash != remote_hash) {
+		fprintf(stderr, "Message bounds broken\n");
+		exit(EXIT_FAILURE);
+	}
 }
 
 #define MESSAGE_TRUNC_SZ 32
@@ -837,6 +925,7 @@ int main(int argc, char **argv)
 		.peer_cid = VMADDR_CID_ANY,
 	};
 
+	srand(time(NULL));
 	init_signals();
 
 	for (;;) {
-- 
2.25.1

[RFC PATCH v1 2/3] test/vsock: add big message test

[Arseniy Krasnov]

This adds test for sending message, bigger than peer's buffer size.
For SOCK_SEQPACKET socket it must fail, as this type of socket has
message size limit.

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 tools/testing/vsock/vsock_test.c | 62 ++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 107c11165887..bb4e8657f1d6 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -560,6 +560,63 @@ static void test_seqpacket_timeout_server(const struct test_opts *opts)
 	close(fd);
 }
 
+static void test_seqpacket_bigmsg_client(const struct test_opts *opts)
+{
+	unsigned long sock_buf_size;
+	ssize_t send_size;
+	socklen_t len;
+	void *data;
+	int fd;
+
+	len = sizeof(sock_buf_size);
+
+	fd = vsock_seqpacket_connect(opts->peer_cid, 1234);
+	if (fd < 0) {
+		perror("connect");
+		exit(EXIT_FAILURE);
+	}
+
+	if (getsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
+		       &sock_buf_size, &len)) {
+		perror("getsockopt");
+		exit(EXIT_FAILURE);
+	}
+
+	sock_buf_size++;
+
+	data = malloc(sock_buf_size);
+	if (!data) {
+		perror("malloc");
+		exit(EXIT_FAILURE);
+	}
+
+	send_size = send(fd, data, sock_buf_size, 0);
+	if (send_size != -1) {
+		fprintf(stderr, "expected 'send(2)' failure, got %zi\n",
+			send_size);
+	}
+
+	control_writeln("CLISENT");
+
+	free(data);
+	close(fd);
+}
+
+static void test_seqpacket_bigmsg_server(const struct test_opts *opts)
+{
+	int fd;
+
+	fd = vsock_seqpacket_accept(VMADDR_CID_ANY, 1234, NULL);
+	if (fd < 0) {
+		perror("accept");
+		exit(EXIT_FAILURE);
+	}
+
+	control_expectln("CLISENT");
+
+	close(fd);
+}
+
 #define BUF_PATTERN_1 'a'
 #define BUF_PATTERN_2 'b'
 
@@ -832,6 +889,11 @@ static struct test_case test_cases[] = {
 		.run_client = test_seqpacket_timeout_client,
 		.run_server = test_seqpacket_timeout_server,
 	},
+	{
+		.name = "SOCK_SEQPACKET big message",
+		.run_client = test_seqpacket_bigmsg_client,
+		.run_server = test_seqpacket_bigmsg_server,
+	},
 	{
 		.name = "SOCK_SEQPACKET invalid receive buffer",
 		.run_client = test_seqpacket_invalid_rec_buffer_client,
-- 
2.25.1

[RFC PATCH v1 3/3] test/vsock: vsock_perf utility

[Arseniy Krasnov]

This adds utility to check vsock receive performance.

Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 tools/testing/vsock/Makefile     |   1 +
 tools/testing/vsock/vsock_perf.c | 386 +++++++++++++++++++++++++++++++
 2 files changed, 387 insertions(+)
 create mode 100644 tools/testing/vsock/vsock_perf.c

diff --git a/tools/testing/vsock/Makefile b/tools/testing/vsock/Makefile
index f8293c6910c9..d36fdd59fe2e 100644
--- a/tools/testing/vsock/Makefile
+++ b/tools/testing/vsock/Makefile
@@ -3,6 +3,7 @@ all: test
 test: vsock_test vsock_diag_test
 vsock_test: vsock_test.o timeout.o control.o util.o
 vsock_diag_test: vsock_diag_test.o timeout.o control.o util.o
+vsock_perf: vsock_perf.o
 
 CFLAGS += -g -O2 -Werror -Wall -I. -I../../include -I../../../usr/include -Wno-pointer-sign -fno-strict-overflow -fno-strict-aliasing -fno-common -MMD -U_FORTIFY_SOURCE -D_GNU_SOURCE
 .PHONY: all test clean
diff --git a/tools/testing/vsock/vsock_perf.c b/tools/testing/vsock/vsock_perf.c
new file mode 100644
index 000000000000..ca7af08dca46
--- /dev/null
+++ b/tools/testing/vsock/vsock_perf.c
@@ -0,0 +1,386 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * vsock_perf - benchmark utility for vsock.
+ *
+ * Copyright (C) 2022 SberDevices.
+ *
+ * Author: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
+ */
+#include <getopt.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <string.h>
+#include <errno.h>
+#include <unistd.h>
+#include <time.h>
+#include <sys/mman.h>
+#include <stdint.h>
+#include <poll.h>
+#include <sys/socket.h>
+#include <linux/vm_sockets.h>
+
+#define DEFAULT_BUF_SIZE_BYTES	(128*1024)
+#define DEFAULT_TO_SEND_BYTES	(64*1024)
+#define DEFAULT_VSOCK_BUF_BYTES (256*1024)
+#define DEFAULT_RCVLOWAT_BYTES	1
+#define DEFAULT_PORT		1234
+
+#define BYTES_PER_GB		(1024 * 1024 * 1024ULL)
+#define NSEC_PER_SEC		(1000000000ULL)
+
+static unsigned int port = DEFAULT_PORT;
+static unsigned long buf_size_bytes = DEFAULT_BUF_SIZE_BYTES;
+static unsigned long vsock_buf_bytes = DEFAULT_VSOCK_BUF_BYTES;
+
+static inline time_t current_nsec(void)
+{
+	struct timespec ts;
+
+	if (clock_gettime(CLOCK_REALTIME, &ts)) {
+		perror("clock_gettime");
+		exit(EXIT_FAILURE);
+	}
+
+	return (ts.tv_sec * NSEC_PER_SEC) + ts.tv_nsec;
+}
+
+/* From lib/cmdline.c. */
+static unsigned long memparse(const char *ptr)
+{
+	char *endptr;
+
+	unsigned long long ret = strtoull(ptr, &endptr, 0);
+
+	switch (*endptr) {
+	case 'E':
+	case 'e':
+		ret <<= 10;
+	case 'P':
+	case 'p':
+		ret <<= 10;
+	case 'T':
+	case 't':
+		ret <<= 10;
+	case 'G':
+	case 'g':
+		ret <<= 10;
+	case 'M':
+	case 'm':
+		ret <<= 10;
+	case 'K':
+	case 'k':
+		ret <<= 10;
+		endptr++;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static void vsock_increase_buf_size(int fd)
+{
+	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_MAX_SIZE,
+		       &vsock_buf_bytes, sizeof(vsock_buf_bytes))) {
+		perror("setsockopt");
+		exit(EXIT_FAILURE);
+	}
+
+	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
+		       &vsock_buf_bytes, sizeof(vsock_buf_bytes))) {
+		perror("setsockopt");
+		exit(EXIT_FAILURE);
+	}
+}
+
+static int vsock_connect(unsigned int cid, unsigned int port)
+{
+	union {
+		struct sockaddr sa;
+		struct sockaddr_vm svm;
+	} addr = {
+		.svm = {
+			.svm_family = AF_VSOCK,
+			.svm_port = port,
+			.svm_cid = cid,
+		},
+	};
+	int fd;
+
+	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
+
+	if (fd < 0)
+		return -1;
+
+	vsock_increase_buf_size(fd);
+
+	if (connect(fd, &addr.sa, sizeof(addr.svm)) < 0) {
+		close(fd);
+		return -1;
+	}
+
+	return fd;
+}
+
+static float get_gbps(unsigned long bytes, time_t ns_delta)
+{
+	return ((float)bytes / BYTES_PER_GB) /
+	       ((float)ns_delta / NSEC_PER_SEC);
+}
+
+static void run_sender(unsigned long to_send_bytes)
+{
+	time_t tx_begin_ns;
+	size_t total_send;
+	int client_fd;
+	char *data;
+	int fd;
+	union {
+		struct sockaddr sa;
+		struct sockaddr_vm svm;
+	} addr = {
+		.svm = {
+			.svm_family = AF_VSOCK,
+			.svm_port = port,
+			.svm_cid = VMADDR_CID_ANY,
+		},
+	};
+	union {
+		struct sockaddr sa;
+		struct sockaddr_vm svm;
+	} clientaddr;
+
+	socklen_t clientaddr_len = sizeof(clientaddr.svm);
+
+	fprintf(stderr, "Run as sender\n");
+	fprintf(stderr, "Listen port %u\n", port);
+	fprintf(stderr, "Send %lu bytes\n", to_send_bytes);
+	fprintf(stderr, "TX buffer %lu bytes\n", buf_size_bytes);
+	fprintf(stderr, "Peer buffer %lu bytes\n", vsock_buf_bytes);
+
+	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
+
+	if (fd < 0) {
+		perror("socket");
+		exit(EXIT_FAILURE);
+	}
+
+	if (bind(fd, &addr.sa, sizeof(addr.svm)) < 0) {
+		perror("bind");
+		exit(EXIT_FAILURE);
+	}
+
+	if (listen(fd, 1) < 0) {
+		perror("listen");
+		exit(EXIT_FAILURE);
+	}
+
+	client_fd = accept(fd, &clientaddr.sa, &clientaddr_len);
+
+	if (client_fd < 0) {
+		perror("accept");
+		exit(EXIT_FAILURE);
+	}
+
+	vsock_increase_buf_size(client_fd);
+
+	data = malloc(buf_size_bytes);
+
+	if (!data) {
+		fprintf(stderr, "malloc failed\n");
+		exit(EXIT_FAILURE);
+	}
+
+	memset(data, 0, buf_size_bytes);
+	total_send = 0;
+	tx_begin_ns = current_nsec();
+
+	while (total_send < to_send_bytes) {
+		ssize_t sent;
+
+		sent = write(client_fd, data, buf_size_bytes);
+
+		if (sent <= 0) {
+			perror("write");
+			exit(EXIT_FAILURE);
+		}
+
+		total_send += sent;
+	}
+
+	fprintf(stderr, "tx performance: %f Gb/s\n",
+			get_gbps(total_send, current_nsec() - tx_begin_ns));
+
+	close(client_fd);
+	close(fd);
+
+	free(data);
+}
+
+static void run_receiver(int peer_cid, unsigned long rcvlowat_bytes)
+{
+	unsigned int read_cnt;
+	time_t rx_begin_ns;
+	time_t in_read_ns;
+	size_t total_recv;
+	void *data;
+	int fd;
+
+	fprintf(stderr, "Run as receiver\n");
+	fprintf(stderr, "Connect to %i:%u\n", peer_cid, port);
+	fprintf(stderr, "RX buffer %lu bytes\n", buf_size_bytes);
+	fprintf(stderr, "Peer buffer %lu bytes\n", vsock_buf_bytes);
+	fprintf(stderr, "SO_RCVLOWAT %lu bytes\n", rcvlowat_bytes);
+
+	fd = vsock_connect(peer_cid, port);
+
+	if (fd < 0) {
+		perror("socket");
+		exit(EXIT_FAILURE);
+	}
+
+	if (setsockopt(fd, SOL_SOCKET, SO_RCVLOWAT,
+		       &rcvlowat_bytes,
+		       sizeof(rcvlowat_bytes))) {
+		perror("setsockopt");
+		exit(EXIT_FAILURE);
+	}
+
+	data = mmap(NULL, buf_size_bytes, PROT_READ | PROT_WRITE,
+		    MAP_ANONYMOUS | MAP_PRIVATE | MAP_POPULATE, -1, 0);
+
+	if (data == MAP_FAILED) {
+		perror("mmap");
+		exit(EXIT_FAILURE);
+	}
+
+	read_cnt = 0;
+	in_read_ns = 0;
+	total_recv = 0;
+	rx_begin_ns = current_nsec();
+
+	while (1) {
+		struct pollfd fds = { 0 };
+
+		fds.fd = fd;
+		fds.events = POLLIN | POLLERR | POLLHUP |
+			     POLLRDHUP | POLLNVAL;
+
+		if (poll(&fds, 1, -1) < 0) {
+			perror("poll");
+			exit(EXIT_FAILURE);
+		}
+
+		if (fds.revents & POLLERR) {
+			fprintf(stderr, "'poll()' error\n");
+			exit(EXIT_FAILURE);
+		}
+
+		if (fds.revents & POLLIN) {
+			ssize_t bytes_read;
+			time_t t;
+
+			t = current_nsec();
+			bytes_read = read(fd, data, buf_size_bytes);
+			in_read_ns += (current_nsec() - t);
+			read_cnt++;
+
+			if (!bytes_read)
+				break;
+
+			if (bytes_read < 0) {
+				perror("recv");
+				exit(EXIT_FAILURE);
+			}
+
+			total_recv += bytes_read;
+		}
+
+		if (fds.revents & (POLLHUP | POLLRDHUP))
+			break;
+	}
+
+	fprintf(stderr, "rx performance %f Gb/s\n",
+			get_gbps(total_recv, current_nsec() - rx_begin_ns));
+	fprintf(stderr, "total in 'read()' %f sec\n", (float)in_read_ns / NSEC_PER_SEC);
+	fprintf(stderr, "POLLIN wakeups: %i\n", read_cnt);
+	fprintf(stderr, "average in 'read()' %f ns\n", (float)in_read_ns / read_cnt);
+
+	munmap(data, buf_size_bytes);
+	close(fd);
+}
+
+static void usage(void)
+{
+	fprintf(stderr, "Help:\n"
+			"\n"
+			"This is benchmarking utility, to test vsock performance.\n"
+			"It runs in two modes: sender or receiver. In sender mode, it waits\n"
+			"connection from receiver, and when established, sender starts data\n"
+			"transmission. Total size of data to send is set by '-m' option.\n"
+			"\n"
+			"Meaning of '-b' depends of sender or receiver mode. In sender\n"
+			"mode, it is size of buffer, passed to 'write()'. In receiver mode,\n"
+			"it is size of rx buffer passed to 'read()'.\n"
+			"\n"
+			"Options:\n"
+			"  -h				This help message\n"
+			"  -s				Sender mode(receiver default)\n"
+			"  -p <port>			Port\n"
+			"  -c <cid>			CID of the peer\n"
+			"  -m <bytes to send>		Megabytes to send\n"
+			"  -b <buffer size>		Depends on sender/receiver mode\n"
+			"  -v <peer buffer size>	Peer buffer size\n"
+			"  -r <SO_RCVLOWAT>		SO_RCVLOWAT\n"
+			"\n");
+	exit(EXIT_FAILURE);
+}
+
+int main(int argc, char **argv)
+{
+	unsigned long to_send_bytes = DEFAULT_TO_SEND_BYTES;
+	unsigned long rcvlowat_bytes = DEFAULT_RCVLOWAT_BYTES;
+	bool receiver_mode = true;
+	int peer_cid = -1;
+	int c;
+
+	while ((c = getopt(argc, argv, "v:r:c:p:m:b:sh")) != -1) {
+		switch (c) {
+		case 'v': /* Peer buffer size. */
+			vsock_buf_bytes = memparse(optarg);
+			break;
+		case 'r': /* SO_RCVLOWAT value. */
+			rcvlowat_bytes = memparse(optarg);
+			break;
+		case 'c': /* CID to connect to. */
+			peer_cid = atoi(optarg);
+			break;
+		case 'p': /* Port to connect to. */
+			port = atoi(optarg);
+			break;
+		case 'm': /* Bytes to send. */
+			to_send_bytes = memparse(optarg);
+			break;
+		case 'b': /* Size of rx/tx buffer. */
+			buf_size_bytes = memparse(optarg);
+			break;
+		case 's': /* Sender mode. */
+			receiver_mode = false;
+			break;
+		case 'h': /* Help. */
+			usage();
+			break;
+		default:
+			usage();
+
+		}
+	}
+
+	if (receiver_mode)
+		run_receiver(peer_cid, rcvlowat_bytes);
+	else
+		run_sender(to_send_bytes);
+
+	return 0;
+}
-- 
2.25.1

Re: [RFC PATCH v1 1/3] test/vsock: rework message bound test

[Stefano Garzarella]

On Tue, Nov 15, 2022 at 08:50:36PM +0000, Arseniy Krasnov wrote:
>This updates message bound test making it more complex. Instead of
>sending 1 bytes messages with one MSG_EOR bit, it sends messages of
>random length(one half of messages are smaller than page size, second
>half are bigger) with random number of MSG_EOR bits set. Receiver
>also don't know total number of messages.
>
>Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>---
> tools/testing/vsock/control.c    |  34 +++++++++
> tools/testing/vsock/control.h    |   2 +
> tools/testing/vsock/util.c       |  13 ++++
> tools/testing/vsock/util.h       |   1 +
> tools/testing/vsock/vsock_test.c | 115 +++++++++++++++++++++++++++----
> 5 files changed, 152 insertions(+), 13 deletions(-)
>
>diff --git a/tools/testing/vsock/control.c b/tools/testing/vsock/control.c
>index 4874872fc5a3..bed1649bdf3d 100644
>--- a/tools/testing/vsock/control.c
>+++ b/tools/testing/vsock/control.c
>@@ -141,6 +141,40 @@ void control_writeln(const char *str)
> 	timeout_end();
> }
>
>+void control_writeulong(unsigned long value)
>+{
>+	char str[32];
>+
>+	if (snprintf(str, sizeof(str), "%lu", value) >= sizeof(str)) {
>+		perror("snprintf");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	control_writeln(str);
>+}
>+
>+unsigned long control_readulong(bool *ok)
>+{
>+	unsigned long value;
>+	char *str;
>+
>+	if (ok)
>+		*ok = false;
>+
>+	str = control_readln();
>+
>+	if (str == NULL)

checkpatch suggests to use !str

>+		return 0;

Maybe we can just call exit(EXIT_FAILURE) here and remove the `ok`
parameter, since I'm not sure we can recover from this error.

>+
>+	value = strtoul(str, NULL, 10);
>+	free(str);
>+
>+	if (ok)
>+		*ok = true;
>+
>+	return value;
>+}
>+
> /* Return the next line from the control socket (without the trailing newline).
>  *
>  * The program terminates if a timeout occurs.
>diff --git a/tools/testing/vsock/control.h b/tools/testing/vsock/control.h
>index 51814b4f9ac1..cdd922dfea68 100644
>--- a/tools/testing/vsock/control.h
>+++ b/tools/testing/vsock/control.h
>@@ -9,7 +9,9 @@ void control_init(const char *control_host, const char *control_port,
> void control_cleanup(void);
> void control_writeln(const char *str);
> char *control_readln(void);
>+unsigned long control_readulong(bool *ok);
> void control_expectln(const char *str);
> bool control_cmpln(char *line, const char *str, bool fail);
>+void control_writeulong(unsigned long value);
>
> #endif /* CONTROL_H */
>diff --git a/tools/testing/vsock/util.c b/tools/testing/vsock/util.c
>index 2acbb7703c6a..351903836774 100644
>--- a/tools/testing/vsock/util.c
>+++ b/tools/testing/vsock/util.c
>@@ -395,3 +395,16 @@ void skip_test(struct test_case *test_cases, size_t test_cases_len,
>
> 	test_cases[test_id].skip = true;
> }
>+
>+unsigned long djb2(const void *data, size_t len)

I would add hash_ as a prefix (or suffix).

>+{
>+	unsigned long hash = 5381;
>+	int i = 0;
>+
>+	while (i < len) {
>+		hash = ((hash << 5) + hash) + ((unsigned char *)data)[i];
>+		i++;
>+	}
>+
>+	return hash;
>+}
>diff --git a/tools/testing/vsock/util.h b/tools/testing/vsock/util.h
>index a3375ad2fb7f..988cc69a4642 100644
>--- a/tools/testing/vsock/util.h
>+++ b/tools/testing/vsock/util.h
>@@ -49,4 +49,5 @@ void run_tests(const struct test_case *test_cases,
> void list_tests(const struct test_case *test_cases);
> void skip_test(struct test_case *test_cases, size_t test_cases_len,
> 	       const char *test_id_str);
>+unsigned long djb2(const void *data, size_t len);
> #endif /* UTIL_H */
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index bb6d691cb30d..107c11165887 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -284,10 +284,14 @@ static void test_stream_msg_peek_server(const struct test_opts *opts)
> 	close(fd);
> }
>
>-#define MESSAGES_CNT 7
>-#define MSG_EOR_IDX (MESSAGES_CNT / 2)
>+#define SOCK_BUF_SIZE (2 * 1024 * 1024)
>+#define MAX_MSG_SIZE (32 * 1024)
>+
> static void test_seqpacket_msg_bounds_client(const struct test_opts *opts)
> {
>+	unsigned long curr_hash;
>+	int page_size;
>+	int msg_count;
> 	int fd;
>
> 	fd = vsock_seqpacket_connect(opts->peer_cid, 1234);
>@@ -296,18 +300,69 @@ static void test_seqpacket_msg_bounds_client(const struct test_opts *opts)
> 		exit(EXIT_FAILURE);
> 	}
>
>-	/* Send several messages, one with MSG_EOR flag */
>-	for (int i = 0; i < MESSAGES_CNT; i++)
>-		send_byte(fd, 1, (i == MSG_EOR_IDX) ? MSG_EOR : 0);
>+	/* Wait, until receiver sets buffer size. */
>+	control_expectln("SRVREADY");
>+
>+	curr_hash = 0;
>+	page_size = getpagesize();
>+	msg_count = SOCK_BUF_SIZE / MAX_MSG_SIZE;
>+
>+	for (int i = 0; i < msg_count; i++) {
>+		ssize_t send_size;
>+		size_t buf_size;
>+		int flags;
>+		void *buf;
>+
>+		/* Use "small" buffers and "big" buffers. */
>+		if (i & 1)
>+			buf_size = page_size +
>+					(rand() % (MAX_MSG_SIZE - page_size));
>+		else
>+			buf_size = 1 + (rand() % page_size);
>+
>+		buf = malloc(buf_size);
>+
>+		if (!buf) {
>+			perror("malloc");
>+			exit(EXIT_FAILURE);
>+		}
>+
>+		/* Set at least one MSG_EOR + some random. */
>+		if (i == (msg_count / 2) || (rand() & 1)) {
>+			flags = MSG_EOR;
>+			curr_hash++;
>+		} else {
>+			flags = 0;
>+		}
>+
>+		send_size = send(fd, buf, buf_size, flags);
>+
>+		if (send_size < 0) {
>+			perror("send");
>+			exit(EXIT_FAILURE);
>+		}
>+
>+		if (send_size != buf_size) {
>+			fprintf(stderr, "Invalid send size\n");
>+			exit(EXIT_FAILURE);
>+		}
>+
>+		curr_hash += send_size;
>+		curr_hash = djb2(&curr_hash, sizeof(curr_hash));
>+	}
>
> 	control_writeln("SENDDONE");
>+	control_writeulong(curr_hash);

Why do we need to hash the size?

Maybe we can send it without making the hash, anyway even if it wraps,
it should wrap the same way in both the server and the client.
(Or maybe we can use uin32_t or uint64_t to make sure both were
using 4 or 8 bytes)

> 	close(fd);
> }
>
> static void test_seqpacket_msg_bounds_server(const struct test_opts *opts)
> {
>+	unsigned long sock_buf_size;
>+	unsigned long remote_hash;
>+	unsigned long curr_hash;
> 	int fd;
>-	char buf[16];
>+	char buf[MAX_MSG_SIZE];
> 	struct msghdr msg = {0};
> 	struct iovec iov = {0};
>
>@@ -317,25 +372,58 @@ static void test_seqpacket_msg_bounds_server(const struct test_opts *opts)
> 		exit(EXIT_FAILURE);
> 	}
>
>+	sock_buf_size = SOCK_BUF_SIZE;
>+
>+	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_MAX_SIZE,
>+		       &sock_buf_size, sizeof(sock_buf_size))) {
>+		perror("getsockopt");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>+		       &sock_buf_size, sizeof(sock_buf_size))) {
>+		perror("getsockopt");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	/* Ready to receive data. */
>+	control_writeln("SRVREADY");
>+	/* Wait, until peer sends whole data. */
> 	control_expectln("SENDDONE");
> 	iov.iov_base = buf;
> 	iov.iov_len = sizeof(buf);
> 	msg.msg_iov = &iov;
> 	msg.msg_iovlen = 1;
>
>-	for (int i = 0; i < MESSAGES_CNT; i++) {
>-		if (recvmsg(fd, &msg, 0) != 1) {
>-			perror("message bound violated");
>-			exit(EXIT_FAILURE);
>-		}
>+	curr_hash = 0;
>
>-		if ((i == MSG_EOR_IDX) ^ !!(msg.msg_flags & MSG_EOR)) {
>-			perror("MSG_EOR");
>+	while (1) {
>+		ssize_t recv_size;
>+
>+		recv_size = recvmsg(fd, &msg, 0);
>+
>+		if (!recv_size)
>+			break;
>+
>+		if (recv_size < 0) {
>+			perror("recvmsg");
> 			exit(EXIT_FAILURE);
> 		}
>+
>+		if (msg.msg_flags & MSG_EOR)
>+			curr_hash++;
>+
>+		curr_hash += recv_size;
>+		curr_hash = djb2(&curr_hash, sizeof(curr_hash));
> 	}
>
> 	close(fd);
>+	remote_hash = control_readulong(NULL);
>+
>+	if (curr_hash != remote_hash) {
>+		fprintf(stderr, "Message bounds broken\n");
>+		exit(EXIT_FAILURE);
>+	}
> }
>
> #define MESSAGE_TRUNC_SZ 32
>@@ -837,6 +925,7 @@ int main(int argc, char **argv)
> 		.peer_cid = VMADDR_CID_ANY,
> 	};
>
>+	srand(time(NULL));
> 	init_signals();
>
> 	for (;;) {
>-- 
>2.25.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC PATCH v1 1/3] test/vsock: rework message bound test

[Stefano Garzarella]

On Tue, Nov 15, 2022 at 08:50:36PM +0000, Arseniy Krasnov wrote:
>This updates message bound test making it more complex. Instead of
>sending 1 bytes messages with one MSG_EOR bit, it sends messages of
>random length(one half of messages are smaller than page size, second
>half are bigger) with random number of MSG_EOR bits set. Receiver
>also don't know total number of messages.
>
>Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>---
> tools/testing/vsock/control.c    |  34 +++++++++
> tools/testing/vsock/control.h    |   2 +
> tools/testing/vsock/util.c       |  13 ++++
> tools/testing/vsock/util.h       |   1 +
> tools/testing/vsock/vsock_test.c | 115 +++++++++++++++++++++++++++----
> 5 files changed, 152 insertions(+), 13 deletions(-)
>
>diff --git a/tools/testing/vsock/control.c b/tools/testing/vsock/control.c
>index 4874872fc5a3..bed1649bdf3d 100644
>--- a/tools/testing/vsock/control.c
>+++ b/tools/testing/vsock/control.c
>@@ -141,6 +141,40 @@ void control_writeln(const char *str)
> 	timeout_end();
> }
>
>+void control_writeulong(unsigned long value)
>+{
>+	char str[32];
>+
>+	if (snprintf(str, sizeof(str), "%lu", value) >= sizeof(str)) {
>+		perror("snprintf");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	control_writeln(str);
>+}
>+
>+unsigned long control_readulong(bool *ok)
>+{
>+	unsigned long value;
>+	char *str;
>+
>+	if (ok)
>+		*ok = false;
>+
>+	str = control_readln();
>+
>+	if (str == NULL)

checkpatch suggests to use !str

>+		return 0;

Maybe we can just call exit(EXIT_FAILURE) here and remove the `ok`
parameter, since I'm not sure we can recover from this error.

>+
>+	value = strtoul(str, NULL, 10);
>+	free(str);
>+
>+	if (ok)
>+		*ok = true;
>+
>+	return value;
>+}
>+
> /* Return the next line from the control socket (without the trailing newline).
>  *
>  * The program terminates if a timeout occurs.
>diff --git a/tools/testing/vsock/control.h b/tools/testing/vsock/control.h
>index 51814b4f9ac1..cdd922dfea68 100644
>--- a/tools/testing/vsock/control.h
>+++ b/tools/testing/vsock/control.h
>@@ -9,7 +9,9 @@ void control_init(const char *control_host, const char *control_port,
> void control_cleanup(void);
> void control_writeln(const char *str);
> char *control_readln(void);
>+unsigned long control_readulong(bool *ok);
> void control_expectln(const char *str);
> bool control_cmpln(char *line, const char *str, bool fail);
>+void control_writeulong(unsigned long value);
>
> #endif /* CONTROL_H */
>diff --git a/tools/testing/vsock/util.c b/tools/testing/vsock/util.c
>index 2acbb7703c6a..351903836774 100644
>--- a/tools/testing/vsock/util.c
>+++ b/tools/testing/vsock/util.c
>@@ -395,3 +395,16 @@ void skip_test(struct test_case *test_cases, size_t test_cases_len,
>
> 	test_cases[test_id].skip = true;
> }
>+
>+unsigned long djb2(const void *data, size_t len)

I would add hash_ as a prefix (or suffix).

>+{
>+	unsigned long hash = 5381;
>+	int i = 0;
>+
>+	while (i < len) {
>+		hash = ((hash << 5) + hash) + ((unsigned char *)data)[i];
>+		i++;
>+	}
>+
>+	return hash;
>+}
>diff --git a/tools/testing/vsock/util.h b/tools/testing/vsock/util.h
>index a3375ad2fb7f..988cc69a4642 100644
>--- a/tools/testing/vsock/util.h
>+++ b/tools/testing/vsock/util.h
>@@ -49,4 +49,5 @@ void run_tests(const struct test_case *test_cases,
> void list_tests(const struct test_case *test_cases);
> void skip_test(struct test_case *test_cases, size_t test_cases_len,
> 	       const char *test_id_str);
>+unsigned long djb2(const void *data, size_t len);
> #endif /* UTIL_H */
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index bb6d691cb30d..107c11165887 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -284,10 +284,14 @@ static void test_stream_msg_peek_server(const struct test_opts *opts)
> 	close(fd);
> }
>
>-#define MESSAGES_CNT 7
>-#define MSG_EOR_IDX (MESSAGES_CNT / 2)
>+#define SOCK_BUF_SIZE (2 * 1024 * 1024)
>+#define MAX_MSG_SIZE (32 * 1024)
>+
> static void test_seqpacket_msg_bounds_client(const struct test_opts *opts)
> {
>+	unsigned long curr_hash;
>+	int page_size;
>+	int msg_count;
> 	int fd;
>
> 	fd = vsock_seqpacket_connect(opts->peer_cid, 1234);
>@@ -296,18 +300,69 @@ static void test_seqpacket_msg_bounds_client(const struct test_opts *opts)
> 		exit(EXIT_FAILURE);
> 	}
>
>-	/* Send several messages, one with MSG_EOR flag */
>-	for (int i = 0; i < MESSAGES_CNT; i++)
>-		send_byte(fd, 1, (i == MSG_EOR_IDX) ? MSG_EOR : 0);
>+	/* Wait, until receiver sets buffer size. */
>+	control_expectln("SRVREADY");
>+
>+	curr_hash = 0;
>+	page_size = getpagesize();
>+	msg_count = SOCK_BUF_SIZE / MAX_MSG_SIZE;
>+
>+	for (int i = 0; i < msg_count; i++) {
>+		ssize_t send_size;
>+		size_t buf_size;
>+		int flags;
>+		void *buf;
>+
>+		/* Use "small" buffers and "big" buffers. */
>+		if (i & 1)
>+			buf_size = page_size +
>+					(rand() % (MAX_MSG_SIZE - page_size));
>+		else
>+			buf_size = 1 + (rand() % page_size);
>+
>+		buf = malloc(buf_size);
>+
>+		if (!buf) {
>+			perror("malloc");
>+			exit(EXIT_FAILURE);
>+		}
>+
>+		/* Set at least one MSG_EOR + some random. */
>+		if (i == (msg_count / 2) || (rand() & 1)) {
>+			flags = MSG_EOR;
>+			curr_hash++;
>+		} else {
>+			flags = 0;
>+		}
>+
>+		send_size = send(fd, buf, buf_size, flags);
>+
>+		if (send_size < 0) {
>+			perror("send");
>+			exit(EXIT_FAILURE);
>+		}
>+
>+		if (send_size != buf_size) {
>+			fprintf(stderr, "Invalid send size\n");
>+			exit(EXIT_FAILURE);
>+		}
>+
>+		curr_hash += send_size;
>+		curr_hash = djb2(&curr_hash, sizeof(curr_hash));
>+	}
>
> 	control_writeln("SENDDONE");
>+	control_writeulong(curr_hash);

Why do we need to hash the size?

Maybe we can send it without making the hash, anyway even if it wraps,
it should wrap the same way in both the server and the client.
(Or maybe we can use uin32_t or uint64_t to make sure both were
using 4 or 8 bytes)

> 	close(fd);
> }
>
> static void test_seqpacket_msg_bounds_server(const struct test_opts *opts)
> {
>+	unsigned long sock_buf_size;
>+	unsigned long remote_hash;
>+	unsigned long curr_hash;
> 	int fd;
>-	char buf[16];
>+	char buf[MAX_MSG_SIZE];
> 	struct msghdr msg = {0};
> 	struct iovec iov = {0};
>
>@@ -317,25 +372,58 @@ static void test_seqpacket_msg_bounds_server(const struct test_opts *opts)
> 		exit(EXIT_FAILURE);
> 	}
>
>+	sock_buf_size = SOCK_BUF_SIZE;
>+
>+	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_MAX_SIZE,
>+		       &sock_buf_size, sizeof(sock_buf_size))) {
>+		perror("getsockopt");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>+		       &sock_buf_size, sizeof(sock_buf_size))) {
>+		perror("getsockopt");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	/* Ready to receive data. */
>+	control_writeln("SRVREADY");
>+	/* Wait, until peer sends whole data. */
> 	control_expectln("SENDDONE");
> 	iov.iov_base = buf;
> 	iov.iov_len = sizeof(buf);
> 	msg.msg_iov = &iov;
> 	msg.msg_iovlen = 1;
>
>-	for (int i = 0; i < MESSAGES_CNT; i++) {
>-		if (recvmsg(fd, &msg, 0) != 1) {
>-			perror("message bound violated");
>-			exit(EXIT_FAILURE);
>-		}
>+	curr_hash = 0;
>
>-		if ((i == MSG_EOR_IDX) ^ !!(msg.msg_flags & MSG_EOR)) {
>-			perror("MSG_EOR");
>+	while (1) {
>+		ssize_t recv_size;
>+
>+		recv_size = recvmsg(fd, &msg, 0);
>+
>+		if (!recv_size)
>+			break;
>+
>+		if (recv_size < 0) {
>+			perror("recvmsg");
> 			exit(EXIT_FAILURE);
> 		}
>+
>+		if (msg.msg_flags & MSG_EOR)
>+			curr_hash++;
>+
>+		curr_hash += recv_size;
>+		curr_hash = djb2(&curr_hash, sizeof(curr_hash));
> 	}
>
> 	close(fd);
>+	remote_hash = control_readulong(NULL);
>+
>+	if (curr_hash != remote_hash) {
>+		fprintf(stderr, "Message bounds broken\n");
>+		exit(EXIT_FAILURE);
>+	}
> }
>
> #define MESSAGE_TRUNC_SZ 32
>@@ -837,6 +925,7 @@ int main(int argc, char **argv)
> 		.peer_cid = VMADDR_CID_ANY,
> 	};
>
>+	srand(time(NULL));
> 	init_signals();
>
> 	for (;;) {
>-- 
>2.25.1

Re: [RFC PATCH v1 2/3] test/vsock: add big message test

[Stefano Garzarella]

On Tue, Nov 15, 2022 at 08:52:35PM +0000, Arseniy Krasnov wrote:
>This adds test for sending message, bigger than peer's buffer size.
>For SOCK_SEQPACKET socket it must fail, as this type of socket has
>message size limit.
>
>Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>---
> tools/testing/vsock/vsock_test.c | 62 ++++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
>
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index 107c11165887..bb4e8657f1d6 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -560,6 +560,63 @@ static void test_seqpacket_timeout_server(const struct test_opts *opts)
> 	close(fd);
> }
>
>+static void test_seqpacket_bigmsg_client(const struct test_opts *opts)
>+{
>+	unsigned long sock_buf_size;
>+	ssize_t send_size;
>+	socklen_t len;
>+	void *data;
>+	int fd;
>+
>+	len = sizeof(sock_buf_size);
>+
>+	fd = vsock_seqpacket_connect(opts->peer_cid, 1234);

Not for this patch, but someday we should add a macro for this port and 
maybe even make it configurable :-)

>+	if (fd < 0) {
>+		perror("connect");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	if (getsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>+		       &sock_buf_size, &len)) {
>+		perror("getsockopt");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	sock_buf_size++;
>+
>+	data = malloc(sock_buf_size);
>+	if (!data) {
>+		perror("malloc");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	send_size = send(fd, data, sock_buf_size, 0);
>+	if (send_size != -1) {

Can we check also `errno`?
IIUC it should contains EMSGSIZE.

>+		fprintf(stderr, "expected 'send(2)' failure, got %zi\n",
>+			send_size);
>+	}
>+
>+	control_writeln("CLISENT");
>+
>+	free(data);
>+	close(fd);
>+}
>+
>+static void test_seqpacket_bigmsg_server(const struct test_opts *opts)
>+{
>+	int fd;
>+
>+	fd = vsock_seqpacket_accept(VMADDR_CID_ANY, 1234, NULL);
>+	if (fd < 0) {
>+		perror("accept");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	control_expectln("CLISENT");
>+
>+	close(fd);
>+}
>+
> #define BUF_PATTERN_1 'a'
> #define BUF_PATTERN_2 'b'
>
>@@ -832,6 +889,11 @@ static struct test_case test_cases[] = {
> 		.run_client = test_seqpacket_timeout_client,
> 		.run_server = test_seqpacket_timeout_server,
> 	},
>+	{
>+		.name = "SOCK_SEQPACKET big message",
>+		.run_client = test_seqpacket_bigmsg_client,
>+		.run_server = test_seqpacket_bigmsg_server,
>+	},

I would add new tests always at the end, so if some CI uses --skip, we 
don't have to update the scripts to skip some tests.

> 	{
> 		.name = "SOCK_SEQPACKET invalid receive buffer",
> 		.run_client = test_seqpacket_invalid_rec_buffer_client,
>-- 
>2.25.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC PATCH v1 2/3] test/vsock: add big message test

[Stefano Garzarella]

On Tue, Nov 15, 2022 at 08:52:35PM +0000, Arseniy Krasnov wrote:
>This adds test for sending message, bigger than peer's buffer size.
>For SOCK_SEQPACKET socket it must fail, as this type of socket has
>message size limit.
>
>Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>---
> tools/testing/vsock/vsock_test.c | 62 ++++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
>
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index 107c11165887..bb4e8657f1d6 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -560,6 +560,63 @@ static void test_seqpacket_timeout_server(const struct test_opts *opts)
> 	close(fd);
> }
>
>+static void test_seqpacket_bigmsg_client(const struct test_opts *opts)
>+{
>+	unsigned long sock_buf_size;
>+	ssize_t send_size;
>+	socklen_t len;
>+	void *data;
>+	int fd;
>+
>+	len = sizeof(sock_buf_size);
>+
>+	fd = vsock_seqpacket_connect(opts->peer_cid, 1234);

Not for this patch, but someday we should add a macro for this port and 
maybe even make it configurable :-)

>+	if (fd < 0) {
>+		perror("connect");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	if (getsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>+		       &sock_buf_size, &len)) {
>+		perror("getsockopt");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	sock_buf_size++;
>+
>+	data = malloc(sock_buf_size);
>+	if (!data) {
>+		perror("malloc");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	send_size = send(fd, data, sock_buf_size, 0);
>+	if (send_size != -1) {

Can we check also `errno`?
IIUC it should contains EMSGSIZE.

>+		fprintf(stderr, "expected 'send(2)' failure, got %zi\n",
>+			send_size);
>+	}
>+
>+	control_writeln("CLISENT");
>+
>+	free(data);
>+	close(fd);
>+}
>+
>+static void test_seqpacket_bigmsg_server(const struct test_opts *opts)
>+{
>+	int fd;
>+
>+	fd = vsock_seqpacket_accept(VMADDR_CID_ANY, 1234, NULL);
>+	if (fd < 0) {
>+		perror("accept");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	control_expectln("CLISENT");
>+
>+	close(fd);
>+}
>+
> #define BUF_PATTERN_1 'a'
> #define BUF_PATTERN_2 'b'
>
>@@ -832,6 +889,11 @@ static struct test_case test_cases[] = {
> 		.run_client = test_seqpacket_timeout_client,
> 		.run_server = test_seqpacket_timeout_server,
> 	},
>+	{
>+		.name = "SOCK_SEQPACKET big message",
>+		.run_client = test_seqpacket_bigmsg_client,
>+		.run_server = test_seqpacket_bigmsg_server,
>+	},

I would add new tests always at the end, so if some CI uses --skip, we 
don't have to update the scripts to skip some tests.

> 	{
> 		.name = "SOCK_SEQPACKET invalid receive buffer",
> 		.run_client = test_seqpacket_invalid_rec_buffer_client,
>-- 
>2.25.1

Re: [RFC PATCH v1 3/3] test/vsock: vsock_perf utility

[Stefano Garzarella]

On Tue, Nov 15, 2022 at 08:54:05PM +0000, Arseniy Krasnov wrote:
>This adds utility to check vsock receive performance.

A small example on how to use it here in the commit message would be 
nice.

>
>Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>---
> tools/testing/vsock/Makefile     |   1 +

Please, can you also update tools/testing/vsock/README ?

> tools/testing/vsock/vsock_perf.c | 386 +++++++++++++++++++++++++++++++
> 2 files changed, 387 insertions(+)
> create mode 100644 tools/testing/vsock/vsock_perf.c
>
>diff --git a/tools/testing/vsock/Makefile b/tools/testing/vsock/Makefile
>index f8293c6910c9..d36fdd59fe2e 100644
>--- a/tools/testing/vsock/Makefile
>+++ b/tools/testing/vsock/Makefile
>@@ -3,6 +3,7 @@ all: test
> test: vsock_test vsock_diag_test
> vsock_test: vsock_test.o timeout.o control.o util.o
> vsock_diag_test: vsock_diag_test.o timeout.o control.o util.o
>+vsock_perf: vsock_perf.o
>
> CFLAGS += -g -O2 -Werror -Wall -I. -I../../include -I../../../usr/include -Wno-pointer-sign -fno-strict-overflow -fno-strict-aliasing -fno-common -MMD -U_FORTIFY_SOURCE -D_GNU_SOURCE
> .PHONY: all test clean
>diff --git a/tools/testing/vsock/vsock_perf.c b/tools/testing/vsock/vsock_perf.c
>new file mode 100644
>index 000000000000..ca7af08dca46
>--- /dev/null
>+++ b/tools/testing/vsock/vsock_perf.c
>@@ -0,0 +1,386 @@
>+// SPDX-License-Identifier: GPL-2.0-only
>+/*
>+ * vsock_perf - benchmark utility for vsock.
>+ *
>+ * Copyright (C) 2022 SberDevices.
>+ *
>+ * Author: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>+ */
>+#include <getopt.h>
>+#include <stdio.h>
>+#include <stdlib.h>
>+#include <stdbool.h>
>+#include <string.h>
>+#include <errno.h>
>+#include <unistd.h>
>+#include <time.h>
>+#include <sys/mman.h>
>+#include <stdint.h>
>+#include <poll.h>
>+#include <sys/socket.h>
>+#include <linux/vm_sockets.h>
>+
>+#define DEFAULT_BUF_SIZE_BYTES	(128*1024)
>+#define DEFAULT_TO_SEND_BYTES	(64*1024)
>+#define DEFAULT_VSOCK_BUF_BYTES (256*1024)

checkpatch suggests to put spaces around '*' (e.g. 128 * 1024)

>+#define DEFAULT_RCVLOWAT_BYTES	1
>+#define DEFAULT_PORT		1234
>+
>+#define BYTES_PER_GB		(1024 * 1024 * 1024ULL)
>+#define NSEC_PER_SEC		(1000000000ULL)
>+
>+static unsigned int port = DEFAULT_PORT;
>+static unsigned long buf_size_bytes = DEFAULT_BUF_SIZE_BYTES;
>+static unsigned long vsock_buf_bytes = DEFAULT_VSOCK_BUF_BYTES;
>+
>+static inline time_t current_nsec(void)
>+{
>+	struct timespec ts;
>+
>+	if (clock_gettime(CLOCK_REALTIME, &ts)) {
>+		perror("clock_gettime");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	return (ts.tv_sec * NSEC_PER_SEC) + ts.tv_nsec;
>+}
>+
>+/* From lib/cmdline.c. */
>+static unsigned long memparse(const char *ptr)
>+{
>+	char *endptr;
>+
>+	unsigned long long ret = strtoull(ptr, &endptr, 0);
>+
>+	switch (*endptr) {
>+	case 'E':
>+	case 'e':
>+		ret <<= 10;
>+	case 'P':
>+	case 'p':
>+		ret <<= 10;
>+	case 'T':
>+	case 't':
>+		ret <<= 10;
>+	case 'G':
>+	case 'g':
>+		ret <<= 10;
>+	case 'M':
>+	case 'm':
>+		ret <<= 10;
>+	case 'K':
>+	case 'k':
>+		ret <<= 10;
>+		endptr++;
>+	default:
>+		break;
>+	}
>+
>+	return ret;
>+}
>+
>+static void vsock_increase_buf_size(int fd)
>+{
>+	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_MAX_SIZE,
>+		       &vsock_buf_bytes, sizeof(vsock_buf_bytes))) {
>+		perror("setsockopt");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>+		       &vsock_buf_bytes, sizeof(vsock_buf_bytes))) {
>+		perror("setsockopt");
>+		exit(EXIT_FAILURE);
>+	}
>+}
>+
>+static int vsock_connect(unsigned int cid, unsigned int port)
>+{
>+	union {
>+		struct sockaddr sa;
>+		struct sockaddr_vm svm;
>+	} addr = {
>+		.svm = {
>+			.svm_family = AF_VSOCK,
>+			.svm_port = port,
>+			.svm_cid = cid,
>+		},
>+	};
>+	int fd;
>+
>+	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
>+
>+	if (fd < 0)
>+		return -1;
>+
>+	vsock_increase_buf_size(fd);
>+
>+	if (connect(fd, &addr.sa, sizeof(addr.svm)) < 0) {
>+		close(fd);
>+		return -1;
>+	}
>+
>+	return fd;
>+}
>+
>+static float get_gbps(unsigned long bytes, time_t ns_delta)
>+{
>+	return ((float)bytes / BYTES_PER_GB) /
>+	       ((float)ns_delta / NSEC_PER_SEC);
>+}
>+
>+static void run_sender(unsigned long to_send_bytes)
>+{
>+	time_t tx_begin_ns;
>+	size_t total_send;
>+	int client_fd;
>+	char *data;
>+	int fd;
>+	union {
>+		struct sockaddr sa;
>+		struct sockaddr_vm svm;
>+	} addr = {
>+		.svm = {
>+			.svm_family = AF_VSOCK,
>+			.svm_port = port,
>+			.svm_cid = VMADDR_CID_ANY,
>+		},
>+	};
>+	union {
>+		struct sockaddr sa;
>+		struct sockaddr_vm svm;
>+	} clientaddr;
>+
>+	socklen_t clientaddr_len = sizeof(clientaddr.svm);
>+
>+	fprintf(stderr, "Run as sender\n");
>+	fprintf(stderr, "Listen port %u\n", port);
>+	fprintf(stderr, "Send %lu bytes\n", to_send_bytes);
>+	fprintf(stderr, "TX buffer %lu bytes\n", buf_size_bytes);
>+	fprintf(stderr, "Peer buffer %lu bytes\n", vsock_buf_bytes);

Why using stderr for this prints?

Also in other places, I think we can use stdout for this kind on prints.

>+
>+	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
>+
>+	if (fd < 0) {
>+		perror("socket");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	if (bind(fd, &addr.sa, sizeof(addr.svm)) < 0) {
>+		perror("bind");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	if (listen(fd, 1) < 0) {
>+		perror("listen");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	client_fd = accept(fd, &clientaddr.sa, &clientaddr_len);
>+
>+	if (client_fd < 0) {
>+		perror("accept");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	vsock_increase_buf_size(client_fd);
>+
>+	data = malloc(buf_size_bytes);
>+
>+	if (!data) {
>+		fprintf(stderr, "malloc failed\n");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	memset(data, 0, buf_size_bytes);
>+	total_send = 0;
>+	tx_begin_ns = current_nsec();
>+
>+	while (total_send < to_send_bytes) {
>+		ssize_t sent;
>+
>+		sent = write(client_fd, data, buf_size_bytes);
>+
>+		if (sent <= 0) {
>+			perror("write");
>+			exit(EXIT_FAILURE);
>+		}
>+
>+		total_send += sent;
>+	}
>+
>+	fprintf(stderr, "tx performance: %f Gb/s\n",
>+			get_gbps(total_send, current_nsec() - tx_begin_ns));

checkpatch suggests to align get_gbps with the open parenthesis.

>+
>+	close(client_fd);
>+	close(fd);
>+
>+	free(data);
>+}
>+
>+static void run_receiver(int peer_cid, unsigned long rcvlowat_bytes)
>+{
>+	unsigned int read_cnt;
>+	time_t rx_begin_ns;
>+	time_t in_read_ns;
>+	size_t total_recv;
>+	void *data;
>+	int fd;
>+
>+	fprintf(stderr, "Run as receiver\n");
>+	fprintf(stderr, "Connect to %i:%u\n", peer_cid, port);
>+	fprintf(stderr, "RX buffer %lu bytes\n", buf_size_bytes);
>+	fprintf(stderr, "Peer buffer %lu bytes\n", vsock_buf_bytes);
>+	fprintf(stderr, "SO_RCVLOWAT %lu bytes\n", rcvlowat_bytes);
>+
>+	fd = vsock_connect(peer_cid, port);
>+
>+	if (fd < 0) {
>+		perror("socket");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	if (setsockopt(fd, SOL_SOCKET, SO_RCVLOWAT,
>+		       &rcvlowat_bytes,
>+		       sizeof(rcvlowat_bytes))) {
>+		perror("setsockopt");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	data = mmap(NULL, buf_size_bytes, PROT_READ | PROT_WRITE,
>+		    MAP_ANONYMOUS | MAP_PRIVATE | MAP_POPULATE, -1, 0);
>+
>+	if (data == MAP_FAILED) {
>+		perror("mmap");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	read_cnt = 0;
>+	in_read_ns = 0;
>+	total_recv = 0;
>+	rx_begin_ns = current_nsec();
>+
>+	while (1) {
>+		struct pollfd fds = { 0 };
>+
>+		fds.fd = fd;
>+		fds.events = POLLIN | POLLERR | POLLHUP |
>+			     POLLRDHUP | POLLNVAL;
>+
>+		if (poll(&fds, 1, -1) < 0) {
>+			perror("poll");
>+			exit(EXIT_FAILURE);
>+		}
>+
>+		if (fds.revents & POLLERR) {
>+			fprintf(stderr, "'poll()' error\n");
>+			exit(EXIT_FAILURE);
>+		}
>+
>+		if (fds.revents & POLLIN) {
>+			ssize_t bytes_read;
>+			time_t t;
>+
>+			t = current_nsec();
>+			bytes_read = read(fd, data, buf_size_bytes);
>+			in_read_ns += (current_nsec() - t);
>+			read_cnt++;
>+
>+			if (!bytes_read)
>+				break;
>+
>+			if (bytes_read < 0) {
>+				perror("recv");
>+				exit(EXIT_FAILURE);
>+			}
>+
>+			total_recv += bytes_read;
>+		}
>+
>+		if (fds.revents & (POLLHUP | POLLRDHUP))
>+			break;
>+	}
>+
>+	fprintf(stderr, "rx performance %f Gb/s\n",
>+			get_gbps(total_recv, current_nsec() - rx_begin_ns));

checkpatch suggests to align get_gbps with the open parenthesis.

>+	fprintf(stderr, "total in 'read()' %f sec\n", (float)in_read_ns / NSEC_PER_SEC);
>+	fprintf(stderr, "POLLIN wakeups: %i\n", read_cnt);
>+	fprintf(stderr, "average in 'read()' %f ns\n", (float)in_read_ns / read_cnt);
>+
>+	munmap(data, buf_size_bytes);
>+	close(fd);
>+}
>+
>+static void usage(void)
>+{
>+	fprintf(stderr, "Help:\n"
>+			"\n"
>+			"This is benchmarking utility, to test vsock performance.\n"
>+			"It runs in two modes: sender or receiver. In sender mode, it waits\n"
>+			"connection from receiver, and when established, sender starts data\n"



>+			"transmission. Total size of data to send is set by '-m' option.\n"
>+			"\n"
>+			"Meaning of '-b' depends of sender or receiver mode. In sender\n"
>+			"mode, it is size of buffer, passed to 'write()'. In receiver mode,\n"
>+			"it is size of rx buffer passed to 'read()'.\n"

This part is better to put near -b description.

>+			"\n"
>+			"Options:\n"
>+			"  -h				This help message\n"
>+			"  -s				Sender mode(receiver default)\n"
>+			"  -p <port>			Port\n"
>+			"  -c <cid>			CID of the peer\n"
>+			"  -m <bytes to send>		Megabytes to send\n"
>+			"  -b <buffer size>		Depends on sender/receiver mode\n"
>+			"  -v <peer buffer size>	Peer buffer size\n"
>+			"  -r <SO_RCVLOWAT>		SO_RCVLOWAT\n"

Can you print also the default values?
(e.g. "-p <port>  Port (%d)\n" ..., DEFAULT_PORT)

>+			"\n");
>+	exit(EXIT_FAILURE);
>+}
>+
>+int main(int argc, char **argv)
>+{
>+	unsigned long to_send_bytes = DEFAULT_TO_SEND_BYTES;
>+	unsigned long rcvlowat_bytes = DEFAULT_RCVLOWAT_BYTES;
>+	bool receiver_mode = true;
>+	int peer_cid = -1;
>+	int c;
>+
>+	while ((c = getopt(argc, argv, "v:r:c:p:m:b:sh")) != -1) {
>+		switch (c) {
>+		case 'v': /* Peer buffer size. */
>+			vsock_buf_bytes = memparse(optarg);
>+			break;
>+		case 'r': /* SO_RCVLOWAT value. */
>+			rcvlowat_bytes = memparse(optarg);
>+			break;
>+		case 'c': /* CID to connect to. */
>+			peer_cid = atoi(optarg);

Maybe better to use strtol() to check errors.

>+			break;
>+		case 'p': /* Port to connect to. */
>+			port = atoi(optarg);
>+			break;
>+		case 'm': /* Bytes to send. */
>+			to_send_bytes = memparse(optarg);
>+			break;
>+		case 'b': /* Size of rx/tx buffer. */
>+			buf_size_bytes = memparse(optarg);
>+			break;
>+		case 's': /* Sender mode. */
>+			receiver_mode = false;
>+			break;
>+		case 'h': /* Help. */
>+			usage();
>+			break;
>+		default:
>+			usage();
>+

checkpatch: Blank lines aren't necessary before a close brace '}

>+		}
>+	}
>+
>+	if (receiver_mode)
>+		run_receiver(peer_cid, rcvlowat_bytes);
>+	else
>+		run_sender(to_send_bytes);
>+
>+	return 0;
>+}
>-- 
>2.25.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC PATCH v1 3/3] test/vsock: vsock_perf utility

[Stefano Garzarella]

On Tue, Nov 15, 2022 at 08:54:05PM +0000, Arseniy Krasnov wrote:
>This adds utility to check vsock receive performance.

A small example on how to use it here in the commit message would be 
nice.

>
>Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>---
> tools/testing/vsock/Makefile     |   1 +

Please, can you also update tools/testing/vsock/README ?

> tools/testing/vsock/vsock_perf.c | 386 +++++++++++++++++++++++++++++++
> 2 files changed, 387 insertions(+)
> create mode 100644 tools/testing/vsock/vsock_perf.c
>
>diff --git a/tools/testing/vsock/Makefile b/tools/testing/vsock/Makefile
>index f8293c6910c9..d36fdd59fe2e 100644
>--- a/tools/testing/vsock/Makefile
>+++ b/tools/testing/vsock/Makefile
>@@ -3,6 +3,7 @@ all: test
> test: vsock_test vsock_diag_test
> vsock_test: vsock_test.o timeout.o control.o util.o
> vsock_diag_test: vsock_diag_test.o timeout.o control.o util.o
>+vsock_perf: vsock_perf.o
>
> CFLAGS += -g -O2 -Werror -Wall -I. -I../../include -I../../../usr/include -Wno-pointer-sign -fno-strict-overflow -fno-strict-aliasing -fno-common -MMD -U_FORTIFY_SOURCE -D_GNU_SOURCE
> .PHONY: all test clean
>diff --git a/tools/testing/vsock/vsock_perf.c b/tools/testing/vsock/vsock_perf.c
>new file mode 100644
>index 000000000000..ca7af08dca46
>--- /dev/null
>+++ b/tools/testing/vsock/vsock_perf.c
>@@ -0,0 +1,386 @@
>+// SPDX-License-Identifier: GPL-2.0-only
>+/*
>+ * vsock_perf - benchmark utility for vsock.
>+ *
>+ * Copyright (C) 2022 SberDevices.
>+ *
>+ * Author: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>+ */
>+#include <getopt.h>
>+#include <stdio.h>
>+#include <stdlib.h>
>+#include <stdbool.h>
>+#include <string.h>
>+#include <errno.h>
>+#include <unistd.h>
>+#include <time.h>
>+#include <sys/mman.h>
>+#include <stdint.h>
>+#include <poll.h>
>+#include <sys/socket.h>
>+#include <linux/vm_sockets.h>
>+
>+#define DEFAULT_BUF_SIZE_BYTES	(128*1024)
>+#define DEFAULT_TO_SEND_BYTES	(64*1024)
>+#define DEFAULT_VSOCK_BUF_BYTES (256*1024)

checkpatch suggests to put spaces around '*' (e.g. 128 * 1024)

>+#define DEFAULT_RCVLOWAT_BYTES	1
>+#define DEFAULT_PORT		1234
>+
>+#define BYTES_PER_GB		(1024 * 1024 * 1024ULL)
>+#define NSEC_PER_SEC		(1000000000ULL)
>+
>+static unsigned int port = DEFAULT_PORT;
>+static unsigned long buf_size_bytes = DEFAULT_BUF_SIZE_BYTES;
>+static unsigned long vsock_buf_bytes = DEFAULT_VSOCK_BUF_BYTES;
>+
>+static inline time_t current_nsec(void)
>+{
>+	struct timespec ts;
>+
>+	if (clock_gettime(CLOCK_REALTIME, &ts)) {
>+		perror("clock_gettime");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	return (ts.tv_sec * NSEC_PER_SEC) + ts.tv_nsec;
>+}
>+
>+/* From lib/cmdline.c. */
>+static unsigned long memparse(const char *ptr)
>+{
>+	char *endptr;
>+
>+	unsigned long long ret = strtoull(ptr, &endptr, 0);
>+
>+	switch (*endptr) {
>+	case 'E':
>+	case 'e':
>+		ret <<= 10;
>+	case 'P':
>+	case 'p':
>+		ret <<= 10;
>+	case 'T':
>+	case 't':
>+		ret <<= 10;
>+	case 'G':
>+	case 'g':
>+		ret <<= 10;
>+	case 'M':
>+	case 'm':
>+		ret <<= 10;
>+	case 'K':
>+	case 'k':
>+		ret <<= 10;
>+		endptr++;
>+	default:
>+		break;
>+	}
>+
>+	return ret;
>+}
>+
>+static void vsock_increase_buf_size(int fd)
>+{
>+	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_MAX_SIZE,
>+		       &vsock_buf_bytes, sizeof(vsock_buf_bytes))) {
>+		perror("setsockopt");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>+		       &vsock_buf_bytes, sizeof(vsock_buf_bytes))) {
>+		perror("setsockopt");
>+		exit(EXIT_FAILURE);
>+	}
>+}
>+
>+static int vsock_connect(unsigned int cid, unsigned int port)
>+{
>+	union {
>+		struct sockaddr sa;
>+		struct sockaddr_vm svm;
>+	} addr = {
>+		.svm = {
>+			.svm_family = AF_VSOCK,
>+			.svm_port = port,
>+			.svm_cid = cid,
>+		},
>+	};
>+	int fd;
>+
>+	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
>+
>+	if (fd < 0)
>+		return -1;
>+
>+	vsock_increase_buf_size(fd);
>+
>+	if (connect(fd, &addr.sa, sizeof(addr.svm)) < 0) {
>+		close(fd);
>+		return -1;
>+	}
>+
>+	return fd;
>+}
>+
>+static float get_gbps(unsigned long bytes, time_t ns_delta)
>+{
>+	return ((float)bytes / BYTES_PER_GB) /
>+	       ((float)ns_delta / NSEC_PER_SEC);
>+}
>+
>+static void run_sender(unsigned long to_send_bytes)
>+{
>+	time_t tx_begin_ns;
>+	size_t total_send;
>+	int client_fd;
>+	char *data;
>+	int fd;
>+	union {
>+		struct sockaddr sa;
>+		struct sockaddr_vm svm;
>+	} addr = {
>+		.svm = {
>+			.svm_family = AF_VSOCK,
>+			.svm_port = port,
>+			.svm_cid = VMADDR_CID_ANY,
>+		},
>+	};
>+	union {
>+		struct sockaddr sa;
>+		struct sockaddr_vm svm;
>+	} clientaddr;
>+
>+	socklen_t clientaddr_len = sizeof(clientaddr.svm);
>+
>+	fprintf(stderr, "Run as sender\n");
>+	fprintf(stderr, "Listen port %u\n", port);
>+	fprintf(stderr, "Send %lu bytes\n", to_send_bytes);
>+	fprintf(stderr, "TX buffer %lu bytes\n", buf_size_bytes);
>+	fprintf(stderr, "Peer buffer %lu bytes\n", vsock_buf_bytes);

Why using stderr for this prints?

Also in other places, I think we can use stdout for this kind on prints.

>+
>+	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
>+
>+	if (fd < 0) {
>+		perror("socket");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	if (bind(fd, &addr.sa, sizeof(addr.svm)) < 0) {
>+		perror("bind");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	if (listen(fd, 1) < 0) {
>+		perror("listen");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	client_fd = accept(fd, &clientaddr.sa, &clientaddr_len);
>+
>+	if (client_fd < 0) {
>+		perror("accept");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	vsock_increase_buf_size(client_fd);
>+
>+	data = malloc(buf_size_bytes);
>+
>+	if (!data) {
>+		fprintf(stderr, "malloc failed\n");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	memset(data, 0, buf_size_bytes);
>+	total_send = 0;
>+	tx_begin_ns = current_nsec();
>+
>+	while (total_send < to_send_bytes) {
>+		ssize_t sent;
>+
>+		sent = write(client_fd, data, buf_size_bytes);
>+
>+		if (sent <= 0) {
>+			perror("write");
>+			exit(EXIT_FAILURE);
>+		}
>+
>+		total_send += sent;
>+	}
>+
>+	fprintf(stderr, "tx performance: %f Gb/s\n",
>+			get_gbps(total_send, current_nsec() - tx_begin_ns));

checkpatch suggests to align get_gbps with the open parenthesis.

>+
>+	close(client_fd);
>+	close(fd);
>+
>+	free(data);
>+}
>+
>+static void run_receiver(int peer_cid, unsigned long rcvlowat_bytes)
>+{
>+	unsigned int read_cnt;
>+	time_t rx_begin_ns;
>+	time_t in_read_ns;
>+	size_t total_recv;
>+	void *data;
>+	int fd;
>+
>+	fprintf(stderr, "Run as receiver\n");
>+	fprintf(stderr, "Connect to %i:%u\n", peer_cid, port);
>+	fprintf(stderr, "RX buffer %lu bytes\n", buf_size_bytes);
>+	fprintf(stderr, "Peer buffer %lu bytes\n", vsock_buf_bytes);
>+	fprintf(stderr, "SO_RCVLOWAT %lu bytes\n", rcvlowat_bytes);
>+
>+	fd = vsock_connect(peer_cid, port);
>+
>+	if (fd < 0) {
>+		perror("socket");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	if (setsockopt(fd, SOL_SOCKET, SO_RCVLOWAT,
>+		       &rcvlowat_bytes,
>+		       sizeof(rcvlowat_bytes))) {
>+		perror("setsockopt");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	data = mmap(NULL, buf_size_bytes, PROT_READ | PROT_WRITE,
>+		    MAP_ANONYMOUS | MAP_PRIVATE | MAP_POPULATE, -1, 0);
>+
>+	if (data == MAP_FAILED) {
>+		perror("mmap");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	read_cnt = 0;
>+	in_read_ns = 0;
>+	total_recv = 0;
>+	rx_begin_ns = current_nsec();
>+
>+	while (1) {
>+		struct pollfd fds = { 0 };
>+
>+		fds.fd = fd;
>+		fds.events = POLLIN | POLLERR | POLLHUP |
>+			     POLLRDHUP | POLLNVAL;
>+
>+		if (poll(&fds, 1, -1) < 0) {
>+			perror("poll");
>+			exit(EXIT_FAILURE);
>+		}
>+
>+		if (fds.revents & POLLERR) {
>+			fprintf(stderr, "'poll()' error\n");
>+			exit(EXIT_FAILURE);
>+		}
>+
>+		if (fds.revents & POLLIN) {
>+			ssize_t bytes_read;
>+			time_t t;
>+
>+			t = current_nsec();
>+			bytes_read = read(fd, data, buf_size_bytes);
>+			in_read_ns += (current_nsec() - t);
>+			read_cnt++;
>+
>+			if (!bytes_read)
>+				break;
>+
>+			if (bytes_read < 0) {
>+				perror("recv");
>+				exit(EXIT_FAILURE);
>+			}
>+
>+			total_recv += bytes_read;
>+		}
>+
>+		if (fds.revents & (POLLHUP | POLLRDHUP))
>+			break;
>+	}
>+
>+	fprintf(stderr, "rx performance %f Gb/s\n",
>+			get_gbps(total_recv, current_nsec() - rx_begin_ns));

checkpatch suggests to align get_gbps with the open parenthesis.

>+	fprintf(stderr, "total in 'read()' %f sec\n", (float)in_read_ns / NSEC_PER_SEC);
>+	fprintf(stderr, "POLLIN wakeups: %i\n", read_cnt);
>+	fprintf(stderr, "average in 'read()' %f ns\n", (float)in_read_ns / read_cnt);
>+
>+	munmap(data, buf_size_bytes);
>+	close(fd);
>+}
>+
>+static void usage(void)
>+{
>+	fprintf(stderr, "Help:\n"
>+			"\n"
>+			"This is benchmarking utility, to test vsock performance.\n"
>+			"It runs in two modes: sender or receiver. In sender mode, it waits\n"
>+			"connection from receiver, and when established, sender starts data\n"



>+			"transmission. Total size of data to send is set by '-m' option.\n"
>+			"\n"
>+			"Meaning of '-b' depends of sender or receiver mode. In sender\n"
>+			"mode, it is size of buffer, passed to 'write()'. In receiver mode,\n"
>+			"it is size of rx buffer passed to 'read()'.\n"

This part is better to put near -b description.

>+			"\n"
>+			"Options:\n"
>+			"  -h				This help message\n"
>+			"  -s				Sender mode(receiver default)\n"
>+			"  -p <port>			Port\n"
>+			"  -c <cid>			CID of the peer\n"
>+			"  -m <bytes to send>		Megabytes to send\n"
>+			"  -b <buffer size>		Depends on sender/receiver mode\n"
>+			"  -v <peer buffer size>	Peer buffer size\n"
>+			"  -r <SO_RCVLOWAT>		SO_RCVLOWAT\n"

Can you print also the default values?
(e.g. "-p <port>  Port (%d)\n" ..., DEFAULT_PORT)

>+			"\n");
>+	exit(EXIT_FAILURE);
>+}
>+
>+int main(int argc, char **argv)
>+{
>+	unsigned long to_send_bytes = DEFAULT_TO_SEND_BYTES;
>+	unsigned long rcvlowat_bytes = DEFAULT_RCVLOWAT_BYTES;
>+	bool receiver_mode = true;
>+	int peer_cid = -1;
>+	int c;
>+
>+	while ((c = getopt(argc, argv, "v:r:c:p:m:b:sh")) != -1) {
>+		switch (c) {
>+		case 'v': /* Peer buffer size. */
>+			vsock_buf_bytes = memparse(optarg);
>+			break;
>+		case 'r': /* SO_RCVLOWAT value. */
>+			rcvlowat_bytes = memparse(optarg);
>+			break;
>+		case 'c': /* CID to connect to. */
>+			peer_cid = atoi(optarg);

Maybe better to use strtol() to check errors.

>+			break;
>+		case 'p': /* Port to connect to. */
>+			port = atoi(optarg);
>+			break;
>+		case 'm': /* Bytes to send. */
>+			to_send_bytes = memparse(optarg);
>+			break;
>+		case 'b': /* Size of rx/tx buffer. */
>+			buf_size_bytes = memparse(optarg);
>+			break;
>+		case 's': /* Sender mode. */
>+			receiver_mode = false;
>+			break;
>+		case 'h': /* Help. */
>+			usage();
>+			break;
>+		default:
>+			usage();
>+

checkpatch: Blank lines aren't necessary before a close brace '}

>+		}
>+	}
>+
>+	if (receiver_mode)
>+		run_receiver(peer_cid, rcvlowat_bytes);
>+	else
>+		run_sender(to_send_bytes);
>+
>+	return 0;
>+}
>-- 
>2.25.1

Re: [RFC PATCH v1 1/3] test/vsock: rework message bound test

[Arseniy Krasnov]

On 21.11.2022 17:46, Stefano Garzarella wrote:
> On Tue, Nov 15, 2022 at 08:50:36PM +0000, Arseniy Krasnov wrote:
>> This updates message bound test making it more complex. Instead of
>> sending 1 bytes messages with one MSG_EOR bit, it sends messages of
>> random length(one half of messages are smaller than page size, second
>> half are bigger) with random number of MSG_EOR bits set. Receiver
>> also don't know total number of messages.
>>
>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>> ---
>> tools/testing/vsock/control.c    |  34 +++++++++
>> tools/testing/vsock/control.h    |   2 +
>> tools/testing/vsock/util.c       |  13 ++++
>> tools/testing/vsock/util.h       |   1 +
>> tools/testing/vsock/vsock_test.c | 115 +++++++++++++++++++++++++++----
>> 5 files changed, 152 insertions(+), 13 deletions(-)
>>
>> diff --git a/tools/testing/vsock/control.c b/tools/testing/vsock/control.c
>> index 4874872fc5a3..bed1649bdf3d 100644
>> --- a/tools/testing/vsock/control.c
>> +++ b/tools/testing/vsock/control.c
>> @@ -141,6 +141,40 @@ void control_writeln(const char *str)
>>     timeout_end();
>> }
>>
>> +void control_writeulong(unsigned long value)
>> +{
>> +    char str[32];
>> +
>> +    if (snprintf(str, sizeof(str), "%lu", value) >= sizeof(str)) {
>> +        perror("snprintf");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>> +    control_writeln(str);
>> +}
>> +
>> +unsigned long control_readulong(bool *ok)
>> +{
>> +    unsigned long value;
>> +    char *str;
>> +
>> +    if (ok)
>> +        *ok = false;
>> +
>> +    str = control_readln();
>> +
>> +    if (str == NULL)
> 
> checkpatch suggests to use !str
> 
>> +        return 0;
> 
> Maybe we can just call exit(EXIT_FAILURE) here and remove the `ok`
> parameter, since I'm not sure we can recover from this error.
> 
>> +
>> +    value = strtoul(str, NULL, 10);
>> +    free(str);
>> +
>> +    if (ok)
>> +        *ok = true;
>> +
>> +    return value;
>> +}
>> +
>> /* Return the next line from the control socket (without the trailing newline).
>>  *
>>  * The program terminates if a timeout occurs.
>> diff --git a/tools/testing/vsock/control.h b/tools/testing/vsock/control.h
>> index 51814b4f9ac1..cdd922dfea68 100644
>> --- a/tools/testing/vsock/control.h
>> +++ b/tools/testing/vsock/control.h
>> @@ -9,7 +9,9 @@ void control_init(const char *control_host, const char *control_port,
>> void control_cleanup(void);
>> void control_writeln(const char *str);
>> char *control_readln(void);
>> +unsigned long control_readulong(bool *ok);
>> void control_expectln(const char *str);
>> bool control_cmpln(char *line, const char *str, bool fail);
>> +void control_writeulong(unsigned long value);
>>
>> #endif /* CONTROL_H */
>> diff --git a/tools/testing/vsock/util.c b/tools/testing/vsock/util.c
>> index 2acbb7703c6a..351903836774 100644
>> --- a/tools/testing/vsock/util.c
>> +++ b/tools/testing/vsock/util.c
>> @@ -395,3 +395,16 @@ void skip_test(struct test_case *test_cases, size_t test_cases_len,
>>
>>     test_cases[test_id].skip = true;
>> }
>> +
>> +unsigned long djb2(const void *data, size_t len)
> 
> I would add hash_ as a prefix (or suffix).
> 
>> +{
>> +    unsigned long hash = 5381;
>> +    int i = 0;
>> +
>> +    while (i < len) {
>> +        hash = ((hash << 5) + hash) + ((unsigned char *)data)[i];
>> +        i++;
>> +    }
>> +
>> +    return hash;
>> +}
>> diff --git a/tools/testing/vsock/util.h b/tools/testing/vsock/util.h
>> index a3375ad2fb7f..988cc69a4642 100644
>> --- a/tools/testing/vsock/util.h
>> +++ b/tools/testing/vsock/util.h
>> @@ -49,4 +49,5 @@ void run_tests(const struct test_case *test_cases,
>> void list_tests(const struct test_case *test_cases);
>> void skip_test(struct test_case *test_cases, size_t test_cases_len,
>>            const char *test_id_str);
>> +unsigned long djb2(const void *data, size_t len);
>> #endif /* UTIL_H */
>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>> index bb6d691cb30d..107c11165887 100644
>> --- a/tools/testing/vsock/vsock_test.c
>> +++ b/tools/testing/vsock/vsock_test.c
>> @@ -284,10 +284,14 @@ static void test_stream_msg_peek_server(const struct test_opts *opts)
>>     close(fd);
>> }
>>
>> -#define MESSAGES_CNT 7
>> -#define MSG_EOR_IDX (MESSAGES_CNT / 2)
>> +#define SOCK_BUF_SIZE (2 * 1024 * 1024)
>> +#define MAX_MSG_SIZE (32 * 1024)
>> +
>> static void test_seqpacket_msg_bounds_client(const struct test_opts *opts)
>> {
>> +    unsigned long curr_hash;
>> +    int page_size;
>> +    int msg_count;
>>     int fd;
>>
>>     fd = vsock_seqpacket_connect(opts->peer_cid, 1234);
>> @@ -296,18 +300,69 @@ static void test_seqpacket_msg_bounds_client(const struct test_opts *opts)
>>         exit(EXIT_FAILURE);
>>     }
>>
>> -    /* Send several messages, one with MSG_EOR flag */
>> -    for (int i = 0; i < MESSAGES_CNT; i++)
>> -        send_byte(fd, 1, (i == MSG_EOR_IDX) ? MSG_EOR : 0);
>> +    /* Wait, until receiver sets buffer size. */
>> +    control_expectln("SRVREADY");
>> +
>> +    curr_hash = 0;
>> +    page_size = getpagesize();
>> +    msg_count = SOCK_BUF_SIZE / MAX_MSG_SIZE;
>> +
>> +    for (int i = 0; i < msg_count; i++) {
>> +        ssize_t send_size;
>> +        size_t buf_size;
>> +        int flags;
>> +        void *buf;
>> +
>> +        /* Use "small" buffers and "big" buffers. */
>> +        if (i & 1)
>> +            buf_size = page_size +
>> +                    (rand() % (MAX_MSG_SIZE - page_size));
>> +        else
>> +            buf_size = 1 + (rand() % page_size);
>> +
>> +        buf = malloc(buf_size);
>> +
>> +        if (!buf) {
>> +            perror("malloc");
>> +            exit(EXIT_FAILURE);
>> +        }
>> +
>> +        /* Set at least one MSG_EOR + some random. */
>> +        if (i == (msg_count / 2) || (rand() & 1)) {
>> +            flags = MSG_EOR;
>> +            curr_hash++;
>> +        } else {
>> +            flags = 0;
>> +        }
>> +
>> +        send_size = send(fd, buf, buf_size, flags);
>> +
>> +        if (send_size < 0) {
>> +            perror("send");
>> +            exit(EXIT_FAILURE);
>> +        }
>> +
>> +        if (send_size != buf_size) {
>> +            fprintf(stderr, "Invalid send size\n");
>> +            exit(EXIT_FAILURE);
>> +        }
>> +
>> +        curr_hash += send_size;
>> +        curr_hash = djb2(&curr_hash, sizeof(curr_hash));
>> +    }
>>
>>     control_writeln("SENDDONE");
>> +    control_writeulong(curr_hash);
> 
> Why do we need to hash the size?
> 
> Maybe we can send it without making the hash, anyway even if it wraps,
> it should wrap the same way in both the server and the client.
> (Or maybe we can use uin32_t or uint64_t to make sure both were
> using 4 or 8 bytes)
Hello, thanks for review. I think if we will use sum of message size(IIUC), in most
paranoic case it won't guarantee message bounds control: single 4 bytes message
could be read as 4 x 1 byte message(IIUC of course). Idea of hashing is simple:
every iteration we do current_hash = hash(previous_hash + size of current message);
I think this is more reliable and protects from case described above.

All other comments - ack.
> 
>>     close(fd);
>> }
>>
>> static void test_seqpacket_msg_bounds_server(const struct test_opts *opts)
>> {
>> +    unsigned long sock_buf_size;
>> +    unsigned long remote_hash;
>> +    unsigned long curr_hash;
>>     int fd;
>> -    char buf[16];
>> +    char buf[MAX_MSG_SIZE];
>>     struct msghdr msg = {0};
>>     struct iovec iov = {0};
>>
>> @@ -317,25 +372,58 @@ static void test_seqpacket_msg_bounds_server(const struct test_opts *opts)
>>         exit(EXIT_FAILURE);
>>     }
>>
>> +    sock_buf_size = SOCK_BUF_SIZE;
>> +
>> +    if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_MAX_SIZE,
>> +               &sock_buf_size, sizeof(sock_buf_size))) {
>> +        perror("getsockopt");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>> +    if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>> +               &sock_buf_size, sizeof(sock_buf_size))) {
>> +        perror("getsockopt");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>> +    /* Ready to receive data. */
>> +    control_writeln("SRVREADY");
>> +    /* Wait, until peer sends whole data. */
>>     control_expectln("SENDDONE");
>>     iov.iov_base = buf;
>>     iov.iov_len = sizeof(buf);
>>     msg.msg_iov = &iov;
>>     msg.msg_iovlen = 1;
>>
>> -    for (int i = 0; i < MESSAGES_CNT; i++) {
>> -        if (recvmsg(fd, &msg, 0) != 1) {
>> -            perror("message bound violated");
>> -            exit(EXIT_FAILURE);
>> -        }
>> +    curr_hash = 0;
>>
>> -        if ((i == MSG_EOR_IDX) ^ !!(msg.msg_flags & MSG_EOR)) {
>> -            perror("MSG_EOR");
>> +    while (1) {
>> +        ssize_t recv_size;
>> +
>> +        recv_size = recvmsg(fd, &msg, 0);
>> +
>> +        if (!recv_size)
>> +            break;
>> +
>> +        if (recv_size < 0) {
>> +            perror("recvmsg");
>>             exit(EXIT_FAILURE);
>>         }
>> +
>> +        if (msg.msg_flags & MSG_EOR)
>> +            curr_hash++;
>> +
>> +        curr_hash += recv_size;
>> +        curr_hash = djb2(&curr_hash, sizeof(curr_hash));
>>     }
>>
>>     close(fd);
>> +    remote_hash = control_readulong(NULL);
>> +
>> +    if (curr_hash != remote_hash) {
>> +        fprintf(stderr, "Message bounds broken\n");
>> +        exit(EXIT_FAILURE);
>> +    }
>> }
>>
>> #define MESSAGE_TRUNC_SZ 32
>> @@ -837,6 +925,7 @@ int main(int argc, char **argv)
>>         .peer_cid = VMADDR_CID_ANY,
>>     };
>>
>> +    srand(time(NULL));
>>     init_signals();
>>
>>     for (;;) {
>> -- 
>> 2.25.1
> 

Re: [RFC PATCH v1 2/3] test/vsock: add big message test

[Arseniy Krasnov]

On 21.11.2022 17:52, Stefano Garzarella wrote:
> On Tue, Nov 15, 2022 at 08:52:35PM +0000, Arseniy Krasnov wrote:
>> This adds test for sending message, bigger than peer's buffer size.
>> For SOCK_SEQPACKET socket it must fail, as this type of socket has
>> message size limit.
>>
>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>> ---
>> tools/testing/vsock/vsock_test.c | 62 ++++++++++++++++++++++++++++++++
>> 1 file changed, 62 insertions(+)
>>
>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>> index 107c11165887..bb4e8657f1d6 100644
>> --- a/tools/testing/vsock/vsock_test.c
>> +++ b/tools/testing/vsock/vsock_test.c
>> @@ -560,6 +560,63 @@ static void test_seqpacket_timeout_server(const struct test_opts *opts)
>>     close(fd);
>> }
>>
>> +static void test_seqpacket_bigmsg_client(const struct test_opts *opts)
>> +{
>> +    unsigned long sock_buf_size;
>> +    ssize_t send_size;
>> +    socklen_t len;
>> +    void *data;
>> +    int fd;
>> +
>> +    len = sizeof(sock_buf_size);
>> +
>> +    fd = vsock_seqpacket_connect(opts->peer_cid, 1234);
> 
> Not for this patch, but someday we should add a macro for this port and maybe even make it configurable :-)
> 
>> +    if (fd < 0) {
>> +        perror("connect");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>> +    if (getsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>> +               &sock_buf_size, &len)) {
>> +        perror("getsockopt");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>> +    sock_buf_size++;
>> +
>> +    data = malloc(sock_buf_size);
>> +    if (!data) {
>> +        perror("malloc");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>> +    send_size = send(fd, data, sock_buf_size, 0);
>> +    if (send_size != -1) {
> 
> Can we check also `errno`?
> IIUC it should contains EMSGSIZE.
> 
>> +        fprintf(stderr, "expected 'send(2)' failure, got %zi\n",
>> +            send_size);
>> +    }
>> +
>> +    control_writeln("CLISENT");
>> +
>> +    free(data);
>> +    close(fd);
>> +}
>> +
>> +static void test_seqpacket_bigmsg_server(const struct test_opts *opts)
>> +{
>> +    int fd;
>> +
>> +    fd = vsock_seqpacket_accept(VMADDR_CID_ANY, 1234, NULL);
>> +    if (fd < 0) {
>> +        perror("accept");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>> +    control_expectln("CLISENT");
>> +
>> +    close(fd);
>> +}
>> +
>> #define BUF_PATTERN_1 'a'
>> #define BUF_PATTERN_2 'b'
>>
>> @@ -832,6 +889,11 @@ static struct test_case test_cases[] = {
>>         .run_client = test_seqpacket_timeout_client,
>>         .run_server = test_seqpacket_timeout_server,
>>     },
>> +    {
>> +        .name = "SOCK_SEQPACKET big message",
>> +        .run_client = test_seqpacket_bigmsg_client,
>> +        .run_server = test_seqpacket_bigmsg_server,
>> +    },
> 
> I would add new tests always at the end, so if some CI uses --skip, we don't have to update the scripts to skip some tests.
Ack this and all above
> 
>>     {
>>         .name = "SOCK_SEQPACKET invalid receive buffer",
>>         .run_client = test_seqpacket_invalid_rec_buffer_client,
>> -- 
>> 2.25.1
> 

Re: [RFC PATCH v1 3/3] test/vsock: vsock_perf utility

[Arseniy Krasnov]

On 21.11.2022 18:28, Stefano Garzarella wrote:
> On Tue, Nov 15, 2022 at 08:54:05PM +0000, Arseniy Krasnov wrote:
>> This adds utility to check vsock receive performance.
> 
> A small example on how to use it here in the commit message would be nice.
> 
>>
>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>> ---
>> tools/testing/vsock/Makefile     |   1 +
> 
> Please, can you also update tools/testing/vsock/README ?
> 
>> tools/testing/vsock/vsock_perf.c | 386 +++++++++++++++++++++++++++++++
>> 2 files changed, 387 insertions(+)
>> create mode 100644 tools/testing/vsock/vsock_perf.c
>>
>> diff --git a/tools/testing/vsock/Makefile b/tools/testing/vsock/Makefile
>> index f8293c6910c9..d36fdd59fe2e 100644
>> --- a/tools/testing/vsock/Makefile
>> +++ b/tools/testing/vsock/Makefile
>> @@ -3,6 +3,7 @@ all: test
>> test: vsock_test vsock_diag_test
>> vsock_test: vsock_test.o timeout.o control.o util.o
>> vsock_diag_test: vsock_diag_test.o timeout.o control.o util.o
>> +vsock_perf: vsock_perf.o
>>
>> CFLAGS += -g -O2 -Werror -Wall -I. -I../../include -I../../../usr/include -Wno-pointer-sign -fno-strict-overflow -fno-strict-aliasing -fno-common -MMD -U_FORTIFY_SOURCE -D_GNU_SOURCE
>> .PHONY: all test clean
>> diff --git a/tools/testing/vsock/vsock_perf.c b/tools/testing/vsock/vsock_perf.c
>> new file mode 100644
>> index 000000000000..ca7af08dca46
>> --- /dev/null
>> +++ b/tools/testing/vsock/vsock_perf.c
>> @@ -0,0 +1,386 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * vsock_perf - benchmark utility for vsock.
>> + *
>> + * Copyright (C) 2022 SberDevices.
>> + *
>> + * Author: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>> + */
>> +#include <getopt.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <stdbool.h>
>> +#include <string.h>
>> +#include <errno.h>
>> +#include <unistd.h>
>> +#include <time.h>
>> +#include <sys/mman.h>
>> +#include <stdint.h>
>> +#include <poll.h>
>> +#include <sys/socket.h>
>> +#include <linux/vm_sockets.h>
>> +
>> +#define DEFAULT_BUF_SIZE_BYTES    (128*1024)
>> +#define DEFAULT_TO_SEND_BYTES    (64*1024)
>> +#define DEFAULT_VSOCK_BUF_BYTES (256*1024)
> 
> checkpatch suggests to put spaces around '*' (e.g. 128 * 1024)
> 
>> +#define DEFAULT_RCVLOWAT_BYTES    1
>> +#define DEFAULT_PORT        1234
>> +
>> +#define BYTES_PER_GB        (1024 * 1024 * 1024ULL)
>> +#define NSEC_PER_SEC        (1000000000ULL)
>> +
>> +static unsigned int port = DEFAULT_PORT;
>> +static unsigned long buf_size_bytes = DEFAULT_BUF_SIZE_BYTES;
>> +static unsigned long vsock_buf_bytes = DEFAULT_VSOCK_BUF_BYTES;
>> +
>> +static inline time_t current_nsec(void)
>> +{
>> +    struct timespec ts;
>> +
>> +    if (clock_gettime(CLOCK_REALTIME, &ts)) {
>> +        perror("clock_gettime");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>> +    return (ts.tv_sec * NSEC_PER_SEC) + ts.tv_nsec;
>> +}
>> +
>> +/* From lib/cmdline.c. */
>> +static unsigned long memparse(const char *ptr)
>> +{
>> +    char *endptr;
>> +
>> +    unsigned long long ret = strtoull(ptr, &endptr, 0);
>> +
>> +    switch (*endptr) {
>> +    case 'E':
>> +    case 'e':
>> +        ret <<= 10;
>> +    case 'P':
>> +    case 'p':
>> +        ret <<= 10;
>> +    case 'T':
>> +    case 't':
>> +        ret <<= 10;
>> +    case 'G':
>> +    case 'g':
>> +        ret <<= 10;
>> +    case 'M':
>> +    case 'm':
>> +        ret <<= 10;
>> +    case 'K':
>> +    case 'k':
>> +        ret <<= 10;
>> +        endptr++;
>> +    default:
>> +        break;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static void vsock_increase_buf_size(int fd)
>> +{
>> +    if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_MAX_SIZE,
>> +               &vsock_buf_bytes, sizeof(vsock_buf_bytes))) {
>> +        perror("setsockopt");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>> +    if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>> +               &vsock_buf_bytes, sizeof(vsock_buf_bytes))) {
>> +        perror("setsockopt");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +}
>> +
>> +static int vsock_connect(unsigned int cid, unsigned int port)
>> +{
>> +    union {
>> +        struct sockaddr sa;
>> +        struct sockaddr_vm svm;
>> +    } addr = {
>> +        .svm = {
>> +            .svm_family = AF_VSOCK,
>> +            .svm_port = port,
>> +            .svm_cid = cid,
>> +        },
>> +    };
>> +    int fd;
>> +
>> +    fd = socket(AF_VSOCK, SOCK_STREAM, 0);
>> +
>> +    if (fd < 0)
>> +        return -1;
>> +
>> +    vsock_increase_buf_size(fd);
>> +
>> +    if (connect(fd, &addr.sa, sizeof(addr.svm)) < 0) {
>> +        close(fd);
>> +        return -1;
>> +    }
>> +
>> +    return fd;
>> +}
>> +
>> +static float get_gbps(unsigned long bytes, time_t ns_delta)
>> +{
>> +    return ((float)bytes / BYTES_PER_GB) /
>> +           ((float)ns_delta / NSEC_PER_SEC);
>> +}
>> +
>> +static void run_sender(unsigned long to_send_bytes)
>> +{
>> +    time_t tx_begin_ns;
>> +    size_t total_send;
>> +    int client_fd;
>> +    char *data;
>> +    int fd;
>> +    union {
>> +        struct sockaddr sa;
>> +        struct sockaddr_vm svm;
>> +    } addr = {
>> +        .svm = {
>> +            .svm_family = AF_VSOCK,
>> +            .svm_port = port,
>> +            .svm_cid = VMADDR_CID_ANY,
>> +        },
>> +    };
>> +    union {
>> +        struct sockaddr sa;
>> +        struct sockaddr_vm svm;
>> +    } clientaddr;
>> +
>> +    socklen_t clientaddr_len = sizeof(clientaddr.svm);
>> +
>> +    fprintf(stderr, "Run as sender\n");
>> +    fprintf(stderr, "Listen port %u\n", port);
>> +    fprintf(stderr, "Send %lu bytes\n", to_send_bytes);
>> +    fprintf(stderr, "TX buffer %lu bytes\n", buf_size_bytes);
>> +    fprintf(stderr, "Peer buffer %lu bytes\n", vsock_buf_bytes);
> 
> Why using stderr for this prints?
> 
> Also in other places, I think we can use stdout for this kind on prints.
> 
>> +
>> +    fd = socket(AF_VSOCK, SOCK_STREAM, 0);
>> +
>> +    if (fd < 0) {
>> +        perror("socket");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>> +    if (bind(fd, &addr.sa, sizeof(addr.svm)) < 0) {
>> +        perror("bind");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>> +    if (listen(fd, 1) < 0) {
>> +        perror("listen");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>> +    client_fd = accept(fd, &clientaddr.sa, &clientaddr_len);
>> +
>> +    if (client_fd < 0) {
>> +        perror("accept");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>> +    vsock_increase_buf_size(client_fd);
>> +
>> +    data = malloc(buf_size_bytes);
>> +
>> +    if (!data) {
>> +        fprintf(stderr, "malloc failed\n");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>> +    memset(data, 0, buf_size_bytes);
>> +    total_send = 0;
>> +    tx_begin_ns = current_nsec();
>> +
>> +    while (total_send < to_send_bytes) {
>> +        ssize_t sent;
>> +
>> +        sent = write(client_fd, data, buf_size_bytes);
>> +
>> +        if (sent <= 0) {
>> +            perror("write");
>> +            exit(EXIT_FAILURE);
>> +        }
>> +
>> +        total_send += sent;
>> +    }
>> +
>> +    fprintf(stderr, "tx performance: %f Gb/s\n",
>> +            get_gbps(total_send, current_nsec() - tx_begin_ns));
> 
> checkpatch suggests to align get_gbps with the open parenthesis.
> 
>> +
>> +    close(client_fd);
>> +    close(fd);
>> +
>> +    free(data);
>> +}
>> +
>> +static void run_receiver(int peer_cid, unsigned long rcvlowat_bytes)
>> +{
>> +    unsigned int read_cnt;
>> +    time_t rx_begin_ns;
>> +    time_t in_read_ns;
>> +    size_t total_recv;
>> +    void *data;
>> +    int fd;
>> +
>> +    fprintf(stderr, "Run as receiver\n");
>> +    fprintf(stderr, "Connect to %i:%u\n", peer_cid, port);
>> +    fprintf(stderr, "RX buffer %lu bytes\n", buf_size_bytes);
>> +    fprintf(stderr, "Peer buffer %lu bytes\n", vsock_buf_bytes);
>> +    fprintf(stderr, "SO_RCVLOWAT %lu bytes\n", rcvlowat_bytes);
>> +
>> +    fd = vsock_connect(peer_cid, port);
>> +
>> +    if (fd < 0) {
>> +        perror("socket");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>> +    if (setsockopt(fd, SOL_SOCKET, SO_RCVLOWAT,
>> +               &rcvlowat_bytes,
>> +               sizeof(rcvlowat_bytes))) {
>> +        perror("setsockopt");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>> +    data = mmap(NULL, buf_size_bytes, PROT_READ | PROT_WRITE,
>> +            MAP_ANONYMOUS | MAP_PRIVATE | MAP_POPULATE, -1, 0);
>> +
>> +    if (data == MAP_FAILED) {
>> +        perror("mmap");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>> +    read_cnt = 0;
>> +    in_read_ns = 0;
>> +    total_recv = 0;
>> +    rx_begin_ns = current_nsec();
>> +
>> +    while (1) {
>> +        struct pollfd fds = { 0 };
>> +
>> +        fds.fd = fd;
>> +        fds.events = POLLIN | POLLERR | POLLHUP |
>> +                 POLLRDHUP | POLLNVAL;
>> +
>> +        if (poll(&fds, 1, -1) < 0) {
>> +            perror("poll");
>> +            exit(EXIT_FAILURE);
>> +        }
>> +
>> +        if (fds.revents & POLLERR) {
>> +            fprintf(stderr, "'poll()' error\n");
>> +            exit(EXIT_FAILURE);
>> +        }
>> +
>> +        if (fds.revents & POLLIN) {
>> +            ssize_t bytes_read;
>> +            time_t t;
>> +
>> +            t = current_nsec();
>> +            bytes_read = read(fd, data, buf_size_bytes);
>> +            in_read_ns += (current_nsec() - t);
>> +            read_cnt++;
>> +
>> +            if (!bytes_read)
>> +                break;
>> +
>> +            if (bytes_read < 0) {
>> +                perror("recv");
>> +                exit(EXIT_FAILURE);
>> +            }
>> +
>> +            total_recv += bytes_read;
>> +        }
>> +
>> +        if (fds.revents & (POLLHUP | POLLRDHUP))
>> +            break;
>> +    }
>> +
>> +    fprintf(stderr, "rx performance %f Gb/s\n",
>> +            get_gbps(total_recv, current_nsec() - rx_begin_ns));
> 
> checkpatch suggests to align get_gbps with the open parenthesis.
> 
>> +    fprintf(stderr, "total in 'read()' %f sec\n", (float)in_read_ns / NSEC_PER_SEC);
>> +    fprintf(stderr, "POLLIN wakeups: %i\n", read_cnt);
>> +    fprintf(stderr, "average in 'read()' %f ns\n", (float)in_read_ns / read_cnt);
>> +
>> +    munmap(data, buf_size_bytes);
>> +    close(fd);
>> +}
>> +
>> +static void usage(void)
>> +{
>> +    fprintf(stderr, "Help:\n"
>> +            "\n"
>> +            "This is benchmarking utility, to test vsock performance.\n"
>> +            "It runs in two modes: sender or receiver. In sender mode, it waits\n"
>> +            "connection from receiver, and when established, sender starts data\n"
> 
> 
> 
>> +            "transmission. Total size of data to send is set by '-m' option.\n"
>> +            "\n"
>> +            "Meaning of '-b' depends of sender or receiver mode. In sender\n"
>> +            "mode, it is size of buffer, passed to 'write()'. In receiver mode,\n"
>> +            "it is size of rx buffer passed to 'read()'.\n"
> 
> This part is better to put near -b description.
> 
>> +            "\n"
>> +            "Options:\n"
>> +            "  -h                This help message\n"
>> +            "  -s                Sender mode(receiver default)\n"
>> +            "  -p <port>            Port\n"
>> +            "  -c <cid>            CID of the peer\n"
>> +            "  -m <bytes to send>        Megabytes to send\n"
>> +            "  -b <buffer size>        Depends on sender/receiver mode\n"
>> +            "  -v <peer buffer size>    Peer buffer size\n"
>> +            "  -r <SO_RCVLOWAT>        SO_RCVLOWAT\n"
> 
> Can you print also the default values?
> (e.g. "-p <port>  Port (%d)\n" ..., DEFAULT_PORT)
> 
>> +            "\n");
>> +    exit(EXIT_FAILURE);
>> +}
>> +
>> +int main(int argc, char **argv)
>> +{
>> +    unsigned long to_send_bytes = DEFAULT_TO_SEND_BYTES;
>> +    unsigned long rcvlowat_bytes = DEFAULT_RCVLOWAT_BYTES;
>> +    bool receiver_mode = true;
>> +    int peer_cid = -1;
>> +    int c;
>> +
>> +    while ((c = getopt(argc, argv, "v:r:c:p:m:b:sh")) != -1) {
>> +        switch (c) {
>> +        case 'v': /* Peer buffer size. */
>> +            vsock_buf_bytes = memparse(optarg);
>> +            break;
>> +        case 'r': /* SO_RCVLOWAT value. */
>> +            rcvlowat_bytes = memparse(optarg);
>> +            break;
>> +        case 'c': /* CID to connect to. */
>> +            peer_cid = atoi(optarg);
> 
> Maybe better to use strtol() to check errors.
> 
>> +            break;
>> +        case 'p': /* Port to connect to. */
>> +            port = atoi(optarg);
>> +            break;
>> +        case 'm': /* Bytes to send. */
>> +            to_send_bytes = memparse(optarg);
>> +            break;
>> +        case 'b': /* Size of rx/tx buffer. */
>> +            buf_size_bytes = memparse(optarg);
>> +            break;
>> +        case 's': /* Sender mode. */
>> +            receiver_mode = false;
>> +            break;
>> +        case 'h': /* Help. */
>> +            usage();
>> +            break;
>> +        default:
>> +            usage();
>> +
> 
> checkpatch: Blank lines aren't necessary before a close brace '}
Ack this and all comments above
> 
>> +        }
>> +    }
>> +
>> +    if (receiver_mode)
>> +        run_receiver(peer_cid, rcvlowat_bytes);
>> +    else
>> +        run_sender(to_send_bytes);
>> +
>> +    return 0;
>> +}
>> -- 
>> 2.25.1
> 

Re: [RFC PATCH v1 2/3] test/vsock: add big message test

[Arseniy Krasnov]

On 21.11.2022 19:50, Arseniy Krasnov wrote:
> On 21.11.2022 17:52, Stefano Garzarella wrote:
>> On Tue, Nov 15, 2022 at 08:52:35PM +0000, Arseniy Krasnov wrote:
>>> This adds test for sending message, bigger than peer's buffer size.
>>> For SOCK_SEQPACKET socket it must fail, as this type of socket has
>>> message size limit.
>>>
>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>> ---
>>> tools/testing/vsock/vsock_test.c | 62 ++++++++++++++++++++++++++++++++
>>> 1 file changed, 62 insertions(+)
>>>
>>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>>> index 107c11165887..bb4e8657f1d6 100644
>>> --- a/tools/testing/vsock/vsock_test.c
>>> +++ b/tools/testing/vsock/vsock_test.c
>>> @@ -560,6 +560,63 @@ static void test_seqpacket_timeout_server(const struct test_opts *opts)
>>>     close(fd);
>>> }
>>>
>>> +static void test_seqpacket_bigmsg_client(const struct test_opts *opts)
>>> +{
>>> +    unsigned long sock_buf_size;
>>> +    ssize_t send_size;
>>> +    socklen_t len;
>>> +    void *data;
>>> +    int fd;
>>> +
>>> +    len = sizeof(sock_buf_size);
>>> +
>>> +    fd = vsock_seqpacket_connect(opts->peer_cid, 1234);
>>
>> Not for this patch, but someday we should add a macro for this port and maybe even make it configurable :-)
>>
>>> +    if (fd < 0) {
>>> +        perror("connect");
>>> +        exit(EXIT_FAILURE);
>>> +    }
>>> +
>>> +    if (getsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>>> +               &sock_buf_size, &len)) {
>>> +        perror("getsockopt");
>>> +        exit(EXIT_FAILURE);
>>> +    }
>>> +
>>> +    sock_buf_size++;
>>> +
>>> +    data = malloc(sock_buf_size);
>>> +    if (!data) {
>>> +        perror("malloc");
>>> +        exit(EXIT_FAILURE);
>>> +    }
>>> +
>>> +    send_size = send(fd, data, sock_buf_size, 0);
>>> +    if (send_size != -1) {
>>
>> Can we check also `errno`?
>> IIUC it should contains EMSGSIZE.
Hm, seems current implementation is a little bit broken and returns ENOMEM, because any negative value, returned by
transport callback is always replaced to ENOMEM. I think i need this patch from Bobby:
https://lore.kernel.org/lkml/d81818b868216c774613dd03641fcfe63cc55a45.1660362668.git.bobby.eshleman@bytedance.com/
May be i can include it to this patchset also fixing review comments(of course keeping Bobby as author). Or more
simple way is to check ENOMEM instead of EMSGSIZE in this test(simple, but a little bit dumb i think).
>>
>>> +        fprintf(stderr, "expected 'send(2)' failure, got %zi\n",
>>> +            send_size);
>>> +    }
>>> +
>>> +    control_writeln("CLISENT");
>>> +
>>> +    free(data);
>>> +    close(fd);
>>> +}
>>> +
>>> +static void test_seqpacket_bigmsg_server(const struct test_opts *opts)
>>> +{
>>> +    int fd;
>>> +
>>> +    fd = vsock_seqpacket_accept(VMADDR_CID_ANY, 1234, NULL);
>>> +    if (fd < 0) {
>>> +        perror("accept");
>>> +        exit(EXIT_FAILURE);
>>> +    }
>>> +
>>> +    control_expectln("CLISENT");
>>> +
>>> +    close(fd);
>>> +}
>>> +
>>> #define BUF_PATTERN_1 'a'
>>> #define BUF_PATTERN_2 'b'
>>>
>>> @@ -832,6 +889,11 @@ static struct test_case test_cases[] = {
>>>         .run_client = test_seqpacket_timeout_client,
>>>         .run_server = test_seqpacket_timeout_server,
>>>     },
>>> +    {
>>> +        .name = "SOCK_SEQPACKET big message",
>>> +        .run_client = test_seqpacket_bigmsg_client,
>>> +        .run_server = test_seqpacket_bigmsg_server,
>>> +    },
>>
>> I would add new tests always at the end, so if some CI uses --skip, we don't have to update the scripts to skip some tests.
> Ack this and all above
>>
>>>     {
>>>         .name = "SOCK_SEQPACKET invalid receive buffer",
>>>         .run_client = test_seqpacket_invalid_rec_buffer_client,
>>> -- 
>>> 2.25.1
>>
> 

Re: [RFC PATCH v1 2/3] test/vsock: add big message test

[Stefano Garzarella]

On Mon, Nov 21, 2022 at 09:40:39PM +0000, Arseniy Krasnov wrote:
>On 21.11.2022 19:50, Arseniy Krasnov wrote:
>> On 21.11.2022 17:52, Stefano Garzarella wrote:
>>> On Tue, Nov 15, 2022 at 08:52:35PM +0000, Arseniy Krasnov wrote:
>>>> This adds test for sending message, bigger than peer's buffer size.
>>>> For SOCK_SEQPACKET socket it must fail, as this type of socket has
>>>> message size limit.
>>>>
>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>>> ---
>>>> tools/testing/vsock/vsock_test.c | 62 ++++++++++++++++++++++++++++++++
>>>> 1 file changed, 62 insertions(+)
>>>>
>>>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>>>> index 107c11165887..bb4e8657f1d6 100644
>>>> --- a/tools/testing/vsock/vsock_test.c
>>>> +++ b/tools/testing/vsock/vsock_test.c
>>>> @@ -560,6 +560,63 @@ static void test_seqpacket_timeout_server(const struct test_opts *opts)
>>>>     close(fd);
>>>> }
>>>>
>>>> +static void test_seqpacket_bigmsg_client(const struct test_opts *opts)
>>>> +{
>>>> +    unsigned long sock_buf_size;
>>>> +    ssize_t send_size;
>>>> +    socklen_t len;
>>>> +    void *data;
>>>> +    int fd;
>>>> +
>>>> +    len = sizeof(sock_buf_size);
>>>> +
>>>> +    fd = vsock_seqpacket_connect(opts->peer_cid, 1234);
>>>
>>> Not for this patch, but someday we should add a macro for this port and maybe even make it configurable :-)
>>>
>>>> +    if (fd < 0) {
>>>> +        perror("connect");
>>>> +        exit(EXIT_FAILURE);
>>>> +    }
>>>> +
>>>> +    if (getsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>>>> +               &sock_buf_size, &len)) {
>>>> +        perror("getsockopt");
>>>> +        exit(EXIT_FAILURE);
>>>> +    }
>>>> +
>>>> +    sock_buf_size++;
>>>> +
>>>> +    data = malloc(sock_buf_size);
>>>> +    if (!data) {
>>>> +        perror("malloc");
>>>> +        exit(EXIT_FAILURE);
>>>> +    }
>>>> +
>>>> +    send_size = send(fd, data, sock_buf_size, 0);
>>>> +    if (send_size != -1) {
>>>
>>> Can we check also `errno`?
>>> IIUC it should contains EMSGSIZE.
>Hm, seems current implementation is a little bit broken and returns ENOMEM, because any negative value, returned by
>transport callback is always replaced to ENOMEM. I think i need this patch from Bobby:
>https://lore.kernel.org/lkml/d81818b868216c774613dd03641fcfe63cc55a45.1660362668.git.bobby.eshleman@bytedance.com/
>May be i can include it to this patchset also fixing review comments(of course keeping Bobby as author). Or more
>simple way is to check ENOMEM instead of EMSGSIZE in this test(simple, but a little bit dumb i think).

Maybe in this patch you can start checking ENOMEM (with a TODO comment),
and then Bobby can change it when sending his patch.

Or you can repost it (I'm not sure if we should keep the legacy behavior 
for other transports or it was an error, but better to discuss it on 
that patch). However, I think we should merge that patch.

@Bobby, what do you think?

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC PATCH v1 2/3] test/vsock: add big message test

[Stefano Garzarella]

On Mon, Nov 21, 2022 at 09:40:39PM +0000, Arseniy Krasnov wrote:
>On 21.11.2022 19:50, Arseniy Krasnov wrote:
>> On 21.11.2022 17:52, Stefano Garzarella wrote:
>>> On Tue, Nov 15, 2022 at 08:52:35PM +0000, Arseniy Krasnov wrote:
>>>> This adds test for sending message, bigger than peer's buffer size.
>>>> For SOCK_SEQPACKET socket it must fail, as this type of socket has
>>>> message size limit.
>>>>
>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>>> ---
>>>> tools/testing/vsock/vsock_test.c | 62 ++++++++++++++++++++++++++++++++
>>>> 1 file changed, 62 insertions(+)
>>>>
>>>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>>>> index 107c11165887..bb4e8657f1d6 100644
>>>> --- a/tools/testing/vsock/vsock_test.c
>>>> +++ b/tools/testing/vsock/vsock_test.c
>>>> @@ -560,6 +560,63 @@ static void test_seqpacket_timeout_server(const struct test_opts *opts)
>>>>     close(fd);
>>>> }
>>>>
>>>> +static void test_seqpacket_bigmsg_client(const struct test_opts *opts)
>>>> +{
>>>> +    unsigned long sock_buf_size;
>>>> +    ssize_t send_size;
>>>> +    socklen_t len;
>>>> +    void *data;
>>>> +    int fd;
>>>> +
>>>> +    len = sizeof(sock_buf_size);
>>>> +
>>>> +    fd = vsock_seqpacket_connect(opts->peer_cid, 1234);
>>>
>>> Not for this patch, but someday we should add a macro for this port and maybe even make it configurable :-)
>>>
>>>> +    if (fd < 0) {
>>>> +        perror("connect");
>>>> +        exit(EXIT_FAILURE);
>>>> +    }
>>>> +
>>>> +    if (getsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>>>> +               &sock_buf_size, &len)) {
>>>> +        perror("getsockopt");
>>>> +        exit(EXIT_FAILURE);
>>>> +    }
>>>> +
>>>> +    sock_buf_size++;
>>>> +
>>>> +    data = malloc(sock_buf_size);
>>>> +    if (!data) {
>>>> +        perror("malloc");
>>>> +        exit(EXIT_FAILURE);
>>>> +    }
>>>> +
>>>> +    send_size = send(fd, data, sock_buf_size, 0);
>>>> +    if (send_size != -1) {
>>>
>>> Can we check also `errno`?
>>> IIUC it should contains EMSGSIZE.
>Hm, seems current implementation is a little bit broken and returns ENOMEM, because any negative value, returned by
>transport callback is always replaced to ENOMEM. I think i need this patch from Bobby:
>https://lore.kernel.org/lkml/d81818b868216c774613dd03641fcfe63cc55a45.1660362668.git.bobby.eshleman@bytedance.com/
>May be i can include it to this patchset also fixing review comments(of course keeping Bobby as author). Or more
>simple way is to check ENOMEM instead of EMSGSIZE in this test(simple, but a little bit dumb i think).

Maybe in this patch you can start checking ENOMEM (with a TODO comment),
and then Bobby can change it when sending his patch.

Or you can repost it (I'm not sure if we should keep the legacy behavior 
for other transports or it was an error, but better to discuss it on 
that patch). However, I think we should merge that patch.

@Bobby, what do you think?

Thanks,
Stefano

Re: [RFC PATCH v1 1/3] test/vsock: rework message bound test

[Stefano Garzarella]

On Mon, Nov 21, 2022 at 04:49:23PM +0000, Arseniy Krasnov wrote:
>On 21.11.2022 17:46, Stefano Garzarella wrote:
>> On Tue, Nov 15, 2022 at 08:50:36PM +0000, Arseniy Krasnov wrote:
>>> This updates message bound test making it more complex. Instead of
>>> sending 1 bytes messages with one MSG_EOR bit, it sends messages of
>>> random length(one half of messages are smaller than page size, second
>>> half are bigger) with random number of MSG_EOR bits set. Receiver
>>> also don't know total number of messages.
>>>
>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>> ---
>>> tools/testing/vsock/control.c    |  34 +++++++++
>>> tools/testing/vsock/control.h    |   2 +
>>> tools/testing/vsock/util.c       |  13 ++++
>>> tools/testing/vsock/util.h       |   1 +
>>> tools/testing/vsock/vsock_test.c | 115 +++++++++++++++++++++++++++----
>>> 5 files changed, 152 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/tools/testing/vsock/control.c b/tools/testing/vsock/control.c
>>> index 4874872fc5a3..bed1649bdf3d 100644
>>> --- a/tools/testing/vsock/control.c
>>> +++ b/tools/testing/vsock/control.c
>>> @@ -141,6 +141,40 @@ void control_writeln(const char *str)
>>>     timeout_end();
>>> }
>>>
>>> +void control_writeulong(unsigned long value)
>>> +{
>>> +    char str[32];
>>> +
>>> +    if (snprintf(str, sizeof(str), "%lu", value) >= sizeof(str)) {
>>> +        perror("snprintf");
>>> +        exit(EXIT_FAILURE);
>>> +    }
>>> +
>>> +    control_writeln(str);
>>> +}
>>> +
>>> +unsigned long control_readulong(bool *ok)
>>> +{
>>> +    unsigned long value;
>>> +    char *str;
>>> +
>>> +    if (ok)
>>> +        *ok = false;
>>> +
>>> +    str = control_readln();
>>> +
>>> +    if (str == NULL)
>>
>> checkpatch suggests to use !str
>>
>>> +        return 0;
>>
>> Maybe we can just call exit(EXIT_FAILURE) here and remove the `ok`
>> parameter, since I'm not sure we can recover from this error.
>>
>>> +
>>> +    value = strtoul(str, NULL, 10);
>>> +    free(str);
>>> +
>>> +    if (ok)
>>> +        *ok = true;
>>> +
>>> +    return value;
>>> +}
>>> +
>>> /* Return the next line from the control socket (without the trailing newline).
>>>  *
>>>  * The program terminates if a timeout occurs.
>>> diff --git a/tools/testing/vsock/control.h b/tools/testing/vsock/control.h
>>> index 51814b4f9ac1..cdd922dfea68 100644
>>> --- a/tools/testing/vsock/control.h
>>> +++ b/tools/testing/vsock/control.h
>>> @@ -9,7 +9,9 @@ void control_init(const char *control_host, const char *control_port,
>>> void control_cleanup(void);
>>> void control_writeln(const char *str);
>>> char *control_readln(void);
>>> +unsigned long control_readulong(bool *ok);
>>> void control_expectln(const char *str);
>>> bool control_cmpln(char *line, const char *str, bool fail);
>>> +void control_writeulong(unsigned long value);
>>>
>>> #endif /* CONTROL_H */
>>> diff --git a/tools/testing/vsock/util.c b/tools/testing/vsock/util.c
>>> index 2acbb7703c6a..351903836774 100644
>>> --- a/tools/testing/vsock/util.c
>>> +++ b/tools/testing/vsock/util.c
>>> @@ -395,3 +395,16 @@ void skip_test(struct test_case *test_cases, size_t test_cases_len,
>>>
>>>     test_cases[test_id].skip = true;
>>> }
>>> +
>>> +unsigned long djb2(const void *data, size_t len)
>>
>> I would add hash_ as a prefix (or suffix).
>>
>>> +{
>>> +    unsigned long hash = 5381;
>>> +    int i = 0;
>>> +
>>> +    while (i < len) {
>>> +        hash = ((hash << 5) + hash) + ((unsigned char *)data)[i];
>>> +        i++;
>>> +    }
>>> +
>>> +    return hash;
>>> +}
>>> diff --git a/tools/testing/vsock/util.h b/tools/testing/vsock/util.h
>>> index a3375ad2fb7f..988cc69a4642 100644
>>> --- a/tools/testing/vsock/util.h
>>> +++ b/tools/testing/vsock/util.h
>>> @@ -49,4 +49,5 @@ void run_tests(const struct test_case *test_cases,
>>> void list_tests(const struct test_case *test_cases);
>>> void skip_test(struct test_case *test_cases, size_t test_cases_len,
>>>            const char *test_id_str);
>>> +unsigned long djb2(const void *data, size_t len);
>>> #endif /* UTIL_H */
>>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>>> index bb6d691cb30d..107c11165887 100644
>>> --- a/tools/testing/vsock/vsock_test.c
>>> +++ b/tools/testing/vsock/vsock_test.c
>>> @@ -284,10 +284,14 @@ static void test_stream_msg_peek_server(const struct test_opts *opts)
>>>     close(fd);
>>> }
>>>
>>> -#define MESSAGES_CNT 7
>>> -#define MSG_EOR_IDX (MESSAGES_CNT / 2)
>>> +#define SOCK_BUF_SIZE (2 * 1024 * 1024)
>>> +#define MAX_MSG_SIZE (32 * 1024)
>>> +
>>> static void test_seqpacket_msg_bounds_client(const struct test_opts *opts)
>>> {
>>> +    unsigned long curr_hash;
>>> +    int page_size;
>>> +    int msg_count;
>>>     int fd;
>>>
>>>     fd = vsock_seqpacket_connect(opts->peer_cid, 1234);
>>> @@ -296,18 +300,69 @@ static void test_seqpacket_msg_bounds_client(const struct test_opts *opts)
>>>         exit(EXIT_FAILURE);
>>>     }
>>>
>>> -    /* Send several messages, one with MSG_EOR flag */
>>> -    for (int i = 0; i < MESSAGES_CNT; i++)
>>> -        send_byte(fd, 1, (i == MSG_EOR_IDX) ? MSG_EOR : 0);
>>> +    /* Wait, until receiver sets buffer size. */
>>> +    control_expectln("SRVREADY");
>>> +
>>> +    curr_hash = 0;
>>> +    page_size = getpagesize();
>>> +    msg_count = SOCK_BUF_SIZE / MAX_MSG_SIZE;
>>> +
>>> +    for (int i = 0; i < msg_count; i++) {
>>> +        ssize_t send_size;
>>> +        size_t buf_size;
>>> +        int flags;
>>> +        void *buf;
>>> +
>>> +        /* Use "small" buffers and "big" buffers. */
>>> +        if (i & 1)
>>> +            buf_size = page_size +
>>> +                    (rand() % (MAX_MSG_SIZE - page_size));
>>> +        else
>>> +            buf_size = 1 + (rand() % page_size);
>>> +
>>> +        buf = malloc(buf_size);
>>> +
>>> +        if (!buf) {
>>> +            perror("malloc");
>>> +            exit(EXIT_FAILURE);
>>> +        }
>>> +
>>> +        /* Set at least one MSG_EOR + some random. */
>>> +        if (i == (msg_count / 2) || (rand() & 1)) {
>>> +            flags = MSG_EOR;
>>> +            curr_hash++;
>>> +        } else {
>>> +            flags = 0;
>>> +        }
>>> +
>>> +        send_size = send(fd, buf, buf_size, flags);
>>> +
>>> +        if (send_size < 0) {
>>> +            perror("send");
>>> +            exit(EXIT_FAILURE);
>>> +        }
>>> +
>>> +        if (send_size != buf_size) {
>>> +            fprintf(stderr, "Invalid send size\n");
>>> +            exit(EXIT_FAILURE);
>>> +        }
>>> +
>>> +        curr_hash += send_size;
>>> +        curr_hash = djb2(&curr_hash, sizeof(curr_hash));
>>> +    }
>>>
>>>     control_writeln("SENDDONE");
>>> +    control_writeulong(curr_hash);
>>
>> Why do we need to hash the size?
>>
>> Maybe we can send it without making the hash, anyway even if it wraps,
>> it should wrap the same way in both the server and the client.
>> (Or maybe we can use uin32_t or uint64_t to make sure both were
>> using 4 or 8 bytes)
>Hello, thanks for review. I think if we will use sum of message size(IIUC), in most
>paranoic case it won't guarantee message bounds control: single 4 bytes message
>could be read as 4 x 1 byte message(IIUC of course). Idea of hashing is simple:
>every iteration we do current_hash = hash(previous_hash + size of current message);
>I think this is more reliable and protects from case described above.

Okay, now I understand what it is for and agree that using hash is 
better.
Please add a comment to explain it.

>
>All other comments - ack.

Thanks,
Stefano

Re: [RFC PATCH v1 1/3] test/vsock: rework message bound test

[Stefano Garzarella]

On Mon, Nov 21, 2022 at 04:49:23PM +0000, Arseniy Krasnov wrote:
>On 21.11.2022 17:46, Stefano Garzarella wrote:
>> On Tue, Nov 15, 2022 at 08:50:36PM +0000, Arseniy Krasnov wrote:
>>> This updates message bound test making it more complex. Instead of
>>> sending 1 bytes messages with one MSG_EOR bit, it sends messages of
>>> random length(one half of messages are smaller than page size, second
>>> half are bigger) with random number of MSG_EOR bits set. Receiver
>>> also don't know total number of messages.
>>>
>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>> ---
>>> tools/testing/vsock/control.c    |  34 +++++++++
>>> tools/testing/vsock/control.h    |   2 +
>>> tools/testing/vsock/util.c       |  13 ++++
>>> tools/testing/vsock/util.h       |   1 +
>>> tools/testing/vsock/vsock_test.c | 115 +++++++++++++++++++++++++++----
>>> 5 files changed, 152 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/tools/testing/vsock/control.c b/tools/testing/vsock/control.c
>>> index 4874872fc5a3..bed1649bdf3d 100644
>>> --- a/tools/testing/vsock/control.c
>>> +++ b/tools/testing/vsock/control.c
>>> @@ -141,6 +141,40 @@ void control_writeln(const char *str)
>>>     timeout_end();
>>> }
>>>
>>> +void control_writeulong(unsigned long value)
>>> +{
>>> +    char str[32];
>>> +
>>> +    if (snprintf(str, sizeof(str), "%lu", value) >= sizeof(str)) {
>>> +        perror("snprintf");
>>> +        exit(EXIT_FAILURE);
>>> +    }
>>> +
>>> +    control_writeln(str);
>>> +}
>>> +
>>> +unsigned long control_readulong(bool *ok)
>>> +{
>>> +    unsigned long value;
>>> +    char *str;
>>> +
>>> +    if (ok)
>>> +        *ok = false;
>>> +
>>> +    str = control_readln();
>>> +
>>> +    if (str == NULL)
>>
>> checkpatch suggests to use !str
>>
>>> +        return 0;
>>
>> Maybe we can just call exit(EXIT_FAILURE) here and remove the `ok`
>> parameter, since I'm not sure we can recover from this error.
>>
>>> +
>>> +    value = strtoul(str, NULL, 10);
>>> +    free(str);
>>> +
>>> +    if (ok)
>>> +        *ok = true;
>>> +
>>> +    return value;
>>> +}
>>> +
>>> /* Return the next line from the control socket (without the trailing newline).
>>>  *
>>>  * The program terminates if a timeout occurs.
>>> diff --git a/tools/testing/vsock/control.h b/tools/testing/vsock/control.h
>>> index 51814b4f9ac1..cdd922dfea68 100644
>>> --- a/tools/testing/vsock/control.h
>>> +++ b/tools/testing/vsock/control.h
>>> @@ -9,7 +9,9 @@ void control_init(const char *control_host, const char *control_port,
>>> void control_cleanup(void);
>>> void control_writeln(const char *str);
>>> char *control_readln(void);
>>> +unsigned long control_readulong(bool *ok);
>>> void control_expectln(const char *str);
>>> bool control_cmpln(char *line, const char *str, bool fail);
>>> +void control_writeulong(unsigned long value);
>>>
>>> #endif /* CONTROL_H */
>>> diff --git a/tools/testing/vsock/util.c b/tools/testing/vsock/util.c
>>> index 2acbb7703c6a..351903836774 100644
>>> --- a/tools/testing/vsock/util.c
>>> +++ b/tools/testing/vsock/util.c
>>> @@ -395,3 +395,16 @@ void skip_test(struct test_case *test_cases, size_t test_cases_len,
>>>
>>>     test_cases[test_id].skip = true;
>>> }
>>> +
>>> +unsigned long djb2(const void *data, size_t len)
>>
>> I would add hash_ as a prefix (or suffix).
>>
>>> +{
>>> +    unsigned long hash = 5381;
>>> +    int i = 0;
>>> +
>>> +    while (i < len) {
>>> +        hash = ((hash << 5) + hash) + ((unsigned char *)data)[i];
>>> +        i++;
>>> +    }
>>> +
>>> +    return hash;
>>> +}
>>> diff --git a/tools/testing/vsock/util.h b/tools/testing/vsock/util.h
>>> index a3375ad2fb7f..988cc69a4642 100644
>>> --- a/tools/testing/vsock/util.h
>>> +++ b/tools/testing/vsock/util.h
>>> @@ -49,4 +49,5 @@ void run_tests(const struct test_case *test_cases,
>>> void list_tests(const struct test_case *test_cases);
>>> void skip_test(struct test_case *test_cases, size_t test_cases_len,
>>>            const char *test_id_str);
>>> +unsigned long djb2(const void *data, size_t len);
>>> #endif /* UTIL_H */
>>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>>> index bb6d691cb30d..107c11165887 100644
>>> --- a/tools/testing/vsock/vsock_test.c
>>> +++ b/tools/testing/vsock/vsock_test.c
>>> @@ -284,10 +284,14 @@ static void test_stream_msg_peek_server(const struct test_opts *opts)
>>>     close(fd);
>>> }
>>>
>>> -#define MESSAGES_CNT 7
>>> -#define MSG_EOR_IDX (MESSAGES_CNT / 2)
>>> +#define SOCK_BUF_SIZE (2 * 1024 * 1024)
>>> +#define MAX_MSG_SIZE (32 * 1024)
>>> +
>>> static void test_seqpacket_msg_bounds_client(const struct test_opts *opts)
>>> {
>>> +    unsigned long curr_hash;
>>> +    int page_size;
>>> +    int msg_count;
>>>     int fd;
>>>
>>>     fd = vsock_seqpacket_connect(opts->peer_cid, 1234);
>>> @@ -296,18 +300,69 @@ static void test_seqpacket_msg_bounds_client(const struct test_opts *opts)
>>>         exit(EXIT_FAILURE);
>>>     }
>>>
>>> -    /* Send several messages, one with MSG_EOR flag */
>>> -    for (int i = 0; i < MESSAGES_CNT; i++)
>>> -        send_byte(fd, 1, (i == MSG_EOR_IDX) ? MSG_EOR : 0);
>>> +    /* Wait, until receiver sets buffer size. */
>>> +    control_expectln("SRVREADY");
>>> +
>>> +    curr_hash = 0;
>>> +    page_size = getpagesize();
>>> +    msg_count = SOCK_BUF_SIZE / MAX_MSG_SIZE;
>>> +
>>> +    for (int i = 0; i < msg_count; i++) {
>>> +        ssize_t send_size;
>>> +        size_t buf_size;
>>> +        int flags;
>>> +        void *buf;
>>> +
>>> +        /* Use "small" buffers and "big" buffers. */
>>> +        if (i & 1)
>>> +            buf_size = page_size +
>>> +                    (rand() % (MAX_MSG_SIZE - page_size));
>>> +        else
>>> +            buf_size = 1 + (rand() % page_size);
>>> +
>>> +        buf = malloc(buf_size);
>>> +
>>> +        if (!buf) {
>>> +            perror("malloc");
>>> +            exit(EXIT_FAILURE);
>>> +        }
>>> +
>>> +        /* Set at least one MSG_EOR + some random. */
>>> +        if (i == (msg_count / 2) || (rand() & 1)) {
>>> +            flags = MSG_EOR;
>>> +            curr_hash++;
>>> +        } else {
>>> +            flags = 0;
>>> +        }
>>> +
>>> +        send_size = send(fd, buf, buf_size, flags);
>>> +
>>> +        if (send_size < 0) {
>>> +            perror("send");
>>> +            exit(EXIT_FAILURE);
>>> +        }
>>> +
>>> +        if (send_size != buf_size) {
>>> +            fprintf(stderr, "Invalid send size\n");
>>> +            exit(EXIT_FAILURE);
>>> +        }
>>> +
>>> +        curr_hash += send_size;
>>> +        curr_hash = djb2(&curr_hash, sizeof(curr_hash));
>>> +    }
>>>
>>>     control_writeln("SENDDONE");
>>> +    control_writeulong(curr_hash);
>>
>> Why do we need to hash the size?
>>
>> Maybe we can send it without making the hash, anyway even if it wraps,
>> it should wrap the same way in both the server and the client.
>> (Or maybe we can use uin32_t or uint64_t to make sure both were
>> using 4 or 8 bytes)
>Hello, thanks for review. I think if we will use sum of message size(IIUC), in most
>paranoic case it won't guarantee message bounds control: single 4 bytes message
>could be read as 4 x 1 byte message(IIUC of course). Idea of hashing is simple:
>every iteration we do current_hash = hash(previous_hash + size of current message);
>I think this is more reliable and protects from case described above.

Okay, now I understand what it is for and agree that using hash is 
better.
Please add a comment to explain it.

>
>All other comments - ack.

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC PATCH v1 2/3] test/vsock: add big message test

[Arseniy Krasnov]

On 23.11.2022 18:40, Bobby Eshleman wrote:
> On Wed, Nov 23, 2022 at 7:22 AM Stefano Garzarella <sgarzare@redhat.com>
> wrote:
> 
>> On Mon, Nov 21, 2022 at 09:40:39PM +0000, Arseniy Krasnov wrote:
>>> On 21.11.2022 19:50, Arseniy Krasnov wrote:
>>>> On 21.11.2022 17:52, Stefano Garzarella wrote:
>>>>> On Tue, Nov 15, 2022 at 08:52:35PM +0000, Arseniy Krasnov wrote:
>>>>>> This adds test for sending message, bigger than peer's buffer size.
>>>>>> For SOCK_SEQPACKET socket it must fail, as this type of socket has
>>>>>> message size limit.
>>>>>>
>>>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>>>>> ---
>>>>>> tools/testing/vsock/vsock_test.c | 62 ++++++++++++++++++++++++++++++++
>>>>>> 1 file changed, 62 insertions(+)
>>>>>>
>>>>>> diff --git a/tools/testing/vsock/vsock_test.c
>> b/tools/testing/vsock/vsock_test.c
>>>>>> index 107c11165887..bb4e8657f1d6 100644
>>>>>> --- a/tools/testing/vsock/vsock_test.c
>>>>>> +++ b/tools/testing/vsock/vsock_test.c
>>>>>> @@ -560,6 +560,63 @@ static void test_seqpacket_timeout_server(const
>> struct test_opts *opts)
>>>>>>     close(fd);
>>>>>> }
>>>>>>
>>>>>> +static void test_seqpacket_bigmsg_client(const struct test_opts
>> *opts)
>>>>>> +{
>>>>>> +    unsigned long sock_buf_size;
>>>>>> +    ssize_t send_size;
>>>>>> +    socklen_t len;
>>>>>> +    void *data;
>>>>>> +    int fd;
>>>>>> +
>>>>>> +    len = sizeof(sock_buf_size);
>>>>>> +
>>>>>> +    fd = vsock_seqpacket_connect(opts->peer_cid, 1234);
>>>>>
>>>>> Not for this patch, but someday we should add a macro for this port
>> and maybe even make it configurable :-)
>>>>>
>>>>>> +    if (fd < 0) {
>>>>>> +        perror("connect");
>>>>>> +        exit(EXIT_FAILURE);
>>>>>> +    }
>>>>>> +
>>>>>> +    if (getsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>>>>>> +               &sock_buf_size, &len)) {
>>>>>> +        perror("getsockopt");
>>>>>> +        exit(EXIT_FAILURE);
>>>>>> +    }
>>>>>> +
>>>>>> +    sock_buf_size++;
>>>>>> +
>>>>>> +    data = malloc(sock_buf_size);
>>>>>> +    if (!data) {
>>>>>> +        perror("malloc");
>>>>>> +        exit(EXIT_FAILURE);
>>>>>> +    }
>>>>>> +
>>>>>> +    send_size = send(fd, data, sock_buf_size, 0);
>>>>>> +    if (send_size != -1) {
>>>>>
>>>>> Can we check also `errno`?
>>>>> IIUC it should contains EMSGSIZE.
>>> Hm, seems current implementation is a little bit broken and returns
>> ENOMEM, because any negative value, returned by
>>> transport callback is always replaced to ENOMEM. I think i need this
>> patch from Bobby:
>>>
>> https://lore.kernel.org/lkml/d81818b868216c774613dd03641fcfe63cc55a45.1660362668.git.bobby.eshleman@bytedance.com/
>>> May be i can include it to this patchset also fixing review comments(of
>> course keeping Bobby as author). Or more
>>> simple way is to check ENOMEM instead of EMSGSIZE in this test(simple,
>> but a little bit dumb i think).
>>
>> Maybe in this patch you can start checking ENOMEM (with a TODO comment),
>> and then Bobby can change it when sending his patch.
>>
>> Or you can repost it (I'm not sure if we should keep the legacy behavior
>> for other transports or it was an error, but better to discuss it on
>> that patch). However, I think we should merge that patch.
>>
>> @Bobby, what do you think?
>>
>> Thanks,
>> Stefano
>>
>>
> This sounds good to me. I removed it from the last rev because I decided not
> to complicate the patch by also including SO_SNDBUF support, so had no
> need. I think it makes sense overall though.
Ok! So I'll use Your patch(both af_vsock.c and transports related things - seems i'll
split it to several patches, I think for transport patches, it is better to ask Vmware
/Microsoft guys also). I'm going to send next version of my tests on this week.

Thank You
> 
> Also, sorry for the delay (I promised last week to send out new rev). I was
> planning on sending out v4 with additional data for the non-nested virt
> case,
> but I've been having some IT troubles with the new phys box.
No problem, im currently rebasing my patches for zerocopy on v3.
> 
> Best,
> Bobby
> 
> PS. sorry if this email format comes out wacky. I'm not at my dev machine
> so just using Gmail's web app directly... Hopefully there is no HTML or
> anything weird.
>