diff mbox series

[bpf-next,v11,2/5] selftests/bpf: Use random netns name for mptcp

Message ID 15d7646940fcbb8477b1be1aa11a5d5485d10b48.1691125344.git.geliang.tang@suse.com (mailing list archive)
State New
Headers show
Series bpf: Force to MPTCP | expand

Commit Message

Geliang Tang Aug. 4, 2023, 5:07 a.m. UTC
Use rand() to generate a random netns name instead of using the fixed
name "mptcp_ns" for every test.

By doing that, we can re-launch the test even if there was an issue
removing the previous netns or if by accident, a netns with this generic
name already existed on the system.

Note that using a different name each will also help adding more
subtests in future commits.

Acked-by: Yonghong Song <yonghong.song@linux.dev>
Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/bpf/prog_tests/mptcp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Martin KaFai Lau Aug. 5, 2023, 12:23 a.m. UTC | #1
On 8/3/23 10:07 PM, Geliang Tang wrote:
> Use rand() to generate a random netns name instead of using the fixed
> name "mptcp_ns" for every test.
> 
> By doing that, we can re-launch the test even if there was an issue
> removing the previous netns or if by accident, a netns with this generic
> name already existed on the system.
> 
> Note that using a different name each will also help adding more
> subtests in future commits.

I run test_progs repeatedly without rebooting qemu to save time. If there is a 
test did not clean up its netns, I would rather uncover it earlier and fix it 
instead. Randomizing the name is hiding the issue and does not help to uncover 
the broken test sooner. Although this change is to mptcp test alone, this could 
be referred in other future tests.

afaik, I don't remember bpf CI ever run into a test failure because the picked 
name had already been used by the system. It seems you ran into this issue a lot 
with the mptcp test in your setup. Could you explain a little more?
Geliang Tang Aug. 7, 2023, 6:40 a.m. UTC | #2
On Fri, Aug 04, 2023 at 05:23:32PM -0700, Martin KaFai Lau wrote:
> On 8/3/23 10:07 PM, Geliang Tang wrote:
> > Use rand() to generate a random netns name instead of using the fixed
> > name "mptcp_ns" for every test.
> > 
> > By doing that, we can re-launch the test even if there was an issue
> > removing the previous netns or if by accident, a netns with this generic
> > name already existed on the system.
> > 
> > Note that using a different name each will also help adding more
> > subtests in future commits.

Hi Martin,

I tried to run mptcp tests simultaneously, and got "Cannot create
namespace file "/var/run/netns/mptcp_ns": File exists" errors sometimes.
So I add this patch to fix it.

It's easy to reproduce, just run this commands in multiple terminals:
 > for i in `seq 1 100`; do sudo ./test_progs -t mptcp; done

> 
> I run test_progs repeatedly without rebooting qemu to save time. If there is
> a test did not clean up its netns, I would rather uncover it earlier and fix
> it instead. Randomizing the name is hiding the issue and does not help to
> uncover the broken test sooner. Although this change is to mptcp test alone,
> this could be referred in other future tests.

I added "ip netns show" after "ip netns del" in v12 to check if there is
a test did not clean up its netns.

Thanks,
-Geliang

> 
> afaik, I don't remember bpf CI ever run into a test failure because the
> picked name had already been used by the system. It seems you ran into this
> issue a lot with the mptcp test in your setup. Could you explain a little
> more?
Martin KaFai Lau Aug. 9, 2023, 6:03 a.m. UTC | #3
On 8/6/23 11:40 PM, Geliang Tang wrote:
> On Fri, Aug 04, 2023 at 05:23:32PM -0700, Martin KaFai Lau wrote:
>> On 8/3/23 10:07 PM, Geliang Tang wrote:
>>> Use rand() to generate a random netns name instead of using the fixed
>>> name "mptcp_ns" for every test.
>>>
>>> By doing that, we can re-launch the test even if there was an issue
>>> removing the previous netns or if by accident, a netns with this generic
>>> name already existed on the system.
>>>
>>> Note that using a different name each will also help adding more
>>> subtests in future commits.
> 
> Hi Martin,
> 
> I tried to run mptcp tests simultaneously, and got "Cannot create
> namespace file "/var/run/netns/mptcp_ns": File exists" errors sometimes.
> So I add this patch to fix it.
> 
> It's easy to reproduce, just run this commands in multiple terminals:
>   > for i in `seq 1 100`; do sudo ./test_progs -t mptcp; done

Not only the "-t mptcp" test. Other tests in test_progs also don't support 
running parallel in multiple terminals. Does it really help to test the bpf part 
of the prog_tests/mptcp.c test by running like this? If it wants to exercise the 
other mptcp networking specific code like this, a separate mptcp test is needed 
outside of test_progs and it won't be run in the bpf CI.

If you agree, can you please avoid introducing unnecessary randomness to the 
test_progs where bpf CI and most users don't run in this way?

Also, please don't resend the patches too fast until the discussion is 
concluded. Please give reasonable time for others to reply.

I have a high level question. In LPC 2022 
(https://lpc.events/event/16/contributions/1354/), I recall there was idea in 
using bpf to make other mptcp decision/policy. Any thought and progress on this? 
This set which only uses bpf to change the protocol feels like an incomplete 
solution.
Geliang Tang Aug. 9, 2023, 8:19 a.m. UTC | #4
On Tue, Aug 08, 2023 at 11:03:30PM -0700, Martin KaFai Lau wrote:
> On 8/6/23 11:40 PM, Geliang Tang wrote:
> > On Fri, Aug 04, 2023 at 05:23:32PM -0700, Martin KaFai Lau wrote:
> > > On 8/3/23 10:07 PM, Geliang Tang wrote:
> > > > Use rand() to generate a random netns name instead of using the fixed
> > > > name "mptcp_ns" for every test.
> > > > 
> > > > By doing that, we can re-launch the test even if there was an issue
> > > > removing the previous netns or if by accident, a netns with this generic
> > > > name already existed on the system.
> > > > 
> > > > Note that using a different name each will also help adding more
> > > > subtests in future commits.
> > 
> > Hi Martin,
> > 
> > I tried to run mptcp tests simultaneously, and got "Cannot create
> > namespace file "/var/run/netns/mptcp_ns": File exists" errors sometimes.
> > So I add this patch to fix it.
> > 
> > It's easy to reproduce, just run this commands in multiple terminals:
> >   > for i in `seq 1 100`; do sudo ./test_progs -t mptcp; done
> 
> Not only the "-t mptcp" test. Other tests in test_progs also don't support
> running parallel in multiple terminals. Does it really help to test the bpf
> part of the prog_tests/mptcp.c test by running like this? If it wants to
> exercise the other mptcp networking specific code like this, a separate
> mptcp test is needed outside of test_progs and it won't be run in the bpf
> CI.
> 
> If you agree, can you please avoid introducing unnecessary randomness to the
> test_progs where bpf CI and most users don't run in this way?

Thanks Martin. Sure, I agree. Let's drop this patch.

> 
> Also, please don't resend the patches too fast until the discussion is
> concluded. Please give reasonable time for others to reply.

Sure. Please give me a clear reminder next time that it's time to resend
a new version of the patches.

> 
> I have a high level question. In LPC 2022
> (https://lpc.events/event/16/contributions/1354/), I recall there was idea
> in using bpf to make other mptcp decision/policy. Any thought and progress
> on this? This set which only uses bpf to change the protocol feels like an
> incomplete solution.

We are implementing MPTCP packet scheduler using BPF. Patches aren't
sent to BPF mail list yet, only temporarily on our mptcp repo[1].

Here are the patches:

 selftests/bpf: Add bpf_burst test
 selftests/bpf: Add bpf_burst scheduler
 bpf: Export more bpf_burst related functions
 selftests/bpf: Add bpf_red test
 selftests/bpf: Add bpf_red scheduler
 selftests/bpf: Add bpf_rr test
 selftests/bpf: Add bpf_rr scheduler
 selftests/bpf: Add bpf_bkup test
 selftests/bpf: Add bpf_bkup scheduler
 selftests/bpf: Add bpf_first test
 selftests/bpf: Add bpf_first scheduler
 selftests/bpf: Add bpf scheduler test
 selftests/bpf: add two mptcp netns helpers
 selftests/bpf: use random netns name for mptcp
 selftests/bpf: Add mptcp sched structs
 bpf: Add bpf_mptcp_sched_kfunc_set
 bpf: Add bpf_mptcp_sched_ops

If you could take a look at these patches in advance, I would greatly
appreciate it. Any feedback is welcome.

[1]
https://github.com/multipath-tcp/mptcp_net-next.git

-Geliang

>
Martin KaFai Lau Aug. 11, 2023, 5:53 a.m. UTC | #5
On 8/9/23 1:19 AM, Geliang Tang wrote:
> On Tue, Aug 08, 2023 at 11:03:30PM -0700, Martin KaFai Lau wrote:
>> On 8/6/23 11:40 PM, Geliang Tang wrote:
>>> On Fri, Aug 04, 2023 at 05:23:32PM -0700, Martin KaFai Lau wrote:
>>>> On 8/3/23 10:07 PM, Geliang Tang wrote:
>>>>> Use rand() to generate a random netns name instead of using the fixed
>>>>> name "mptcp_ns" for every test.
>>>>>
>>>>> By doing that, we can re-launch the test even if there was an issue
>>>>> removing the previous netns or if by accident, a netns with this generic
>>>>> name already existed on the system.
>>>>>
>>>>> Note that using a different name each will also help adding more
>>>>> subtests in future commits.
>>>
>>> Hi Martin,
>>>
>>> I tried to run mptcp tests simultaneously, and got "Cannot create
>>> namespace file "/var/run/netns/mptcp_ns": File exists" errors sometimes.
>>> So I add this patch to fix it.
>>>
>>> It's easy to reproduce, just run this commands in multiple terminals:
>>>    > for i in `seq 1 100`; do sudo ./test_progs -t mptcp; done
>>
>> Not only the "-t mptcp" test. Other tests in test_progs also don't support
>> running parallel in multiple terminals. Does it really help to test the bpf
>> part of the prog_tests/mptcp.c test by running like this? If it wants to
>> exercise the other mptcp networking specific code like this, a separate
>> mptcp test is needed outside of test_progs and it won't be run in the bpf
>> CI.
>>
>> If you agree, can you please avoid introducing unnecessary randomness to the
>> test_progs where bpf CI and most users don't run in this way?
> 
> Thanks Martin. Sure, I agree. Let's drop this patch.

Thanks you.

>> I have a high level question. In LPC 2022
>> (https://lpc.events/event/16/contributions/1354/), I recall there was idea
>> in using bpf to make other mptcp decision/policy. Any thought and progress
>> on this? This set which only uses bpf to change the protocol feels like an
>> incomplete solution.
> 
> We are implementing MPTCP packet scheduler using BPF. Patches aren't
> sent to BPF mail list yet, only temporarily on our mptcp repo[1].
> 
> Here are the patches:
> 
>   selftests/bpf: Add bpf_burst test
>   selftests/bpf: Add bpf_burst scheduler
>   bpf: Export more bpf_burst related functions
>   selftests/bpf: Add bpf_red test
>   selftests/bpf: Add bpf_red scheduler
>   selftests/bpf: Add bpf_rr test
>   selftests/bpf: Add bpf_rr scheduler
>   selftests/bpf: Add bpf_bkup test
>   selftests/bpf: Add bpf_bkup scheduler
>   selftests/bpf: Add bpf_first test
>   selftests/bpf: Add bpf_first scheduler
>   selftests/bpf: Add bpf scheduler test
>   selftests/bpf: add two mptcp netns helpers
>   selftests/bpf: use random netns name for mptcp
>   selftests/bpf: Add mptcp sched structs
>   bpf: Add bpf_mptcp_sched_kfunc_set
>   bpf: Add bpf_mptcp_sched_ops
> 
> If you could take a look at these patches in advance, I would greatly
> appreciate it. Any feedback is welcome.
> 
> [1]
> https://github.com/multipath-tcp/mptcp_net-next.git

Thanks for sharing. I did not go into the details. iiuc, the scheduler is 
specific to a namespace. Do you see if it is useful to have more finer control 
like depending on what IP address it is connected to? BPF policy is usually 
found more useful to have finer policy control than global or per-netns.

The same question goes for the fmod_ret here in this patch. The progs/mptcpify.c 
selftest is as good as upgrading all TCP connections. Is it your only use case 
and no need for finer selection?
Geliang Tang Aug. 11, 2023, 9:29 a.m. UTC | #6
On Thu, Aug 10, 2023 at 10:53:38PM -0700, Martin KaFai Lau wrote:
> On 8/9/23 1:19 AM, Geliang Tang wrote:
> > On Tue, Aug 08, 2023 at 11:03:30PM -0700, Martin KaFai Lau wrote:
> > > On 8/6/23 11:40 PM, Geliang Tang wrote:
> > > > On Fri, Aug 04, 2023 at 05:23:32PM -0700, Martin KaFai Lau wrote:
> > > > > On 8/3/23 10:07 PM, Geliang Tang wrote:
> > > > > > Use rand() to generate a random netns name instead of using the fixed
> > > > > > name "mptcp_ns" for every test.
> > > > > > 
> > > > > > By doing that, we can re-launch the test even if there was an issue
> > > > > > removing the previous netns or if by accident, a netns with this generic
> > > > > > name already existed on the system.
> > > > > > 
> > > > > > Note that using a different name each will also help adding more
> > > > > > subtests in future commits.
> > > > 
> > > > Hi Martin,
> > > > 
> > > > I tried to run mptcp tests simultaneously, and got "Cannot create
> > > > namespace file "/var/run/netns/mptcp_ns": File exists" errors sometimes.
> > > > So I add this patch to fix it.
> > > > 
> > > > It's easy to reproduce, just run this commands in multiple terminals:
> > > >    > for i in `seq 1 100`; do sudo ./test_progs -t mptcp; done
> > > 
> > > Not only the "-t mptcp" test. Other tests in test_progs also don't support
> > > running parallel in multiple terminals. Does it really help to test the bpf
> > > part of the prog_tests/mptcp.c test by running like this? If it wants to
> > > exercise the other mptcp networking specific code like this, a separate
> > > mptcp test is needed outside of test_progs and it won't be run in the bpf
> > > CI.
> > > 
> > > If you agree, can you please avoid introducing unnecessary randomness to the
> > > test_progs where bpf CI and most users don't run in this way?
> > 
> > Thanks Martin. Sure, I agree. Let's drop this patch.
> 
> Thanks you.
> 
> > > I have a high level question. In LPC 2022
> > > (https://lpc.events/event/16/contributions/1354/), I recall there was idea
> > > in using bpf to make other mptcp decision/policy. Any thought and progress
> > > on this? This set which only uses bpf to change the protocol feels like an
> > > incomplete solution.
> > 
> > We are implementing MPTCP packet scheduler using BPF. Patches aren't
> > sent to BPF mail list yet, only temporarily on our mptcp repo[1].
> > 
> > Here are the patches:
> > 
> >   selftests/bpf: Add bpf_burst test
> >   selftests/bpf: Add bpf_burst scheduler
> >   bpf: Export more bpf_burst related functions
> >   selftests/bpf: Add bpf_red test
> >   selftests/bpf: Add bpf_red scheduler
> >   selftests/bpf: Add bpf_rr test
> >   selftests/bpf: Add bpf_rr scheduler
> >   selftests/bpf: Add bpf_bkup test
> >   selftests/bpf: Add bpf_bkup scheduler
> >   selftests/bpf: Add bpf_first test
> >   selftests/bpf: Add bpf_first scheduler
> >   selftests/bpf: Add bpf scheduler test
> >   selftests/bpf: add two mptcp netns helpers
> >   selftests/bpf: use random netns name for mptcp
> >   selftests/bpf: Add mptcp sched structs
> >   bpf: Add bpf_mptcp_sched_kfunc_set
> >   bpf: Add bpf_mptcp_sched_ops
> > 
> > If you could take a look at these patches in advance, I would greatly
> > appreciate it. Any feedback is welcome.
> > 
> > [1]
> > https://github.com/multipath-tcp/mptcp_net-next.git
> 
> Thanks for sharing. I did not go into the details. iiuc, the scheduler is
> specific to a namespace. Do you see if it is useful to have more finer
> control like depending on what IP address it is connected to? BPF policy is
> usually found more useful to have finer policy control than global or
> per-netns.
> 
> The same question goes for the fmod_ret here in this patch. The
> progs/mptcpify.c selftest is as good as upgrading all TCP connections. Is it
> your only use case and no need for finer selection?

This per-netns control is just the first step. We do need finer selection. The
most ideal mode is to select one app to upgrade it's TCP connections only. So
per-cgroup control is much better than per-netns. But we haven't found a good
per-cgroup solution yet.

Thanks,
-Geliang

>
Martin KaFai Lau Aug. 11, 2023, 6:50 p.m. UTC | #7
On 8/11/23 2:29 AM, Geliang Tang wrote:
> On Thu, Aug 10, 2023 at 10:53:38PM -0700, Martin KaFai Lau wrote:
>> On 8/9/23 1:19 AM, Geliang Tang wrote:
>>> On Tue, Aug 08, 2023 at 11:03:30PM -0700, Martin KaFai Lau wrote:
>>>> On 8/6/23 11:40 PM, Geliang Tang wrote:
>>>>> On Fri, Aug 04, 2023 at 05:23:32PM -0700, Martin KaFai Lau wrote:
>>>>>> On 8/3/23 10:07 PM, Geliang Tang wrote:
>>>>>>> Use rand() to generate a random netns name instead of using the fixed
>>>>>>> name "mptcp_ns" for every test.
>>>>>>>
>>>>>>> By doing that, we can re-launch the test even if there was an issue
>>>>>>> removing the previous netns or if by accident, a netns with this generic
>>>>>>> name already existed on the system.
>>>>>>>
>>>>>>> Note that using a different name each will also help adding more
>>>>>>> subtests in future commits.
>>>>>
>>>>> Hi Martin,
>>>>>
>>>>> I tried to run mptcp tests simultaneously, and got "Cannot create
>>>>> namespace file "/var/run/netns/mptcp_ns": File exists" errors sometimes.
>>>>> So I add this patch to fix it.
>>>>>
>>>>> It's easy to reproduce, just run this commands in multiple terminals:
>>>>>     > for i in `seq 1 100`; do sudo ./test_progs -t mptcp; done
>>>>
>>>> Not only the "-t mptcp" test. Other tests in test_progs also don't support
>>>> running parallel in multiple terminals. Does it really help to test the bpf
>>>> part of the prog_tests/mptcp.c test by running like this? If it wants to
>>>> exercise the other mptcp networking specific code like this, a separate
>>>> mptcp test is needed outside of test_progs and it won't be run in the bpf
>>>> CI.
>>>>
>>>> If you agree, can you please avoid introducing unnecessary randomness to the
>>>> test_progs where bpf CI and most users don't run in this way?
>>>
>>> Thanks Martin. Sure, I agree. Let's drop this patch.
>>
>> Thanks you.
>>
>>>> I have a high level question. In LPC 2022
>>>> (https://lpc.events/event/16/contributions/1354/), I recall there was idea
>>>> in using bpf to make other mptcp decision/policy. Any thought and progress
>>>> on this? This set which only uses bpf to change the protocol feels like an
>>>> incomplete solution.
>>>
>>> We are implementing MPTCP packet scheduler using BPF. Patches aren't
>>> sent to BPF mail list yet, only temporarily on our mptcp repo[1].
>>>
>>> Here are the patches:
>>>
>>>    selftests/bpf: Add bpf_burst test
>>>    selftests/bpf: Add bpf_burst scheduler
>>>    bpf: Export more bpf_burst related functions
>>>    selftests/bpf: Add bpf_red test
>>>    selftests/bpf: Add bpf_red scheduler
>>>    selftests/bpf: Add bpf_rr test
>>>    selftests/bpf: Add bpf_rr scheduler
>>>    selftests/bpf: Add bpf_bkup test
>>>    selftests/bpf: Add bpf_bkup scheduler
>>>    selftests/bpf: Add bpf_first test
>>>    selftests/bpf: Add bpf_first scheduler
>>>    selftests/bpf: Add bpf scheduler test
>>>    selftests/bpf: add two mptcp netns helpers
>>>    selftests/bpf: use random netns name for mptcp
>>>    selftests/bpf: Add mptcp sched structs
>>>    bpf: Add bpf_mptcp_sched_kfunc_set
>>>    bpf: Add bpf_mptcp_sched_ops
>>>
>>> If you could take a look at these patches in advance, I would greatly
>>> appreciate it. Any feedback is welcome.
>>>
>>> [1]
>>> https://github.com/multipath-tcp/mptcp_net-next.git
>>
>> Thanks for sharing. I did not go into the details. iiuc, the scheduler is
>> specific to a namespace. Do you see if it is useful to have more finer
>> control like depending on what IP address it is connected to? BPF policy is
>> usually found more useful to have finer policy control than global or
>> per-netns.
>>
>> The same question goes for the fmod_ret here in this patch. The
>> progs/mptcpify.c selftest is as good as upgrading all TCP connections. Is it
>> your only use case and no need for finer selection?
> 
> This per-netns control is just the first step. We do need finer selection. The
> most ideal mode is to select one app to upgrade it's TCP connections only. So
> per-cgroup control is much better than per-netns. But we haven't found a good
> per-cgroup solution yet.

Selecting an app or cgroup can sort of be done by getting the current task or 
current cgroup (there is helper to do that). I am imagining eventually it will 
want to decide the protocol upgrade and/or the mptcp-scheduler when the 
destination IP is decided. This fmod_ret upgrade for all acts like a global knob 
(sysctl) and feels like a hack or at least incomplete. However, I also don't see 
a clean way to do that for now in the current shape.

Please respin another revision to address the earlier selftest comment on the 
netns name. Thanks.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index cd0c42fff7c0..4ccca3d39a8f 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -7,7 +7,7 @@ 
 #include "network_helpers.h"
 #include "mptcp_sock.skel.h"
 
-#define NS_TEST "mptcp_ns"
+char NS_TEST[32];
 
 #ifndef TCP_CA_NAME_MAX
 #define TCP_CA_NAME_MAX	16
@@ -147,6 +147,8 @@  static void test_base(void)
 	if (!ASSERT_GE(cgroup_fd, 0, "test__join_cgroup"))
 		return;
 
+	srand(time(NULL));
+	snprintf(NS_TEST, sizeof(NS_TEST), "mptcp_ns_%d", rand());
 	SYS(fail, "ip netns add %s", NS_TEST);
 	SYS(fail, "ip -net %s link set dev lo up", NS_TEST);