From: "D. Wythe" <alibuda@linux.alibaba.com>
To: Zhu Yanjun <yanjun.zhu@linux.dev>,
kgraul@linux.ibm.com, wenjia@linux.ibm.com, jaka@linux.ibm.com,
wintera@linux.ibm.com, guwen@linux.alibaba.com
Cc: kuba@kernel.org, davem@davemloft.net, netdev@vger.kernel.org,
linux-s390@vger.kernel.org, linux-rdma@vger.kernel.org,
tonylu@linux.alibaba.com, pabeni@redhat.com, edumazet@google.com
Subject: Re: [PATCH net-next 1/2] net/smc: refatoring initialization of smc sock
Date: Mon, 13 May 2024 11:22:18 +0800 [thread overview]
Message-ID: <52825ab1-9162-422b-93f7-5981e3b6ad78@linux.alibaba.com> (raw)
In-Reply-To: <11f7d33c-80b1-40db-87c0-566ed24c389e@linux.dev>
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;
>> }
next prev parent reply other threads:[~2024-05-13 3:22 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-10 4:12 [PATCH net-next 0/2] Introduce IPPROTO_SMC D. Wythe
2024-05-10 4:12 ` [PATCH net-next 1/2] net/smc: refatoring initialization of smc sock D. Wythe
2024-05-10 9:50 ` Dust Li
2024-05-11 2:26 ` D. Wythe
2024-05-11 12:21 ` Zhu Yanjun
2024-05-13 3:22 ` D. Wythe [this message]
2024-05-10 4:12 ` [PATCH net-next 2/2] net/smc: Introduce IPPROTO_SMC D. Wythe
2024-05-10 9:57 ` Dust Li
2024-05-11 2:23 ` D. Wythe
2024-05-11 2:46 ` Dust Li
2024-05-11 3:02 ` D. Wythe
2024-05-10 17:09 ` kernel test robot
2024-05-10 18:32 ` kernel test robot
2024-05-10 9:14 ` [PATCH net-next 0/2] " D. Wythe
2024-05-10 10:22 ` Wenjia Zhang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52825ab1-9162-422b-93f7-5981e3b6ad78@linux.alibaba.com \
--to=alibuda@linux.alibaba.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=guwen@linux.alibaba.com \
--cc=jaka@linux.ibm.com \
--cc=kgraul@linux.ibm.com \
--cc=kuba@kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=linux-s390@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=tonylu@linux.alibaba.com \
--cc=wenjia@linux.ibm.com \
--cc=wintera@linux.ibm.com \
--cc=yanjun.zhu@linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).