Hi Alexei,
Thank you for your reply!
On 07/05/2024 22:51, Alexei Starovoitov wrote:
On Tue, May 7, 2024 at 9:02 AM Matthieu Baerts matttbe@kernel.org wrote:
Hi Alexei,
Thank you for the review!
On 07/05/2024 16:44, Alexei Starovoitov wrote:
On Tue, May 7, 2024 at 3:53 AM Matthieu Baerts (NGI0) matttbe@kernel.org wrote:
From: Geliang Tang tanggeliang@kylinos.cn
Each MPTCP subtest tests test__start_subtest(suffix), then invokes test_suffix(). It makes sense to add a new macro RUN_MPTCP_TEST to simpolify the code.
Signed-off-by: Geliang Tang tanggeliang@kylinos.cn Reviewed-by: Mat Martineau martineau@kernel.org Signed-off-by: Matthieu Baerts (NGI0) matttbe@kernel.org
tools/testing/selftests/bpf/prog_tests/mptcp.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c index baf976a7a1dd..9d1b255bb654 100644 --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c @@ -347,10 +347,14 @@ static void test_mptcpify(void) close(cgroup_fd); }
+#define RUN_MPTCP_TEST(suffix) \ +do { \
if (test__start_subtest(#suffix)) \test_##suffix(); \+} while (0)
Please no. Don't hide it behind macros.
I understand, I'm personally not a big fan of hiding code being a macro too. This one saves only one line. Geliang added a few more tests in our tree [1], for a total of 9, so that's only saving 9 lines.
Related to that, if you don't mind, Geliang also added another macro -- MPTCP_SCHED_TEST -- for tests that are currently only in our tree [2] (not ready yet). We asked him to reduce the size of this macro to the minimum. We accepted it because it removed quite a lot of similar code with very small differences [3]. Do you think we should revert this modification too?
Yeah. Pls don't hide such things in macros. Refactor into helper function in normal C.
Sure, we will revert that.
But, what do you mean "in your tree" ? That's your development tree and you plan to send all that properly as patches to bpf-next someday?
Yes, correct, we have some WIP patches in MPTCP development tree where we added a new bpf_struct_ops structure to implement new MPTCP packet schedulers in BPF. This work was paused for a while because we had to refine the packet scheduler API, but this task is now ongoing. Hopefully we will be able to send patches to bpf-next this "soon".
Cheers, Matt