On 13/05/2022 04:24, Guangguan Wang wrote:
> Connect with O_NONBLOCK will not be completed immediately
> and returns -EINPROGRESS. It is possible to use selector/poll
> for completion by selecting the socket for writing. After select
> indicates writability, a second connect function call will return
> 0 to indicate connected successfully as TCP does, but smc returns
> -EISCONN. Use socket state for smc to indicate connect state, which
> can help smc aligning the connect behaviour with TCP.
>
> Signed-off-by: Guangguan Wang <guangguan.wang@xxxxxxxxxxxxxxxxx>
> Acked-by: Karsten Graul <kgraul@xxxxxxxxxxxxx>
> ---
> net/smc/af_smc.c | 50 ++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 46 insertions(+), 4 deletions(-)
>
> diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
> index fce16b9d6e1a..5f70642a8044 100644
> --- a/net/smc/af_smc.c
> +++ b/net/smc/af_smc.c
> @@ -1544,9 +1544,29 @@ static int smc_connect(struct socket *sock, struct sockaddr *addr,
> goto out_err;
>
> lock_sock(sk);
> + switch (sock->state) {
> + default:
> + rc = -EINVAL;
> + goto out;
> + case SS_CONNECTED:
> + rc = sk->sk_state == SMC_ACTIVE ? -EISCONN : -EINVAL;
> + goto out;
> + case SS_CONNECTING:
> + if (sk->sk_state == SMC_ACTIVE)
> + goto connected;
I stumbled over this when thinking about the fallback processing. If for whatever reason
fallback==true during smc_connect(), the "if (smc->use_fallback)" below would set sock->state
to e.g. SS_CONNECTED. But in the fallback case sk_state keeps SMC_INIT. So during the next call
the SS_CONNECTING case above would break because sk_state in NOT SMC_ACTIVE, and we would end
up calling kernel_connect() again. Which seems to be no problem when kernel_connect() returns
-EISCONN and we return this to the caller. But is this how it should work, or does it work by chance?
> + break;
> + case SS_UNCONNECTED:
> + sock->state = SS_CONNECTING;
> + break;
> + }
> +
> switch (sk->sk_state) {
> default:
> goto out;
> + case SMC_CLOSED:
> + rc = sock_error(sk) ? : -ECONNABORTED;
> + sock->state = SS_UNCONNECTED;
> + goto out;
> case SMC_ACTIVE:
> rc = -EISCONN;
> goto out;
> @@ -1565,20 +1585,24 @@ static int smc_connect(struct socket *sock, struct sockaddr *addr,
> goto out;
>
> sock_hold(&smc->sk); /* sock put in passive closing */
> - if (smc->use_fallback)
> + if (smc->use_fallback) {
> + sock->state = rc ? SS_CONNECTING : SS_CONNECTED;
> goto out;
> + }
> if (flags & O_NONBLOCK) {
> if (queue_work(smc_hs_wq, &smc->connect_work))
> smc->connect_nonblock = 1;
> rc = -EINPROGRESS;
> + goto out;
> } else {
> rc = __smc_connect(smc);
> if (rc < 0)
> goto out;
> - else
> - rc = 0; /* success cases including fallback */
> }
>
> +connected:
> + rc = 0;
> + sock->state = SS_CONNECTED;
> out:
> release_sock(sk);
> out_err:
> @@ -1693,6 +1717,7 @@ struct sock *smc_accept_dequeue(struct sock *parent,
> }
> if (new_sock) {
> sock_graft(new_sk, new_sock);
> + new_sock->state = SS_CONNECTED;
> if (isk->use_fallback) {
> smc_sk(new_sk)->clcsock->file = new_sock->file;
> isk->clcsock->file->private_data = isk->clcsock;
> @@ -2424,7 +2449,7 @@ static int smc_listen(struct socket *sock, int backlog)
>
> rc = -EINVAL;
> if ((sk->sk_state != SMC_INIT && sk->sk_state != SMC_LISTEN) ||
> - smc->connect_nonblock)
> + smc->connect_nonblock || sock->state != SS_UNCONNECTED)
> goto out;
>
> rc = 0;
> @@ -2716,6 +2741,17 @@ static int smc_shutdown(struct socket *sock, int how)
>
> lock_sock(sk);
>
> + if (sock->state == SS_CONNECTING) {
> + if (sk->sk_state == SMC_ACTIVE)
> + sock->state = SS_CONNECTED;
> + else if (sk->sk_state == SMC_PEERCLOSEWAIT1 ||
> + sk->sk_state == SMC_PEERCLOSEWAIT2 ||
> + sk->sk_state == SMC_APPCLOSEWAIT1 ||
> + sk->sk_state == SMC_APPCLOSEWAIT2 ||
> + sk->sk_state == SMC_APPFINCLOSEWAIT)
> + sock->state = SS_DISCONNECTING;
> + }
> +
> rc = -ENOTCONN;
> if ((sk->sk_state != SMC_ACTIVE) &&
> (sk->sk_state != SMC_PEERCLOSEWAIT1) &&
> @@ -2729,6 +2765,7 @@ static int smc_shutdown(struct socket *sock, int how)
> sk->sk_shutdown = smc->clcsock->sk->sk_shutdown;
> if (sk->sk_shutdown == SHUTDOWN_MASK) {
> sk->sk_state = SMC_CLOSED;
> + sk->sk_socket->state = SS_UNCONNECTED;
> sock_put(sk);
> }
> goto out;
> @@ -2754,6 +2791,10 @@ static int smc_shutdown(struct socket *sock, int how)
> /* map sock_shutdown_cmd constants to sk_shutdown value range */
> sk->sk_shutdown |= how + 1;
>
> + if (sk->sk_state == SMC_CLOSED)
> + sock->state = SS_UNCONNECTED;
> + else
> + sock->state = SS_DISCONNECTING;
> out:
> release_sock(sk);
> return rc ? rc : rc1;
> @@ -3139,6 +3180,7 @@ static int __smc_create(struct net *net, struct socket *sock, int protocol,
>
> rc = -ENOBUFS;
> sock->ops = &smc_sock_ops;
> + sock->state = SS_UNCONNECTED;
> sk = smc_sock_alloc(net, sock, protocol);
> if (!sk)
> goto out;
--
Karsten
