mptcp: cleanup state propagation.

[PATCH mptcp-net-next] mptcp: cleanup state propagation.

Posted by Paolo Abeni 5 months, 1 week ago

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);
 }
-- 
2.41.0

Re: [PATCH mptcp-net-next] mptcp: cleanup state propagation.

Posted by Paolo Abeni 4 months, 1 week ago

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

Re: [PATCH mptcp-net-next] mptcp: cleanup state propagation.

Posted by Matthieu Baerts 4 months, 1 week ago

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
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net-next] mptcp: cleanup state propagation.

Posted by Matthieu Baerts 4 months, 3 weeks ago

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
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net-next] mptcp: cleanup state propagation.

Posted by Mat Martineau 5 months ago

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>