Awesome, really glad to see this! A couple of followup questions...

(a) Is there any way to force a packet flush on record end? At the moment
nginx will fragment multiple records across packet boundaries, which is
suboptimal as it means that I need a minimum of two packets to decode any
record - e.g. if I set my record size to 1370 bytes, the first packet will
contain the first full record plus another 20-50 bytes of next record.

(b) Current NGX_SSL_BUFSIZE is set to 16KB which is effectively guaranteed
to overflow the CWND of a new connection and introduce another RTT for
interactive traffic - e.g. HTTP pages. I would love to see a lower starting
record size to mitigate this problem -- defaults matter!

On the subject of optimizing record size, the GFE team at Google recently
landed ~following logic:

- new connections default to small record size
-- each record fits into a TCP packet
-- packets are flushed at record boundaries
- server tracks number of bytes written since reset and timestamp of last
write
-- if bytes written > {configurable byte threshold) then boost record size
to 16KB
-- if last write timestamp > now - {configurable time threshold} then reset
sent byte count

In other words, start with small record size to optimize for delivery of
small/interactive objects (bulk of HTTP traffic). Then, if large file is
being transferred bump record size to 16KB and continue using that until
the connection goes idle.. when communication resumes, start with small
record size and repeat. Overall, this is aimed to optimize delivery of
small files where incremental delivery is a priority, and also for large
downloads where overall throughput is a priority.

Both byte and time thresholds are exposed as configurable flags, and
current defaults in GFE are 1MB and 1s.

This would require a bit more work than the current patch, but I'd love to
see a similar strategy in nginx. Hardcoding a fixed record size will
inevitably lead to suboptimal delivery of either interactive or bulk
traffic. Thoughts?

ig



On Fri, Dec 20, 2013 at 4:19 AM, Maxim Dounin <mdounin@mdounin.ru> wrote:

> details: http://hg.nginx.org/nginx/rev/a297b7ad6f94
> branches:
> changeset: 5487:a297b7ad6f94
> user: Maxim Dounin <mdounin@mdounin.ru>
> date: Fri Dec 20 16:18:25 2013 +0400
> description:
> SSL: ssl_buffer_size directive.
>
> diffstat:
>
> src/event/ngx_event_openssl.c | 9 ++++++---
> src/event/ngx_event_openssl.h | 2 ++
> src/http/modules/ngx_http_ssl_module.c | 13 +++++++++++++
> src/http/modules/ngx_http_ssl_module.h | 2 ++
> 4 files changed, 23 insertions(+), 3 deletions(-)
>
> diffs (121 lines):
>
> diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c
> --- a/src/event/ngx_event_openssl.c
> +++ b/src/event/ngx_event_openssl.c
> @@ -190,6 +190,8 @@ ngx_ssl_create(ngx_ssl_t *ssl, ngx_uint_
> return NGX_ERROR;
> }
>
> + ssl->buffer_size = NGX_SSL_BUFSIZE;
> +
> /* client side options */
>
> SSL_CTX_set_options(ssl->ctx, SSL_OP_MICROSOFT_SESS_ID_BUG);
> @@ -726,6 +728,7 @@ ngx_ssl_create_connection(ngx_ssl_t *ssl
> }
>
> sc->buffer = ((flags & NGX_SSL_BUFFER) != 0);
> + sc->buffer_size = ssl->buffer_size;
>
> sc->connection = SSL_new(ssl->ctx);
>
> @@ -1222,7 +1225,7 @@ ngx_ssl_send_chain(ngx_connection_t *c,
> buf = c->ssl->buf;
>
> if (buf == NULL) {
> - buf = ngx_create_temp_buf(c->pool, NGX_SSL_BUFSIZE);
> + buf = ngx_create_temp_buf(c->pool, c->ssl->buffer_size);
> if (buf == NULL) {
> return NGX_CHAIN_ERROR;
> }
> @@ -1231,14 +1234,14 @@ ngx_ssl_send_chain(ngx_connection_t *c,
> }
>
> if (buf->start == NULL) {
> - buf->start = ngx_palloc(c->pool, NGX_SSL_BUFSIZE);
> + buf->start = ngx_palloc(c->pool, c->ssl->buffer_size);
> if (buf->start == NULL) {
> return NGX_CHAIN_ERROR;
> }
>
> buf->pos = buf->start;
> buf->last = buf->start;
> - buf->end = buf->start + NGX_SSL_BUFSIZE;
> + buf->end = buf->start + c->ssl->buffer_size;
> }
>
> send = buf->last - buf->pos;
> diff --git a/src/event/ngx_event_openssl.h b/src/event/ngx_event_openssl.h
> --- a/src/event/ngx_event_openssl.h
> +++ b/src/event/ngx_event_openssl.h
> @@ -29,6 +29,7 @@
> typedef struct {
> SSL_CTX *ctx;
> ngx_log_t *log;
> + size_t buffer_size;
> } ngx_ssl_t;
>
>
> @@ -37,6 +38,7 @@ typedef struct {
>
> ngx_int_t last;
> ngx_buf_t *buf;
> + size_t buffer_size;
>
> ngx_connection_handler_pt handler;
>
> diff --git a/src/http/modules/ngx_http_ssl_module.c
> b/src/http/modules/ngx_http_ssl_module.c
> --- a/src/http/modules/ngx_http_ssl_module.c
> +++ b/src/http/modules/ngx_http_ssl_module.c
> @@ -111,6 +111,13 @@ static ngx_command_t ngx_http_ssl_comma
> offsetof(ngx_http_ssl_srv_conf_t, ciphers),
> NULL },
>
> + { ngx_string("ssl_buffer_size"),
> + NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_TAKE1,
> + ngx_conf_set_size_slot,
> + NGX_HTTP_SRV_CONF_OFFSET,
> + offsetof(ngx_http_ssl_srv_conf_t, buffer_size),
> + NULL },
> +
> { ngx_string("ssl_verify_client"),
> NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_TAKE1,
> ngx_conf_set_enum_slot,
> @@ -424,6 +431,7 @@ ngx_http_ssl_create_srv_conf(ngx_conf_t
>
> sscf->enable = NGX_CONF_UNSET;
> sscf->prefer_server_ciphers = NGX_CONF_UNSET;
> + sscf->buffer_size = NGX_CONF_UNSET_SIZE;
> sscf->verify = NGX_CONF_UNSET_UINT;
> sscf->verify_depth = NGX_CONF_UNSET_UINT;
> sscf->builtin_session_cache = NGX_CONF_UNSET;
> @@ -465,6 +473,9 @@ ngx_http_ssl_merge_srv_conf(ngx_conf_t *
> (NGX_CONF_BITMASK_SET|NGX_SSL_SSLv3|NGX_SSL_TLSv1
> |NGX_SSL_TLSv1_1|NGX_SSL_TLSv1_2));
>
> + ngx_conf_merge_size_value(conf->buffer_size, prev->buffer_size,
> + NGX_SSL_BUFSIZE);
> +
> ngx_conf_merge_uint_value(conf->verify, prev->verify, 0);
> ngx_conf_merge_uint_value(conf->verify_depth, prev->verify_depth, 1);
>
> @@ -572,6 +583,8 @@ ngx_http_ssl_merge_srv_conf(ngx_conf_t *
> return NGX_CONF_ERROR;
> }
>
> + conf->ssl.buffer_size = conf->buffer_size;
> +
> if (conf->verify) {
>
> if (conf->client_certificate.len == 0 && conf->verify != 3) {
> diff --git a/src/http/modules/ngx_http_ssl_module.h
> b/src/http/modules/ngx_http_ssl_module.h
> --- a/src/http/modules/ngx_http_ssl_module.h
> +++ b/src/http/modules/ngx_http_ssl_module.h
> @@ -26,6 +26,8 @@ typedef struct {
> ngx_uint_t verify;
> ngx_uint_t verify_depth;
>
> + size_t buffer_size;
> +
> ssize_t builtin_session_cache;
>
> time_t session_timeout;
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel