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

["D. Wythe" <alibuda@xxxxxxxxxxxxxxxxx>]

On 5/11/24 8:21 PM, Zhu Yanjun wrote:
> 在 2024/5/10 6:12, D. Wythe 写道:
> > From: "D. Wythe" <alibuda@xxxxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxx>
> > ---
> >   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;
> >   }