> On Oct 17, 2014, at 7:59 AM, sudhe...@apache.org wrote:
>
> Repository: trafficserver
> Updated Branches:
> refs/heads/master 609443dbe -> dee36e716
>
>
> [TS-2503]: Dynamic TLS Record Sizing for better page load latencies.
> The strategy used is essentially to use small TLS records that fit into a
> single TCP segment for the first ~1 MB of data, increase record size to 16 KB
> after that to optimize throughput, and then reset record size back to a single
> segment after ~1 second of inactivity—lather, rinse, repeat.
>
>
> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
> Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/dee36e71
> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/dee36e71
> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/dee36e71
>
> Branch: refs/heads/master
> Commit: dee36e7163373c1d61bc6b97e529269dcde134c5
> Parents: 609443d
> Author: Sudheer Vinukonda <sudhe...@yahoo-inc.com>
> Authored: Fri Oct 17 14:57:15 2014 +0000
> Committer: Sudheer Vinukonda <sudhe...@yahoo-inc.com>
> Committed: Fri Oct 17 14:57:15 2014 +0000
>
> ----------------------------------------------------------------------
> .../configuration/records.config.en.rst | 7 ++++
> iocore/net/P_SSLNetVConnection.h | 13 ++++++
> iocore/net/P_SSLUtils.h | 2 +
> iocore/net/SSLNetVConnection.cc | 43 +++++++++++++++++++-
> iocore/net/SSLUtils.cc | 4 ++
> 5 files changed, 68 insertions(+), 1 deletion(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/dee36e71/doc/reference/configuration/records.config.en.rst
> ----------------------------------------------------------------------
> diff --git a/doc/reference/configuration/records.config.en.rst
> b/doc/reference/configuration/records.config.en.rst
> index 07244b7..ca30055 100644
> --- a/doc/reference/configuration/records.config.en.rst
> +++ b/doc/reference/configuration/records.config.en.rst
> @@ -2168,6 +2168,13 @@ SSL Termination
> buffering at the SSL layer. The default of ``0`` means to always
> write all available data into a single SSL record.
>
> + A value of ``-1`` means TLS record size is dynamically determined. The
> + strategy employed is to use small TLS records that fit into a single
> + TCP segment for the first ~1 MB of data, but, increase the record size to
> + 16 KB after that to optimize throughput. The record size is reset back to
> + a single segment after ~1 second of inactivity and the record size ramping
> + mechanism is repeated again.
> +
> .. ts:cv:: CONFIG proxy.config.ssl.session_cache INT 2
>
> Enables the SSL Session Cache:
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/dee36e71/iocore/net/P_SSLNetVConnection.h
> ----------------------------------------------------------------------
> diff --git a/iocore/net/P_SSLNetVConnection.h
> b/iocore/net/P_SSLNetVConnection.h
> index 51b7391..4a6d7c8 100644
> --- a/iocore/net/P_SSLNetVConnection.h
> +++ b/iocore/net/P_SSLNetVConnection.h
> @@ -52,6 +52,17 @@
> #define SSL_TLSEXT_ERR_NOACK 3
> #endif
>
> +// TS-2503: dynamic TLS record sizing
> +// For smaller records, we should also reserve space for various TCP options
> +// (timestamps, SACKs.. up to 40 bytes [1]), and account for TLS record
> overhead
> +// (another 20-60 bytes on average, depending on the negotiated ciphersuite
> [2]).
> +// All in all: 1500 - 40 (IP) - 20 (TCP) - 40 (TCP options) - TLS overhead
> (60-100)
> +// For larger records, the size is determined by TLS protocol record size
> +#define SSL_DEF_TLS_RECORD_SIZE 1300 // 1500 - 40 (IP) - 20
> (TCP) - 40 (TCP options) - TLS overhead (60-100)
> +#define SSL_MAX_TLS_RECORD_SIZE 16383 // 2^14 - 1
> +#define SSL_DEF_TLS_RECORD_BYTE_THRESHOLD 1000000
> +#define SSL_DEF_TLS_RECORD_MSEC_THRESHOLD 1000
> +
> class SSLNextProtocolSet;
> struct SSLCertLookup;
>
> @@ -105,6 +116,8 @@ public:
>
> SSL *ssl;
> ink_hrtime sslHandshakeBeginTime;
> + ink_hrtime sslLastWriteTime;
> + int64_t sslTotalBytesSent;
>
> static int advertise_next_protocol(SSL * ssl, const unsigned char ** out,
> unsigned * outlen, void *);
> static int select_next_protocol(SSL * ssl, const unsigned char ** out,
> unsigned char * outlen, const unsigned char * in, unsigned inlen, void *);
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/dee36e71/iocore/net/P_SSLUtils.h
> ----------------------------------------------------------------------
> diff --git a/iocore/net/P_SSLUtils.h b/iocore/net/P_SSLUtils.h
> index ec7b0b9..3b77e7d 100644
> --- a/iocore/net/P_SSLUtils.h
> +++ b/iocore/net/P_SSLUtils.h
> @@ -70,6 +70,8 @@ enum SSL_Stats
> ssl_total_tickets_verified_stat,
> ssl_total_tickets_not_found_stat,
> ssl_total_tickets_renewed_stat,
> + ssl_total_dyn_def_tls_record_count,
> + ssl_total_dyn_max_tls_record_count,
> ssl_session_cache_hit,
> ssl_session_cache_miss,
> ssl_session_cache_eviction,
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/dee36e71/iocore/net/SSLNetVConnection.cc
> ----------------------------------------------------------------------
> diff --git a/iocore/net/SSLNetVConnection.cc b/iocore/net/SSLNetVConnection.cc
> index ba6f435..01d9eae 100644
> --- a/iocore/net/SSLNetVConnection.cc
> +++ b/iocore/net/SSLNetVConnection.cc
> @@ -617,12 +617,26 @@ SSLNetVConnection::load_buffer_and_write(int64_t
> towrite, int64_t &wattempted, i
> ProxyMutex *mutex = this_ethread()->mutex;
> int64_t r = 0;
> int64_t l = 0;
> + uint32_t dynamic_tls_record_size = 0;
> ssl_error_t err = SSL_ERROR_NONE;
>
> // XXX Rather than dealing with the block directly, we should use the
> IOBufferReader API.
> int64_t offset = buf.reader()->start_offset;
> IOBufferBlock *b = buf.reader()->block;
>
> + // Dynamic TLS record sizing
> + ink_hrtime now = 0;
> + if (SSLConfigParams::ssl_maxrecord == -1) {
> + now = ink_get_hrtime_internal();
> + int msec_since_last_write = ink_hrtime_to_msec(now - sslLastWriteTime);
ink_hrtime_diff_msec
> +
> + if (msec_since_last_write > SSL_DEF_TLS_RECORD_MSEC_THRESHOLD) {
> + // reset sslTotalBytesSent upon inactivity for
> SSL_DEF_TLS_RECORD_MSEC_THRESHOLD
> + sslTotalBytesSent = 0;
> + }
> + Debug("ssl", "SSLNetVConnection::loadBufferAndCallWrite, now %" PRId64
> ",lastwrite %" PRId64 " ,msec_since_last_write %d", now, sslLastWriteTime,
> msec_since_last_write);
> + }
> +
> if (HttpProxyPort::TRANSPORT_BLIND_TUNNEL == this->attributes) {
> return this->super::load_buffer_and_write(towrite, wattempted,
> total_wrote, buf, needs);
This is not your change, but s/total_wrote/total_written/ ;)
> }
> @@ -648,7 +662,25 @@ SSLNetVConnection::load_buffer_and_write(int64_t
> towrite, int64_t &wattempted, i
> // operations.
> int64_t orig_l = l;
> if (SSLConfigParams::ssl_maxrecord > 0 && l >
> SSLConfigParams::ssl_maxrecord) {
> - l = SSLConfigParams::ssl_maxrecord;
> + l = SSLConfigParams::ssl_maxrecord;
> +
> + // if ssl_maxrecord is configured higher than SSL_MAX_TLS_RECORD_SIZE
> + // round off the SSL_write at multiples of SSL_MAX_TLS_RECORD_SIZE to
> + // ensure openSSL can flush at TLS record boundary
> + if (l > SSL_MAX_TLS_RECORD_SIZE) {
> + l -= (l % SSL_MAX_TLS_RECORD_SIZE);
> + }
AFAICT, this is not necessary. The SSL transport will fragment at record
boundaries. This code just confuses the reader.
> + } else if (SSLConfigParams::ssl_maxrecord == -1) {
> + if (sslTotalBytesSent < SSL_DEF_TLS_RECORD_BYTE_THRESHOLD) {
> + dynamic_tls_record_size = SSL_DEF_TLS_RECORD_SIZE;
> + SSL_INCREMENT_DYN_STAT(ssl_total_dyn_def_tls_record_count);
> + } else {
> + dynamic_tls_record_size = SSL_MAX_TLS_RECORD_SIZE;
> + SSL_INCREMENT_DYN_STAT(ssl_total_dyn_max_tls_record_count);
> + }
> + if (l > dynamic_tls_record_size) {
> + l = dynamic_tls_record_size;
> + }
> }
>
> if (!l) {
> @@ -660,6 +692,7 @@ SSLNetVConnection::load_buffer_and_write(int64_t towrite,
> int64_t &wattempted, i
> Debug("ssl", "SSLNetVConnection::loadBufferAndCallWrite, before
> SSLWriteBuffer, l=%" PRId64", towrite=%" PRId64", b=%p",
> l, towrite, b);
> err = SSLWriteBuffer(ssl, b->start() + offset, l, r);
> +
> if (r == l) {
> wattempted = total_wrote;
> }
> @@ -676,6 +709,8 @@ SSLNetVConnection::load_buffer_and_write(int64_t towrite,
> int64_t &wattempted, i
> } while (r == l && total_wrote < towrite && b);
>
> if (r > 0) {
> + sslLastWriteTime = now;
> + sslTotalBytesSent += total_wrote;
> if (total_wrote != wattempted) {
> Debug("ssl", "SSLNetVConnection::loadBufferAndCallWrite, wrote some
> bytes, but not all requested.");
> // I'm not sure how this could happen. We should have tried and hit an
> EAGAIN.
> @@ -744,6 +779,8 @@ SSLNetVConnection::SSLNetVConnection():
> npnSet(NULL),
> npnEndpoint(NULL)
> {
> + sslLastWriteTime = 0;
> + sslTotalBytesSent = 0;
Why isn't this in the initializer list?
You also should reset these in SSLNetVConnection::free. I expect the handshake
stat is wrong too, since that is not reset either.
> }
>
> void
> @@ -936,6 +973,8 @@ SSLNetVConnection::sslServerHandShakeEvent(int &err)
> }
>
> sslHandShakeComplete = true;
> + sslLastWriteTime = 0;
> + sslTotalBytesSent = 0;
This is weird, are you compensating for the missing code in free()?
>
> if (sslHandshakeBeginTime) {
> const ink_hrtime ssl_handshake_time = ink_get_hrtime() -
> sslHandshakeBeginTime;
> @@ -1058,6 +1097,8 @@ SSLNetVConnection::sslClientHandShakeEvent(int &err)
> }
>
> sslHandShakeComplete = true;
> + sslLastWriteTime = 0;
> + sslTotalBytesSent = 0;
This too
> return EVENT_DONE;
>
> case SSL_ERROR_WANT_WRITE:
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/dee36e71/iocore/net/SSLUtils.cc
> ----------------------------------------------------------------------
> diff --git a/iocore/net/SSLUtils.cc b/iocore/net/SSLUtils.cc
> index 27e0c19..286ba69 100644
> --- a/iocore/net/SSLUtils.cc
> +++ b/iocore/net/SSLUtils.cc
> @@ -1794,6 +1794,10 @@ SSLWriteBuffer(SSL * ssl, const void * buf, int64_t
> nbytes, int64_t& nwritten)
> int ret = SSL_write(ssl, buf, (int)nbytes);
> if (ret > 0) {
> nwritten = ret;
> + BIO *bio = SSL_get_wbio(ssl);
> + if (bio != NULL) {
> + BIO_flush(bio);
> + }
> return SSL_ERROR_NONE;
> }
>
>
