> On Dec 29, 2016, at 1:13 PM, Maxim Dounin <[email protected]> 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 <[email protected]> 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 <[email protected]> > # 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 <[email protected]> > # 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 _______________________________________________ nginx-devel mailing list [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
