Skip to content

Commit

Permalink
Improvements for TLS with I/O threads (#1271)
Browse files Browse the repository at this point in the history
Main thread profiling revealed significant overhead in TLS operations,
even with read/write offloaded to I/O threads:

Perf results:

**10.82%** 8.82% `valkey-server libssl.so.3 [.] SSL_pending` # Called by
main thread after I/O completion

**10.16%** 5.06% `valkey-server libcrypto.so.3 [.] ERR_clear_error` #
Called for every event regardless of thread handling

This commit further optimizes TLS operations by moving more work from
the main thread to I/O threads:

Improve TLS offloading to I/O threads with two main changes:

1. Move `ERR_clear_error()` calls closer to SSL operations
   - Currently, error queue is cleared for every TLS event
   - Now only clear before actual SSL function calls
   - This prevents unnecessary clearing in main thread when operations
     are handled by I/O threads

2. Optimize `SSL_pending()` checks
   - Add `TLS_CONN_FLAG_HAS_PENDING` flag to track pending data
   - Move pending check to follow read operations immediately
   - I/O thread sets flag when pending data exists
   - Main thread uses flag to update pending list

Performance improvements:
Testing setup based on
https://valkey.io/blog/unlock-one-million-rps-part2/

Before:
- SET: 896,047 ops/sec
- GET: 875,794 ops/sec

After:
- SET: 985,784 ops/sec (+10% improvement)
- GET: 1,066,171 ops/sec (+22% improvement)

Signed-off-by: Uri Yagelnik <[email protected]>
  • Loading branch information
uriyage authored Nov 18, 2024
1 parent aa2dd3e commit 94113fd
Showing 1 changed file with 17 additions and 8 deletions.
25 changes: 17 additions & 8 deletions src/tls.c
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,7 @@ typedef enum {
#define TLS_CONN_FLAG_WRITE_WANT_READ (1 << 1)
#define TLS_CONN_FLAG_FD_SET (1 << 2)
#define TLS_CONN_FLAG_POSTPONE_UPDATE_STATE (1 << 3)
#define TLS_CONN_FLAG_HAS_PENDING (1 << 4)

typedef struct tls_connection {
connection c;
Expand Down Expand Up @@ -614,7 +615,7 @@ static void updatePendingData(tls_connection *conn) {

/* If SSL has pending data, already read from the socket, we're at risk of not calling the read handler again, make
* sure to add it to a list of pending connection that should be handled anyway. */
if (SSL_pending(conn->ssl) > 0) {
if (conn->flags & TLS_CONN_FLAG_HAS_PENDING) {
if (!conn->pending_list_node) {
listAddNodeTail(pending_list, conn);
conn->pending_list_node = listLast(pending_list);
Expand All @@ -625,6 +626,14 @@ static void updatePendingData(tls_connection *conn) {
}
}

void updateSSLPendingFlag(tls_connection *conn) {
if (SSL_pending(conn->ssl) > 0) {
conn->flags |= TLS_CONN_FLAG_HAS_PENDING;
} else {
conn->flags &= ~TLS_CONN_FLAG_HAS_PENDING;
}
}

static void updateSSLEvent(tls_connection *conn) {
if (conn->flags & TLS_CONN_FLAG_POSTPONE_UPDATE_STATE) return;

Expand Down Expand Up @@ -653,15 +662,14 @@ static void tlsHandleEvent(tls_connection *conn, int mask) {
TLSCONN_DEBUG("tlsEventHandler(): fd=%d, state=%d, mask=%d, r=%d, w=%d, flags=%d", fd, conn->c.state, mask,
conn->c.read_handler != NULL, conn->c.write_handler != NULL, conn->flags);

ERR_clear_error();

switch (conn->c.state) {
case CONN_STATE_CONNECTING:
conn_error = anetGetError(conn->c.fd);
if (conn_error) {
conn->c.last_errno = conn_error;
conn->c.state = CONN_STATE_ERROR;
} else {
ERR_clear_error();
if (!(conn->flags & TLS_CONN_FLAG_FD_SET)) {
SSL_set_fd(conn->ssl, conn->c.fd);
conn->flags |= TLS_CONN_FLAG_FD_SET;
Expand Down Expand Up @@ -690,6 +698,7 @@ static void tlsHandleEvent(tls_connection *conn, int mask) {
conn->c.conn_handler = NULL;
break;
case CONN_STATE_ACCEPTING:
ERR_clear_error();
ret = SSL_accept(conn->ssl);
if (ret <= 0) {
WantIOType want = 0;
Expand Down Expand Up @@ -747,10 +756,7 @@ static void tlsHandleEvent(tls_connection *conn, int mask) {
conn->flags &= ~TLS_CONN_FLAG_READ_WANT_WRITE;
if (!callHandler((connection *)conn, conn->c.read_handler)) return;
}

if (mask & AE_READABLE) {
updatePendingData(conn);
}
updatePendingData(conn);

break;
}
Expand Down Expand Up @@ -941,6 +947,7 @@ static int connTLSRead(connection *conn_, void *buf, size_t buf_len) {
if (conn->c.state != CONN_STATE_CONNECTED) return -1;
ERR_clear_error();
ret = SSL_read(conn->ssl, buf, buf_len);
updateSSLPendingFlag(conn);
return updateStateAfterSSLIO(conn, ret, 1);
}

Expand Down Expand Up @@ -992,7 +999,7 @@ static int connTLSBlockingConnect(connection *conn_, const char *addr, int port,
* which means the specified timeout will not be enforced accurately. */
SSL_set_fd(conn->ssl, conn->c.fd);
setBlockingTimeout(conn, timeout);

ERR_clear_error();
if ((ret = SSL_connect(conn->ssl)) <= 0) {
conn->c.state = CONN_STATE_ERROR;
return C_ERR;
Expand Down Expand Up @@ -1023,6 +1030,7 @@ static ssize_t connTLSSyncRead(connection *conn_, char *ptr, ssize_t size, long
setBlockingTimeout(conn, timeout);
ERR_clear_error();
int ret = SSL_read(conn->ssl, ptr, size);
updateSSLPendingFlag(conn);
ret = updateStateAfterSSLIO(conn, ret, 0);
unsetBlockingTimeout(conn);

Expand All @@ -1041,6 +1049,7 @@ static ssize_t connTLSSyncReadLine(connection *conn_, char *ptr, ssize_t size, l

ERR_clear_error();
int ret = SSL_read(conn->ssl, &c, 1);
updateSSLPendingFlag(conn);
ret = updateStateAfterSSLIO(conn, ret, 0);
if (ret <= 0) {
nread = -1;
Expand Down

0 comments on commit 94113fd

Please sign in to comment.