From: Matthieu Baerts <matttbe@kernel.org>
To: Geliang Tang <geliang@kernel.org>, Mat Martineau <martineau@kernel.org>
Cc: mptcp@lists.linux.dev
Subject: Re: [PATCH mptcp-next v6 4/9] selftests/bpf: Add MPTCP_SCHED_TEST macro
Date: Mon, 8 Apr 2024 21:58:43 +0200 [thread overview]
Message-ID: <f44d5960-8aa3-4914-bae3-4f0c072e07b4@kernel.org> (raw)
In-Reply-To: <fb0c45182eaab1fd92f1302113d0298bc16d20f6.camel@kernel.org>
Hi Geliang, Mat,
On 08/04/2024 05:10, Geliang Tang wrote:
> Hi Matt,
>
> On Thu, 2024-04-04 at 19:52 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 04/04/2024 15:03, Geliang Tang wrote:
>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>
>>> This patch defines MPTCP_SCHED_TEST macro, a template for all
>>> scheduler
>>> tests. Every scheduler is identified by argument name, and use
>>> sysctl
>>> to set net.mptcp.scheduler as "bpf_name" to use this sched. Add two
>>> veth net devices to simulate the multiple addresses case. Use 'ip
>>> mptcp
>>> endpoint' command to add the new endpoint ADDR2 to PM netlink.
>>> Arguments
>>> addr1/add2 means whether the data has been sent on the first/second
>>> subflow
>>> or not. Send data and check bytes_sent of 'ss' output after it
>>> using
>>> send_data_and_verify().
>>
>> Should we not squash this in "selftests/bpf: Add bpf scheduler test"
>> as
>> well? We already have a lot of different commits.
>>
>>>
>>> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
>>> ---
>>> .../testing/selftests/bpf/prog_tests/mptcp.c | 30
>>> +++++++++++++++++++
>>> 1 file changed, 30 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c
>>> b/tools/testing/selftests/bpf/prog_tests/mptcp.c
>>> index ffcd5ebe38b9..8a6b09a97698 100644
>>> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
>>> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
>>> @@ -498,6 +498,36 @@ static void test_default(void)
>>> cleanup_netns(nstoken);
>>> }
>>>
>>> +#define MPTCP_SCHED_TEST(name, addr1, addr2) \
>>> +static void test_##name(void) \
>>> +{ \
>>> + struct mptcp_bpf_##name *skel; \
>>> + struct nstoken *nstoken; \
>>> + struct bpf_link *link; \
>>> + struct bpf_map *map; \
>>> + \
>>> + skel = mptcp_bpf_##name##__open_and_load(); \
>>> + if (!ASSERT_OK_PTR(skel, "open_and_load " #name)) \
>>> + return;
>>> \
>>> + \
>>> + map = bpf_object__find_map_by_name(skel->obj, #name); \
>>> + link =
>>> bpf_map__attach_struct_ops(map); \
>>> + if (!ASSERT_OK_PTR(link, "attach_struct_ops " #name)) \
>>> + goto skel_destroy; \
>>> + \
>>> + nstoken = sched_init("subflow", "bpf_" #name); \
>>> + if (!ASSERT_OK_PTR(nstoken, "sched_init " #name)) \
>>> + goto link_destroy; \
>>> + \
>>> + send_data_and_verify(#name, atoi(#addr1), atoi(#addr2));\
>>
>> Can you not use the values of addr1 and addr2 directly as number
>> instead
>> of string + atoi()?
>>
>>> + \
>>> + cleanup_netns(nstoken);
>>> \
>>> +link_destroy: \
>>> + bpf_link__destroy(link); \
>>> +skel_destroy: \
>>> + mptcp_bpf_##name##__destroy(skel); \
>>> +}
>>
>> I don't mind having functions defined in macros, but I think we
>> should
>> try to reduce their size to the minimum (if possible) because they
>> are
>> hard to read and debug in case of error.
>>
>> Here, do you think we could have a smaller macro passing the name as
>> a
>> string (for the error messages) and the different functions pointers
>> you
>> need to a new helper? (+ using generic skel structure?)
>
> Thanks for your review, it's very useful. v7 is sent, address all your
> comments except this one. I didn't got it, please give me more details
> about this.
My comment was similar to the one from Mat:
https://lore.kernel.org/mptcp/f9daffc3-c1a6-0ec3-f821-107ab97f551c@kernel.org/
In short: macros can be hard to debug.
I also see why you are using a macro here, all these functions are very
similar. Here, there are more functions that can "reduced", so maybe
that's OK?
@Mat: what do you think? Or maybe another idea?
(Or maybe it is possible to have a "small" macro, passing function
pointers to a (proper) helper? But not sure if it is less complex. And
maybe not worth it for the tests.)
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
next prev parent reply other threads:[~2024-04-08 19:58 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-04 13:03 [PATCH mptcp-next v6 0/9] refactor mptcp bpf tests Geliang Tang
2024-04-04 13:03 ` [PATCH mptcp-next v6 1/9] selftests/bpf: Add RUN_MPTCP_TEST macro Geliang Tang
2024-04-04 13:03 ` [PATCH mptcp-next v6 2/9] Squash to "selftests/bpf: Add bpf scheduler test" 1 verify Geliang Tang
2024-04-04 17:50 ` Matthieu Baerts
2024-04-04 13:03 ` [PATCH mptcp-next v6 3/9] Squash to "selftests/bpf: Add bpf scheduler test" 2 time Geliang Tang
2024-04-04 17:50 ` Matthieu Baerts
2024-04-04 13:03 ` [PATCH mptcp-next v6 4/9] selftests/bpf: Add MPTCP_SCHED_TEST macro Geliang Tang
2024-04-04 17:52 ` Matthieu Baerts
2024-04-08 3:10 ` Geliang Tang
2024-04-08 19:58 ` Matthieu Baerts [this message]
2024-04-04 13:03 ` [PATCH mptcp-next v6 5/9] Squash to "selftests/bpf: Add bpf_first scheduler & test" Geliang Tang
2024-04-04 13:03 ` [PATCH mptcp-next v6 6/9] Squash to "selftests/bpf: Add bpf_bkup " Geliang Tang
2024-04-04 13:03 ` [PATCH mptcp-next v6 7/9] Squash to "selftests/bpf: Add bpf_rr " Geliang Tang
2024-04-04 13:03 ` [PATCH mptcp-next v6 8/9] Squash to "selftests/bpf: Add bpf_red " Geliang Tang
2024-04-04 13:03 ` [PATCH mptcp-next v6 9/9] Squash to "selftests/bpf: Add bpf_burst " Geliang Tang
2024-04-04 13:54 ` [PATCH mptcp-next v6 0/9] refactor mptcp bpf tests MPTCP CI
2024-04-04 17:47 ` Matthieu Baerts
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f44d5960-8aa3-4914-bae3-4f0c072e07b4@kernel.org \
--to=matttbe@kernel.org \
--cc=geliang@kernel.org \
--cc=martineau@kernel.org \
--cc=mptcp@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).