Hello! On Thu, Sep 15, 2022 at 09:41:36AM +0400, Sergey Kandaurov wrote:
> > On 26 Aug 2022, at 07:01, Maxim Dounin <mdou...@mdounin.ru> wrote: > > > > # HG changeset patch > > # User Maxim Dounin <mdou...@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. Quoting the comment before the function: * OpenSSL's i2d_SSL_SESSION() and d2i_SSL_SESSION are slow, * so they are outside the code locked by shared pool mutex Hope this helps. > > + 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. I don't think it worth the effort. Either way, it is completely unrelated to this patch series. -- Maxim Dounin http://mdounin.ru/ _______________________________________________ nginx-devel mailing list -- nginx-devel@nginx.org To unsubscribe send an email to nginx-devel-le...@nginx.org