| Message ID | 8363fd6542b179fdc30b42e78c922b0201c9f3fd.1705331716.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | c6d26639b0a9152c3c7835ab369d6c5c3116e75f |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | mptcp: locking cleanup | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 43 lines checked |
| matttbe/build | success | Build and static analysis OK |
| matttbe/KVM_Validation__debug__except_selftest_mptcp_join_ | warning | Unstable: 1 failed test(s): selftest_diag |
| matttbe/KVM_Validation__debug__only_selftest_mptcp_join_ | success | Success! ✅ |
| matttbe/KVM_Validation__normal | warning | Unstable: 2 failed test(s): packetdrill_regressions selftest_mptcp_join |
Hi Paolo, On 15/01/2024 16:16, Paolo Abeni wrote: > Such field is there to avoid acquiring the data lock in a few spots, > but if adds complexity to the already non trivial locking schema. > > All the relevant call sites (mptcp-level reinjection, set socket optins), > are slow-path, drop such field in favor of 'cb_flags', adding the relevant > locking. In your cover-letter, you mentioned: > Patch 1 has no fixes, but still is logically tied to the other patches. I think it makes sense to have this patch in -net, especially because it is logically tied to the others. Would it be OK for you if I add: Fixes: e9d09baca676 ("mptcp: avoid atomic bit manipulation when possible") Or do you prefer without? Cheers, Matt
On Thu, 2024-02-08 at 12:36 +0100, Matthieu Baerts wrote: > Hi Paolo, > > On 15/01/2024 16:16, Paolo Abeni wrote: > > Such field is there to avoid acquiring the data lock in a few spots, > > but if adds complexity to the already non trivial locking schema. > > > > All the relevant call sites (mptcp-level reinjection, set socket optins), > > are slow-path, drop such field in favor of 'cb_flags', adding the relevant > > locking. > > In your cover-letter, you mentioned: > > > Patch 1 has no fixes, but still is logically tied to the other patches. > > I think it makes sense to have this patch in -net, especially because it > is logically tied to the others. Would it be OK for you if I add: > > Fixes: e9d09baca676 ("mptcp: avoid atomic bit manipulation when possible") The obove, or add a sentence to the commit message alike: """ This will also simplify the next patch """ > > Or do you prefer without? > > Cheers, > Matt
Hi Paolo, On 08/02/2024 16:12, Paolo Abeni wrote: > On Thu, 2024-02-08 at 12:36 +0100, Matthieu Baerts wrote: >> Hi Paolo, >> >> On 15/01/2024 16:16, Paolo Abeni wrote: >>> Such field is there to avoid acquiring the data lock in a few spots, >>> but if adds complexity to the already non trivial locking schema. >>> >>> All the relevant call sites (mptcp-level reinjection, set socket optins), >>> are slow-path, drop such field in favor of 'cb_flags', adding the relevant >>> locking. >> >> In your cover-letter, you mentioned: >> >>> Patch 1 has no fixes, but still is logically tied to the other patches. >> >> I think it makes sense to have this patch in -net, especially because it >> is logically tied to the others. Would it be OK for you if I add: >> >> Fixes: e9d09baca676 ("mptcp: avoid atomic bit manipulation when possible") > > The obove, or add a sentence to the commit message alike: > > """ > This will also simplify the next patch > """ Good idea! I added both: > This patch could be seen as an improvement, instead of a fix. But it > simplifies the next patch. The 'Fixes' tag has been added to help having > this series backported to stable. I hope that's OK! :) Cheers, Matt
On Thu, 2024-02-08 at 19:05 +0100, Matthieu Baerts wrote: > On 08/02/2024 16:12, Paolo Abeni wrote: > > > This patch could be seen as an improvement, instead of a fix. But it > > simplifies the next patch. The 'Fixes' tag has been added to help having > > this series backported to stable. > > I hope that's OK! :) fine by me, thanks! /P
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index e570c7e2b18c..5129e1235cc5 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -1505,8 +1505,11 @@ static void mptcp_update_post_push(struct mptcp_sock *msk, void mptcp_check_and_set_pending(struct sock *sk) { - if (mptcp_send_head(sk)) - mptcp_sk(sk)->push_pending |= BIT(MPTCP_PUSH_PENDING); + if (mptcp_send_head(sk)) { + mptcp_data_lock(sk); + mptcp_sk(sk)->cb_flags |= BIT(MPTCP_PUSH_PENDING); + mptcp_data_unlock(sk); + } } static int __subflow_push_pending(struct sock *sk, struct sock *ssk, @@ -3145,7 +3148,6 @@ static int mptcp_disconnect(struct sock *sk, int flags) mptcp_destroy_common(msk, MPTCP_CF_FASTCLOSE); WRITE_ONCE(msk->flags, 0); msk->cb_flags = 0; - msk->push_pending = 0; msk->recovery = false; msk->can_ack = false; msk->fully_established = false; @@ -3333,8 +3335,7 @@ static void mptcp_release_cb(struct sock *sk) struct mptcp_sock *msk = mptcp_sk(sk); for (;;) { - unsigned long flags = (msk->cb_flags & MPTCP_FLAGS_PROCESS_CTX_NEED) | - msk->push_pending; + unsigned long flags = (msk->cb_flags & MPTCP_FLAGS_PROCESS_CTX_NEED); struct list_head join_list; if (!flags) @@ -3350,7 +3351,6 @@ static void mptcp_release_cb(struct sock *sk) * datapath acquires the msk socket spinlock while helding * the subflow socket lock */ - msk->push_pending = 0; msk->cb_flags &= ~flags; spin_unlock_bh(&sk->sk_lock.slock); diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index f7b9c1b995df..ebb32b7563be 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -286,7 +286,6 @@ struct mptcp_sock { int rmem_released; unsigned long flags; unsigned long cb_flags; - unsigned long push_pending; bool recovery; /* closing subflow write queue reinjected */ bool can_ack; bool fully_established;
Such field is there to avoid acquiring the data lock in a few spots, but if adds complexity to the already non trivial locking schema. All the relevant call sites (mptcp-level reinjection, set socket optins), are slow-path, drop such field in favor of 'cb_flags', adding the relevant locking. Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- net/mptcp/protocol.c | 12 ++++++------ net/mptcp/protocol.h | 1 - 2 files changed, 6 insertions(+), 7 deletions(-)