Welcome! Log In Create A New Profile

Advanced

Re: [PATCH 02 of 11] SSL: reduced logging of session cache failures (ticket #621)

Maxim Dounin
September 16, 2022 05:04PM
Hello!

On Thu, Sep 15, 2022 at 09:37:17AM +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 1661481947 -10800
> > # Fri Aug 26 05:45:47 2022 +0300
> > # Node ID 5b137f110e84af974ef2b9efcf35bec2d883c187
> > # Parent 2cd8fbeb4edc5a99b725585edc02a16a8a0c503e
> > SSL: reduced logging of session cache failures (ticket #621).
> >
> > Session cache allocations might fail as long as the new session is different
> > in size from the one least recently used (and freed when the first allocation
> > fails). In particular, it might not be possible to allocate space for
> > sessions with client certificates, since they are noticeably bigger than
> > normal sessions.
> >
> > To ensure such allocation failures won't clutter logs, logging level changed
> > to "warn", and logging is now limited to at most one warning per second.
> >
> > 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
> > @@ -3949,8 +3949,11 @@ failed:
> >
> > ngx_shmtx_unlock(&shpool->mutex);
> >
> > - ngx_log_error(NGX_LOG_ALERT, c->log, 0,
> > - "could not allocate new session%s", shpool->log_ctx);
> > + if (cache->fail_time != ngx_time()) {
> > + cache->fail_time = ngx_time();
> > + ngx_log_error(NGX_LOG_WARN, c->log, 0,
> > + "could not allocate new session%s", shpool->log_ctx);
> > + }
> >
>
> This makes three ngx_time() calls in this function in total.
> A good reason to cache value in a local variable.

Well, not really.

This is an error code path, which is not normally used. In the
hot path, there is still just one ngx_time() call. Trying to
provide appropriate cached value in all cases where this error
path is followed means that it have to be done at the very start
of the function, and it is going to make the function less
readable, and might actually make it less optimal.

Further, the

if (fail_time != ngx_time()) {
fail_time = ngx_time();
...
}

pattern is used elsewhere, and I would rather keep the code
similar in all cases.

Not to mention that ngx_time() is not really a call, but rather a
macro, which expands to ngx_cached_time->sec. While it might be
beneficial to cache the value if it is used multiple times, since
ngx_cached_time is volatile, the performance impact is minimal.

> > return 0;
> > }
> > 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
> > @@ -150,6 +150,7 @@ typedef struct {
> > ngx_rbtree_t session_rbtree;
> > ngx_rbtree_node_t sentinel;
> > ngx_queue_t expire_queue;
> > + time_t fail_time;
> > } ngx_ssl_session_cache_t;
> >
>
> Missed initialization to something sensible (zero?).

It doesn't really matter here, but won't hurt anyway. Added the following
chunk:

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
@@ -3770,6 +3770,8 @@ ngx_ssl_session_cache_init(ngx_shm_zone_

ngx_queue_init(&cache->expire_queue);

+ cache->fail_time = 0;
+
len = sizeof(" in SSL session shared cache \"\"") + shm_zone->shm.name.len;

shpool->log_ctx = ngx_slab_alloc(shpool, len);

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org
Subject Author Views Posted

[PATCH 02 of 11] SSL: reduced logging of session cache failures (ticket #621)

Maxim Dounin 264 August 25, 2022 11:14PM

Re: [PATCH 02 of 11] SSL: reduced logging of session cache failures (ticket #621)

Sergey Kandaurov 31 September 15, 2022 01:38AM

Re: [PATCH 02 of 11] SSL: reduced logging of session cache failures (ticket #621)

Maxim Dounin 31 September 16, 2022 05:04PM

Re: [PATCH 02 of 11] SSL: reduced logging of session cache failures (ticket #621)

Sergey Kandaurov 22 September 26, 2022 06:14AM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 111
Record Number of Users: 6 on February 13, 2018
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready