| Message ID | 240a97d522887a938faeb24a6a3e3a9ed24570b1.1700667908.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 55cd89098414ddcd3de9775dfb4a33825587c29a |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [mptcp-net-next] mptcp: cleanup state propagation. | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 31 lines checked |
| matttbe/KVM_Validation__normal__except_selftest_mptcp_join_ | success | Success! ✅ |
| matttbe/KVM_Validation__normal__only_selftest_mptcp_join_ | success | Success! ✅ |
| matttbe/KVM_Validation__debug__only_selftest_mptcp_join_ | success | Success! ✅ |
| matttbe/KVM_Validation__debug__except_selftest_mptcp_join_ | success | Success! ✅ |
Hi Paolo,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal (except selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/6501777365794816
- Summary: https://api.cirrus-ci.com/v1/artifact/task/6501777365794816/summary/summary.txt
- KVM Validation: normal (only selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/4672190017175552
- Summary: https://api.cirrus-ci.com/v1/artifact/task/4672190017175552/summary/summary.txt
- KVM Validation: debug (only selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/5235139970596864
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5235139970596864/summary/summary.txt
- KVM Validation: debug (except selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/5798089924018176
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5798089924018176/summary/summary.txt
Initiator: Matthieu Baerts
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/e29d5c16e056
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-debug
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)
On Wed, 22 Nov 2023, Paolo Abeni wrote: > When propagating the first subflow state via the release_cb, the > msk must actually take to 2 separate actions: state sync-up and > send buffer sync-up. Relaying on the specific flags instead of > bounding both in __mptcp_sync_state(), allows cleaning up a bit > the code. > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > net/mptcp/protocol.c | 3 +-- > net/mptcp/subflow.c | 5 ++--- > 2 files changed, 3 insertions(+), 5 deletions(-) > Looks good to me, thanks Paolo. Reviewed-by: Mat Martineau <martineau@kernel.org>
Hi Paolo, Mat, On 22/11/2023 17:21, Paolo Abeni wrote: > When propagating the first subflow state via the release_cb, the > msk must actually take to 2 separate actions: state sync-up and > send buffer sync-up. Relaying on the specific flags instead of > bounding both in __mptcp_sync_state(), allows cleaning up a bit > the code. Thank you for the patch and the review! Now in our tree (feat. for net-next): New patches for t/upstream: - 55cd89098414: mptcp: cleanup state propagation - Results: b1ca2c54c539..3050a643425e (export) Tests are now in progress: https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20231204T183518 Cheers, Matt
Hi, On Wed, 2023-11-22 at 17:21 +0100, Paolo Abeni wrote: > When propagating the first subflow state via the release_cb, the > msk must actually take to 2 separate actions: state sync-up and > send buffer sync-up. Relaying on the specific flags instead of > bounding both in __mptcp_sync_state(), allows cleaning up a bit > the code. > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > net/mptcp/protocol.c | 3 +-- > net/mptcp/subflow.c | 5 ++--- > 2 files changed, 3 insertions(+), 5 deletions(-) > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index 199b2e09f1ac..4fc038d29623 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -3404,9 +3404,8 @@ static void mptcp_release_cb(struct sock *sk) > if (unlikely(msk->cb_flags)) { > /* be sure to sync the msk state before taking actions > * depending on sk_state (MPTCP_ERROR_REPORT) > - * On sk release avoid actions depending on the first subflow > */ > - if (__test_and_clear_bit(MPTCP_SYNC_STATE, &msk->cb_flags) && msk->first) > + if (__test_and_clear_bit(MPTCP_SYNC_STATE, &msk->cb_flags)) > __mptcp_sync_state(sk, msk->pending_state); > if (__test_and_clear_bit(MPTCP_ERROR_REPORT, &msk->cb_flags)) > __mptcp_error_report(sk); > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c > index 6d7684c35e93..64302a1ac306 100644 > --- a/net/mptcp/subflow.c > +++ b/net/mptcp/subflow.c > @@ -421,9 +421,6 @@ static bool subflow_use_different_dport(struct mptcp_sock *msk, const struct soc > > void __mptcp_sync_state(struct sock *sk, int state) > { > - struct mptcp_sock *msk = mptcp_sk(sk); > - > - __mptcp_propagate_sndbuf(sk, msk->first); > if (sk->sk_state == TCP_SYN_SENT) { > inet_sk_state_store(sk, state); > sk->sk_state_change(sk); > @@ -436,10 +433,12 @@ static void mptcp_propagate_state(struct sock *sk, struct sock *ssk) > > mptcp_data_lock(sk); > if (!sock_owned_by_user(sk)) { > + __mptcp_propagate_sndbuf(sk, msk->first); > __mptcp_sync_state(sk, ssk->sk_state); > } else { > msk->pending_state = ssk->sk_state; > __set_bit(MPTCP_SYNC_STATE, &msk->cb_flags); > + __set_bit(MPTCP_SYNC_SNDBUF, &msk->cb_flags); > } > mptcp_data_unlock(sk); > } Could you please drop this patch? And not sent it upstream... I'm working on a series of pre-req for https://github.com/multipath-tcp/mptcp_net-next/issues/464 and - among many other things - it basically will require reverting this patch :( Thanks! Paolo
Hi Paolo, On 22/12/2023 10:16, Paolo Abeni wrote: > Hi, > > On Wed, 2023-11-22 at 17:21 +0100, Paolo Abeni wrote: >> When propagating the first subflow state via the release_cb, the >> msk must actually take to 2 separate actions: state sync-up and >> send buffer sync-up. Relaying on the specific flags instead of >> bounding both in __mptcp_sync_state(), allows cleaning up a bit >> the code. >> >> Signed-off-by: Paolo Abeni <pabeni@redhat.com> >> --- >> net/mptcp/protocol.c | 3 +-- >> net/mptcp/subflow.c | 5 ++--- >> 2 files changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c >> index 199b2e09f1ac..4fc038d29623 100644 >> --- a/net/mptcp/protocol.c >> +++ b/net/mptcp/protocol.c >> @@ -3404,9 +3404,8 @@ static void mptcp_release_cb(struct sock *sk) >> if (unlikely(msk->cb_flags)) { >> /* be sure to sync the msk state before taking actions >> * depending on sk_state (MPTCP_ERROR_REPORT) >> - * On sk release avoid actions depending on the first subflow >> */ >> - if (__test_and_clear_bit(MPTCP_SYNC_STATE, &msk->cb_flags) && msk->first) >> + if (__test_and_clear_bit(MPTCP_SYNC_STATE, &msk->cb_flags)) >> __mptcp_sync_state(sk, msk->pending_state); >> if (__test_and_clear_bit(MPTCP_ERROR_REPORT, &msk->cb_flags)) >> __mptcp_error_report(sk); >> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c >> index 6d7684c35e93..64302a1ac306 100644 >> --- a/net/mptcp/subflow.c >> +++ b/net/mptcp/subflow.c >> @@ -421,9 +421,6 @@ static bool subflow_use_different_dport(struct mptcp_sock *msk, const struct soc >> >> void __mptcp_sync_state(struct sock *sk, int state) >> { >> - struct mptcp_sock *msk = mptcp_sk(sk); >> - >> - __mptcp_propagate_sndbuf(sk, msk->first); >> if (sk->sk_state == TCP_SYN_SENT) { >> inet_sk_state_store(sk, state); >> sk->sk_state_change(sk); >> @@ -436,10 +433,12 @@ static void mptcp_propagate_state(struct sock *sk, struct sock *ssk) >> >> mptcp_data_lock(sk); >> if (!sock_owned_by_user(sk)) { >> + __mptcp_propagate_sndbuf(sk, msk->first); >> __mptcp_sync_state(sk, ssk->sk_state); >> } else { >> msk->pending_state = ssk->sk_state; >> __set_bit(MPTCP_SYNC_STATE, &msk->cb_flags); >> + __set_bit(MPTCP_SYNC_SNDBUF, &msk->cb_flags); >> } >> mptcp_data_unlock(sk); >> } > > Could you please drop this patch? And not sent it upstream... > > I'm working on a series of pre-req for > https://github.com/multipath-tcp/mptcp_net-next/issues/464 > and - among many other things - it basically will require reverting > this patch :( Thank you for the heads-up! While at it, I also fixed a compilation issue after the last sync with net-next. New patches for t/upstream: - 6835eade0d25: Revert "mptcp: cleanup state propagation" - ba09c8c53a73: Revert "Squash to "bpf: Add bpf_mptcp_sched_ops"" - Results: 748f20530b90..4720030d95d3 (export) (Tests are not in progress) Cheers, Matt
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 199b2e09f1ac..4fc038d29623 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -3404,9 +3404,8 @@ static void mptcp_release_cb(struct sock *sk) if (unlikely(msk->cb_flags)) { /* be sure to sync the msk state before taking actions * depending on sk_state (MPTCP_ERROR_REPORT) - * On sk release avoid actions depending on the first subflow */ - if (__test_and_clear_bit(MPTCP_SYNC_STATE, &msk->cb_flags) && msk->first) + if (__test_and_clear_bit(MPTCP_SYNC_STATE, &msk->cb_flags)) __mptcp_sync_state(sk, msk->pending_state); if (__test_and_clear_bit(MPTCP_ERROR_REPORT, &msk->cb_flags)) __mptcp_error_report(sk); diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index 6d7684c35e93..64302a1ac306 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -421,9 +421,6 @@ static bool subflow_use_different_dport(struct mptcp_sock *msk, const struct soc void __mptcp_sync_state(struct sock *sk, int state) { - struct mptcp_sock *msk = mptcp_sk(sk); - - __mptcp_propagate_sndbuf(sk, msk->first); if (sk->sk_state == TCP_SYN_SENT) { inet_sk_state_store(sk, state); sk->sk_state_change(sk); @@ -436,10 +433,12 @@ static void mptcp_propagate_state(struct sock *sk, struct sock *ssk) mptcp_data_lock(sk); if (!sock_owned_by_user(sk)) { + __mptcp_propagate_sndbuf(sk, msk->first); __mptcp_sync_state(sk, ssk->sk_state); } else { msk->pending_state = ssk->sk_state; __set_bit(MPTCP_SYNC_STATE, &msk->cb_flags); + __set_bit(MPTCP_SYNC_SNDBUF, &msk->cb_flags); } mptcp_data_unlock(sk); }
When propagating the first subflow state via the release_cb, the msk must actually take to 2 separate actions: state sync-up and send buffer sync-up. Relaying on the specific flags instead of bounding both in __mptcp_sync_state(), allows cleaning up a bit the code. Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- net/mptcp/protocol.c | 3 +-- net/mptcp/subflow.c | 5 ++--- 2 files changed, 3 insertions(+), 5 deletions(-)