[PATCH mptcp-next] Squash to "selftests: mptcp: more stable join tests-cases"

Geliang Tang posted 1 patch 2 years, 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/613b986defe37faa7298095b364d9c81d6899664.1641549011.git.geliang.tang@suse.com
Maintainers: Matthieu Baerts <matthieu.baerts@tessares.net>, "David S. Miller" <davem@davemloft.net>, Mat Martineau <mathew.j.martineau@linux.intel.com>, Jakub Kicinski <kuba@kernel.org>, Shuah Khan <shuah@kernel.org>
tools/testing/selftests/net/mptcp/mptcp_join.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[PATCH mptcp-next] Squash to "selftests: mptcp: more stable join tests-cases"
Posted by Geliang Tang 2 years, 3 months ago
IPv6 removing test failed on my test every time, this patch fixed it.

12 remove subflow and signal IPv6       syn[ ok ] - synack[fail] got 1 JOIN[s] synack expected 2
 - ack[fail] got 1 JOIN[s] ack expected 2
Server ns stats
TcpPassiveOpens                 2                  0.0
TcpAttemptFails                 1                  0.0
TcpInSegs                       54                 0.0
TcpOutSegs                      58                 0.0
TcpRetransSegs                  2                  0.0
TcpExtEmbryonicRsts             1                  0.0
TcpExtDelayedACKs               15                 0.0
TcpExtTCPPureAcks               23                 0.0
TcpExtTCPTimeouts               1                  0.0
TcpExtTCPSynRetrans             2                  0.0
TcpExtTCPOrigDataSent           24                 0.0
TcpExtTCPDelivered              24                 0.0
MPTcpExtMPCapableSYNRX          1                  0.0
MPTcpExtMPCapableACKRX          1                  0.0
MPTcpExtMPJoinSynRx             2                  0.0
MPTcpExtMPJoinAckRx             1                  0.0
MPTcpExtEchoAdd                 1                  0.0
MPTcpExtRmSubflow               1                  0.0
Client ns stats
TcpActiveOpens                  3                  0.0
TcpInSegs                       58                 0.0
TcpOutSegs                      53                 0.0
TcpRetransSegs                  1                  0.0
TcpOutRsts                      3                  0.0
TcpExtDelayedACKs               3                  0.0
TcpExtTCPPureAcks               29                 0.0
TcpExtTCPTimeouts               1                  0.0
TcpExtTCPSynRetrans             1                  0.0
TcpExtTCPOrigDataSent           24                 0.0
TcpExtTCPDelivered              26                 0.0
TcpExtTcpTimeoutRehash          1                  0.0
MPTcpExtMPCapableSYNTX          1                  0.0
MPTcpExtMPCapableSYNACKRX       1                  0.0
MPTcpExtMPJoinSynAckRx          1                  0.0
MPTcpExtAddAddr                 1                  0.0
MPTcpExtRmAddr                  1                  0.0
MPTcpExtRmSubflow               1                  0.0
                                        add[ ok ] - echo  [ ok ]
                                        rm [fail] got 0 RM_ADDR[s] expected 1
 - sf    [ ok ]
Server ns stats
TcpPassiveOpens                 2                  0.0
TcpAttemptFails                 1                  0.0
TcpInSegs                       54                 0.0
TcpOutSegs                      58                 0.0
TcpRetransSegs                  2                  0.0
TcpExtEmbryonicRsts             1                  0.0

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index e48ce23d2386..41a2510e2f97 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -393,8 +393,8 @@ do_transfer()
 				do
 					id=${dump[$pos]}
 					rm_addr=$(rm_addr_count ${connector_ns})
-					ip netns exec ${listener_ns} ./pm_nl_ctl del $id
 					wait_rm_addr ${connector_ns} ${rm_addr}
+					ip netns exec ${listener_ns} ./pm_nl_ctl del $id
 					let counter+=1
 					let pos+=5
 				done
@@ -442,8 +442,8 @@ do_transfer()
 					# rm_addr are serialized, allow the previous one to complete
 					id=${dump[$pos]}
 					rm_addr=$(rm_addr_count ${listener_ns})
-					ip netns exec ${connector_ns} ./pm_nl_ctl del $id
 					wait_rm_addr ${listener_ns} ${rm_addr}
+					ip netns exec ${connector_ns} ./pm_nl_ctl del $id
 					let counter+=1
 					let pos+=5
 				done
-- 
2.31.1


Re: [PATCH mptcp-next] Squash to "selftests: mptcp: more stable join tests-cases"
Posted by Matthieu Baerts 2 years, 3 months ago
Hi Geliang,

On 07/01/2022 10:51, Geliang Tang wrote:
> IPv6 removing test failed on my test every time, this patch fixed it.

Thank you for looking at that!

The public CI has many similar errors and maybe (hard to say) more with
your patch, e.g.:

https://api.cirrus-ci.com/v1/artifact/task/5889173428109312/summary/summary.txt


> Our CI did some validations and here is its report:
>
> - KVM Validation: normal:
>   - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
>   - Task: https://cirrus-ci.com/task/5889173428109312
>   - Summary:
https://api.cirrus-ci.com/v1/artifact/task/5889173428109312/summary/summary.txt
>
> - KVM Validation: debug:
>   - Unstable: 2 failed test(s): selftest_diag selftest_mptcp_join 🔴:
>   - Task: https://cirrus-ci.com/task/5326223474688000
>   - Summary:
https://api.cirrus-ci.com/v1/artifact/task/5326223474688000/summary/summary.txt
>
> Initiator: Patchew Applier
> Commits:
https://github.com/multipath-tcp/mptcp_net-next/commits/3b5258004128

Could it be linked?

Should we eventually wait before and after having emitted the del
command with different counters?

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next] Squash to "selftests: mptcp: more stable join tests-cases"
Posted by Geliang Tang 2 years, 3 months ago
On Fri, Jan 07, 2022 at 12:03:00PM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 07/01/2022 10:51, Geliang Tang wrote:
> > IPv6 removing test failed on my test every time, this patch fixed it.
> 
> Thank you for looking at that!
> 
> The public CI has many similar errors and maybe (hard to say) more with
> your patch, e.g.:
> 
> https://api.cirrus-ci.com/v1/artifact/task/5889173428109312/summary/summary.txt
> 
> 
> > Our CI did some validations and here is its report:
> >
> > - KVM Validation: normal:
> >   - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
> >   - Task: https://cirrus-ci.com/task/5889173428109312
> >   - Summary:
> https://api.cirrus-ci.com/v1/artifact/task/5889173428109312/summary/summary.txt
> >
> > - KVM Validation: debug:
> >   - Unstable: 2 failed test(s): selftest_diag selftest_mptcp_join 🔴:
> >   - Task: https://cirrus-ci.com/task/5326223474688000
> >   - Summary:
> https://api.cirrus-ci.com/v1/artifact/task/5326223474688000/summary/summary.txt
> >
> > Initiator: Patchew Applier
> > Commits:
> https://github.com/multipath-tcp/mptcp_net-next/commits/3b5258004128
> 
> Could it be linked?

Hi Matt,

I tested this on my Thinkpad X1 laptop, only two failures occur. But
I think it's somehow linked with the commit "selftests: mptcp: more
stable join tests-cases", it dropped many latencies (sleep 1) in the
selftests.

The CI is runing in a KVM, right? I think we didn't tune the latencies
of the selftests for a KVM yet (At least for me). I think we need to login
into a KVM to run the selftests there and change the latencies there. I
think different latencies are needed. We need to make sure the selftests
could be passed on both a real machine and a KVM.

Could you tell me how to get the CI KVM image? Or could you do this
test?

Thanks
-Geliang

> 
> Should we eventually wait before and after having emitted the del
> command with different counters?
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
> 


Re: [PATCH mptcp-next] Squash to "selftests: mptcp: more stable join tests-cases"
Posted by Matthieu Baerts 2 years, 3 months ago
Hi Geliang,

On 10/01/2022 08:43, Geliang Tang wrote:
> On Fri, Jan 07, 2022 at 12:03:00PM +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 07/01/2022 10:51, Geliang Tang wrote:
>>> IPv6 removing test failed on my test every time, this patch fixed it.
>>
>> Thank you for looking at that!
>>
>> The public CI has many similar errors and maybe (hard to say) more with
>> your patch, e.g.:
>>
>> https://api.cirrus-ci.com/v1/artifact/task/5889173428109312/summary/summary.txt
>>
>>
>>> Our CI did some validations and here is its report:
>>>
>>> - KVM Validation: normal:
>>>   - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
>>>   - Task: https://cirrus-ci.com/task/5889173428109312
>>>   - Summary:
>> https://api.cirrus-ci.com/v1/artifact/task/5889173428109312/summary/summary.txt
>>>
>>> - KVM Validation: debug:
>>>   - Unstable: 2 failed test(s): selftest_diag selftest_mptcp_join 🔴:
>>>   - Task: https://cirrus-ci.com/task/5326223474688000
>>>   - Summary:
>> https://api.cirrus-ci.com/v1/artifact/task/5326223474688000/summary/summary.txt
>>>
>>> Initiator: Patchew Applier
>>> Commits:
>> https://github.com/multipath-tcp/mptcp_net-next/commits/3b5258004128
>>
>> Could it be linked?
> 
> Hi Matt,
> 
> I tested this on my Thinkpad X1 laptop, only two failures occur. But
> I think it's somehow linked with the commit "selftests: mptcp: more
> stable join tests-cases", it dropped many latencies (sleep 1) in the
> selftests.
> 
> The CI is runing in a KVM, right? I think we didn't tune the latencies
> of the selftests for a KVM yet (At least for me). I think we need to login
> into a KVM to run the selftests there and change the latencies there. I
> think different latencies are needed. We need to make sure the selftests
> could be passed on both a real machine and a KVM.
> 
> Could you tell me how to get the CI KVM image? Or could you do this
> test?

Sorry for the late reply.

To test, it is very easy, you just need to install a recent version of
docker before:

  cd <kernel source code>
  docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
    --pull always mptcp/mptcp-upstream-virtme-docker:latest \
    auto-all

This will do what the CI is doing: running all tests, once without and
once with a "debug" kernel config.

There are different arguments you can give instead of "auto-all": only
"normal" or "debug" kernel config, having a shell instead of running all
tests, etc.. Please see:

https://github.com/multipath-tcp/mptcp-upstream-virtme-docker#entrypoint-options

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next] Squash to "selftests: mptcp: more stable join tests-cases"
Posted by Paolo Abeni 2 years, 3 months ago
Hello,

On Fri, 2022-01-07 at 17:51 +0800, Geliang Tang wrote:
> IPv6 removing test failed on my test every time, this patch fixed it.
> 
> 12 remove subflow and signal IPv6       syn[ ok ] - synack[fail] got 1 JOIN[s] synack expected 2
>  - ack[fail] got 1 JOIN[s] ack expected 2
> Server ns stats
> TcpPassiveOpens                 2                  0.0
> TcpAttemptFails                 1                  0.0
> TcpInSegs                       54                 0.0
> TcpOutSegs                      58                 0.0
> TcpRetransSegs                  2                  0.0
> TcpExtEmbryonicRsts             1                  0.0
> TcpExtDelayedACKs               15                 0.0
> TcpExtTCPPureAcks               23                 0.0
> TcpExtTCPTimeouts               1                  0.0
> TcpExtTCPSynRetrans             2                  0.0
> TcpExtTCPOrigDataSent           24                 0.0
> TcpExtTCPDelivered              24                 0.0
> MPTcpExtMPCapableSYNRX          1                  0.0
> MPTcpExtMPCapableACKRX          1                  0.0
> MPTcpExtMPJoinSynRx             2                  0.0
> MPTcpExtMPJoinAckRx             1                  0.0
> MPTcpExtEchoAdd                 1                  0.0
> MPTcpExtRmSubflow               1                  0.0
> Client ns stats
> TcpActiveOpens                  3                  0.0
> TcpInSegs                       58                 0.0
> TcpOutSegs                      53                 0.0
> TcpRetransSegs                  1                  0.0
> TcpOutRsts                      3                  0.0
> TcpExtDelayedACKs               3                  0.0
> TcpExtTCPPureAcks               29                 0.0
> TcpExtTCPTimeouts               1                  0.0
> TcpExtTCPSynRetrans             1                  0.0
> TcpExtTCPOrigDataSent           24                 0.0
> TcpExtTCPDelivered              26                 0.0
> TcpExtTcpTimeoutRehash          1                  0.0
> MPTcpExtMPCapableSYNTX          1                  0.0
> MPTcpExtMPCapableSYNACKRX       1                  0.0
> MPTcpExtMPJoinSynAckRx          1                  0.0
> MPTcpExtAddAddr                 1                  0.0
> MPTcpExtRmAddr                  1                  0.0
> MPTcpExtRmSubflow               1                  0.0
>                                         add[ ok ] - echo  [ ok ]
>                                         rm [fail] got 0 RM_ADDR[s] expected 1
>  - sf    [ ok ]
> Server ns stats
> TcpPassiveOpens                 2                  0.0
> TcpAttemptFails                 1                  0.0
> TcpInSegs                       54                 0.0
> TcpOutSegs                      58                 0.0
> TcpRetransSegs                  2                  0.0
> TcpExtEmbryonicRsts             1                  0.0
> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index e48ce23d2386..41a2510e2f97 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -393,8 +393,8 @@ do_transfer()
>  				do
>  					id=${dump[$pos]}
>  					rm_addr=$(rm_addr_count ${connector_ns})
> -					ip netns exec ${listener_ns} ./pm_nl_ctl del $id
>  					wait_rm_addr ${connector_ns} ${rm_addr}
> +					ip netns exec ${listener_ns} ./pm_nl_ctl del $id
>  					let counter+=1
>  					let pos+=5

Uhmm.... this is equivalent to replacing 'wait_rm_addr()' with 'sleep
1'. It defeats the purpouse of wait_rm_addr itself: reducing the delay
required for 'del address' related operation. Additionally, an
unconditional 'sleep 1' could impact negatively in other test-cases.

Possibly some alike:

---
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index e48ce23d2386..b209a303d1fe 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -366,7 +366,7 @@ do_transfer()
 
        # let the mptcp subflow be established in background before
        # do endpoint manipulation
-       [ $addr_nr_ns1 = "0" -a $addr_nr_ns2 = "0" ] || sleep 1
+       [ $addr_nr_ns1 = "0" -a $addr_nr_ns2 = "0" ] || sleep 2
 
        if [ $addr_nr_ns1 -gt 0 ]; then
                let add_nr_ns1=addr_nr_ns1
---

could be more appropriate, but 2secs to complete MPC + 2 MPJ HS looks
really too much...

Can you please capture a pcap trace with the failing test?

Thanks!

Paolo


Re: [PATCH mptcp-next] Squash to "selftests: mptcp: more stable join tests-cases"
Posted by Geliang Tang 2 years, 3 months ago
Hi Paolo,

On Mon, Jan 10, 2022 at 12:11:38PM +0100, Paolo Abeni wrote:
> Hello,
> 
> On Fri, 2022-01-07 at 17:51 +0800, Geliang Tang wrote:
> > IPv6 removing test failed on my test every time, this patch fixed it.
> > 
> > 12 remove subflow and signal IPv6       syn[ ok ] - synack[fail] got 1 JOIN[s] synack expected 2
> >  - ack[fail] got 1 JOIN[s] ack expected 2
> > Server ns stats
> > TcpPassiveOpens                 2                  0.0
> > TcpAttemptFails                 1                  0.0
> > TcpInSegs                       54                 0.0
> > TcpOutSegs                      58                 0.0
> > TcpRetransSegs                  2                  0.0
> > TcpExtEmbryonicRsts             1                  0.0
> > TcpExtDelayedACKs               15                 0.0
> > TcpExtTCPPureAcks               23                 0.0
> > TcpExtTCPTimeouts               1                  0.0
> > TcpExtTCPSynRetrans             2                  0.0
> > TcpExtTCPOrigDataSent           24                 0.0
> > TcpExtTCPDelivered              24                 0.0
> > MPTcpExtMPCapableSYNRX          1                  0.0
> > MPTcpExtMPCapableACKRX          1                  0.0
> > MPTcpExtMPJoinSynRx             2                  0.0
> > MPTcpExtMPJoinAckRx             1                  0.0
> > MPTcpExtEchoAdd                 1                  0.0
> > MPTcpExtRmSubflow               1                  0.0
> > Client ns stats
> > TcpActiveOpens                  3                  0.0
> > TcpInSegs                       58                 0.0
> > TcpOutSegs                      53                 0.0
> > TcpRetransSegs                  1                  0.0
> > TcpOutRsts                      3                  0.0
> > TcpExtDelayedACKs               3                  0.0
> > TcpExtTCPPureAcks               29                 0.0
> > TcpExtTCPTimeouts               1                  0.0
> > TcpExtTCPSynRetrans             1                  0.0
> > TcpExtTCPOrigDataSent           24                 0.0
> > TcpExtTCPDelivered              26                 0.0
> > TcpExtTcpTimeoutRehash          1                  0.0
> > MPTcpExtMPCapableSYNTX          1                  0.0
> > MPTcpExtMPCapableSYNACKRX       1                  0.0
> > MPTcpExtMPJoinSynAckRx          1                  0.0
> > MPTcpExtAddAddr                 1                  0.0
> > MPTcpExtRmAddr                  1                  0.0
> > MPTcpExtRmSubflow               1                  0.0
> >                                         add[ ok ] - echo  [ ok ]
> >                                         rm [fail] got 0 RM_ADDR[s] expected 1
> >  - sf    [ ok ]
> > Server ns stats
> > TcpPassiveOpens                 2                  0.0
> > TcpAttemptFails                 1                  0.0
> > TcpInSegs                       54                 0.0
> > TcpOutSegs                      58                 0.0
> > TcpRetransSegs                  2                  0.0
> > TcpExtEmbryonicRsts             1                  0.0
> > 
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > ---
> >  tools/testing/selftests/net/mptcp/mptcp_join.sh | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > index e48ce23d2386..41a2510e2f97 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > @@ -393,8 +393,8 @@ do_transfer()
> >  				do
> >  					id=${dump[$pos]}
> >  					rm_addr=$(rm_addr_count ${connector_ns})
> > -					ip netns exec ${listener_ns} ./pm_nl_ctl del $id
> >  					wait_rm_addr ${connector_ns} ${rm_addr}
> > +					ip netns exec ${listener_ns} ./pm_nl_ctl del $id
> >  					let counter+=1
> >  					let pos+=5
> 
> Uhmm.... this is equivalent to replacing 'wait_rm_addr()' with 'sleep
> 1'. It defeats the purpouse of wait_rm_addr itself: reducing the delay
> required for 'del address' related operation. Additionally, an
> unconditional 'sleep 1' could impact negatively in other test-cases.
> 
> Possibly some alike:
> 
> ---
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index e48ce23d2386..b209a303d1fe 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -366,7 +366,7 @@ do_transfer()
>  
>         # let the mptcp subflow be established in background before
>         # do endpoint manipulation
> -       [ $addr_nr_ns1 = "0" -a $addr_nr_ns2 = "0" ] || sleep 1
> +       [ $addr_nr_ns1 = "0" -a $addr_nr_ns2 = "0" ] || sleep 2
>  
>         if [ $addr_nr_ns1 -gt 0 ]; then
>                 let add_nr_ns1=addr_nr_ns1
> ---

This patch doesn't work on my test either. I changed it to "sleep 2",
the "remove subflow and signal IPv6" test still failed.

> 
> could be more appropriate, but 2secs to complete MPC + 2 MPJ HS looks
> really too much...
> 
> Can you please capture a pcap trace with the failing test?

The pcap is attached.

Thanks,
-Geliang

> 
> Thanks!
> 
> Paolo
> 
Re: [PATCH mptcp-next] Squash to "selftests: mptcp: more stable join tests-cases"
Posted by Paolo Abeni 2 years, 3 months ago
On Tue, 2022-01-11 at 11:45 +0800, Geliang Tang wrote:
> Hi Paolo,
> 
> On Mon, Jan 10, 2022 at 12:11:38PM +0100, Paolo Abeni wrote:
> > Hello,
> > 
> > On Fri, 2022-01-07 at 17:51 +0800, Geliang Tang wrote:
> > > IPv6 removing test failed on my test every time, this patch fixed it.
> > > 
> > > 12 remove subflow and signal IPv6       syn[ ok ] - synack[fail] got 1 JOIN[s] synack expected 2
> > >  - ack[fail] got 1 JOIN[s] ack expected 2
> > > Server ns stats
> > > TcpPassiveOpens                 2                  0.0
> > > TcpAttemptFails                 1                  0.0
> > > TcpInSegs                       54                 0.0
> > > TcpOutSegs                      58                 0.0
> > > TcpRetransSegs                  2                  0.0
> > > TcpExtEmbryonicRsts             1                  0.0
> > > TcpExtDelayedACKs               15                 0.0
> > > TcpExtTCPPureAcks               23                 0.0
> > > TcpExtTCPTimeouts               1                  0.0
> > > TcpExtTCPSynRetrans             2                  0.0
> > > TcpExtTCPOrigDataSent           24                 0.0
> > > TcpExtTCPDelivered              24                 0.0
> > > MPTcpExtMPCapableSYNRX          1                  0.0
> > > MPTcpExtMPCapableACKRX          1                  0.0
> > > MPTcpExtMPJoinSynRx             2                  0.0
> > > MPTcpExtMPJoinAckRx             1                  0.0
> > > MPTcpExtEchoAdd                 1                  0.0
> > > MPTcpExtRmSubflow               1                  0.0
> > > Client ns stats
> > > TcpActiveOpens                  3                  0.0
> > > TcpInSegs                       58                 0.0
> > > TcpOutSegs                      53                 0.0
> > > TcpRetransSegs                  1                  0.0
> > > TcpOutRsts                      3                  0.0
> > > TcpExtDelayedACKs               3                  0.0
> > > TcpExtTCPPureAcks               29                 0.0
> > > TcpExtTCPTimeouts               1                  0.0
> > > TcpExtTCPSynRetrans             1                  0.0
> > > TcpExtTCPOrigDataSent           24                 0.0
> > > TcpExtTCPDelivered              26                 0.0
> > > TcpExtTcpTimeoutRehash          1                  0.0
> > > MPTcpExtMPCapableSYNTX          1                  0.0
> > > MPTcpExtMPCapableSYNACKRX       1                  0.0
> > > MPTcpExtMPJoinSynAckRx          1                  0.0
> > > MPTcpExtAddAddr                 1                  0.0
> > > MPTcpExtRmAddr                  1                  0.0
> > > MPTcpExtRmSubflow               1                  0.0
> > >                                         add[ ok ] - echo  [ ok ]
> > >                                         rm [fail] got 0 RM_ADDR[s] expected 1
> > >  - sf    [ ok ]
> > > Server ns stats
> > > TcpPassiveOpens                 2                  0.0
> > > TcpAttemptFails                 1                  0.0
> > > TcpInSegs                       54                 0.0
> > > TcpOutSegs                      58                 0.0
> > > TcpRetransSegs                  2                  0.0
> > > TcpExtEmbryonicRsts             1                  0.0
> > > 
> > > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > > ---
> > >  tools/testing/selftests/net/mptcp/mptcp_join.sh | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > index e48ce23d2386..41a2510e2f97 100755
> > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > @@ -393,8 +393,8 @@ do_transfer()
> > >  				do
> > >  					id=${dump[$pos]}
> > >  					rm_addr=$(rm_addr_count ${connector_ns})
> > > -					ip netns exec ${listener_ns} ./pm_nl_ctl del $id
> > >  					wait_rm_addr ${connector_ns} ${rm_addr}
> > > +					ip netns exec ${listener_ns} ./pm_nl_ctl del $id
> > >  					let counter+=1
> > >  					let pos+=5
> > 
> > Uhmm.... this is equivalent to replacing 'wait_rm_addr()' with 'sleep
> > 1'. It defeats the purpouse of wait_rm_addr itself: reducing the delay
> > required for 'del address' related operation. Additionally, an
> > unconditional 'sleep 1' could impact negatively in other test-cases.
> > 
> > Possibly some alike:
> > 
> > ---
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > index e48ce23d2386..b209a303d1fe 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > @@ -366,7 +366,7 @@ do_transfer()
> >  
> >         # let the mptcp subflow be established in background before
> >         # do endpoint manipulation
> > -       [ $addr_nr_ns1 = "0" -a $addr_nr_ns2 = "0" ] || sleep 1
> > +       [ $addr_nr_ns1 = "0" -a $addr_nr_ns2 = "0" ] || sleep 2
> >  
> >         if [ $addr_nr_ns1 -gt 0 ]; then
> >                 let add_nr_ns1=addr_nr_ns1
> > ---
> 
> This patch doesn't work on my test either. I changed it to "sleep 2",
> the "remove subflow and signal IPv6" test still failed.
> 
> > 
> > could be more appropriate, but 2secs to complete MPC + 2 MPJ HS looks
> > really too much...
> > 
> > Can you please capture a pcap trace with the failing test?
> 
> The pcap is attached.

Thank you! 

I'm really fail to understand why that happens, but the server replies
to the subflow endpoint MPJ syn with more than 1s delay, and that
causes the failure.

The interesting part is that the ipv6 routing configuration is actually
broken, as we lack an ipv6 route for the server on each link. The
following addresses the issue here, but I don't see how things were
somewhat working so far...

---
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index e48ce23d2386..cdfb7da47384 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -76,6 +76,7 @@ init()
 
 		# let $ns2 reach any $ns1 address from any interface
 		ip -net "$ns2" route add default via 10.0.$i.1 dev ns2eth$i metric 10$i
+		ip -net "$ns2" route add default via dead:beef:$i::1 dev ns2eth$i metric 10$i
 	done
 }
 
@@ -1545,7 +1546,7 @@ ipv6_tests()
 	ip netns exec $ns1 ./pm_nl_ctl limits 0 2
 	ip netns exec $ns1 ./pm_nl_ctl add dead:beef:2::1 flags signal
 	ip netns exec $ns2 ./pm_nl_ctl limits 1 2
-	ip netns exec $ns2 ./pm_nl_ctl add dead:beef:3::2 flags subflow
+	ip netns exec $ns2 ./pm_nl_ctl add dead:beef:3::2 dev ns2eth3 flags subflow
 	run_tests $ns1 $ns2 dead:beef:1::1 0 -1 -1 slow
 	chk_join_nr "remove subflow and signal IPv6" 2 2 2
 	chk_add_nr 1 1
---
/P


Re: [PATCH mptcp-next] Squash to "selftests: mptcp: more stable join tests-cases"
Posted by Geliang Tang 2 years, 3 months ago
On Tue, Jan 11, 2022 at 01:02:15PM +0100, Paolo Abeni wrote:
> On Tue, 2022-01-11 at 11:45 +0800, Geliang Tang wrote:
> > Hi Paolo,
> > 
> > On Mon, Jan 10, 2022 at 12:11:38PM +0100, Paolo Abeni wrote:
> > > Hello,
> > > 
> > > On Fri, 2022-01-07 at 17:51 +0800, Geliang Tang wrote:
> > > > IPv6 removing test failed on my test every time, this patch fixed it.
> > > > 
> > > > 12 remove subflow and signal IPv6       syn[ ok ] - synack[fail] got 1 JOIN[s] synack expected 2
> > > >  - ack[fail] got 1 JOIN[s] ack expected 2
> > > > Server ns stats
> > > > TcpPassiveOpens                 2                  0.0
> > > > TcpAttemptFails                 1                  0.0
> > > > TcpInSegs                       54                 0.0
> > > > TcpOutSegs                      58                 0.0
> > > > TcpRetransSegs                  2                  0.0
> > > > TcpExtEmbryonicRsts             1                  0.0
> > > > TcpExtDelayedACKs               15                 0.0
> > > > TcpExtTCPPureAcks               23                 0.0
> > > > TcpExtTCPTimeouts               1                  0.0
> > > > TcpExtTCPSynRetrans             2                  0.0
> > > > TcpExtTCPOrigDataSent           24                 0.0
> > > > TcpExtTCPDelivered              24                 0.0
> > > > MPTcpExtMPCapableSYNRX          1                  0.0
> > > > MPTcpExtMPCapableACKRX          1                  0.0
> > > > MPTcpExtMPJoinSynRx             2                  0.0
> > > > MPTcpExtMPJoinAckRx             1                  0.0
> > > > MPTcpExtEchoAdd                 1                  0.0
> > > > MPTcpExtRmSubflow               1                  0.0
> > > > Client ns stats
> > > > TcpActiveOpens                  3                  0.0
> > > > TcpInSegs                       58                 0.0
> > > > TcpOutSegs                      53                 0.0
> > > > TcpRetransSegs                  1                  0.0
> > > > TcpOutRsts                      3                  0.0
> > > > TcpExtDelayedACKs               3                  0.0
> > > > TcpExtTCPPureAcks               29                 0.0
> > > > TcpExtTCPTimeouts               1                  0.0
> > > > TcpExtTCPSynRetrans             1                  0.0
> > > > TcpExtTCPOrigDataSent           24                 0.0
> > > > TcpExtTCPDelivered              26                 0.0
> > > > TcpExtTcpTimeoutRehash          1                  0.0
> > > > MPTcpExtMPCapableSYNTX          1                  0.0
> > > > MPTcpExtMPCapableSYNACKRX       1                  0.0
> > > > MPTcpExtMPJoinSynAckRx          1                  0.0
> > > > MPTcpExtAddAddr                 1                  0.0
> > > > MPTcpExtRmAddr                  1                  0.0
> > > > MPTcpExtRmSubflow               1                  0.0
> > > >                                         add[ ok ] - echo  [ ok ]
> > > >                                         rm [fail] got 0 RM_ADDR[s] expected 1
> > > >  - sf    [ ok ]
> > > > Server ns stats
> > > > TcpPassiveOpens                 2                  0.0
> > > > TcpAttemptFails                 1                  0.0
> > > > TcpInSegs                       54                 0.0
> > > > TcpOutSegs                      58                 0.0
> > > > TcpRetransSegs                  2                  0.0
> > > > TcpExtEmbryonicRsts             1                  0.0
> > > > 
> > > > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > > > ---
> > > >  tools/testing/selftests/net/mptcp/mptcp_join.sh | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > index e48ce23d2386..41a2510e2f97 100755
> > > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > @@ -393,8 +393,8 @@ do_transfer()
> > > >  				do
> > > >  					id=${dump[$pos]}
> > > >  					rm_addr=$(rm_addr_count ${connector_ns})
> > > > -					ip netns exec ${listener_ns} ./pm_nl_ctl del $id
> > > >  					wait_rm_addr ${connector_ns} ${rm_addr}
> > > > +					ip netns exec ${listener_ns} ./pm_nl_ctl del $id
> > > >  					let counter+=1
> > > >  					let pos+=5
> > > 
> > > Uhmm.... this is equivalent to replacing 'wait_rm_addr()' with 'sleep
> > > 1'. It defeats the purpouse of wait_rm_addr itself: reducing the delay
> > > required for 'del address' related operation. Additionally, an
> > > unconditional 'sleep 1' could impact negatively in other test-cases.
> > > 
> > > Possibly some alike:
> > > 
> > > ---
> > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > index e48ce23d2386..b209a303d1fe 100755
> > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > @@ -366,7 +366,7 @@ do_transfer()
> > >  
> > >         # let the mptcp subflow be established in background before
> > >         # do endpoint manipulation
> > > -       [ $addr_nr_ns1 = "0" -a $addr_nr_ns2 = "0" ] || sleep 1
> > > +       [ $addr_nr_ns1 = "0" -a $addr_nr_ns2 = "0" ] || sleep 2
> > >  
> > >         if [ $addr_nr_ns1 -gt 0 ]; then
> > >                 let add_nr_ns1=addr_nr_ns1
> > > ---
> > 
> > This patch doesn't work on my test either. I changed it to "sleep 2",
> > the "remove subflow and signal IPv6" test still failed.
> > 
> > > 
> > > could be more appropriate, but 2secs to complete MPC + 2 MPJ HS looks
> > > really too much...
> > > 
> > > Can you please capture a pcap trace with the failing test?
> > 
> > The pcap is attached.
> 
> Thank you! 
> 
> I'm really fail to understand why that happens, but the server replies
> to the subflow endpoint MPJ syn with more than 1s delay, and that
> causes the failure.
> 
> The interesting part is that the ipv6 routing configuration is actually
> broken, as we lack an ipv6 route for the server on each link. The
> following addresses the issue here, but I don't see how things were
> somewhat working so far...
> 
> ---
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index e48ce23d2386..cdfb7da47384 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -76,6 +76,7 @@ init()
>  
>  		# let $ns2 reach any $ns1 address from any interface
>  		ip -net "$ns2" route add default via 10.0.$i.1 dev ns2eth$i metric 10$i
> +		ip -net "$ns2" route add default via dead:beef:$i::1 dev ns2eth$i metric 10$i
>  	done
>  }
>  
> @@ -1545,7 +1546,7 @@ ipv6_tests()
>  	ip netns exec $ns1 ./pm_nl_ctl limits 0 2
>  	ip netns exec $ns1 ./pm_nl_ctl add dead:beef:2::1 flags signal
>  	ip netns exec $ns2 ./pm_nl_ctl limits 1 2
> -	ip netns exec $ns2 ./pm_nl_ctl add dead:beef:3::2 flags subflow
> +	ip netns exec $ns2 ./pm_nl_ctl add dead:beef:3::2 dev ns2eth3 flags subflow
>  	run_tests $ns1 $ns2 dead:beef:1::1 0 -1 -1 slow
>  	chk_join_nr "remove subflow and signal IPv6" 2 2 2
>  	chk_add_nr 1 1
> ---

Hi Paolo,

This patch works on my test. Should we change this "single subflow IPv6" test too?

---

@@ -1654,7 +1655,7 @@ ipv6_tests()
        reset
        pm_nl_set_limits $ns1 0 1
        pm_nl_set_limits $ns2 0 1
-       pm_nl_add_endpoint $ns2 dead:beef:3::2 flags subflow
+       pm_nl_add_endpoint $ns2 dead:beef:3::2 dev ns2eth3 flags subflow
        run_tests $ns1 $ns2 dead:beef:1::1 0 0 0 slow
        chk_join_nr "single subflow IPv6" 1 1 1

---

This "single subflow IPv6" test passes every time with or without this
change, but the "remove subflow and signal IPv6" test fails. What's the
difference between the two tests?

Thanks,
-Geliang

> /P
> 


Re: [PATCH mptcp-next] Squash to "selftests: mptcp: more stable join tests-cases"
Posted by Paolo Abeni 2 years, 3 months ago
On Thu, 2022-01-13 at 18:35 +0800, Geliang Tang wrote:
> On Tue, Jan 11, 2022 at 01:02:15PM +0100, Paolo Abeni wrote:
> > On Tue, 2022-01-11 at 11:45 +0800, Geliang Tang wrote:
> > > Hi Paolo,
> > > 
> > > On Mon, Jan 10, 2022 at 12:11:38PM +0100, Paolo Abeni wrote:
> > > > Hello,
> > > > 
> > > > On Fri, 2022-01-07 at 17:51 +0800, Geliang Tang wrote:
> > > > > IPv6 removing test failed on my test every time, this patch fixed it.
> > > > > 
> > > > > 12 remove subflow and signal IPv6       syn[ ok ] - synack[fail] got 1 JOIN[s] synack expected 2
> > > > >  - ack[fail] got 1 JOIN[s] ack expected 2
> > > > > Server ns stats
> > > > > TcpPassiveOpens                 2                  0.0
> > > > > TcpAttemptFails                 1                  0.0
> > > > > TcpInSegs                       54                 0.0
> > > > > TcpOutSegs                      58                 0.0
> > > > > TcpRetransSegs                  2                  0.0
> > > > > TcpExtEmbryonicRsts             1                  0.0
> > > > > TcpExtDelayedACKs               15                 0.0
> > > > > TcpExtTCPPureAcks               23                 0.0
> > > > > TcpExtTCPTimeouts               1                  0.0
> > > > > TcpExtTCPSynRetrans             2                  0.0
> > > > > TcpExtTCPOrigDataSent           24                 0.0
> > > > > TcpExtTCPDelivered              24                 0.0
> > > > > MPTcpExtMPCapableSYNRX          1                  0.0
> > > > > MPTcpExtMPCapableACKRX          1                  0.0
> > > > > MPTcpExtMPJoinSynRx             2                  0.0
> > > > > MPTcpExtMPJoinAckRx             1                  0.0
> > > > > MPTcpExtEchoAdd                 1                  0.0
> > > > > MPTcpExtRmSubflow               1                  0.0
> > > > > Client ns stats
> > > > > TcpActiveOpens                  3                  0.0
> > > > > TcpInSegs                       58                 0.0
> > > > > TcpOutSegs                      53                 0.0
> > > > > TcpRetransSegs                  1                  0.0
> > > > > TcpOutRsts                      3                  0.0
> > > > > TcpExtDelayedACKs               3                  0.0
> > > > > TcpExtTCPPureAcks               29                 0.0
> > > > > TcpExtTCPTimeouts               1                  0.0
> > > > > TcpExtTCPSynRetrans             1                  0.0
> > > > > TcpExtTCPOrigDataSent           24                 0.0
> > > > > TcpExtTCPDelivered              26                 0.0
> > > > > TcpExtTcpTimeoutRehash          1                  0.0
> > > > > MPTcpExtMPCapableSYNTX          1                  0.0
> > > > > MPTcpExtMPCapableSYNACKRX       1                  0.0
> > > > > MPTcpExtMPJoinSynAckRx          1                  0.0
> > > > > MPTcpExtAddAddr                 1                  0.0
> > > > > MPTcpExtRmAddr                  1                  0.0
> > > > > MPTcpExtRmSubflow               1                  0.0
> > > > >                                         add[ ok ] - echo  [ ok ]
> > > > >                                         rm [fail] got 0 RM_ADDR[s] expected 1
> > > > >  - sf    [ ok ]
> > > > > Server ns stats
> > > > > TcpPassiveOpens                 2                  0.0
> > > > > TcpAttemptFails                 1                  0.0
> > > > > TcpInSegs                       54                 0.0
> > > > > TcpOutSegs                      58                 0.0
> > > > > TcpRetransSegs                  2                  0.0
> > > > > TcpExtEmbryonicRsts             1                  0.0
> > > > > 
> > > > > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > > > > ---
> > > > >  tools/testing/selftests/net/mptcp/mptcp_join.sh | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > > index e48ce23d2386..41a2510e2f97 100755
> > > > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > > @@ -393,8 +393,8 @@ do_transfer()
> > > > >  				do
> > > > >  					id=${dump[$pos]}
> > > > >  					rm_addr=$(rm_addr_count ${connector_ns})
> > > > > -					ip netns exec ${listener_ns} ./pm_nl_ctl del $id
> > > > >  					wait_rm_addr ${connector_ns} ${rm_addr}
> > > > > +					ip netns exec ${listener_ns} ./pm_nl_ctl del $id
> > > > >  					let counter+=1
> > > > >  					let pos+=5
> > > > 
> > > > Uhmm.... this is equivalent to replacing 'wait_rm_addr()' with 'sleep
> > > > 1'. It defeats the purpouse of wait_rm_addr itself: reducing the delay
> > > > required for 'del address' related operation. Additionally, an
> > > > unconditional 'sleep 1' could impact negatively in other test-cases.
> > > > 
> > > > Possibly some alike:
> > > > 
> > > > ---
> > > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > index e48ce23d2386..b209a303d1fe 100755
> > > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > @@ -366,7 +366,7 @@ do_transfer()
> > > >  
> > > >         # let the mptcp subflow be established in background before
> > > >         # do endpoint manipulation
> > > > -       [ $addr_nr_ns1 = "0" -a $addr_nr_ns2 = "0" ] || sleep 1
> > > > +       [ $addr_nr_ns1 = "0" -a $addr_nr_ns2 = "0" ] || sleep 2
> > > >  
> > > >         if [ $addr_nr_ns1 -gt 0 ]; then
> > > >                 let add_nr_ns1=addr_nr_ns1
> > > > ---
> > > 
> > > This patch doesn't work on my test either. I changed it to "sleep 2",
> > > the "remove subflow and signal IPv6" test still failed.
> > > 
> > > > 
> > > > could be more appropriate, but 2secs to complete MPC + 2 MPJ HS looks
> > > > really too much...
> > > > 
> > > > Can you please capture a pcap trace with the failing test?
> > > 
> > > The pcap is attached.
> > 
> > Thank you! 
> > 
> > I'm really fail to understand why that happens, but the server replies
> > to the subflow endpoint MPJ syn with more than 1s delay, and that
> > causes the failure.
> > 
> > The interesting part is that the ipv6 routing configuration is actually
> > broken, as we lack an ipv6 route for the server on each link. The
> > following addresses the issue here, but I don't see how things were
> > somewhat working so far...
> > 
> > ---
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > index e48ce23d2386..cdfb7da47384 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > @@ -76,6 +76,7 @@ init()
> >  
> >  		# let $ns2 reach any $ns1 address from any interface
> >  		ip -net "$ns2" route add default via 10.0.$i.1 dev ns2eth$i metric 10$i
> > +		ip -net "$ns2" route add default via dead:beef:$i::1 dev ns2eth$i metric 10$i
> >  	done
> >  }
> >  
> > @@ -1545,7 +1546,7 @@ ipv6_tests()
> >  	ip netns exec $ns1 ./pm_nl_ctl limits 0 2
> >  	ip netns exec $ns1 ./pm_nl_ctl add dead:beef:2::1 flags signal
> >  	ip netns exec $ns2 ./pm_nl_ctl limits 1 2
> > -	ip netns exec $ns2 ./pm_nl_ctl add dead:beef:3::2 flags subflow
> > +	ip netns exec $ns2 ./pm_nl_ctl add dead:beef:3::2 dev ns2eth3 flags subflow
> >  	run_tests $ns1 $ns2 dead:beef:1::1 0 -1 -1 slow
> >  	chk_join_nr "remove subflow and signal IPv6" 2 2 2
> >  	chk_add_nr 1 1
> > ---
> 
> Hi Paolo,
> 
> This patch works on my test. Should we change this "single subflow IPv6" test too?

In theory, yes...
> 
> ---
> 
> @@ -1654,7 +1655,7 @@ ipv6_tests()
>         reset
>         pm_nl_set_limits $ns1 0 1
>         pm_nl_set_limits $ns2 0 1
> -       pm_nl_add_endpoint $ns2 dead:beef:3::2 flags subflow
> +       pm_nl_add_endpoint $ns2 dead:beef:3::2 dev ns2eth3 flags subflow
>         run_tests $ns1 $ns2 dead:beef:1::1 0 0 0 slow
>         chk_join_nr "single subflow IPv6" 1 1 1
> 
> ---
> 
> This "single subflow IPv6" test passes every time with or without this
> change, 

... in pratice, as you noted, it's not needed.

> but the "remove subflow and signal IPv6" test fails. What's the
> difference between the two tests?

I'm wild guessing, as I still fail to understand the full picture
completely. In the "single subflow IPv6" test, is irrelevant if the
additional subflow uses the "wrong" interface, because we just need to
establish it.

In the "remove subflow and signal IPv6" we have additional time
constrains due to multiple subflow creation and removal, and the wrong
routing setup causes the large delay I mentioned before, ence the
failure.

I'm very suprised the wrong routing setup casues a delay instead of an
unreachable error and that puzzle me much. 

Do you prefer sending a new version of the squash-to patch, or do you
prefer I'll go ahead with that?

Please, let me know,

/P


Re: [PATCH mptcp-next] Squash to "selftests: mptcp: more stable join tests-cases"
Posted by Matthieu Baerts 2 years, 3 months ago
Hello,

On 13/01/2022 11:52, Paolo Abeni wrote:
> Do you prefer sending a new version of the squash-to patch, or do you
> prefer I'll go ahead with that?

Small detail just to be sure: as you probably know the patch "selftests:
mptcp: more stable join tests-cases" has been accepted in net-next. So
we can no longer have a "squash-to" patch.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next] Squash to "selftests: mptcp: more stable join tests-cases"
Posted by Geliang Tang 2 years, 3 months ago
On Thu, Jan 13, 2022 at 11:52:20AM +0100, Paolo Abeni wrote:
> On Thu, 2022-01-13 at 18:35 +0800, Geliang Tang wrote:
> > On Tue, Jan 11, 2022 at 01:02:15PM +0100, Paolo Abeni wrote:
> > > On Tue, 2022-01-11 at 11:45 +0800, Geliang Tang wrote:
> > > > Hi Paolo,
> > > > 
> > > > On Mon, Jan 10, 2022 at 12:11:38PM +0100, Paolo Abeni wrote:
> > > > > Hello,
> > > > > 
> > > > > On Fri, 2022-01-07 at 17:51 +0800, Geliang Tang wrote:
> > > > > > IPv6 removing test failed on my test every time, this patch fixed it.
> > > > > > 
> > > > > > 12 remove subflow and signal IPv6       syn[ ok ] - synack[fail] got 1 JOIN[s] synack expected 2
> > > > > >  - ack[fail] got 1 JOIN[s] ack expected 2
> > > > > > Server ns stats
> > > > > > TcpPassiveOpens                 2                  0.0
> > > > > > TcpAttemptFails                 1                  0.0
> > > > > > TcpInSegs                       54                 0.0
> > > > > > TcpOutSegs                      58                 0.0
> > > > > > TcpRetransSegs                  2                  0.0
> > > > > > TcpExtEmbryonicRsts             1                  0.0
> > > > > > TcpExtDelayedACKs               15                 0.0
> > > > > > TcpExtTCPPureAcks               23                 0.0
> > > > > > TcpExtTCPTimeouts               1                  0.0
> > > > > > TcpExtTCPSynRetrans             2                  0.0
> > > > > > TcpExtTCPOrigDataSent           24                 0.0
> > > > > > TcpExtTCPDelivered              24                 0.0
> > > > > > MPTcpExtMPCapableSYNRX          1                  0.0
> > > > > > MPTcpExtMPCapableACKRX          1                  0.0
> > > > > > MPTcpExtMPJoinSynRx             2                  0.0
> > > > > > MPTcpExtMPJoinAckRx             1                  0.0
> > > > > > MPTcpExtEchoAdd                 1                  0.0
> > > > > > MPTcpExtRmSubflow               1                  0.0
> > > > > > Client ns stats
> > > > > > TcpActiveOpens                  3                  0.0
> > > > > > TcpInSegs                       58                 0.0
> > > > > > TcpOutSegs                      53                 0.0
> > > > > > TcpRetransSegs                  1                  0.0
> > > > > > TcpOutRsts                      3                  0.0
> > > > > > TcpExtDelayedACKs               3                  0.0
> > > > > > TcpExtTCPPureAcks               29                 0.0
> > > > > > TcpExtTCPTimeouts               1                  0.0
> > > > > > TcpExtTCPSynRetrans             1                  0.0
> > > > > > TcpExtTCPOrigDataSent           24                 0.0
> > > > > > TcpExtTCPDelivered              26                 0.0
> > > > > > TcpExtTcpTimeoutRehash          1                  0.0
> > > > > > MPTcpExtMPCapableSYNTX          1                  0.0
> > > > > > MPTcpExtMPCapableSYNACKRX       1                  0.0
> > > > > > MPTcpExtMPJoinSynAckRx          1                  0.0
> > > > > > MPTcpExtAddAddr                 1                  0.0
> > > > > > MPTcpExtRmAddr                  1                  0.0
> > > > > > MPTcpExtRmSubflow               1                  0.0
> > > > > >                                         add[ ok ] - echo  [ ok ]
> > > > > >                                         rm [fail] got 0 RM_ADDR[s] expected 1
> > > > > >  - sf    [ ok ]
> > > > > > Server ns stats
> > > > > > TcpPassiveOpens                 2                  0.0
> > > > > > TcpAttemptFails                 1                  0.0
> > > > > > TcpInSegs                       54                 0.0
> > > > > > TcpOutSegs                      58                 0.0
> > > > > > TcpRetransSegs                  2                  0.0
> > > > > > TcpExtEmbryonicRsts             1                  0.0
> > > > > > 
> > > > > > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > > > > > ---
> > > > > >  tools/testing/selftests/net/mptcp/mptcp_join.sh | 4 ++--
> > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > > > index e48ce23d2386..41a2510e2f97 100755
> > > > > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > > > @@ -393,8 +393,8 @@ do_transfer()
> > > > > >  				do
> > > > > >  					id=${dump[$pos]}
> > > > > >  					rm_addr=$(rm_addr_count ${connector_ns})
> > > > > > -					ip netns exec ${listener_ns} ./pm_nl_ctl del $id
> > > > > >  					wait_rm_addr ${connector_ns} ${rm_addr}
> > > > > > +					ip netns exec ${listener_ns} ./pm_nl_ctl del $id
> > > > > >  					let counter+=1
> > > > > >  					let pos+=5
> > > > > 
> > > > > Uhmm.... this is equivalent to replacing 'wait_rm_addr()' with 'sleep
> > > > > 1'. It defeats the purpouse of wait_rm_addr itself: reducing the delay
> > > > > required for 'del address' related operation. Additionally, an
> > > > > unconditional 'sleep 1' could impact negatively in other test-cases.
> > > > > 
> > > > > Possibly some alike:
> > > > > 
> > > > > ---
> > > > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > > b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > > index e48ce23d2386..b209a303d1fe 100755
> > > > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > > > @@ -366,7 +366,7 @@ do_transfer()
> > > > >  
> > > > >         # let the mptcp subflow be established in background before
> > > > >         # do endpoint manipulation
> > > > > -       [ $addr_nr_ns1 = "0" -a $addr_nr_ns2 = "0" ] || sleep 1
> > > > > +       [ $addr_nr_ns1 = "0" -a $addr_nr_ns2 = "0" ] || sleep 2
> > > > >  
> > > > >         if [ $addr_nr_ns1 -gt 0 ]; then
> > > > >                 let add_nr_ns1=addr_nr_ns1
> > > > > ---
> > > > 
> > > > This patch doesn't work on my test either. I changed it to "sleep 2",
> > > > the "remove subflow and signal IPv6" test still failed.
> > > > 
> > > > > 
> > > > > could be more appropriate, but 2secs to complete MPC + 2 MPJ HS looks
> > > > > really too much...
> > > > > 
> > > > > Can you please capture a pcap trace with the failing test?
> > > > 
> > > > The pcap is attached.
> > > 
> > > Thank you! 
> > > 
> > > I'm really fail to understand why that happens, but the server replies
> > > to the subflow endpoint MPJ syn with more than 1s delay, and that
> > > causes the failure.
> > > 
> > > The interesting part is that the ipv6 routing configuration is actually
> > > broken, as we lack an ipv6 route for the server on each link. The
> > > following addresses the issue here, but I don't see how things were
> > > somewhat working so far...
> > > 
> > > ---
> > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > index e48ce23d2386..cdfb7da47384 100755
> > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > @@ -76,6 +76,7 @@ init()
> > >  
> > >  		# let $ns2 reach any $ns1 address from any interface
> > >  		ip -net "$ns2" route add default via 10.0.$i.1 dev ns2eth$i metric 10$i
> > > +		ip -net "$ns2" route add default via dead:beef:$i::1 dev ns2eth$i metric 10$i
> > >  	done
> > >  }
> > >  
> > > @@ -1545,7 +1546,7 @@ ipv6_tests()
> > >  	ip netns exec $ns1 ./pm_nl_ctl limits 0 2
> > >  	ip netns exec $ns1 ./pm_nl_ctl add dead:beef:2::1 flags signal
> > >  	ip netns exec $ns2 ./pm_nl_ctl limits 1 2
> > > -	ip netns exec $ns2 ./pm_nl_ctl add dead:beef:3::2 flags subflow
> > > +	ip netns exec $ns2 ./pm_nl_ctl add dead:beef:3::2 dev ns2eth3 flags subflow
> > >  	run_tests $ns1 $ns2 dead:beef:1::1 0 -1 -1 slow
> > >  	chk_join_nr "remove subflow and signal IPv6" 2 2 2
> > >  	chk_add_nr 1 1
> > > ---
> > 
> > Hi Paolo,
> > 
> > This patch works on my test. Should we change this "single subflow IPv6" test too?
> 
> In theory, yes...
> > 
> > ---
> > 
> > @@ -1654,7 +1655,7 @@ ipv6_tests()
> >         reset
> >         pm_nl_set_limits $ns1 0 1
> >         pm_nl_set_limits $ns2 0 1
> > -       pm_nl_add_endpoint $ns2 dead:beef:3::2 flags subflow
> > +       pm_nl_add_endpoint $ns2 dead:beef:3::2 dev ns2eth3 flags subflow
> >         run_tests $ns1 $ns2 dead:beef:1::1 0 0 0 slow
> >         chk_join_nr "single subflow IPv6" 1 1 1
> > 
> > ---
> > 
> > This "single subflow IPv6" test passes every time with or without this
> > change, 
> 
> ... in pratice, as you noted, it's not needed.
> 
> > but the "remove subflow and signal IPv6" test fails. What's the
> > difference between the two tests?
> 
> I'm wild guessing, as I still fail to understand the full picture
> completely. In the "single subflow IPv6" test, is irrelevant if the
> additional subflow uses the "wrong" interface, because we just need to
> establish it.
> 
> In the "remove subflow and signal IPv6" we have additional time
> constrains due to multiple subflow creation and removal, and the wrong
> routing setup causes the large delay I mentioned before, ence the
> failure.
> 
> I'm very suprised the wrong routing setup casues a delay instead of an
> unreachable error and that puzzle me much. 
> 
> Do you prefer sending a new version of the squash-to patch, or do you
> prefer I'll go ahead with that?
> 
> Please, let me know,

Hi Paolo,

Please send the new version. I'll test it if needed.

Thanks,
-Geliang

> 
> /P
>