> On 26 Aug 2022, at 07:01, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> # HG changeset patch
> # User Maxim Dounin <mdounin@mdounin.ru>
> # Date 1661481950 -10800
> # Fri Aug 26 05:45:50 2022 +0300
> # Node ID e88baee178eed529c6170678e373f5e2e0883c37
> # Parent f4ae0f4ee928cf20346530e96f1431314ecd0171
> SSL: single allocation in session cache on 32-bit platforms.
>
> Given the present typical SSL session sizes, on 32-bit platforms it is
> now beneficial to store all data in a single allocation, since rbtree
> node + session id + ASN1 representation of a session takes 256 bytes of
> shared memory (36 + 32 + 150 = about 218 bytes plus SNI server name).
>
> Storing all data in a single allocation is beneficial for SNI names up to
> about 40 characters long and makes it possible to store about 4000 sessions
> in one megabyte (instead of about 3000 sessions now). This also slightly
> simplifies the code.
>
> 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
> @@ -3789,9 +3789,9 @@ ngx_ssl_session_cache_init(ngx_shm_zone_
> * Typical length of the external ASN1 representation of a session
> * is about 150 bytes plus SNI server name.
> *
> - * On 32-bit platforms we allocate separately an rbtree node,
> - * a session id, and an ASN1 representation, they take accordingly
> - * 64, 32, and 256 bytes.
> + * On 32-bit platforms we allocate an rbtree node, a session id, and
> + * an ASN1 representation in a single allocation, it typically takes
> + * 256 bytes.
> *
> * On 64-bit platforms we allocate separately an rbtree node + session_id,
> * and an ASN1 representation, they take accordingly 128 and 256 bytes.
> @@ -3804,7 +3804,8 @@ static int
> ngx_ssl_new_session(ngx_ssl_conn_t *ssl_conn, ngx_ssl_session_t *sess)
> {
> int len;
> - u_char *p, *id, *cached_sess, *session_id;
> + u_char *p, *session_id;
> + size_t n;
> uint32_t hash;
> SSL_CTX *ssl_ctx;
> unsigned int session_id_length;
> @@ -3863,23 +3864,13 @@ ngx_ssl_new_session(ngx_ssl_conn_t *ssl_
> /* drop one or two expired sessions */
> ngx_ssl_expire_sessions(cache, shpool, 1);
>
> - cached_sess = ngx_slab_alloc_locked(shpool, len);
> -
> - if (cached_sess == NULL) {
> -
> - /* drop the oldest non-expired session and try once more */
> -
> - ngx_ssl_expire_sessions(cache, shpool, 0);
> -
> - cached_sess = ngx_slab_alloc_locked(shpool, len);
> -
> - if (cached_sess == NULL) {
> - sess_id = NULL;
> - goto failed;
> - }
> - }
> -
> - sess_id = ngx_slab_alloc_locked(shpool, sizeof(ngx_ssl_sess_id_t));
> +#if (NGX_PTR_SIZE == 8)
> + n = sizeof(ngx_ssl_sess_id_t);
> +#else
> + n = offsetof(ngx_ssl_sess_id_t, session) + len;
> +#endif
> +
> + sess_id = ngx_slab_alloc_locked(shpool, n);
>
> if (sess_id == NULL) {
>
> @@ -3887,7 +3878,7 @@ ngx_ssl_new_session(ngx_ssl_conn_t *ssl_
>
> ngx_ssl_expire_sessions(cache, shpool, 0);
>
> - sess_id = ngx_slab_alloc_locked(shpool, sizeof(ngx_ssl_sess_id_t));
> + sess_id = ngx_slab_alloc_locked(shpool, n);
>
> if (sess_id == NULL) {
> goto failed;
> @@ -3896,30 +3887,25 @@ ngx_ssl_new_session(ngx_ssl_conn_t *ssl_
>
> #if (NGX_PTR_SIZE == 8)
>
> - id = sess_id->sess_id;
> -
> -#else
> -
> - id = ngx_slab_alloc_locked(shpool, session_id_length);
> -
> - if (id == NULL) {
> + sess_id->session = ngx_slab_alloc_locked(shpool, len);
> +
> + if (sess_id->session == NULL) {
>
> /* drop the oldest non-expired session and try once more */
>
> ngx_ssl_expire_sessions(cache, shpool, 0);
>
> - id = ngx_slab_alloc_locked(shpool, session_id_length);
> -
> - if (id == NULL) {
> + sess_id->session = ngx_slab_alloc_locked(shpool, len);
> +
> + if (sess_id->session == NULL) {
> goto failed;
> }
> }
>
> #endif
>
> - ngx_memcpy(cached_sess, buf, len);
> -
> - ngx_memcpy(id, session_id, session_id_length);
> + ngx_memcpy(sess_id->session, buf, len);

Converting to ASN1 looks feasible without intermediate buf[]
to avoid extra memory copy.

> + ngx_memcpy(sess_id->id, session_id, session_id_length);
>
> hash = ngx_crc32_short(session_id, session_id_length);
>
> @@ -3929,9 +3915,7 @@ ngx_ssl_new_session(ngx_ssl_conn_t *ssl_
>
> sess_id->node.key = hash;
> sess_id->node.data = (u_char) session_id_length;
> - sess_id->id = id;
> sess_id->len = len;

With comment in the previous patch#04, this makes the following:

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
@@ -3817,7 +3817,6 @@ ngx_ssl_new_session(ngx_ssl_conn_t *ssl_
ngx_slab_pool_t *shpool;
ngx_ssl_sess_id_t *sess_id;
ngx_ssl_session_cache_t *cache;
- u_char buf[NGX_SSL_MAX_SESSION_SIZE];

#ifdef TLS1_3_VERSION

@@ -3843,9 +3842,6 @@ ngx_ssl_new_session(ngx_ssl_conn_t *ssl_
return 0;
}

- p = buf;
- i2d_SSL_SESSION(sess, &p);
-
session_id = (u_char *) SSL_SESSION_get_id(sess, &session_id_length);

/* do not cache sessions with too long session id */
@@ -3907,7 +3903,10 @@ ngx_ssl_new_session(ngx_ssl_conn_t *ssl_

#endif

- ngx_memcpy(sess_id->session, buf, len);
+ p = sess_id->session;
+ i2d_SSL_SESSION(sess, &p);
+ sess_id->len = len;
+
ngx_memcpy(sess_id->id, session_id, session_id_length);

hash = ngx_crc32_short(session_id, session_id_length);
@@ -3918,7 +3917,6 @@ ngx_ssl_new_session(ngx_ssl_conn_t *ssl_

sess_id->node.key = hash;
sess_id->node.data = (u_char) session_id_length;
- sess_id->len = len;

sess_id->expire = ngx_time() + SSL_CTX_get_timeout(ssl_ctx);


> - sess_id->session = cached_sess;
>
> sess_id->expire = ngx_time() + SSL_CTX_get_timeout(ssl_ctx);
>
> @@ -3945,10 +3929,6 @@ ngx_ssl_new_session(ngx_ssl_conn_t *ssl_
>
> failed:
>
> - if (cached_sess) {
> - ngx_slab_free_locked(shpool, cached_sess);
> - }
> -
> if (sess_id) {
> ngx_slab_free_locked(shpool, sess_id);
> }
> @@ -4045,9 +4025,8 @@ ngx_ssl_get_cached_session(ngx_ssl_conn_
>
> ngx_rbtree_delete(&cache->session_rbtree, node);
>
> +#if (NGX_PTR_SIZE == 8)
> ngx_slab_free_locked(shpool, sess_id->session);
> -#if (NGX_PTR_SIZE == 4)
> - ngx_slab_free_locked(shpool, sess_id->id);
> #endif
> ngx_slab_free_locked(shpool, sess_id);
>
> @@ -4135,9 +4114,8 @@ ngx_ssl_remove_session(SSL_CTX *ssl, ngx
>
> ngx_rbtree_delete(&cache->session_rbtree, node);
>
> +#if (NGX_PTR_SIZE == 8)
> ngx_slab_free_locked(shpool, sess_id->session);
> -#if (NGX_PTR_SIZE == 4)
> - ngx_slab_free_locked(shpool, sess_id->id);
> #endif
> ngx_slab_free_locked(shpool, sess_id);
>
> @@ -4184,9 +4162,8 @@ ngx_ssl_expire_sessions(ngx_ssl_session_
>
> ngx_rbtree_delete(&cache->session_rbtree, &sess_id->node);
>
> +#if (NGX_PTR_SIZE == 8)
> ngx_slab_free_locked(shpool, sess_id->session);
> -#if (NGX_PTR_SIZE == 4)
> - ngx_slab_free_locked(shpool, sess_id->id);
> #endif
> ngx_slab_free_locked(shpool, sess_id);
> }
> 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
> @@ -134,14 +134,14 @@ typedef struct ngx_ssl_sess_id_s ngx_ss
>
> struct ngx_ssl_sess_id_s {
> ngx_rbtree_node_t node;
> - u_char *id;
> size_t len;
> - u_char *session;
> ngx_queue_t queue;
> time_t expire;
> + u_char id[32];
> #if (NGX_PTR_SIZE == 8)
> - void *stub;
> - u_char sess_id[32];
> + u_char *session;
> +#else
> + u_char session[1];
> #endif
> };
>

This reminds me that this structure coupled with ngx_ssl_ticket_key_t
and ngx_ssl_session_cache_t aren't used outside and can be made private.

--
Sergey Kandaurov

_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org