On 21.03.23 08:19, Kai Shen wrote:
SMC-R performs not so well on fallback situations right now,
especially on short link server fallback occasions. We are planning
to make SMC-R widely used and handling this fallback performance
issue is really crucial to us. Here we introduce a shadow socket
method to try to relief this problem.
Could you please elaborate the problem?
Basicly, we use two more accept queues to hold incoming connections,
one for fallback connections and the other for smc-r connections.
We implement this method by using two more 'shadow' sockets and
make the connection path of fallback connections almost the same as
normal tcp connections.
Now the SMC-R accept path is like:
1. incoming connection
2. schedule work to smc sock alloc, tcp accept and push to smc
acceptq
3. wake up user to accept
When fallback happens on servers, the accepting path is the same
which costs more than normal tcp accept path. In fallback
situations, the step 2 above is not necessary and the smc sock is
also not needed. So we use two more shadow sockets when one smc
socket start listening. When new connection comes, we pop the req
to the fallback socket acceptq or the non-fallback socket acceptq
according to its syn_smc flag. As a result, when fallback happen we
can graft the user socket with a normal tcp sock instead of a smc
sock and get rid of the cost generated by step 2 and smc sock
releasing.
+-----> non-fallback socket acceptq
|
incoming req --+
|
+-----> fallback socket acceptq
With the help of shadow socket, we gain similar performance as tcp
connections on short link nginx server fallback occasions as what
is illustrated below.
Cases are like "./wrk http://x.x.x.x:x/
-H 'Connection: Close' -c 1600 -t 32 -d 20 --latency"
TCP:
Requests/sec: 145438.65
Transfer/sec: 21.64MB
Server fallback occasions on original SMC-R:
Requests/sec: 114192.82
Transfer/sec: 16.99MB
Server fallback occasions on SMC-R with shadow sockets:
Requests/sec: 143528.11
Transfer/sec: 21.35MB
Generally, I don't have a good feeling about the two non-listenning
sockets, and I can not see why it is necessary to introduce the socket
actsock instead of using the clcsock itself. Maybe you can convince me
with a good reason.
On the other hand, as a result of using another accept queue, the
fastopenq lock is not the right lock to access when accepting. So
we need to find the right fastopenq lock in inet_csk_accept.
Signed-off-by: Kai Shen <KaiShen@xxxxxxxxxxxxxxxxx>
---
net/ipv4/inet_connection_sock.c | 13 ++-
net/smc/af_smc.c | 143 ++++++++++++++++++++++++++++++--
net/smc/smc.h | 2 +
3 files changed, 150 insertions(+), 8 deletions(-)
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 65ad4251f6fd..ba2ec5ad4c04 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -658,6 +658,7 @@ struct sock *inet_csk_accept(struct sock *sk, int flags, int *err, bool kern)
{
struct inet_connection_sock *icsk = inet_csk(sk);
struct request_sock_queue *queue = &icsk->icsk_accept_queue;
+ spinlock_t *fastopenq_lock = &queue->fastopenq.lock;
struct request_sock *req;
struct sock *newsk;
int error;
@@ -689,7 +690,15 @@ struct sock *inet_csk_accept(struct sock *sk, int flags, int *err, bool kern)
if (sk->sk_protocol == IPPROTO_TCP &&
tcp_rsk(req)->tfo_listener) {
- spin_lock_bh(&queue->fastopenq.lock);
+#if IS_ENABLED(CONFIG_SMC)
+ if (tcp_sk(sk)->syn_smc) {
+ struct request_sock_queue *orig_queue;
+
+ orig_queue = &inet_csk(req->rsk_listener)->icsk_accept_queue;
+ fastopenq_lock = &orig_queue->fastopenq.lock;
+ }
+#endif
+ spin_lock_bh(fastopenq_lock);
if (tcp_rsk(req)->tfo_listener) {
/* We are still waiting for the final ACK from 3WHS
* so can't free req now. Instead, we set req->sk to
@@ -700,7 +709,7 @@ struct sock *inet_csk_accept(struct sock *sk, int flags, int *err, bool kern)
req->sk = NULL;
req = NULL;
}
- spin_unlock_bh(&queue->fastopenq.lock);
+ spin_unlock_bh(fastopenq_lock);
}
out:
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index a4cccdfdc00a..ad6c3b9ec9a6 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -126,7 +126,9 @@ static struct sock *smc_tcp_syn_recv_sock(const struct sock *sk,
smc = smc_clcsock_user_data(sk);
- if (READ_ONCE(sk->sk_ack_backlog) + atomic_read(&smc->queued_smc_hs) >
+ if (READ_ONCE(sk->sk_ack_backlog) + atomic_read(&smc->queued_smc_hs)
+ + READ_ONCE(smc->actsock->sk->sk_ack_backlog)
+ + READ_ONCE(smc->fbsock->sk->sk_ack_backlog) >
sk->sk_max_ack_backlog)
goto drop;
@@ -286,6 +288,10 @@ static int __smc_release(struct smc_sock *smc)
/* wake up clcsock accept */
rc = kernel_sock_shutdown(smc->clcsock,
SHUT_RDWR);
+ if (smc->fbsock)
+ sock_release(smc->fbsock);
+ if (smc->actsock)
+ sock_release(smc->actsock);
}
sk->sk_state = SMC_CLOSED;
sk->sk_state_change(sk);
@@ -1681,7 +1687,7 @@ static int smc_clcsock_accept(struct smc_sock *lsmc, struct smc_sock **new_smc)
mutex_lock(&lsmc->clcsock_release_lock);
if (lsmc->clcsock)
- rc = kernel_accept(lsmc->clcsock, &new_clcsock, SOCK_NONBLOCK);
+ rc = kernel_accept(lsmc->actsock, &new_clcsock, SOCK_NONBLOCK);
mutex_unlock(&lsmc->clcsock_release_lock);
lock_sock(lsk);
if (rc < 0 && rc != -EAGAIN)
@@ -2486,9 +2492,46 @@ static void smc_tcp_listen_work(struct work_struct *work)
sock_put(&lsmc->sk); /* sock_hold in smc_clcsock_data_ready() */
}
+#define SMC_LINK 1
+#define FALLBACK_LINK 2
+static inline int smc_sock_pop_to_another_acceptq(struct smc_sock *lsmc)
+{
+ struct sock *lsk = lsmc->clcsock->sk;
+ struct inet_connection_sock *icsk = inet_csk(lsk);
+ struct inet_connection_sock *dest_icsk;
+ struct request_sock_queue *queue = &icsk->icsk_accept_queue;
+ struct request_sock_queue *dest_queue;
+ struct request_sock *req;
+ struct sock *dst_sock;
+ int ret;
+
+ req = reqsk_queue_remove(queue, lsk);
+ if (!req)
+ return -EINVAL;
+
+ if (tcp_sk(req->sk)->syn_smc || lsmc->sockopt_defer_accept) {
+ dst_sock = lsmc->actsock->sk;
+ ret = SMC_LINK;
+ } else {
+ dst_sock = lsmc->fbsock->sk;
+ ret = FALLBACK_LINK;
+ }
+
+ dest_icsk = inet_csk(dst_sock);
+ dest_queue = &dest_icsk->icsk_accept_queue;
+
+ spin_lock_bh(&dest_queue->rskq_lock);
+ WRITE_ONCE(req->dl_next, dest_queue->rskq_accept_head);
+ sk_acceptq_added(dst_sock);
+ dest_queue->rskq_accept_head = req;
+ spin_unlock_bh(&dest_queue->rskq_lock);
+ return ret;
+}
+
static void smc_clcsock_data_ready(struct sock *listen_clcsock)
{
struct smc_sock *lsmc;
+ int ret;
read_lock_bh(&listen_clcsock->sk_callback_lock);
lsmc = smc_clcsock_user_data(listen_clcsock);
@@ -2496,14 +2539,41 @@ static void smc_clcsock_data_ready(struct sock *listen_clcsock)
goto out;
lsmc->clcsk_data_ready(listen_clcsock);
if (lsmc->sk.sk_state == SMC_LISTEN) {
- sock_hold(&lsmc->sk); /* sock_put in smc_tcp_listen_work() */
- if (!queue_work(smc_tcp_ls_wq, &lsmc->tcp_listen_work)) > - sock_put(&lsmc->sk);
+ ret = smc_sock_pop_to_another_acceptq(lsmc);
+ if (ret == SMC_LINK) {
+ sock_hold(&lsmc->sk); /* sock_put in smc_tcp_listen_work() */
+ if (!queue_work(smc_tcp_ls_wq, &lsmc->tcp_listen_work))
+ sock_put(&lsmc->sk);
+ } else if (ret == FALLBACK_LINK) {
+ lsmc->sk.sk_data_ready(&lsmc->sk);
+ }
}
out:
read_unlock_bh(&listen_clcsock->sk_callback_lock);
}
+static void smc_shadow_socket_init(struct socket *sock)
+{
+ struct inet_connection_sock *icsk = inet_csk(sock->sk);
+ struct request_sock_queue *queue = &icsk->icsk_accept_queue;
+
+ tcp_set_state(sock->sk, TCP_LISTEN);
+ sock->sk->sk_ack_backlog = 0;
+
+ inet_csk_delack_init(sock->sk);
+
+ spin_lock_init(&queue->rskq_lock);
+
+ spin_lock_init(&queue->fastopenq.lock);
+ queue->fastopenq.rskq_rst_head = NULL;
+ queue->fastopenq.rskq_rst_tail = NULL;
+ queue->fastopenq.qlen = 0;
+
+ queue->rskq_accept_head = NULL;
+
+ tcp_sk(sock->sk)->syn_smc = 1;
+}
+
static int smc_listen(struct socket *sock, int backlog)
{
struct sock *sk = sock->sk;
@@ -2551,6 +2621,18 @@ static int smc_listen(struct socket *sock, int backlog)
if (smc->limit_smc_hs)
tcp_sk(smc->clcsock->sk)->smc_hs_congested = smc_hs_congested;
+ rc = sock_create_kern(sock_net(sk), PF_INET, SOCK_STREAM, IPPROTO_TCP,
+ &smc->fbsock);
+ if (rc)
+ goto out;
+ smc_shadow_socket_init(smc->fbsock);
+
+ rc = sock_create_kern(sock_net(sk), PF_INET, SOCK_STREAM, IPPROTO_TCP,
+ &smc->actsock);
+ if (rc)
+ goto out;
+ smc_shadow_socket_init(smc->actsock);
+
rc = kernel_listen(smc->clcsock, backlog);
if (rc) {
write_lock_bh(&smc->clcsock->sk->sk_callback_lock);
@@ -2569,6 +2651,30 @@ static int smc_listen(struct socket *sock, int backlog)
return rc;
}
+static inline bool tcp_reqsk_queue_empty(struct sock *sk)
+{
+ struct inet_connection_sock *icsk = inet_csk(sk);
+ struct request_sock_queue *queue = &icsk->icsk_accept_queue;
+
+ return reqsk_queue_empty(queue);
+}
+
Since this is only used by smc, I'd like to suggest to use
smc_tcp_reqsk_queue_empty instead of tcp_reqsk_queue_empty.
+static inline void
+smc_restore_fbsock_protocol_family(struct socket *new_sock, struct socket *sock)
+{
+ struct smc_sock *lsmc = smc_sk(sock->sk);
+
+ new_sock->sk->sk_data_ready = lsmc->fbsock->sk->sk_data_ready;
+ new_sock->ops = lsmc->fbsock->ops;
+ new_sock->type = lsmc->fbsock->type;
+
+ module_put(sock->ops->owner);
+ __module_get(new_sock->ops->owner);
+
+ if (tcp_sk(new_sock->sk)->syn_smc)
+ pr_err("new sock is not fallback.\n");
+}
+
static int smc_accept(struct socket *sock, struct socket *new_sock,
int flags, bool kern)
{
@@ -2579,6 +2685,18 @@ static int smc_accept(struct socket *sock, struct socket *new_sock,
int rc = 0;
lsmc = smc_sk(sk);
+ /* There is a lock in inet_csk_accept, so to make a fast path we do not lock_sock here */
+ if (lsmc->sk.sk_state == SMC_LISTEN && !tcp_reqsk_queue_empty(lsmc->fbsock->sk)) {
+ rc = lsmc->clcsock->ops->accept(lsmc->fbsock, new_sock, O_NONBLOCK, true);
+ if (rc == -EAGAIN)
+ goto normal_path;
+ if (rc < 0)
+ return rc;
+ smc_restore_fbsock_protocol_family(new_sock, sock);
+ return rc;
+ }
+
+normal_path:
sock_hold(sk); /* sock_put below */
lock_sock(sk);
@@ -2593,6 +2711,18 @@ static int smc_accept(struct socket *sock, struct socket *new_sock,
add_wait_queue_exclusive(sk_sleep(sk), &wait);
while (!(nsk = smc_accept_dequeue(sk, new_sock))) {
set_current_state(TASK_INTERRUPTIBLE);
+ if (!tcp_reqsk_queue_empty(lsmc->fbsock->sk)) {
+ rc = lsmc->clcsock->ops->accept(lsmc->fbsock, new_sock, O_NONBLOCK, true);
+ if (rc == -EAGAIN)
+ goto next_round;
+ if (rc < 0)
+ break;
+
+ smc_restore_fbsock_protocol_family(new_sock, sock);
+ nsk = new_sock->sk;
+ break;
+ }
+next_round:
if (!timeo) {
rc = -EAGAIN;
break;
@@ -2731,7 +2861,8 @@ static __poll_t smc_accept_poll(struct sock *parent)
__poll_t mask = 0;
spin_lock(&isk->accept_q_lock);
- if (!list_empty(&isk->accept_q))
+ if (!list_empty(&isk->accept_q) ||
+ !reqsk_queue_empty(&inet_csk(isk->fbsock->sk)->icsk_accept_queue))
mask = EPOLLIN | EPOLLRDNORM;
spin_unlock(&isk->accept_q_lock);
diff --git a/net/smc/smc.h b/net/smc/smc.h
index 5ed765ea0c73..9a62c8f37e26 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -241,6 +241,8 @@ struct smc_connection {
struct smc_sock { /* smc sock container */
struct sock sk;
struct socket *clcsock; /* internal tcp socket */
+ struct socket *fbsock; /* socket for fallback connection */
+ struct socket *actsock; /* socket for non-fallback conneciotn */
void (*clcsk_state_change)(struct sock *sk);
/* original stat_change fct. */
void (*clcsk_data_ready)(struct sock *sk);
