| Message ID | 1645779233-19742-1-git-send-email-liyonglong@chinatelecom.cn (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 2b7de9309fe070e5f6032de3d1a522983e38c982 |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | mptcp: send ADD_ADDR echo before create subflows | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 40 lines checked |
| matttbe/KVM_Validation__normal | warning | Unstable: 2 failed test(s): packetdrill_mp_join packetdrill_sockopts |
| matttbe/KVM_Validation__debug | warning | Unstable: 2 failed test(s): packetdrill_mp_join packetdrill_sockopts |
Hi Yonglong, Thank you for your modifications, that's great! Our CI did some validations and here is its report: - KVM Validation: normal: - Unstable: 2 failed test(s): packetdrill_mp_join packetdrill_sockopts
I have have send corresponding pull-request to packdrill: https://github.com/multipath-tcp/packetdrill/pull/80 On 2/25/2022 4:53 PM, Yonglong Li wrote: > In some corner case, It may receive ADD_ADDR retransmit package > before created subflows done. > It's more reasonable to send ADD_ADDR echo before creating subflows. > > Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn> > --- > net/mptcp/pm_netlink.c | 18 +++++------------- > 1 file changed, 5 insertions(+), 13 deletions(-) > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c > index e3b0384..c1f4bef 100644 > --- a/net/mptcp/pm_netlink.c > +++ b/net/mptcp/pm_netlink.c > @@ -650,7 +650,6 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk) > unsigned int add_addr_accept_max; > struct mptcp_addr_info remote; > unsigned int subflows_max; > - bool reset_port = false; > int i, nr; > > add_addr_accept_max = mptcp_pm_get_add_addr_accept_max(msk); > @@ -661,14 +660,15 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk) > msk->pm.remote.family); > > remote = msk->pm.remote; > + mptcp_pm_announce_addr(msk, &remote, true); > + mptcp_pm_nl_addr_send_ack(msk); > + > if (lookup_subflow_by_daddr(&msk->conn_list, &remote)) > - goto add_addr_echo; > + return; > > /* pick id 0 port, if none is provided the remote address */ > - if (!remote.port) { > - reset_port = true; > + if (!remote.port) > remote.port = sk->sk_dport; > - } > > /* connect to the specified remote address, using whatever > * local address the routing configuration will pick. > @@ -684,14 +684,6 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk) > for (i = 0; i < nr; i++) > __mptcp_subflow_connect(sk, &addrs[i], &remote); > spin_lock_bh(&msk->pm.lock); > - > - /* be sure to echo exactly the received address */ > - if (reset_port) > - remote.port = 0; > - > -add_addr_echo: > - mptcp_pm_announce_addr(msk, &remote, true); > - mptcp_pm_nl_addr_send_ack(msk); > } > > void mptcp_pm_nl_addr_send_ack(struct mptcp_sock *msk) >
Hello, On Fri, 2022-02-25 at 16:53 +0800, Yonglong Li wrote: > In some corner case, It may receive ADD_ADDR retransmit package > before created subflows done. > It's more reasonable to send ADD_ADDR echo before creating subflows. > > Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn> The patch LGTM, and beyond fixing some corner case it looks like a nice cleanup. I think the changelog could be clairified a bit (even if I'm possibly the last person to give this specific kind of adivice ;). What about something alike: """ In some corner cases, the peer handing an incoming ADD_ADDR option, can receive a retransmitted ADD_ADDR for the same address before the subflow creation completes. We can avoid the above issue by generating and sending the ADD_ADDR echo before starting the MPJ subflow connection. """ @Matt could you handle such change while applaying the patch? Thanks!
Hi Paolo, On 01/03/2022 10:15, Paolo Abeni wrote: > Hello, > > On Fri, 2022-02-25 at 16:53 +0800, Yonglong Li wrote: >> In some corner case, It may receive ADD_ADDR retransmit package >> before created subflows done. >> It's more reasonable to send ADD_ADDR echo before creating subflows. >> >> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn> > > The patch LGTM, and beyond fixing some corner case it looks like a nice > cleanup. Can I add your Acked-by? :) > I think the changelog could be clairified a bit (even if I'm possibly > the last person to give this specific kind of adivice ;). What about > something alike: > > """ > In some corner cases, the peer handing an incoming ADD_ADDR option, can > receive a retransmitted ADD_ADDR for the same address before the > subflow creation completes. > > We can avoid the above issue by generating and sending the ADD_ADDR > echo before starting the MPJ subflow connection. > """ Good idea! Indeed clearer with this updated commit message. > @Matt could you handle such change while applaying the patch? Sure I can if there is no objection from Yonglong! Cheers, Matt
Hi Yonglong, Paolo On 25/02/2022 09:53, Yonglong Li wrote: > In some corner case, It may receive ADD_ADDR retransmit package > before created subflows done. > It's more reasonable to send ADD_ADDR echo before creating subflows. Thank you for this patch and the review! Applied in our tree (feat. for net-next) with Paolo's ACK: - 2b7de9309fe0: mptcp: send ADD_ADDR echo before create subflows - Results: 097423b70867..64f0c8f3cf8b This patch could be backported but as mentioned by Paolo, this fix is more an optimisation and it changes the behaviour. Builds and tests are now in progress: https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220301T115847 https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export Note: packetdrill tests have been adapted to this new behaviour. Cheers, Matt
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c index e3b0384..c1f4bef 100644 --- a/net/mptcp/pm_netlink.c +++ b/net/mptcp/pm_netlink.c @@ -650,7 +650,6 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk) unsigned int add_addr_accept_max; struct mptcp_addr_info remote; unsigned int subflows_max; - bool reset_port = false; int i, nr; add_addr_accept_max = mptcp_pm_get_add_addr_accept_max(msk); @@ -661,14 +660,15 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk) msk->pm.remote.family); remote = msk->pm.remote; + mptcp_pm_announce_addr(msk, &remote, true); + mptcp_pm_nl_addr_send_ack(msk); + if (lookup_subflow_by_daddr(&msk->conn_list, &remote)) - goto add_addr_echo; + return; /* pick id 0 port, if none is provided the remote address */ - if (!remote.port) { - reset_port = true; + if (!remote.port) remote.port = sk->sk_dport; - } /* connect to the specified remote address, using whatever * local address the routing configuration will pick. @@ -684,14 +684,6 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk) for (i = 0; i < nr; i++) __mptcp_subflow_connect(sk, &addrs[i], &remote); spin_lock_bh(&msk->pm.lock); - - /* be sure to echo exactly the received address */ - if (reset_port) - remote.port = 0; - -add_addr_echo: - mptcp_pm_announce_addr(msk, &remote, true); - mptcp_pm_nl_addr_send_ack(msk); } void mptcp_pm_nl_addr_send_ack(struct mptcp_sock *msk)
In some corner case, It may receive ADD_ADDR retransmit package before created subflows done. It's more reasonable to send ADD_ADDR echo before creating subflows. Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn> --- net/mptcp/pm_netlink.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-)