[PATCH] Fix drain logic for small number of connections
Joel Cunningham
joel.cunningham at me.com
Fri Dec 30 18:13:01 UTC 2016
> On Dec 29, 2016, at 1:13 PM, Maxim Dounin <mdounin at mdounin.ru> wrote:
>
> Hello!
>
> On Wed, Dec 28, 2016 at 12:56:46PM -0600, Joel Cunningham wrote:
>
>>> On Dec 28, 2016, at 11:05 AM, Maxim Dounin <mdounin at mdounin.ru> wrote:
>>>
>>> Hello!
>>>
>>> On Tue, Dec 27, 2016 at 04:37:36PM -0600, Joel Cunningham wrote:
>>>
>>>> Ping…any interest in accepting this patch?
>>>
>>> I'm not sure this patch improves things.
>>>
>>> Currently, 32 means that under constant load nginx will drain old
>>> connections once per 32 connection attempts. With the patch, 32
>>> is reduced to a smaller number, but this doesn't make any
>>> difference under constant load: if there are enough connections
>>> accepted on the current event loop iteration, all connections from
>>> the previous event loop iteration will be closed as long as they
>>> haven't managed to provide some data.
>>>
>>
>> You’re correct that when the worker limit is small (less than 4
>> times 32), we’ll will free less connections during a drain and
>> under a constant load, ngx_drain_connections ends being called
>> more often. This seemed a non-issue for situations with small
>> worker limits because freeing up to half or more (in my case
>> all) of the connections on the reusable_connections_queue seemed
>> inappropriate.
>>
>> Is it the case that 32 was chosen to limit how often we drain
>> rather than being a limit on how much of the queue to drain (and
>> limiting the drain is just a side-effect)?
>
> The idea of draining multiple connections at once is to minimize
> further need for draining, by providing some amount of free
> connections. This allows to save some resources by improving
> memory access locality.
>
> Additionally, this also provides some time for connections to be
> closed - e.g., it might not be possible to close an SSL connection
> immediately.
>
>>> The difference may occur if worker_connections limit is reached
>>> occasionally: in this case smaller number can mean that not all
>>> keepalive connections will be closed. But it is not clear how
>>> this is different from a server with a large number of established
>>> connections and only a small number of reusable ones.
>>
>> This is the case I’m running into. You’re correct that we’d
>> still close out newly accepted connections if the number of
>> reusable_connections_queue is smaller than the drain limit. It
>> happens to be the case in my situation where the
>> reusable_connections_queue is 99% keep alive connections and
>> only a single accepted connection which hasn’t been given the
>> chance to receive the request (the request is actually already
>> queued in the socket buffer, but since accept FD has a lower
>> value than the new connection FD, the connection gets closed out
>> before we can receive on it)
>
> Order of events is not bound to the file descriptor value. For
> most event methods it is rather bound to the time of the event.
> Moreover, using accept filters and/or deferred accept allows to
> read requests immediately from newly accepted sockets, making it
> completely up to the client to provide requests fast enough.
Thanks for mentioning the accept filters/deferred accept. I’ll have to look into enabling that in my port
>
> I agree though that if connection shortage is occasional the
> current behaviour might not be optimal with worker_connections set
> to a small value, or there are only a few connections can be
> reused. Instead of gradually reducing keepalive (and post accept)
> timeout to a smaller value it closes all keepalive connections
> from time to time.
>
> Probably optimal solution would be to close no more than some
> fraction of reusable connections at once. Patch series below.
I tested out the patch series and I did run into a problem. Looks like the reusable_connections_n increment and decrement are swapped. The first call to ngx_reusable_connection causes unsigned integer wrap around. I swapped the increment/decrement and the patch gives the expected behavior of only freeing 1/8 (or at least 1) of the reusable connections.
Thanks for the alternate approach, I agree it’s better use a drain limit based on the number of reusable connections rather than total number of connections (worker limit)
>
> # HG changeset patch
> # User Maxim Dounin <mdounin at mdounin.ru>
> # Date 1483036498 -10800
> # Thu Dec 29 21:34:58 2016 +0300
> # Node ID 016ea0d2dc42b5945a037618920ea8ed2d07a186
> # Parent a42afc225e98afe1f7c3b428c81c8af7eba362c0
> Added cycle parameter to ngx_drain_connections().
>
> No functional changes, mostly style.
>
> diff --git a/src/core/ngx_connection.c b/src/core/ngx_connection.c
> --- a/src/core/ngx_connection.c
> +++ b/src/core/ngx_connection.c
> @@ -13,7 +13,7 @@
> ngx_os_io_t ngx_io;
>
>
> -static void ngx_drain_connections(void);
> +static void ngx_drain_connections(ngx_cycle_t *cycle);
>
>
> ngx_listening_t *
> @@ -1046,7 +1046,7 @@ ngx_get_connection(ngx_socket_t s, ngx_l
> c = ngx_cycle->free_connections;
>
> if (c == NULL) {
> - ngx_drain_connections();
> + ngx_drain_connections((ngx_cycle_t *) ngx_cycle);
> c = ngx_cycle->free_connections;
> }
>
> @@ -1226,18 +1226,18 @@ ngx_reusable_connection(ngx_connection_t
>
>
> static void
> -ngx_drain_connections(void)
> +ngx_drain_connections(ngx_cycle_t *cycle)
> {
> ngx_int_t i;
> ngx_queue_t *q;
> ngx_connection_t *c;
>
> for (i = 0; i < 32; i++) {
> - if (ngx_queue_empty(&ngx_cycle->reusable_connections_queue)) {
> + if (ngx_queue_empty(&cycle->reusable_connections_queue)) {
> break;
> }
>
> - q = ngx_queue_last(&ngx_cycle->reusable_connections_queue);
> + q = ngx_queue_last(&cycle->reusable_connections_queue);
> c = ngx_queue_data(q, ngx_connection_t, queue);
>
> ngx_log_debug0(NGX_LOG_DEBUG_CORE, c->log, 0,
> # HG changeset patch
> # User Maxim Dounin <mdounin at mdounin.ru>
> # Date 1483036544 -10800
> # Thu Dec 29 21:35:44 2016 +0300
> # Node ID 166379461168a32d2137053961e8afc3435567db
> # Parent 016ea0d2dc42b5945a037618920ea8ed2d07a186
> Improved connection draining with small number of connections.
>
> Closing up to 32 connections might be too aggressive if worker_connections
> is set to a comparable number (and/or there are only a small number of
> reusable connections). If an occasional connection shorage happens in
> such a configuration, it leads to closing all reusable connections instead
> of gradually reducing keepalive timeout to a smaller value. To improve
> granularity in such configurations we now close no more than 1/8 of all
> reusable connections at once.
>
> Suggested by Joel Cunningham.
>
> diff --git a/src/core/ngx_connection.c b/src/core/ngx_connection.c
> --- a/src/core/ngx_connection.c
> +++ b/src/core/ngx_connection.c
> @@ -1204,6 +1204,7 @@ ngx_reusable_connection(ngx_connection_t
>
> if (c->reusable) {
> ngx_queue_remove(&c->queue);
> + ngx_cycle->reusable_connections_n++;
>
> #if (NGX_STAT_STUB)
> (void) ngx_atomic_fetch_add(ngx_stat_waiting, -1);
> @@ -1217,6 +1218,7 @@ ngx_reusable_connection(ngx_connection_t
>
> ngx_queue_insert_head(
> (ngx_queue_t *) &ngx_cycle->reusable_connections_queue, &c->queue);
> + ngx_cycle->reusable_connections_n—;
>
> #if (NGX_STAT_STUB)
> (void) ngx_atomic_fetch_add(ngx_stat_waiting, 1);
> @@ -1228,11 +1230,13 @@ ngx_reusable_connection(ngx_connection_t
> static void
> ngx_drain_connections(ngx_cycle_t *cycle)
> {
> - ngx_int_t i;
> + ngx_uint_t i, n;
> ngx_queue_t *q;
> ngx_connection_t *c;
>
> - for (i = 0; i < 32; i++) {
> + n = ngx_max(ngx_min(32, cycle->reusable_connections_n / 8), 1);
> +
> + for (i = 0; i < n; i++) {
> if (ngx_queue_empty(&cycle->reusable_connections_queue)) {
> break;
> }
> diff --git a/src/core/ngx_cycle.h b/src/core/ngx_cycle.h
> --- a/src/core/ngx_cycle.h
> +++ b/src/core/ngx_cycle.h
> @@ -53,6 +53,7 @@ struct ngx_cycle_s {
> ngx_uint_t modules_used; /* unsigned modules_used:1; */
>
> ngx_queue_t reusable_connections_queue;
> + ngx_uint_t reusable_connections_n;
>
> ngx_array_t listening;
> ngx_array_t paths;
>
>
Joel
More information about the nginx-devel
mailing list