diff mbox series

[net-next,1/2] net/smc: refatoring initialization of smc sock

Message ID 1715314333-107290-2-git-send-email-alibuda@linux.alibaba.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Introduce IPPROTO_SMC | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 925 this patch: 925
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 10 of 10 maintainers
netdev/build_clang success Errors and warnings before: 936 this patch: 936
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 936 this patch: 936
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 127 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-05-10--15-00 (tests: 1011)

Commit Message

D. Wythe May 10, 2024, 4:12 a.m. UTC
From: "D. Wythe" <alibuda@linux.alibaba.com>

This patch aims to isolate the shared components of SMC socket
allocation by introducing smc_sock_init() for sock initialization
and __smc_create_clcsk() for the initialization of clcsock.

This is in preparation for the subsequent implementation of the
AF_INET version of SMC.

Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
---
 net/smc/af_smc.c | 93 +++++++++++++++++++++++++++++++-------------------------
 1 file changed, 52 insertions(+), 41 deletions(-)

Comments

Dust Li May 10, 2024, 9:50 a.m. UTC | #1
On 2024-05-10 12:12:12, D. Wythe wrote:
>From: "D. Wythe" <alibuda@linux.alibaba.com>
>
>This patch aims to isolate the shared components of SMC socket
>allocation by introducing smc_sock_init() for sock initialization
>and __smc_create_clcsk() for the initialization of clcsock.
>
>This is in preparation for the subsequent implementation of the
>AF_INET version of SMC.
>
>Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
>---
> net/smc/af_smc.c | 93 +++++++++++++++++++++++++++++++-------------------------
> 1 file changed, 52 insertions(+), 41 deletions(-)
>
>diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>index 9389f0c..1f03724 100644
>--- a/net/smc/af_smc.c
>+++ b/net/smc/af_smc.c
>@@ -361,34 +361,43 @@ static void smc_destruct(struct sock *sk)
> 		return;
> }
> 
>-static struct sock *smc_sock_alloc(struct net *net, struct socket *sock,
>-				   int protocol)
>+static void smc_sock_init(struct net *net, struct sock *sk, int protocol)
> {
>-	struct smc_sock *smc;
>-	struct proto *prot;
>-	struct sock *sk;
>-
>-	prot = (protocol == SMCPROTO_SMC6) ? &smc_proto6 : &smc_proto;
>-	sk = sk_alloc(net, PF_SMC, GFP_KERNEL, prot, 0);
>-	if (!sk)
>-		return NULL;
>+	struct smc_sock *smc = smc_sk(sk);
> 
>-	sock_init_data(sock, sk); /* sets sk_refcnt to 1 */
> 	sk->sk_state = SMC_INIT;
>-	sk->sk_destruct = smc_destruct;
> 	sk->sk_protocol = protocol;
>+	mutex_init(&smc->clcsock_release_lock);
> 	WRITE_ONCE(sk->sk_sndbuf, 2 * READ_ONCE(net->smc.sysctl_wmem));
> 	WRITE_ONCE(sk->sk_rcvbuf, 2 * READ_ONCE(net->smc.sysctl_rmem));
>-	smc = smc_sk(sk);
> 	INIT_WORK(&smc->tcp_listen_work, smc_tcp_listen_work);
> 	INIT_WORK(&smc->connect_work, smc_connect_work);
> 	INIT_DELAYED_WORK(&smc->conn.tx_work, smc_tx_work);
> 	INIT_LIST_HEAD(&smc->accept_q);
> 	spin_lock_init(&smc->accept_q_lock);
> 	spin_lock_init(&smc->conn.send_lock);
>-	sk->sk_prot->hash(sk);
>-	mutex_init(&smc->clcsock_release_lock);
> 	smc_init_saved_callbacks(smc);
>+	smc->limit_smc_hs = net->smc.limit_smc_hs;
>+	smc->use_fallback = false; /* assume rdma capability first */
>+	smc->fallback_rsn = 0;
>+
>+	sk->sk_destruct = smc_destruct;
>+	sk->sk_prot->hash(sk);

Why change the order here ? e.g.

Before:
    sk->sk_destruct = smc_destruct;
    mutex_init(&smc->clcsock_release_lock);
After
    mutex_init(&smc->clcsock_release_lock);
    sk->sk_destruct = smc_destruct;

Same for sk->sk_prot->hash(sk)


>+}
>+
>+static struct sock *smc_sock_alloc(struct net *net, struct socket *sock,
>+				   int protocol)
>+{
>+	struct proto *prot;
>+	struct sock *sk;
>+
>+	prot = (protocol == SMCPROTO_SMC6) ? &smc_proto6 : &smc_proto;
>+	sk = sk_alloc(net, PF_SMC, GFP_KERNEL, prot, 0);
>+	if (!sk)
>+		return NULL;
>+
>+	sock_init_data(sock, sk); /* sets sk_refcnt to 1 */
>+	smc_sock_init(net, sk, protocol);
> 
> 	return sk;
> }
>@@ -3321,6 +3330,31 @@ static ssize_t smc_splice_read(struct socket *sock, loff_t *ppos,
> 	.splice_read	= smc_splice_read,
> };
> 
>+static int __smc_create_clcsk(struct net *net, struct sock *sk, int family)

Why add '__' prefix here ?


>+{
>+	struct smc_sock *smc = smc_sk(sk);
>+	int rc;
>+
>+	rc = sock_create_kern(net, family, SOCK_STREAM, IPPROTO_TCP,
>+			      &smc->clcsock);
>+	if (rc) {
>+		sk_common_release(sk);
>+		return rc;
>+	}
>+
>+	/* smc_clcsock_release() does not wait smc->clcsock->sk's
>+	 * destruction;  its sk_state might not be TCP_CLOSE after
>+	 * smc->sk is close()d, and TCP timers can be fired later,
>+	 * which need net ref.
>+	 */
>+	sk = smc->clcsock->sk;
>+	__netns_tracker_free(net, &sk->ns_tracker, false);
>+	sk->sk_net_refcnt = 1;
>+	get_net_track(net, &sk->ns_tracker, GFP_KERNEL);
>+	sock_inuse_add(net, 1);
>+	return 0;
>+}
>+
> static int __smc_create(struct net *net, struct socket *sock, int protocol,
> 			int kern, struct socket *clcsock)
> {
>@@ -3346,35 +3380,12 @@ static int __smc_create(struct net *net, struct socket *sock, int protocol,
> 
> 	/* create internal TCP socket for CLC handshake and fallback */
> 	smc = smc_sk(sk);
>-	smc->use_fallback = false; /* assume rdma capability first */
>-	smc->fallback_rsn = 0;
>-
>-	/* default behavior from limit_smc_hs in every net namespace */
>-	smc->limit_smc_hs = net->smc.limit_smc_hs;
> 
> 	rc = 0;
>-	if (!clcsock) {
>-		rc = sock_create_kern(net, family, SOCK_STREAM, IPPROTO_TCP,
>-				      &smc->clcsock);
>-		if (rc) {
>-			sk_common_release(sk);
>-			goto out;
>-		}
>-
>-		/* smc_clcsock_release() does not wait smc->clcsock->sk's
>-		 * destruction;  its sk_state might not be TCP_CLOSE after
>-		 * smc->sk is close()d, and TCP timers can be fired later,
>-		 * which need net ref.
>-		 */
>-		sk = smc->clcsock->sk;
>-		__netns_tracker_free(net, &sk->ns_tracker, false);
>-		sk->sk_net_refcnt = 1;
>-		get_net_track(net, &sk->ns_tracker, GFP_KERNEL);
>-		sock_inuse_add(net, 1);
>-	} else {
>+	if (!clcsock)
>+		rc = __smc_create_clcsk(net, sk, family);
>+	else
> 		smc->clcsock = clcsock;
>-	}
>-
> out:
> 	return rc;
> }
>-- 
>1.8.3.1
>
D. Wythe May 11, 2024, 2:26 a.m. UTC | #2
On 5/10/24 5:50 PM, Dust Li wrote:
> On 2024-05-10 12:12:12, D. Wythe wrote:
>> From: "D. Wythe" <alibuda@linux.alibaba.com>
>>
>> This patch aims to isolate the shared components of SMC socket
>> allocation by introducing smc_sock_init() for sock initialization
>> and __smc_create_clcsk() for the initialization of clcsock.
>>
>> This is in preparation for the subsequent implementation of the
>> AF_INET version of SMC.
>>
>> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
>> ---
>> net/smc/af_smc.c | 93 +++++++++++++++++++++++++++++++-------------------------
>> 1 file changed, 52 insertions(+), 41 deletions(-)
>>
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index 9389f0c..1f03724 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -361,34 +361,43 @@ static void smc_destruct(struct sock *sk)
>> 		return;
>> }
>>
>> -static struct sock *smc_sock_alloc(struct net *net, struct socket *sock,
>> -				   int protocol)
>> +static void smc_sock_init(struct net *net, struct sock *sk, int protocol)
>> {
>> -	struct smc_sock *smc;
>> -	struct proto *prot;
>> -	struct sock *sk;
>> -
>> -	prot = (protocol == SMCPROTO_SMC6) ? &smc_proto6 : &smc_proto;
>> -	sk = sk_alloc(net, PF_SMC, GFP_KERNEL, prot, 0);
>> -	if (!sk)
>> -		return NULL;
>> +	struct smc_sock *smc = smc_sk(sk);
>>
>> -	sock_init_data(sock, sk); /* sets sk_refcnt to 1 */
>> 	sk->sk_state = SMC_INIT;
>> -	sk->sk_destruct = smc_destruct;
>> 	sk->sk_protocol = protocol;
>> +	mutex_init(&smc->clcsock_release_lock);
>> 	WRITE_ONCE(sk->sk_sndbuf, 2 * READ_ONCE(net->smc.sysctl_wmem));
>> 	WRITE_ONCE(sk->sk_rcvbuf, 2 * READ_ONCE(net->smc.sysctl_rmem));
>> -	smc = smc_sk(sk);
>> 	INIT_WORK(&smc->tcp_listen_work, smc_tcp_listen_work);
>> 	INIT_WORK(&smc->connect_work, smc_connect_work);
>> 	INIT_DELAYED_WORK(&smc->conn.tx_work, smc_tx_work);
>> 	INIT_LIST_HEAD(&smc->accept_q);
>> 	spin_lock_init(&smc->accept_q_lock);
>> 	spin_lock_init(&smc->conn.send_lock);
>> -	sk->sk_prot->hash(sk);
>> -	mutex_init(&smc->clcsock_release_lock);
>> 	smc_init_saved_callbacks(smc);
>> +	smc->limit_smc_hs = net->smc.limit_smc_hs;
>> +	smc->use_fallback = false; /* assume rdma capability first */
>> +	smc->fallback_rsn = 0;
>> +
>> +	sk->sk_destruct = smc_destruct;
>> +	sk->sk_prot->hash(sk);
> Why change the order here ? e.g.
>
> Before:
>      sk->sk_destruct = smc_destruct;
>      mutex_init(&smc->clcsock_release_lock);
> After
>      mutex_init(&smc->clcsock_release_lock);
>      sk->sk_destruct = smc_destruct;
>
> Same for sk->sk_prot->hash(sk)

Yes, you are right, I will fix it in the next version.
>
>
>> +}
>> +
>> +static struct sock *smc_sock_alloc(struct net *net, struct socket *sock,
>> +				   int protocol)
>> +{
>> +	struct proto *prot;
>> +	struct sock *sk;
>> +
>> +	prot = (protocol == SMCPROTO_SMC6) ? &smc_proto6 : &smc_proto;
>> +	sk = sk_alloc(net, PF_SMC, GFP_KERNEL, prot, 0);
>> +	if (!sk)
>> +		return NULL;
>> +
>> +	sock_init_data(sock, sk); /* sets sk_refcnt to 1 */
>> +	smc_sock_init(net, sk, protocol);
>>
>> 	return sk;
>> }
>> @@ -3321,6 +3330,31 @@ static ssize_t smc_splice_read(struct socket *sock, loff_t *ppos,
>> 	.splice_read	= smc_splice_read,
>> };
>>
>> +static int __smc_create_clcsk(struct net *net, struct sock *sk, int family)
> Why add '__' prefix here ?

Good question, I also realize that this is not suitable, I will delete 
it in the next version.

Thanks.

>
>> +{
>> +	struct smc_sock *smc = smc_sk(sk);
>> +	int rc;
>> +
>> +	rc = sock_create_kern(net, family, SOCK_STREAM, IPPROTO_TCP,
>> +			      &smc->clcsock);
>> +	if (rc) {
>> +		sk_common_release(sk);
>> +		return rc;
>> +	}
>> +
>> +	/* smc_clcsock_release() does not wait smc->clcsock->sk's
>> +	 * destruction;  its sk_state might not be TCP_CLOSE after
>> +	 * smc->sk is close()d, and TCP timers can be fired later,
>> +	 * which need net ref.
>> +	 */
>> +	sk = smc->clcsock->sk;
>> +	__netns_tracker_free(net, &sk->ns_tracker, false);
>> +	sk->sk_net_refcnt = 1;
>> +	get_net_track(net, &sk->ns_tracker, GFP_KERNEL);
>> +	sock_inuse_add(net, 1);
>> +	return 0;
>> +}
>> +
>> static int __smc_create(struct net *net, struct socket *sock, int protocol,
>> 			int kern, struct socket *clcsock)
>> {
>> @@ -3346,35 +3380,12 @@ static int __smc_create(struct net *net, struct socket *sock, int protocol,
>>
>> 	/* create internal TCP socket for CLC handshake and fallback */
>> 	smc = smc_sk(sk);
>> -	smc->use_fallback = false; /* assume rdma capability first */
>> -	smc->fallback_rsn = 0;
>> -
>> -	/* default behavior from limit_smc_hs in every net namespace */
>> -	smc->limit_smc_hs = net->smc.limit_smc_hs;
>>
>> 	rc = 0;
>> -	if (!clcsock) {
>> -		rc = sock_create_kern(net, family, SOCK_STREAM, IPPROTO_TCP,
>> -				      &smc->clcsock);
>> -		if (rc) {
>> -			sk_common_release(sk);
>> -			goto out;
>> -		}
>> -
>> -		/* smc_clcsock_release() does not wait smc->clcsock->sk's
>> -		 * destruction;  its sk_state might not be TCP_CLOSE after
>> -		 * smc->sk is close()d, and TCP timers can be fired later,
>> -		 * which need net ref.
>> -		 */
>> -		sk = smc->clcsock->sk;
>> -		__netns_tracker_free(net, &sk->ns_tracker, false);
>> -		sk->sk_net_refcnt = 1;
>> -		get_net_track(net, &sk->ns_tracker, GFP_KERNEL);
>> -		sock_inuse_add(net, 1);
>> -	} else {
>> +	if (!clcsock)
>> +		rc = __smc_create_clcsk(net, sk, family);
>> +	else
>> 		smc->clcsock = clcsock;
>> -	}
>> -
>> out:
>> 	return rc;
>> }
>> -- 
>> 1.8.3.1
>>
Zhu Yanjun May 11, 2024, 12:21 p.m. UTC | #3
在 2024/5/10 6:12, D. Wythe 写道:
> From: "D. Wythe" <alibuda@linux.alibaba.com>
> 
> This patch aims to isolate the shared components of SMC socket
> allocation by introducing smc_sock_init() for sock initialization
> and __smc_create_clcsk() for the initialization of clcsock.
> 
> This is in preparation for the subsequent implementation of the
> AF_INET version of SMC.
> 
> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
> ---
>   net/smc/af_smc.c | 93 +++++++++++++++++++++++++++++++-------------------------
>   1 file changed, 52 insertions(+), 41 deletions(-)
> 
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index 9389f0c..1f03724 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -361,34 +361,43 @@ static void smc_destruct(struct sock *sk)
>   		return;
>   }
>   
> -static struct sock *smc_sock_alloc(struct net *net, struct socket *sock,
> -				   int protocol)
> +static void smc_sock_init(struct net *net, struct sock *sk, int protocol)
>   {
> -	struct smc_sock *smc;
> -	struct proto *prot;
> -	struct sock *sk;
> -
> -	prot = (protocol == SMCPROTO_SMC6) ? &smc_proto6 : &smc_proto;
> -	sk = sk_alloc(net, PF_SMC, GFP_KERNEL, prot, 0);
> -	if (!sk)
> -		return NULL;
> +	struct smc_sock *smc = smc_sk(sk);
>   
> -	sock_init_data(sock, sk); /* sets sk_refcnt to 1 */
>   	sk->sk_state = SMC_INIT;
> -	sk->sk_destruct = smc_destruct;
>   	sk->sk_protocol = protocol;
> +	mutex_init(&smc->clcsock_release_lock);

Please add mutex_destroy(&smc->clcsock_release_lock); when 
smc->clcsock_release_lock is no longer used.

Or else some tools will notify errors.

Zhu Yanjun

>   	WRITE_ONCE(sk->sk_sndbuf, 2 * READ_ONCE(net->smc.sysctl_wmem));
>   	WRITE_ONCE(sk->sk_rcvbuf, 2 * READ_ONCE(net->smc.sysctl_rmem));
> -	smc = smc_sk(sk);
>   	INIT_WORK(&smc->tcp_listen_work, smc_tcp_listen_work);
>   	INIT_WORK(&smc->connect_work, smc_connect_work);
>   	INIT_DELAYED_WORK(&smc->conn.tx_work, smc_tx_work);
>   	INIT_LIST_HEAD(&smc->accept_q);
>   	spin_lock_init(&smc->accept_q_lock);
>   	spin_lock_init(&smc->conn.send_lock);
> -	sk->sk_prot->hash(sk);
> -	mutex_init(&smc->clcsock_release_lock);
>   	smc_init_saved_callbacks(smc);
> +	smc->limit_smc_hs = net->smc.limit_smc_hs;
> +	smc->use_fallback = false; /* assume rdma capability first */
> +	smc->fallback_rsn = 0;
> +
> +	sk->sk_destruct = smc_destruct;
> +	sk->sk_prot->hash(sk);
> +}
> +
> +static struct sock *smc_sock_alloc(struct net *net, struct socket *sock,
> +				   int protocol)
> +{
> +	struct proto *prot;
> +	struct sock *sk;
> +
> +	prot = (protocol == SMCPROTO_SMC6) ? &smc_proto6 : &smc_proto;
> +	sk = sk_alloc(net, PF_SMC, GFP_KERNEL, prot, 0);
> +	if (!sk)
> +		return NULL;
> +
> +	sock_init_data(sock, sk); /* sets sk_refcnt to 1 */
> +	smc_sock_init(net, sk, protocol);
>   
>   	return sk;
>   }
> @@ -3321,6 +3330,31 @@ static ssize_t smc_splice_read(struct socket *sock, loff_t *ppos,
>   	.splice_read	= smc_splice_read,
>   };
>   
> +static int __smc_create_clcsk(struct net *net, struct sock *sk, int family)
> +{
> +	struct smc_sock *smc = smc_sk(sk);
> +	int rc;
> +
> +	rc = sock_create_kern(net, family, SOCK_STREAM, IPPROTO_TCP,
> +			      &smc->clcsock);
> +	if (rc) {
> +		sk_common_release(sk);
> +		return rc;
> +	}
> +
> +	/* smc_clcsock_release() does not wait smc->clcsock->sk's
> +	 * destruction;  its sk_state might not be TCP_CLOSE after
> +	 * smc->sk is close()d, and TCP timers can be fired later,
> +	 * which need net ref.
> +	 */
> +	sk = smc->clcsock->sk;
> +	__netns_tracker_free(net, &sk->ns_tracker, false);
> +	sk->sk_net_refcnt = 1;
> +	get_net_track(net, &sk->ns_tracker, GFP_KERNEL);
> +	sock_inuse_add(net, 1);
> +	return 0;
> +}
> +
>   static int __smc_create(struct net *net, struct socket *sock, int protocol,
>   			int kern, struct socket *clcsock)
>   {
> @@ -3346,35 +3380,12 @@ static int __smc_create(struct net *net, struct socket *sock, int protocol,
>   
>   	/* create internal TCP socket for CLC handshake and fallback */
>   	smc = smc_sk(sk);
> -	smc->use_fallback = false; /* assume rdma capability first */
> -	smc->fallback_rsn = 0;
> -
> -	/* default behavior from limit_smc_hs in every net namespace */
> -	smc->limit_smc_hs = net->smc.limit_smc_hs;
>   
>   	rc = 0;
> -	if (!clcsock) {
> -		rc = sock_create_kern(net, family, SOCK_STREAM, IPPROTO_TCP,
> -				      &smc->clcsock);
> -		if (rc) {
> -			sk_common_release(sk);
> -			goto out;
> -		}
> -
> -		/* smc_clcsock_release() does not wait smc->clcsock->sk's
> -		 * destruction;  its sk_state might not be TCP_CLOSE after
> -		 * smc->sk is close()d, and TCP timers can be fired later,
> -		 * which need net ref.
> -		 */
> -		sk = smc->clcsock->sk;
> -		__netns_tracker_free(net, &sk->ns_tracker, false);
> -		sk->sk_net_refcnt = 1;
> -		get_net_track(net, &sk->ns_tracker, GFP_KERNEL);
> -		sock_inuse_add(net, 1);
> -	} else {
> +	if (!clcsock)
> +		rc = __smc_create_clcsk(net, sk, family);
> +	else
>   		smc->clcsock = clcsock;
> -	}
> -
>   out:
>   	return rc;
>   }
D. Wythe May 13, 2024, 3:22 a.m. UTC | #4
On 5/11/24 8:21 PM, Zhu Yanjun wrote:
> 在 2024/5/10 6:12, D. Wythe 写道:
>> From: "D. Wythe" <alibuda@linux.alibaba.com>
>>
>> This patch aims to isolate the shared components of SMC socket
>> allocation by introducing smc_sock_init() for sock initialization
>> and __smc_create_clcsk() for the initialization of clcsock.
>>
>> This is in preparation for the subsequent implementation of the
>> AF_INET version of SMC.
>>
>> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
>> ---
>>   net/smc/af_smc.c | 93 
>> +++++++++++++++++++++++++++++++-------------------------
>>   1 file changed, 52 insertions(+), 41 deletions(-)
>>
>> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
>> index 9389f0c..1f03724 100644
>> --- a/net/smc/af_smc.c
>> +++ b/net/smc/af_smc.c
>> @@ -361,34 +361,43 @@ static void smc_destruct(struct sock *sk)
>>           return;
>>   }
>>   -static struct sock *smc_sock_alloc(struct net *net, struct socket 
>> *sock,
>> -                   int protocol)
>> +static void smc_sock_init(struct net *net, struct sock *sk, int 
>> protocol)
>>   {
>> -    struct smc_sock *smc;
>> -    struct proto *prot;
>> -    struct sock *sk;
>> -
>> -    prot = (protocol == SMCPROTO_SMC6) ? &smc_proto6 : &smc_proto;
>> -    sk = sk_alloc(net, PF_SMC, GFP_KERNEL, prot, 0);
>> -    if (!sk)
>> -        return NULL;
>> +    struct smc_sock *smc = smc_sk(sk);
>>   -    sock_init_data(sock, sk); /* sets sk_refcnt to 1 */
>>       sk->sk_state = SMC_INIT;
>> -    sk->sk_destruct = smc_destruct;
>>       sk->sk_protocol = protocol;
>> +    mutex_init(&smc->clcsock_release_lock);
>
> Please add mutex_destroy(&smc->clcsock_release_lock); when 
> smc->clcsock_release_lock is no longer used.
>
> Or else some tools will notify errors.
>
> Zhu Yanjun


It seems that the problem you mentioned is not caused by this patch, 
after all, this patch is solely for refactoring.
Adding the fix you mentioned in this refactoring patch would not be 
appropriate. Perhaps, you could submit a separate
patch to address the issue. What do you think?

D. Wythe

>
>>       WRITE_ONCE(sk->sk_sndbuf, 2 * READ_ONCE(net->smc.sysctl_wmem));
>>       WRITE_ONCE(sk->sk_rcvbuf, 2 * READ_ONCE(net->smc.sysctl_rmem));
>> -    smc = smc_sk(sk);
>>       INIT_WORK(&smc->tcp_listen_work, smc_tcp_listen_work);
>>       INIT_WORK(&smc->connect_work, smc_connect_work);
>>       INIT_DELAYED_WORK(&smc->conn.tx_work, smc_tx_work);
>>       INIT_LIST_HEAD(&smc->accept_q);
>>       spin_lock_init(&smc->accept_q_lock);
>>       spin_lock_init(&smc->conn.send_lock);
>> -    sk->sk_prot->hash(sk);
>> -    mutex_init(&smc->clcsock_release_lock);
>>       smc_init_saved_callbacks(smc);
>> +    smc->limit_smc_hs = net->smc.limit_smc_hs;
>> +    smc->use_fallback = false; /* assume rdma capability first */
>> +    smc->fallback_rsn = 0;
>> +
>> +    sk->sk_destruct = smc_destruct;
>> +    sk->sk_prot->hash(sk);
>> +}
>> +
>> +static struct sock *smc_sock_alloc(struct net *net, struct socket 
>> *sock,
>> +                   int protocol)
>> +{
>> +    struct proto *prot;
>> +    struct sock *sk;
>> +
>> +    prot = (protocol == SMCPROTO_SMC6) ? &smc_proto6 : &smc_proto;
>> +    sk = sk_alloc(net, PF_SMC, GFP_KERNEL, prot, 0);
>> +    if (!sk)
>> +        return NULL;
>> +
>> +    sock_init_data(sock, sk); /* sets sk_refcnt to 1 */
>> +    smc_sock_init(net, sk, protocol);
>>         return sk;
>>   }
>> @@ -3321,6 +3330,31 @@ static ssize_t smc_splice_read(struct socket 
>> *sock, loff_t *ppos,
>>       .splice_read    = smc_splice_read,
>>   };
>>   +static int __smc_create_clcsk(struct net *net, struct sock *sk, 
>> int family)
>> +{
>> +    struct smc_sock *smc = smc_sk(sk);
>> +    int rc;
>> +
>> +    rc = sock_create_kern(net, family, SOCK_STREAM, IPPROTO_TCP,
>> +                  &smc->clcsock);
>> +    if (rc) {
>> +        sk_common_release(sk);
>> +        return rc;
>> +    }
>> +
>> +    /* smc_clcsock_release() does not wait smc->clcsock->sk's
>> +     * destruction;  its sk_state might not be TCP_CLOSE after
>> +     * smc->sk is close()d, and TCP timers can be fired later,
>> +     * which need net ref.
>> +     */
>> +    sk = smc->clcsock->sk;
>> +    __netns_tracker_free(net, &sk->ns_tracker, false);
>> +    sk->sk_net_refcnt = 1;
>> +    get_net_track(net, &sk->ns_tracker, GFP_KERNEL);
>> +    sock_inuse_add(net, 1);
>> +    return 0;
>> +}
>> +
>>   static int __smc_create(struct net *net, struct socket *sock, int 
>> protocol,
>>               int kern, struct socket *clcsock)
>>   {
>> @@ -3346,35 +3380,12 @@ static int __smc_create(struct net *net, 
>> struct socket *sock, int protocol,
>>         /* create internal TCP socket for CLC handshake and fallback */
>>       smc = smc_sk(sk);
>> -    smc->use_fallback = false; /* assume rdma capability first */
>> -    smc->fallback_rsn = 0;
>> -
>> -    /* default behavior from limit_smc_hs in every net namespace */
>> -    smc->limit_smc_hs = net->smc.limit_smc_hs;
>>         rc = 0;
>> -    if (!clcsock) {
>> -        rc = sock_create_kern(net, family, SOCK_STREAM, IPPROTO_TCP,
>> -                      &smc->clcsock);
>> -        if (rc) {
>> -            sk_common_release(sk);
>> -            goto out;
>> -        }
>> -
>> -        /* smc_clcsock_release() does not wait smc->clcsock->sk's
>> -         * destruction;  its sk_state might not be TCP_CLOSE after
>> -         * smc->sk is close()d, and TCP timers can be fired later,
>> -         * which need net ref.
>> -         */
>> -        sk = smc->clcsock->sk;
>> -        __netns_tracker_free(net, &sk->ns_tracker, false);
>> -        sk->sk_net_refcnt = 1;
>> -        get_net_track(net, &sk->ns_tracker, GFP_KERNEL);
>> -        sock_inuse_add(net, 1);
>> -    } else {
>> +    if (!clcsock)
>> +        rc = __smc_create_clcsk(net, sk, family);
>> +    else
>>           smc->clcsock = clcsock;
>> -    }
>> -
>>   out:
>>       return rc;
>>   }
diff mbox series

Patch

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 9389f0c..1f03724 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -361,34 +361,43 @@  static void smc_destruct(struct sock *sk)
 		return;
 }
 
-static struct sock *smc_sock_alloc(struct net *net, struct socket *sock,
-				   int protocol)
+static void smc_sock_init(struct net *net, struct sock *sk, int protocol)
 {
-	struct smc_sock *smc;
-	struct proto *prot;
-	struct sock *sk;
-
-	prot = (protocol == SMCPROTO_SMC6) ? &smc_proto6 : &smc_proto;
-	sk = sk_alloc(net, PF_SMC, GFP_KERNEL, prot, 0);
-	if (!sk)
-		return NULL;
+	struct smc_sock *smc = smc_sk(sk);
 
-	sock_init_data(sock, sk); /* sets sk_refcnt to 1 */
 	sk->sk_state = SMC_INIT;
-	sk->sk_destruct = smc_destruct;
 	sk->sk_protocol = protocol;
+	mutex_init(&smc->clcsock_release_lock);
 	WRITE_ONCE(sk->sk_sndbuf, 2 * READ_ONCE(net->smc.sysctl_wmem));
 	WRITE_ONCE(sk->sk_rcvbuf, 2 * READ_ONCE(net->smc.sysctl_rmem));
-	smc = smc_sk(sk);
 	INIT_WORK(&smc->tcp_listen_work, smc_tcp_listen_work);
 	INIT_WORK(&smc->connect_work, smc_connect_work);
 	INIT_DELAYED_WORK(&smc->conn.tx_work, smc_tx_work);
 	INIT_LIST_HEAD(&smc->accept_q);
 	spin_lock_init(&smc->accept_q_lock);
 	spin_lock_init(&smc->conn.send_lock);
-	sk->sk_prot->hash(sk);
-	mutex_init(&smc->clcsock_release_lock);
 	smc_init_saved_callbacks(smc);
+	smc->limit_smc_hs = net->smc.limit_smc_hs;
+	smc->use_fallback = false; /* assume rdma capability first */
+	smc->fallback_rsn = 0;
+
+	sk->sk_destruct = smc_destruct;
+	sk->sk_prot->hash(sk);
+}
+
+static struct sock *smc_sock_alloc(struct net *net, struct socket *sock,
+				   int protocol)
+{
+	struct proto *prot;
+	struct sock *sk;
+
+	prot = (protocol == SMCPROTO_SMC6) ? &smc_proto6 : &smc_proto;
+	sk = sk_alloc(net, PF_SMC, GFP_KERNEL, prot, 0);
+	if (!sk)
+		return NULL;
+
+	sock_init_data(sock, sk); /* sets sk_refcnt to 1 */
+	smc_sock_init(net, sk, protocol);
 
 	return sk;
 }
@@ -3321,6 +3330,31 @@  static ssize_t smc_splice_read(struct socket *sock, loff_t *ppos,
 	.splice_read	= smc_splice_read,
 };
 
+static int __smc_create_clcsk(struct net *net, struct sock *sk, int family)
+{
+	struct smc_sock *smc = smc_sk(sk);
+	int rc;
+
+	rc = sock_create_kern(net, family, SOCK_STREAM, IPPROTO_TCP,
+			      &smc->clcsock);
+	if (rc) {
+		sk_common_release(sk);
+		return rc;
+	}
+
+	/* smc_clcsock_release() does not wait smc->clcsock->sk's
+	 * destruction;  its sk_state might not be TCP_CLOSE after
+	 * smc->sk is close()d, and TCP timers can be fired later,
+	 * which need net ref.
+	 */
+	sk = smc->clcsock->sk;
+	__netns_tracker_free(net, &sk->ns_tracker, false);
+	sk->sk_net_refcnt = 1;
+	get_net_track(net, &sk->ns_tracker, GFP_KERNEL);
+	sock_inuse_add(net, 1);
+	return 0;
+}
+
 static int __smc_create(struct net *net, struct socket *sock, int protocol,
 			int kern, struct socket *clcsock)
 {
@@ -3346,35 +3380,12 @@  static int __smc_create(struct net *net, struct socket *sock, int protocol,
 
 	/* create internal TCP socket for CLC handshake and fallback */
 	smc = smc_sk(sk);
-	smc->use_fallback = false; /* assume rdma capability first */
-	smc->fallback_rsn = 0;
-
-	/* default behavior from limit_smc_hs in every net namespace */
-	smc->limit_smc_hs = net->smc.limit_smc_hs;
 
 	rc = 0;
-	if (!clcsock) {
-		rc = sock_create_kern(net, family, SOCK_STREAM, IPPROTO_TCP,
-				      &smc->clcsock);
-		if (rc) {
-			sk_common_release(sk);
-			goto out;
-		}
-
-		/* smc_clcsock_release() does not wait smc->clcsock->sk's
-		 * destruction;  its sk_state might not be TCP_CLOSE after
-		 * smc->sk is close()d, and TCP timers can be fired later,
-		 * which need net ref.
-		 */
-		sk = smc->clcsock->sk;
-		__netns_tracker_free(net, &sk->ns_tracker, false);
-		sk->sk_net_refcnt = 1;
-		get_net_track(net, &sk->ns_tracker, GFP_KERNEL);
-		sock_inuse_add(net, 1);
-	} else {
+	if (!clcsock)
+		rc = __smc_create_clcsk(net, sk, family);
+	else
 		smc->clcsock = clcsock;
-	}
-
 out:
 	return rc;
 }