net/mptcp/protocol.c | 12 ------------ 1 file changed, 12 deletions(-)
This reverts commit 4293248c6704b854bf816aa1967e433402bee11c.
Additional locks are not needed, all the touched sections
are already under mptcp socket lock protection.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/protocol.c | 12 ------------
1 file changed, 12 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 5243c58789a4..ff567e9d0b1f 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1605,10 +1605,8 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags)
out:
/* ensure the rtx timer is running */
- mptcp_data_lock(sk);
if (!mptcp_timer_pending(sk))
mptcp_reset_timer(sk);
- mptcp_data_unlock(sk);
if (copied)
__mptcp_check_send_data_fin(sk);
}
@@ -2516,10 +2514,8 @@ static void __mptcp_retrans(struct sock *sk)
reset_timer:
mptcp_check_and_set_pending(sk);
- mptcp_data_lock(sk);
if (!mptcp_timer_pending(sk))
mptcp_reset_timer(sk);
- mptcp_data_unlock(sk);
}
static void mptcp_mp_fail_no_response(struct mptcp_sock *msk)
@@ -2707,10 +2703,8 @@ void mptcp_subflow_shutdown(struct sock *sk, struct sock *ssk, int how)
} else {
pr_debug("Sending DATA_FIN on subflow %p", ssk);
tcp_send_ack(ssk);
- mptcp_data_lock(sk);
if (!mptcp_timer_pending(sk))
mptcp_reset_timer(sk);
- mptcp_data_unlock(sk);
}
break;
}
@@ -2811,10 +2805,8 @@ static void __mptcp_destroy_sock(struct sock *sk)
/* join list will be eventually flushed (with rst) at sock lock release time*/
list_splice_init(&msk->conn_list, &conn_list);
- mptcp_data_lock(sk);
mptcp_stop_timer(sk);
sk_stop_timer(sk, &sk->sk_timer);
- mptcp_data_unlock(sk);
msk->pm.status = 0;
mptcp_release_sched(msk);
@@ -2877,9 +2869,7 @@ static void mptcp_close(struct sock *sk, long timeout)
__mptcp_destroy_sock(sk);
do_cancel_work = true;
} else {
- mptcp_data_lock(sk);
sk_reset_timer(sk, &sk->sk_timer, jiffies + TCP_TIMEWAIT_LEN);
- mptcp_data_unlock(sk);
}
release_sock(sk);
if (do_cancel_work)
@@ -2924,10 +2914,8 @@ static int mptcp_disconnect(struct sock *sk, int flags)
__mptcp_close_ssk(sk, ssk, subflow, MPTCP_CF_FASTCLOSE);
}
- mptcp_data_lock(sk);
mptcp_stop_timer(sk);
sk_stop_timer(sk, &sk->sk_timer);
- mptcp_data_unlock(sk);
if (mptcp_sk(sk)->token)
mptcp_event(MPTCP_EVENT_CLOSED, mptcp_sk(sk), NULL, GFP_KERNEL);
--
2.35.1
Hi Paolo, Mat, On 05/05/2022 19:12, Paolo Abeni wrote: > This reverts commit 4293248c6704b854bf816aa1967e433402bee11c. > > Additional locks are not needed, all the touched sections > are already under mptcp socket lock protection. Thank you for the patch and the review! Now in our tree (fixes for net-next) with Mat's RvB and a Fixes tag. New patches for t/upstream: - 2ff5e4eba944: Revert "mptcp: add data lock for sk timers" - Results: bc6528b2b1d7..22856348dac5 (export) Builds and tests are now in progress: https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220506T190735 https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export Cheers, Matt -- Tessares | Belgium | Hybrid Access Solutions www.tessares.net
On Fri, 6 May 2022, Matthieu Baerts wrote: > Hi Paolo, Mat, > > On 05/05/2022 19:12, Paolo Abeni wrote: >> This reverts commit 4293248c6704b854bf816aa1967e433402bee11c. >> >> Additional locks are not needed, all the touched sections >> are already under mptcp socket lock protection. > > Thank you for the patch and the review! > > Now in our tree (fixes for net-next) with Mat's RvB and a Fixes tag. > > New patches for t/upstream: > - 2ff5e4eba944: Revert "mptcp: add data lock for sk timers" > - Results: bc6528b2b1d7..22856348dac5 (export) This shows up in a different place in the export branch commit history than I expected: * 1238ef9dd5ce (HEAD, tag: export/20220509T115202, mptcp-nn/export) DO-NOT-MERGE: mptcp: enabled by default * a89b2ebc4a26 DO-NOT-MERGE: mptcp: use kmalloc on kasan build * 8adddf50a9e9 DO-NOT-MERGE: git markup: features other trees * c0e05f5207ec selftests/bpf: add bpf_first test * 90b2072ffc21 selftests/bpf: add bpf_first scheduler * 2f547420d0f4 mptcp: add bpf_mptcp_sched_ops * cf597ecbdb4b mptcp: add get_subflow wrappers * 1b3d7a5f1f10 mptcp: add sched in mptcp_sock * e31f54bd6183 mptcp: add a new sysctl scheduler * 74d3c74494e6 mptcp: add struct mptcp_sched_ops * 416cb433f1df selftests/bpf: verify first of struct mptcp_sock * 57dcfb6f61ec selftests/bpf: verify ca_name of struct mptcp_sock * 8996b41ffcd2 selftests/bpf: verify token of struct mptcp_sock * f8f99d2ff6a7 selftests/bpf: test bpf_skc_to_mptcp_sock * 3fb057749027 selftests/bpf: add MPTCP test base * 1585ded1ef3d selftests/bpf: Enable CONFIG_IKCONFIG_PROC in config * d4fc73d43d5b bpf: add bpf_skc_to_mptcp_sock_proto * 581159326c14 bpf: expose is_mptcp flag to bpf_tcp_sock * b20a348378e2 DO-NOT-MERGE: git markup: features net-next-next * fa71bf0a2db6 selftests: mptcp: add MP_FAIL reset testcase * fddb2da3de32 DO-NOT-MERGE: git markup: features net-next Expected to see the patch here. * 218186cd781d mptcp: sockopt: add TCP_DEFER_ACCEPT support * 6d8839d159a7 DO-NOT-MERGE: git markup: fixes net-next * f858740fef1e TopGit-driven merge of branches: |\ | * baa752231c4d DO-NOT-MERGE: git markup: end common net net-next | * cad34cf702bc DO-NOT-MERGE: mptcp: add CI support | * d7945d0bf36f DO-NOT-MERGE: git markup: fixes net | * d8e908a27426 selftests: mptcp: add subflow limits test-cases | * 001ce634cb09 mptcp: fix subflow accounting on close | * ca2debea5aa8 net/sched: act_pedit: really ensure the skb is writable | * 945110b5a812 DO-NOT-MERGE: git markup: fixes other trees | * 55775678c466 x86/pm: Fix false positive kmemleak report in msr_build_context() | * 7a38d7853776 DO-NOT-MERGE: git markup: net * | 4f43f697b6be Revert "mptcp: add data lock for sk timers" ^^^ but it's here instead * | f57e72f418df selftests: mptcp: fix a mp_fail test warning * | 49161dcf2772 DO-NOT-MERGE: git markup: net-next * | c908565eecf2 (mptcp-nn/net-next) Merge tag 'batadv-next-pullrequest-202205 Matthieu, is that what you intended? > > > Builds and tests are now in progress: > > https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220506T190735 > https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export -- Mat Martineau Intel
Hi Mat, On 10/05/2022 02:11, Mat Martineau wrote: > On Fri, 6 May 2022, Matthieu Baerts wrote: > >> Hi Paolo, Mat, >> >> On 05/05/2022 19:12, Paolo Abeni wrote: >>> This reverts commit 4293248c6704b854bf816aa1967e433402bee11c. >>> >>> Additional locks are not needed, all the touched sections >>> are already under mptcp socket lock protection. >> >> Thank you for the patch and the review! >> >> Now in our tree (fixes for net-next) with Mat's RvB and a Fixes tag. >> >> New patches for t/upstream: >> - 2ff5e4eba944: Revert "mptcp: add data lock for sk timers" >> - Results: bc6528b2b1d7..22856348dac5 (export) > > This shows up in a different place in the export branch commit history > than I expected: > > * 1238ef9dd5ce (HEAD, tag: export/20220509T115202, mptcp-nn/export) > DO-NOT-MERGE: mptcp: enabled by default > * a89b2ebc4a26 DO-NOT-MERGE: mptcp: use kmalloc on kasan build > * 8adddf50a9e9 DO-NOT-MERGE: git markup: features other trees > * c0e05f5207ec selftests/bpf: add bpf_first test > * 90b2072ffc21 selftests/bpf: add bpf_first scheduler > * 2f547420d0f4 mptcp: add bpf_mptcp_sched_ops > * cf597ecbdb4b mptcp: add get_subflow wrappers > * 1b3d7a5f1f10 mptcp: add sched in mptcp_sock > * e31f54bd6183 mptcp: add a new sysctl scheduler > * 74d3c74494e6 mptcp: add struct mptcp_sched_ops > * 416cb433f1df selftests/bpf: verify first of struct mptcp_sock > * 57dcfb6f61ec selftests/bpf: verify ca_name of struct mptcp_sock > * 8996b41ffcd2 selftests/bpf: verify token of struct mptcp_sock > * f8f99d2ff6a7 selftests/bpf: test bpf_skc_to_mptcp_sock > * 3fb057749027 selftests/bpf: add MPTCP test base > * 1585ded1ef3d selftests/bpf: Enable CONFIG_IKCONFIG_PROC in config > * d4fc73d43d5b bpf: add bpf_skc_to_mptcp_sock_proto > * 581159326c14 bpf: expose is_mptcp flag to bpf_tcp_sock > * b20a348378e2 DO-NOT-MERGE: git markup: features net-next-next > * fa71bf0a2db6 selftests: mptcp: add MP_FAIL reset testcase > * fddb2da3de32 DO-NOT-MERGE: git markup: features net-next > > Expected to see the patch here. > > * 218186cd781d mptcp: sockopt: add TCP_DEFER_ACCEPT support > * 6d8839d159a7 DO-NOT-MERGE: git markup: fixes net-next > * f858740fef1e TopGit-driven merge of branches: > |\ > | * baa752231c4d DO-NOT-MERGE: git markup: end common net net-next > | * cad34cf702bc DO-NOT-MERGE: mptcp: add CI support > | * d7945d0bf36f DO-NOT-MERGE: git markup: fixes net > | * d8e908a27426 selftests: mptcp: add subflow limits test-cases > | * 001ce634cb09 mptcp: fix subflow accounting on close > | * ca2debea5aa8 net/sched: act_pedit: really ensure the skb is writable > | * 945110b5a812 DO-NOT-MERGE: git markup: fixes other trees > | * 55775678c466 x86/pm: Fix false positive kmemleak report in > msr_build_context() > | * 7a38d7853776 DO-NOT-MERGE: git markup: net > * | 4f43f697b6be Revert "mptcp: add data lock for sk timers" > > ^^^ but it's here instead > > * | f57e72f418df selftests: mptcp: fix a mp_fail test warning > * | 49161dcf2772 DO-NOT-MERGE: git markup: net-next > * | c908565eecf2 (mptcp-nn/net-next) Merge tag > 'batadv-next-pullrequest-202205 > > > Matthieu, is that what you intended? Yes, that was my initial intension: if there is an issue with a patch in net-next, we probably don't want to wait for a sync with -net to send the patch. So it is on top of net-next only. It is just in case we force a sync with -net evenn if net and net-next have not been recently sync yet. Do you prefer to have these patches on top of -net and net-next instead? Cheers, Matt -- Tessares | Belgium | Hybrid Access Solutions www.tessares.net
On Tue, 10 May 2022, Matthieu Baerts wrote: > Hi Mat, > > On 10/05/2022 02:11, Mat Martineau wrote: >> On Fri, 6 May 2022, Matthieu Baerts wrote: >> >>> Hi Paolo, Mat, >>> >>> On 05/05/2022 19:12, Paolo Abeni wrote: >>>> This reverts commit 4293248c6704b854bf816aa1967e433402bee11c. >>>> >>>> Additional locks are not needed, all the touched sections >>>> are already under mptcp socket lock protection. >>> >>> Thank you for the patch and the review! >>> >>> Now in our tree (fixes for net-next) with Mat's RvB and a Fixes tag. >>> >>> New patches for t/upstream: >>> - 2ff5e4eba944: Revert "mptcp: add data lock for sk timers" >>> - Results: bc6528b2b1d7..22856348dac5 (export) >> >> This shows up in a different place in the export branch commit history >> than I expected: >> >> * 1238ef9dd5ce (HEAD, tag: export/20220509T115202, mptcp-nn/export) >> DO-NOT-MERGE: mptcp: enabled by default >> * a89b2ebc4a26 DO-NOT-MERGE: mptcp: use kmalloc on kasan build >> * 8adddf50a9e9 DO-NOT-MERGE: git markup: features other trees >> * c0e05f5207ec selftests/bpf: add bpf_first test >> * 90b2072ffc21 selftests/bpf: add bpf_first scheduler >> * 2f547420d0f4 mptcp: add bpf_mptcp_sched_ops >> * cf597ecbdb4b mptcp: add get_subflow wrappers >> * 1b3d7a5f1f10 mptcp: add sched in mptcp_sock >> * e31f54bd6183 mptcp: add a new sysctl scheduler >> * 74d3c74494e6 mptcp: add struct mptcp_sched_ops >> * 416cb433f1df selftests/bpf: verify first of struct mptcp_sock >> * 57dcfb6f61ec selftests/bpf: verify ca_name of struct mptcp_sock >> * 8996b41ffcd2 selftests/bpf: verify token of struct mptcp_sock >> * f8f99d2ff6a7 selftests/bpf: test bpf_skc_to_mptcp_sock >> * 3fb057749027 selftests/bpf: add MPTCP test base >> * 1585ded1ef3d selftests/bpf: Enable CONFIG_IKCONFIG_PROC in config >> * d4fc73d43d5b bpf: add bpf_skc_to_mptcp_sock_proto >> * 581159326c14 bpf: expose is_mptcp flag to bpf_tcp_sock >> * b20a348378e2 DO-NOT-MERGE: git markup: features net-next-next >> * fa71bf0a2db6 selftests: mptcp: add MP_FAIL reset testcase >> * fddb2da3de32 DO-NOT-MERGE: git markup: features net-next >> >> Expected to see the patch here. >> >> * 218186cd781d mptcp: sockopt: add TCP_DEFER_ACCEPT support >> * 6d8839d159a7 DO-NOT-MERGE: git markup: fixes net-next >> * f858740fef1e TopGit-driven merge of branches: >> |\ >> | * baa752231c4d DO-NOT-MERGE: git markup: end common net net-next >> | * cad34cf702bc DO-NOT-MERGE: mptcp: add CI support >> | * d7945d0bf36f DO-NOT-MERGE: git markup: fixes net >> | * d8e908a27426 selftests: mptcp: add subflow limits test-cases >> | * 001ce634cb09 mptcp: fix subflow accounting on close >> | * ca2debea5aa8 net/sched: act_pedit: really ensure the skb is writable >> | * 945110b5a812 DO-NOT-MERGE: git markup: fixes other trees >> | * 55775678c466 x86/pm: Fix false positive kmemleak report in >> msr_build_context() >> | * 7a38d7853776 DO-NOT-MERGE: git markup: net >> * | 4f43f697b6be Revert "mptcp: add data lock for sk timers" >> >> ^^^ but it's here instead >> >> * | f57e72f418df selftests: mptcp: fix a mp_fail test warning >> * | 49161dcf2772 DO-NOT-MERGE: git markup: net-next >> * | c908565eecf2 (mptcp-nn/net-next) Merge tag >> 'batadv-next-pullrequest-202205 >> >> >> Matthieu, is that what you intended? > > Yes, that was my initial intension: if there is an issue with a patch in > net-next, we probably don't want to wait for a sync with -net to send > the patch. So it is on top of net-next only. It is just in case we force > a sync with -net evenn if net and net-next have not been recently sync yet. > > Do you prefer to have these patches on top of -net and net-next instead? > That is my preference for future mptcp-next patches, yes. But you don't need to re-arrange these two patches in the tree. None of the patches staged for net-next (before or after the topgit-driven merge) are waiting for a sync unless there's a known conflict with a pending patch in export-net. When I prepare a series to send to the netdev list I often "git rebase --onto net-next/master <from-sha> <to-sha>" the relevant patches, and I don't want a merge commit in the middle of the range. It's also helpful to find out if there are any surprise conflicts at that stage so I can consider that before sending. -- Mat Martineau Intel
Hi Mat, On 11/05/2022 22:40, Mat Martineau wrote: > On Tue, 10 May 2022, Matthieu Baerts wrote: > >> Hi Mat, >> >> On 10/05/2022 02:11, Mat Martineau wrote: >>> On Fri, 6 May 2022, Matthieu Baerts wrote: >>> >>>> Hi Paolo, Mat, >>>> >>>> On 05/05/2022 19:12, Paolo Abeni wrote: >>>>> This reverts commit 4293248c6704b854bf816aa1967e433402bee11c. >>>>> >>>>> Additional locks are not needed, all the touched sections >>>>> are already under mptcp socket lock protection. >>>> >>>> Thank you for the patch and the review! >>>> >>>> Now in our tree (fixes for net-next) with Mat's RvB and a Fixes tag. >>>> >>>> New patches for t/upstream: >>>> - 2ff5e4eba944: Revert "mptcp: add data lock for sk timers" >>>> - Results: bc6528b2b1d7..22856348dac5 (export) >>> >>> This shows up in a different place in the export branch commit history >>> than I expected: >>> >>> * 1238ef9dd5ce (HEAD, tag: export/20220509T115202, mptcp-nn/export) >>> DO-NOT-MERGE: mptcp: enabled by default >>> * a89b2ebc4a26 DO-NOT-MERGE: mptcp: use kmalloc on kasan build >>> * 8adddf50a9e9 DO-NOT-MERGE: git markup: features other trees >>> * c0e05f5207ec selftests/bpf: add bpf_first test >>> * 90b2072ffc21 selftests/bpf: add bpf_first scheduler >>> * 2f547420d0f4 mptcp: add bpf_mptcp_sched_ops >>> * cf597ecbdb4b mptcp: add get_subflow wrappers >>> * 1b3d7a5f1f10 mptcp: add sched in mptcp_sock >>> * e31f54bd6183 mptcp: add a new sysctl scheduler >>> * 74d3c74494e6 mptcp: add struct mptcp_sched_ops >>> * 416cb433f1df selftests/bpf: verify first of struct mptcp_sock >>> * 57dcfb6f61ec selftests/bpf: verify ca_name of struct mptcp_sock >>> * 8996b41ffcd2 selftests/bpf: verify token of struct mptcp_sock >>> * f8f99d2ff6a7 selftests/bpf: test bpf_skc_to_mptcp_sock >>> * 3fb057749027 selftests/bpf: add MPTCP test base >>> * 1585ded1ef3d selftests/bpf: Enable CONFIG_IKCONFIG_PROC in config >>> * d4fc73d43d5b bpf: add bpf_skc_to_mptcp_sock_proto >>> * 581159326c14 bpf: expose is_mptcp flag to bpf_tcp_sock >>> * b20a348378e2 DO-NOT-MERGE: git markup: features net-next-next >>> * fa71bf0a2db6 selftests: mptcp: add MP_FAIL reset testcase >>> * fddb2da3de32 DO-NOT-MERGE: git markup: features net-next >>> >>> Expected to see the patch here. >>> >>> * 218186cd781d mptcp: sockopt: add TCP_DEFER_ACCEPT support >>> * 6d8839d159a7 DO-NOT-MERGE: git markup: fixes net-next >>> * f858740fef1e TopGit-driven merge of branches: >>> |\ >>> | * baa752231c4d DO-NOT-MERGE: git markup: end common net net-next >>> | * cad34cf702bc DO-NOT-MERGE: mptcp: add CI support >>> | * d7945d0bf36f DO-NOT-MERGE: git markup: fixes net >>> | * d8e908a27426 selftests: mptcp: add subflow limits test-cases >>> | * 001ce634cb09 mptcp: fix subflow accounting on close >>> | * ca2debea5aa8 net/sched: act_pedit: really ensure the skb is writable >>> | * 945110b5a812 DO-NOT-MERGE: git markup: fixes other trees >>> | * 55775678c466 x86/pm: Fix false positive kmemleak report in >>> msr_build_context() >>> | * 7a38d7853776 DO-NOT-MERGE: git markup: net >>> * | 4f43f697b6be Revert "mptcp: add data lock for sk timers" >>> >>> ^^^ but it's here instead >>> >>> * | f57e72f418df selftests: mptcp: fix a mp_fail test warning >>> * | 49161dcf2772 DO-NOT-MERGE: git markup: net-next >>> * | c908565eecf2 (mptcp-nn/net-next) Merge tag >>> 'batadv-next-pullrequest-202205 >>> >>> >>> Matthieu, is that what you intended? >> >> Yes, that was my initial intension: if there is an issue with a patch in >> net-next, we probably don't want to wait for a sync with -net to send >> the patch. So it is on top of net-next only. It is just in case we force >> a sync with -net evenn if net and net-next have not been recently sync >> yet. >> >> Do you prefer to have these patches on top of -net and net-next instead? >> > > That is my preference for future mptcp-next patches, yes. But you don't > need to re-arrange these two patches in the tree. > > None of the patches staged for net-next (before or after the > topgit-driven merge) are waiting for a sync unless there's a known > conflict with a pending patch in export-net. > > When I prepare a series to send to the netdev list I often "git rebase > --onto net-next/master <from-sha> <to-sha>" the relevant patches, and I > don't want a merge commit in the middle of the range. It's also helpful > to find out if there are any surprise conflicts at that stage so I can > consider that before sending. Fine for me, I just did the modification to put all fixes for net-next on top of -net as well! Cheers, Matt -- Tessares | Belgium | Hybrid Access Solutions www.tessares.net
On Thu, 5 May 2022, Paolo Abeni wrote: > This reverts commit 4293248c6704b854bf816aa1967e433402bee11c. > > Additional locks are not needed, all the touched sections > are already under mptcp socket lock protection. > I agree that this needs to be reverted: Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com> But msk->sk_timer is *also* accessed in two places without the mptcp socket lock (but with the data lock): * mptcp_pm_mp_fail_received() (stop timer when mp_fail received) * subflow_check_data_avail() (start timer on infinite mapping rx) I think these were the reason the locking was added, because we don't want the use of msk->sk_timer for MP_FAIL timeout to interfere with other use of msk->sk_timer when a msk is closing. Now I see that the data lock doesn't work for that purpose. In addition to removing the data locks as this patch does, it looks like the two functions I listed above need to use msk->cb_flags and deferred events to safely check msk->sk_state and modify msk->sk_timer. What do you think? I can send a patch on Friday. - Mat > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > net/mptcp/protocol.c | 12 ------------ > 1 file changed, 12 deletions(-) > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index 5243c58789a4..ff567e9d0b1f 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -1605,10 +1605,8 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags) > > out: > /* ensure the rtx timer is running */ > - mptcp_data_lock(sk); > if (!mptcp_timer_pending(sk)) > mptcp_reset_timer(sk); > - mptcp_data_unlock(sk); > if (copied) > __mptcp_check_send_data_fin(sk); > } > @@ -2516,10 +2514,8 @@ static void __mptcp_retrans(struct sock *sk) > reset_timer: > mptcp_check_and_set_pending(sk); > > - mptcp_data_lock(sk); > if (!mptcp_timer_pending(sk)) > mptcp_reset_timer(sk); > - mptcp_data_unlock(sk); > } > > static void mptcp_mp_fail_no_response(struct mptcp_sock *msk) > @@ -2707,10 +2703,8 @@ void mptcp_subflow_shutdown(struct sock *sk, struct sock *ssk, int how) > } else { > pr_debug("Sending DATA_FIN on subflow %p", ssk); > tcp_send_ack(ssk); > - mptcp_data_lock(sk); > if (!mptcp_timer_pending(sk)) > mptcp_reset_timer(sk); > - mptcp_data_unlock(sk); > } > break; > } > @@ -2811,10 +2805,8 @@ static void __mptcp_destroy_sock(struct sock *sk) > /* join list will be eventually flushed (with rst) at sock lock release time*/ > list_splice_init(&msk->conn_list, &conn_list); > > - mptcp_data_lock(sk); > mptcp_stop_timer(sk); > sk_stop_timer(sk, &sk->sk_timer); > - mptcp_data_unlock(sk); > msk->pm.status = 0; > mptcp_release_sched(msk); > > @@ -2877,9 +2869,7 @@ static void mptcp_close(struct sock *sk, long timeout) > __mptcp_destroy_sock(sk); > do_cancel_work = true; > } else { > - mptcp_data_lock(sk); > sk_reset_timer(sk, &sk->sk_timer, jiffies + TCP_TIMEWAIT_LEN); > - mptcp_data_unlock(sk); > } > release_sock(sk); > if (do_cancel_work) > @@ -2924,10 +2914,8 @@ static int mptcp_disconnect(struct sock *sk, int flags) > __mptcp_close_ssk(sk, ssk, subflow, MPTCP_CF_FASTCLOSE); > } > > - mptcp_data_lock(sk); > mptcp_stop_timer(sk); > sk_stop_timer(sk, &sk->sk_timer); > - mptcp_data_unlock(sk); > > if (mptcp_sk(sk)->token) > mptcp_event(MPTCP_EVENT_CLOSED, mptcp_sk(sk), NULL, GFP_KERNEL); > -- > 2.35.1 > > > -- Mat Martineau Intel
On Thu, 2022-05-05 at 16:40 -0700, Mat Martineau wrote: > On Thu, 5 May 2022, Paolo Abeni wrote: > > > This reverts commit 4293248c6704b854bf816aa1967e433402bee11c. > > > > Additional locks are not needed, all the touched sections > > are already under mptcp socket lock protection. > > > > I agree that this needs to be reverted: > > Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com> > > > But msk->sk_timer is *also* accessed in two places without the mptcp > socket lock (but with the data lock): > * mptcp_pm_mp_fail_received() (stop timer when mp_fail received) > > * subflow_check_data_avail() (start timer on infinite mapping rx) I'm reasonably no additional lock is required to call sk_stop_timer() and/or sk_reset_timer(): they boil down to the timer_{del,mod} primitives which in turns are irq safe. The mptcp wrappers *could* require additional locking because they additionally touch mptcp_sk(sk)->timer_ival. I *think* we could avoid the lock even there with some additional barrier, but it looks every caller is already under the lock. I think we don't need to defer touching the timer. Eventully we could remove the check on the msk socket status, which again looks not needed. Cheers, Paolo
© 2016 - 2024 Red Hat, Inc.