| Message ID | 45298408838519faf5eb1ca8e39593c3a5d4bac9.1612869198.git.pabeni@redhat.com |
|---|---|
| State | Accepted, archived |
| Commit | f1c293d570e18e8ba0c7af97129158df05860731 |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | mptcp: improve mptcp release callback | expand |
Hi Paolo, Thank you for the patch! On 09/02/2021 12:42, Paolo Abeni wrote: > If we receive a MPTCP_PUSH_PENDING even from a subflow when > mptcp_release_cb() is serving the previous one, the latter > will be delayed up to the next release_sock(msk). > > Address the issue implementing a test/serve loop for such > event. > > Additionally rename the push helper to __mptcp_push_pending() > to be more consistent with the existing code. (...) > @@ -2942,13 +2942,14 @@ static void mptcp_release_cb(struct sock *sk) > { > unsigned long flags, nflags; > > - /* push_pending may touch wmem_reserved, do it before the later > - * cleanup > - */ > - if (test_and_clear_bit(MPTCP_CLEAN_UNA, &mptcp_sk(sk)->flags)) > - __mptcp_clean_una(sk); > - if (test_and_clear_bit(MPTCP_PUSH_PENDING, &mptcp_sk(sk)->flags)) { > - /* mptcp_push_pending() acquires the subflow socket lock > + for (;;) { > + flags = 0; > + if (test_and_clear_bit(MPTCP_PUSH_PENDING, &mptcp_sk(sk)->flags)) > + flags |= MPTCP_PUSH_PENDING; > + if (!flags) > + break; This looks strange: "flags" is reset to 0 then set if the previous condition is valid and we check what was done at the previous instruction. Maybe clearer with "else" instead of "if (!flags)"? > + > + /* the following actions acquire the subflow socket lock > * > * 1) can't be invoked in atomic scope > * 2) must avoid ABBA deadlock with msk socket spinlock: the RX > @@ -2957,13 +2958,21 @@ static void mptcp_release_cb(struct sock *sk) > */ > > spin_unlock_bh(&sk->sk_lock.slock); > - mptcp_push_pending(sk, 0); > + if (flags & MPTCP_PUSH_PENDING) > + __mptcp_push_pending(sk, 0); > + > + cond_resched(); > spin_lock_bh(&sk->sk_lock.slock); > } > + > + if (test_and_clear_bit(MPTCP_CLEAN_UNA, &mptcp_sk(sk)->flags)) > + __mptcp_clean_una(sk); > if (test_and_clear_bit(MPTCP_ERROR_REPORT, &mptcp_sk(sk)->flags)) > __mptcp_error_report(sk); > > - /* clear any wmem reservation and errors */ > + /* push_pending may touch wmem_reserved, ensure we do the cleanup > + * later But we still need to update "rmem", right? :) (oh yes but even before, we were only talking about "wmem", fine!) > + */ > __mptcp_update_wmem(sk); > __mptcp_update_rmem(sk); > > Cheers, Matt
On 09/02/2021 15:22, Matthieu Baerts wrote: > Hi Paolo, > > Thank you for the patch! > > On 09/02/2021 12:42, Paolo Abeni wrote: >> If we receive a MPTCP_PUSH_PENDING even from a subflow when >> mptcp_release_cb() is serving the previous one, the latter >> will be delayed up to the next release_sock(msk). >> >> Address the issue implementing a test/serve loop for such >> event. >> >> Additionally rename the push helper to __mptcp_push_pending() >> to be more consistent with the existing code. > > (...) > >> @@ -2942,13 +2942,14 @@ static void mptcp_release_cb(struct sock *sk) >> { >> unsigned long flags, nflags; >> - /* push_pending may touch wmem_reserved, do it before the later >> - * cleanup >> - */ >> - if (test_and_clear_bit(MPTCP_CLEAN_UNA, &mptcp_sk(sk)->flags)) >> - __mptcp_clean_una(sk); >> - if (test_and_clear_bit(MPTCP_PUSH_PENDING, &mptcp_sk(sk)->flags)) { >> - /* mptcp_push_pending() acquires the subflow socket lock >> + for (;;) { >> + flags = 0; >> + if (test_and_clear_bit(MPTCP_PUSH_PENDING, >> &mptcp_sk(sk)->flags)) >> + flags |= MPTCP_PUSH_PENDING; >> + if (!flags) >> + break; > > This looks strange: "flags" is reset to 0 then set if the previous > condition is valid and we check what was done at the previous > instruction. Maybe clearer with "else" instead of "if (!flags)"? My bad, I understand why after having looked at the next patch! I hit the send button too fast :) Cheers, Matt
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 7e5016903e11c..0393cef5a08d8 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -1442,7 +1442,7 @@ static void mptcp_push_release(struct sock *sk, struct sock *ssk, release_sock(ssk); } -static void mptcp_push_pending(struct sock *sk, unsigned int flags) +static void __mptcp_push_pending(struct sock *sk, unsigned int flags) { struct sock *prev_ssk = NULL, *ssk = NULL; struct mptcp_sock *msk = mptcp_sk(sk); @@ -1694,14 +1694,14 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len) wait_for_memory: mptcp_set_nospace(sk); - mptcp_push_pending(sk, msg->msg_flags); + __mptcp_push_pending(sk, msg->msg_flags); ret = sk_stream_wait_memory(sk, &timeo); if (ret) goto out; } if (copied) - mptcp_push_pending(sk, msg->msg_flags); + __mptcp_push_pending(sk, msg->msg_flags); out: release_sock(sk); @@ -2942,13 +2942,14 @@ static void mptcp_release_cb(struct sock *sk) { unsigned long flags, nflags; - /* push_pending may touch wmem_reserved, do it before the later - * cleanup - */ - if (test_and_clear_bit(MPTCP_CLEAN_UNA, &mptcp_sk(sk)->flags)) - __mptcp_clean_una(sk); - if (test_and_clear_bit(MPTCP_PUSH_PENDING, &mptcp_sk(sk)->flags)) { - /* mptcp_push_pending() acquires the subflow socket lock + for (;;) { + flags = 0; + if (test_and_clear_bit(MPTCP_PUSH_PENDING, &mptcp_sk(sk)->flags)) + flags |= MPTCP_PUSH_PENDING; + if (!flags) + break; + + /* the following actions acquire the subflow socket lock * * 1) can't be invoked in atomic scope * 2) must avoid ABBA deadlock with msk socket spinlock: the RX @@ -2957,13 +2958,21 @@ static void mptcp_release_cb(struct sock *sk) */ spin_unlock_bh(&sk->sk_lock.slock); - mptcp_push_pending(sk, 0); + if (flags & MPTCP_PUSH_PENDING) + __mptcp_push_pending(sk, 0); + + cond_resched(); spin_lock_bh(&sk->sk_lock.slock); } + + if (test_and_clear_bit(MPTCP_CLEAN_UNA, &mptcp_sk(sk)->flags)) + __mptcp_clean_una(sk); if (test_and_clear_bit(MPTCP_ERROR_REPORT, &mptcp_sk(sk)->flags)) __mptcp_error_report(sk); - /* clear any wmem reservation and errors */ + /* push_pending may touch wmem_reserved, ensure we do the cleanup + * later + */ __mptcp_update_wmem(sk); __mptcp_update_rmem(sk);
If we receive a MPTCP_PUSH_PENDING even from a subflow when mptcp_release_cb() is serving the previous one, the latter will be delayed up to the next release_sock(msk). Address the issue implementing a test/serve loop for such event. Additionally rename the push helper to __mptcp_push_pending() to be more consistent with the existing code. Fixes: 6e628cd3a8f7 ("mptcp: use mptcp release_cb for delayed tasks") Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- net/mptcp/protocol.c | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-)