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

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

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
> 

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