1. Patchew
  2. MPTCP
  3. [PATCH mptcp-net v2 0/9] mptcp: fix inconsistent backup usage
  4. View patch

[PATCH mptcp-net v2 7/9] selftests: mptcp: join: check backup support in signal endp

Matthieu Baerts (NGI0) posted 9 patches 2 months ago
There is a newer version of this series
[PATCH mptcp-net v2 7/9] selftests: mptcp: join: check backup support in signal endp
Posted by Matthieu Baerts (NGI0) 2 months ago 
Before the previous commit, 'signal' endpoints with the 'backup' flag
were ignored when sending the MP_JOIN.

The MPTCP Join selftest has then been modified to validate this case:
the "single address, backup" test, is now validating the MP_JOIN with a
backup flag as it is what we expect it to do with such name. The
previous version has been kept, but renamed to "single address, switch
to backup" to avoid confusions.

The "single address with port, backup" test is also now validating the
MPJ with a backup flag, which makes more sense than checking the switch
to backup with an MP_PRIO.

The "mpc backup both sides" test is now validating that the backup flag
is also set in MP_JOIN from and to the addresses used in the initial
subflow, using the special ID 0.

The 'Fixes' tag here below is the same as the one from the previous
commit: this patch here is not fixing anything wrong in the selftests,
but it validates the previous fix for an issue introduced by this commit
ID.

Fixes: 4596a2c1b7f5 ("mptcp: allow creating non-backup subflows")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Notes:
 - v2:
   - Also validating MPJ to/from ID0 with backup
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 34 ++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 175127a9c00c..ffcf558b4610 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -2639,6 +2639,19 @@ backup_tests()
 
 	# single address, backup
 	if reset "single address, backup" &&
+	   continue_if mptcp_lib_kallsyms_has "subflow_rebuild_header$"; then
+		pm_nl_set_limits $ns1 0 1
+		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup
+		pm_nl_set_limits $ns2 1 1
+		sflags=nobackup speed=slow \
+			run_tests $ns1 $ns2 10.0.1.1
+		chk_join_nr 1 1 1
+		chk_add_nr 1 1
+		chk_prio_nr 1 0 0 1
+	fi
+
+	# single address, switch to backup
+	if reset "single address, switch to backup" &&
 	   continue_if mptcp_lib_kallsyms_has "subflow_rebuild_header$"; then
 		pm_nl_set_limits $ns1 0 1
 		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
@@ -2654,13 +2667,13 @@ backup_tests()
 	if reset "single address with port, backup" &&
 	   continue_if mptcp_lib_kallsyms_has "subflow_rebuild_header$"; then
 		pm_nl_set_limits $ns1 0 1
-		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal port 10100
+		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup port 10100
 		pm_nl_set_limits $ns2 1 1
-		sflags=backup speed=slow \
+		sflags=nobackup speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 1 1 1
 		chk_add_nr 1 1
-		chk_prio_nr 1 1 0 0
+		chk_prio_nr 1 0 0 1
 	fi
 
 	if reset "mpc backup" &&
@@ -2674,12 +2687,21 @@ backup_tests()
 
 	if reset "mpc backup both sides" &&
 	   continue_if mptcp_lib_kallsyms_doesnt_have "T mptcp_subflow_send_ack$"; then
-		pm_nl_add_endpoint $ns1 10.0.1.1 flags subflow,backup
+		pm_nl_set_limits $ns1 0 2
+		pm_nl_set_limits $ns2 1 2
+		pm_nl_add_endpoint $ns1 10.0.1.1 flags signal,backup
 		pm_nl_add_endpoint $ns2 10.0.1.2 flags subflow,backup
+
+		# 10.0.2.2 (non-backup) -> 10.0.1.1 (backup)
+		pm_nl_add_endpoint $ns2 10.0.2.2 flags subflow
+		# 10.0.1.2 (backup) -> 10.0.2.1 (non-backup)
+		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
+		ip -net "$ns2" route add 10.0.2.1 via 10.0.1.1 dev ns2eth1 # force this path
+
 		speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
-		chk_join_nr 0 0 0
-		chk_prio_nr 1 1 0 0
+		chk_join_nr 2 2 2
+		chk_prio_nr 1 1 1 1
 	fi
 
 	if reset "mpc switch to backup" &&

-- 
2.45.2
Re: [PATCH mptcp-net v2 7/9] selftests: mptcp: join: check backup support in signal endp
Posted by Geliang Tang 2 months ago 
On Tue, 2024-07-16 at 22:53 +0200, Matthieu Baerts (NGI0) wrote:
> Before the previous commit, 'signal' endpoints with the 'backup' flag
> were ignored when sending the MP_JOIN.
> 
> The MPTCP Join selftest has then been modified to validate this case:
> the "single address, backup" test, is now validating the MP_JOIN with
> a
> backup flag as it is what we expect it to do with such name. The
> previous version has been kept, but renamed to "single address,
> switch
> to backup" to avoid confusions.
> 
> The "single address with port, backup" test is also now validating
> the
> MPJ with a backup flag, which makes more sense than checking the
> switch
> to backup with an MP_PRIO.
> 
> The "mpc backup both sides" test is now validating that the backup
> flag
> is also set in MP_JOIN from and to the addresses used in the initial
> subflow, using the special ID 0.
> 
> The 'Fixes' tag here below is the same as the one from the previous
> commit: this patch here is not fixing anything wrong in the
> selftests,
> but it validates the previous fix for an issue introduced by this
> commit
> ID.
> 
> Fixes: 4596a2c1b7f5 ("mptcp: allow creating non-backup subflows")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Notes:
>  - v2:
>    - Also validating MPJ to/from ID0 with backup
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 34
> ++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 175127a9c00c..ffcf558b4610 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -2639,6 +2639,19 @@ backup_tests()
>  
>  	# single address, backup
>  	if reset "single address, backup" &&
> +	   continue_if mptcp_lib_kallsyms_has
> "subflow_rebuild_header$"; then
> +		pm_nl_set_limits $ns1 0 1
> +		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup
> +		pm_nl_set_limits $ns2 1 1
> +		sflags=nobackup speed=slow \
> +			run_tests $ns1 $ns2 10.0.1.1
> +		chk_join_nr 1 1 1
> +		chk_add_nr 1 1
> +		chk_prio_nr 1 0 0 1
> +	fi
> +
> +	# single address, switch to backup
> +	if reset "single address, switch to backup" &&
>  	   continue_if mptcp_lib_kallsyms_has
> "subflow_rebuild_header$"; then
>  		pm_nl_set_limits $ns1 0 1
>  		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
> @@ -2654,13 +2667,13 @@ backup_tests()

'''
>  	if reset "single address with port, backup" &&
>  	   continue_if mptcp_lib_kallsyms_has
> "subflow_rebuild_header$"; then
>  		pm_nl_set_limits $ns1 0 1
> -		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal port
> 10100
> +		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup
> port 10100
>  		pm_nl_set_limits $ns2 1 1
> -		sflags=backup speed=slow \
> +		sflags=nobackup speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 1 1 1
>  		chk_add_nr 1 1
> -		chk_prio_nr 1 1 0 0
> +		chk_prio_nr 1 0 0 1
>  	fi
'''

How about keep this block unchanged. "nobackup" is already tested in
the above newly added test "single address, backup"? They are almost
the same now.

WDYT?

Thanks,
-Geliang

>  
>  	if reset "mpc backup" &&
> @@ -2674,12 +2687,21 @@ backup_tests()
>  
>  	if reset "mpc backup both sides" &&
>  	   continue_if mptcp_lib_kallsyms_doesnt_have "T
> mptcp_subflow_send_ack$"; then
> -		pm_nl_add_endpoint $ns1 10.0.1.1 flags
> subflow,backup
> +		pm_nl_set_limits $ns1 0 2
> +		pm_nl_set_limits $ns2 1 2
> +		pm_nl_add_endpoint $ns1 10.0.1.1 flags signal,backup
>  		pm_nl_add_endpoint $ns2 10.0.1.2 flags
> subflow,backup
> +
> +		# 10.0.2.2 (non-backup) -> 10.0.1.1 (backup)
> +		pm_nl_add_endpoint $ns2 10.0.2.2 flags subflow
> +		# 10.0.1.2 (backup) -> 10.0.2.1 (non-backup)
> +		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
> +		ip -net "$ns2" route add 10.0.2.1 via 10.0.1.1 dev
> ns2eth1 # force this path
> +
>  		speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
> -		chk_join_nr 0 0 0
> -		chk_prio_nr 1 1 0 0
> +		chk_join_nr 2 2 2
> +		chk_prio_nr 1 1 1 1
>  	fi
>  
>  	if reset "mpc switch to backup" &&
>
Re: [PATCH mptcp-net v2 7/9] selftests: mptcp: join: check backup support in signal endp
Posted by Matthieu Baerts 2 months ago 

On 17/07/2024 06:39, Geliang Tang wrote:
> On Tue, 2024-07-16 at 22:53 +0200, Matthieu Baerts (NGI0) wrote:
>> Before the previous commit, 'signal' endpoints with the 'backup' flag
>> were ignored when sending the MP_JOIN.
>>
>> The MPTCP Join selftest has then been modified to validate this case:
>> the "single address, backup" test, is now validating the MP_JOIN with
>> a
>> backup flag as it is what we expect it to do with such name. The
>> previous version has been kept, but renamed to "single address,
>> switch
>> to backup" to avoid confusions.
>>
>> The "single address with port, backup" test is also now validating
>> the
>> MPJ with a backup flag, which makes more sense than checking the
>> switch
>> to backup with an MP_PRIO.
>>
>> The "mpc backup both sides" test is now validating that the backup
>> flag
>> is also set in MP_JOIN from and to the addresses used in the initial
>> subflow, using the special ID 0.
>>
>> The 'Fixes' tag here below is the same as the one from the previous
>> commit: this patch here is not fixing anything wrong in the
>> selftests,
>> but it validates the previous fix for an issue introduced by this
>> commit
>> ID.
>>
>> Fixes: 4596a2c1b7f5 ("mptcp: allow creating non-backup subflows")
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> Notes:
>>  - v2:
>>    - Also validating MPJ to/from ID0 with backup
>> ---
>>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 34
>> ++++++++++++++++++++-----
>>  1 file changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> index 175127a9c00c..ffcf558b4610 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> @@ -2639,6 +2639,19 @@ backup_tests()
>>  
>>  	# single address, backup
>>  	if reset "single address, backup" &&
>> +	   continue_if mptcp_lib_kallsyms_has
>> "subflow_rebuild_header$"; then
>> +		pm_nl_set_limits $ns1 0 1
>> +		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup
>> +		pm_nl_set_limits $ns2 1 1
>> +		sflags=nobackup speed=slow \
>> +			run_tests $ns1 $ns2 10.0.1.1
>> +		chk_join_nr 1 1 1
>> +		chk_add_nr 1 1
>> +		chk_prio_nr 1 0 0 1
>> +	fi
>> +
>> +	# single address, switch to backup
>> +	if reset "single address, switch to backup" &&
>>  	   continue_if mptcp_lib_kallsyms_has
>> "subflow_rebuild_header$"; then
>>  		pm_nl_set_limits $ns1 0 1
>>  		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
>> @@ -2654,13 +2667,13 @@ backup_tests()
> 
> '''
>>  	if reset "single address with port, backup" &&
>>  	   continue_if mptcp_lib_kallsyms_has
>> "subflow_rebuild_header$"; then
>>  		pm_nl_set_limits $ns1 0 1
>> -		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal port
>> 10100
>> +		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal,backup
>> port 10100
>>  		pm_nl_set_limits $ns2 1 1
>> -		sflags=backup speed=slow \
>> +		sflags=nobackup speed=slow \
>>  			run_tests $ns1 $ns2 10.0.1.1
>>  		chk_join_nr 1 1 1
>>  		chk_add_nr 1 1
>> -		chk_prio_nr 1 1 0 0
>> +		chk_prio_nr 1 0 0 1
>>  	fi
> '''
> 
> How about keep this block unchanged. "nobackup" is already tested in
> the above newly added test "single address, backup"? They are almost
> the same now.

The switch from 'nobackup' to 'backup' during the connection is also
tested just before, in "single address, switch to backup".

I think it makes more sense to validate the cases where the endpoints
are set with the backup flag before establishing connections: that
sounds like the most common use-cases. I guess people don't play with
the 'endpoint change (no)backup' option that often, no?

Supporting the switch of the backup flag for existing connections seems
like a nice to have feature to me, but not the main one related to the
backup case. So I think we should reflect that in our test suite. (And
still, we validate the switch here, but now we also check that the flag
is correctly set in the most common cases).

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.