Re: [PATCH mptcp-next v6 4/9] selftests/bpf: Add MPTCP_SCHED_TEST macro

[Geliang Tang <geliang@kernel.org>]

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.

-Geliang

> 
> Maybe it will not work or will not be easier to read, but worth a try
> I
> think. If it is not possible, please explain why.
> 
> 
> > +
> >  static void test_first(void)
> >  {
> >  	struct mptcp_bpf_first *first_skel;
> 
> Cheers,
> Matt