[PATCH 05 of 11] SSL: single allocation in session cache on 32-bit platforms

Sergey Kandaurov pluknet at nginx.com
Mon Sep 26 10:13:30 UTC 2022


> On 17 Sep 2022, at 01:04, Maxim Dounin <mdounin at mdounin.ru> wrote:
> 
> Hello!
> 
> On Thu, Sep 15, 2022 at 09:41:36AM +0400, Sergey Kandaurov wrote:
> 
>>> On 26 Aug 2022, at 07:01, Maxim Dounin <mdounin at mdounin.ru> wrote:
>>> 
>>> # HG changeset patch
>>> # User Maxim Dounin <mdounin at 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.

Thanks, somehow missed that.

[..]

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

Sure.

-- 
Sergey Kandaurov



More information about the nginx-devel mailing list