conf, router: Make the listen(2) backlog configurable

[ac000]

@oopsoop2 on GitHub reported a performance issue related to the default listen(2) backlog size of 511 on nginx. They found that increasing it helped, nginx has a config option to configure this.

They would like to be able to do the same on Unit (which also defaults to 511 on some systems, incl Linux). This seems reasonable.

This adds a new per-listener 'backlog' config option, e.g

  {
      "listeners": {
          "[::1]:8080": {
              "pass": "routes",
              "backlog": 1024
          },
      }

      ...
  }

This doesn't effect the control socket.

[hongzhidao]

I feel the prefix can be removed from the commit subject, "conf, router" is a bit strange.

[hongzhidao]

Hi @ac000,
What about moving it into nxt_router_listener_conf?
For example:

diff --git a/src/nxt_router.c b/src/nxt_router.c
index 43209451..fe4bbabc 100644
--- a/src/nxt_router.c
+++ b/src/nxt_router.c
@@ -1964,13 +1964,10 @@ nxt_router_conf_create(nxt_task_t *task, nxt_router_temp_conf_t *tmcf,
                 break;
             }

-            skcf = nxt_router_socket_conf(task, tmcf, &name);
-            if (skcf == NULL) {
-                goto fail;
-            }
-
             nxt_memzero(&lscf, sizeof(lscf));

+            lscf.backlog = -1;
+
             ret = nxt_conf_map_object(mp, listener, nxt_router_listener_conf,
                                       nxt_nitems(nxt_router_listener_conf),
                                       &lscf);
@@ -1981,6 +1978,13 @@ nxt_router_conf_create(nxt_task_t *task, nxt_router_temp_conf_t *tmcf,

             nxt_debug(task, "application: %V", &lscf.applicatio
n);

+            /* use lscf.backlog */
+
+            skcf = nxt_router_socket_conf(task, tmcf, &name);
+            if (skcf == NULL) {
+                goto fail;
+            }
+
             // STUB, default values if http block is not define
d.

I usually prefer to get object property by nxt_conf_map_object() rather than explicitly nxt_conf_get_object_member().

Others look good.

[ac000]

I did try that originally but then it started getting messy... let me take another look...

[ac000]

OK, so it means

1. Adding a new 'backlog' entry to nxt_router_listener_conf[]

2. Adding a new 'backlog' member to nxt_router_listener_conf_t

3. Adding a pointer to nxt_router_listener_conf_t in nxt_router_temp_conf_t

So I took the simpler route, but sure, I can do the above instead...

[ac000]

I feel the prefix can be removed from the commit subject, "conf, router" is a bit strange.

It's simply because sometimes a change effects multiple areas, e.g

b5fe3eaf auto, wasm-wc: Copy the .so into build/lib/unit/modules/
8e254a4d tstr, conf: Ensure error strings are nul-terminated
5d1ce5c4 auto, perl: Fix building the Perl language module with clang

In this case it adds a new config option that effects the router. If I removed it, I would need to change the subject as well as this change only effects the routers listen sockets...

[ac000]

• Make use of nxt_conf_map_object() to get the backlog value

This meant the following needed to be done

1. Add a new 'backlog' entry to nxt_router_listener_conf[]
2. Add a new 'backlog' member to nxt_router_listener_conf_t
3. Add a pointer to nxt_router_listener_conf_t in nxt_router_temp_conf_t

That last one required moving the nxt_router_listener_conf_t structure definition into src/nxt_router.h

It's a bit of mess due to all the variable re-alignment required (but that's a result of how we do our variable declarations...)

I will likely split this up into several patches once we're happy with the actual code...

$ git range-diff c6d5699c...fbf15bdd
1:  c6d5699c ! 1:  fbf15bdd conf, router: Make the listen(2) backlog configurable
    @@ src/nxt_conf_validation.c: nxt_conf_vldt_forwarded(nxt_conf_validation_t *vldt,
          nxt_conf_value_t *value)
     
      ## src/nxt_router.c ##
    -@@ src/nxt_router.c: static void nxt_router_app_prefork_ready(nxt_task_t *task,
    - static void nxt_router_app_prefork_error(nxt_task_t *task,
    -     nxt_port_recv_msg_t *msg, void *data);
    - static nxt_socket_conf_t *nxt_router_socket_conf(nxt_task_t *task,
    --    nxt_router_temp_conf_t *tmcf, nxt_str_t *name);
    -+    nxt_router_temp_conf_t *tmcf, nxt_str_t *name, int backlog);
    - static nxt_int_t nxt_router_listen_socket_find(nxt_router_temp_conf_t *tmcf,
    -     nxt_socket_conf_t *nskcf, nxt_sockaddr_t *sa);
    +@@ src/nxt_router.c: typedef struct {
    + } nxt_router_app_conf_t;
    + 
    + 
    +-typedef struct {
    +-    nxt_str_t         pass;
    +-    nxt_str_t         application;
    +-} nxt_router_listener_conf_t;
    +-
    + 
    + #if (NXT_TLS)
    + 
    +@@ src/nxt_router.c: static nxt_conf_map_t  nxt_router_listener_conf[] = {
    +         NXT_CONF_MAP_STR_COPY,
    +         offsetof(nxt_router_listener_conf_t, application),
    +     },
    ++
    ++    {
    ++        nxt_string("backlog"),
    ++        NXT_CONF_MAP_INT32,
    ++        offsetof(nxt_router_listener_conf_t, backlog),
    ++    },
    + };
    + 
      
     @@ src/nxt_router.c: nxt_router_conf_create(nxt_task_t *task, nxt_router_temp_conf_t *tmcf,
    -         next = 0;
    - 
    -         for ( ;; ) {
    -+            int               listen_backlog = -1;
    -+            nxt_conf_value_t  *backlog;
    -+
    -+            static const nxt_str_t  backlog_path = nxt_string("backlog");
    -+
    -             listener = nxt_conf_next_object_member(listeners, &name, &next);
    -             if (listener == NULL) {
                      break;
                  }
      
     -            skcf = nxt_router_socket_conf(task, tmcf, &name);
    -+            backlog = nxt_conf_get_object_member(listener, &backlog_path, NULL);
    -+            if (backlog != NULL) {
    -+                listen_backlog = nxt_conf_get_number(backlog);
    +-            if (skcf == NULL) {
    +-                goto fail;
    +-            }
    +-
    +             nxt_memzero(&lscf, sizeof(lscf));
    + 
    ++            lscf.backlog = -1;
    ++
    +             ret = nxt_conf_map_object(mp, listener, nxt_router_listener_conf,
    +                                       nxt_nitems(nxt_router_listener_conf),
    +                                       &lscf);
    +@@ src/nxt_router.c: nxt_router_conf_create(nxt_task_t *task, nxt_router_temp_conf_t *tmcf,
    + 
    +             nxt_debug(task, "application: %V", &lscf.application);
    + 
    ++            tmcf->listener_conf = &lscf;
    ++
    ++            skcf = nxt_router_socket_conf(task, tmcf, &name);
    ++            if (skcf == NULL) {
    ++                goto fail;
     +            }
     +
    -+            skcf = nxt_router_socket_conf(task, tmcf, &name, listen_backlog);
    -             if (skcf == NULL) {
    -                 goto fail;
    -             }
    -@@ src/nxt_router.c: nxt_router_application_init(nxt_router_conf_t *rtcf, nxt_str_t *name,
    - 
    - static nxt_socket_conf_t *
    +             // STUB, default values if http block is not defined.
    +             skcf->header_buffer_size = 2048;
    +             skcf->large_header_buffer_size = 8192;
    +@@ src/nxt_router.c: static nxt_socket_conf_t *
      nxt_router_socket_conf(nxt_task_t *task, nxt_router_temp_conf_t *tmcf,
    --    nxt_str_t *name)
    -+    nxt_str_t *name, int backlog)
    +     nxt_str_t *name)
      {
    -     size_t               size;
    -     nxt_int_t            ret;
    +-    size_t               size;
    +-    nxt_int_t            ret;
    +-    nxt_bool_t           wildcard;
    +-    nxt_sockaddr_t       *sa;
    +-    nxt_socket_conf_t    *skcf;
    +-    nxt_listen_socket_t  *ls;
    ++    size_t                      size;
    ++    nxt_int_t                   ret;
    ++    nxt_bool_t                  wildcard;
    ++    nxt_sockaddr_t              *sa;
    ++    nxt_socket_conf_t           *skcf;
    ++    nxt_listen_socket_t         *ls;
    ++    nxt_router_listener_conf_t  *lscf;
    + 
    +     sa = nxt_sockaddr_parse(tmcf->mem_pool, name);
    +     if (nxt_slow_path(sa == NULL)) {
     @@ src/nxt_router.c: nxt_router_socket_conf(nxt_task_t *task, nxt_router_temp_conf_t *tmcf,
    + 
              nxt_listen_socket_remote_size(ls);
      
    ++        lscf = tmcf->listener_conf;
    ++
              ls->socket = -1;
     -        ls->backlog = NXT_LISTEN_BACKLOG;
    -+        ls->backlog = backlog > -1 ? backlog : NXT_LISTEN_BACKLOG;
    ++        ls->backlog = lscf->backlog > -1 ? lscf->backlog : NXT_LISTEN_BACKLOG;
              ls->flags = NXT_NONBLOCK;
              ls->read_after_accept = 1;
          }
    @@ src/nxt_router.c: nxt_router_listen_socket_ready(nxt_task_t *task, nxt_port_recv
          if (nxt_slow_path(ret != NXT_OK)) {
              goto fail;
          }
    +
    + ## src/nxt_router.h ##
    +@@ src/nxt_router.h: typedef struct {
    +     }                      action;
    + } nxt_router_engine_conf_t;
    + 
    ++typedef struct {
    ++    nxt_str_t                pass;
    ++    nxt_str_t                application;
    ++    int                      backlog;
    ++} nxt_router_listener_conf_t;
    + 
    + typedef struct {
    + #if (NXT_TLS)
    +-    nxt_queue_t            tls;        /* of nxt_router_tlssock_t */
    ++    nxt_queue_t                 tls;        /* of nxt_router_tlssock_t */
    + #endif
    + 
    + #if (NXT_HAVE_NJS)
    +-    nxt_queue_t            js_modules;
    ++    nxt_queue_t                 js_modules;
    + #endif
    + 
    +-    nxt_queue_t            apps;       /* of nxt_app_t */
    +-    nxt_queue_t            previous;   /* of nxt_app_t */
    ++    nxt_queue_t                 apps;       /* of nxt_app_t */
    ++    nxt_queue_t                 previous;   /* of nxt_app_t */
    + 
    +-    uint32_t               new_threads;
    +-    uint32_t               stream;
    +-    uint32_t               count;
    ++    uint32_t                    new_threads;
    ++    uint32_t                    stream;
    ++    uint32_t                    count;
    + 
    +-    nxt_event_engine_t     *engine;
    +-    nxt_port_t             *port;
    +-    nxt_array_t            *engines;
    +-    nxt_router_conf_t      *router_conf;
    +-    nxt_mp_t               *mem_pool;
    ++    nxt_event_engine_t          *engine;
    ++    nxt_port_t                  *port;
    ++    nxt_array_t                 *engines;
    ++    nxt_router_conf_t           *router_conf;
    ++    nxt_router_listener_conf_t  *listener_conf;
    ++    nxt_mp_t                    *mem_pool;
    + } nxt_router_temp_conf_t;
    + 
    + 

[hongzhidao]

That's what I mean.

[hongzhidao]

It looks like your previous way is better, could you try it again?

nxt_router_socket_conf(task, tmcf, &name, lscf.backlog);

That we don't need to add listener_conf to tmcf.
I guess it's more concise.

[ac000]

Ah, a mixture of the original and this way, yeah, that maybe makes sense.

[ac000]

This basically just reverts the last change to make the next change much clearer...

[ac000]

• Use nxt_conf_map_object() to get the backlog from the configuration and pass it directly into nxt_router_socket_conf()

$ git range-diff c6d5699c...4f99b044
1:  c6d5699c ! 1:  4f99b044 conf, router: Make the listen(2) backlog configurable
    @@ src/nxt_conf_validation.c: nxt_conf_vldt_forwarded(nxt_conf_validation_t *vldt,
          nxt_conf_value_t *value)
     
      ## src/nxt_router.c ##
    +@@ src/nxt_router.c: typedef struct {
    + typedef struct {
    +     nxt_str_t         pass;
    +     nxt_str_t         application;
    ++    int               backlog;
    + } nxt_router_listener_conf_t;
    + 
    + 
     @@ src/nxt_router.c: static void nxt_router_app_prefork_ready(nxt_task_t *task,
      static void nxt_router_app_prefork_error(nxt_task_t *task,
          nxt_port_recv_msg_t *msg, void *data);
    @@ src/nxt_router.c: static void nxt_router_app_prefork_ready(nxt_task_t *task,
      static nxt_int_t nxt_router_listen_socket_find(nxt_router_temp_conf_t *tmcf,
          nxt_socket_conf_t *nskcf, nxt_sockaddr_t *sa);
      
    -@@ src/nxt_router.c: nxt_router_conf_create(nxt_task_t *task, nxt_router_temp_conf_t *tmcf,
    -         next = 0;
    +@@ src/nxt_router.c: static nxt_conf_map_t  nxt_router_listener_conf[] = {
    +         NXT_CONF_MAP_STR_COPY,
    +         offsetof(nxt_router_listener_conf_t, application),
    +     },
    ++
    ++    {
    ++        nxt_string("backlog"),
    ++        NXT_CONF_MAP_INT32,
    ++        offsetof(nxt_router_listener_conf_t, backlog),
    ++    },
    + };
      
    -         for ( ;; ) {
    -+            int               listen_backlog = -1;
    -+            nxt_conf_value_t  *backlog;
    -+
    -+            static const nxt_str_t  backlog_path = nxt_string("backlog");
    -+
    -             listener = nxt_conf_next_object_member(listeners, &name, &next);
    -             if (listener == NULL) {
    + 
    +@@ src/nxt_router.c: nxt_router_conf_create(nxt_task_t *task, nxt_router_temp_conf_t *tmcf,
                      break;
                  }
      
     -            skcf = nxt_router_socket_conf(task, tmcf, &name);
    -+            backlog = nxt_conf_get_object_member(listener, &backlog_path, NULL);
    -+            if (backlog != NULL) {
    -+                listen_backlog = nxt_conf_get_number(backlog);
    +-            if (skcf == NULL) {
    +-                goto fail;
    +-            }
    +-
    +             nxt_memzero(&lscf, sizeof(lscf));
    + 
    ++            lscf.backlog = -1;
    ++
    +             ret = nxt_conf_map_object(mp, listener, nxt_router_listener_conf,
    +                                       nxt_nitems(nxt_router_listener_conf),
    +                                       &lscf);
    +@@ src/nxt_router.c: nxt_router_conf_create(nxt_task_t *task, nxt_router_temp_conf_t *tmcf,
    + 
    +             nxt_debug(task, "application: %V", &lscf.application);
    + 
    ++            skcf = nxt_router_socket_conf(task, tmcf, &name, lscf.backlog);
    ++            if (skcf == NULL) {
    ++                goto fail;
     +            }
     +
    -+            skcf = nxt_router_socket_conf(task, tmcf, &name, listen_backlog);
    -             if (skcf == NULL) {
    -                 goto fail;
    -             }
    +             // STUB, default values if http block is not defined.
    +             skcf->header_buffer_size = 2048;
    +             skcf->large_header_buffer_size = 8192;
     @@ src/nxt_router.c: nxt_router_application_init(nxt_router_conf_t *rtcf, nxt_str_t *name,
      
      static nxt_socket_conf_t *

[ac000]

Update the commit regarding the commit that changed the default listen backlog on Linux.

$ git range-diff 4f99b044...9c7c57ca
1:  4f99b044 ! 1:  9c7c57ca conf, router: Make the listen(2) backlog configurable
    @@ Commit message
         helped, nginx has a config option to configure this.
     
         They would like to be able to do the same on Unit (which also defaults
    -    to 511 on some systems, incl Linux). This seems reasonable.
    +    to 511 on some systems). This seems reasonable.
    +
    +    NOTE: On Linux before commit 97c15fa38 ("socket: Use a default listen
    +    backlog of -1 on Linux") we defaulted to 511. Since that commit we
    +    default to the Kernels default, which before 5.4 is 128 and after is
    +    4096.
     
         This adds a new per-listener 'backlog' config option, e.g
     

[ac000]

Rebased with master