[v3] mptcp: MIB counters for sent MP_JOIN

[PATCH mptcp-next v3 1/8] selftests: mptcp: join: no extra msg if no counter

Posted by Matthieu Baerts (NGI0) 4 months ago

The checksum and fail counters might not be available. Then no need to
display an extra message with missing info.

While at it, fix the indentation around, which is wrong since the same
commit.

Fixes: 47867f0a7e83 ("selftests: mptcp: join: skip check if MIB counter not supported")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 01c1e0871aca..a1f80dac59a7 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1112,7 +1112,7 @@ chk_csum_nr()
 
 	print_check "sum"
 	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtDataCsumErr")
-	if [ "$count" != "$csum_ns1" ]; then
+	if [ -n "$count" ] && [ "$count" != "$csum_ns1" ]; then
 		extra_msg+=" ns1=$count"
 	fi
 	if [ -z "$count" ]; then
@@ -1125,7 +1125,7 @@ chk_csum_nr()
 	fi
 	print_check "csum"
 	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtDataCsumErr")
-	if [ "$count" != "$csum_ns2" ]; then
+	if [ -n "$count" ] && [ "$count" != "$csum_ns2" ]; then
 		extra_msg+=" ns2=$count"
 	fi
 	if [ -z "$count" ]; then
@@ -1169,13 +1169,13 @@ chk_fail_nr()
 
 	print_check "ftx"
 	count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPFailTx")
-	if [ "$count" != "$fail_tx" ]; then
+	if [ -n "$count" ] && [ "$count" != "$fail_tx" ]; then
 		extra_msg+=",tx=$count"
 	fi
 	if [ -z "$count" ]; then
 		print_skip
 	elif { [ "$count" != "$fail_tx" ] && [ $allow_tx_lost -eq 0 ]; } ||
-	   { [ "$count" -gt "$fail_tx" ] && [ $allow_tx_lost -eq 1 ]; }; then
+	     { [ "$count" -gt "$fail_tx" ] && [ $allow_tx_lost -eq 1 ]; }; then
 		fail_test "got $count MP_FAIL[s] TX expected $fail_tx"
 	else
 		print_ok
@@ -1183,13 +1183,13 @@ chk_fail_nr()
 
 	print_check "failrx"
 	count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtMPFailRx")
-	if [ "$count" != "$fail_rx" ]; then
+	if [ -n "$count" ] && [ "$count" != "$fail_rx" ]; then
 		extra_msg+=",rx=$count"
 	fi
 	if [ -z "$count" ]; then
 		print_skip
 	elif { [ "$count" != "$fail_rx" ] && [ $allow_rx_lost -eq 0 ]; } ||
-	   { [ "$count" -gt "$fail_rx" ] && [ $allow_rx_lost -eq 1 ]; }; then
+	     { [ "$count" -gt "$fail_rx" ] && [ $allow_rx_lost -eq 1 ]; }; then
 		fail_test "got $count MP_FAIL[s] RX expected $fail_rx"
 	else
 		print_ok

-- 
2.45.2

Re: [PATCH mptcp-next v3 1/8] selftests: mptcp: join: no extra msg if no counter

Posted by Geliang Tang 3 months, 4 weeks ago

Hi Matt,

Thanks for these patches.

On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0) wrote:
> The checksum and fail counters might not be available. Then no need
> to
> display an extra message with missing info.
> 
> While at it, fix the indentation around, which is wrong since the
> same
> commit.
> 
> Fixes: 47867f0a7e83 ("selftests: mptcp: join: skip check if MIB
> counter not supported")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 01c1e0871aca..a1f80dac59a7 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -1112,7 +1112,7 @@ chk_csum_nr()
>  
>  	print_check "sum"
>  	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtDataCsumErr")
> -	if [ "$count" != "$csum_ns1" ]; then
> +	if [ -n "$count" ] && [ "$count" != "$csum_ns1" ]; then

We have checked [ -z "$count" ] in mptcp_lib_get_counter() already, so
I think no need to double check it here. Can we just let
mptcp_lib_get_counter() return "0" in this [ -z "$count" ] case, then
we can drop all these [ -n "$count" ] or [ -z "$count" ] (like in
chk_cestab_nr()) checks after mptcp_lib_get_counter()? WDYT?

Regards,
-Geliang

>  		extra_msg+=" ns1=$count"
>  	fi
>  	if [ -z "$count" ]; then
> @@ -1125,7 +1125,7 @@ chk_csum_nr()
>  	fi
>  	print_check "csum"
>  	count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtDataCsumErr")
> -	if [ "$count" != "$csum_ns2" ]; then
> +	if [ -n "$count" ] && [ "$count" != "$csum_ns2" ]; then
>  		extra_msg+=" ns2=$count"
>  	fi
>  	if [ -z "$count" ]; then
> @@ -1169,13 +1169,13 @@ chk_fail_nr()
>  
>  	print_check "ftx"
>  	count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPFailTx")
> -	if [ "$count" != "$fail_tx" ]; then
> +	if [ -n "$count" ] && [ "$count" != "$fail_tx" ]; then
>  		extra_msg+=",tx=$count"
>  	fi
>  	if [ -z "$count" ]; then
>  		print_skip
>  	elif { [ "$count" != "$fail_tx" ] && [ $allow_tx_lost -eq 0
> ]; } ||
> -	   { [ "$count" -gt "$fail_tx" ] && [ $allow_tx_lost -eq 1
> ]; }; then
> +	     { [ "$count" -gt "$fail_tx" ] && [ $allow_tx_lost -eq 1
> ]; }; then
>  		fail_test "got $count MP_FAIL[s] TX expected
> $fail_tx"
>  	else
>  		print_ok
> @@ -1183,13 +1183,13 @@ chk_fail_nr()
>  
>  	print_check "failrx"
>  	count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtMPFailRx")
> -	if [ "$count" != "$fail_rx" ]; then
> +	if [ -n "$count" ] && [ "$count" != "$fail_rx" ]; then
>  		extra_msg+=",rx=$count"
>  	fi
>  	if [ -z "$count" ]; then
>  		print_skip
>  	elif { [ "$count" != "$fail_rx" ] && [ $allow_rx_lost -eq 0
> ]; } ||
> -	   { [ "$count" -gt "$fail_rx" ] && [ $allow_rx_lost -eq 1
> ]; }; then
> +	     { [ "$count" -gt "$fail_rx" ] && [ $allow_rx_lost -eq 1
> ]; }; then
>  		fail_test "got $count MP_FAIL[s] RX expected
> $fail_rx"
>  	else
>  		print_ok
>

Re: [PATCH mptcp-next v3 1/8] selftests: mptcp: join: no extra msg if no counter

Posted by Matthieu Baerts 3 months, 4 weeks ago

Hi Geliang,

Thank you for the review!

On 08/08/2024 04:38, Geliang Tang wrote:
> Hi Matt,
> 
> Thanks for these patches.
> 
> On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0) wrote:
>> The checksum and fail counters might not be available. Then no need
>> to
>> display an extra message with missing info.
>>
>> While at it, fix the indentation around, which is wrong since the
>> same
>> commit.
>>
>> Fixes: 47867f0a7e83 ("selftests: mptcp: join: skip check if MIB
>> counter not supported")
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> index 01c1e0871aca..a1f80dac59a7 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> @@ -1112,7 +1112,7 @@ chk_csum_nr()
>>  
>>  	print_check "sum"
>>  	count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtDataCsumErr")
>> -	if [ "$count" != "$csum_ns1" ]; then
>> +	if [ -n "$count" ] && [ "$count" != "$csum_ns1" ]; then
> 
> We have checked [ -z "$count" ] in mptcp_lib_get_counter() already, so
> I think no need to double check it here. Can we just let
> mptcp_lib_get_counter() return "0" in this [ -z "$count" ] case, then
> we can drop all these [ -n "$count" ] or [ -z "$count" ] (like in
> chk_cestab_nr()) checks after mptcp_lib_get_counter()? WDYT?

I don't think we can do that: mptcp_lib_get_counter() will return
nothing if the counter doesn't exist: if the kernel being used doesn't
support it. If it is our CI running the tests, there will be a failure
thanks to "mptcp_lib_fail_if_expected_feature()", but only with *our*
CI, because "SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES" is set to 1.

So here, we need to check if '$count' is not empty, before comparing it
with '$csum_ns1'. If it is empty, the check will be skipped (see 4 lines
below), but we don't want to print a useless "extra message" in this
case, with just "ns1=".

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

Re: [PATCH mptcp-next v3 1/8] selftests: mptcp: join: no extra msg if no counter

Posted by Geliang Tang 3 months, 3 weeks ago

On Thu, 2024-08-08 at 12:17 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for the review!
> 
> On 08/08/2024 04:38, Geliang Tang wrote:
> > Hi Matt,
> > 
> > Thanks for these patches.
> > 
> > On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0) wrote:
> > > The checksum and fail counters might not be available. Then no
> > > need
> > > to
> > > display an extra message with missing info.
> > > 
> > > While at it, fix the indentation around, which is wrong since the
> > > same
> > > commit.
> > > 
> > > Fixes: 47867f0a7e83 ("selftests: mptcp: join: skip check if MIB
> > > counter not supported")
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > >  tools/testing/selftests/net/mptcp/mptcp_join.sh | 12 ++++++-----
> > > -
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > index 01c1e0871aca..a1f80dac59a7 100755
> > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > @@ -1112,7 +1112,7 @@ chk_csum_nr()
> > >  
> > >  	print_check "sum"
> > >  	count=$(mptcp_lib_get_counter ${ns1}
> > > "MPTcpExtDataCsumErr")
> > > -	if [ "$count" != "$csum_ns1" ]; then
> > > +	if [ -n "$count" ] && [ "$count" != "$csum_ns1" ]; then
> > 
> > We have checked [ -z "$count" ] in mptcp_lib_get_counter() already,
> > so
> > I think no need to double check it here. Can we just let
> > mptcp_lib_get_counter() return "0" in this [ -z "$count" ] case,
> > then
> > we can drop all these [ -n "$count" ] or [ -z "$count" ] (like in
> > chk_cestab_nr()) checks after mptcp_lib_get_counter()? WDYT?
> 
> I don't think we can do that: mptcp_lib_get_counter() will return
> nothing if the counter doesn't exist: if the kernel being used
> doesn't
> support it. If it is our CI running the tests, there will be a
> failure
> thanks to "mptcp_lib_fail_if_expected_feature()", but only with *our*
> CI, because "SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES" is set to 1.
> 
> So here, we need to check if '$count' is not empty, before comparing
> it
> with '$csum_ns1'. If it is empty, the check will be skipped (see 4
> lines
> below), but we don't want to print a useless "extra message" in this
> case, with just "ns1=".

If so, let's keep this patch as is. I'll try to fix it later.

> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v3 1/8] selftests: mptcp: join: no extra msg if no counter

Posted by Matthieu Baerts 3 months, 3 weeks ago

Hi Geliang,

On 09/08/2024 04:30, Geliang Tang wrote:
> On Thu, 2024-08-08 at 12:17 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> Thank you for the review!
>>
>> On 08/08/2024 04:38, Geliang Tang wrote:
>>> Hi Matt,
>>>
>>> Thanks for these patches.
>>>
>>> On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0) wrote:
>>>> The checksum and fail counters might not be available. Then no
>>>> need
>>>> to
>>>> display an extra message with missing info.
>>>>
>>>> While at it, fix the indentation around, which is wrong since the
>>>> same
>>>> commit.
>>>>
>>>> Fixes: 47867f0a7e83 ("selftests: mptcp: join: skip check if MIB
>>>> counter not supported")
>>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>>> ---
>>>>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 12 ++++++-----
>>>> -
>>>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> index 01c1e0871aca..a1f80dac59a7 100755
>>>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> @@ -1112,7 +1112,7 @@ chk_csum_nr()
>>>>  
>>>>  	print_check "sum"
>>>>  	count=$(mptcp_lib_get_counter ${ns1}
>>>> "MPTcpExtDataCsumErr")
>>>> -	if [ "$count" != "$csum_ns1" ]; then
>>>> +	if [ -n "$count" ] && [ "$count" != "$csum_ns1" ]; then
>>>
>>> We have checked [ -z "$count" ] in mptcp_lib_get_counter() already,
>>> so
>>> I think no need to double check it here. Can we just let
>>> mptcp_lib_get_counter() return "0" in this [ -z "$count" ] case,
>>> then
>>> we can drop all these [ -n "$count" ] or [ -z "$count" ] (like in
>>> chk_cestab_nr()) checks after mptcp_lib_get_counter()? WDYT?
>>
>> I don't think we can do that: mptcp_lib_get_counter() will return
>> nothing if the counter doesn't exist: if the kernel being used
>> doesn't
>> support it. If it is our CI running the tests, there will be a
>> failure
>> thanks to "mptcp_lib_fail_if_expected_feature()", but only with *our*
>> CI, because "SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES" is set to 1.
>>
>> So here, we need to check if '$count' is not empty, before comparing
>> it
>> with '$csum_ns1'. If it is empty, the check will be skipped (see 4
>> lines
>> below), but we don't want to print a useless "extra message" in this
>> case, with just "ns1=".
> 
> If so, let's keep this patch as is. I'll try to fix it later.

I don't think there is anything to fix there. We need these two checks:
one for our CI to detect missing features, and the other one to skip the
test if the (old) kernel (ran by another CI) doesn't support the counter.

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

Re: [PATCH mptcp-next v3 1/8] selftests: mptcp: join: no extra msg if no counter

Posted by Geliang Tang 3 months, 3 weeks ago

On Fri, 2024-08-09 at 13:32 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 09/08/2024 04:30, Geliang Tang wrote:
> > On Thu, 2024-08-08 at 12:17 +0200, Matthieu Baerts wrote:
> > > Hi Geliang,
> > > 
> > > Thank you for the review!
> > > 
> > > On 08/08/2024 04:38, Geliang Tang wrote:
> > > > Hi Matt,
> > > > 
> > > > Thanks for these patches.
> > > > 
> > > > On Tue, 2024-08-06 at 13:18 +0200, Matthieu Baerts (NGI0)
> > > > wrote:
> > > > > The checksum and fail counters might not be available. Then
> > > > > no
> > > > > need
> > > > > to
> > > > > display an extra message with missing info.
> > > > > 
> > > > > While at it, fix the indentation around, which is wrong since
> > > > > the
> > > > > same
> > > > > commit.
> > > > > 
> > > > > Fixes: 47867f0a7e83 ("selftests: mptcp: join: skip check if
> > > > > MIB
> > > > > counter not supported")
> > > > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > > > ---
> > > > >  tools/testing/selftests/net/mptcp/mptcp_join.sh | 12 ++++++-
> > > > > ----
> > > > > -
> > > > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > > b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > > index 01c1e0871aca..a1f80dac59a7 100755
> > > > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > > @@ -1112,7 +1112,7 @@ chk_csum_nr()
> > > > >  
> > > > >  	print_check "sum"
> > > > >  	count=$(mptcp_lib_get_counter ${ns1}
> > > > > "MPTcpExtDataCsumErr")
> > > > > -	if [ "$count" != "$csum_ns1" ]; then
> > > > > +	if [ -n "$count" ] && [ "$count" != "$csum_ns1" ];
> > > > > then
> > > > 
> > > > We have checked [ -z "$count" ] in mptcp_lib_get_counter()
> > > > already,
> > > > so
> > > > I think no need to double check it here. Can we just let
> > > > mptcp_lib_get_counter() return "0" in this [ -z "$count" ]
> > > > case,
> > > > then
> > > > we can drop all these [ -n "$count" ] or [ -z "$count" ] (like
> > > > in
> > > > chk_cestab_nr()) checks after mptcp_lib_get_counter()? WDYT?
> > > 
> > > I don't think we can do that: mptcp_lib_get_counter() will return
> > > nothing if the counter doesn't exist: if the kernel being used
> > > doesn't
> > > support it. If it is our CI running the tests, there will be a
> > > failure
> > > thanks to "mptcp_lib_fail_if_expected_feature()", but only with
> > > *our*
> > > CI, because "SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES" is set to
> > > 1.
> > > 
> > > So here, we need to check if '$count' is not empty, before
> > > comparing
> > > it
> > > with '$csum_ns1'. If it is empty, the check will be skipped (see
> > > 4
> > > lines
> > > below), but we don't want to print a useless "extra message" in
> > > this
> > > case, with just "ns1=".
> > 
> > If so, let's keep this patch as is. I'll try to fix it later.
> 
> I don't think there is anything to fix there. We need these two
> checks:
> one for our CI to detect missing features, and the other one to skip
> the
> test if the (old) kernel (ran by another CI) doesn't support the
> counter.

Yes, indeed, you are right.

> 
> Cheers,
> Matt