[MPTCP] [PATCH] mptcp: Re-factor mptcp_create_subflow()

[MPTCP] [PATCH] mptcp: Re-factor mptcp_create_subflow()

[Peter Krystad]

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

Move to subflow.c (because it belongs there) and rename.
Outgoing MP_JOIN requests will use this routine to create
subflow sockets.

squashto: Associate MPTCP context with TCP socket

Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
---
 net/mptcp/protocol.c | 33 +++------------------------------
 net/mptcp/protocol.h |  1 +
 net/mptcp/subflow.c  | 25 +++++++++++++++++++++++++
 3 files changed, 29 insertions(+), 30 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 775510bdbd4c..f2909457ef31 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -817,33 +817,6 @@ static struct proto mptcp_prot = {
 	.no_autobind	= 1,
 };
 
-static int mptcp_subflow_create(struct sock *sk)
-{
-	struct mptcp_sock *msk = mptcp_sk(sk);
-	struct net *net = sock_net(sk);
-	struct socket *sf;
-	int err;
-
-	pr_debug("msk=%p", msk);
-	err = sock_create_kern(net, PF_INET, SOCK_STREAM, IPPROTO_TCP, &sf);
-	if (!err) {
-		lock_sock(sf->sk);
-		err = tcp_set_ulp(sf->sk, "mptcp");
-		release_sock(sf->sk);
-		if (!err) {
-			struct subflow_context *subflow = subflow_ctx(sf->sk);
-
-			pr_debug("subflow=%p", subflow);
-			msk->subflow = sf;
-			subflow->conn = sk;
-			subflow->request_mptcp = 1; // @@ if MPTCP enabled
-			subflow->request_cksum = 1; // @@ if checksum enabled
-			subflow->version = 0;
-		}
-	}
-	return err;
-}
-
 static int mptcp_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 {
 	struct mptcp_sock *msk = mptcp_sk(sock->sk);
@@ -855,7 +828,7 @@ static int mptcp_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 		return err;
 
 	if (!msk->subflow) {
-		err = mptcp_subflow_create(sock->sk);
+		err = subflow_create_socket(sock->sk, &msk->subflow);
 		if (err)
 			return err;
 	}
@@ -874,7 +847,7 @@ static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 		return err;
 
 	if (!msk->subflow) {
-		err = mptcp_subflow_create(sock->sk);
+		err = subflow_create_socket(sock->sk, &msk->subflow);
 		if (err)
 			return err;
 	}
@@ -933,7 +906,7 @@ static int mptcp_listen(struct socket *sock, int backlog)
 	pr_debug("msk=%p", msk);
 
 	if (!msk->subflow) {
-		err = mptcp_subflow_create(sock->sk);
+		err = subflow_create_socket(sock->sk, &msk->subflow);
 		if (err)
 			return err;
 	}
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index ffc2452b4e77..e081b48d3f0d 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -201,6 +201,7 @@ mptcp_subflow_tcp_socket(const struct subflow_context *subflow)
 }
 
 void subflow_init(void);
+int subflow_create_socket(struct sock *sk, struct socket **new_sock);
 
 extern const struct inet_connection_sock_af_ops ipv4_specific;
 
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index a858cc966724..1236739425bc 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -201,6 +201,31 @@ static void subflow_data_ready(struct sock *sk)
 	}
 }
 
+int subflow_create_socket(struct sock *sk, struct socket **new_sock)
+{
+	struct net *net = sock_net(sk);
+	struct socket *sf;
+	int err;
+
+	err = sock_create_kern(net, PF_INET, SOCK_STREAM, IPPROTO_TCP, &sf);
+	if (!err) {
+		lock_sock(sf->sk);
+		err = tcp_set_ulp(sf->sk, "mptcp");
+		release_sock(sf->sk);
+		if (!err) {
+			struct subflow_context *subflow = subflow_ctx(sf->sk);
+
+			pr_debug("subflow=%p", subflow);
+			*new_sock = sf;
+			subflow->conn = sk;
+			subflow->request_mptcp = 1; // @@ if MPTCP enabled
+			subflow->request_cksum = 1; // @@ if checksum enabled
+			subflow->version = 0;
+		}
+	}
+	return err;
+}
+
 static struct subflow_context *subflow_create_ctx(struct sock *sk,
 						  struct socket *sock,
 						  gfp_t priority)
-- 
2.17.2

Re: [MPTCP] [PATCH] mptcp: Re-factor mptcp_create_subflow()

[Matthieu Baerts]

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

Hi Peter,

On 25/06/2019 01:06, Peter Krystad wrote:
> Move to subflow.c (because it belongs there) and rename.
> Outgoing MP_JOIN requests will use this routine to create
> subflow sockets.

Good idea and thank you for the patch!

I can apply it later today if nobody is against that. (it is easy to
revert anyway).

Cheers,
Matt

> squashto: Associate MPTCP context with TCP socket
> 
> Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
> ---
>  net/mptcp/protocol.c | 33 +++------------------------------
>  net/mptcp/protocol.h |  1 +
>  net/mptcp/subflow.c  | 25 +++++++++++++++++++++++++
>  3 files changed, 29 insertions(+), 30 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 775510bdbd4c..f2909457ef31 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -817,33 +817,6 @@ static struct proto mptcp_prot = {
>  	.no_autobind	= 1,
>  };
>  
> -static int mptcp_subflow_create(struct sock *sk)
> -{
> -	struct mptcp_sock *msk = mptcp_sk(sk);
> -	struct net *net = sock_net(sk);
> -	struct socket *sf;
> -	int err;
> -
> -	pr_debug("msk=%p", msk);
> -	err = sock_create_kern(net, PF_INET, SOCK_STREAM, IPPROTO_TCP, &sf);
> -	if (!err) {
> -		lock_sock(sf->sk);
> -		err = tcp_set_ulp(sf->sk, "mptcp");
> -		release_sock(sf->sk);
> -		if (!err) {
> -			struct subflow_context *subflow = subflow_ctx(sf->sk);
> -
> -			pr_debug("subflow=%p", subflow);
> -			msk->subflow = sf;
> -			subflow->conn = sk;
> -			subflow->request_mptcp = 1; // @@ if MPTCP enabled
> -			subflow->request_cksum = 1; // @@ if checksum enabled
> -			subflow->version = 0;
> -		}
> -	}
> -	return err;
> -}
> -
>  static int mptcp_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>  {
>  	struct mptcp_sock *msk = mptcp_sk(sock->sk);
> @@ -855,7 +828,7 @@ static int mptcp_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>  		return err;
>  
>  	if (!msk->subflow) {
> -		err = mptcp_subflow_create(sock->sk);
> +		err = subflow_create_socket(sock->sk, &msk->subflow);
>  		if (err)
>  			return err;
>  	}
> @@ -874,7 +847,7 @@ static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
>  		return err;
>  
>  	if (!msk->subflow) {
> -		err = mptcp_subflow_create(sock->sk);
> +		err = subflow_create_socket(sock->sk, &msk->subflow);
>  		if (err)
>  			return err;
>  	}
> @@ -933,7 +906,7 @@ static int mptcp_listen(struct socket *sock, int backlog)
>  	pr_debug("msk=%p", msk);
>  
>  	if (!msk->subflow) {
> -		err = mptcp_subflow_create(sock->sk);
> +		err = subflow_create_socket(sock->sk, &msk->subflow);
>  		if (err)
>  			return err;
>  	}
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index ffc2452b4e77..e081b48d3f0d 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -201,6 +201,7 @@ mptcp_subflow_tcp_socket(const struct subflow_context *subflow)
>  }
>  
>  void subflow_init(void);
> +int subflow_create_socket(struct sock *sk, struct socket **new_sock);
>  
>  extern const struct inet_connection_sock_af_ops ipv4_specific;
>  
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index a858cc966724..1236739425bc 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -201,6 +201,31 @@ static void subflow_data_ready(struct sock *sk)
>  	}
>  }
>  
> +int subflow_create_socket(struct sock *sk, struct socket **new_sock)
> +{
> +	struct net *net = sock_net(sk);
> +	struct socket *sf;
> +	int err;
> +
> +	err = sock_create_kern(net, PF_INET, SOCK_STREAM, IPPROTO_TCP, &sf);
> +	if (!err) {
> +		lock_sock(sf->sk);
> +		err = tcp_set_ulp(sf->sk, "mptcp");
> +		release_sock(sf->sk);
> +		if (!err) {
> +			struct subflow_context *subflow = subflow_ctx(sf->sk);
> +
> +			pr_debug("subflow=%p", subflow);
> +			*new_sock = sf;
> +			subflow->conn = sk;
> +			subflow->request_mptcp = 1; // @@ if MPTCP enabled
> +			subflow->request_cksum = 1; // @@ if checksum enabled
> +			subflow->version = 0;
> +		}
> +	}
> +	return err;
> +}
> +
>  static struct subflow_context *subflow_create_ctx(struct sock *sk,
>  						  struct socket *sock,
>  						  gfp_t priority)
> 

-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

Re: [MPTCP] [PATCH] mptcp: Re-factor mptcp_create_subflow()

[Matthieu Baerts]

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

Hi Peter,

On 25/06/2019 01:06, Peter Krystad wrote:
> Move to subflow.c (because it belongs there) and rename.
> Outgoing MP_JOIN requests will use this routine to create
> subflow sockets.
> 
> squashto: Associate MPTCP context with TCP socket
> 
> Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
> ---
>  net/mptcp/protocol.c | 33 +++------------------------------
>  net/mptcp/protocol.h |  1 +
>  net/mptcp/subflow.c  | 25 +++++++++++++++++++++++++
>  3 files changed, 29 insertions(+), 30 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 775510bdbd4c..f2909457ef31 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -817,33 +817,6 @@ static struct proto mptcp_prot = {
>  	.no_autobind	= 1,
>  };
>  
> -static int mptcp_subflow_create(struct sock *sk)
> -{
> -	struct mptcp_sock *msk = mptcp_sk(sk);
> -	struct net *net = sock_net(sk);
> -	struct socket *sf;
> -	int err;
> -
> -	pr_debug("msk=%p", msk);
> -	err = sock_create_kern(net, PF_INET, SOCK_STREAM, IPPROTO_TCP, &sf);
> -	if (!err) {
> -		lock_sock(sf->sk);
> -		err = tcp_set_ulp(sf->sk, "mptcp");
> -		release_sock(sf->sk);
> -		if (!err) {
> -			struct subflow_context *subflow = subflow_ctx(sf->sk);
> -
> -			pr_debug("subflow=%p", subflow);
> -			msk->subflow = sf;
> -			subflow->conn = sk;
> -			subflow->request_mptcp = 1; // @@ if MPTCP enabled
> -			subflow->request_cksum = 1; // @@ if checksum enabled
> -			subflow->version = 0;
> -		}
> -	}
> -	return err;
> -}
> -
>  static int mptcp_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>  {
>  	struct mptcp_sock *msk = mptcp_sk(sock->sk);
> @@ -855,7 +828,7 @@ static int mptcp_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>  		return err;
>  
>  	if (!msk->subflow) {
> -		err = mptcp_subflow_create(sock->sk);
> +		err = subflow_create_socket(sock->sk, &msk->subflow);
>  		if (err)
>  			return err;
>  	}
> @@ -874,7 +847,7 @@ static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
>  		return err;
>  
>  	if (!msk->subflow) {
> -		err = mptcp_subflow_create(sock->sk);
> +		err = subflow_create_socket(sock->sk, &msk->subflow);
>  		if (err)
>  			return err;
>  	}
> @@ -933,7 +906,7 @@ static int mptcp_listen(struct socket *sock, int backlog)
>  	pr_debug("msk=%p", msk);
>  
>  	if (!msk->subflow) {
> -		err = mptcp_subflow_create(sock->sk);
> +		err = subflow_create_socket(sock->sk, &msk->subflow);
>  		if (err)
>  			return err;
>  	}
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index ffc2452b4e77..e081b48d3f0d 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -201,6 +201,7 @@ mptcp_subflow_tcp_socket(const struct subflow_context *subflow)
>  }
>  
>  void subflow_init(void);
> +int subflow_create_socket(struct sock *sk, struct socket **new_sock);
>  
>  extern const struct inet_connection_sock_af_ops ipv4_specific;
>  
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index a858cc966724..1236739425bc 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -201,6 +201,31 @@ static void subflow_data_ready(struct sock *sk)
>  	}
>  }
>  
> +int subflow_create_socket(struct sock *sk, struct socket **new_sock)
> +{
> +	struct net *net = sock_net(sk);
> +	struct socket *sf;
> +	int err;
> +
> +	err = sock_create_kern(net, PF_INET, SOCK_STREAM, IPPROTO_TCP, &sf);
> +	if (!err) {
> +		lock_sock(sf->sk);
> +		err = tcp_set_ulp(sf->sk, "mptcp");
> +		release_sock(sf->sk);
> +		if (!err) {
> +			struct subflow_context *subflow = subflow_ctx(sf->sk);
> +
> +			pr_debug("subflow=%p", subflow);
> +			*new_sock = sf;
> +			subflow->conn = sk;
> +			subflow->request_mptcp = 1; // @@ if MPTCP enabled
> +			subflow->request_cksum = 1; // @@ if checksum enabled
> +			subflow->version = 0;
> +		}
> +	}

When applying this, I remember what Paolo recently said: upstream
prefers keeping the expected/main code path with no indentation.

I hope you don't mind if I slightly change this when applying the patch.
We would then have:

    err = sock_create_kern(net, PF_INET, SOCK_STREAM, IPPROTO_TCP, &sf);
    if (err)
        return err;

   lock_sock(sf->sk);
   (...)

Cheers,
Matt

> +	return err;
> +}
> +
>  static struct subflow_context *subflow_create_ctx(struct sock *sk,
>  						  struct socket *sock,
>  						  gfp_t priority)
> 

-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

Re: [MPTCP] [PATCH] mptcp: Re-factor mptcp_create_subflow()

[Matthieu Baerts]

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

Hi Peter,

On 25/06/2019 14:32, Matthieu Baerts wrote:
> Hi Peter,
> 
> On 25/06/2019 01:06, Peter Krystad wrote:
>> Move to subflow.c (because it belongs there) and rename.
>> Outgoing MP_JOIN requests will use this routine to create
>> subflow sockets.
> 
> Good idea and thank you for the patch!
> 
> I can apply it later today if nobody is against that. (it is easy to
> revert anyway).
> 
> Cheers,
> Matt
> 
>> squashto: Associate MPTCP context with TCP socket

Just applied:
- 5e767308567e + d442d01ba8f1: "squash" in "mptcp: Associate MPTCP
context with TCP socket"
- 9d3014b27993: conflict in
t/mptcp-Handle-MP_CAPABLE-options-for-outgoing-connections
- bbcd3e436f8f + 5cc99acf0f23: conflict in
t/mptcp-Create-SUBFLOW-socket-for-incoming-connections
- 9456db61cc94: "squash" in "mptcp: Create SUBFLOW socket for incoming
connections"
- e50d314e3d44: conflict in t/mptcp-Implement-MPTCP-receive-path
- d73c270e1ed9..7f5c1f309977: result

Tests are still OK!

Cheers,
Matt

-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium