> On 17 Sep 2022, at 01:04, Maxim Dounin <mdounin@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@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.

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

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