This function also writes the name of the test with its ID, making clear
a new test has been executed.
Without that, the ADD_ADDR results from this test was appended at the
end of the previous test causing confusions. Especially when the second
test was failing, we had:
17 signal invalid addresses syn[ ok ] - synack[ ok ] - ack[ ok ]
add[ ok ] - echo [ ok ]
add[fail] got 2 ADD_ADDR[s] expected 3
In fact, this 17th test was OK but not the 18th one.
Now we have:
17 signal invalid addresses syn[ ok ] - synack[ ok ] - ack[ ok ]
add[ ok ] - echo [ ok ]
18 signal addresses race test syn[ ok ] - synack[ ok ] - ack[ ok ]
add[fail] got 2 ADD_ADDR[s] expected 3
Fixes: 33c563ad28e3 ("selftests: mptcp: add_addr and echo race test")
Reported-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
tools/testing/selftests/net/mptcp/mptcp_join.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 76c45c845d3b..67be3e1215d7 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1342,6 +1342,7 @@ signal_address_tests()
pm_nl_add_endpoint $ns2 10.0.3.2 flags signal
pm_nl_add_endpoint $ns2 10.0.4.2 flags signal
run_tests $ns1 $ns2 10.0.1.1
+ chk_join_nr "signal addresses race test" 3 3 3
# the server will not signal the address terminating
# the MPC subflow
--
2.33.1
Hi Matt,
I think in this test chk_join_nr is omitted on purpose, since it is more
likely to fail. As we can see, chk_add_nr failed sometimes, if we add this
chk_join_nr, it will fail more frequently.
Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月22日周六 17:32写道：
>
> This function also writes the name of the test with its ID, making clear
> a new test has been executed.
>
> Without that, the ADD_ADDR results from this test was appended at the
> end of the previous test causing confusions. Especially when the second
> test was failing, we had:
>
> 17 signal invalid addresses syn[ ok ] - synack[ ok ] - ack[ ok ]
> add[ ok ] - echo [ ok ]
> add[fail] got 2 ADD_ADDR[s] expected 3
>
> In fact, this 17th test was OK but not the 18th one.
>
> Now we have:
>
> 17 signal invalid addresses syn[ ok ] - synack[ ok ] - ack[ ok ]
> add[ ok ] - echo [ ok ]
> 18 signal addresses race test syn[ ok ] - synack[ ok ] - ack[ ok ]
> add[fail] got 2 ADD_ADDR[s] expected 3
I think if chk_add_nr fail here, chk_join_nr must fail too. If we got 2
ADD_ADDR[s], we got 2 JOIN[s] syn too, not 3. Furthermore, if chk_add_nr
passed, we can't make sure chk_join_nr will pass too. Since MP_JOIN is
translated after ADD_ADDR, it may be lost.
>
> Fixes: 33c563ad28e3 ("selftests: mptcp: add_addr and echo race test")
> Reported-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
> tools/testing/selftests/net/mptcp/mptcp_join.sh | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 76c45c845d3b..67be3e1215d7 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -1342,6 +1342,7 @@ signal_address_tests()
> pm_nl_add_endpoint $ns2 10.0.3.2 flags signal
> pm_nl_add_endpoint $ns2 10.0.4.2 flags signal
> run_tests $ns1 $ns2 10.0.1.1
> + chk_join_nr "signal addresses race test" 3 3 3
I think we can omit the join numbers check, just print out the name of the
test. Don't pass the join numbers to chk_join_nr, just the msg:
chk_join_nr "signal addresses race test"
Then in chk_join_nr, test the input numbers before doing the numbers check:
'''
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -918,7 +918,7 @@ chk_join_nr()
printf "%02u %-36s %s" "$TEST_COUNT" "$msg" "syn"
count=`ip netns exec $ns1 nstat -as | grep MPTcpExtMPJoinSynRx |
awk '{print $2}'`
[ -z "$count" ] && count=0
- if [ "$count" != "$syn_nr" ]; then
+ if [ -n "$syn_nr" ] && [ "$count" != "$syn_nr" ]; then
echo "[fail] got $count JOIN[s] syn expected $syn_nr"
ret=1
dump_stats=1
@@ -929,7 +929,7 @@ chk_join_nr()
echo -n " - synack"
count=`ip netns exec $ns2 nstat -as | grep MPTcpExtMPJoinSynAckRx
| awk '{print $2}'`
[ -z "$count" ] && count=0
- if [ "$count" != "$syn_ack_nr" ]; then
+ if [ -n "$syn_ack_nr" ] && [ "$count" != "$syn_ack_nr" ]; then
echo "[fail] got $count JOIN[s] synack expected $syn_ack_nr"
ret=1
dump_stats=1
@@ -940,7 +940,7 @@ chk_join_nr()
echo -n " - ack"
count=`ip netns exec $ns1 nstat -as | grep MPTcpExtMPJoinAckRx |
awk '{print $2}'`
[ -z "$count" ] && count=0
- if [ "$count" != "$ack_nr" ]; then
+ if [ -n ""$ack_nr"" ] && [ "$count" != "$ack_nr" ]; then
echo "[fail] got $count JOIN[s] ack expected $ack_nr"
ret=1
dump_stats=1
'''
WDYT?
Thanks,
-Geliang
>
> # the server will not signal the address terminating
> # the MPC subflow
> --
> 2.33.1
>
>
Hi Geliang,
Thank you for looking at that!
On 22/01/2022 15:04, Geliang Tang wrote:
> I think in this test chk_join_nr is omitted on purpose, since it is more
> likely to fail. As we can see, chk_add_nr failed sometimes, if we add this
> chk_join_nr, it will fail more frequently.
May you explain why this tests is likely to fail? Both the commit
message and the comment in the code don't explain that.
> Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月22日周六 17:32写道：
>>
>> This function also writes the name of the test with its ID, making clear
>> a new test has been executed.
>>
>> Without that, the ADD_ADDR results from this test was appended at the
>> end of the previous test causing confusions. Especially when the second
>> test was failing, we had:
>>
>> 17 signal invalid addresses syn[ ok ] - synack[ ok ] - ack[ ok ]
>> add[ ok ] - echo [ ok ]
>> add[fail] got 2 ADD_ADDR[s] expected 3
>>
>> In fact, this 17th test was OK but not the 18th one.
>>
>> Now we have:
>>
>> 17 signal invalid addresses syn[ ok ] - synack[ ok ] - ack[ ok ]
>> add[ ok ] - echo [ ok ]
>> 18 signal addresses race test syn[ ok ] - synack[ ok ] - ack[ ok ]
>> add[fail] got 2 ADD_ADDR[s] expected 3
>
> I think if chk_add_nr fail here, chk_join_nr must fail too. If we got 2
> ADD_ADDR[s], we got 2 JOIN[s] syn too, not 3. Furthermore, if chk_add_nr
> passed, we can't make sure chk_join_nr will pass too. Since MP_JOIN is
> translated after ADD_ADDR, it may be lost.
Indeed, to simplify the commit message, I mixed both a "success" and a
"failure" output. I should not have done that and only put a "success"
one to keep the output small.
>> Fixes: 33c563ad28e3 ("selftests: mptcp: add_addr and echo race test")
>> Reported-by: Paolo Abeni <pabeni@redhat.com>
>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>> ---
>> tools/testing/selftests/net/mptcp/mptcp_join.sh | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> index 76c45c845d3b..67be3e1215d7 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> @@ -1342,6 +1342,7 @@ signal_address_tests()
>> pm_nl_add_endpoint $ns2 10.0.3.2 flags signal
>> pm_nl_add_endpoint $ns2 10.0.4.2 flags signal
>> run_tests $ns1 $ns2 10.0.1.1
>> + chk_join_nr "signal addresses race test" 3 3 3
>
> I think we can omit the join numbers check, just print out the name of the
> test. Don't pass the join numbers to chk_join_nr, just the msg:
>
> chk_join_nr "signal addresses race test"
Do you mean we could have situations where ADD_ADDR have been properly
sent and received but we check counters just before MPJ are emitted? In
this case we would mark the test as failed because MPJs are not OK
(checked too soon in fact) while ADD_ADDR are?
If yes and if it helps, we can also wait for the kernel to have sent
exactly 3 ADD_ADDR before checking the MPJ. On the other hand, do we not
already cover this UC where we look for MPJs emitted after having
received an ADD_ADDR, no?
If we keep checking all counters, I think it might be clearer: MPJ
counters are not OK because we didn't receive/process the ADD_ADDR, no?
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
Hi Matt,
I guess we will get this error:
'''
01 signal addresses race test syn[fail] got 2 JOIN[s] syn expected 3
- synack[fail] got 2 JOIN[s] synack expected 3
- ack[fail] got 2 JOIN[s] ack expected 3
add[ ok ] - echo [ok]
'''
But it didn't happen in my tests. I only got this error:
'''
01 signal addresses race test syn[fail] got 2 JOIN[s] syn expected 3
- synack[fail] got 2 JOIN[s] synack expected 3
- ack[fail] got 2 JOIN[s] ack expected 3
add[ ok ] - echo [fail] got 2
ADD_ADDR echo[s] expected 3
'''
I'm not sure now. Please ignore my comments.
Thanks,
-Geliang
Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月24日周一 17:39写道：
>
> Hi Geliang,
>
> Thank you for looking at that!
>
> On 22/01/2022 15:04, Geliang Tang wrote:
> > I think in this test chk_join_nr is omitted on purpose, since it is more
> > likely to fail. As we can see, chk_add_nr failed sometimes, if we add this
> > chk_join_nr, it will fail more frequently.
>
> May you explain why this tests is likely to fail? Both the commit
> message and the comment in the code don't explain that.
>
> > Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月22日周六 17:32写道：
> >>
> >> This function also writes the name of the test with its ID, making clear
> >> a new test has been executed.
> >>
> >> Without that, the ADD_ADDR results from this test was appended at the
> >> end of the previous test causing confusions. Especially when the second
> >> test was failing, we had:
> >>
> >> 17 signal invalid addresses syn[ ok ] - synack[ ok ] - ack[ ok ]
> >> add[ ok ] - echo [ ok ]
> >> add[fail] got 2 ADD_ADDR[s] expected 3
> >>
> >> In fact, this 17th test was OK but not the 18th one.
> >>
> >> Now we have:
> >>
> >> 17 signal invalid addresses syn[ ok ] - synack[ ok ] - ack[ ok ]
> >> add[ ok ] - echo [ ok ]
> >> 18 signal addresses race test syn[ ok ] - synack[ ok ] - ack[ ok ]
> >> add[fail] got 2 ADD_ADDR[s] expected 3
> >
> > I think if chk_add_nr fail here, chk_join_nr must fail too. If we got 2
> > ADD_ADDR[s], we got 2 JOIN[s] syn too, not 3. Furthermore, if chk_add_nr
> > passed, we can't make sure chk_join_nr will pass too. Since MP_JOIN is
> > translated after ADD_ADDR, it may be lost.
>
> Indeed, to simplify the commit message, I mixed both a "success" and a
> "failure" output. I should not have done that and only put a "success"
> one to keep the output small.
>
> >> Fixes: 33c563ad28e3 ("selftests: mptcp: add_addr and echo race test")
> >> Reported-by: Paolo Abeni <pabeni@redhat.com>
> >> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> >> ---
> >> tools/testing/selftests/net/mptcp/mptcp_join.sh | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> >> index 76c45c845d3b..67be3e1215d7 100755
> >> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> >> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> >> @@ -1342,6 +1342,7 @@ signal_address_tests()
> >> pm_nl_add_endpoint $ns2 10.0.3.2 flags signal
> >> pm_nl_add_endpoint $ns2 10.0.4.2 flags signal
> >> run_tests $ns1 $ns2 10.0.1.1
> >> + chk_join_nr "signal addresses race test" 3 3 3
> >
> > I think we can omit the join numbers check, just print out the name of the
> > test. Don't pass the join numbers to chk_join_nr, just the msg:
> >
> > chk_join_nr "signal addresses race test"
>
> Do you mean we could have situations where ADD_ADDR have been properly
> sent and received but we check counters just before MPJ are emitted? In
> this case we would mark the test as failed because MPJs are not OK
> (checked too soon in fact) while ADD_ADDR are?
>
> If yes and if it helps, we can also wait for the kernel to have sent
> exactly 3 ADD_ADDR before checking the MPJ. On the other hand, do we not
> already cover this UC where we look for MPJs emitted after having
> received an ADD_ADDR, no?
>
> If we keep checking all counters, I think it might be clearer: MPJ
> counters are not OK because we didn't receive/process the ADD_ADDR, no?
>
> Cheers,
> Matt
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
On Mon, 24 Jan 2022, Matthieu Baerts wrote:
> Hi Geliang,
>
> Thank you for looking at that!
>
> On 22/01/2022 15:04, Geliang Tang wrote:
>> I think in this test chk_join_nr is omitted on purpose, since it is more
>> likely to fail. As we can see, chk_add_nr failed sometimes, if we add this
>> chk_join_nr, it will fail more frequently.
>
> May you explain why this tests is likely to fail? Both the commit
> message and the comment in the code don't explain that.
>
>> Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月22日周六 17:32写道：
>>>
>>> This function also writes the name of the test with its ID, making clear
>>> a new test has been executed.
>>>
>>> Without that, the ADD_ADDR results from this test was appended at the
>>> end of the previous test causing confusions. Especially when the second
>>> test was failing, we had:
>>>
>>> 17 signal invalid addresses syn[ ok ] - synack[ ok ] - ack[ ok ]
>>> add[ ok ] - echo [ ok ]
>>> add[fail] got 2 ADD_ADDR[s] expected 3
>>>
>>> In fact, this 17th test was OK but not the 18th one.
>>>
>>> Now we have:
>>>
>>> 17 signal invalid addresses syn[ ok ] - synack[ ok ] - ack[ ok ]
>>> add[ ok ] - echo [ ok ]
>>> 18 signal addresses race test syn[ ok ] - synack[ ok ] - ack[ ok ]
>>> add[fail] got 2 ADD_ADDR[s] expected 3
>>
>> I think if chk_add_nr fail here, chk_join_nr must fail too. If we got 2
>> ADD_ADDR[s], we got 2 JOIN[s] syn too, not 3. Furthermore, if chk_add_nr
>> passed, we can't make sure chk_join_nr will pass too. Since MP_JOIN is
>> translated after ADD_ADDR, it may be lost.
>
> Indeed, to simplify the commit message, I mixed both a "success" and a
> "failure" output. I should not have done that and only put a "success"
> one to keep the output small.
>
Given that Geliang withdrew his comments, and I don't have changes to
request, I think this looks ok for the export branch:
Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
Not sure if you want to adjust the commit message as you mention just
above.
-Mat
>>> Fixes: 33c563ad28e3 ("selftests: mptcp: add_addr and echo race test")
>>> Reported-by: Paolo Abeni <pabeni@redhat.com>
>>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>>> ---
>>> tools/testing/selftests/net/mptcp/mptcp_join.sh | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>> index 76c45c845d3b..67be3e1215d7 100755
>>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>> @@ -1342,6 +1342,7 @@ signal_address_tests()
>>> pm_nl_add_endpoint $ns2 10.0.3.2 flags signal
>>> pm_nl_add_endpoint $ns2 10.0.4.2 flags signal
>>> run_tests $ns1 $ns2 10.0.1.1
>>> + chk_join_nr "signal addresses race test" 3 3 3
>>
>> I think we can omit the join numbers check, just print out the name of the
>> test. Don't pass the join numbers to chk_join_nr, just the msg:
>>
>> chk_join_nr "signal addresses race test"
>
> Do you mean we could have situations where ADD_ADDR have been properly
> sent and received but we check counters just before MPJ are emitted? In
> this case we would mark the test as failed because MPJs are not OK
> (checked too soon in fact) while ADD_ADDR are?
>
> If yes and if it helps, we can also wait for the kernel to have sent
> exactly 3 ADD_ADDR before checking the MPJ. On the other hand, do we not
> already cover this UC where we look for MPJs emitted after having
> received an ADD_ADDR, no?
>
> If we keep checking all counters, I think it might be clearer: MPJ
> counters are not OK because we didn't receive/process the ADD_ADDR, no?
>
--
Mat Martineau
Intel
Hi Mat, Geliang, Paolo, On 26/01/2022 02:26, Mat Martineau wrote: > On Mon, 24 Jan 2022, Matthieu Baerts wrote: > >> Hi Geliang, >> >> Thank you for looking at that! >> >> On 22/01/2022 15:04, Geliang Tang wrote: >>> I think in this test chk_join_nr is omitted on purpose, since it is more >>> likely to fail. As we can see, chk_add_nr failed sometimes, if we add >>> this >>> chk_join_nr, it will fail more frequently. >> >> May you explain why this tests is likely to fail? Both the commit >> message and the comment in the code don't explain that. >> >>> Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月22日周六 >>> 17:32写道： >>>> >>>> This function also writes the name of the test with its ID, making >>>> clear >>>> a new test has been executed. >>>> >>>> Without that, the ADD_ADDR results from this test was appended at the >>>> end of the previous test causing confusions. Especially when the second >>>> test was failing, we had: >>>> >>>> 17 signal invalid addresses syn[ ok ] - synack[ ok ] - ack[ ok ] >>>> add[ ok ] - echo [ ok ] >>>> add[fail] got 2 ADD_ADDR[s] >>>> expected 3 >>>> >>>> In fact, this 17th test was OK but not the 18th one. >>>> >>>> Now we have: >>>> >>>> 17 signal invalid addresses syn[ ok ] - synack[ ok ] - ack[ ok ] >>>> add[ ok ] - echo [ ok ] >>>> 18 signal addresses race test syn[ ok ] - synack[ ok ] - ack[ ok ] >>>> add[fail] got 2 ADD_ADDR[s] >>>> expected 3 >>> >>> I think if chk_add_nr fail here, chk_join_nr must fail too. If we got 2 >>> ADD_ADDR[s], we got 2 JOIN[s] syn too, not 3. Furthermore, if chk_add_nr >>> passed, we can't make sure chk_join_nr will pass too. Since MP_JOIN is >>> translated after ADD_ADDR, it may be lost. >> >> Indeed, to simplify the commit message, I mixed both a "success" and a >> "failure" output. I should not have done that and only put a "success" >> one to keep the output small. >> > > Given that Geliang withdrew his comments, and I don't have changes to > request, I think this looks ok for the export branch: > > Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com> Thank you for the reviews and suggestions! > Not sure if you want to adjust the commit message as you mention just > above. Yes, just did by writing output from the CI: https://api.cirrus-ci.com/v1/artifact/task/5375837896704000/summary/summary.txt - 948d50855388: mptcp: mptcp_parse_option is no longer exported - ee286fda85da: selftests: mptcp: add missing join check - Results: 419c18898efd..d57649f20e6e Builds and tests are now in progress: https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220128T164437 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
© 2016 - 2026 Red Hat, Inc.