diff mbox series

[bpf-next,v2,2/4] bpf: sockmap, add a sendmsg test so we can check that path

Message ID 20240124185403.1104141-3-john.fastabend@gmail.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series transition sockmap testing to test_progs | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success SINGLE THREAD; Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/build_tools success Errors and warnings before: 1 this patch: 0
netdev/cc_maintainers success CCed 0 of 0 maintainers
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch warning WARNING: Possible repeated word: 'number'
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 fail Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc

Commit Message

John Fastabend Jan. 24, 2024, 6:54 p.m. UTC 
Sendmsg path with multiple buffers is slightly different from a single
send in how we have to handle and walk the sg when doing pops. Lets
ensure this walk is correct.

Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
 .../bpf/prog_tests/sockmap_helpers.h          |  8 +++
 .../bpf/prog_tests/sockmap_msg_helpers.c      | 53 +++++++++++++++++++
 2 files changed, 61 insertions(+)

Comments

Jakub Sitnicki Jan. 26, 2024, 12:17 p.m. UTC | #1 
On Wed, Jan 24, 2024 at 10:54 AM -08, John Fastabend wrote:
> Sendmsg path with multiple buffers is slightly different from a single
> send in how we have to handle and walk the sg when doing pops. Lets
> ensure this walk is correct.
>
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> ---
>  .../bpf/prog_tests/sockmap_helpers.h          |  8 +++
>  .../bpf/prog_tests/sockmap_msg_helpers.c      | 53 +++++++++++++++++++
>  2 files changed, 61 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
> index 781cbdf01d7b..4d8d24482032 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
> @@ -103,6 +103,14 @@
>  		__ret;                                                         \
>  	})
>  
> +#define xsendmsg(fd, msg, flags)                                               \
> +	({                                                                     \
> +		ssize_t __ret = sendmsg((fd), (msg), (flags));                 \
> +		if (__ret == -1)                                               \
> +			FAIL_ERRNO("sendmsg");                                 \
> +		__ret;                                                         \
> +	})
> +
>  #define xrecv_nonblock(fd, buf, len, flags)                                    \
>  	({                                                                     \
>  		ssize_t __ret = recv_timeout((fd), (buf), (len), (flags),      \
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_msg_helpers.c b/tools/testing/selftests/bpf/prog_tests/sockmap_msg_helpers.c
> index 9ffe02f45808..e5e618e84950 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_msg_helpers.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_msg_helpers.c
> @@ -52,6 +52,50 @@ static void pop_simple_send(struct msg_test_opts *opts, int start, int len)
>  	ASSERT_OK(cmp, "pop cmp end bytes failed");
>  }
>  
> +static void pop_complex_send(struct msg_test_opts *opts, int start, int len)
> +{
> +	struct test_sockmap_msg_helpers *skel = opts->skel;
> +	char buf[] = "abcdefghijklmnopqrstuvwxyz";
> +	size_t sent, recv, total = 0;
> +	struct msghdr msg = {0};
> +	struct iovec iov[15];
> +	char *recvbuf;
> +	int i;
> +
> +	for (i = 0; i < 15; i++) {

Always nice to use ARRAY_SIZE.

> +		iov[i].iov_base = buf;
> +		iov[i].iov_len = sizeof(buf);
> +		total += sizeof(buf);
> +	}
> +
> +	recvbuf = malloc(total);
> +	if (!recvbuf)
> +		FAIL("pop complex send malloc failure\n");

390 bytes, why not have it on stack?

> +
> +	msg.msg_iov = iov;
> +	msg.msg_iovlen = 15;
> +
> +	skel->bss->pop = true;
> +
> +	if (start == -1)
> +		start = sizeof(buf) - len - 1;
> +
> +	skel->bss->pop_start = start;
> +	skel->bss->pop_len = len;
> +
> +	sent = xsendmsg(opts->client, &msg, 0);
> +	if (sent != total)
> +		FAIL("xsend failed");
> +
> +	ASSERT_OK(skel->bss->err, "pop error");
> +
> +	recv = xrecv_nonblock(opts->server, recvbuf, total, 0);
> +	if (recv != sent - skel->bss->pop_len)
> +		FAIL("Received incorrect number number of bytes after pop");
> +
> +	free(recvbuf);
> +}
> +
>  static void test_sockmap_pop(void)
>  {
>  	struct msg_test_opts opts;
> @@ -92,6 +136,15 @@ static void test_sockmap_pop(void)
>  	/* Pop from end */
>  	pop_simple_send(&opts, POP_END, 5);
>  
> +	/* Empty pop from start of sendmsg */
> +	pop_complex_send(&opts, 0, 0);
> +	/* Pop from start of sendmsg */
> +	pop_complex_send(&opts, 0, 10);
> +	/* Pop from middle of sendmsg */
> +	pop_complex_send(&opts, 100, 10);
> +	/* Pop from end of sendmsg */
> +	pop_complex_send(&opts, 394, 10);

Isn't the start offset here past the end? 15*26=390?

> +
>  close_sockets:
>  	close(client);
>  	close(server);
Jakub Sitnicki Jan. 26, 2024, 2:24 p.m. UTC | #2 
On Fri, Jan 26, 2024 at 01:17 PM +01, Jakub Sitnicki wrote:
> On Wed, Jan 24, 2024 at 10:54 AM -08, John Fastabend wrote:

[...]

>> @@ -92,6 +136,15 @@ static void test_sockmap_pop(void)
>>  	/* Pop from end */
>>  	pop_simple_send(&opts, POP_END, 5);
>>  
>> +	/* Empty pop from start of sendmsg */
>> +	pop_complex_send(&opts, 0, 0);
>> +	/* Pop from start of sendmsg */
>> +	pop_complex_send(&opts, 0, 10);
>> +	/* Pop from middle of sendmsg */
>> +	pop_complex_send(&opts, 100, 10);
>> +	/* Pop from end of sendmsg */
>> +	pop_complex_send(&opts, 394, 10);
>
> Isn't the start offset here past the end? 15*26=390?

Plus terminating null bytes. Nevermind. I can't count.
John Fastabend Jan. 26, 2024, 3:38 p.m. UTC | #3 
Jakub Sitnicki wrote:
> On Wed, Jan 24, 2024 at 10:54 AM -08, John Fastabend wrote:
> > Sendmsg path with multiple buffers is slightly different from a single
> > send in how we have to handle and walk the sg when doing pops. Lets
> > ensure this walk is correct.
> >
> > Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> > ---
> >  .../bpf/prog_tests/sockmap_helpers.h          |  8 +++
> >  .../bpf/prog_tests/sockmap_msg_helpers.c      | 53 +++++++++++++++++++
> >  2 files changed, 61 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
> > index 781cbdf01d7b..4d8d24482032 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
> > +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
> > @@ -103,6 +103,14 @@
> >  		__ret;                                                         \
> >  	})
> >  
> > +#define xsendmsg(fd, msg, flags)                                               \
> > +	({                                                                     \
> > +		ssize_t __ret = sendmsg((fd), (msg), (flags));                 \
> > +		if (__ret == -1)                                               \
> > +			FAIL_ERRNO("sendmsg");                                 \
> > +		__ret;                                                         \
> > +	})
> > +
> >  #define xrecv_nonblock(fd, buf, len, flags)                                    \
> >  	({                                                                     \
> >  		ssize_t __ret = recv_timeout((fd), (buf), (len), (flags),      \
> > diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_msg_helpers.c b/tools/testing/selftests/bpf/prog_tests/sockmap_msg_helpers.c
> > index 9ffe02f45808..e5e618e84950 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/sockmap_msg_helpers.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_msg_helpers.c
> > @@ -52,6 +52,50 @@ static void pop_simple_send(struct msg_test_opts *opts, int start, int len)
> >  	ASSERT_OK(cmp, "pop cmp end bytes failed");
> >  }
> >  
> > +static void pop_complex_send(struct msg_test_opts *opts, int start, int len)
> > +{
> > +	struct test_sockmap_msg_helpers *skel = opts->skel;
> > +	char buf[] = "abcdefghijklmnopqrstuvwxyz";
> > +	size_t sent, recv, total = 0;
> > +	struct msghdr msg = {0};
> > +	struct iovec iov[15];
> > +	char *recvbuf;
> > +	int i;
> > +
> > +	for (i = 0; i < 15; i++) {
> 
> Always nice to use ARRAY_SIZE.

Agree will do.

> 
> > +		iov[i].iov_base = buf;
> > +		iov[i].iov_len = sizeof(buf);
> > +		total += sizeof(buf);
> > +	}
> > +
> > +	recvbuf = malloc(total);
> > +	if (!recvbuf)
> > +		FAIL("pop complex send malloc failure\n");
> 
> 390 bytes, why not have it on stack?

Sure one less thing that could fail seems like a win.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
index 781cbdf01d7b..4d8d24482032 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
@@ -103,6 +103,14 @@ 
 		__ret;                                                         \
 	})
 
+#define xsendmsg(fd, msg, flags)                                               \
+	({                                                                     \
+		ssize_t __ret = sendmsg((fd), (msg), (flags));                 \
+		if (__ret == -1)                                               \
+			FAIL_ERRNO("sendmsg");                                 \
+		__ret;                                                         \
+	})
+
 #define xrecv_nonblock(fd, buf, len, flags)                                    \
 	({                                                                     \
 		ssize_t __ret = recv_timeout((fd), (buf), (len), (flags),      \
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_msg_helpers.c b/tools/testing/selftests/bpf/prog_tests/sockmap_msg_helpers.c
index 9ffe02f45808..e5e618e84950 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_msg_helpers.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_msg_helpers.c
@@ -52,6 +52,50 @@  static void pop_simple_send(struct msg_test_opts *opts, int start, int len)
 	ASSERT_OK(cmp, "pop cmp end bytes failed");
 }
 
+static void pop_complex_send(struct msg_test_opts *opts, int start, int len)
+{
+	struct test_sockmap_msg_helpers *skel = opts->skel;
+	char buf[] = "abcdefghijklmnopqrstuvwxyz";
+	size_t sent, recv, total = 0;
+	struct msghdr msg = {0};
+	struct iovec iov[15];
+	char *recvbuf;
+	int i;
+
+	for (i = 0; i < 15; i++) {
+		iov[i].iov_base = buf;
+		iov[i].iov_len = sizeof(buf);
+		total += sizeof(buf);
+	}
+
+	recvbuf = malloc(total);
+	if (!recvbuf)
+		FAIL("pop complex send malloc failure\n");
+
+	msg.msg_iov = iov;
+	msg.msg_iovlen = 15;
+
+	skel->bss->pop = true;
+
+	if (start == -1)
+		start = sizeof(buf) - len - 1;
+
+	skel->bss->pop_start = start;
+	skel->bss->pop_len = len;
+
+	sent = xsendmsg(opts->client, &msg, 0);
+	if (sent != total)
+		FAIL("xsend failed");
+
+	ASSERT_OK(skel->bss->err, "pop error");
+
+	recv = xrecv_nonblock(opts->server, recvbuf, total, 0);
+	if (recv != sent - skel->bss->pop_len)
+		FAIL("Received incorrect number number of bytes after pop");
+
+	free(recvbuf);
+}
+
 static void test_sockmap_pop(void)
 {
 	struct msg_test_opts opts;
@@ -92,6 +136,15 @@  static void test_sockmap_pop(void)
 	/* Pop from end */
 	pop_simple_send(&opts, POP_END, 5);
 
+	/* Empty pop from start of sendmsg */
+	pop_complex_send(&opts, 0, 0);
+	/* Pop from start of sendmsg */
+	pop_complex_send(&opts, 0, 10);
+	/* Pop from middle of sendmsg */
+	pop_complex_send(&opts, 100, 10);
+	/* Pop from end of sendmsg */
+	pop_complex_send(&opts, 394, 10);
+
 close_sockets:
 	close(client);
 	close(server);