[PATCH] Fix drain logic for small number of connections
Maxim Dounin
mdounin at mdounin.ru
Thu Dec 29 19:13:34 UTC 2016
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.
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 <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;
--
Maxim Dounin
http://nginx.org/
More information about the nginx-devel
mailing list