Hello!

On Thu, Sep 15, 2022 at 09:41:36AM +0400, Sergey Kandaurov wrote:

> > 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.

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-leave@nginx.org