Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT"

[Yonglong Li <liyonglong@chinatelecom.cn>]

Hi Geliang,

On 2021/7/14 11:10, Geliang Tang wrote:
> Hi Yonglong,
> 
> Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月13日周二 下午6:30写道：
>>
>>
>>
>> On 2021/7/13 14:44, Geliang Tang wrote:
>>> Keep the debug info for "send ack".
>>>
>>> Don't drop mptcp_pm_should_add_signal_ipv6() and
>>> mptcp_pm_should_add_signal_port().
>>>
>>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>>> ---
>>>  net/mptcp/pm_netlink.c | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>> index 3e35720317ae..2cd6caaedb08 100644
>>> --- a/net/mptcp/pm_netlink.c
>>> +++ b/net/mptcp/pm_netlink.c
>>> @@ -543,8 +543,11 @@ void mptcp_pm_nl_addr_send_ack(struct mptcp_sock *msk)
>>>               bool slow;
>>>
>>>               spin_unlock_bh(&msk->pm.lock);
>>> -             pr_debug("send ack for %s",
>>> -                      mptcp_pm_should_add_signal(msk) ? "add_addr" : "rm_addr");
>>> +             pr_debug("send ack for %s%s%s",
>>> +                      mptcp_pm_should_add_signal_addr(msk) ? "add_addr" :
>>> +                      (mptcp_pm_should_add_signal_echo(msk) ? "add_echo" : "rm_addr"),
>>> +                      mptcp_pm_should_add_signal_ipv6(msk) ? " [ipv6]" : "",
>>> +                      mptcp_pm_should_add_signal_port(msk) ? " [port]" : "");
>>>
>>>               slow = lock_sock_fast(ssk);
>>>               tcp_send_ack(ssk);
>>>
>> Hi Geliang,
>>
>> I think the debug log will be incorrect if add_addr and add_echo events trigger at the same time.
>> WDYT?
> 
> Yes, how about moving this pr_debug line under the pm lock, just swap this
> pr_debug line with the spin_unlock_bh line?
> 
> If so, no need to use READ_ONCE() in mptcp_pm_should_add_signal_ipv6/port
> too.
I prefer to remove ipv6 check just like v8. if you want to debug we can get more detail info
from debug log in mptcp_established_options_add_addr.

> 
> 
> I had tested this squash-to patches all night yesterday. And I got this
> error in the debug log:
> 
> add_signal error, add_addr=2, echo=1
> 
> This means the race occurs, right?
> 
It seams like anther race case or bug? if more than one 'echo add addr' event be trigger at the same
time this log will be show.

> Does it mean that this version cannot deal with the race either? I'm not
> sure.
> 
> Could you please share your test scripts to me?
> 
I add a test case in mptcp_join.sh, and run 'mptcp_join.sh -X -c' in a loop, if test case failed the race maybe occurs.
and then analsys the pcap file to check the race.

the test case add in mptcp_join.sh:

diff --git a/mptcp_join.sh b/mptcp_join.sh
index 523c779..162a451 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1462,6 +1462,24 @@ checksum_tests()
        chk_csum_nr "checksum test 1 0"
 }

+xxoo()
+{
+       reset
+       ip netns exec $ns1 ./pm_nl_ctl limits 4 4
+       ip netns exec $ns2 ./pm_nl_ctl limits 4 4
+       ip netns exec $ns1 ./pm_nl_ctl add 10.0.1.1 flags signal
+       ip netns exec $ns1 ./pm_nl_ctl add 10.0.2.1 flags signal
+       ip netns exec $ns1 ./pm_nl_ctl add 10.0.3.1 flags signal
+       ip netns exec $ns1 ./pm_nl_ctl add 10.0.4.1 flags signal
+       ip netns exec $ns2 ./pm_nl_ctl add 10.0.1.2 flags signal
+       ip netns exec $ns2 ./pm_nl_ctl add 10.0.2.2 flags signal
+       ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 flags signal
+       ip netns exec $ns2 ./pm_nl_ctl add 10.0.4.2 flags signal
+       run_tests $ns1 $ns2 10.0.1.1
+       chk_add_nr 4 4
+}
+
+
 all_tests()
 {
        subflows_tests
@@ -1566,6 +1584,9 @@ while getopts 'fsltra64bpkchCS' opt; do
                S)
                        checksum_tests
                        ;;
+               X)
+                       xxoo
+                       ;;
                c)
                        ;;
                C)


> Thanks,
> -Geliang
> 
> 
> 
>>
>> --
>> Li YongLong
> 
> 

-- 
Li YongLong