* [PATCH mptcp-net 0/2] selftests: avoid confusions around 'signal invalid addresses'
@ 2022-01-22 9:31 Matthieu Baerts
2022-01-22 9:31 ` [PATCH mptcp-net 1/2] mptcp: mptcp_parse_option is no longer exported Matthieu Baerts
2022-01-22 9:31 ` [PATCH mptcp-net 2/2] selftests: mptcp: add missing join check Matthieu Baerts
0 siblings, 2 replies; 10+ messages in thread
From: Matthieu Baerts @ 2022-01-22 9:31 UTC (permalink / raw)
To: mptcp; +Cc: Matthieu Baerts
With Paolo, we were trying to understand why one test was failing, see:
https://github.com/multipath-tcp/mptcp_net-next/issues/254
Paolo noticed that even if the CI was reporting this:
17 signal invalid addresses syn[ ok ] - synack[ ok ] - ack[ ok ]
add[ ok ] - echo [ ok ]
add[fail] got 2 ADD_ADDR[s] expected 3
The issue here was not with the 17th test but with the next one. This patch can
be sent to -net.
While at it, we can also fully remove mptcp_parse_option() from mptcp.h. Not
sure this patch needs to be sent to -net but it should not be rejected from
there :)
Matthieu Baerts (2):
mptcp: mptcp_parse_option is no longer exported
selftests: mptcp: add missing join check
include/net/mptcp.h | 6 ------
tools/testing/selftests/net/mptcp/mptcp_join.sh | 1 +
2 files changed, 1 insertion(+), 6 deletions(-)
--
2.33.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH mptcp-net 1/2] mptcp: mptcp_parse_option is no longer exported
2022-01-22 9:31 [PATCH mptcp-net 0/2] selftests: avoid confusions around 'signal invalid addresses' Matthieu Baerts
@ 2022-01-22 9:31 ` Matthieu Baerts
2022-01-24 10:59 ` Paolo Abeni
2022-01-25 1:21 ` Mat Martineau
2022-01-22 9:31 ` [PATCH mptcp-net 2/2] selftests: mptcp: add missing join check Matthieu Baerts
1 sibling, 2 replies; 10+ messages in thread
From: Matthieu Baerts @ 2022-01-22 9:31 UTC (permalink / raw)
To: mptcp; +Cc: Matthieu Baerts
Options parsing in now done from mptcp_incoming_options().
mptcp_parse_option() has been removed from mptcp.h when CONFIG_MPTCP is
defined but not when it is not.
Fixes: cfde141ea3fa ("mptcp: move option parsing into mptcp_incoming_options()")
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
include/net/mptcp.h | 6 ------
1 file changed, 6 deletions(-)
diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 04f53352a1c9..8b1afd6f5cc4 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -218,12 +218,6 @@ static inline bool rsk_drop_req(const struct request_sock *req)
return false;
}
-static inline void mptcp_parse_option(const struct sk_buff *skb,
- const unsigned char *ptr, int opsize,
- struct tcp_options_received *opt_rx)
-{
-}
-
static inline bool mptcp_syn_options(struct sock *sk, const struct sk_buff *skb,
unsigned int *size,
struct mptcp_out_options *opts)
--
2.33.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH mptcp-net 2/2] selftests: mptcp: add missing join check
2022-01-22 9:31 [PATCH mptcp-net 0/2] selftests: avoid confusions around 'signal invalid addresses' Matthieu Baerts
2022-01-22 9:31 ` [PATCH mptcp-net 1/2] mptcp: mptcp_parse_option is no longer exported Matthieu Baerts
@ 2022-01-22 9:31 ` Matthieu Baerts
2022-01-22 14:04 ` Geliang Tang
1 sibling, 1 reply; 10+ messages in thread
From: Matthieu Baerts @ 2022-01-22 9:31 UTC (permalink / raw)
To: mptcp; +Cc: Matthieu Baerts, Paolo Abeni
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH mptcp-net 2/2] selftests: mptcp: add missing join check
2022-01-22 9:31 ` [PATCH mptcp-net 2/2] selftests: mptcp: add missing join check Matthieu Baerts
@ 2022-01-22 14:04 ` Geliang Tang
2022-01-24 9:39 ` Matthieu Baerts
0 siblings, 1 reply; 10+ messages in thread
From: Geliang Tang @ 2022-01-22 14:04 UTC (permalink / raw)
To: Matthieu Baerts; +Cc: MPTCP Upstream, Paolo Abeni
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
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH mptcp-net 2/2] selftests: mptcp: add missing join check
2022-01-22 14:04 ` Geliang Tang
@ 2022-01-24 9:39 ` Matthieu Baerts
2022-01-25 3:24 ` Geliang Tang
2022-01-26 1:26 ` Mat Martineau
0 siblings, 2 replies; 10+ messages in thread
From: Matthieu Baerts @ 2022-01-24 9:39 UTC (permalink / raw)
To: Geliang Tang; +Cc: MPTCP Upstream, Paolo Abeni
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH mptcp-net 1/2] mptcp: mptcp_parse_option is no longer exported
2022-01-22 9:31 ` [PATCH mptcp-net 1/2] mptcp: mptcp_parse_option is no longer exported Matthieu Baerts
@ 2022-01-24 10:59 ` Paolo Abeni
2022-01-25 1:21 ` Mat Martineau
1 sibling, 0 replies; 10+ messages in thread
From: Paolo Abeni @ 2022-01-24 10:59 UTC (permalink / raw)
To: Matthieu Baerts, mptcp
On Sat, 2022-01-22 at 10:31 +0100, Matthieu Baerts wrote:
> Options parsing in now done from mptcp_incoming_options().
>
> mptcp_parse_option() has been removed from mptcp.h when CONFIG_MPTCP is
> defined but not when it is not.
>
> Fixes: cfde141ea3fa ("mptcp: move option parsing into mptcp_incoming_options()")
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
> include/net/mptcp.h | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 04f53352a1c9..8b1afd6f5cc4 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -218,12 +218,6 @@ static inline bool rsk_drop_req(const struct request_sock *req)
> return false;
> }
>
> -static inline void mptcp_parse_option(const struct sk_buff *skb,
> - const unsigned char *ptr, int opsize,
> - struct tcp_options_received *opt_rx)
> -{
> -}
> -
> static inline bool mptcp_syn_options(struct sock *sk, const struct sk_buff *skb,
> unsigned int *size,
> struct mptcp_out_options *opts)
Nice catch!
Acked-by: Paolo Abeni <pabeni@redhat.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH mptcp-net 1/2] mptcp: mptcp_parse_option is no longer exported
2022-01-22 9:31 ` [PATCH mptcp-net 1/2] mptcp: mptcp_parse_option is no longer exported Matthieu Baerts
2022-01-24 10:59 ` Paolo Abeni
@ 2022-01-25 1:21 ` Mat Martineau
1 sibling, 0 replies; 10+ messages in thread
From: Mat Martineau @ 2022-01-25 1:21 UTC (permalink / raw)
To: Matthieu Baerts; +Cc: mptcp
On Sat, 22 Jan 2022, Matthieu Baerts wrote:
> Options parsing in now done from mptcp_incoming_options().
>
> mptcp_parse_option() has been removed from mptcp.h when CONFIG_MPTCP is
> defined but not when it is not.
>
> Fixes: cfde141ea3fa ("mptcp: move option parsing into mptcp_incoming_options()")
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
> include/net/mptcp.h | 6 ------
> 1 file changed, 6 deletions(-)
>
Looks good to me, but it seems more like a net-next cleanup patch. When
building without CONFIG_MPTCP I don't see mptcp_parse_option showing up as
a symbol in the files that include mptcp.h.
Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 04f53352a1c9..8b1afd6f5cc4 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -218,12 +218,6 @@ static inline bool rsk_drop_req(const struct request_sock *req)
> return false;
> }
>
> -static inline void mptcp_parse_option(const struct sk_buff *skb,
> - const unsigned char *ptr, int opsize,
> - struct tcp_options_received *opt_rx)
> -{
> -}
> -
> static inline bool mptcp_syn_options(struct sock *sk, const struct sk_buff *skb,
> unsigned int *size,
> struct mptcp_out_options *opts)
> --
> 2.33.1
>
>
>
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH mptcp-net 2/2] selftests: mptcp: add missing join check
2022-01-24 9:39 ` Matthieu Baerts
@ 2022-01-25 3:24 ` Geliang Tang
2022-01-26 1:26 ` Mat Martineau
1 sibling, 0 replies; 10+ messages in thread
From: Geliang Tang @ 2022-01-25 3:24 UTC (permalink / raw)
To: Matthieu Baerts; +Cc: MPTCP Upstream, Paolo Abeni
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH mptcp-net 2/2] selftests: mptcp: add missing join check
2022-01-24 9:39 ` Matthieu Baerts
2022-01-25 3:24 ` Geliang Tang
@ 2022-01-26 1:26 ` Mat Martineau
2022-01-28 16:44 ` Matthieu Baerts
1 sibling, 1 reply; 10+ messages in thread
From: Mat Martineau @ 2022-01-26 1:26 UTC (permalink / raw)
To: Matthieu Baerts; +Cc: Geliang Tang, MPTCP Upstream, Paolo Abeni
[-- Attachment #1: Type: text/plain, Size: 3907 bytes --]
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH mptcp-net 2/2] selftests: mptcp: add missing join check
2022-01-26 1:26 ` Mat Martineau
@ 2022-01-28 16:44 ` Matthieu Baerts
0 siblings, 0 replies; 10+ messages in thread
From: Matthieu Baerts @ 2022-01-28 16:44 UTC (permalink / raw)
To: Mat Martineau; +Cc: Geliang Tang, MPTCP Upstream, Paolo Abeni
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-01-28 16:45 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-22 9:31 [PATCH mptcp-net 0/2] selftests: avoid confusions around 'signal invalid addresses' Matthieu Baerts
2022-01-22 9:31 ` [PATCH mptcp-net 1/2] mptcp: mptcp_parse_option is no longer exported Matthieu Baerts
2022-01-24 10:59 ` Paolo Abeni
2022-01-25 1:21 ` Mat Martineau
2022-01-22 9:31 ` [PATCH mptcp-net 2/2] selftests: mptcp: add missing join check Matthieu Baerts
2022-01-22 14:04 ` Geliang Tang
2022-01-24 9:39 ` Matthieu Baerts
2022-01-25 3:24 ` Geliang Tang
2022-01-26 1:26 ` Mat Martineau
2022-01-28 16:44 ` Matthieu Baerts
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.