From: Yonglong Li <liyonglong@chinatelecom.cn>
To: Geliang Tang <geliangtang@gmail.com>
Cc: mptcp@lists.linux.dev
Subject: Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT"
Date: Wed, 14 Jul 2021 17:49:13 +0800 [thread overview]
Message-ID: <f7ef8d86-d644-89c5-6153-f9fb4f31b897@chinatelecom.cn> (raw)
In-Reply-To: <CA+WQbwtK0expftc6LPrLY701iippBZ3f7EMoTKR=WTu5p1qxjg@mail.gmail.com>
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
next prev parent reply other threads:[~2021-07-14 9:49 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-13 6:44 [MPTCP][PATCH v2 mptcp-next] Squash to "mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other" Geliang Tang
2021-07-13 6:44 ` [MPTCP][PATCH v2 mptcp-next] Squash to "mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal" Geliang Tang
2021-07-13 7:32 ` Geliang Tang
2021-07-13 20:39 ` Mat Martineau
2021-07-15 3:17 ` Geliang Tang
2021-07-13 10:30 ` Yonglong Li
2021-07-13 6:44 ` [MPTCP][PATCH mptcp-next] Squash to "mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT" Geliang Tang
2021-07-13 10:30 ` Yonglong Li
2021-07-14 3:10 ` Geliang Tang
2021-07-14 9:49 ` Yonglong Li [this message]
2021-07-15 3:45 ` Geliang Tang
2021-07-15 6:13 ` Yonglong Li
2021-07-13 7:30 ` [MPTCP][PATCH v2 mptcp-next] Squash to "mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other" Geliang Tang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f7ef8d86-d644-89c5-6153-f9fb4f31b897@chinatelecom.cn \
--to=liyonglong@chinatelecom.cn \
--cc=geliangtang@gmail.com \
--cc=mptcp@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).