1. Patchew
  2. MPTCP
  3. [PATCH mptcp-next v2 0/2] mptcp: MIB counters for sent MP_JOIN
  4. View patch

[PATCH mptcp-next v2 2/2] selftests: mptcp: join: validate MPJ SYN TX MIB counters

Matthieu Baerts (NGI0) posted 2 patches 3 months ago
There is a newer version of this series
[PATCH mptcp-next v2 2/2] selftests: mptcp: join: validate MPJ SYN TX MIB counters
Posted by Matthieu Baerts (NGI0) 3 months ago 
A few new MIB counters have been added. They are being validated here.

Most of the time, there are no errors, but sometimes, more MPJ SYN are
queued compared to the numbers that are received.

Only one test has an error, the one to connect to a broadcast IP address.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 63 +++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index fbb0174145ad..c1f1ebd2340c 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1372,6 +1372,66 @@ chk_join_nr()
 	fi
 }
 
+chk_join_tx_nr()
+{
+	local syn_nr=$1
+	local festab=$2
+	local create=$3
+	local bind=$4
+	local connect=$5
+	local count
+
+	print_check "syn TX"
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynTx")
+	if [ -z "$count" ]; then
+		print_skip
+	elif [ "$count" != "$syn_nr" ]; then
+		fail_test "got $count JOIN[s] syn TX expected $syn_nr"
+	else
+		print_ok
+	fi
+
+	print_check "syn TX Fully Estab Error"
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynTxFEstabErr")
+	if [ -z "$count" ]; then
+		print_skip
+	elif [ "$count" != "$festab" ]; then
+		fail_test "got $count JOIN[s] syn TX Fully Estab Error expected $festab"
+	else
+		print_ok
+	fi
+
+	print_check "syn TX Create Socket Error"
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynTxCreatSkErr")
+	if [ -z "$count" ]; then
+		print_skip
+	elif [ "$count" != "$create" ]; then
+		fail_test "got $count JOIN[s] syn TX Create Socket Error expected $create"
+	else
+		print_ok
+	fi
+
+	print_check "syn TX Bind Error"
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynTxBindErr")
+	if [ -z "$count" ]; then
+		print_skip
+	elif [ "$count" != "$bind" ]; then
+		fail_test "got $count JOIN[s] syn TX Bind Error expected $bind"
+	else
+		print_ok
+	fi
+
+	print_check "syn TX Connect Error"
+	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynTxConnectErr")
+	if [ -z "$count" ]; then
+		print_skip
+	elif [ "$count" != "$connect" ]; then
+		fail_test "got $count JOIN[s] syn TX Connect Error expected $connect"
+	else
+		print_ok
+	fi
+}
+
 # a negative value for 'stale_max' means no upper bound:
 # for bidirectional transfer, if one peer sleep for a while
 # - as these tests do - we can have a quite high number of
@@ -1907,6 +1967,7 @@ subflows_error_tests()
 		speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 0 0 0
+		chk_join_tx_nr 0 0 0 0 0
 	fi
 
 	# multiple subflows, with subflow creation error
@@ -1919,6 +1980,7 @@ subflows_error_tests()
 		speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 1 1 1
+		chk_join_tx_nr 2 0 0 0 0
 	fi
 
 	# multiple subflows, with subflow timeout on MPJ
@@ -2306,6 +2368,7 @@ remove_tests()
 		addr_nr_ns1=-3 speed=10 \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 1 1 1
+		chk_join_tx_nr 2 0 0 0 1
 		chk_add_nr 3 3
 		chk_rm_nr 3 1 invert
 		chk_rst_nr 0 0

-- 
2.45.2
Re: [PATCH mptcp-next v2 2/2] selftests: mptcp: join: validate MPJ SYN TX MIB counters
Posted by Geliang Tang 3 months ago 
Hi Matt,

Thanks for these patches.

On Mon, 2024-07-29 at 12:18 +0200, Matthieu Baerts (NGI0) wrote:
> A few new MIB counters have been added. They are being validated
> here.
> 
> Most of the time, there are no errors, but sometimes, more MPJ SYN
> are
> queued compared to the numbers that are received.
> 
> Only one test has an error, the one to connect to a broadcast IP
> address.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 63
> +++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index fbb0174145ad..c1f1ebd2340c 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -1372,6 +1372,66 @@ chk_join_nr()
>  	fi
>  }
>  
> +chk_join_tx_nr()
> +{
> +	local syn_nr=$1
> +	local festab=$2
> +	local create=$3
> +	local bind=$4
> +	local connect=$5
> +	local count
> +
> +	print_check "syn TX"
> +	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynTx")
> +	if [ -z "$count" ]; then
> +		print_skip
> +	elif [ "$count" != "$syn_nr" ]; then
> +		fail_test "got $count JOIN[s] syn TX expected
> $syn_nr"
> +	else
> +		print_ok
> +	fi

Do you think it is necessary to check MPTcpExtMPJoinSynTx in
chk_join_nr()?

> +
> +	print_check "syn TX Fully Estab Error"
> +	count=$(mptcp_lib_get_counter ${ns2}
> "MPTcpExtMPJoinSynTxFEstabErr")
> +	if [ -z "$count" ]; then
> +		print_skip
> +	elif [ "$count" != "$festab" ]; then
> +		fail_test "got $count JOIN[s] syn TX Fully Estab
> Error expected $festab"
> +	else
> +		print_ok
> +	fi
> +
> +	print_check "syn TX Create Socket Error"
> +	count=$(mptcp_lib_get_counter ${ns2}
> "MPTcpExtMPJoinSynTxCreatSkErr")
> +	if [ -z "$count" ]; then
> +		print_skip
> +	elif [ "$count" != "$create" ]; then
> +		fail_test "got $count JOIN[s] syn TX Create Socket
> Error expected $create"
> +	else
> +		print_ok
> +	fi

There is no test that can trigger MPTcpExtMPJoinSynTxFEstabErr and
MPTcpExtMPJoinSynTxCreatSkErr. Does this mean that there is no need to
add these two MIBs? Maybe adding pr_debug logs is enough? WDYT?
 
> +
> +	print_check "syn TX Bind Error"
> +	count=$(mptcp_lib_get_counter ${ns2}
> "MPTcpExtMPJoinSynTxBindErr")
> +	if [ -z "$count" ]; then
> +		print_skip
> +	elif [ "$count" != "$bind" ]; then
> +		fail_test "got $count JOIN[s] syn TX Bind Error
> expected $bind"
> +	else
> +		print_ok
> +	fi
> +
> +	print_check "syn TX Connect Error"
> +	count=$(mptcp_lib_get_counter ${ns2}
> "MPTcpExtMPJoinSynTxConnectErr")
> +	if [ -z "$count" ]; then
> +		print_skip
> +	elif [ "$count" != "$connect" ]; then
> +		fail_test "got $count JOIN[s] syn TX Connect Error
> expected $connect"
> +	else
> +		print_ok
> +	fi
> +}
> +
>  # a negative value for 'stale_max' means no upper bound:
>  # for bidirectional transfer, if one peer sleep for a while
>  # - as these tests do - we can have a quite high number of
> @@ -1907,6 +1967,7 @@ subflows_error_tests()

Add an invalid address here in this test:

	pm_nl_add_endpoint $ns2 10.0.5.2 flags subflow

>  		speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 0 0 0
> +		chk_join_tx_nr 0 0 0 0 0

It will trigger a "syn TX Bind Error":

		chk_join_tx_nr 0 0 0 1 0


Thanks!
Geliang 
>  	# multiple subflows, with subflow creation error
> @@ -1919,6 +1980,7 @@ subflows_error_tests()
>  		speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 1 1 1
> +		chk_join_tx_nr 2 0 0 0 0
>  	fi
>  
>  	# multiple subflows, with subflow timeout on MPJ
> @@ -2306,6 +2368,7 @@ remove_tests()
>  		addr_nr_ns1=-3 speed=10 \
>  			run_tests $ns1 $ns2 10.0.1.1
>  		chk_join_nr 1 1 1
> +		chk_join_tx_nr 2 0 0 0 1
>  		chk_add_nr 3 3
>  		chk_rm_nr 3 1 invert
>  		chk_rst_nr 0 0
>
Re: [PATCH mptcp-next v2 2/2] selftests: mptcp: join: validate MPJ SYN TX MIB counters
Posted by Matthieu Baerts 3 months ago 
Hi Geliang,

Thank you for the review!

On 31/07/2024 04:59, Geliang Tang wrote:
> Hi Matt,
> 
> Thanks for these patches.
> 
> On Mon, 2024-07-29 at 12:18 +0200, Matthieu Baerts (NGI0) wrote:
>> A few new MIB counters have been added. They are being validated
>> here.
>>
>> Most of the time, there are no errors, but sometimes, more MPJ SYN
>> are
>> queued compared to the numbers that are received.
>>
>> Only one test has an error, the one to connect to a broadcast IP
>> address.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 63
>> +++++++++++++++++++++++++
>>  1 file changed, 63 insertions(+)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> index fbb0174145ad..c1f1ebd2340c 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> @@ -1372,6 +1372,66 @@ chk_join_nr()
>>  	fi
>>  }
>>  
>> +chk_join_tx_nr()
>> +{
>> +	local syn_nr=$1
>> +	local festab=$2
>> +	local create=$3
>> +	local bind=$4
>> +	local connect=$5
>> +	local count
>> +
>> +	print_check "syn TX"
>> +	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinSynTx")
>> +	if [ -z "$count" ]; then
>> +		print_skip
>> +	elif [ "$count" != "$syn_nr" ]; then
>> +		fail_test "got $count JOIN[s] syn TX expected
>> $syn_nr"
>> +	else
>> +		print_ok
>> +	fi
> 
> Do you think it is necessary to check MPTcpExtMPJoinSynTx in
> chk_join_nr()?

I thought about that, then I realised some tests were sending more
MP_JOIN than the received ones (e.g. when using "invalid addresses"). We
would then need to change the default counters, but chk_join_nr() can
already optionally check the checksum, making it difficult to read when
we have to pass 14 parameters:

  chk_join_nr 1 1 1 0 0 0 0 0 0 2 0 0 0 1

It looks simpler with a dedicated helper, and checking this only in some
places. WDYT?

>> +
>> +	print_check "syn TX Fully Estab Error"
>> +	count=$(mptcp_lib_get_counter ${ns2}
>> "MPTcpExtMPJoinSynTxFEstabErr")
>> +	if [ -z "$count" ]; then
>> +		print_skip
>> +	elif [ "$count" != "$festab" ]; then
>> +		fail_test "got $count JOIN[s] syn TX Fully Estab
>> Error expected $festab"
>> +	else
>> +		print_ok
>> +	fi
>> +
>> +	print_check "syn TX Create Socket Error"
>> +	count=$(mptcp_lib_get_counter ${ns2}
>> "MPTcpExtMPJoinSynTxCreatSkErr")
>> +	if [ -z "$count" ]; then
>> +		print_skip
>> +	elif [ "$count" != "$create" ]; then
>> +		fail_test "got $count JOIN[s] syn TX Create Socket
>> Error expected $create"
>> +	else
>> +		print_ok
>> +	fi
> 
> There is no test that can trigger MPTcpExtMPJoinSynTxFEstabErr and
> MPTcpExtMPJoinSynTxCreatSkErr. Does this mean that there is no need to
> add these two MIBs? Maybe adding pr_debug logs is enough? WDYT?

MPTcpExtMPJoinSynTxFEstabErr can be easily triggered by the userspace
PM. Maybe we don't need the counter for this case then? We could also do
the check in the userspace PM side, and return a netlink message
instead, but I don't think it is needed. Not sure what's best here. I
can remove it or leave it.

For MPTcpExtMPJoinSynTxCreatSkErr, it can be triggered if it cannot
allocate memory, if the socket limit has been reached out, if a security
module blocked it, etc. I think we should keep it, and we don't need to
validate it in the tests.

>> +
>> +	print_check "syn TX Bind Error"
>> +	count=$(mptcp_lib_get_counter ${ns2}
>> "MPTcpExtMPJoinSynTxBindErr")
>> +	if [ -z "$count" ]; then
>> +		print_skip
>> +	elif [ "$count" != "$bind" ]; then
>> +		fail_test "got $count JOIN[s] syn TX Bind Error
>> expected $bind"
>> +	else
>> +		print_ok
>> +	fi
>> +
>> +	print_check "syn TX Connect Error"
>> +	count=$(mptcp_lib_get_counter ${ns2}
>> "MPTcpExtMPJoinSynTxConnectErr")
>> +	if [ -z "$count" ]; then
>> +		print_skip
>> +	elif [ "$count" != "$connect" ]; then
>> +		fail_test "got $count JOIN[s] syn TX Connect Error
>> expected $connect"
>> +	else
>> +		print_ok
>> +	fi
>> +}
>> +
>>  # a negative value for 'stale_max' means no upper bound:
>>  # for bidirectional transfer, if one peer sleep for a while
>>  # - as these tests do - we can have a quite high number of
>> @@ -1907,6 +1967,7 @@ subflows_error_tests()
> 
> Add an invalid address here in this test:
> 
> 	pm_nl_add_endpoint $ns2 10.0.5.2 flags subflow
> 
>>  		speed=slow \
>>  			run_tests $ns1 $ns2 10.0.1.1
>>  		chk_join_nr 0 0 0
>> +		chk_join_tx_nr 0 0 0 0 0
> 
> It will trigger a "syn TX Bind Error":
> 
> 		chk_join_tx_nr 0 0 0 1 0

Good idea! I will do that in the v3.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.
Re: [PATCH mptcp-next v2 2/2] selftests: mptcp: join: validate MPJ SYN TX MIB counters
Posted by Geliang Tang 3 months ago 
Hi Matt,

On Wed, 2024-07-31 at 17:02 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for the review!
> 
> On 31/07/2024 04:59, Geliang Tang wrote:
> > Hi Matt,
> > 
> > Thanks for these patches.
> > 
> > On Mon, 2024-07-29 at 12:18 +0200, Matthieu Baerts (NGI0) wrote:
> > > A few new MIB counters have been added. They are being validated
> > > here.
> > > 
> > > Most of the time, there are no errors, but sometimes, more MPJ
> > > SYN
> > > are
> > > queued compared to the numbers that are received.
> > > 
> > > Only one test has an error, the one to connect to a broadcast IP
> > > address.
> > > 
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > >  tools/testing/selftests/net/mptcp/mptcp_join.sh | 63
> > > +++++++++++++++++++++++++
> > >  1 file changed, 63 insertions(+)
> > > 
> > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > index fbb0174145ad..c1f1ebd2340c 100755
> > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > @@ -1372,6 +1372,66 @@ chk_join_nr()
> > >  	fi
> > >  }
> > >  
> > > +chk_join_tx_nr()
> > > +{
> > > +	local syn_nr=$1
> > > +	local festab=$2
> > > +	local create=$3
> > > +	local bind=$4
> > > +	local connect=$5
> > > +	local count
> > > +
> > > +	print_check "syn TX"
> > > +	count=$(mptcp_lib_get_counter ${ns2}
> > > "MPTcpExtMPJoinSynTx")
> > > +	if [ -z "$count" ]; then
> > > +		print_skip
> > > +	elif [ "$count" != "$syn_nr" ]; then
> > > +		fail_test "got $count JOIN[s] syn TX expected
> > > $syn_nr"
> > > +	else
> > > +		print_ok
> > > +	fi
> > 
> > Do you think it is necessary to check MPTcpExtMPJoinSynTx in
> > chk_join_nr()?
> 
> I thought about that, then I realised some tests were sending more
> MP_JOIN than the received ones (e.g. when using "invalid addresses").
> We
> would then need to change the default counters, but chk_join_nr() can
> already optionally check the checksum, making it difficult to read
> when
> we have to pass 14 parameters:
> 
>   chk_join_nr 1 1 1 0 0 0 0 0 0 2 0 0 0 1
> 
> It looks simpler with a dedicated helper, and checking this only in
> some
> places. WDYT?

We can handle this like "addr_nr_ns1". In chk_join_nr(), the default
value of this "syn_tx_nr" is the same as "syn_nr". If we need a
specific value, we can pass it to run_tests() through a "syn_tx_nr"
variable:

	syn_tx_nr=1 \
		run_tests $ns1 $ns2 10.0.1.1

> 
> > > +
> > > +	print_check "syn TX Fully Estab Error"
> > > +	count=$(mptcp_lib_get_counter ${ns2}
> > > "MPTcpExtMPJoinSynTxFEstabErr")
> > > +	if [ -z "$count" ]; then
> > > +		print_skip
> > > +	elif [ "$count" != "$festab" ]; then
> > > +		fail_test "got $count JOIN[s] syn TX Fully Estab
> > > Error expected $festab"
> > > +	else
> > > +		print_ok
> > > +	fi
> > > +
> > > +	print_check "syn TX Create Socket Error"
> > > +	count=$(mptcp_lib_get_counter ${ns2}
> > > "MPTcpExtMPJoinSynTxCreatSkErr")
> > > +	if [ -z "$count" ]; then
> > > +		print_skip
> > > +	elif [ "$count" != "$create" ]; then
> > > +		fail_test "got $count JOIN[s] syn TX Create
> > > Socket
> > > Error expected $create"
> > > +	else
> > > +		print_ok
> > > +	fi
> > 
> > There is no test that can trigger MPTcpExtMPJoinSynTxFEstabErr and
> > MPTcpExtMPJoinSynTxCreatSkErr. Does this mean that there is no need
> > to
> > add these two MIBs? Maybe adding pr_debug logs is enough? WDYT?
> 
> MPTcpExtMPJoinSynTxFEstabErr can be easily triggered by the userspace
> PM. Maybe we don't need the counter for this case then? We could also
> do
> the check in the userspace PM side, and return a netlink message
> instead, but I don't think it is needed. Not sure what's best here. I
> can remove it or leave it.

If so I think it's better to remove it, since too many MIBs are added
in this function.

> 
> For MPTcpExtMPJoinSynTxCreatSkErr, it can be triggered if it cannot
> allocate memory, if the socket limit has been reached out, if a
> security
> module blocked it, etc. I think we should keep it, and we don't need
> to
> validate it in the tests.

These errors are not related to MPTCP protocol. I think we can remove
this MIB but keep its pr_debug log. WDYT?

Regards,
-Geliang

> 
> > > +
> > > +	print_check "syn TX Bind Error"
> > > +	count=$(mptcp_lib_get_counter ${ns2}
> > > "MPTcpExtMPJoinSynTxBindErr")
> > > +	if [ -z "$count" ]; then
> > > +		print_skip
> > > +	elif [ "$count" != "$bind" ]; then
> > > +		fail_test "got $count JOIN[s] syn TX Bind Error
> > > expected $bind"
> > > +	else
> > > +		print_ok
> > > +	fi
> > > +
> > > +	print_check "syn TX Connect Error"
> > > +	count=$(mptcp_lib_get_counter ${ns2}
> > > "MPTcpExtMPJoinSynTxConnectErr")
> > > +	if [ -z "$count" ]; then
> > > +		print_skip
> > > +	elif [ "$count" != "$connect" ]; then
> > > +		fail_test "got $count JOIN[s] syn TX Connect
> > > Error
> > > expected $connect"
> > > +	else
> > > +		print_ok
> > > +	fi
> > > +}
> > > +
> > >  # a negative value for 'stale_max' means no upper bound:
> > >  # for bidirectional transfer, if one peer sleep for a while
> > >  # - as these tests do - we can have a quite high number of
> > > @@ -1907,6 +1967,7 @@ subflows_error_tests()
> > 
> > Add an invalid address here in this test:
> > 
> > 	pm_nl_add_endpoint $ns2 10.0.5.2 flags subflow
> > 
> > >  		speed=slow \
> > >  			run_tests $ns1 $ns2 10.0.1.1
> > >  		chk_join_nr 0 0 0
> > > +		chk_join_tx_nr 0 0 0 0 0
> > 
> > It will trigger a "syn TX Bind Error":
> > 
> > 		chk_join_tx_nr 0 0 0 1 0
> 
> Good idea! I will do that in the v3.
> 
> Cheers,
> Matt
Re: [PATCH mptcp-next v2 2/2] selftests: mptcp: join: validate MPJ SYN TX MIB counters
Posted by Matthieu Baerts 2 months, 3 weeks ago 
Hi Geliang,

On 01/08/2024 03:36, Geliang Tang wrote:
> Hi Matt,
> 
> On Wed, 2024-07-31 at 17:02 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> Thank you for the review!
>>
>> On 31/07/2024 04:59, Geliang Tang wrote:
>>> Hi Matt,
>>>
>>> Thanks for these patches.
>>>
>>> On Mon, 2024-07-29 at 12:18 +0200, Matthieu Baerts (NGI0) wrote:
>>>> A few new MIB counters have been added. They are being validated
>>>> here.
>>>>
>>>> Most of the time, there are no errors, but sometimes, more MPJ
>>>> SYN
>>>> are
>>>> queued compared to the numbers that are received.
>>>>
>>>> Only one test has an error, the one to connect to a broadcast IP
>>>> address.
>>>>
>>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>>> ---
>>>>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 63
>>>> +++++++++++++++++++++++++
>>>>  1 file changed, 63 insertions(+)
>>>>
>>>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> index fbb0174145ad..c1f1ebd2340c 100755
>>>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> @@ -1372,6 +1372,66 @@ chk_join_nr()
>>>>  	fi
>>>>  }
>>>>  
>>>> +chk_join_tx_nr()
>>>> +{
>>>> +	local syn_nr=$1
>>>> +	local festab=$2
>>>> +	local create=$3
>>>> +	local bind=$4
>>>> +	local connect=$5
>>>> +	local count
>>>> +
>>>> +	print_check "syn TX"
>>>> +	count=$(mptcp_lib_get_counter ${ns2}
>>>> "MPTcpExtMPJoinSynTx")
>>>> +	if [ -z "$count" ]; then
>>>> +		print_skip
>>>> +	elif [ "$count" != "$syn_nr" ]; then
>>>> +		fail_test "got $count JOIN[s] syn TX expected
>>>> $syn_nr"
>>>> +	else
>>>> +		print_ok
>>>> +	fi
>>>
>>> Do you think it is necessary to check MPTcpExtMPJoinSynTx in
>>> chk_join_nr()?
>>
>> I thought about that, then I realised some tests were sending more
>> MP_JOIN than the received ones (e.g. when using "invalid addresses").
>> We
>> would then need to change the default counters, but chk_join_nr() can
>> already optionally check the checksum, making it difficult to read
>> when
>> we have to pass 14 parameters:
>>
>>   chk_join_nr 1 1 1 0 0 0 0 0 0 2 0 0 0 1
>>
>> It looks simpler with a dedicated helper, and checking this only in
>> some
>> places. WDYT?
> 
> We can handle this like "addr_nr_ns1". In chk_join_nr(), the default
> value of this "syn_tx_nr" is the same as "syn_nr". If we need a
> specific value, we can pass it to run_tests() through a "syn_tx_nr"
> variable:
> 
> 	syn_tx_nr=1 \
> 		run_tests $ns1 $ns2 10.0.1.1

Yes, I can. But if we do that, we might want to reduce the number of
checks being displayed, not to display 5 more lines per test.

Maybe we can have two "Join [RT]x: OK" in case of success, and a detail
of the failures if not? I can look at that.

>>>> +
>>>> +	print_check "syn TX Fully Estab Error"
>>>> +	count=$(mptcp_lib_get_counter ${ns2}
>>>> "MPTcpExtMPJoinSynTxFEstabErr")
>>>> +	if [ -z "$count" ]; then
>>>> +		print_skip
>>>> +	elif [ "$count" != "$festab" ]; then
>>>> +		fail_test "got $count JOIN[s] syn TX Fully Estab
>>>> Error expected $festab"
>>>> +	else
>>>> +		print_ok
>>>> +	fi
>>>> +
>>>> +	print_check "syn TX Create Socket Error"
>>>> +	count=$(mptcp_lib_get_counter ${ns2}
>>>> "MPTcpExtMPJoinSynTxCreatSkErr")
>>>> +	if [ -z "$count" ]; then
>>>> +		print_skip
>>>> +	elif [ "$count" != "$create" ]; then
>>>> +		fail_test "got $count JOIN[s] syn TX Create
>>>> Socket
>>>> Error expected $create"
>>>> +	else
>>>> +		print_ok
>>>> +	fi
>>>
>>> There is no test that can trigger MPTcpExtMPJoinSynTxFEstabErr and
>>> MPTcpExtMPJoinSynTxCreatSkErr. Does this mean that there is no need
>>> to
>>> add these two MIBs? Maybe adding pr_debug logs is enough? WDYT?
>>
>> MPTcpExtMPJoinSynTxFEstabErr can be easily triggered by the userspace
>> PM. Maybe we don't need the counter for this case then? We could also
>> do
>> the check in the userspace PM side, and return a netlink message
>> instead, but I don't think it is needed. Not sure what's best here. I
>> can remove it or leave it.
> 
> If so I think it's better to remove it, since too many MIBs are added
> in this function.

For this one, it looks like only the userspace PM can cause it, and the
userspace daemon will get feedback in this case. So OK for me to remove it.

>> For MPTcpExtMPJoinSynTxCreatSkErr, it can be triggered if it cannot
>> allocate memory, if the socket limit has been reached out, if a
>> security
>> module blocked it, etc. I think we should keep it, and we don't need
>> to
>> validate it in the tests.
> 
> These errors are not related to MPTCP protocol. I think we can remove
> this MIB but keep its pr_debug log. WDYT?

I think we should keep it, otherwise it will be difficult to debug this
issue if we don't cover all cases. The other ones should mainly be due
to a wrong config from the user, but theyu could also fail due to
security restrictions. The pr_debug()'s don't work for everybody, they
require CONFIG_DEBUG=y or CONFIG_DYNAMIC_DEBUG=y, or a specific build
per file, etc.

In other words, it sounds safer to me to keep this one to cover all
cases. Otherwise, we might struggle during to help users, and ending up
asking the users to compile a modified kernel.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.
Re: [PATCH mptcp-next v2 2/2] selftests: mptcp: join: validate MPJ SYN TX MIB counters
Posted by Geliang Tang 3 months ago 
Hi Matt,

On Wed, 2024-07-31 at 17:02 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for the review!
> 
> On 31/07/2024 04:59, Geliang Tang wrote:
> > Hi Matt,
> > 
> > Thanks for these patches.
> > 
> > On Mon, 2024-07-29 at 12:18 +0200, Matthieu Baerts (NGI0) wrote:
> > > A few new MIB counters have been added. They are being validated
> > > here.
> > > 
> > > Most of the time, there are no errors, but sometimes, more MPJ
> > > SYN
> > > are
> > > queued compared to the numbers that are received.
> > > 
> > > Only one test has an error, the one to connect to a broadcast IP
> > > address.
> > > 
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > >  tools/testing/selftests/net/mptcp/mptcp_join.sh | 63
> > > +++++++++++++++++++++++++
> > >  1 file changed, 63 insertions(+)
> > > 
> > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > index fbb0174145ad..c1f1ebd2340c 100755
> > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > @@ -1372,6 +1372,66 @@ chk_join_nr()
> > >  	fi
> > >  }
> > >  
> > > +chk_join_tx_nr()
> > > +{
> > > +	local syn_nr=$1
> > > +	local festab=$2
> > > +	local create=$3
> > > +	local bind=$4
> > > +	local connect=$5
> > > +	local count
> > > +
> > > +	print_check "syn TX"
> > > +	count=$(mptcp_lib_get_counter ${ns2}
> > > "MPTcpExtMPJoinSynTx")
> > > +	if [ -z "$count" ]; then
> > > +		print_skip
> > > +	elif [ "$count" != "$syn_nr" ]; then
> > > +		fail_test "got $count JOIN[s] syn TX expected
> > > $syn_nr"
> > > +	else
> > > +		print_ok
> > > +	fi
> > 
> > Do you think it is necessary to check MPTcpExtMPJoinSynTx in
> > chk_join_nr()?
> 
> I thought about that, then I realised some tests were sending more
> MP_JOIN than the received ones (e.g. when using "invalid addresses").
> We
> would then need to change the default counters, but chk_join_nr() can
> already optionally check the checksum, making it difficult to read
> when
> we have to pass 14 parameters:
> 
>   chk_join_nr 1 1 1 0 0 0 0 0 0 2 0 0 0 1
> 
> It looks simpler with a dedicated helper, and checking this only in
> some
> places. WDYT?

We can handle this like "addr_nr_ns1". In chk_join_nr(), the default
value of this "syn_tx_nr" is the same as "syn_nr". If we need a
specific value, we can pass it to run_tests() through a "syn_tx_nr"
variable:

	syn_tx_nr=1 \
		run_tests $ns1 $ns2 10.0.1.1

> 
> > > +
> > > +	print_check "syn TX Fully Estab Error"
> > > +	count=$(mptcp_lib_get_counter ${ns2}
> > > "MPTcpExtMPJoinSynTxFEstabErr")
> > > +	if [ -z "$count" ]; then
> > > +		print_skip
> > > +	elif [ "$count" != "$festab" ]; then
> > > +		fail_test "got $count JOIN[s] syn TX Fully Estab
> > > Error expected $festab"
> > > +	else
> > > +		print_ok
> > > +	fi
> > > +
> > > +	print_check "syn TX Create Socket Error"
> > > +	count=$(mptcp_lib_get_counter ${ns2}
> > > "MPTcpExtMPJoinSynTxCreatSkErr")
> > > +	if [ -z "$count" ]; then
> > > +		print_skip
> > > +	elif [ "$count" != "$create" ]; then
> > > +		fail_test "got $count JOIN[s] syn TX Create
> > > Socket
> > > Error expected $create"
> > > +	else
> > > +		print_ok
> > > +	fi
> > 
> > There is no test that can trigger MPTcpExtMPJoinSynTxFEstabErr and
> > MPTcpExtMPJoinSynTxCreatSkErr. Does this mean that there is no need
> > to
> > add these two MIBs? Maybe adding pr_debug logs is enough? WDYT?
> 
> MPTcpExtMPJoinSynTxFEstabErr can be easily triggered by the userspace
> PM. Maybe we don't need the counter for this case then? We could also
> do
> the check in the userspace PM side, and return a netlink message
> instead, but I don't think it is needed. Not sure what's best here. I
> can remove it or leave it.

If so I think it's better to remove it, since too many MIBs are added
in this function.

> 
> For MPTcpExtMPJoinSynTxCreatSkErr, it can be triggered if it cannot
> allocate memory, if the socket limit has been reached out, if a
> security
> module blocked it, etc. I think we should keep it, and we don't need
> to
> validate it in the tests.

These errors are not related to MPTCP protocol. I think we can remove
this MIB but keep its pr_debug log. WDYT?

Regards,
-Geliang

> 
> > > +
> > > +	print_check "syn TX Bind Error"
> > > +	count=$(mptcp_lib_get_counter ${ns2}
> > > "MPTcpExtMPJoinSynTxBindErr")
> > > +	if [ -z "$count" ]; then
> > > +		print_skip
> > > +	elif [ "$count" != "$bind" ]; then
> > > +		fail_test "got $count JOIN[s] syn TX Bind Error
> > > expected $bind"
> > > +	else
> > > +		print_ok
> > > +	fi
> > > +
> > > +	print_check "syn TX Connect Error"
> > > +	count=$(mptcp_lib_get_counter ${ns2}
> > > "MPTcpExtMPJoinSynTxConnectErr")
> > > +	if [ -z "$count" ]; then
> > > +		print_skip
> > > +	elif [ "$count" != "$connect" ]; then
> > > +		fail_test "got $count JOIN[s] syn TX Connect
> > > Error
> > > expected $connect"
> > > +	else
> > > +		print_ok
> > > +	fi
> > > +}
> > > +
> > >  # a negative value for 'stale_max' means no upper bound:
> > >  # for bidirectional transfer, if one peer sleep for a while
> > >  # - as these tests do - we can have a quite high number of
> > > @@ -1907,6 +1967,7 @@ subflows_error_tests()
> > 
> > Add an invalid address here in this test:
> > 
> > 	pm_nl_add_endpoint $ns2 10.0.5.2 flags subflow
> > 
> > >  		speed=slow \
> > >  			run_tests $ns1 $ns2 10.0.1.1
> > >  		chk_join_nr 0 0 0
> > > +		chk_join_tx_nr 0 0 0 0 0
> > 
> > It will trigger a "syn TX Bind Error":
> > 
> > 		chk_join_tx_nr 0 0 0 1 0
> 
> Good idea! I will do that in the v3.
> 
> Cheers,
> Matt

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.