A peer can notify the other one that a subflow has to be treated as
"backup" by two different ways: either by sending a dedicated MP_PRIO
notification, or by setting the backup flag in the MP_JOIN handshake.
The selftests were previously monitoring the former, but not the latter.
This is what is now done here by looking at these new MIB counters when
validating the 'backup' cases:
MPTcpExtMPJoinSynBackupRx
MPTcpExtMPJoinSynAckBackupRx
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 will help to validate a new 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>
---
tools/testing/selftests/net/mptcp/mptcp_join.sh | 42 +++++++++++++++++++------
1 file changed, 32 insertions(+), 10 deletions(-)
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 55d84a1bde15..175127a9c00c 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1634,6 +1634,8 @@ chk_prio_nr()
{
local mp_prio_nr_tx=$1
local mp_prio_nr_rx=$2
+ local mpj_syn=$3
+ local mpj_syn_ack=$4
local count
print_check "ptx"
@@ -1655,6 +1657,26 @@ chk_prio_nr()
else
print_ok
fi
+
+ print_check "syn backup"
+ count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinSynBackupRx")
+ if [ -z "$count" ]; then
+ print_skip
+ elif [ "$count" != "$mpj_syn" ]; then
+ fail_test "got $count MPJ SYN[s] with Backup expected $mpj_syn"
+ else
+ print_ok
+ fi
+
+ print_check "synack backup"
+ count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynAckBackupRx")
+ if [ -z "$count" ]; then
+ print_skip
+ elif [ "$count" != "$mpj_syn_ack" ]; then
+ fail_test "got $count MPJ SYNACK[s] with Backup expected $mpj_syn_ack"
+ else
+ print_ok
+ fi
}
chk_subflow_nr()
@@ -2612,7 +2634,7 @@ backup_tests()
sflags=nobackup speed=slow \
run_tests $ns1 $ns2 10.0.1.1
chk_join_nr 1 1 1
- chk_prio_nr 0 1
+ chk_prio_nr 0 1 1 0
fi
# single address, backup
@@ -2625,7 +2647,7 @@ backup_tests()
run_tests $ns1 $ns2 10.0.1.1
chk_join_nr 1 1 1
chk_add_nr 1 1
- chk_prio_nr 1 1
+ chk_prio_nr 1 1 0 0
fi
# single address with port, backup
@@ -2638,7 +2660,7 @@ backup_tests()
run_tests $ns1 $ns2 10.0.1.1
chk_join_nr 1 1 1
chk_add_nr 1 1
- chk_prio_nr 1 1
+ chk_prio_nr 1 1 0 0
fi
if reset "mpc backup" &&
@@ -2647,7 +2669,7 @@ backup_tests()
speed=slow \
run_tests $ns1 $ns2 10.0.1.1
chk_join_nr 0 0 0
- chk_prio_nr 0 1
+ chk_prio_nr 0 1 0 0
fi
if reset "mpc backup both sides" &&
@@ -2657,7 +2679,7 @@ backup_tests()
speed=slow \
run_tests $ns1 $ns2 10.0.1.1
chk_join_nr 0 0 0
- chk_prio_nr 1 1
+ chk_prio_nr 1 1 0 0
fi
if reset "mpc switch to backup" &&
@@ -2666,7 +2688,7 @@ backup_tests()
sflags=backup speed=slow \
run_tests $ns1 $ns2 10.0.1.1
chk_join_nr 0 0 0
- chk_prio_nr 0 1
+ chk_prio_nr 0 1 0 0
fi
if reset "mpc switch to backup both sides" &&
@@ -2676,7 +2698,7 @@ backup_tests()
sflags=backup speed=slow \
run_tests $ns1 $ns2 10.0.1.1
chk_join_nr 0 0 0
- chk_prio_nr 1 1
+ chk_prio_nr 1 1 0 0
fi
}
@@ -3053,7 +3075,7 @@ fullmesh_tests()
addr_nr_ns2=1 sflags=backup,fullmesh speed=slow \
run_tests $ns1 $ns2 10.0.1.1
chk_join_nr 2 2 2
- chk_prio_nr 0 1
+ chk_prio_nr 0 1 1 0
chk_rm_nr 0 1
fi
@@ -3066,7 +3088,7 @@ fullmesh_tests()
sflags=nobackup,nofullmesh speed=slow \
run_tests $ns1 $ns2 10.0.1.1
chk_join_nr 2 2 2
- chk_prio_nr 0 1
+ chk_prio_nr 0 1 1 0
chk_rm_nr 0 1
fi
}
@@ -3318,7 +3340,7 @@ userspace_tests()
sflags=backup speed=slow \
run_tests $ns1 $ns2 10.0.1.1
chk_join_nr 1 1 0
- chk_prio_nr 0 0
+ chk_prio_nr 0 0 0 0
fi
# userspace pm type prevents rm_addr
--
2.45.2Hi Matt,
Thanks for these patches.
On Tue, 2024-07-16 at 22:53 +0200, Matthieu Baerts (NGI0) wrote:
> A peer can notify the other one that a subflow has to be treated as
> "backup" by two different ways: either by sending a dedicated MP_PRIO
> notification, or by setting the backup flag in the MP_JOIN handshake.
>
> The selftests were previously monitoring the former, but not the
> latter.
> This is what is now done here by looking at these new MIB counters
> when
> validating the 'backup' cases:
>
> MPTcpExtMPJoinSynBackupRx
> MPTcpExtMPJoinSynAckBackupRx
>
> 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 will help to validate a new 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>
> ---
> tools/testing/selftests/net/mptcp/mptcp_join.sh | 42
> +++++++++++++++++++------
> 1 file changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 55d84a1bde15..175127a9c00c 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -1634,6 +1634,8 @@ chk_prio_nr()
> {
> local mp_prio_nr_tx=$1
> local mp_prio_nr_rx=$2
> + local mpj_syn=$3
> + local mpj_syn_ack=$4
How about setting default values for them:
local mpj_syn=${3:-0}
local mpj_syn_ack=${4:-0}
> local count
>
> print_check "ptx"
> @@ -1655,6 +1657,26 @@ chk_prio_nr()
> else
> print_ok
> fi
> +
> + print_check "syn backup"
> + count=$(mptcp_lib_get_counter ${ns1}
> "MPTcpExtMPJoinSynBackupRx")
> + if [ -z "$count" ]; then
> + print_skip
> + elif [ "$count" != "$mpj_syn" ]; then
> + fail_test "got $count MPJ SYN[s] with Backup
> expected $mpj_syn"
And make this log more like the one in chk_join_nr:
fail_test "got $count JOIN[s] syn with Backup expected
$mpj_syn"
> + else
> + print_ok
> + fi
> +
> + print_check "synack backup"
> + count=$(mptcp_lib_get_counter ${ns2}
> "MPTcpExtMPJoinSynAckBackupRx")
> + if [ -z "$count" ]; then
> + print_skip
> + elif [ "$count" != "$mpj_syn_ack" ]; then
> + fail_test "got $count MPJ SYNACK[s] with Backup
> expected $mpj_syn_ack"
The same here.
> + else
> + print_ok
> + fi
> }
>
> chk_subflow_nr()
> @@ -2612,7 +2634,7 @@ backup_tests()
> sflags=nobackup speed=slow \
> run_tests $ns1 $ns2 10.0.1.1
> chk_join_nr 1 1 1
> - chk_prio_nr 0 1
> + chk_prio_nr 0 1 1 0
The last value is the default one, only "chk_prio_nr 0 1 1" is enough.
> fi
>
> # single address, backup
> @@ -2625,7 +2647,7 @@ backup_tests()
> run_tests $ns1 $ns2 10.0.1.1
> chk_join_nr 1 1 1
> chk_add_nr 1 1
> - chk_prio_nr 1 1
> + chk_prio_nr 1 1 0 0
With default values, no need to change this then.
WDYT?
Thanks,
-Geliang
> fi
>
> # single address with port, backup
> @@ -2638,7 +2660,7 @@ backup_tests()
> run_tests $ns1 $ns2 10.0.1.1
> chk_join_nr 1 1 1
> chk_add_nr 1 1
> - chk_prio_nr 1 1
> + chk_prio_nr 1 1 0 0
> fi
>
> if reset "mpc backup" &&
> @@ -2647,7 +2669,7 @@ backup_tests()
> speed=slow \
> run_tests $ns1 $ns2 10.0.1.1
> chk_join_nr 0 0 0
> - chk_prio_nr 0 1
> + chk_prio_nr 0 1 0 0
> fi
>
> if reset "mpc backup both sides" &&
> @@ -2657,7 +2679,7 @@ backup_tests()
> speed=slow \
> run_tests $ns1 $ns2 10.0.1.1
> chk_join_nr 0 0 0
> - chk_prio_nr 1 1
> + chk_prio_nr 1 1 0 0
> fi
>
> if reset "mpc switch to backup" &&
> @@ -2666,7 +2688,7 @@ backup_tests()
> sflags=backup speed=slow \
> run_tests $ns1 $ns2 10.0.1.1
> chk_join_nr 0 0 0
> - chk_prio_nr 0 1
> + chk_prio_nr 0 1 0 0
> fi
>
> if reset "mpc switch to backup both sides" &&
> @@ -2676,7 +2698,7 @@ backup_tests()
> sflags=backup speed=slow \
> run_tests $ns1 $ns2 10.0.1.1
> chk_join_nr 0 0 0
> - chk_prio_nr 1 1
> + chk_prio_nr 1 1 0 0
> fi
> }
>
> @@ -3053,7 +3075,7 @@ fullmesh_tests()
> addr_nr_ns2=1 sflags=backup,fullmesh speed=slow \
> run_tests $ns1 $ns2 10.0.1.1
> chk_join_nr 2 2 2
> - chk_prio_nr 0 1
> + chk_prio_nr 0 1 1 0
> chk_rm_nr 0 1
> fi
>
> @@ -3066,7 +3088,7 @@ fullmesh_tests()
> sflags=nobackup,nofullmesh speed=slow \
> run_tests $ns1 $ns2 10.0.1.1
> chk_join_nr 2 2 2
> - chk_prio_nr 0 1
> + chk_prio_nr 0 1 1 0
> chk_rm_nr 0 1
> fi
> }
> @@ -3318,7 +3340,7 @@ userspace_tests()
> sflags=backup speed=slow \
> run_tests $ns1 $ns2 10.0.1.1
> chk_join_nr 1 1 0
> - chk_prio_nr 0 0
> + chk_prio_nr 0 0 0 0
> fi
>
> # userspace pm type prevents rm_addr
>
Hi Geliang,
On 17/07/2024 03:25, Geliang Tang wrote:
> Hi Matt,
>
> Thanks for these patches.
>
> On Tue, 2024-07-16 at 22:53 +0200, Matthieu Baerts (NGI0) wrote:
>> A peer can notify the other one that a subflow has to be treated as
>> "backup" by two different ways: either by sending a dedicated MP_PRIO
>> notification, or by setting the backup flag in the MP_JOIN handshake.
>>
>> The selftests were previously monitoring the former, but not the
>> latter.
>> This is what is now done here by looking at these new MIB counters
>> when
>> validating the 'backup' cases:
>>
>> MPTcpExtMPJoinSynBackupRx
>> MPTcpExtMPJoinSynAckBackupRx
>>
>> 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 will help to validate a new 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>
>> ---
>> tools/testing/selftests/net/mptcp/mptcp_join.sh | 42
>> +++++++++++++++++++------
>> 1 file changed, 32 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> index 55d84a1bde15..175127a9c00c 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> @@ -1634,6 +1634,8 @@ chk_prio_nr()
>> {
>> local mp_prio_nr_tx=$1
>> local mp_prio_nr_rx=$2
>> + local mpj_syn=$3
>> + local mpj_syn_ack=$4
>
> How about setting default values for them:
>
> local mpj_syn=${3:-0}
> local mpj_syn_ack=${4:-0}
I prefer not to: in our existing test suite, these counters can be set,
it is then important to review all of these test cases.
I also think it is important to check them in all our backup cases.
That's why I don't want to not check them if they are not set.
>
>> local count
>>
>> print_check "ptx"
>> @@ -1655,6 +1657,26 @@ chk_prio_nr()
>> else
>> print_ok
>> fi
>> +
>> + print_check "syn backup"
>> + count=$(mptcp_lib_get_counter ${ns1}
>> "MPTcpExtMPJoinSynBackupRx")
>> + if [ -z "$count" ]; then
>> + print_skip
>> + elif [ "$count" != "$mpj_syn" ]; then
>> + fail_test "got $count MPJ SYN[s] with Backup
>> expected $mpj_syn"
>
> And make this log more like the one in chk_join_nr:
>
> fail_test "got $count JOIN[s] syn with Backup expected
> $mpj_syn"
Good point!
>> + else
>> + print_ok
>> + fi
>> +
>> + print_check "synack backup"
>> + count=$(mptcp_lib_get_counter ${ns2}
>> "MPTcpExtMPJoinSynAckBackupRx")
>> + if [ -z "$count" ]; then
>> + print_skip
>> + elif [ "$count" != "$mpj_syn_ack" ]; then
>> + fail_test "got $count MPJ SYNACK[s] with Backup
>> expected $mpj_syn_ack"
>
> The same here.
Will do!
>> + else
>> + print_ok
>> + fi
>> }
>>
>> chk_subflow_nr()
>> @@ -2612,7 +2634,7 @@ backup_tests()
>> sflags=nobackup speed=slow \
>> run_tests $ns1 $ns2 10.0.1.1
>> chk_join_nr 1 1 1
>> - chk_prio_nr 0 1
>> + chk_prio_nr 0 1 1 0
>
> The last value is the default one, only "chk_prio_nr 0 1 1" is enough.
I think it is also better to avoid that in the 'chk_*' helpers,
otherwise it is hard to follow what is going to be done.
>
>> fi
>>
>> # single address, backup
>> @@ -2625,7 +2647,7 @@ backup_tests()
>> run_tests $ns1 $ns2 10.0.1.1
>> chk_join_nr 1 1 1
>> chk_add_nr 1 1
>> - chk_prio_nr 1 1
>> + chk_prio_nr 1 1 0 0
>
> With default values, no need to change this then.
Please note that I also find this change safer for the backports: if the
commit cannot be backported without conflicts, we can check why: maybe
the test behaviour was different before, and the number of MPJ with
backup flags was different. (But this is only because here it is a
series for -net. If we extend a check to verify more things in very
specific cases, something that doesn't make sense to check in other
cases, then it can be fine to have optional parameters I think.)
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
© 2016 - 2024 Red Hat, Inc.