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
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 >
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.
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
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.
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
© 2016 - 2024 Red Hat, Inc.