[PATCH mptcp-next v4 0/2] mptcp: refactor MP_FAIL response

[PATCH mptcp-next v4 0/2] mptcp: refactor MP_FAIL response

[Geliang Tang]

v4:
 - rename fail_sock to fail_ssk, fail_timeout to fail_tout.
 - move fail_ssk and fail_tout check from mptcp_mp_fail_no_response to
mptcp_worker
 - split into two patches.

Geliang Tang (2):
  mptcp: refactor MP_FAIL response timeout
  mptcp: trace MP_FAIL subflow in mptcp_sock

 net/mptcp/pm.c       |  7 ++-----
 net/mptcp/protocol.c | 37 +++++++++++--------------------------
 net/mptcp/protocol.h |  3 ++-
 net/mptcp/subflow.c  | 12 +++---------
 4 files changed, 18 insertions(+), 41 deletions(-)

-- 
2.35.3

[PATCH mptcp-next v4 1/2] mptcp: refactor MP_FAIL response timeout

[Geliang Tang]

mptcp_mp_fail_no_response shouldn't be invoked on each worker run, it
should be invoked only when MP_FAIL response timeout occurs.

This patch refactors the MP_FAIL response timeout logic. Add fail_tout
in mptcp_sock to record the MP_FAIL timestamp. Check it in mptcp_worker()
before invoking mptcp_mp_fail_no_response(). Drop the code to reuse
sk_timer for MP_FAIL response.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/281
Fixes: d9fb797046c5 ("mptcp: Do not traverse the subflow connection list without lock")
Acked-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm.c       |  5 +----
 net/mptcp/protocol.c |  4 +++-
 net/mptcp/protocol.h |  1 +
 net/mptcp/subflow.c  | 10 ++--------
 4 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 59a85220edc9..45a9e02abf24 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -299,7 +299,6 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
 	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
-	struct sock *s = (struct sock *)msk;
 
 	pr_debug("fail_seq=%llu", fail_seq);
 
@@ -312,10 +311,8 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
 		subflow->send_mp_fail = 1;
 		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPFAILTX);
 		subflow->send_infinite_map = 1;
-	} else if (!sock_flag(sk, SOCK_DEAD)) {
+	} else {
 		pr_debug("MP_FAIL response received");
-
-		sk_stop_timer(s, &s->sk_timer);
 	}
 }
 
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index d6aef4b13b8a..917df5fb9708 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2562,7 +2562,8 @@ static void mptcp_worker(struct work_struct *work)
 	if (test_and_clear_bit(MPTCP_WORK_RTX, &msk->flags))
 		__mptcp_retrans(sk);
 
-	mptcp_mp_fail_no_response(msk);
+	if (time_after(jiffies, msk->fail_tout))
+		mptcp_mp_fail_no_response(msk);
 
 unlock:
 	release_sock(sk);
@@ -2589,6 +2590,7 @@ static int __mptcp_init_sock(struct sock *sk)
 	WRITE_ONCE(msk->csum_enabled, mptcp_is_checksum_enabled(sock_net(sk)));
 	WRITE_ONCE(msk->allow_infinite_fallback, true);
 	msk->recovery = false;
+	msk->fail_tout = 0;
 
 	mptcp_pm_data_init(msk);
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index d406b5afbee4..f7b01275af94 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -306,6 +306,7 @@ struct mptcp_sock {
 
 	u32 setsockopt_seq;
 	char		ca_name[TCP_CA_NAME_MAX];
+	unsigned long	fail_tout;
 };
 
 #define mptcp_data_lock(sk) spin_lock_bh(&(sk)->sk_lock.slock)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 8841e8cd9ad8..866d54a0e83c 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -974,7 +974,6 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
 	bool csum_reqd = READ_ONCE(msk->csum_enabled);
-	struct sock *sk = (struct sock *)msk;
 	struct mptcp_ext *mpext;
 	struct sk_buff *skb;
 	u16 data_len;
@@ -1016,9 +1015,6 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
 		pr_debug("infinite mapping received");
 		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_INFINITEMAPRX);
 		subflow->map_data_len = 0;
-		if (!sock_flag(ssk, SOCK_DEAD))
-			sk_stop_timer(sk, &sk->sk_timer);
-
 		return MAPPING_INVALID;
 	}
 
@@ -1238,11 +1234,9 @@ static bool subflow_check_data_avail(struct sock *ssk)
 				tcp_send_active_reset(ssk, GFP_ATOMIC);
 				while ((skb = skb_peek(&ssk->sk_receive_queue)))
 					sk_eat_skb(ssk, skb);
-			} else if (!sock_flag(ssk, SOCK_DEAD)) {
+			} else {
 				WRITE_ONCE(subflow->mp_fail_response_expect, true);
-				sk_reset_timer((struct sock *)msk,
-					       &((struct sock *)msk)->sk_timer,
-					       jiffies + TCP_RTO_MAX);
+				msk->fail_tout = jiffies + TCP_RTO_MAX;
 			}
 			WRITE_ONCE(subflow->data_avail, MPTCP_SUBFLOW_NODATA);
 			return true;
-- 
2.35.3

Re: [PATCH mptcp-next v4 1/2] mptcp: refactor MP_FAIL response timeout

[Paolo Abeni]

On Thu, 2022-06-09 at 08:00 +0800, Geliang Tang wrote:
> mptcp_mp_fail_no_response shouldn't be invoked on each worker run, it
> should be invoked only when MP_FAIL response timeout occurs.
> 
> This patch refactors the MP_FAIL response timeout logic. Add fail_tout
> in mptcp_sock to record the MP_FAIL timestamp. Check it in mptcp_worker()
> before invoking mptcp_mp_fail_no_response(). Drop the code to reuse
> sk_timer for MP_FAIL response.

I'm sorry for not noticing it on the previous version - likely for the
lack of clarity on my side: we still need to start the timer for
mp_fail response: otherwise we are not assured that the mptcp_worker
will be triggered.

Additionally we need some more care to manipulate the timer reset. I
think it's easier if I send a squash-to patch - assuming I'll have some
time for that early next week.

/P

Re: [PATCH mptcp-next v4 1/2] mptcp: refactor MP_FAIL response timeout

[Mat Martineau]

On Thu, 9 Jun 2022, Paolo Abeni wrote:

> On Thu, 2022-06-09 at 08:00 +0800, Geliang Tang wrote:
>> mptcp_mp_fail_no_response shouldn't be invoked on each worker run, it
>> should be invoked only when MP_FAIL response timeout occurs.
>>
>> This patch refactors the MP_FAIL response timeout logic. Add fail_tout
>> in mptcp_sock to record the MP_FAIL timestamp. Check it in mptcp_worker()
>> before invoking mptcp_mp_fail_no_response(). Drop the code to reuse
>> sk_timer for MP_FAIL response.
>
> I'm sorry for not noticing it on the previous version - likely for the
> lack of clarity on my side: we still need to start the timer for
> mp_fail response: otherwise we are not assured that the mptcp_worker
> will be triggered.
>

(capturing this on the mailing list in addition to the recent meeting)

Yes, I agree. I saw that the timer stop calls were removed but missed the 
timer reset removal. It is imporant for the timer to still be started.

> Additionally we need some more care to manipulate the timer reset. I
> think it's easier if I send a squash-to patch - assuming I'll have some
> time for that early next week.
>

Thanks Paolo!

--
Mat Martineau
Intel

[PATCH mptcp-next v4 2/2] mptcp: trace MP_FAIL subflow in mptcp_sock

[Geliang Tang]

This patch adds fail_ssk struct member in struct mptcp_sock to record
the MP_FAIL subsocket. It can replace the mp_fail_response_expect flag
in struct mptcp_subflow_context.

Drop mp_fail_response_expect_subflow() helper too, just use this fail_ssk
in mptcp_mp_fail_no_response() to reset the subflow.

Acked-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm.c       |  2 +-
 net/mptcp/protocol.c | 35 +++++++++--------------------------
 net/mptcp/protocol.h |  2 +-
 net/mptcp/subflow.c  |  2 +-
 4 files changed, 12 insertions(+), 29 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 45a9e02abf24..2a57d95d5492 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -305,7 +305,7 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
 	if (!READ_ONCE(msk->allow_infinite_fallback))
 		return;
 
-	if (!READ_ONCE(subflow->mp_fail_response_expect)) {
+	if (!msk->fail_ssk) {
 		pr_debug("send MP_FAIL response and infinite map");
 
 		subflow->send_mp_fail = 1;
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 917df5fb9708..58427fabb061 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2167,21 +2167,6 @@ static void mptcp_retransmit_timer(struct timer_list *t)
 	sock_put(sk);
 }
 
-static struct mptcp_subflow_context *
-mp_fail_response_expect_subflow(struct mptcp_sock *msk)
-{
-	struct mptcp_subflow_context *subflow, *ret = NULL;
-
-	mptcp_for_each_subflow(msk, subflow) {
-		if (READ_ONCE(subflow->mp_fail_response_expect)) {
-			ret = subflow;
-			break;
-		}
-	}
-
-	return ret;
-}
-
 static void mptcp_timeout_timer(struct timer_list *t)
 {
 	struct sock *sk = from_timer(sk, t, sk_timer);
@@ -2507,19 +2492,16 @@ static void __mptcp_retrans(struct sock *sk)
 
 static void mptcp_mp_fail_no_response(struct mptcp_sock *msk)
 {
-	struct mptcp_subflow_context *subflow;
-	struct sock *ssk;
+	struct sock *ssk = msk->fail_ssk;
 	bool slow;
 
-	subflow = mp_fail_response_expect_subflow(msk);
-	if (subflow) {
-		pr_debug("MP_FAIL doesn't respond, reset the subflow");
+	pr_debug("MP_FAIL doesn't respond, reset the subflow");
 
-		ssk = mptcp_subflow_tcp_sock(subflow);
-		slow = lock_sock_fast(ssk);
-		mptcp_subflow_reset(ssk);
-		unlock_sock_fast(ssk, slow);
-	}
+	slow = lock_sock_fast(ssk);
+	mptcp_subflow_reset(ssk);
+	unlock_sock_fast(ssk, slow);
+
+	msk->fail_ssk = NULL;
 }
 
 static void mptcp_worker(struct work_struct *work)
@@ -2562,7 +2544,7 @@ static void mptcp_worker(struct work_struct *work)
 	if (test_and_clear_bit(MPTCP_WORK_RTX, &msk->flags))
 		__mptcp_retrans(sk);
 
-	if (time_after(jiffies, msk->fail_tout))
+	if (msk->fail_ssk && time_after(jiffies, msk->fail_tout))
 		mptcp_mp_fail_no_response(msk);
 
 unlock:
@@ -2590,6 +2572,7 @@ static int __mptcp_init_sock(struct sock *sk)
 	WRITE_ONCE(msk->csum_enabled, mptcp_is_checksum_enabled(sock_net(sk)));
 	WRITE_ONCE(msk->allow_infinite_fallback, true);
 	msk->recovery = false;
+	msk->fail_ssk = NULL;
 	msk->fail_tout = 0;
 
 	mptcp_pm_data_init(msk);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index f7b01275af94..bef7dea9f358 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -306,6 +306,7 @@ struct mptcp_sock {
 
 	u32 setsockopt_seq;
 	char		ca_name[TCP_CA_NAME_MAX];
+	struct sock	*fail_ssk;
 	unsigned long	fail_tout;
 };
 
@@ -469,7 +470,6 @@ struct mptcp_subflow_context {
 		local_id_valid : 1, /* local_id is correctly initialized */
 		valid_csum_seen : 1;        /* at least one csum validated */
 	enum mptcp_data_avail data_avail;
-	bool	mp_fail_response_expect;
 	bool	scheduled;
 	u32	remote_nonce;
 	u64	thmac;
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 866d54a0e83c..5351d54e514a 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1235,7 +1235,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
 				while ((skb = skb_peek(&ssk->sk_receive_queue)))
 					sk_eat_skb(ssk, skb);
 			} else {
-				WRITE_ONCE(subflow->mp_fail_response_expect, true);
+				msk->fail_ssk = ssk;
 				msk->fail_tout = jiffies + TCP_RTO_MAX;
 			}
 			WRITE_ONCE(subflow->data_avail, MPTCP_SUBFLOW_NODATA);
-- 
2.35.3

Re: [PATCH mptcp-next v4 2/2] mptcp: trace MP_FAIL subflow in mptcp_sock

[Mat Martineau]

On Thu, 9 Jun 2022, Geliang Tang wrote:

> This patch adds fail_ssk struct member in struct mptcp_sock to record
> the MP_FAIL subsocket. It can replace the mp_fail_response_expect flag
> in struct mptcp_subflow_context.
>
> Drop mp_fail_response_expect_subflow() helper too, just use this fail_ssk
> in mptcp_mp_fail_no_response() to reset the subflow.
>
> Acked-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> net/mptcp/pm.c       |  2 +-
> net/mptcp/protocol.c | 35 +++++++++--------------------------
> net/mptcp/protocol.h |  2 +-
> net/mptcp/subflow.c  |  2 +-
> 4 files changed, 12 insertions(+), 29 deletions(-)
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 45a9e02abf24..2a57d95d5492 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -305,7 +305,7 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
> 	if (!READ_ONCE(msk->allow_infinite_fallback))
> 		return;
>
> -	if (!READ_ONCE(subflow->mp_fail_response_expect)) {
> +	if (!msk->fail_ssk) {
> 		pr_debug("send MP_FAIL response and infinite map");
>
> 		subflow->send_mp_fail = 1;
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 917df5fb9708..58427fabb061 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2167,21 +2167,6 @@ static void mptcp_retransmit_timer(struct timer_list *t)
> 	sock_put(sk);
> }
>
> -static struct mptcp_subflow_context *
> -mp_fail_response_expect_subflow(struct mptcp_sock *msk)
> -{
> -	struct mptcp_subflow_context *subflow, *ret = NULL;
> -
> -	mptcp_for_each_subflow(msk, subflow) {
> -		if (READ_ONCE(subflow->mp_fail_response_expect)) {
> -			ret = subflow;
> -			break;
> -		}
> -	}
> -
> -	return ret;
> -}
> -
> static void mptcp_timeout_timer(struct timer_list *t)
> {
> 	struct sock *sk = from_timer(sk, t, sk_timer);
> @@ -2507,19 +2492,16 @@ static void __mptcp_retrans(struct sock *sk)
>
> static void mptcp_mp_fail_no_response(struct mptcp_sock *msk)
> {
> -	struct mptcp_subflow_context *subflow;
> -	struct sock *ssk;
> +	struct sock *ssk = msk->fail_ssk;
> 	bool slow;
>
> -	subflow = mp_fail_response_expect_subflow(msk);
> -	if (subflow) {
> -		pr_debug("MP_FAIL doesn't respond, reset the subflow");
> +	pr_debug("MP_FAIL doesn't respond, reset the subflow");
>
> -		ssk = mptcp_subflow_tcp_sock(subflow);
> -		slow = lock_sock_fast(ssk);
> -		mptcp_subflow_reset(ssk);
> -		unlock_sock_fast(ssk, slow);
> -	}
> +	slow = lock_sock_fast(ssk);
> +	mptcp_subflow_reset(ssk);
> +	unlock_sock_fast(ssk, slow);
> +
> +	msk->fail_ssk = NULL;
> }
>
> static void mptcp_worker(struct work_struct *work)
> @@ -2562,7 +2544,7 @@ static void mptcp_worker(struct work_struct *work)
> 	if (test_and_clear_bit(MPTCP_WORK_RTX, &msk->flags))
> 		__mptcp_retrans(sk);
>
> -	if (time_after(jiffies, msk->fail_tout))

Hi Geliang -

This condition could be unexpectedly true depending on how close 'jiffies' 
is to wrapping around, if msk->fail_tout remains at its default 0 
value...

> +	if (msk->fail_ssk && time_after(jiffies, msk->fail_tout))

...so it's important to fix it like this!

I think the two patches should be re-squashed to avoid introducing the 
issue in the previous commit. Sound good?

Also, I think the change should be for mptcp-net so we can get the fix in 
to 5.19-rcX

Thanks!

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


> 		mptcp_mp_fail_no_response(msk);
>
> unlock:
> @@ -2590,6 +2572,7 @@ static int __mptcp_init_sock(struct sock *sk)
> 	WRITE_ONCE(msk->csum_enabled, mptcp_is_checksum_enabled(sock_net(sk)));
> 	WRITE_ONCE(msk->allow_infinite_fallback, true);
> 	msk->recovery = false;
> +	msk->fail_ssk = NULL;
> 	msk->fail_tout = 0;
>
> 	mptcp_pm_data_init(msk);
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index f7b01275af94..bef7dea9f358 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -306,6 +306,7 @@ struct mptcp_sock {
>
> 	u32 setsockopt_seq;
> 	char		ca_name[TCP_CA_NAME_MAX];
> +	struct sock	*fail_ssk;
> 	unsigned long	fail_tout;
> };
>
> @@ -469,7 +470,6 @@ struct mptcp_subflow_context {
> 		local_id_valid : 1, /* local_id is correctly initialized */
> 		valid_csum_seen : 1;        /* at least one csum validated */
> 	enum mptcp_data_avail data_avail;
> -	bool	mp_fail_response_expect;
> 	bool	scheduled;
> 	u32	remote_nonce;
> 	u64	thmac;
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 866d54a0e83c..5351d54e514a 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1235,7 +1235,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
> 				while ((skb = skb_peek(&ssk->sk_receive_queue)))
> 					sk_eat_skb(ssk, skb);
> 			} else {
> -				WRITE_ONCE(subflow->mp_fail_response_expect, true);
> +				msk->fail_ssk = ssk;
> 				msk->fail_tout = jiffies + TCP_RTO_MAX;
> 			}
> 			WRITE_ONCE(subflow->data_avail, MPTCP_SUBFLOW_NODATA);
> -- 
> 2.35.3
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next v4 2/2] mptcp: trace MP_FAIL subflow in mptcp_sock

[Geliang Tang]

On Wed, Jun 08, 2022 at 05:31:00PM -0700, Mat Martineau wrote:
> On Thu, 9 Jun 2022, Geliang Tang wrote:
> 
> > This patch adds fail_ssk struct member in struct mptcp_sock to record
> > the MP_FAIL subsocket. It can replace the mp_fail_response_expect flag
> > in struct mptcp_subflow_context.
> > 
> > Drop mp_fail_response_expect_subflow() helper too, just use this fail_ssk
> > in mptcp_mp_fail_no_response() to reset the subflow.
> > 
> > Acked-by: Paolo Abeni <pabeni@redhat.com>
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > ---
> > net/mptcp/pm.c       |  2 +-
> > net/mptcp/protocol.c | 35 +++++++++--------------------------
> > net/mptcp/protocol.h |  2 +-
> > net/mptcp/subflow.c  |  2 +-
> > 4 files changed, 12 insertions(+), 29 deletions(-)
> > 
> > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > index 45a9e02abf24..2a57d95d5492 100644
> > --- a/net/mptcp/pm.c
> > +++ b/net/mptcp/pm.c
> > @@ -305,7 +305,7 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
> > 	if (!READ_ONCE(msk->allow_infinite_fallback))
> > 		return;
> > 
> > -	if (!READ_ONCE(subflow->mp_fail_response_expect)) {
> > +	if (!msk->fail_ssk) {
> > 		pr_debug("send MP_FAIL response and infinite map");
> > 
> > 		subflow->send_mp_fail = 1;
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 917df5fb9708..58427fabb061 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -2167,21 +2167,6 @@ static void mptcp_retransmit_timer(struct timer_list *t)
> > 	sock_put(sk);
> > }
> > 
> > -static struct mptcp_subflow_context *
> > -mp_fail_response_expect_subflow(struct mptcp_sock *msk)
> > -{
> > -	struct mptcp_subflow_context *subflow, *ret = NULL;
> > -
> > -	mptcp_for_each_subflow(msk, subflow) {
> > -		if (READ_ONCE(subflow->mp_fail_response_expect)) {
> > -			ret = subflow;
> > -			break;
> > -		}
> > -	}
> > -
> > -	return ret;
> > -}
> > -
> > static void mptcp_timeout_timer(struct timer_list *t)
> > {
> > 	struct sock *sk = from_timer(sk, t, sk_timer);
> > @@ -2507,19 +2492,16 @@ static void __mptcp_retrans(struct sock *sk)
> > 
> > static void mptcp_mp_fail_no_response(struct mptcp_sock *msk)
> > {
> > -	struct mptcp_subflow_context *subflow;
> > -	struct sock *ssk;
> > +	struct sock *ssk = msk->fail_ssk;
> > 	bool slow;
> > 
> > -	subflow = mp_fail_response_expect_subflow(msk);
> > -	if (subflow) {
> > -		pr_debug("MP_FAIL doesn't respond, reset the subflow");
> > +	pr_debug("MP_FAIL doesn't respond, reset the subflow");
> > 
> > -		ssk = mptcp_subflow_tcp_sock(subflow);
> > -		slow = lock_sock_fast(ssk);
> > -		mptcp_subflow_reset(ssk);
> > -		unlock_sock_fast(ssk, slow);
> > -	}
> > +	slow = lock_sock_fast(ssk);
> > +	mptcp_subflow_reset(ssk);
> > +	unlock_sock_fast(ssk, slow);
> > +
> > +	msk->fail_ssk = NULL;
> > }
> > 
> > static void mptcp_worker(struct work_struct *work)
> > @@ -2562,7 +2544,7 @@ static void mptcp_worker(struct work_struct *work)
> > 	if (test_and_clear_bit(MPTCP_WORK_RTX, &msk->flags))
> > 		__mptcp_retrans(sk);
> > 
> > -	if (time_after(jiffies, msk->fail_tout))
> 
> Hi Geliang -
> 
> This condition could be unexpectedly true depending on how close 'jiffies'
> is to wrapping around, if msk->fail_tout remains at its default 0 value...
> 
> > +	if (msk->fail_ssk && time_after(jiffies, msk->fail_tout))
> 
> ...so it's important to fix it like this!
> 
> I think the two patches should be re-squashed to avoid introducing the issue
> in the previous commit. Sound good?

Sure. Please update the subject and commit log when squashing this patch:

'''
mptcp: refactor MP_FAIL response

mptcp_mp_fail_no_response shouldn't be invoked on each worker run, it
should be invoked only when MP_FAIL response timeout occurs.

This patch refactors the MP_FAIL response logic.

Add fail_tout in mptcp_sock to record the MP_FAIL timestamp. Check it
in mptcp_worker() before invoking mptcp_mp_fail_no_response(). Drop
the code to reuse sk_timer for MP_FAIL response.

Add fail_ssk struct member in struct mptcp_sock to record the MP_FAIL
subsocket. It can replace the mp_fail_response_expect flag in struct
mptcp_subflow_context. Drop mp_fail_response_expect_subflow() helper,
just use this fail_ssk in mptcp_mp_fail_no_response() to reset the
subflow.
'''

Thanks,
-Geliang

> 
> Also, I think the change should be for mptcp-net so we can get the fix in to
> 5.19-rcX
> 
> Thanks!
> 
> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> 
> 
> > 		mptcp_mp_fail_no_response(msk);
> > 
> > unlock:
> > @@ -2590,6 +2572,7 @@ static int __mptcp_init_sock(struct sock *sk)
> > 	WRITE_ONCE(msk->csum_enabled, mptcp_is_checksum_enabled(sock_net(sk)));
> > 	WRITE_ONCE(msk->allow_infinite_fallback, true);
> > 	msk->recovery = false;
> > +	msk->fail_ssk = NULL;
> > 	msk->fail_tout = 0;
> > 
> > 	mptcp_pm_data_init(msk);
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index f7b01275af94..bef7dea9f358 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -306,6 +306,7 @@ struct mptcp_sock {
> > 
> > 	u32 setsockopt_seq;
> > 	char		ca_name[TCP_CA_NAME_MAX];
> > +	struct sock	*fail_ssk;
> > 	unsigned long	fail_tout;
> > };
> > 
> > @@ -469,7 +470,6 @@ struct mptcp_subflow_context {
> > 		local_id_valid : 1, /* local_id is correctly initialized */
> > 		valid_csum_seen : 1;        /* at least one csum validated */
> > 	enum mptcp_data_avail data_avail;
> > -	bool	mp_fail_response_expect;
> > 	bool	scheduled;
> > 	u32	remote_nonce;
> > 	u64	thmac;
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index 866d54a0e83c..5351d54e514a 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -1235,7 +1235,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
> > 				while ((skb = skb_peek(&ssk->sk_receive_queue)))
> > 					sk_eat_skb(ssk, skb);
> > 			} else {
> > -				WRITE_ONCE(subflow->mp_fail_response_expect, true);
> > +				msk->fail_ssk = ssk;
> > 				msk->fail_tout = jiffies + TCP_RTO_MAX;
> > 			}
> > 			WRITE_ONCE(subflow->data_avail, MPTCP_SUBFLOW_NODATA);
> > -- 
> > 2.35.3
> > 
> > 
> > 
> 
> --
> Mat Martineau
> Intel
> 

Re: [PATCH mptcp-next v4 2/2] mptcp: trace MP_FAIL subflow in mptcp_sock

[Matthieu Baerts]

Hi Geliang, Mat, Paolo,

On 09/06/2022 03:23, Geliang Tang wrote:
> On Wed, Jun 08, 2022 at 05:31:00PM -0700, Mat Martineau wrote:
>> On Thu, 9 Jun 2022, Geliang Tang wrote:
>>
>>> This patch adds fail_ssk struct member in struct mptcp_sock to record
>>> the MP_FAIL subsocket. It can replace the mp_fail_response_expect flag
>>> in struct mptcp_subflow_context.
>>>
>>> Drop mp_fail_response_expect_subflow() helper too, just use this fail_ssk
>>> in mptcp_mp_fail_no_response() to reset the subflow.
>>>
>>> Acked-by: Paolo Abeni <pabeni@redhat.com>
>>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
>>> ---
>>> net/mptcp/pm.c       |  2 +-
>>> net/mptcp/protocol.c | 35 +++++++++--------------------------
>>> net/mptcp/protocol.h |  2 +-
>>> net/mptcp/subflow.c  |  2 +-
>>> 4 files changed, 12 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>>> index 45a9e02abf24..2a57d95d5492 100644
>>> --- a/net/mptcp/pm.c
>>> +++ b/net/mptcp/pm.c
>>> @@ -305,7 +305,7 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
>>> 	if (!READ_ONCE(msk->allow_infinite_fallback))
>>> 		return;
>>>
>>> -	if (!READ_ONCE(subflow->mp_fail_response_expect)) {
>>> +	if (!msk->fail_ssk) {
>>> 		pr_debug("send MP_FAIL response and infinite map");
>>>
>>> 		subflow->send_mp_fail = 1;
>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>> index 917df5fb9708..58427fabb061 100644
>>> --- a/net/mptcp/protocol.c
>>> +++ b/net/mptcp/protocol.c
>>> @@ -2167,21 +2167,6 @@ static void mptcp_retransmit_timer(struct timer_list *t)
>>> 	sock_put(sk);
>>> }
>>>
>>> -static struct mptcp_subflow_context *
>>> -mp_fail_response_expect_subflow(struct mptcp_sock *msk)
>>> -{
>>> -	struct mptcp_subflow_context *subflow, *ret = NULL;
>>> -
>>> -	mptcp_for_each_subflow(msk, subflow) {
>>> -		if (READ_ONCE(subflow->mp_fail_response_expect)) {
>>> -			ret = subflow;
>>> -			break;
>>> -		}
>>> -	}
>>> -
>>> -	return ret;
>>> -}
>>> -
>>> static void mptcp_timeout_timer(struct timer_list *t)
>>> {
>>> 	struct sock *sk = from_timer(sk, t, sk_timer);
>>> @@ -2507,19 +2492,16 @@ static void __mptcp_retrans(struct sock *sk)
>>>
>>> static void mptcp_mp_fail_no_response(struct mptcp_sock *msk)
>>> {
>>> -	struct mptcp_subflow_context *subflow;
>>> -	struct sock *ssk;
>>> +	struct sock *ssk = msk->fail_ssk;
>>> 	bool slow;
>>>
>>> -	subflow = mp_fail_response_expect_subflow(msk);
>>> -	if (subflow) {
>>> -		pr_debug("MP_FAIL doesn't respond, reset the subflow");
>>> +	pr_debug("MP_FAIL doesn't respond, reset the subflow");
>>>
>>> -		ssk = mptcp_subflow_tcp_sock(subflow);
>>> -		slow = lock_sock_fast(ssk);
>>> -		mptcp_subflow_reset(ssk);
>>> -		unlock_sock_fast(ssk, slow);
>>> -	}
>>> +	slow = lock_sock_fast(ssk);
>>> +	mptcp_subflow_reset(ssk);
>>> +	unlock_sock_fast(ssk, slow);
>>> +
>>> +	msk->fail_ssk = NULL;
>>> }
>>>
>>> static void mptcp_worker(struct work_struct *work)
>>> @@ -2562,7 +2544,7 @@ static void mptcp_worker(struct work_struct *work)
>>> 	if (test_and_clear_bit(MPTCP_WORK_RTX, &msk->flags))
>>> 		__mptcp_retrans(sk);
>>>
>>> -	if (time_after(jiffies, msk->fail_tout))
>>
>> Hi Geliang -
>>
>> This condition could be unexpectedly true depending on how close 'jiffies'
>> is to wrapping around, if msk->fail_tout remains at its default 0 value...
>>
>>> +	if (msk->fail_ssk && time_after(jiffies, msk->fail_tout))
>>
>> ...so it's important to fix it like this!
>>
>> I think the two patches should be re-squashed to avoid introducing the issue
>> in the previous commit. Sound good?
> 
> Sure. Please update the subject and commit log when squashing this patch:
> 
> '''
> mptcp: refactor MP_FAIL response
> 
> mptcp_mp_fail_no_response shouldn't be invoked on each worker run, it
> should be invoked only when MP_FAIL response timeout occurs.
> 
> This patch refactors the MP_FAIL response logic.
> 
> Add fail_tout in mptcp_sock to record the MP_FAIL timestamp. Check it
> in mptcp_worker() before invoking mptcp_mp_fail_no_response(). Drop
> the code to reuse sk_timer for MP_FAIL response.
> 
> Add fail_ssk struct member in struct mptcp_sock to record the MP_FAIL
> subsocket. It can replace the mp_fail_response_expect flag in struct
> mptcp_subflow_context. Drop mp_fail_response_expect_subflow() helper,
> just use this fail_ssk in mptcp_mp_fail_no_response() to reset the
> subflow.
> '''
> 
> Thanks,
> -Geliang
> 
>>
>> Also, I think the change should be for mptcp-net so we can get the fix in to
>> 5.19-rcX

Thank you for the patches and the reviews!

I just squashed these two patches and applied in "Fixed for -net" part.

@Mat: careful that there is a conflict when merging with net-next,
please see below:

I also renamed the subject because "mptcp: refactor MP_FAIL response"
didn't sound like a fix. Feel free to suggest better subject :)


- 9f0b1fb5ff57: mptcp: invoke MP_FAIL response when needed
- Results: 96aa91051266..5ffedd7d32e6 (export-net)

- a87560169aa5: conflict in t/mptcp-add-scheduled-in-mptcp_subflow_context
- Results: 91b7cc881cdd..19fb763aac71 (export)


Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220609T145706
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20220609T145706
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export-net

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: mptcp: trace MP_FAIL subflow in mptcp_sock: Tests Results

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Unstable: 1 failed test(s): packetdrill_add_addr 🔴:
  - Task: https://cirrus-ci.com/task/5388166994591744
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5388166994591744/summary/summary.txt

- KVM Validation: debug:
  - Unstable: 2 failed test(s): selftest_diag selftest_simult_flows 🔴:
  - Task: https://cirrus-ci.com/task/6514066901434368
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6514066901434368/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/1da22c975795


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)