1. Patchew
  2. MPTCP
  3. [PATCH mptcp-net v2 0/6] selftests: mptcp: join: fix flaky tests
  4. View patch

[PATCH mptcp-net v2 2/6] selftests: mptcp: join: rm: set backup flag

Matthieu Baerts (NGI0) posted 6 patches 3 weeks, 4 days ago
[PATCH mptcp-net v2 2/6] selftests: mptcp: join: rm: set backup flag
Posted by Matthieu Baerts (NGI0) 3 weeks, 4 days ago 
Some of these 'remove' tests rarely fail because a subflow has been
reset instead of cleanly removed. This can happen when one extra subflow
which has never carried data is being closed (FIN) on one side, while
the other is sending data for the first time.

To avoid such subflows to be used right at the end, the backup flag has
been added. With that, data will be only carried on the initial subflow.

Fixes: d2c4333a801c ("selftests: mptcp: add testcases for removing addrs")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 54 ++++++++++++-------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index e0cdb9c662aa..399805812197 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -2552,7 +2552,7 @@ remove_tests()
 	if reset "remove single subflow"; then
 		pm_nl_set_limits $ns1 0 1
 		pm_nl_set_limits $ns2 0 1
-		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
+		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow,backup
 		addr_nr_ns2=-1 speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 1 1 1
@@ -2565,8 +2565,8 @@ remove_tests()
 	if reset "remove multiple subflows"; then
 		pm_nl_set_limits $ns1 0 2
 		pm_nl_set_limits $ns2 0 2
-		pm_nl_add_endpoint $ns2 10.0.2.2 flags subflow
-		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
+		pm_nl_add_endpoint $ns2 10.0.2.2 flags subflow,backup
+		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow,backup
 		addr_nr_ns2=-2 speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 2 2 2
@@ -2577,7 +2577,7 @@ remove_tests()
 	# single address, remove
 	if reset "remove single address"; then
 		pm_nl_set_limits $ns1 0 1
-		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
+		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup
 		pm_nl_set_limits $ns2 1 1
 		addr_nr_ns1=-1 speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
@@ -2590,9 +2590,9 @@ remove_tests()
 	# subflow and signal, remove
 	if reset "remove subflow and signal"; then
 		pm_nl_set_limits $ns1 0 2
-		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
+		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup
 		pm_nl_set_limits $ns2 1 2
-		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
+		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow,backup
 		addr_nr_ns1=-1 addr_nr_ns2=-1 speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 2 2 2
@@ -2604,10 +2604,10 @@ remove_tests()
 	# subflows and signal, remove
 	if reset "remove subflows and signal"; then
 		pm_nl_set_limits $ns1 0 3
-		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
+		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup
 		pm_nl_set_limits $ns2 1 3
-		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
-		pm_nl_add_endpoint $ns2 10.0.4.2 flags subflow
+		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow,backup
+		pm_nl_add_endpoint $ns2 10.0.4.2 flags subflow,backup
 		addr_nr_ns1=-1 addr_nr_ns2=-2 speed=10 \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 3 3 3
@@ -2619,9 +2619,9 @@ remove_tests()
 	# addresses remove
 	if reset "remove addresses"; then
 		pm_nl_set_limits $ns1 3 3
-		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal id 250
-		pm_nl_add_endpoint $ns1 10.0.3.1 flags signal
-		pm_nl_add_endpoint $ns1 10.0.4.1 flags signal
+		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup id 250
+		pm_nl_add_endpoint $ns1 10.0.3.1 flags signal,backup
+		pm_nl_add_endpoint $ns1 10.0.4.1 flags signal,backup
 		pm_nl_set_limits $ns2 3 3
 		addr_nr_ns1=-3 speed=10 \
 			run_tests $ns1 $ns2 10.0.1.1
@@ -2634,10 +2634,10 @@ remove_tests()
 	# invalid addresses remove
 	if reset "remove invalid addresses"; then
 		pm_nl_set_limits $ns1 3 3
-		pm_nl_add_endpoint $ns1 10.0.12.1 flags signal
+		pm_nl_add_endpoint $ns1 10.0.12.1 flags signal,backup
 		# broadcast IP: no packet for this address will be received on ns1
-		pm_nl_add_endpoint $ns1 224.0.0.1 flags signal
-		pm_nl_add_endpoint $ns1 10.0.3.1 flags signal
+		pm_nl_add_endpoint $ns1 224.0.0.1 flags signal,backup
+		pm_nl_add_endpoint $ns1 10.0.3.1 flags signal,backup
 		pm_nl_set_limits $ns2 2 2
 		addr_nr_ns1=-3 speed=10 \
 			run_tests $ns1 $ns2 10.0.1.1
@@ -2651,10 +2651,10 @@ remove_tests()
 	# subflows and signal, flush
 	if reset "flush subflows and signal"; then
 		pm_nl_set_limits $ns1 0 3
-		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
+		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup
 		pm_nl_set_limits $ns2 1 3
-		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
-		pm_nl_add_endpoint $ns2 10.0.4.2 flags subflow
+		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow,backup
+		pm_nl_add_endpoint $ns2 10.0.4.2 flags subflow,backup
 		addr_nr_ns1=-8 addr_nr_ns2=-8 speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 3 3 3
@@ -2667,9 +2667,9 @@ remove_tests()
 	if reset "flush subflows"; then
 		pm_nl_set_limits $ns1 3 3
 		pm_nl_set_limits $ns2 3 3
-		pm_nl_add_endpoint $ns2 10.0.2.2 flags subflow id 150
-		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
-		pm_nl_add_endpoint $ns2 10.0.4.2 flags subflow
+		pm_nl_add_endpoint $ns2 10.0.2.2 flags subflow,backup id 150
+		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow,backup
+		pm_nl_add_endpoint $ns2 10.0.4.2 flags subflow,backup
 		addr_nr_ns1=-8 addr_nr_ns2=-8 speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 3 3 3
@@ -2686,9 +2686,9 @@ remove_tests()
 	# addresses flush
 	if reset "flush addresses"; then
 		pm_nl_set_limits $ns1 3 3
-		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal id 250
-		pm_nl_add_endpoint $ns1 10.0.3.1 flags signal
-		pm_nl_add_endpoint $ns1 10.0.4.1 flags signal
+		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup id 250
+		pm_nl_add_endpoint $ns1 10.0.3.1 flags signal,backup
+		pm_nl_add_endpoint $ns1 10.0.4.1 flags signal,backup
 		pm_nl_set_limits $ns2 3 3
 		addr_nr_ns1=-8 addr_nr_ns2=-8 speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
@@ -2701,9 +2701,9 @@ remove_tests()
 	# invalid addresses flush
 	if reset "flush invalid addresses"; then
 		pm_nl_set_limits $ns1 3 3
-		pm_nl_add_endpoint $ns1 10.0.12.1 flags signal
-		pm_nl_add_endpoint $ns1 10.0.3.1 flags signal
-		pm_nl_add_endpoint $ns1 10.0.14.1 flags signal
+		pm_nl_add_endpoint $ns1 10.0.12.1 flags signal,backup
+		pm_nl_add_endpoint $ns1 10.0.3.1 flags signal,backup
+		pm_nl_add_endpoint $ns1 10.0.14.1 flags signal,backup
 		pm_nl_set_limits $ns2 3 3
 		addr_nr_ns1=-8 speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1

-- 
2.51.0
Re: [PATCH mptcp-net v2 2/6] selftests: mptcp: join: rm: set backup flag
Posted by Geliang Tang 3 weeks, 2 days ago 
Hi Matt,

Thanks for this fix. It works indeed.

On Sun, 2025-11-02 at 12:30 +0100, Matthieu Baerts (NGI0) wrote:
> Some of these 'remove' tests rarely fail because a subflow has been

In my testing, only the "flush addresses" test case fails
intermittently. I'm wondering which other test cases have failed in
your environment?

I'm thinking we shouldn't add the "backup" flag to every test in
remove_tests, but only to the specific ones that are actually failing.

Similarly for patches 3 and 4, we don't need to add "test_linkfail=128"
to every test. We should only apply it to the specific ones that are
actually failing.

WDYT?

Thanks,
-Geliang

> reset instead of cleanly removed. This can happen when one extra
> subflow
> which has never carried data is being closed (FIN) on one side, while
> the other is sending data for the first time.
> 
> To avoid such subflows to be used right at the end, the backup flag
> has
> been added. With that, data will be only carried on the initial
> subflow.
> 
> Fixes: d2c4333a801c ("selftests: mptcp: add testcases for removing
> addrs")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 54 ++++++++++++---
> ----------
>  1 file changed, 27 insertions(+), 27 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index e0cdb9c662aa..399805812197 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -2552,7 +2552,7 @@ remove_tests()
>  	if reset "remove single subflow"; then
>  		pm_nl_set_limits $ns1 0 1
>  		pm_nl_set_limits $ns2 0 1
> -		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
> +		pm_nl_add_endpoint $ns2 10.0.3.2 flags
> subflow,backup
>  		addr_nr_ns2=-1 speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 1 1 1
> @@ -2565,8 +2565,8 @@ remove_tests()
>  	if reset "remove multiple subflows"; then
>  		pm_nl_set_limits $ns1 0 2
>  		pm_nl_set_limits $ns2 0 2
> -		pm_nl_add_endpoint $ns2 10.0.2.2 flags subflow
> -		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
> +		pm_nl_add_endpoint $ns2 10.0.2.2 flags
> subflow,backup
> +		pm_nl_add_endpoint $ns2 10.0.3.2 flags
> subflow,backup
>  		addr_nr_ns2=-2 speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 2 2 2
> @@ -2577,7 +2577,7 @@ remove_tests()
>  	# single address, remove
>  	if reset "remove single address"; then
>  		pm_nl_set_limits $ns1 0 1
> -		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
> +		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup
>  		pm_nl_set_limits $ns2 1 1
>  		addr_nr_ns1=-1 speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
> @@ -2590,9 +2590,9 @@ remove_tests()
>  	# subflow and signal, remove
>  	if reset "remove subflow and signal"; then
>  		pm_nl_set_limits $ns1 0 2
> -		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
> +		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup
>  		pm_nl_set_limits $ns2 1 2
> -		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
> +		pm_nl_add_endpoint $ns2 10.0.3.2 flags
> subflow,backup
>  		addr_nr_ns1=-1 addr_nr_ns2=-1 speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 2 2 2
> @@ -2604,10 +2604,10 @@ remove_tests()
>  	# subflows and signal, remove
>  	if reset "remove subflows and signal"; then
>  		pm_nl_set_limits $ns1 0 3
> -		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
> +		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup
>  		pm_nl_set_limits $ns2 1 3
> -		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
> -		pm_nl_add_endpoint $ns2 10.0.4.2 flags subflow
> +		pm_nl_add_endpoint $ns2 10.0.3.2 flags
> subflow,backup
> +		pm_nl_add_endpoint $ns2 10.0.4.2 flags
> subflow,backup
>  		addr_nr_ns1=-1 addr_nr_ns2=-2 speed=10 \
>  			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 3 3 3
> @@ -2619,9 +2619,9 @@ remove_tests()
>  	# addresses remove
>  	if reset "remove addresses"; then
>  		pm_nl_set_limits $ns1 3 3
> -		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal id 250
> -		pm_nl_add_endpoint $ns1 10.0.3.1 flags signal
> -		pm_nl_add_endpoint $ns1 10.0.4.1 flags signal
> +		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup
> id 250
> +		pm_nl_add_endpoint $ns1 10.0.3.1 flags signal,backup
> +		pm_nl_add_endpoint $ns1 10.0.4.1 flags signal,backup
>  		pm_nl_set_limits $ns2 3 3
>  		addr_nr_ns1=-3 speed=10 \
>  			run_tests $ns1 $ns2 10.0.1.1
> @@ -2634,10 +2634,10 @@ remove_tests()
>  	# invalid addresses remove
>  	if reset "remove invalid addresses"; then
>  		pm_nl_set_limits $ns1 3 3
> -		pm_nl_add_endpoint $ns1 10.0.12.1 flags signal
> +		pm_nl_add_endpoint $ns1 10.0.12.1 flags
> signal,backup
>  		# broadcast IP: no packet for this address will be
> received on ns1
> -		pm_nl_add_endpoint $ns1 224.0.0.1 flags signal
> -		pm_nl_add_endpoint $ns1 10.0.3.1 flags signal
> +		pm_nl_add_endpoint $ns1 224.0.0.1 flags
> signal,backup
> +		pm_nl_add_endpoint $ns1 10.0.3.1 flags signal,backup
>  		pm_nl_set_limits $ns2 2 2
>  		addr_nr_ns1=-3 speed=10 \
>  			run_tests $ns1 $ns2 10.0.1.1
> @@ -2651,10 +2651,10 @@ remove_tests()
>  	# subflows and signal, flush
>  	if reset "flush subflows and signal"; then
>  		pm_nl_set_limits $ns1 0 3
> -		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
> +		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup
>  		pm_nl_set_limits $ns2 1 3
> -		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
> -		pm_nl_add_endpoint $ns2 10.0.4.2 flags subflow
> +		pm_nl_add_endpoint $ns2 10.0.3.2 flags
> subflow,backup
> +		pm_nl_add_endpoint $ns2 10.0.4.2 flags
> subflow,backup
>  		addr_nr_ns1=-8 addr_nr_ns2=-8 speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 3 3 3
> @@ -2667,9 +2667,9 @@ remove_tests()
>  	if reset "flush subflows"; then
>  		pm_nl_set_limits $ns1 3 3
>  		pm_nl_set_limits $ns2 3 3
> -		pm_nl_add_endpoint $ns2 10.0.2.2 flags subflow id
> 150
> -		pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
> -		pm_nl_add_endpoint $ns2 10.0.4.2 flags subflow
> +		pm_nl_add_endpoint $ns2 10.0.2.2 flags
> subflow,backup id 150
> +		pm_nl_add_endpoint $ns2 10.0.3.2 flags
> subflow,backup
> +		pm_nl_add_endpoint $ns2 10.0.4.2 flags
> subflow,backup
>  		addr_nr_ns1=-8 addr_nr_ns2=-8 speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 3 3 3
> @@ -2686,9 +2686,9 @@ remove_tests()
>  	# addresses flush
>  	if reset "flush addresses"; then
>  		pm_nl_set_limits $ns1 3 3
> -		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal id 250
> -		pm_nl_add_endpoint $ns1 10.0.3.1 flags signal
> -		pm_nl_add_endpoint $ns1 10.0.4.1 flags signal
> +		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup
> id 250
> +		pm_nl_add_endpoint $ns1 10.0.3.1 flags signal,backup
> +		pm_nl_add_endpoint $ns1 10.0.4.1 flags signal,backup
>  		pm_nl_set_limits $ns2 3 3
>  		addr_nr_ns1=-8 addr_nr_ns2=-8 speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
> @@ -2701,9 +2701,9 @@ remove_tests()
>  	# invalid addresses flush
>  	if reset "flush invalid addresses"; then
>  		pm_nl_set_limits $ns1 3 3
> -		pm_nl_add_endpoint $ns1 10.0.12.1 flags signal
> -		pm_nl_add_endpoint $ns1 10.0.3.1 flags signal
> -		pm_nl_add_endpoint $ns1 10.0.14.1 flags signal
> +		pm_nl_add_endpoint $ns1 10.0.12.1 flags
> signal,backup
> +		pm_nl_add_endpoint $ns1 10.0.3.1 flags signal,backup
> +		pm_nl_add_endpoint $ns1 10.0.14.1 flags
> signal,backup
>  		pm_nl_set_limits $ns2 3 3
>  		addr_nr_ns1=-8 speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
>
Re: [PATCH mptcp-net v2 2/6] selftests: mptcp: join: rm: set backup flag
Posted by Matthieu Baerts 3 weeks, 2 days ago 
Hi Geliang,

Thank you for the review.

4 Nov 2025 07:06:34 Geliang Tang <geliang@kernel.org>:

> Hi Matt,
>
> Thanks for this fix. It works indeed.
>
> On Sun, 2025-11-02 at 12:30 +0100, Matthieu Baerts (NGI0) wrote:
>> Some of these 'remove' tests rarely fail because a subflow has been
>
> In my testing, only the "flush addresses" test case fails
> intermittently. I'm wondering which other test cases have failed in
> your environment?

I took the unstable ones from the Netdev and MPTCP CIs:

https://netdev.bots.linux.dev/contest.html?pass=0&executor=vmksft-mptcp-dbg&ld-cases=1

https://ci-results.mptcp.dev/flakes.html

> I'm thinking we shouldn't add the "backup" flag to every test in
> remove_tests, but only to the specific ones that are actually failing.
>
> Similarly for patches 3 and 4, we don't need to add "test_linkfail=128"
> to every test. We should only apply it to the specific ones that are
> actually failing.

If my analysis is correct, it means the simple fixes I did (backup and
longer file size) can be applied to multiple tests. Then probably better
to avoid these other tests to fail for the same reasons even if it is very
rare, and very hard to reproduce, no? Each false positive takes a lot of
resources (mainly time) to debug.

Cheers,
Matt
Re: [PATCH mptcp-net v2 2/6] selftests: mptcp: join: rm: set backup flag
Posted by Geliang Tang 3 weeks, 2 days ago 
Hi Matt,

On Tue, 2025-11-04 at 07:57 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for the review.
> 
> 4 Nov 2025 07:06:34 Geliang Tang <geliang@kernel.org>:
> 
> > Hi Matt,
> > 
> > Thanks for this fix. It works indeed.
> > 
> > On Sun, 2025-11-02 at 12:30 +0100, Matthieu Baerts (NGI0) wrote:
> > > Some of these 'remove' tests rarely fail because a subflow has
> > > been
> > 
> > In my testing, only the "flush addresses" test case fails
> > intermittently. I'm wondering which other test cases have failed in
> > your environment?
> 
> I took the unstable ones from the Netdev and MPTCP CIs:
> 
> https://netdev.bots.linux.dev/contest.html?pass=0&executor=vmksft-mptcp-dbg&ld-cases=1
> 
> https://ci-results.mptcp.dev/flakes.html
> 
> > I'm thinking we shouldn't add the "backup" flag to every test in
> > remove_tests, but only to the specific ones that are actually
> > failing.
> > 
> > Similarly for patches 3 and 4, we don't need to add
> > "test_linkfail=128"
> > to every test. We should only apply it to the specific ones that
> > are
> > actually failing.
> 
> If my analysis is correct, it means the simple fixes I did (backup
> and
> longer file size) can be applied to multiple tests. Then probably
> better
> to avoid these other tests to fail for the same reasons even if it is
> very
> rare, and very hard to reproduce, no? Each false positive takes a lot
> of
> resources (mainly time) to debug.

I'll leave it to you to decide. However, I still believe it's better to
apply these fixes to the tests one by one, in case we see failures in
the future.

I'll respond with my Reviewed-by tag in the cover letter.

Thanks,
-Geliang

> 
> Cheers,
> Matt
>
Re: [PATCH mptcp-net v2 2/6] selftests: mptcp: join: rm: set backup flag
Posted by Matthieu Baerts 3 weeks, 2 days ago 
On 04/11/2025 08:29, Geliang Tang wrote:
> Hi Matt,
> 
> On Tue, 2025-11-04 at 07:57 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> Thank you for the review.
>>
>> 4 Nov 2025 07:06:34 Geliang Tang <geliang@kernel.org>:
>>
>>> Hi Matt,
>>>
>>> Thanks for this fix. It works indeed.
>>>
>>> On Sun, 2025-11-02 at 12:30 +0100, Matthieu Baerts (NGI0) wrote:
>>>> Some of these 'remove' tests rarely fail because a subflow has
>>>> been
>>>
>>> In my testing, only the "flush addresses" test case fails
>>> intermittently. I'm wondering which other test cases have failed in
>>> your environment?
>>
>> I took the unstable ones from the Netdev and MPTCP CIs:
>>
>> https://netdev.bots.linux.dev/contest.html?pass=0&executor=vmksft-mptcp-dbg&ld-cases=1
>>
>> https://ci-results.mptcp.dev/flakes.html
>>
>>> I'm thinking we shouldn't add the "backup" flag to every test in
>>> remove_tests, but only to the specific ones that are actually
>>> failing.
>>>
>>> Similarly for patches 3 and 4, we don't need to add
>>> "test_linkfail=128"
>>> to every test. We should only apply it to the specific ones that
>>> are
>>> actually failing.
>>
>> If my analysis is correct, it means the simple fixes I did (backup
>> and
>> longer file size) can be applied to multiple tests. Then probably
>> better
>> to avoid these other tests to fail for the same reasons even if it is
>> very
>> rare, and very hard to reproduce, no? Each false positive takes a lot
>> of
>> resources (mainly time) to debug.
> 
> I'll leave it to you to decide. However, I still believe it's better to
> apply these fixes to the tests one by one, in case we see failures in
> the future.

Because these modifications are not modifying what is being validated in
the test, but only avoid noises that would make the tests failing, I
think it is better to prevent issues and waiting for them to happen.

> I'll respond with my Reviewed-by tag in the cover letter.

Thanks!

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.
Re: [PATCH mptcp-net v2 2/6] selftests: mptcp: join: rm: set backup flag
Posted by Matthieu Baerts 3 weeks, 2 days ago 
On 04/11/2025 09:47, Matthieu Baerts wrote:
> On 04/11/2025 08:29, Geliang Tang wrote:
>> Hi Matt,
>>
>> On Tue, 2025-11-04 at 07:57 +0100, Matthieu Baerts wrote:
>>> Hi Geliang,
>>>
>>> Thank you for the review.
>>>
>>> 4 Nov 2025 07:06:34 Geliang Tang <geliang@kernel.org>:
>>>
>>>> Hi Matt,
>>>>
>>>> Thanks for this fix. It works indeed.
>>>>
>>>> On Sun, 2025-11-02 at 12:30 +0100, Matthieu Baerts (NGI0) wrote:
>>>>> Some of these 'remove' tests rarely fail because a subflow has
>>>>> been
>>>>
>>>> In my testing, only the "flush addresses" test case fails
>>>> intermittently. I'm wondering which other test cases have failed in
>>>> your environment?
>>>
>>> I took the unstable ones from the Netdev and MPTCP CIs:
>>>
>>> https://netdev.bots.linux.dev/contest.html?pass=0&executor=vmksft-mptcp-dbg&ld-cases=1
>>>
>>> https://ci-results.mptcp.dev/flakes.html
>>>
>>>> I'm thinking we shouldn't add the "backup" flag to every test in
>>>> remove_tests, but only to the specific ones that are actually
>>>> failing.
>>>>
>>>> Similarly for patches 3 and 4, we don't need to add
>>>> "test_linkfail=128"
>>>> to every test. We should only apply it to the specific ones that
>>>> are
>>>> actually failing.
>>>
>>> If my analysis is correct, it means the simple fixes I did (backup
>>> and
>>> longer file size) can be applied to multiple tests. Then probably
>>> better
>>> to avoid these other tests to fail for the same reasons even if it is
>>> very
>>> rare, and very hard to reproduce, no? Each false positive takes a lot
>>> of
>>> resources (mainly time) to debug.
>>
>> I'll leave it to you to decide. However, I still believe it's better to
>> apply these fixes to the tests one by one, in case we see failures in
>> the future.
> 
> Because these modifications are not modifying what is being validated in
> the test, but only avoid noises that would make the tests failing, I
> think it is better to prevent issues and waiting for them to happen.

I meant to say: "it is better to prevent issues *than* waiting for them
to happen.".

> 
>> I'll respond with my Reviewed-by tag in the cover letter.
> 
> Thanks!
> 
> Cheers,
> Matt

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.