[PATCH mptcp-next] Squash-to: "mptcp: more careful RM_ADDR generation"

[PATCH mptcp-next] Squash-to: "mptcp: more careful RM_ADDR generation"

[Paolo Abeni]

When the self-tests flushed the subflows on both sides of
the MPTCP connections, the number of subflows actually deleted
on each side is unreliable.

The test code always flushes  first the client side, but the
actual delete operations is performed asynchronously, and one
on more subflows could be processed after that the other end
already deleted it.

The grand total of the deleted subflows is expected to be
in the [nr_subflows: 2* nr_subflows] range.

Address the issue checking the total number of deleted subflows
in the simult flush cases and enforcing the above constraint

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 39 ++++++++++++++++---
 1 file changed, 33 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 8e6a5292175a..58320856a392 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1130,16 +1130,24 @@ chk_rm_nr()
 {
 	local rm_addr_nr=$1
 	local rm_subflow_nr=$2
-	local invert=${3:-""}
+	local invert
+	local simult
 	local count
 	local dump_stats
 	local addr_ns
 	local subflow_ns
 
+	shift 2
+	while [ -n "$1" ]; do
+		[ "$1" = "invert" ] && invert=true
+		[ "$1" = "simult" ] && simult=true
+		shift
+	done
+
 	if [ -z $invert ]; then
 		addr_ns=$ns1
 		subflow_ns=$ns2
-	elif [ $invert = "invert" ]; then
+	elif [ $invert = "true" ]; then
 		addr_ns=$ns2
 		subflow_ns=$ns1
 	fi
@@ -1158,6 +1166,25 @@ chk_rm_nr()
 	echo -n " - sf    "
 	count=`ip netns exec $subflow_ns nstat -as | grep MPTcpExtRmSubflow | awk '{print $2}'`
 	[ -z "$count" ] && count=0
+	if [ -n "$simult" ]; then
+		local cnt=$(ip netns exec $addr_ns nstat -as | grep MPTcpExtRmSubflow | awk '{print $2}')
+		local suffix
+
+		# in case of simult flush, the subflow removal count on each side is
+		# unreliable
+		[ -z "$cnt" ] && cnt=0
+		count=$((count + cnt))
+		[ "$count" != "$rm_subflow_nr" ] && suffix="$count in [$rm_subflow_nr:$((rm_subflow_nr*2))]"
+		if [ $count -ge "$rm_subflow_nr" ] && \
+		   [ "$count" -le "$((rm_subflow_nr *2 ))" ]; then
+			echo "[ ok ] $suffix"
+		else
+			echo "[fail] got $count RM_SUBFLOW[s] expected in range [$rm_subflow_nr:$((rm_subflow_nr*2))]"
+			ret=1
+			dump_stats=1
+		fi
+		return
+	fi
 	if [ "$count" != "$rm_subflow_nr" ]; then
 		echo "[fail] got $count RM_SUBFLOW[s] expected $rm_subflow_nr"
 		ret=1
@@ -1646,7 +1673,7 @@ remove_tests()
 	run_tests $ns1 $ns2 10.0.1.1 0 -8 -8 slow
 	chk_join_nr "flush subflows and signal" 3 3 3
 	chk_add_nr 1 1
-	chk_rm_nr 1 1 invert
+	chk_rm_nr 1 3 invert simult
 
 	# subflows flush
 	reset
@@ -1657,7 +1684,7 @@ remove_tests()
 	pm_nl_add_endpoint $ns2 10.0.4.2 flags subflow
 	run_tests $ns1 $ns2 10.0.1.1 0 -8 -8 slow
 	chk_join_nr "flush subflows" 3 3 3
-	chk_rm_nr 0 3
+	chk_rm_nr 0 3 simult
 
 	# addresses flush
 	reset
@@ -1669,7 +1696,7 @@ remove_tests()
 	run_tests $ns1 $ns2 10.0.1.1 0 -8 -8 slow
 	chk_join_nr "flush addresses" 3 3 3
 	chk_add_nr 3 3
-	chk_rm_nr 3 3 invert
+	chk_rm_nr 3 3 invert simult
 
 	# invalid addresses flush
 	reset
@@ -1953,7 +1980,7 @@ add_addr_ports_tests()
 	run_tests $ns1 $ns2 10.0.1.1 0 -8 -2 slow
 	chk_join_nr "flush subflows and signal with port" 3 3 3
 	chk_add_nr 1 1
-	chk_rm_nr 1 1 invert
+	chk_rm_nr 1 3 invert simult
 
 	# multiple addresses with port
 	reset
-- 
2.34.1

Re: [PATCH mptcp-next] Squash-to: "mptcp: more careful RM_ADDR generation"

[Matthieu Baerts]

Hi Paolo,

On 21/02/2022 17:54, Paolo Abeni wrote:
> When the self-tests flushed the subflows on both sides of
> the MPTCP connections, the number of subflows actually deleted
> on each side is unreliable.
> 
> The test code always flushes  first the client side, but the
> actual delete operations is performed asynchronously, and one
> on more subflows could be processed after that the other end
> already deleted it.
> 
> The grand total of the deleted subflows is expected to be
> in the [nr_subflows: 2* nr_subflows] range.
> 
> Address the issue checking the total number of deleted subflows
> in the simult flush cases and enforcing the above constraint

Thank you for the patch!

It seems fixing the issue 262 on my side: the modified tests have been
running for a couple of hours without issues!

Tested-by: Matthieu Baerts <matthieu.baerts@tessares.net>

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next] Squash-to: "mptcp: more careful RM_ADDR generation"

[Matthieu Baerts]

Hi Paolo,

On 21/02/2022 17:54, Paolo Abeni wrote:
> When the self-tests flushed the subflows on both sides of
> the MPTCP connections, the number of subflows actually deleted
> on each side is unreliable.
> 
> The test code always flushes  first the client side, but the
> actual delete operations is performed asynchronously, and one
> on more subflows could be processed after that the other end
> already deleted it.
> 
> The grand total of the deleted subflows is expected to be
> in the [nr_subflows: 2* nr_subflows] range.
> 
> Address the issue checking the total number of deleted subflows
> in the simult flush cases and enforcing the above constraint

Thank you for the update!

It is now in our tree, I hope that's OK for you Mat!

- 939b82e97af4: "squashed" in "mptcp: more careful RM_ADDR generation"
- Results: ffd93449fa08..489fdaf33250

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220222T105959
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next] Squash-to: "mptcp: more careful RM_ADDR generation"

[Mat Martineau]

On Tue, 22 Feb 2022, Matthieu Baerts wrote:

> Hi Paolo,
>
> On 21/02/2022 17:54, Paolo Abeni wrote:
>> When the self-tests flushed the subflows on both sides of
>> the MPTCP connections, the number of subflows actually deleted
>> on each side is unreliable.
>>
>> The test code always flushes  first the client side, but the
>> actual delete operations is performed asynchronously, and one
>> on more subflows could be processed after that the other end
>> already deleted it.
>>
>> The grand total of the deleted subflows is expected to be
>> in the [nr_subflows: 2* nr_subflows] range.
>>
>> Address the issue checking the total number of deleted subflows
>> in the simult flush cases and enforcing the above constraint
>
> Thank you for the update!
>
> It is now in our tree, I hope that's OK for you Mat!
>

Yes, thanks for applying!


--
Mat Martineau
Intel