[PATCH mptcp-next 1/3] mptcp: optimize release_cb for the common case

[PATCH mptcp-next 1/3] mptcp: optimize release_cb for the common case

[Paolo Abeni]

The mptcp release callback checks several flags in atomic
context, but only MPTCP_CLEAN_UNA can be up frequently.

Reorganize the code to avoid multiple conditionals in the
most common scenarios.

Additional clarify a related comment.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index fbb14dfe62b3..bdba1ddee2a7 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3118,15 +3118,17 @@ static void mptcp_release_cb(struct sock *sk)
 		spin_lock_bh(&sk->sk_lock.slock);
 	}
 
-	/* be sure to set the current sk state before tacking actions
-	 * depending on sk_state
-	 */
-	if (__test_and_clear_bit(MPTCP_CONNECTED, &msk->cb_flags))
-		__mptcp_set_connected(sk);
 	if (__test_and_clear_bit(MPTCP_CLEAN_UNA, &msk->cb_flags))
 		__mptcp_clean_una_wakeup(sk);
-	if (__test_and_clear_bit(MPTCP_ERROR_REPORT, &msk->cb_flags))
-		__mptcp_error_report(sk);
+	if (unlikely(&msk->cb_flags)) {
+		/* be sure to set the current sk state before tacking actions
+		 * depending on sk_state, that is processing MPTCP_ERROR_REPORT
+		 */
+		if (__test_and_clear_bit(MPTCP_CONNECTED, &msk->cb_flags))
+			__mptcp_set_connected(sk);
+		if (__test_and_clear_bit(MPTCP_ERROR_REPORT, &msk->cb_flags))
+			__mptcp_error_report(sk);
+	}
 
 	__mptcp_update_rmem(sk);
 }
-- 
2.35.1

[PATCH mptcp-next 2/3] mptcp: reset the packet scheduler on incoming MP_PRIO.

[Paolo Abeni]

When an incoming MP_PRIO option changes the backup
status of any subflow, we need to reset the packet
scheduler status, or the next send could keep using
the previously selected subflow, without taking in account
the new priorities.

Reported-by: Davide Caratti <dcaratti@redhat.com>
Fixes: 40453a5c61f4 ("mptcp: add the incoming MP_PRIO support")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/pm.c       | 19 +++++++++++++++----
 net/mptcp/protocol.c |  2 ++
 net/mptcp/protocol.h |  1 +
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index d0d31d5c198a..2f6ee422c6df 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -262,14 +262,25 @@ void mptcp_pm_rm_addr_received(struct mptcp_sock *msk,
 	spin_unlock_bh(&pm->lock);
 }
 
-void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
+void mptcp_pm_mp_prio_received(struct sock *ssk, u8 bkup)
 {
-	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
+	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
+	struct sock *sk = subflow->conn;
+	struct mptcp_sock *msk;
 
 	pr_debug("subflow->backup=%d, bkup=%d\n", subflow->backup, bkup);
-	subflow->backup = bkup;
+	msk = mptcp_sk(sk);
+	if (subflow->backup == bkup) {
+		subflow->backup = bkup;
+		mptcp_data_lock(sk);
+		if (!sock_owned_by_user(sk))
+			msk->last_snd = NULL;
+		else
+			__set_bit(MPTCP_RESET_SCHEDULER,  &msk->cb_flags);
+		mptcp_data_unlock(sk);
+	}
 
-	mptcp_event(MPTCP_EVENT_SUB_PRIORITY, mptcp_sk(subflow->conn), sk, GFP_ATOMIC);
+	mptcp_event(MPTCP_EVENT_SUB_PRIORITY, msk, ssk, GFP_ATOMIC);
 }
 
 void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index bdba1ddee2a7..cec42bc16d12 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3128,6 +3128,8 @@ static void mptcp_release_cb(struct sock *sk)
 			__mptcp_set_connected(sk);
 		if (__test_and_clear_bit(MPTCP_ERROR_REPORT, &msk->cb_flags))
 			__mptcp_error_report(sk);
+		if (__test_and_clear_bit(MPTCP_RESET_SCHEDULER, &msk->cb_flags))
+			msk->last_snd = NULL;
 	}
 
 	__mptcp_update_rmem(sk);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index c8bada4537e2..d59c16d248a4 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -124,6 +124,7 @@
 #define MPTCP_RETRANSMIT	4
 #define MPTCP_FLUSH_JOIN_LIST	5
 #define MPTCP_CONNECTED		6
+#define MPTCP_RESET_SCHEDULER	7
 
 static inline bool before64(__u64 seq1, __u64 seq2)
 {
-- 
2.35.1

[PATCH mptcp-next 3/3] mptcp: reset the packet scheduler on PRIO change

[Paolo Abeni]

Similar to the previous patch, for priority changes
requested by the local PM.

Reported-and-suggested-by: Davide Caratti <dcaratti@redhat.com>
Fixes: 067065422fcd ("mptcp: add the outgoing MP_PRIO support")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/pm_netlink.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index c1f4befb1e45..10a73898c8db 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -727,6 +727,8 @@ static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
 		if (!addresses_equal(&local, addr, addr->port))
 			continue;
 
+		if (subflow->backup != bkup)
+			msk->last_snd = NULL;
 		subflow->backup = bkup;
 		subflow->send_mp_prio = 1;
 		subflow->request_bkup = bkup;
-- 
2.35.1

Re: [PATCH mptcp-next 2/3] mptcp: reset the packet scheduler on incoming MP_PRIO.

[Mat Martineau]

On Fri, 18 Mar 2022, Paolo Abeni wrote:

> When an incoming MP_PRIO option changes the backup
> status of any subflow, we need to reset the packet
> scheduler status, or the next send could keep using
> the previously selected subflow, without taking in account
> the new priorities.
>
> Reported-by: Davide Caratti <dcaratti@redhat.com>
> Fixes: 40453a5c61f4 ("mptcp: add the incoming MP_PRIO support")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/pm.c       | 19 +++++++++++++++----
> net/mptcp/protocol.c |  2 ++
> net/mptcp/protocol.h |  1 +
> 3 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index d0d31d5c198a..2f6ee422c6df 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -262,14 +262,25 @@ void mptcp_pm_rm_addr_received(struct mptcp_sock *msk,
> 	spin_unlock_bh(&pm->lock);
> }
>
> -void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> +void mptcp_pm_mp_prio_received(struct sock *ssk, u8 bkup)
> {
> -	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> +	struct sock *sk = subflow->conn;
> +	struct mptcp_sock *msk;
>
> 	pr_debug("subflow->backup=%d, bkup=%d\n", subflow->backup, bkup);
> -	subflow->backup = bkup;
> +	msk = mptcp_sk(sk);
> +	if (subflow->backup == bkup) {

Above should be "if (subflow->backup != bkup)"?

> +		subflow->backup = bkup;
> +		mptcp_data_lock(sk);
> +		if (!sock_owned_by_user(sk))
> +			msk->last_snd = NULL;
> +		else
> +			__set_bit(MPTCP_RESET_SCHEDULER,  &msk->cb_flags);
> +		mptcp_data_unlock(sk);
> +	}
>
> -	mptcp_event(MPTCP_EVENT_SUB_PRIORITY, mptcp_sk(subflow->conn), sk, GFP_ATOMIC);
> +	mptcp_event(MPTCP_EVENT_SUB_PRIORITY, msk, ssk, GFP_ATOMIC);
> }
>
> void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index bdba1ddee2a7..cec42bc16d12 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3128,6 +3128,8 @@ static void mptcp_release_cb(struct sock *sk)
> 			__mptcp_set_connected(sk);
> 		if (__test_and_clear_bit(MPTCP_ERROR_REPORT, &msk->cb_flags))
> 			__mptcp_error_report(sk);
> +		if (__test_and_clear_bit(MPTCP_RESET_SCHEDULER, &msk->cb_flags))
> +			msk->last_snd = NULL;
> 	}
>
> 	__mptcp_update_rmem(sk);
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index c8bada4537e2..d59c16d248a4 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -124,6 +124,7 @@
> #define MPTCP_RETRANSMIT	4
> #define MPTCP_FLUSH_JOIN_LIST	5
> #define MPTCP_CONNECTED		6
> +#define MPTCP_RESET_SCHEDULER	7
>
> static inline bool before64(__u64 seq1, __u64 seq2)
> {
> -- 
> 2.35.1



--
Mat Martineau
Intel

Re: [PATCH mptcp-next 1/3] mptcp: optimize release_cb for the common case

[Mat Martineau]

On Fri, 18 Mar 2022, Paolo Abeni wrote:

> The mptcp release callback checks several flags in atomic
> context, but only MPTCP_CLEAN_UNA can be up frequently.
>
> Reorganize the code to avoid multiple conditionals in the
> most common scenarios.
>
> Additional clarify a related comment.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/protocol.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>

This one LGTM:

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index fbb14dfe62b3..bdba1ddee2a7 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3118,15 +3118,17 @@ static void mptcp_release_cb(struct sock *sk)
> 		spin_lock_bh(&sk->sk_lock.slock);
> 	}
>
> -	/* be sure to set the current sk state before tacking actions
> -	 * depending on sk_state
> -	 */
> -	if (__test_and_clear_bit(MPTCP_CONNECTED, &msk->cb_flags))
> -		__mptcp_set_connected(sk);
> 	if (__test_and_clear_bit(MPTCP_CLEAN_UNA, &msk->cb_flags))
> 		__mptcp_clean_una_wakeup(sk);
> -	if (__test_and_clear_bit(MPTCP_ERROR_REPORT, &msk->cb_flags))
> -		__mptcp_error_report(sk);
> +	if (unlikely(&msk->cb_flags)) {
> +		/* be sure to set the current sk state before tacking actions
> +		 * depending on sk_state, that is processing MPTCP_ERROR_REPORT
> +		 */
> +		if (__test_and_clear_bit(MPTCP_CONNECTED, &msk->cb_flags))
> +			__mptcp_set_connected(sk);
> +		if (__test_and_clear_bit(MPTCP_ERROR_REPORT, &msk->cb_flags))
> +			__mptcp_error_report(sk);
> +	}
>
> 	__mptcp_update_rmem(sk);
> }
> -- 
> 2.35.1
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next 3/3] mptcp: reset the packet scheduler on PRIO change

[Mat Martineau]

On Fri, 18 Mar 2022, Paolo Abeni wrote:

> Similar to the previous patch, for priority changes
> requested by the local PM.
>
> Reported-and-suggested-by: Davide Caratti <dcaratti@redhat.com>
> Fixes: 067065422fcd ("mptcp: add the outgoing MP_PRIO support")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/pm_netlink.c | 2 ++
> 1 file changed, 2 insertions(+)
>

Looks good, thanks:

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index c1f4befb1e45..10a73898c8db 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -727,6 +727,8 @@ static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
> 		if (!addresses_equal(&local, addr, addr->port))
> 			continue;
>
> +		if (subflow->backup != bkup)
> +			msk->last_snd = NULL;
> 		subflow->backup = bkup;
> 		subflow->send_mp_prio = 1;
> 		subflow->request_bkup = bkup;
> -- 
> 2.35.1
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next 2/3] mptcp: reset the packet scheduler on incoming MP_PRIO.

[Paolo Abeni]

On Mon, 2022-03-21 at 17:21 -0700, Mat Martineau wrote:
> On Fri, 18 Mar 2022, Paolo Abeni wrote:
> 
> > When an incoming MP_PRIO option changes the backup
> > status of any subflow, we need to reset the packet
> > scheduler status, or the next send could keep using
> > the previously selected subflow, without taking in account
> > the new priorities.
> > 
> > Reported-by: Davide Caratti <dcaratti@redhat.com>
> > Fixes: 40453a5c61f4 ("mptcp: add the incoming MP_PRIO support")
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > net/mptcp/pm.c       | 19 +++++++++++++++----
> > net/mptcp/protocol.c |  2 ++
> > net/mptcp/protocol.h |  1 +
> > 3 files changed, 18 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > index d0d31d5c198a..2f6ee422c6df 100644
> > --- a/net/mptcp/pm.c
> > +++ b/net/mptcp/pm.c
> > @@ -262,14 +262,25 @@ void mptcp_pm_rm_addr_received(struct mptcp_sock *msk,
> > 	spin_unlock_bh(&pm->lock);
> > }
> > 
> > -void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> > +void mptcp_pm_mp_prio_received(struct sock *ssk, u8 bkup)
> > {
> > -	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> > +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> > +	struct sock *sk = subflow->conn;
> > +	struct mptcp_sock *msk;
> > 
> > 	pr_debug("subflow->backup=%d, bkup=%d\n", subflow->backup, bkup);
> > -	subflow->backup = bkup;
> > +	msk = mptcp_sk(sk);
> > +	if (subflow->backup == bkup) {
> 
> Above should be "if (subflow->backup != bkup)"?

Ouch, my bad! You are right, v2 is coming...

/P