From eec4ef4ecd2e7443d21093bafbf19aa807228035 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Mon, 26 Feb 2018 14:00:21 -0800 Subject: [PATCH 1/4] perf: coalesce buffer instead of calling SSL_write() for each slice. This results in significant performance gains, because it avoids: 1) emitting a lot of tiny TLS records, which results in less overhead, 2) calling write() for each slice. Fixes #2667. Signed-off-by: Piotr Sikora --- source/common/ssl/ssl_socket.cc | 73 +++++++++++++++------------------ source/common/ssl/ssl_socket.h | 1 + 2 files changed, 33 insertions(+), 41 deletions(-) diff --git a/source/common/ssl/ssl_socket.cc b/source/common/ssl/ssl_socket.cc index 63de5d4720b1..98890612aff9 100644 --- a/source/common/ssl/ssl_socket.cc +++ b/source/common/ssl/ssl_socket.cc @@ -153,57 +153,48 @@ Network::IoResult SslSocket::doWrite(Buffer::Instance& write_buffer, bool end_st } } - uint64_t original_buffer_length = write_buffer.length(); + uint64_t bytes_to_write; + if (bytes_to_retry_) { + bytes_to_write = bytes_to_retry_; + bytes_to_retry_ = 0; + } else { + bytes_to_write = std::min(write_buffer.length(), static_cast(16384)); + } + uint64_t total_bytes_written = 0; - bool keep_writing = true; - while ((original_buffer_length != total_bytes_written) && keep_writing) { - // Protect against stack overflow if the buffer has a very large buffer chain. - // TODO(mattklein123): See the comment on getRawSlices() for why we have to also check - // original_buffer_length != total_bytes_written during loop iteration. + while (bytes_to_write) { // TODO(mattklein123): As it relates to our fairness efforts, we might want to limit the number // of iterations of this loop, either by pure iterations, bytes written, etc. - const uint64_t MAX_SLICES = 32; - Buffer::RawSlice slices[MAX_SLICES]; - uint64_t num_slices = write_buffer.getRawSlices(slices, MAX_SLICES); - - uint64_t inner_bytes_written = 0; - for (uint64_t i = 0; (i < num_slices) && (original_buffer_length != total_bytes_written); i++) { - // SSL_write() requires that if a previous call returns SSL_ERROR_WANT_WRITE, we need to call - // it again with the same parameters. Most implementations keep track of the last write size. - // In our case we don't need to do that because: a) SSL_write() will not write partial - // buffers. b) We only move() into the write buffer, which means that it's impossible for a - // particular chain to increase in size. So as long as we start writing where we left off we - // are guaranteed to call SSL_write() with the same parameters. - int rc = SSL_write(ssl_.get(), slices[i].mem_, slices[i].len_); - ENVOY_CONN_LOG(trace, "ssl write returns: {}", callbacks_->connection(), rc); - if (rc > 0) { - inner_bytes_written += rc; - total_bytes_written += rc; - } else { - int err = SSL_get_error(ssl_.get(), rc); - switch (err) { - case SSL_ERROR_WANT_WRITE: - keep_writing = false; - break; - case SSL_ERROR_WANT_READ: - // Renegotiation has started. We don't handle renegotiation so just fall through. - default: - drainErrorQueue(); - return {PostIoAction::Close, total_bytes_written, false}; - } + // SSL_write() requires that if a previous call returns SSL_ERROR_WANT_WRITE, we need to call + // it again with the same parameters. This is done by tracking last write size, but not write + // data, since linearize() will return the same undrained data anyway. + ASSERT(bytes_to_write <= write_buffer.length()); + int rc = SSL_write(ssl_.get(), write_buffer.linearize(bytes_to_write), bytes_to_write); + ENVOY_CONN_LOG(trace, "ssl write returns: {}", callbacks_->connection(), rc); + if (rc > 0) { + ASSERT(rc == static_cast(bytes_to_write)); + total_bytes_written += rc; + write_buffer.drain(rc); + bytes_to_write = std::min(write_buffer.length(), static_cast(16384)); + } else { + int err = SSL_get_error(ssl_.get(), rc); + switch (err) { + case SSL_ERROR_WANT_WRITE: + bytes_to_retry_ = bytes_to_write; break; + case SSL_ERROR_WANT_READ: + // Renegotiation has started. We don't handle renegotiation so just fall through. + default: + drainErrorQueue(); + return {PostIoAction::Close, total_bytes_written, false}; } - } - // Draining must be done within the inner loop, otherwise we will keep getting the same slices - // at the beginning of the buffer. - if (inner_bytes_written > 0) { - write_buffer.drain(inner_bytes_written); + break; } } - if (total_bytes_written == original_buffer_length && end_stream) { + if (write_buffer.length() == 0 && end_stream) { shutdownSsl(); } diff --git a/source/common/ssl/ssl_socket.h b/source/common/ssl/ssl_socket.h index 28091f2fbc90..54f2a4f6ba2a 100644 --- a/source/common/ssl/ssl_socket.h +++ b/source/common/ssl/ssl_socket.h @@ -55,6 +55,7 @@ class SslSocket : public Network::TransportSocket, bssl::UniquePtr ssl_; bool handshake_complete_{}; bool shutdown_sent_{}; + uint64_t bytes_to_retry_{}; mutable std::string cached_sha_256_peer_certificate_digest_; mutable std::string cached_url_encoded_pem_encoded_peer_certificate_; }; From 5671737b44f59f0311ae67330a580359052ef36e Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Tue, 27 Feb 2018 20:12:37 -0800 Subject: [PATCH 2/4] review: fix comment about SslSocket::doWrite(). Signed-off-by: Piotr Sikora --- source/common/network/connection_impl.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 004f3f2c2b1d..45cffe459695 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -324,9 +324,9 @@ void ConnectionImpl::write(Buffer::Instance& data, bool end_stream) { // TODO(mattklein123): All data currently gets moved from the source buffer to the write buffer. // This can lead to inefficient behavior if writing a bunch of small chunks. In this case, it // would likely be more efficient to copy data below a certain size. VERY IMPORTANT: If this is - // ever changed, read the comment in Ssl::ConnectionImpl::doWriteToSocket() VERY carefully. - // That code assumes that we never change existing write_buffer_ chain elements between calls - // to SSL_write(). That code will have to change if we ever copy here. + // ever changed, read the comment in SslSocket::doWrite() VERY carefully. That code assumes that + // we never change existing write_buffer_ chain elements between calls to SSL_write(). That code + // might need to change if we ever copy here. write_buffer_->move(data); // Activating a write event before the socket is connected has the side-effect of tricking From dc36746985243f46a72e9fcd95c303c86df3f271 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Thu, 1 Mar 2018 03:44:18 -0800 Subject: [PATCH 3/4] review: remove unnecessary SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER. Signed-off-by: Piotr Sikora --- source/common/ssl/ssl_socket.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/source/common/ssl/ssl_socket.cc b/source/common/ssl/ssl_socket.cc index 98890612aff9..a9ce7f1e5a40 100644 --- a/source/common/ssl/ssl_socket.cc +++ b/source/common/ssl/ssl_socket.cc @@ -16,7 +16,6 @@ namespace Ssl { SslSocket::SslSocket(Context& ctx, InitialState state) : ctx_(dynamic_cast(ctx)), ssl_(ctx_.newSsl()) { - SSL_set_mode(ssl_.get(), SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER); if (state == InitialState::Client) { SSL_set_connect_state(ssl_.get()); } else { From fdea6a2ca54f0678cb365d512302a2a7d5d87569 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Thu, 1 Mar 2018 14:29:52 -0800 Subject: [PATCH 4/4] review: style. Signed-off-by: Piotr Sikora --- source/common/ssl/ssl_socket.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/ssl/ssl_socket.cc b/source/common/ssl/ssl_socket.cc index a9ce7f1e5a40..ba30469c5163 100644 --- a/source/common/ssl/ssl_socket.cc +++ b/source/common/ssl/ssl_socket.cc @@ -161,7 +161,7 @@ Network::IoResult SslSocket::doWrite(Buffer::Instance& write_buffer, bool end_st } uint64_t total_bytes_written = 0; - while (bytes_to_write) { + while (bytes_to_write > 0) { // TODO(mattklein123): As it relates to our fairness efforts, we might want to limit the number // of iterations of this loop, either by pure iterations, bytes written, etc.