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

[Geliang Tang <geliang.tang@suse.com>]

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
>