net/smc: Optimizing performance in short-lived scenarios

net/smc/af_smc.c +34 -0

+
+	if (READ_ONCE(sk->sk_ack_backlog) + atomic_read(&smc->smc_pendings) >
+				sk->sk_max_ack_backlog)
+		goto drop;
+
+	if (sk_acceptq_is_full(&smc->sk)) {
+		NET_INC_STATS(sock_net(sk), LINUX_MIB_LISTENOVERFLOWS);
+		goto drop;
+	}
+
+	/* passthrough to origin syn recv sock fct */
+	return smc->ori_af_ops->syn_recv_sock(sk, skb, req, dst, req_unhash, own_req);
+
+drop:
+	dst_release(dst);
+	tcp_listendrop(sk);
+	return NULL;
+}
+
 static struct smc_hashinfo smc_v4_hashinfo = {
 	.lock = __RW_LOCK_UNLOCKED(smc_v4_hashinfo.lock),
 };

@@ -1595,6 +1623,9 @@ static void smc_listen_out(struct smc_sock *new_smc)
 	struct smc_sock *lsmc = new_smc->listen_smc;
 	struct sock *newsmcsk = &new_smc->sk;
 
+	if (tcp_sk(new_smc->clcsock->sk)->syn_smc)
+		atomic_dec(&lsmc->smc_pendings);
+
 	if (lsmc->sk.sk_state == SMC_LISTEN) {
 		lock_sock_nested(&lsmc->sk, SINGLE_DEPTH_NESTING);
 		smc_accept_enqueue(&lsmc->sk, newsmcsk);

@@ -2200,6 +2231,9 @@ static void smc_tcp_listen_work(struct work_struct *work)
 		if (!new_smc)
 			continue;
 
+		if (tcp_sk(new_smc->clcsock->sk)->syn_smc)
+			atomic_inc(&lsmc->smc_pendings);
+
 		new_smc->listen_smc = lsmc;
 		new_smc->use_fallback = lsmc->use_fallback;
 		new_smc->fallback_rsn = lsmc->fallback_rsn;

@@ -2266,6 +2300,15 @@ static int smc_listen(struct socket *sock, int backlog)
 	smc->clcsock->sk->sk_data_ready = smc_clcsock_data_ready;
 	smc->clcsock->sk->sk_user_data =
 		(void *)((uintptr_t)smc | SK_USER_DATA_NOCOPY);
+
+	/* save origin ops */
+	smc->ori_af_ops = inet_csk(smc->clcsock->sk)->icsk_af_ops;
+
+	smc->af_ops = *smc->ori_af_ops;
+	smc->af_ops.syn_recv_sock = smc_tcp_syn_recv_sock;
+
+	inet_csk(smc->clcsock->sk)->icsk_af_ops = &smc->af_ops;
+
 	rc = kernel_listen(smc->clcsock, backlog);
 	if (rc) {
 		smc->clcsock->sk->sk_data_ready = smc->clcsk_data_ready;

net/smc/smc.h +0 -0

index 37b2001..5e5e38d 100644

net/smc/smc.h +0 -0

 	int			sockopt_defer_accept;
 						/* sockopt TCP_DEFER_ACCEPT
 						 * value

net/smc/af_smc.c static void smc_set_keepalive(struct sock *sk, int val) · line 84

Quoted 9 lines · D. Wythe

net/smc/af_smc.c +9 -0

+static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk, struct sk_buff *skb,
+					  struct request_sock *req,
+					  struct dst_entry *dst,
+					  struct request_sock *req_unhash,
+					  bool *own_req)
+{
+	struct smc_sock *smc;
+
+	smc = (struct smc_sock *)((uintptr_t)sk->sk_user_data & ~SK_USER_DATA_NOCOPY);

Did you run checkpatch.pl for these patches, for me this and other lines look longer than 80 characters.

net/smc/smc.h struct smc_sock { /* smc sock container */ · line 255

Quoted 9 lines

net/smc/smc.h +1 -0

index 37b2001..5e5e38d 100644
@@ -252,6 +252,10 @@ struct smc_sock {				/* smc sock container */
  	bool			use_fallback;	/* fallback to tcp */
  	int			fallback_rsn;	/* reason for fallback */
  	u32			peer_diagnosis; /* decline reason from peer */
+	atomic_t                smc_pendings;   /* pending smc connections */

I don't like the name smc_pendings, its not very specific. What about queued_smc_hs?
And for the comment: queued smc handshakes

net/smc/smc.h struct smc_sock { /* smc sock container */ · line 258

Quoted 3 lines

net/smc/smc.h +3 -0

+	struct inet_connection_sock_af_ops		af_ops;
+	const struct inet_connection_sock_af_ops	*ori_af_ops;
+						/* origin af ops */

origin -> original

Quoted 3 lines

net/smc/smc.h +0 -0

  	int			sockopt_defer_accept;
  						/* sockopt TCP_DEFER_ACCEPT
  						 * value

From: "D. Wythe" <[email protected]>

This patch intends to provide a mechanism to allow automatic fallback to

TCP according to the pressure of SMC handshake process. At present, frequent visits will cause the incoming connections to be backlogged in SMC handshake queue, raise the connections established time. Which is quite unacceptable for those applications who base on short lived connections.

There are two ways to implement this mechanism:

1. Fallback when TCP established.
2. Fallback before TCP established.

In the first way, we need to wait and receive CLC messages that the client will potentially send, and then actively reply with a decline message, in a sense, which is also a sort of SMC handshake, affect the connections established time on its way.

In the second way, the only problem is that we need to inject SMC logic into TCP when it is about to reply the incoming SYN, since we already do that, it's seems not a problem anymore. And advantage is obvious, few additional processes are required to complete the fallback.

This patch use the second way.

Link https://lore.kernel.org/all/[email protected]/ open here

Signed-off-by D. Wythe <[email protected]>

---
 include/linux/tcp.h  |  1 +
 net/ipv4/tcp_input.c |  3 ++-
 net/smc/af_smc.c     | 18 ++++++++++++++++++
 3 files changed, 21 insertions(+), 1 deletion(-)

include/linux/tcp.h +0 -0

index 78b91bb..1c4ae5d 100644

include/linux/tcp.h +0 -0

 	bool	syn_smc;	/* SYN includes SMC */
 #endif
 

net/ipv4/tcp_input.c +0 -0

index af94a6d..e817ec6 100644

net/smc/af_smc.c +15 -0

+{
+	const struct smc_sock *smc;
+
+	smc = (const struct smc_sock *)
+		((uintptr_t)sk->sk_user_data & ~SK_USER_DATA_NOCOPY);
+
+	if (!smc)
+		return true;
+
+	if (workqueue_congested(WORK_CPU_UNBOUND, smc_hs_wq))
+		return true;
+
+	return false;
+}
+
 static struct smc_hashinfo smc_v4_hashinfo = {
 	.lock = __RW_LOCK_UNLOCKED(smc_v4_hashinfo.lock),
 };

net/smc/af_smc.c +1 -0

+
 	rc = kernel_listen(smc->clcsock, backlog);
 	if (rc) {
 		smc->clcsock->sk->sk_data_ready = smc->clcsk_data_ready;

Quoted 3 lines · D. Wythe

From: "D. Wythe" <[email protected]>

This patch intends to provide a mechanism to allow automatic fallback to

I would like to avoid the wording fallback all over here. The term SMC fallback is used for SMC connections that are in our socket list, but use TCP because something went wrong during handshake.
What you changes result in are TCP-only connections which are not handled by the SMC module at all. So the comments should use a different naming for that. What the patch actually does is to disable the SMC experimental TCP header option, so the client receives no SMC indication and does not proceed with SMC. Is this correct?

Please also see my comments below.

Quoted 36 lines

TCP according to the pressure of SMC handshake process. At present, frequent visits will cause the incoming connections to be backlogged in SMC handshake queue, raise the connections established time. Which is quite unacceptable for those applications who base on short lived connections.

There are two ways to implement this mechanism:

1. Fallback when TCP established.
2. Fallback before TCP established.

In the first way, we need to wait and receive CLC messages that the client will potentially send, and then actively reply with a decline message, in a sense, which is also a sort of SMC handshake, affect the connections established time on its way.

In the second way, the only problem is that we need to inject SMC logic into TCP when it is about to reply the incoming SYN, since we already do that, it's seems not a problem anymore. And advantage is obvious, few additional processes are required to complete the fallback.

This patch use the second way.

Link https://lore.kernel.org/all/[email protected]/ open here

Signed-off-by D. Wythe <[email protected]>

---
  include/linux/tcp.h  |  1 +
  net/ipv4/tcp_input.c |  3 ++-
  net/smc/af_smc.c     | 18 ++++++++++++++++++
  3 files changed, 21 insertions(+), 1 deletion(-)

include/linux/tcp.h +0 -0

index 78b91bb..1c4ae5d 100644

include/linux/tcp.h struct tcp_sock { · line 397

Quoted 4 lines

include/linux/tcp.h +1 -0

@@ -394,6 +394,7 @@ struct tcp_sock {
  	bool	is_mptcp;
  #endif
  #if IS_ENABLED(CONFIG_SMC)
+	bool	(*smc_in_limited)(const struct sock *sk);

Better variable name: smc_hs_congested

Quoted 8 lines

include/linux/tcp.h +0 -0

  	bool	syn_smc;	/* SYN includes SMC */
  #endif
 

net/ipv4/tcp_input.c +0 -0

index af94a6d..e817ec6 100644

net/ipv4/tcp_input.c static void tcp_openreq_init(struct request_sock *req, · line 6707

Quoted 6 lines

net/ipv4/tcp_input.c +2 -1

@@ -6703,7 +6703,8 @@ static void tcp_openreq_init(struct request_sock *req,
  	ireq->ir_num = ntohs(tcp_hdr(skb)->dest);
  	ireq->ir_mark = inet_request_mark(sk, skb);
  #if IS_ENABLED(CONFIG_SMC)
-	ireq->smc_ok = rx_opt->smc_ok;
+	ireq->smc_ok = rx_opt->smc_ok && !(tcp_sk(sk)->smc_in_limited &&
+			tcp_sk(sk)->smc_in_limited(sk));

Use new name here and elsewhere ...

net/smc/af_smc.c static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk, struct sk_buff · line 104

Quoted 12 lines

net/ipv4/tcp_input.c +0 -0

  #endif
  }
 

net/smc/af_smc.c +1 -0

index ebfce3d..8175f60 100644
@@ -101,6 +101,22 @@ static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk, struct sk_buff
  	return NULL;
  }
 
+static bool smc_is_in_limited(const struct sock *sk)

Better function name: smc_hs_congested()

Quoted 20 lines

net/smc/af_smc.c +15 -0

+{
+	const struct smc_sock *smc;
+
+	smc = (const struct smc_sock *)
+		((uintptr_t)sk->sk_user_data & ~SK_USER_DATA_NOCOPY);
+
+	if (!smc)
+		return true;
+
+	if (workqueue_congested(WORK_CPU_UNBOUND, smc_hs_wq))
+		return true;
+
+	return false;
+}
+
  static struct smc_hashinfo smc_v4_hashinfo = {
  	.lock = __RW_LOCK_UNLOCKED(smc_v4_hashinfo.lock),
  };

@@ -2309,6 +2325,8 @@ static int smc_listen(struct socket *sock, int backlog)
 

net/smc/af_smc.c static int smc_listen(struct socket *sock, int backlog) · line 2328

Quoted 3 lines

net/smc/af_smc.c +1 -0

  	inet_csk(smc->clcsock->sk)->icsk_af_ops = &smc->af_ops;
 
+	tcp_sk(smc->clcsock->sk)->smc_in_limited = smc_is_in_limited;

Use new names here, too.

Quoted 4 lines

net/smc/af_smc.c +1 -0

+
  	rc = kernel_listen(smc->clcsock, backlog);
  	if (rc) {
  		smc->clcsock->sk->sk_data_ready = smc->clcsk_data_ready;

From: "D. Wythe" <[email protected]>

This patch aims to add dynamic control for SMC auto fallback, since we

don't have socket option level for SMC yet, which requires we need to implement it at the same time.

This patch does the following:

- add new socket option level: SOL_SMC.
- add new SMC socket option: SMC_AUTO_FALLBACK.
- provide getter/setter for SMC socket options.

Signed-off-by D. Wythe <[email protected]>

---
 include/linux/socket.h   |  1 +
 include/uapi/linux/smc.h |  4 +++
 net/smc/af_smc.c         | 69 +++++++++++++++++++++++++++++++++++++++++++++++-
 net/smc/smc.h            |  1 +
 4 files changed, 74 insertions(+), 1 deletion(-)

include/linux/socket.h +1 -0

index 8ef26d8..6f85f5d 100644
@@ -366,6 +366,7 @@ struct ucred {
 #define SOL_XDP		283
 #define SOL_MPTCP	284
 #define SOL_MCTP	285
+#define SOL_SMC		286
 
 /* IPX options */
 #define IPX_TYPE	1

include/uapi/linux/smc.h +0 -0

index 6c2874f..9f2cbf8 100644

include/uapi/linux/smc.h +1 -0

+
 #endif /* _UAPI_LINUX_SMC_H */

net/smc/af_smc.c +68 -1

index 8175f60..c313561 100644
@@ -2325,7 +2325,8 @@ static int smc_listen(struct socket *sock, int backlog)
 
 	inet_csk(smc->clcsock->sk)->icsk_af_ops = &smc->af_ops;
 
-	tcp_sk(smc->clcsock->sk)->smc_in_limited = smc_is_in_limited;
+	if (smc->auto_fallback)
+		tcp_sk(smc->clcsock->sk)->smc_in_limited = smc_is_in_limited;
 
 	rc = kernel_listen(smc->clcsock, backlog);
 	if (rc) {

@@ -2620,6 +2621,67 @@ static int smc_shutdown(struct socket *sock, int how)
 	return rc ? rc : rc1;
 }
 
+static int __smc_getsockopt(struct socket *sock, int level, int optname,
+			    char __user *optval, int __user *optlen)
+{
+	struct smc_sock *smc;
+	int val, len;
+
+	smc = smc_sk(sock->sk);
+
+	if (get_user(len, optlen))
+		return -EFAULT;
+
+	len = min_t(int, len, sizeof(int));
+
+	if (len < 0)
+		return -EINVAL;
+
+	switch (optname) {
+	case SMC_AUTO_FALLBACK:
+		val = smc->auto_fallback;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	if (put_user(len, optlen))
+		return -EFAULT;
+	if (copy_to_user(optval, &val, len))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int __smc_setsockopt(struct socket *sock, int level, int optname,
+			    sockptr_t optval, unsigned int optlen)
+{
+	struct sock *sk = sock->sk;
+	struct smc_sock *smc;
+	int val, rc;
+
+	smc = smc_sk(sk);
+
+	lock_sock(sk);
+	switch (optname) {
+	case SMC_AUTO_FALLBACK:
+		if (optlen < sizeof(int))
+			return -EINVAL;
+		if (copy_from_sockptr(&val, optval, sizeof(int)))
+			return -EFAULT;
+
+		smc->auto_fallback = !!val;
+		rc = 0;
+		break;
+	default:
+		rc = -EOPNOTSUPP;
+		break;
+	}
+	release_sock(sk);
+
+	return rc;
+}
+
 static int smc_setsockopt(struct socket *sock, int level, int optname,
 			  sockptr_t optval, unsigned int optlen)
 {

@@ -2629,6 +2691,8 @@ static int smc_setsockopt(struct socket *sock, int level, int optname,
 
 	if (level == SOL_TCP && optname == TCP_ULP)
 		return -EOPNOTSUPP;
+	else if (level == SOL_SMC)
+		return __smc_setsockopt(sock, level, optname, optval, optlen);
 
 	smc = smc_sk(sk);
 

@@ -2711,6 +2775,9 @@ static int smc_getsockopt(struct socket *sock, int level, int optname,
 	struct smc_sock *smc;
 	int rc;
 
+	if (level == SOL_SMC)
+		return __smc_getsockopt(sock, level, optname, optval, optlen);
+
 	smc = smc_sk(sock->sk);
 	mutex_lock(&smc->clcsock_release_lock);
 	if (!smc->clcsock) {

net/smc/smc.h +0 -0

index 5e5e38d..a0bdf75 100644

net/smc/smc.h +0 -0

 	bool			use_fallback;	/* fallback to tcp */
 	int			fallback_rsn;	/* reason for fallback */
 	u32			peer_diagnosis; /* decline reason from peer */

From: "D. Wythe" <[email protected]>

Although we can control SMC auto fallback through socket options, which means that applications who need it must modify their code. It's quite troublesome for many existing applications. This patch modifies the global default value of auto fallback through netlink, providing a way to auto fallback without modifying any code for applications.

Quoted 8 lines · D. Wythe

From: "D. Wythe" <[email protected]>

Although we can control SMC auto fallback through socket options, which means that applications who need it must modify their code. It's quite troublesome for many existing applications. This patch modifies the global default value of auto fallback through netlink, providing a way to auto fallback without modifying any code for applications.

And of course also in this patch: no "auto fallback" in comments or as part of variable names.

Do you plan to enhance the smc-tools user space part, too?