Re: [PATCH mptcp-next v2 7/8] selftests: bpf: mptcp: test the send limit of a scheduler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] selftests/bpf: deprecated CHECK() macro used in test_limit()
- [Low] selftests/bpf: non-standard multi-line comment style in mptcp limit test
--

commit fb18275d8cce4c512f78d41262ea65272c01b932
Author: Abdul Wasey <w453y.me@gmail.com>

selftests: bpf: mptcp: test the send limit of a scheduler

This commit adds a bpf_limit round-robin MPTCP scheduler that sets send_limit
to 4 KB on the selected subflow and a selftest verifying that no subflow
receives more than 4 KB between two get_send calls when sending 64 KB chunks.

> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> index d77c9f8c53c75..4302c16cb6bb4 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
[ ... ]
> @@ -820,6 +821,109 @@ static void test_burst(void)
>  	mptcp_bpf_burst__destroy(skel);
>  }
>  
> +struct big_send_arg {
> +	int lfd;
> +	int err;
> +};
> +
> +/* send total_bytes in 64 KB writes, so that data fragments get larger than
> + * the scheduler's limit
> + */
> +static void *big_send(void *data)
[ ... ]
> +/* At most 4 KB per decision, also inside a data fragment: no subflow gets
> + * more than that between two calls of the scheduler.
> + */
> +static void test_limit(void)

[Severity: Low]
This isn't a bug, but in BPF code and selftests, should multi-line comments
above big_send() and test_limit() place the opening /* on its own line with
the comment text starting on the next line?

> +{
[ ... ]
> +	ASSERT_OK(arg.err, "send");
> +	ASSERT_EQ(recvd, total_bytes, "recv");
> +	CHECK(has_bytes_sent(ADDR_2), "limit", "should have bytes_sent on addr2\n");

[Severity: Low]
Since CHECK() is deprecated in BPF selftests, could test_limit() use
ASSERT_OK(has_bytes_sent(ADDR_2), "has_bytes_sent addr2") here instead,
matching the other ASSERT_*() calls in this function?

> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_limit.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_limit.c
> new file mode 100644
> index 0000000000000..431f07d6b6696
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_limit.c
> @@ -0,0 +1,94 @@
[ ... ]
> +/* An MPTCP-level retransmission is not one of our decisions: do not count
> + * what it sends.
> + */
> +SEC("struct_ops")
> +int BPF_PROG(bpf_limit_get_retrans, struct mptcp_sock *msk)

[Severity: Low]
Similarly, should the multi-line comment above bpf_limit_get_retrans() have
the opening /* on its own line to follow BPF coding style?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031-mptcp-sched-opp-retrans-v2-0-e9c4a30f6841@kernel.org?part=7