You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "zwoop (via GitHub)" <gi...@apache.org> on 2026/05/26 19:47:42 UTC
[PR] Reduce TLS write-path overhead [trafficserver]
zwoop opened a new pull request, #13202:
URL: https://github.com/apache/trafficserver/pull/13202
Insert a buffer BIO on the write side so multiple TLS records accumulate without syscalls, flushed once after the write loop. This reduces write() calls from one-per-record to one-per-flush, directly addressing the ~15% of CPU spent in write() syscalls. TCP_CORK/NOPUSH wraps the write loop as a secondary optimization for TCP segment batching. Consolidate SSL ex_data from 9 slots to 1 struct pointer, skip handshake buffer allocation when blind-tunnel is not configured, fix sslTotalBytesSent to update inside the write loop for correct dynamic record sizing ramp, batch metric counter increments to reduce cross-thread cache-line contention, and use the cached NetHandler timestamp instead of calling ink_get_hrtime per write event.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#issuecomment-4596838979
[approve ci rocky autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#issuecomment-4713998739
Cherry-picked to 10.2.x
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#issuecomment-4653976729
Addressed the review comments
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen commented on code in PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#discussion_r3367589631
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -737,18 +737,17 @@ SSLNetVConnection::load_buffer_and_write(int64_t towrite, MIOBufferAccessor &buf
Dbg(dbg_ctl_ssl, "towrite=%" PRId64, towrite);
+ // Per-thread scratch to coalesce fragmented blocks into one SSL_write. Reuse across
+ // connections is safe: a WANT_WRITE retry flushes SSL's own record buffer, not this.
Review Comment:
I think we should rephrase why the gather_buf is thread_local. Isn't it this way because we don't set `SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER`? Maybe something like:
```
// SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER is off, so SSL_write retries after WANT_WRITE/WANT_READ
// require the same buffer address. We ensure the same buffer address between retries by reusing a
// thread_local buffer.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#issuecomment-4628446801
Re-ran the tests again, and this time, for small objects, this patch was 25% more RPS. :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#discussion_r3360437009
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -737,18 +737,19 @@ SSLNetVConnection::load_buffer_and_write(int64_t towrite, MIOBufferAccessor &buf
Dbg(dbg_ctl_ssl, "towrite=%" PRId64, towrite);
+ // Per-thread scratch to coalesce fragmented blocks into one SSL_write. Reuse across
+ // connections is safe: a WANT_WRITE retry flushes SSL's own record buffer, not this.
+ static thread_local char gather_buf[SSL_MAX_TLS_RECORD_SIZE];
+
ERR_clear_error();
do {
- // What is remaining left in the next block?
- l = buf.reader()->block_read_avail();
- char *current_block = buf.reader()->start();
+ IOBufferReader *reader = buf.reader();
- // check if to amount to write exceeds that in this buffer
+ // Unlike the per-block original, l may span blocks so it can be coalesced below.
+ int64_t avail = reader->read_avail();
int64_t wavail = towrite - total_written;
- if (l > wavail) {
- l = wavail;
- }
+ l = (wavail < avail) ? wavail : avail;
Review Comment:
Good catch — fixed. `towrite` is already bounded by `read_avail()` in the caller (write_to_net_io), so the per-iteration `read_avail()` walk was redundant; now using `towrite - total_written` directly with no O(n) block-chain walk.
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -737,18 +737,19 @@ SSLNetVConnection::load_buffer_and_write(int64_t towrite, MIOBufferAccessor &buf
Dbg(dbg_ctl_ssl, "towrite=%" PRId64, towrite);
+ // Per-thread scratch to coalesce fragmented blocks into one SSL_write. Reuse across
+ // connections is safe: a WANT_WRITE retry flushes SSL's own record buffer, not this.
+ static thread_local char gather_buf[SSL_MAX_TLS_RECORD_SIZE];
Review Comment:
Updated the PR description to match the implementation — this is a userspace staging-buffer gather (coalesce plaintext, one SSL_write per chunk), not the buffer-BIO approach the original description described.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "c-taylor (via GitHub)" <gi...@apache.org>.
c-taylor commented on PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#issuecomment-4664459254
Should this be 10.2 tagged?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#discussion_r3343233840
##########
src/iocore/net/TLSBasicSupport.cc:
##########
@@ -52,6 +52,9 @@ TLSBasicSupport::initialize()
TLSBasicSupport *
TLSBasicSupport::getInstance(SSL *ssl)
{
+ if (auto *exd = SSLExDataGet(ssl)) {
+ return exd->basic;
+ }
return static_cast<TLSBasicSupport *>(SSL_get_ex_data(ssl, _ex_data_index));
Review Comment:
Why do we need this fallback?
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -111,47 +111,53 @@ SSLNetVConnection::_make_ssl_connection(SSL_CTX *ctx)
this->options.f_tcp_fastopen ? this->get_remote_addr() : static_cast<const sockaddr *>(nullptr));
SSL_set_bio(ssl, bio, bio);
- } else {
+ } else if (this->transparentPassThrough || this->allowPlain) {
+ // Need handshake buffers to capture CLIENT_HELLO for potential blind-tunnel replay
this->initialize_handshake_buffers();
BIO *rbio = BIO_new(BIO_s_mem());
BIO *wbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
BIO_set_mem_eof_return(wbio, -1);
SSL_set_bio(ssl, rbio, wbio);
+ } else {
+ // No blind-tunnel possible: use direct socket BIO, skip handshake buffers.
+ // Buffer BIO on write side coalesces TLS records into fewer write() syscalls.
+ BIO *rbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
+ BIO *sock_wbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
+ BIO *buf_wbio = BIO_new(BIO_f_buffer());
+ BIO_set_write_buffer_size(buf_wbio, 65536);
+ BIO *wbio = BIO_push(buf_wbio, sock_wbio);
+ SSL_set_bio(ssl, rbio, wbio);
+ }
#if TS_HAS_TLS_EARLY_DATA
+ if (this->get_context() != NET_VCONNECTION_OUT) {
update_early_data_config(ssl, SSLConfigParams::server_max_early_data, SSLConfigParams::server_recv_max_early_data);
-#endif
}
+#endif
this->_bindSSLObject();
}
}
void
SSLNetVConnection::_bindSSLObject()
{
- SSLNetVCAttach(this->ssl, this);
- TLSBasicSupport::bind(this->ssl, this);
+ this->_ssl_ex_data.vc = this;
+ this->_ssl_ex_data.basic = static_cast<TLSBasicSupport *>(this);
+ this->_ssl_ex_data.event = static_cast<TLSEventSupport *>(this);
+ this->_ssl_ex_data.alpn = static_cast<ALPNSupport *>(this);
+ this->_ssl_ex_data.resumption = static_cast<TLSSessionResumptionSupport *>(this);
+ this->_ssl_ex_data.sni = static_cast<TLSSNISupport *>(this);
+ this->_ssl_ex_data.early_data = static_cast<TLSEarlyDataSupport *>(this);
+ this->_ssl_ex_data.tunnel = static_cast<TLSTunnelSupport *>(this);
+ this->_ssl_ex_data.cert_switch = static_cast<TLSCertSwitchSupport *>(this);
+ SSLNetVCAttach(this->ssl, &this->_ssl_ex_data);
TLSEventSupport::bind(this->ssl, this);
Review Comment:
Why do we need to keep this one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#issuecomment-4627329727
@moonchen I redid the entire PR, because the filter BIO APIs aren't available in Boring. This simpler version is still a significant improvement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#issuecomment-4578827662
[approve ci autest 2]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#issuecomment-4607603401
@maskit I removed the SSL_get_ex_data stuff, it does reduce the number of reallocations, but it's pretty marginal impact on performance (maybe 1 microsecond / connection).
The performance on the BIO buffer is real though, I benchmarked it again, and still get a 15% performance improvement:
## ATS master
| Run | Req/sec | Bytes/sec | Reqs (200 OK) | Total | Avg Size | Errors | p50 (ms) | Duration |
|-----|---------|-----------|---------------|-------|----------|--------|----------|----------|
| 1 | 522769 | 164.5 MB/s | 23.547M | 23.547M | 315 B | 0.00% | 2.22 | 45.0s |
### Timing (ms)
| Metric | Latency | TTFB | Connect |
|--------|--------:|--------:|--------:|
| Min | 0.455 | 0.452 | 0.445 |
| μ (Mean) | 2.87 | 2.86 | 5.86 |
| σ (StdDev) | 1.41 | 1.41 | 7.11 |
| p50 | 2.22 | 2.21 | 4.72 |
| p95 | 5.65 | 5.64 | 8.83 |
| p99 | 7.96 | 7.96 | 43.86 |
| Max | 111.8 | 111.7 | 63.14 |
| Skew (γ₁) | 3.57 | 3.55 | 4.63 |
| Samples | 1.472M | 1.472M | 1.5K |
### Latency Distribution
```
p50 │█ 2.22
p95 │██ 5.65
p99 │██ 7.96
min=0.455 μ=2.87 max=111.8
```
## With BIO buffering
| Run | Req/sec | Bytes/sec | Reqs (200 OK) | Total | Avg Size | Errors | p50 (ms) | Duration |
|-----|---------|-----------|---------------|-------|----------|--------|----------|----------|
| 1 | 602903 | 189.9 MB/s | 27.161M | 27.161M | 315 B | 0.00% | 1.92 | 45.1s |
### Timing (ms)
| Metric | Latency | TTFB | Connect |
|--------|--------:|--------:|--------:|
| Min | 0.341 | 0.336 | 0.323 |
| μ (Mean) | 2.48 | 2.48 | 4.96 |
| σ (StdDev) | 1.81 | 1.81 | 4.03 |
| p50 | 1.92 | 1.91 | 4.61 |
| p95 | 4.75 | 4.75 | 8.95 |
| p99 | 6.46 | 6.46 | 10.78 |
| Max | 159.0 | 159.0 | 43.59 |
| Skew (γ₁) | 28.18 | 28.18 | 5.40 |
| Samples | 1.698M | 1.698M | 1.5K |
### Latency Distribution
```
p50 │█ 1.92
p95 │█ 4.75
p99 │█ 6.46
min=0.341 μ=2.48 max=159.0
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#discussion_r3364843294
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -737,18 +737,17 @@ SSLNetVConnection::load_buffer_and_write(int64_t towrite, MIOBufferAccessor &buf
Dbg(dbg_ctl_ssl, "towrite=%" PRId64, towrite);
+ // Per-thread scratch to coalesce fragmented blocks into one SSL_write. Reuse across
+ // connections is safe: a WANT_WRITE retry flushes SSL's own record buffer, not this.
+ static thread_local char gather_buf[SSL_MAX_TLS_RECORD_SIZE];
+
ERR_clear_error();
do {
- // What is remaining left in the next block?
- l = buf.reader()->block_read_avail();
- char *current_block = buf.reader()->start();
-
- // check if to amount to write exceeds that in this buffer
- int64_t wavail = towrite - total_written;
+ IOBufferReader *reader = buf.reader();
- if (l > wavail) {
- l = wavail;
- }
+ // towrite is already bounded by read_avail() in the caller, so this never exceeds
Review Comment:
Could you (debug) assert this at the top of the function to enforce the precondition?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#discussion_r3359805874
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -737,18 +737,19 @@ SSLNetVConnection::load_buffer_and_write(int64_t towrite, MIOBufferAccessor &buf
Dbg(dbg_ctl_ssl, "towrite=%" PRId64, towrite);
+ // Per-thread scratch to coalesce fragmented blocks into one SSL_write. Reuse across
+ // connections is safe: a WANT_WRITE retry flushes SSL's own record buffer, not this.
+ static thread_local char gather_buf[SSL_MAX_TLS_RECORD_SIZE];
+
ERR_clear_error();
do {
- // What is remaining left in the next block?
- l = buf.reader()->block_read_avail();
- char *current_block = buf.reader()->start();
+ IOBufferReader *reader = buf.reader();
- // check if to amount to write exceeds that in this buffer
+ // Unlike the per-block original, l may span blocks so it can be coalesced below.
+ int64_t avail = reader->read_avail();
int64_t wavail = towrite - total_written;
- if (l > wavail) {
- l = wavail;
- }
+ l = (wavail < avail) ? wavail : avail;
Review Comment:
IOBufferReader::read_avail() walks the entire IOBufferBlock chain (O(n) in block count). This function is on the TLS write hot-path, and switching from per-block block_read_avail() to read_avail() can add significant overhead when buffers are highly fragmented (ironically the exact case this change targets). Consider using IOBufferReader::is_read_avail_more_than() to early-exit once the desired threshold is met (e.g., up to one TLS record) and only fall back to read_avail() when you actually need the exact total.
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -737,18 +737,19 @@ SSLNetVConnection::load_buffer_and_write(int64_t towrite, MIOBufferAccessor &buf
Dbg(dbg_ctl_ssl, "towrite=%" PRId64, towrite);
+ // Per-thread scratch to coalesce fragmented blocks into one SSL_write. Reuse across
+ // connections is safe: a WANT_WRITE retry flushes SSL's own record buffer, not this.
+ static thread_local char gather_buf[SSL_MAX_TLS_RECORD_SIZE];
Review Comment:
The PR description says the optimization is to insert a buffered BIO on the write side so multiple TLS records accumulate without syscalls and are flushed once after the write loop. This change instead coalesces plaintext across IOBuffer blocks before calling SSL_write(), and it does not change the per-SSL_write BIO_flush() behavior in _ssl_write_buffer(). Please either update the PR description to match the actual approach, or extend the implementation to add the described buffered-BIO + single flush semantics.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#issuecomment-4637769063
[approve ci]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#issuecomment-4633044088
[approve ci]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#discussion_r3352305713
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -123,11 +123,26 @@ SSLNetVConnection::_make_ssl_connection(SSL_CTX *ctx)
SSL_set_bio(ssl, bio, bio);
} else {
- this->initialize_handshake_buffers();
- BIO *rbio = BIO_new(BIO_s_mem());
- BIO *wbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
- BIO_set_mem_eof_return(wbio, -1);
- SSL_set_bio(ssl, rbio, wbio);
+ if (this->transparentPassThrough || this->allowPlain) {
+ // Blind-tunnel possible: keep handshake buffers to capture/replay the CLIENT_HELLO.
+ this->initialize_handshake_buffers();
+ BIO *rbio = BIO_new(BIO_s_mem());
+ BIO *wbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
+ BIO_set_mem_eof_return(wbio, -1);
+ SSL_set_bio(ssl, rbio, wbio);
+ } else {
+ // No blind-tunnel: direct socket BIO, skip handshake buffers. On OpenSSL a
+ // write-side buffer BIO coalesces TLS records into fewer write() syscalls;
+ // BoringSSL has no filter BIOs, so it uses the plain socket BIO.
+ BIO *rbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
+ BIO *wbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
+#if !defined(OPENSSL_IS_BORINGSSL)
Review Comment:
Not a blocker but I'd gate it by the availability of `BIO_f_buffer`. This would cause a build error if ATS is built with AWS-LC.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#issuecomment-4662364362
[approve ci autest 1]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen commented on PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#issuecomment-4639387499
One comment suggestion, but otherwise LGTM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop merged PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#discussion_r3376588670
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -737,18 +737,17 @@ SSLNetVConnection::load_buffer_and_write(int64_t towrite, MIOBufferAccessor &buf
Dbg(dbg_ctl_ssl, "towrite=%" PRId64, towrite);
+ // Per-thread scratch to coalesce fragmented blocks into one SSL_write. Reuse across
+ // connections is safe: a WANT_WRITE retry flushes SSL's own record buffer, not this.
Review Comment:
Done — rephrased the comment as suggested.
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -737,18 +737,17 @@ SSLNetVConnection::load_buffer_and_write(int64_t towrite, MIOBufferAccessor &buf
Dbg(dbg_ctl_ssl, "towrite=%" PRId64, towrite);
+ // Per-thread scratch to coalesce fragmented blocks into one SSL_write. Reuse across
+ // connections is safe: a WANT_WRITE retry flushes SSL's own record buffer, not this.
+ static thread_local char gather_buf[SSL_MAX_TLS_RECORD_SIZE];
+
ERR_clear_error();
do {
- // What is remaining left in the next block?
- l = buf.reader()->block_read_avail();
- char *current_block = buf.reader()->start();
-
- // check if to amount to write exceeds that in this buffer
- int64_t wavail = towrite - total_written;
+ IOBufferReader *reader = buf.reader();
- if (l > wavail) {
- l = wavail;
- }
+ // towrite is already bounded by read_avail() in the caller, so this never exceeds
Review Comment:
Done — added `ink_assert(towrite <= buf.reader()->read_avail())` at the top of the function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#issuecomment-4596836872
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce TLS write-path overhead [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#discussion_r3352278545
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -123,11 +123,26 @@ SSLNetVConnection::_make_ssl_connection(SSL_CTX *ctx)
SSL_set_bio(ssl, bio, bio);
} else {
- this->initialize_handshake_buffers();
- BIO *rbio = BIO_new(BIO_s_mem());
- BIO *wbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
- BIO_set_mem_eof_return(wbio, -1);
- SSL_set_bio(ssl, rbio, wbio);
+ if (this->transparentPassThrough || this->allowPlain) {
+ // Blind-tunnel possible: keep handshake buffers to capture/replay the CLIENT_HELLO.
+ this->initialize_handshake_buffers();
+ BIO *rbio = BIO_new(BIO_s_mem());
+ BIO *wbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
+ BIO_set_mem_eof_return(wbio, -1);
+ SSL_set_bio(ssl, rbio, wbio);
+ } else {
+ // No blind-tunnel: direct socket BIO, skip handshake buffers. On OpenSSL a
+ // write-side buffer BIO coalesces TLS records into fewer write() syscalls;
+ // BoringSSL has no filter BIOs, so it uses the plain socket BIO.
+ BIO *rbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
+ BIO *wbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
+#if !defined(OPENSSL_IS_BORINGSSL)
+ BIO *buf_wbio = BIO_new(BIO_f_buffer());
+ BIO_set_write_buffer_size(buf_wbio, 65536);
+ wbio = BIO_push(buf_wbio, wbio);
+#endif
Review Comment:
BIO_new(BIO_f_buffer()) / BIO_push() results are not checked for failure. If allocation or push fails, buf_wbio may be nullptr (crash on BIO_set_write_buffer_size) or BIO_push may return nullptr, leaving wbio invalid.
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -123,11 +123,26 @@ SSLNetVConnection::_make_ssl_connection(SSL_CTX *ctx)
SSL_set_bio(ssl, bio, bio);
} else {
- this->initialize_handshake_buffers();
- BIO *rbio = BIO_new(BIO_s_mem());
- BIO *wbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
- BIO_set_mem_eof_return(wbio, -1);
- SSL_set_bio(ssl, rbio, wbio);
+ if (this->transparentPassThrough || this->allowPlain) {
+ // Blind-tunnel possible: keep handshake buffers to capture/replay the CLIENT_HELLO.
+ this->initialize_handshake_buffers();
+ BIO *rbio = BIO_new(BIO_s_mem());
+ BIO *wbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
+ BIO_set_mem_eof_return(wbio, -1);
+ SSL_set_bio(ssl, rbio, wbio);
+ } else {
+ // No blind-tunnel: direct socket BIO, skip handshake buffers. On OpenSSL a
+ // write-side buffer BIO coalesces TLS records into fewer write() syscalls;
+ // BoringSSL has no filter BIOs, so it uses the plain socket BIO.
+ BIO *rbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
+ BIO *wbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
+#if !defined(OPENSSL_IS_BORINGSSL)
+ BIO *buf_wbio = BIO_new(BIO_f_buffer());
+ BIO_set_write_buffer_size(buf_wbio, 65536);
+ wbio = BIO_push(buf_wbio, wbio);
+#endif
+ SSL_set_bio(ssl, rbio, wbio);
+ }
Review Comment:
A buffered filter BIO is pushed onto the SSL wbio before the TLS handshake runs, but the handshake path (_ssl_accept / _sslStartHandShake) never calls BIO_flush. Because BIO_f_buffer can retain handshake records until flush / buffer-full, SSL_accept() may return WANT_READ while the ServerHello/etc are still stuck in the buffer, potentially stalling the handshake.
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -123,11 +123,26 @@ SSLNetVConnection::_make_ssl_connection(SSL_CTX *ctx)
SSL_set_bio(ssl, bio, bio);
} else {
- this->initialize_handshake_buffers();
- BIO *rbio = BIO_new(BIO_s_mem());
- BIO *wbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
- BIO_set_mem_eof_return(wbio, -1);
- SSL_set_bio(ssl, rbio, wbio);
+ if (this->transparentPassThrough || this->allowPlain) {
+ // Blind-tunnel possible: keep handshake buffers to capture/replay the CLIENT_HELLO.
+ this->initialize_handshake_buffers();
Review Comment:
This change skips handshake buffers unless transparent passthrough / allow-plain is enabled, but SSLNetVConnection::read_raw_data() (used during CLIENT_HELLO buffering) is also where PROXY protocol is parsed and the allowlist is enforced. With handshake buffers disabled, the PROXY header will be consumed by OpenSSL as TLS bytes and proxy protocol will no longer work on normal TLS ports.
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -796,6 +811,14 @@ SSLNetVConnection::load_buffer_and_write(int64_t towrite, MIOBufferAccessor &buf
Metrics::Counter::increment(net_rsb.calls_to_write);
} while (num_really_written == try_to_write && total_written < towrite);
+ // Flush the buffer BIO once here, not per-record, so a response coalesces into
+ // one write(). No-op on a plain socket BIO.
+ if (total_written > 0) {
+ if (BIO *wbio = SSL_get_wbio(ssl); wbio != nullptr) {
+ (void)BIO_flush(wbio);
+ }
+ }
Review Comment:
BIO_flush() return value is ignored. With a write-side buffer BIO, flush can fail / retry (e.g. EAGAIN) leaving encrypted bytes pending in the BIO. If the application buffer becomes empty, UnixNetVConnection::write_to_net() will disable further write events, so the pending BIO output may never be flushed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org