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. 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. # 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; -- Maxim Dounin http://nginx.org/ _______________________________________________ nginx-devel mailing list [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
