Re: [MPTCP] [PATCH] mptcp: selftests: exercise GSO, retransmission and reorder code path

Re: [MPTCP] [PATCH] mptcp: selftests: exercise GSO, retransmission and reorder code path

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 1238 bytes --]

On Tue, 2019-05-21 at 11:08 +0200, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > This introduces a few artifacts on the virtual network topology
> > via the netem scheduler and disabling features on some veth, so
> > that we exercise the MTCP stack more.
> 
> I would suggest to not do this unconditionally.
> 
> Perhaps we can make it so that the tests are run twice,
> once as-is and once with tc+tso off?
> 
> This way we might still catch tso-related bugs, as feature path
> is slightly different.

I thought about that. Currently we have a set of very similar test -
e.g. ns1 -> ns3, ns1 -> ns4 and the opposite directions should be
basically the same, with a little higher latencies for ns1<->ns4
compared to ns1 <-> ns3

With the proposed patch we still have some tests with no artifact (ns1
-> ns2) but we effectively lose the forwarding scenario with no
artifact.

I'd like to avoid the excessive grow (in term of time) of these tests.

What if we remove a ns (ns4) and we do 2 iteration of the remaining,
one with no TC/with TSO and the other with such artifacts?

The total number of tests should not change and we would cover any
scenario.

WDYT?

thanks,

Paolo

Re: [MPTCP] [PATCH] mptcp: selftests: exercise GSO, retransmission and reorder code path

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 1177 bytes --]


On Tue, 21 May 2019, Matthieu Baerts wrote:

> On 21/05/2019 15:55, Paolo Abeni wrote:
>> On Tue, 2019-05-21 at 15:28 +0200, Matthieu Baerts wrote:
>>> On 21/05/2019 12:51, Florian Westphal wrote:
>>>> Thats another option, but I don't have a preference, your change is fine
>>>> as-is.
>>>
>>> Paolo, do you want to change something or can I apply this patch?
>>
>> If nobody opposes, I think it can go in as-is, thanks!
>
> Great! If someone opposes, we can easily revert/modify later!
>
> Just "squashed" into commit 40e8869bdd0f ("mptcp: selftests:
> switch to netns+veth based tests")
>
> - squash: 2707b30453da
> - signed-off: b23efa13d890
> - no conflict: /
> - export commit: fb5407ead32f
>

Thanks for the test updates, Paolo.

One thing to note: the packet reordering is only affecting TCP reassembly, 
and does not exercise reassembly at the MPTCP layer differently. The 
stream is still delivered to the MPTCP layer in-order.

MPTCP-level reassembly in the prototype implementation does not yet handle 
out-of-order mappings. That will get built in when updating 
mptcp_recvmsg() for multiple subflows.

--
Mat Martineau
Intel

Re: [MPTCP] [PATCH] mptcp: selftests: exercise GSO, retransmission and reorder code path

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]

On 21/05/2019 15:55, Paolo Abeni wrote:
> On Tue, 2019-05-21 at 15:28 +0200, Matthieu Baerts wrote:
>> On 21/05/2019 12:51, Florian Westphal wrote:
>>> Thats another option, but I don't have a preference, your change is fine
>>> as-is.
>>
>> Paolo, do you want to change something or can I apply this patch?
> 
> If nobody opposes, I think it can go in as-is, thanks!

Great! If someone opposes, we can easily revert/modify later!

Just "squashed" into commit 40e8869bdd0f ("mptcp: selftests:
 switch to netns+veth based tests")

- squash: 2707b30453da
- signed-off: b23efa13d890
- no conflict: /
- export commit: fb5407ead32f

Cheers,
Matt

> 
> Paolo
> 

-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

Re: [MPTCP] [PATCH] mptcp: selftests: exercise GSO, retransmission and reorder code path

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 339 bytes --]

On Tue, 2019-05-21 at 15:28 +0200, Matthieu Baerts wrote:
> On 21/05/2019 12:51, Florian Westphal wrote:
> > Thats another option, but I don't have a preference, your change is fine
> > as-is.
> 
> Paolo, do you want to change something or can I apply this patch?

If nobody opposes, I think it can go in as-is, thanks!

Paolo

Re: [MPTCP] [PATCH] mptcp: selftests: exercise GSO, retransmission and reorder code path

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 1986 bytes --]

Hi Paolo, Florian,

Thank you for the patch and the review!

That's nice nothing is broken after this!

On 21/05/2019 12:51, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
>> On Tue, 2019-05-21 at 11:08 +0200, Florian Westphal wrote:
>>> Paolo Abeni <pabeni(a)redhat.com> wrote:
>>>> This introduces a few artifacts on the virtual network topology
>>>> via the netem scheduler and disabling features on some veth, so
>>>> that we exercise the MTCP stack more.
>>>
>>> I would suggest to not do this unconditionally.
>>>
>>> Perhaps we can make it so that the tests are run twice,
>>> once as-is and once with tc+tso off?
>>>
>>> This way we might still catch tso-related bugs, as feature path
>>> is slightly different.
>>
>> I thought about that. Currently we have a set of very similar test -
>> e.g. ns1 -> ns3, ns1 -> ns4 and the opposite directions should be
>> basically the same, with a little higher latencies for ns1<->ns4
>> compared to ns1 <-> ns3
>>
>> With the proposed patch we still have some tests with no artifact (ns1
>> -> ns2) but we effectively lose the forwarding scenario with no
>> artifact.
>>
>> I'd like to avoid the excessive grow (in term of time) of these tests.
> 
> Ah, thats right, we still have it enabled on other netns.
> 
> OK, thats fine then.
> 
>> What if we remove a ns (ns4) and we do 2 iteration of the remaining,
>> one with no TC/with TSO and the other with such artifacts?
> 
> Thats another option, but I don't have a preference, your change is fine
> as-is.

Paolo, do you want to change something or can I apply this patch?

Cheers,
Matt

> _______________________________________________
> mptcp mailing list
> mptcp(a)lists.01.org
> https://lists.01.org/mailman/listinfo/mptcp
> 

-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

Re: [MPTCP] [PATCH] mptcp: selftests: exercise GSO, retransmission and reorder code path

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 1389 bytes --]

Paolo Abeni <pabeni(a)redhat.com> wrote:
> On Tue, 2019-05-21 at 11:08 +0200, Florian Westphal wrote:
> > Paolo Abeni <pabeni(a)redhat.com> wrote:
> > > This introduces a few artifacts on the virtual network topology
> > > via the netem scheduler and disabling features on some veth, so
> > > that we exercise the MTCP stack more.
> > 
> > I would suggest to not do this unconditionally.
> > 
> > Perhaps we can make it so that the tests are run twice,
> > once as-is and once with tc+tso off?
> > 
> > This way we might still catch tso-related bugs, as feature path
> > is slightly different.
> 
> I thought about that. Currently we have a set of very similar test -
> e.g. ns1 -> ns3, ns1 -> ns4 and the opposite directions should be
> basically the same, with a little higher latencies for ns1<->ns4
> compared to ns1 <-> ns3
> 
> With the proposed patch we still have some tests with no artifact (ns1
> -> ns2) but we effectively lose the forwarding scenario with no
> artifact.
> 
> I'd like to avoid the excessive grow (in term of time) of these tests.

Ah, thats right, we still have it enabled on other netns.

OK, thats fine then.

> What if we remove a ns (ns4) and we do 2 iteration of the remaining,
> one with no TC/with TSO and the other with such artifacts?

Thats another option, but I don't have a preference, your change is fine
as-is.

Re: [MPTCP] [PATCH] mptcp: selftests: exercise GSO, retransmission and reorder code path

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 490 bytes --]

Paolo Abeni <pabeni(a)redhat.com> wrote:
> This introduces a few artifacts on the virtual network topology
> via the netem scheduler and disabling features on some veth, so
> that we exercise the MTCP stack more.

Hi Paolo

I would suggest to not do this unconditionally.

Perhaps we can make it so that the tests are run twice,
once as-is and once with tc+tso off?

This way we might still catch tso-related bugs, as feature path
is slightly different.

What do you think?

[MPTCP] [PATCH] mptcp: selftests: exercise GSO, retransmission and reorder code path

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 2205 bytes --]

This introduces a few artifacts on the virtual network topology
via the netem scheduler and disabling features on some veth, so
that we exercise the MTCP stack more.

Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
notes:
 - to my surprise, self-tests are still succesfull here, even if
   some of them take considerably longer
 - this can be squashed into commit 40e8869bdd0f ("mptcp: selftests:
 switch to netns+veth based tests")
---
 tools/testing/selftests/net/mptcp/mptcp_connect.sh | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index ee478e94d8a2..1ce5c85577fb 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -45,8 +45,10 @@ for i in 1 2 3 4;do
 	ip -net ns$i link set lo up
 done
 
-#  ns1            ns2                 ns3             ns4
-# ns1eth2    ns2eth1 ns2eth3  ns3eth2   ns3eth4   ns4eth3
+#  ns1              ns2                    ns3                     ns4
+# ns1eth2    ns2eth1   ns2eth3      ns3eth2   ns3eth4       ns4eth3
+#                           - drop 1% ->            reorder 25%
+#                           <- TSO off -
 
 ip link add ns1eth2 netns ns1 type veth peer name ns2eth1 netns ns2
 ip link add ns2eth3 netns ns2 type veth peer name ns3eth2 netns ns3
@@ -62,6 +64,7 @@ ip -net ns2 link set ns2eth1 up
 ip -net ns2 addr add 10.0.2.1/24 dev ns2eth3
 ip -net ns2 link set ns2eth3 up
 ip -net ns2 route add default via 10.0.2.2
+tc -net ns2 qdisc add dev ns2eth3 root netem loss random 1
 ip netns exec ns2 sysctl -q net.ipv4.ip_forward=1
 
 ip -net ns3 addr add 10.0.2.2/24 dev ns3eth2
@@ -70,6 +73,8 @@ ip -net ns3 link set ns3eth2 up
 ip -net ns3 addr add 10.0.3.2/24 dev ns3eth4
 ip -net ns3 link set ns3eth4 up
 ip -net ns3 route add default via 10.0.2.1
+tc -net ns3 qdisc add dev ns3eth4 root netem delay 10ms reorder 25% 50% gap 5
+ip netns exec ns3 ethtook -K ns3eth2 tso off
 ip netns exec ns3 sysctl -q net.ipv4.ip_forward=1
 
 ip -net ns4 addr add 10.0.3.1/24 dev ns4eth3
-- 
2.20.1