[PATCH mptcp-next] Squash to "mptcp: implement fastclose xmit path"

Matthieu Baerts posted 1 patch 2 years, 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/20211130165527.1506827-1-matthieu.baerts@tessares.net
Maintainers: Jakub Kicinski <kuba@kernel.org>, Matthieu Baerts <matthieu.baerts@tessares.net>, "David S. Miller" <davem@davemloft.net>, Mat Martineau <mathew.j.martineau@linux.intel.com>
net/mptcp/options.c | 62 +++++++++++++++++++++++++++------------------
1 file changed, 37 insertions(+), 25 deletions(-)
[PATCH mptcp-next] Squash to "mptcp: implement fastclose xmit path"
Posted by Matthieu Baerts 2 years, 3 months ago
This is linked to Geliang's patch with the same title.

If I understand the RFC properly, we can use MP_RST with an MP_FASTCLOSE
and an MP_FAIL (or none of them) but we cannot use MP_FASTCLOSE and
MP_FAIL together.

This patch implements this logic in both mptcp_established_options() and
mptcp_write_options(). Before this patch, the first function was not
allowing any of these 3 options to be used at the same time while the
second one was allowing MP_FAIL to be used with either MP_FASTCLOSE or
MP_RST.

Small note: I tried to keep Paolo's idea of reducing conditions for
"normal" options by moving MP_RST at the end and allow to jump there
from MP_FAIL and MP_FASTCLOSE but I'm not sure it makes it easy to read
and really improves perfs. We can also move MP_RST check at the
beginning.

@Geliang: feel free to re-use this patch if it looks OK to you.

PS: I didn't try this patch, just quickly wrote it to better explain
what I had in mind.

Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Geliang Tang <geliang.tang@suse.com>
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---

Notes:
    to be squashed in "mptcp: implement fastclose xmit path"

 net/mptcp/options.c | 62 +++++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 25 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 8a1020e4285c..07091971a51a 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -829,11 +829,17 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
 
 	if (unlikely(skb && TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST)) {
 		if (mptcp_established_options_fastclose(sk, &opt_size, remaining, opts) ||
-		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts) ||
-		    mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
+		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts)) {
 			*size += opt_size;
 			remaining -= opt_size;
 		}
+
+		/* MP_RST can be used with MP_FASTCLOSE and MP_FAIL if there is room */
+		if (mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
+			*size += opt_size;
+			remaining -= opt_size;
+		}
+
 		return true;
 	}
 
@@ -1257,21 +1263,7 @@ static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
 void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 			 struct mptcp_out_options *opts)
 {
-	if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
-		const struct sock *ssk = (const struct sock *)tp;
-		struct mptcp_subflow_context *subflow;
-
-		subflow = mptcp_subflow_ctx(ssk);
-		subflow->send_mp_fail = 0;
-
-		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
-				      TCPOLEN_MPTCP_FAIL,
-				      0, 0);
-		put_unaligned_be64(opts->fail_seq, ptr);
-		ptr += 2;
-	}
-
-	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and RST are mutually exclusive,
+	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and FAIL are mutually exclusive,
 	 * see mptcp_established_options*()
 	 */
 	if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
@@ -1458,19 +1450,39 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 				ptr += 1;
 			}
 		}
-	} else if (unlikely(OPTION_MPTCP_RST & opts->suboptions)) {
-		/* RST is mutually exclusive with everything else */
-		*ptr++ = mptcp_option(MPTCPOPT_RST,
-				      TCPOLEN_MPTCP_RST,
-				      opts->reset_transient,
-				      opts->reset_reason);
-		return;
 	} else if (unlikely(OPTION_MPTCP_FASTCLOSE & opts->suboptions)) {
-		/* FASTCLOSE is mutually exclusive with everything else */
+		/* FASTCLOSE is mutually exclusive with others except RST */
 		*ptr++ = mptcp_option(MPTCPOPT_MP_FASTCLOSE,
 				      TCPOLEN_MPTCP_FASTCLOSE,
 				      0, 0);
 		put_unaligned_be64(opts->rcvr_key, ptr);
+
+		if (OPTION_MPTCP_RST & opts->suboptions)
+			goto mp_rst;
+		return;
+	} else if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
+		/* MP_FAIL is mutually exclusive with others except RST */
+		const struct sock *ssk = (const struct sock *)tp;
+		struct mptcp_subflow_context *subflow;
+
+		subflow = mptcp_subflow_ctx(ssk);
+		subflow->send_mp_fail = 0;
+
+		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
+				      TCPOLEN_MPTCP_FAIL,
+				      0, 0);
+		put_unaligned_be64(opts->fail_seq, ptr);
+		ptr += 2;
+
+		if (OPTION_MPTCP_RST & opts->suboptions)
+			goto mp_rst;
+		return
+	} else if (unlikely(OPTION_MPTCP_RST & opts->suboptions)) {
+mp_rst:
+		*ptr++ = mptcp_option(MPTCPOPT_RST,
+				      TCPOLEN_MPTCP_RST,
+				      opts->reset_transient,
+				      opts->reset_reason);
 		return;
 	}
 
-- 
2.32.0


Re: [PATCH mptcp-next] Squash to "mptcp: implement fastclose xmit path"
Posted by Paolo Abeni 2 years, 3 months ago
On Tue, 2021-11-30 at 17:55 +0100, Matthieu Baerts wrote:
> This is linked to Geliang's patch with the same title.
> 
> If I understand the RFC properly, we can use MP_RST with an MP_FASTCLOSE
> and an MP_FAIL (or none of them) but we cannot use MP_FASTCLOSE and
> MP_FAIL together.
> 
> This patch implements this logic in both mptcp_established_options() and
> mptcp_write_options(). Before this patch, the first function was not
> allowing any of these 3 options to be used at the same time while the
> second one was allowing MP_FAIL to be used with either MP_FASTCLOSE or
> MP_RST.
> 
> Small note: I tried to keep Paolo's idea of reducing conditions for
> "normal" options by moving MP_RST at the end and allow to jump there
> from MP_FAIL and MP_FASTCLOSE but I'm not sure it makes it easy to read
> and really improves perfs. We can also move MP_RST check at the
> beginning.
> 
> @Geliang: feel free to re-use this patch if it looks OK to you.
> 
> PS: I didn't try this patch, just quickly wrote it to better explain
> what I had in mind.
> 
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Geliang Tang <geliang.tang@suse.com>
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
> 
> Notes:
>     to be squashed in "mptcp: implement fastclose xmit path"
> 
>  net/mptcp/options.c | 62 +++++++++++++++++++++++++++------------------
>  1 file changed, 37 insertions(+), 25 deletions(-)
> 
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 8a1020e4285c..07091971a51a 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -829,11 +829,17 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
>  
>  	if (unlikely(skb && TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST)) {
>  		if (mptcp_established_options_fastclose(sk, &opt_size, remaining, opts) ||
> -		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts) ||
> -		    mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
> +		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts)) {
>  			*size += opt_size;
>  			remaining -= opt_size;
>  		}
> +
> +		/* MP_RST can be used with MP_FASTCLOSE and MP_FAIL if there is room */
> +		if (mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
> +			*size += opt_size;
> +			remaining -= opt_size;
> +		}
> +
>  		return true;
>  	}
>  
> @@ -1257,21 +1263,7 @@ static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
>  void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>  			 struct mptcp_out_options *opts)
>  {
> -	if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
> -		const struct sock *ssk = (const struct sock *)tp;
> -		struct mptcp_subflow_context *subflow;
> -
> -		subflow = mptcp_subflow_ctx(ssk);
> -		subflow->send_mp_fail = 0;
> -
> -		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
> -				      TCPOLEN_MPTCP_FAIL,
> -				      0, 0);
> -		put_unaligned_be64(opts->fail_seq, ptr);
> -		ptr += 2;
> -	}
> -
> -	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and RST are mutually exclusive,
> +	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and FAIL are mutually exclusive,
>  	 * see mptcp_established_options*()
>  	 */
>  	if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
> @@ -1458,19 +1450,39 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>  				ptr += 1;
>  			}
>  		}
> -	} else if (unlikely(OPTION_MPTCP_RST & opts->suboptions)) {
> -		/* RST is mutually exclusive with everything else */
> -		*ptr++ = mptcp_option(MPTCPOPT_RST,
> -				      TCPOLEN_MPTCP_RST,
> -				      opts->reset_transient,
> -				      opts->reset_reason);
> -		return;
>  	} else if (unlikely(OPTION_MPTCP_FASTCLOSE & opts->suboptions)) {
> -		/* FASTCLOSE is mutually exclusive with everything else */
> +		/* FASTCLOSE is mutually exclusive with others except RST */
>  		*ptr++ = mptcp_option(MPTCPOPT_MP_FASTCLOSE,
>  				      TCPOLEN_MPTCP_FASTCLOSE,
>  				      0, 0);
>  		put_unaligned_be64(opts->rcvr_key, ptr);
> +
> +		if (OPTION_MPTCP_RST & opts->suboptions)
> +			goto mp_rst;
> +		return;
> +	} else if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
> +		/* MP_FAIL is mutually exclusive with others except RST */
> +		const struct sock *ssk = (const struct sock *)tp;
> +		struct mptcp_subflow_context *subflow;
> +
> +		subflow = mptcp_subflow_ctx(ssk);
> +		subflow->send_mp_fail = 0;
> +
> +		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
> +				      TCPOLEN_MPTCP_FAIL,
> +				      0, 0);
> +		put_unaligned_be64(opts->fail_seq, ptr);
> +		ptr += 2;
> +
> +		if (OPTION_MPTCP_RST & opts->suboptions)
> +			goto mp_rst;
> +		return

As the CI spotted, missing ';' here.

Otherwise LGTM!

Thanks,

Paolo


Re: [PATCH mptcp-next] Squash to "mptcp: implement fastclose xmit path"
Posted by Matthieu Baerts 2 years, 3 months ago
Hi Paolo, Geliang,

On 01/12/2021 11:40, Paolo Abeni wrote:
> On Tue, 2021-11-30 at 17:55 +0100, Matthieu Baerts wrote:
>> This is linked to Geliang's patch with the same title.
>>
>> If I understand the RFC properly, we can use MP_RST with an MP_FASTCLOSE
>> and an MP_FAIL (or none of them) but we cannot use MP_FASTCLOSE and
>> MP_FAIL together.
>>
>> This patch implements this logic in both mptcp_established_options() and
>> mptcp_write_options(). Before this patch, the first function was not
>> allowing any of these 3 options to be used at the same time while the
>> second one was allowing MP_FAIL to be used with either MP_FASTCLOSE or
>> MP_RST.
>>
>> Small note: I tried to keep Paolo's idea of reducing conditions for
>> "normal" options by moving MP_RST at the end and allow to jump there
>> from MP_FAIL and MP_FASTCLOSE but I'm not sure it makes it easy to read
>> and really improves perfs. We can also move MP_RST check at the
>> beginning.
>>
>> @Geliang: feel free to re-use this patch if it looks OK to you.
>>
>> PS: I didn't try this patch, just quickly wrote it to better explain
>> what I had in mind.
>>
>> Cc: Paolo Abeni <pabeni@redhat.com>
>> Cc: Geliang Tang <geliang.tang@suse.com>
>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>> ---
>>
>> Notes:
>>     to be squashed in "mptcp: implement fastclose xmit path"
>>
>>  net/mptcp/options.c | 62 +++++++++++++++++++++++++++------------------
>>  1 file changed, 37 insertions(+), 25 deletions(-)
>>
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index 8a1020e4285c..07091971a51a 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -829,11 +829,17 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
>>  
>>  	if (unlikely(skb && TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST)) {
>>  		if (mptcp_established_options_fastclose(sk, &opt_size, remaining, opts) ||
>> -		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts) ||
>> -		    mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
>> +		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts)) {
>>  			*size += opt_size;
>>  			remaining -= opt_size;
>>  		}
>> +
>> +		/* MP_RST can be used with MP_FASTCLOSE and MP_FAIL if there is room */
>> +		if (mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
>> +			*size += opt_size;
>> +			remaining -= opt_size;
>> +		}
>> +
>>  		return true;
>>  	}
>>  
>> @@ -1257,21 +1263,7 @@ static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
>>  void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>  			 struct mptcp_out_options *opts)
>>  {
>> -	if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
>> -		const struct sock *ssk = (const struct sock *)tp;
>> -		struct mptcp_subflow_context *subflow;
>> -
>> -		subflow = mptcp_subflow_ctx(ssk);
>> -		subflow->send_mp_fail = 0;
>> -
>> -		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
>> -				      TCPOLEN_MPTCP_FAIL,
>> -				      0, 0);
>> -		put_unaligned_be64(opts->fail_seq, ptr);
>> -		ptr += 2;
>> -	}
>> -
>> -	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and RST are mutually exclusive,
>> +	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and FAIL are mutually exclusive,
>>  	 * see mptcp_established_options*()
>>  	 */
>>  	if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
>> @@ -1458,19 +1450,39 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>  				ptr += 1;
>>  			}
>>  		}
>> -	} else if (unlikely(OPTION_MPTCP_RST & opts->suboptions)) {
>> -		/* RST is mutually exclusive with everything else */
>> -		*ptr++ = mptcp_option(MPTCPOPT_RST,
>> -				      TCPOLEN_MPTCP_RST,
>> -				      opts->reset_transient,
>> -				      opts->reset_reason);
>> -		return;
>>  	} else if (unlikely(OPTION_MPTCP_FASTCLOSE & opts->suboptions)) {
>> -		/* FASTCLOSE is mutually exclusive with everything else */
>> +		/* FASTCLOSE is mutually exclusive with others except RST */
>>  		*ptr++ = mptcp_option(MPTCPOPT_MP_FASTCLOSE,
>>  				      TCPOLEN_MPTCP_FASTCLOSE,
>>  				      0, 0);
>>  		put_unaligned_be64(opts->rcvr_key, ptr);
>> +
>> +		if (OPTION_MPTCP_RST & opts->suboptions)
>> +			goto mp_rst;
>> +		return;
>> +	} else if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
>> +		/* MP_FAIL is mutually exclusive with others except RST */
>> +		const struct sock *ssk = (const struct sock *)tp;
>> +		struct mptcp_subflow_context *subflow;
>> +
>> +		subflow = mptcp_subflow_ctx(ssk);
>> +		subflow->send_mp_fail = 0;
>> +
>> +		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
>> +				      TCPOLEN_MPTCP_FAIL,
>> +				      0, 0);
>> +		put_unaligned_be64(opts->fail_seq, ptr);
>> +		ptr += 2;
>> +
>> +		if (OPTION_MPTCP_RST & opts->suboptions)
>> +			goto mp_rst;
>> +		return
> 
> As the CI spotted, missing ';' here.

Yes sorry, I quickly did it before having to go.

> Otherwise LGTM!

Thank you for the review!

@Geliang: do you prefer to use this diff -- without the missing ';' --
in a new version or do you prefer if I apply it with the fix directly?

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

Re: [PATCH mptcp-next] Squash to "mptcp: implement fastclose xmit path"
Posted by Mat Martineau 2 years, 3 months ago
On Wed, 1 Dec 2021, Matthieu Baerts wrote:

> Hi Paolo, Geliang,
>
> On 01/12/2021 11:40, Paolo Abeni wrote:
>> On Tue, 2021-11-30 at 17:55 +0100, Matthieu Baerts wrote:
>>> This is linked to Geliang's patch with the same title.
>>>
>>> If I understand the RFC properly, we can use MP_RST with an MP_FASTCLOSE
>>> and an MP_FAIL (or none of them) but we cannot use MP_FASTCLOSE and
>>> MP_FAIL together.
>>>
>>> This patch implements this logic in both mptcp_established_options() and
>>> mptcp_write_options(). Before this patch, the first function was not
>>> allowing any of these 3 options to be used at the same time while the
>>> second one was allowing MP_FAIL to be used with either MP_FASTCLOSE or
>>> MP_RST.
>>>
>>> Small note: I tried to keep Paolo's idea of reducing conditions for
>>> "normal" options by moving MP_RST at the end and allow to jump there
>>> from MP_FAIL and MP_FASTCLOSE but I'm not sure it makes it easy to read
>>> and really improves perfs. We can also move MP_RST check at the
>>> beginning.
>>>
>>> @Geliang: feel free to re-use this patch if it looks OK to you.
>>>
>>> PS: I didn't try this patch, just quickly wrote it to better explain
>>> what I had in mind.
>>>
>>> Cc: Paolo Abeni <pabeni@redhat.com>
>>> Cc: Geliang Tang <geliang.tang@suse.com>
>>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>>> ---
>>>
>>> Notes:
>>>     to be squashed in "mptcp: implement fastclose xmit path"
>>>
>>>  net/mptcp/options.c | 62 +++++++++++++++++++++++++++------------------
>>>  1 file changed, 37 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>> index 8a1020e4285c..07091971a51a 100644
>>> --- a/net/mptcp/options.c
>>> +++ b/net/mptcp/options.c
>>> @@ -829,11 +829,17 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
>>>
>>>  	if (unlikely(skb && TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST)) {
>>>  		if (mptcp_established_options_fastclose(sk, &opt_size, remaining, opts) ||
>>> -		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts) ||
>>> -		    mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
>>> +		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts)) {
>>>  			*size += opt_size;
>>>  			remaining -= opt_size;
>>>  		}
>>> +
>>> +		/* MP_RST can be used with MP_FASTCLOSE and MP_FAIL if there is room */
>>> +		if (mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
>>> +			*size += opt_size;
>>> +			remaining -= opt_size;
>>> +		}
>>> +
>>>  		return true;
>>>  	}
>>>
>>> @@ -1257,21 +1263,7 @@ static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
>>>  void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>>  			 struct mptcp_out_options *opts)
>>>  {
>>> -	if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
>>> -		const struct sock *ssk = (const struct sock *)tp;
>>> -		struct mptcp_subflow_context *subflow;
>>> -
>>> -		subflow = mptcp_subflow_ctx(ssk);
>>> -		subflow->send_mp_fail = 0;
>>> -
>>> -		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
>>> -				      TCPOLEN_MPTCP_FAIL,
>>> -				      0, 0);
>>> -		put_unaligned_be64(opts->fail_seq, ptr);
>>> -		ptr += 2;
>>> -	}
>>> -
>>> -	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and RST are mutually exclusive,
>>> +	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and FAIL are mutually exclusive,
>>>  	 * see mptcp_established_options*()
>>>  	 */
>>>  	if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
>>> @@ -1458,19 +1450,39 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>>  				ptr += 1;
>>>  			}
>>>  		}
>>> -	} else if (unlikely(OPTION_MPTCP_RST & opts->suboptions)) {
>>> -		/* RST is mutually exclusive with everything else */
>>> -		*ptr++ = mptcp_option(MPTCPOPT_RST,
>>> -				      TCPOLEN_MPTCP_RST,
>>> -				      opts->reset_transient,
>>> -				      opts->reset_reason);
>>> -		return;
>>>  	} else if (unlikely(OPTION_MPTCP_FASTCLOSE & opts->suboptions)) {
>>> -		/* FASTCLOSE is mutually exclusive with everything else */
>>> +		/* FASTCLOSE is mutually exclusive with others except RST */
>>>  		*ptr++ = mptcp_option(MPTCPOPT_MP_FASTCLOSE,
>>>  				      TCPOLEN_MPTCP_FASTCLOSE,
>>>  				      0, 0);
>>>  		put_unaligned_be64(opts->rcvr_key, ptr);
>>> +
>>> +		if (OPTION_MPTCP_RST & opts->suboptions)
>>> +			goto mp_rst;
>>> +		return;
>>> +	} else if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
>>> +		/* MP_FAIL is mutually exclusive with others except RST */
>>> +		const struct sock *ssk = (const struct sock *)tp;
>>> +		struct mptcp_subflow_context *subflow;
>>> +
>>> +		subflow = mptcp_subflow_ctx(ssk);
>>> +		subflow->send_mp_fail = 0;
>>> +
>>> +		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
>>> +				      TCPOLEN_MPTCP_FAIL,
>>> +				      0, 0);
>>> +		put_unaligned_be64(opts->fail_seq, ptr);
>>> +		ptr += 2;
>>> +
>>> +		if (OPTION_MPTCP_RST & opts->suboptions)
>>> +			goto mp_rst;
>>> +		return
>>
>> As the CI spotted, missing ';' here.
>
> Yes sorry, I quickly did it before having to go.
>
>> Otherwise LGTM!
>
> Thank you for the review!
>
> @Geliang: do you prefer to use this diff -- without the missing ';' --
> in a new version or do you prefer if I apply it with the fix directly?
>

Matthieu -

Since you were handling Geliang's revisions of this series while I was 
away, I assume you are not waiting to hear from me, but anyway: these 
changes (plus ';') combined with Geliang's v3 all look good to me too.

--
Mat Martineau
Intel

Re: Squash to "mptcp: implement fastclose xmit path": Build Failure
Posted by MPTCP CI 2 years, 3 months ago
Hi Matthieu,

Thank you for your modifications, that's great!

But sadly, our CI spotted some issues with it when trying to build it.

You can find more details there:

  https://patchwork.kernel.org/project/mptcp/patch/20211130165527.1506827-1-matthieu.baerts@tessares.net/
  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/1521989225

Status: failure
Initiator: MPTCPimporter
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/6efac2b03aba

Feel free to reply to this email if you cannot access logs, if you need
some support to fix the error, if this doesn't seem to be caused by your
modifications or if the error is a false positive one.

Cheers,
MPTCP GH Action bot