[MPTCP] [PATCH 7/7] mptcp: Refactor mptcp_established_options() to single hook

[MPTCP] [PATCH 7/7] mptcp: Refactor mptcp_established_options() to single hook

[Peter Krystad]

[-- Attachment #1: Type: text/plain, Size: 5780 bytes --]

Make there only be one callout from tcp_output.c for this.
Includes refactoring from the first patch in the MP_JOIN RFC.

squash to: Write MPTCP DSS headers

Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
---
 include/net/mptcp.h   | 21 ++++++---------------
 net/ipv4/tcp_output.c | 21 ++++++---------------
 net/mptcp/options.c   | 36 ++++++++++++++++++++++++++++++------
 3 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index d84489adbd86..ec3d7f7e5f45 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -57,13 +57,11 @@ void mptcp_parse_option(const unsigned char *ptr, int opsize,
 bool mptcp_syn_options(struct sock *sk, unsigned int *size,
 			       struct mptcp_out_options* opts);
 void mptcp_rcv_synsent(struct sock *sk);
-bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
-				   unsigned int *size, unsigned int remaining,
-				   struct mptcp_out_options* opts);
 bool mptcp_synack_options(const struct request_sock *req, unsigned int *size,
 			  struct mptcp_out_options *opts);
-bool mptcp_established_options(struct sock *sk, unsigned int *size,
-				       struct mptcp_out_options *opts);
+bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
+			       unsigned int *size, unsigned int remaining,
+			       struct mptcp_out_options* opts);
 
 void mptcp_attach_dss(struct sock *sk, struct sk_buff *skb,
 		      struct tcp_options_received *opt_rx);
@@ -106,15 +104,6 @@ static inline void mptcp_rcv_synsent(struct sock *sk)
 {
 }
 
-static inline bool mptcp_established_options_dss(struct sock *sk,
-						 struct sk_buff *skb,
-						 unsigned int *size,
-						 unsigned int remaining,
-						 struct mptcp_out_options* opts)
-{
-	return false;
-}
-
 static inline bool mptcp_synack_options(const struct request_sock *req,
 					unsigned int *size,
 					struct mptcp_out_options *opts)
@@ -123,8 +112,10 @@ static inline bool mptcp_synack_options(const struct request_sock *req,
 }
 
 static inline bool mptcp_established_options(struct sock *sk,
+					     struct sk_buff *skb,
 					     unsigned int *size,
-					     struct mptcp_out_options *opts)
+					     unsigned int remaining,
+					     struct mptcp_out_options* opts);
 {
 	return false;
 }
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 0b4865c55e4e..6ad88b70de6c 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -443,6 +443,7 @@ struct tcp_out_options {
 
 static void mptcp_options_write(__be32 *ptr, struct tcp_out_options *opts)
 {
+	pr_debug("entered");
 #if IS_ENABLED(CONFIG_MPTCP)
 	if (unlikely(OPTION_MPTCP & opts->options))
 		mptcp_write_options(ptr, &opts->mptcp);
@@ -796,22 +797,12 @@ static unsigned int tcp_established_options(struct sock *sk, struct sk_buff *skb
 	 */
 	if (sk_is_mptcp(sk)) {
 		unsigned int remaining = MAX_TCP_OPTION_SPACE - size;
-		unsigned int opt_size;
+		unsigned int opt_size = 0;
 
-		if (mptcp_established_options(sk, &opt_size, &opts->mptcp)) {
-			if (remaining >= opt_size) {
-				opts->options |= OPTION_MPTCP;
-				size += opt_size;
-			}
-		} else {
-			unsigned int dss_size;
-
-			if (mptcp_established_options_dss(sk, skb, &dss_size,
-							  remaining,
-							  &opts->mptcp)) {
-				opts->options |= OPTION_MPTCP;
-				size += dss_size;
-			}
+		if (mptcp_established_options(sk, skb, &opt_size, remaining,
+					      &opts->mptcp)) {
+			opts->options |= OPTION_MPTCP;
+			size += opt_size;
 		}
 	}
 
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 98d582207cb0..f06ca0c48e57 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -249,12 +249,14 @@ void mptcp_rcv_synsent(struct sock *sk)
 	}
 }
 
-bool mptcp_established_options(struct sock *sk, unsigned int *size,
-			       struct mptcp_out_options *opts)
+static bool mptcp_established_options_mp(struct sock *sk, unsigned int *size,
+					 unsigned int remaining,
+					 struct mptcp_out_options *opts)
 {
 	struct subflow_context *subflow = subflow_ctx(sk);
 
-	if (subflow->mp_capable && !subflow->fourth_ack) {
+	if (subflow->mp_capable && !subflow->fourth_ack &&
+	    remaining >= TCPOLEN_MPTCP_MPC_ACK) {
 		opts->suboptions = OPTION_MPTCP_MPC_ACK;
 		opts->sndr_key = subflow->local_key;
 		opts->rcvr_key = subflow->remote_key;
@@ -267,9 +269,10 @@ bool mptcp_established_options(struct sock *sk, unsigned int *size,
 	return false;
 }
 
-bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
-				   unsigned int *size, unsigned int remaining,
-				   struct mptcp_out_options *opts)
+static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
+					  unsigned int *size,
+					  unsigned int remaining,
+					  struct mptcp_out_options *opts)
 {
 	unsigned int dss_size = 0;
 	struct mptcp_ext *mpext;
@@ -338,6 +341,27 @@ bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
 	return true;
 }
 
+bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
+			       unsigned int *size, unsigned int remaining,
+			       struct mptcp_out_options *opts)
+{
+	unsigned int opt_size = 0;
+
+	if (mptcp_established_options_mp(sk, &opt_size, remaining, opts)) {
+		*size += opt_size;
+		remaining -= opt_size;
+		return true;
+	}
+	else if (mptcp_established_options_dss(sk, skb, &opt_size, remaining,
+					       opts)) {
+		*size += opt_size;
+		remaining -= opt_size;
+		return true;
+	}
+
+	return false;
+}
+
 bool mptcp_synack_options(const struct request_sock *req, unsigned int *size,
 			  struct mptcp_out_options *opts)
 {
-- 
2.17.2

Re: [MPTCP] [PATCH 7/7] mptcp: Refactor mptcp_established_options() to single hook

[Peter Krystad]

[-- Attachment #1: Type: text/plain, Size: 2523 bytes --]

On Fri, 2019-06-07 at 14:42 -0700, Mat Martineau wrote:
> On Wed, 5 Jun 2019, Peter Krystad wrote:
> 
> > Make there only be one callout from tcp_output.c for this.
> > Includes refactoring from the first patch in the MP_JOIN RFC.
> > 
> > squash to: Write MPTCP DSS headers
> > 
> > Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
> > ---
> > include/net/mptcp.h   | 21 ++++++---------------
> > net/ipv4/tcp_output.c | 21 ++++++---------------
> > net/mptcp/options.c   | 36 ++++++++++++++++++++++++++++++------
> > 3 files changed, 42 insertions(+), 36 deletions(-)
> > 
> 
> ...
> 
> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index 98d582207cb0..f06ca0c48e57 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> 
> ...
> 
> > @@ -338,6 +341,27 @@ bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
> > 	return true;
> > }
> > 
> > +bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
> > +			       unsigned int *size, unsigned int remaining,
> > +			       struct mptcp_out_options *opts)
> > +{
> > +	unsigned int opt_size = 0;
> > +
> > +	if (mptcp_established_options_mp(sk, &opt_size, remaining, opts)) {
> > +		*size += opt_size;
> > +		remaining -= opt_size;
> > +		return true;
> > +	}
> > +	else if (mptcp_established_options_dss(sk, skb, &opt_size, remaining,
> > +					       opts)) {
> > +		*size += opt_size;
> > +		remaining -= opt_size;
> > +		return true;
> > +	}
> 
> Given that the code for both clauses is the same, maybe
> 
> if (mptcp_established_options_mp(...) ||
>      mptcp_established_options_dss(...)) {
>      *size += opt_size;
>      remaining -= opt_size;
>      return true;
> }
> 
> would reduce duplication.

In the future I think it is likely this if/else clause will be changed to
allow sending multiple options a single header so I was planning to leave as
is. 

> Since the DSS variant is the common case, I was tempted to suggest 
> collapsing all the MP and DSS code in to this one function. I don't think 
> that makes a real difference since the compiler will probably inline 
> everything anyway.
> 
> I agree with Paolo's suggestion to do a single, early mp_capable check.
> 

This I did.

Peter.

> > +
> > +	return false;
> > +}
> > +
> > bool mptcp_synack_options(const struct request_sock *req, unsigned int *size,
> > 			  struct mptcp_out_options *opts)
> > {
> 
> --
> Mat Martineau
> Intel

Re: [MPTCP] [PATCH 7/7] mptcp: Refactor mptcp_established_options() to single hook

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 2128 bytes --]


On Wed, 5 Jun 2019, Peter Krystad wrote:

> Make there only be one callout from tcp_output.c for this.
> Includes refactoring from the first patch in the MP_JOIN RFC.
>
> squash to: Write MPTCP DSS headers
>
> Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
> ---
> include/net/mptcp.h   | 21 ++++++---------------
> net/ipv4/tcp_output.c | 21 ++++++---------------
> net/mptcp/options.c   | 36 ++++++++++++++++++++++++++++++------
> 3 files changed, 42 insertions(+), 36 deletions(-)
>

...

> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 98d582207cb0..f06ca0c48e57 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c

...

> @@ -338,6 +341,27 @@ bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
> 	return true;
> }
>
> +bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
> +			       unsigned int *size, unsigned int remaining,
> +			       struct mptcp_out_options *opts)
> +{
> +	unsigned int opt_size = 0;
> +
> +	if (mptcp_established_options_mp(sk, &opt_size, remaining, opts)) {
> +		*size += opt_size;
> +		remaining -= opt_size;
> +		return true;
> +	}
> +	else if (mptcp_established_options_dss(sk, skb, &opt_size, remaining,
> +					       opts)) {
> +		*size += opt_size;
> +		remaining -= opt_size;
> +		return true;
> +	}

Given that the code for both clauses is the same, maybe

if (mptcp_established_options_mp(...) ||
     mptcp_established_options_dss(...)) {
     *size += opt_size;
     remaining -= opt_size;
     return true;
}

would reduce duplication.

Since the DSS variant is the common case, I was tempted to suggest 
collapsing all the MP and DSS code in to this one function. I don't think 
that makes a real difference since the compiler will probably inline 
everything anyway.

I agree with Paolo's suggestion to do a single, early mp_capable check.

> +
> +	return false;
> +}
> +
> bool mptcp_synack_options(const struct request_sock *req, unsigned int *size,
> 			  struct mptcp_out_options *opts)
> {


--
Mat Martineau
Intel

Re: [MPTCP] [PATCH 7/7] mptcp: Refactor mptcp_established_options() to single hook

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 1538 bytes --]

Hi,

I have a couple of very minor comments here, please see below.

On Wed, 2019-06-05 at 17:36 -0700, Peter Krystad wrote:
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 0b4865c55e4e..6ad88b70de6c 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/
> @@ -443,6 +443,7 @@ struct tcp_out_options {
>  
>  static void mptcp_options_write(__be32 *ptr, struct tcp_out_options *opts)
>  {
> +	pr_debug("entered");

I think the above debug message could be moved under the CONFIG_MPTCP
conditional, or possibly even dropped.

>  #if IS_ENABLED(CONFIG_MPTCP)
>  	if (unlikely(OPTION_MPTCP & opts->options))
>  		mptcp_write_options(ptr, &opts->mptcp);

[...]

> @@ -338,6 +341,27 @@ bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
>  	return true;
>  }
>  
> +bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
> +			       unsigned int *size, unsigned int remaining,
> +			       struct mptcp_out_options *opts)
> +{
> +	unsigned int opt_size = 0;
> +
> +	if (mptcp_established_options_mp(sk, &opt_size, remaining, opts)) {
> +		*size += opt_size;
> +		remaining -= opt_size;
> +		return true;
> +	}
> +	else if (mptcp_established_options_dss(sk, skb, &opt_size, remaining,
> +					       opts)) {

I think the preferred kernel style is: '} else if ('...

Perhpas moving here the 'if (subflow->mp_capable' test from both
mptcp_established_options_dss() and mptcp_established_options_mp()
would simplify the code?

Thanks!

Paolo