]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
4 years agoCLEANUP: dns: Remove useless test on ns->dgram in dns_connect_nameserver()
Christopher Faulet [Thu, 4 Mar 2021 15:58:35 +0000 (16:58 +0100)] 
CLEANUP: dns: Remove useless test on ns->dgram in dns_connect_nameserver()

When dns_connect_nameserver() is called, the nameserver has always a dgram
field properly defined. The caller, dns_send_nameserver(), already performed
the appropriate verification.

4 years agoCLEANUP: dns: Use DISGUISE() on a never-failing ring_attach() call
Christopher Faulet [Thu, 4 Mar 2021 15:53:27 +0000 (16:53 +0100)] 
CLEANUP: dns: Use DISGUISE() on a never-failing ring_attach() call

When a DNS session is created, the call to ring_attach() never fails. The
ring is freshly initialized and there is other watcher on it. Thus, the call
always succeeds.

Instead of catching an error that must never happen, we use the DISGUISE()
macro to make static analyzers happy.

4 years agoBUG/MINOR: server-state: Don't load server-state file for disabled backends
Christopher Faulet [Thu, 4 Mar 2021 15:35:26 +0000 (16:35 +0100)] 
BUG/MINOR: server-state: Don't load server-state file for disabled backends

Recent changes on the server-state file loading have introduced a
regression. HAproxy crashes if a backend with no server-state file is
disabled in the configuration. Indeed, configuration of such backends is not
finalized. Thus many fields are not defined.

To fix the bug, disabled backends must be ignored. In addition a BUG_ON()
has been added to verify the proxy mode regarding the server-state file. It
must be specified (none, global or local) for enabled backends.

No backport needed.

4 years agoBUG/MINOR: hlua: Don't strip last non-LWS char in hlua_pushstrippedstring()
Christopher Faulet [Wed, 3 Mar 2021 18:36:51 +0000 (19:36 +0100)] 
BUG/MINOR: hlua: Don't strip last non-LWS char in hlua_pushstrippedstring()

hlua_pushstrippedstring() function strips leading and trailing LWS
characters. But the result length it too short by 1 byte. Thus the last
non-LWS character is stripped. Note that a string containing only LWS
characters resulting to a stipped string with an invalid length (-1). This
leads to a lua runtime error.

This bug was reported in the issue #1155. It must be backported as far as
1.7.

4 years agoREGTESTS: test http-reuse if no server target
Amaury Denoyelle [Tue, 2 Mar 2021 13:39:20 +0000 (14:39 +0100)] 
REGTESTS: test http-reuse if no server target

Add two new regtests which check the behavior of http-reuse when the
connection target is not a server. More specifically check the dispatch
and transparent backend. In these cases, the behavior should be similar
to http-reuse never mode.

4 years agoMINOR: backend: handle reuse for conns with no server as target
Amaury Denoyelle [Tue, 2 Mar 2021 13:38:53 +0000 (14:38 +0100)] 
MINOR: backend: handle reuse for conns with no server as target

If dispatch mode or transparent backend is used, the backend connection
target is a proxy instead of a server. In these cases, the reuse of
backend connections is not consistent.

With the default behavior, no reuse is done and every new request uses a
new connection. However, if http-reuse is set to never, the connection
are stored by the mux in the session and can be reused for future
requests in the same session.

As no server is used for these connections, no reuse can be made outside
of the session, similarly to http-reuse never mode. A different
http-reuse config value should not have an impact. To achieve this, mark
these connections as private to have a defined behavior.

For this feature to properly work, the connection hash has been slightly
adjusted. The server pointer as an input as been replaced by a generic
target pointer to refer to the server or proxy instance. The hash is
always calculated on connect_server even if the connection target is not
a server. This also requires to allocate the connection hash node for
every backend connections, not just the one with a server target.

4 years agoBUG/MINOR: backend: free allocated bind_addr if reuse conn
Amaury Denoyelle [Tue, 2 Mar 2021 11:01:06 +0000 (12:01 +0100)] 
BUG/MINOR: backend: free allocated bind_addr if reuse conn

Fix a leak in connect_server which happens when a connection is reused
and a bind_addr was allocated because transparent mode is active. The
connection has already an allocated bind_addr so free the newly
allocated one.

No backport needed.

4 years agoCLEANUP: backend: fix a wrong comment
Amaury Denoyelle [Wed, 3 Mar 2021 10:24:33 +0000 (11:24 +0100)] 
CLEANUP: backend: fix a wrong comment

missing 'not' when skipping reuse if proxy mode not HTTP

4 years agoRevert "CI: Pin VTest to a known good commit"
Tim Duesterhus [Tue, 2 Mar 2021 18:18:59 +0000 (19:18 +0100)] 
Revert "CI: Pin VTest to a known good commit"

The issue with VTest now is fixed in VTest master since commit
vtest/VTest@040bb6737a6bd6c1f8941bc65ab8f3fc52d71130.

This reverts commit 24105300b87b5cfc1f21fd0788a0865723b043c8.

4 years agoCLEANUP: Use isttest(const struct ist) whenever possible
Tim Duesterhus [Tue, 2 Mar 2021 17:57:28 +0000 (18:57 +0100)] 
CLEANUP: Use isttest(const struct ist) whenever possible

Refactoring performed with the following Coccinelle patch:

    @@
    struct ist i;
    @@

    - i.ptr != NULL
    + isttest(i)

4 years agoCLEANUP: Use istadv(const struct ist, const size_t) whenever possible
Tim Duesterhus [Tue, 2 Mar 2021 17:57:27 +0000 (18:57 +0100)] 
CLEANUP: Use istadv(const struct ist, const size_t) whenever possible

Refactoring performed with the following Coccinelle patch:

    @@
    struct ist i;
    expression e;
    @@

    - i.ptr += e;
    - i.len -= e;
    + i = istadv(i, e);

4 years agoCLEANUP: Reapply the ist2() replacement patch
Tim Duesterhus [Tue, 2 Mar 2021 17:57:26 +0000 (18:57 +0100)] 
CLEANUP: Reapply the ist2() replacement patch

One location was not matched due to a typo. Reapply the patch for consistency.

see 92c696e663ab4bbcffd5dc0afedf1d0afd1c7279
see a3298023b04923ba12429d79c559dc7a850ae122

4 years agoBUG/MINOR: mux-h2: Fix typo in scheme adjustment
Tim Duesterhus [Sun, 28 Feb 2021 15:12:20 +0000 (16:12 +0100)] 
BUG/MINOR: mux-h2: Fix typo in scheme adjustment

That comma should've been a semicolon. Fortunately, as it is now there
is no impact thanks to operators precedence, and all expressions are
properly evaluated. But this is troubling and the risk is high to
turn it into an effective bug with a minor change.

Introduced in b8ce8905cf63ecd06b36af39c05103fadf3cc347 which first
appeared in 2.1-dev3. This fix must be backported to 2.1+.

4 years agoMINOR: contrib: Enhance peers dissector heuristic.
Frédéric Lécaille [Thu, 21 Jan 2021 15:25:45 +0000 (16:25 +0100)] 
MINOR: contrib: Enhance peers dissector heuristic.

When receiving a stick-table message header as two first bytes of a TCP segement
we consider this as being part of a peer protocol session.

4 years agoMINOR: contrib: add support for heartbeat control messages.
Frédéric Lécaille [Thu, 21 Jan 2021 15:23:29 +0000 (16:23 +0100)] 
MINOR: contrib: add support for heartbeat control messages.

Nothing really complicated: add a new control message type for such heartbeat
messages.

4 years agoDOC: spoe: Add a note about fragmentation support in HAProxy
Christopher Faulet [Tue, 2 Mar 2021 09:05:03 +0000 (10:05 +0100)] 
DOC: spoe: Add a note about fragmentation support in HAProxy

Add a note in SPOE.txt to make it clear that HAPRoxy does not support the
fragmentation. It can send fragmented frames if an agent supports it but it
cannot receives and handles fragmented frames.

This patch should fix the issue #659. It may be backported as far as 1.8.

4 years agoBUILD: quic: Implicit conversion between SSL related enums.
Frédéric Lécaille [Tue, 2 Mar 2021 09:28:04 +0000 (10:28 +0100)] 
BUILD: quic: Implicit conversion between SSL related enums.

Fix such compilation issues:

include/haproxy/quic_tls.h:157:10: error: implicit conversion from
'enum ssl_encryption_level_t' to 'enum quic_tls_enc_level'
[-Werror=enum-conversion]
  157 |   return ssl_encryption_application;
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~
src/xprt_quic.c: In function 'quic_conn_enc_level_init':
src/xprt_quic.c:2358:13: error: implicit conversion from
'enum quic_tls_enc_level' to 'enum ssl_encryption_level_t'
[-Werror=enum-conversion]
 2358 |  qel->level = quic_to_ssl_enc_level(level);
      |             ^

Not detected by all the compilators.

4 years agoBUILD: proxy: Missing header inclusion for quic_transport_params_init()
Frédéric Lécaille [Thu, 18 Feb 2021 15:35:43 +0000 (16:35 +0100)] 
BUILD: proxy: Missing header inclusion for quic_transport_params_init()

Since this commit:
144289b45 ("REORG: move init_default_instance() to proxy.c and pass it the defproxy pointer")
as quic_transport_params_init() has been moved from cfgparse.c to proxy.c this
latter source file must include xprt_quic.h header.

Should fix #1153 issue.

4 years agoCLEANUP: Use IST_NULL whenever possible
Tim Duesterhus [Sun, 28 Feb 2021 15:11:37 +0000 (16:11 +0100)] 
CLEANUP: Use IST_NULL whenever possible

Refactoring performed with the following Coccinelle patch:

    @@
    @@

    - ist2(NULL, 0)
    + IST_NULL

4 years agoCLEANUP: Use ist2(const void*, size_t) whenever possible
Tim Duesterhus [Sun, 28 Feb 2021 15:11:36 +0000 (16:11 +0100)] 
CLEANUP: Use ist2(const void*, size_t) whenever possible

Refactoring performed with the following Coccinelle patch:

    @@
    struct ist i;
    expression p, l;
    @@

    - i.ptr = p;
    - i.len = l;
    + i = ist2(p, l);

4 years agoBUG/MEDIUM: spoe: Kill applets if there are pending connections and nbthread > 1
Christopher Faulet [Mon, 1 Mar 2021 14:01:14 +0000 (15:01 +0100)] 
BUG/MEDIUM: spoe: Kill applets if there are pending connections and nbthread > 1

When the processing stage is finished for a SPOE applet, before returning it
into the idle list, we check if the assigned server appears as full or if
there are some pending connections on the backend or the assigned server. If
yes, it means we reach a maxconn and we close the applet to free a
slot. Otherwise, the applet can be reused. This test is only performed if
there are more than one thread.

It is important to close SPOE applets when there are pending connections for
multithreaded instances because connections with the SPOE agents are
persistent and local to a thread (applets are local to a thread). If a
maxconn is configured, some threads may take all available slots for a
while, leaving remaining threads without any free slot to process SPOE
messages. It is especially true if the maxconn is low.

This patch should fix the issue #705. It must be backported as far as
1.8. However, the code in 1.8 is quite different, a test must be performed
to be sure it works well.

4 years agoBUG/MINOR: connection: Use the client's dst family for adressless servers
Christopher Faulet [Mon, 1 Mar 2021 10:33:59 +0000 (11:33 +0100)] 
BUG/MINOR: connection: Use the client's dst family for adressless servers

When the selected server has no address, the destination address of the
client is used. However, for now, only the address is set, not the
family. Thus depending on how the server is configured and the client's
destination address, the server address family may be wrong.

For instance, with such server :

   server srv 0.0.0.0:0

The server address family is AF_INET. The server connection will fail if a
client is asking for an IPv6 destination.

To fix the bug, we take care to set the rigth family, the family of the
client destination address.

This patch should fix the issue #202. It must be backported to all stable
versions.

4 years agoBUG/MINOR: tcp-act: Don't forget to set the original port for IPv4 set-dst rule
Christopher Faulet [Mon, 1 Mar 2021 10:21:14 +0000 (11:21 +0100)] 
BUG/MINOR: tcp-act: Don't forget to set the original port for IPv4 set-dst rule

If an IPv4 is set via a TCP/HTTP set-dst rule, the original port must be
preserved or set to 0 if the previous family was neither AF_INET nor
AF_INET6. The first case is not an issue because the port remains the
same. But if the previous family was, for instance, AF_UNIX, the port is not
set to 0 and have an undefined value.

This patch must be backported as far as 1.7.

4 years agoCLEANUP: assorted typo fixes in the code and comments
Ilya Shipitsin [Fri, 19 Feb 2021 19:23:36 +0000 (00:23 +0500)] 
CLEANUP: assorted typo fixes in the code and comments

This is 18th iteration of typo fixes

4 years agoCI: codespell: skip Makefile for spell check
Ilya Shipitsin [Fri, 19 Feb 2021 19:18:00 +0000 (00:18 +0500)] 
CI: codespell: skip Makefile for spell check

checking Makefile is noisy, we do not benefit from it

4 years ago[RELEASE] Released version 2.4-dev10 v2.4-dev10
Willy Tarreau [Fri, 26 Feb 2021 21:49:10 +0000 (22:49 +0100)] 
[RELEASE] Released version 2.4-dev10

Released version 2.4-dev10 with the following main changes :
    - BUILD: SSL: introduce fine guard for RAND_keep_random_devices_open
    - MINOR: Configure the `cpp` userdiff driver for *.[ch] in .gitattributes
    - BUG/MINOR: ssl/cli: potential null pointer dereference in "set ssl cert"
    - BUG/MINOR: sample: secure convs that accept base64 string and var name as args
    - BUG/MEDIUM: vars: make functions vars_get_by_{name,desc} thread-safe
    - CLEANUP: vars: make smp_fetch_var() to reuse vars_get_by_desc()
    - DOC: muxes: add a diagram of the exchanges between muxes and outer world
    - BUG/MEDIUM: proxy: use thread-safe stream killing on hard-stop
    - BUG/MEDIUM: cli/shutdown sessions: make it thread-safe
    - BUG/MINOR: proxy: wake up all threads when sending the hard-stop signal
    - MINOR: stream: add an "epoch" to figure which streams appeared when
    - MINOR: cli/streams: make "show sess" dump all streams till the new epoch
    - MINOR: streams: use one list per stream instead of a global one
    - MEDIUM: streams: do not use the streams lock anymore
    - BUILD: dns: avoid a build warning when threads are disabled (dss unused)
    - MEDIUM: task: remove the tasks_run_queue counter and have one per thread
    - MINOR: tasks: do not maintain the rqueue_size counter anymore
    - CLEANUP: tasks: use a less confusing name for task_list_size
    - CLEANUP: task: move the tree root detection from __task_wakeup() to task_wakeup()
    - MINOR: task: limit the remote thread wakeup to the global runqueue only
    - MINOR: task: move the allocated tasks counter to the per-thread struct
    - CLEANUP: task: split the large tasklet_wakeup_on() function in two
    - BUG/MINOR: fd: properly wait for !running_mask in fd_set_running_excl()
    - BUG/MINOR: resolvers: Fix condition to release received ARs if not assigned
    - BUG/MINOR: resolvers: Only renew TTL for SRV records with an additional record
    - BUG/MINOR: resolvers: new callback to properly handle SRV record errors
    - BUG/MEDIUM: resolvers: Reset server address and port for obselete SRV records
    - BUG/MEDIUM: resolvers: Reset address for unresolved servers
    - DOC: Update the module list in MAINTAINERS file
    - MINOR: htx: Add function to reserve the max possible size for an HTX DATA block
    - DOC: Update the HTX API documentation
    - DOC: Update the filters guide
    - BUG/MEDIUM: contrib/prometheus-exporter: fix segfault in listener name dump
    - MINOR: task: split the counts of local and global tasks picked
    - MINOR: task: do not use __task_unlink_rq() from process_runnable_tasks()
    - MINOR: task: don't decrement then increment the local run queue
    - CLEANUP: task: re-merge __task_unlink_rq() with task_unlink_rq()
    - MINOR: task: make grq_total atomic to move it outside of the grq_lock
    - MINOR: tasks: also compute the tasklet latency when DEBUG_TASK is set
    - MINOR: task: make tasklet wakeup latency measurements more accurate
    - MINOR: server: Be more strict on the server-state line parsing
    - MINOR: server: Only fill one array when parsing a server-state line
    - MEDIUM: server: Refactor apply_server_state() to make it more readable
    - CLEANUP: server: Rename state_line node to node instead of name_name
    - CLEANUP: server: Rename state_line structure into server_state_line
    - CLEANUP: server: Use a local eb-tree to store lines of the global server-state file
    - MINOR: server: Be more strict when reading the version of a server-state file
    - MEDIUM: server: Store parsed params of a server-state line in the tree
    - MINOR: server: Remove cached line from global server-state tree when found
    - MINOR: server: Move loading state of servers in a dedicated function
    - MEDIUM: server: Use a tree to store local server-state lines
    - MINOR: server: Parse and store server-state lines in a dedicated function
    - MEDIUM: server: Don't load server-state file if a line is corrupted
    - REORG: server: Export and rename some functions updating server info
    - REORG: server-state: Move functions to deal with server-state in its own file
    - MINOR: server-state: Don't load server-state file for serverless proxies
    - CLEANUP: muxes: Remove useless if condition in show_fd function
    - BUG/MINOR: stats: fix compare of no-maint url suffix
    - MINOR: task: limit the number of subsequent heavy tasks with flag TASK_HEAVY
    - MINOR: ssl: mark the SSL handshake tasklet as heavy
    - CLEANUP: server: rename srv_cleanup_{idle,toremove}_connections()
    - BUG/MINOR: ssl: potential null pointer dereference in ckchs_dup()
    - MINOR: task: add one extra tasklet class: TL_HEAVY
    - MINOR: task: place the heavy elements in TL_HEAVY
    - MINOR: task: only limit TL_HEAVY tasks but not others
    - BUG/MINOR: http-ana: Only consider dst address to process originalto option
    - MINOR: tools: Add net_addr structure describing a network addess
    - MINOR: tools: Add function to compare an address to a network address
    - MEDIUM: http-ana: Add IPv6 support for forwardfor and orignialto options
    - CLEANUP: hlua: Use net_addr structure internally to parse and compare addresses
    - REGTESTS: Add script to test except param for fowardedfor/originalto options
    - DOC: scheduler: add a diagram showing the different queues and their usages
    - CLEANUP: tree-wide: replace free(x);x=NULL with ha_free(&x)
    - CLEANUP: config: replace a few free() with ha_free()
    - CLEANUP: vars: always zero the pointers after a free()
    - CLEANUP: ssl: remove a useless "if" before freeing an error message
    - CLEANUP: ssl: make ssl_sock_free_srv_ctx() zero the pointers after free
    - CLEANUP: ssl: use realloc() instead of free()+malloc()

4 years agoCLEANUP: ssl: use realloc() instead of free()+malloc()
Willy Tarreau [Fri, 26 Feb 2021 20:05:08 +0000 (21:05 +0100)] 
CLEANUP: ssl: use realloc() instead of free()+malloc()

There was a free(ptr) followed by ptr=malloc(ptr, len), which is the
equivalent of ptr = realloc(ptr, len) but slower and less clean. Let's
replace this.

4 years agoCLEANUP: ssl: make ssl_sock_free_srv_ctx() zero the pointers after free
Willy Tarreau [Fri, 26 Feb 2021 20:06:32 +0000 (21:06 +0100)] 
CLEANUP: ssl: make ssl_sock_free_srv_ctx() zero the pointers after free

In ssl_sock_free_srv_ctx() there are some calls to free() which are not
followed by a zeroing of the pointers. For now this function is only used
during deinit but it could be used at run time in the near future, so
better secure this.

4 years agoCLEANUP: ssl: remove a useless "if" before freeing an error message
Willy Tarreau [Fri, 26 Feb 2021 20:12:15 +0000 (21:12 +0100)] 
CLEANUP: ssl: remove a useless "if" before freeing an error message

Just an old "if (err) free(err)" that managed to escape cleanups.

4 years agoCLEANUP: vars: always zero the pointers after a free()
Willy Tarreau [Fri, 26 Feb 2021 20:19:53 +0000 (21:19 +0100)] 
CLEANUP: vars: always zero the pointers after a free()

In sample_store(), depending on the new sample types, the area pointer
was not always zeroed after being freed. Let's make sure it's always the
case to avoid the risk of dangling pointers being misused.

4 years agoCLEANUP: config: replace a few free() with ha_free()
Willy Tarreau [Fri, 26 Feb 2021 19:51:47 +0000 (20:51 +0100)] 
CLEANUP: config: replace a few free() with ha_free()

A few occurrences of calls to free() to free a section name,
peers name or server name were using casts and didn't include
the trailing free, let's switch them to ha_free().

4 years agoCLEANUP: tree-wide: replace free(x);x=NULL with ha_free(&x)
Willy Tarreau [Sat, 20 Feb 2021 09:46:51 +0000 (10:46 +0100)] 
CLEANUP: tree-wide: replace free(x);x=NULL with ha_free(&x)

This makes the code more readable and less prone to copy-paste errors.
In addition, it allows to place some __builtin_constant_p() predicates
to trigger a link-time error in case the compiler knows that the freed
area is constant. It will also produce compile-time error if trying to
free something that is not a regular pointer (e.g. a function).

The DEBUG_MEM_STATS macro now also defines an instance for ha_free()
so that all these calls can be checked.

178 occurrences were converted. The vast majority of them were handled
by the following Coccinelle script, some slightly refined to better deal
with "&*x" or with long lines:

  @ rule @
  expression E;
  @@
  - free(E);
  - E = NULL;
  + ha_free(&E);

It was verified that the resulting code is the same, more or less a
handful of cases where the compiler optimized slightly differently
the temporary variable that holds the copy of the pointer.

A non-negligible amount of {free(str);str=NULL;str_len=0;} are still
present in the config part (mostly header names in proxies). These
ones should also be cleaned for the same reasons, and probably be
turned into ist strings.

4 years agoDOC: scheduler: add a diagram showing the different queues and their usages
Willy Tarreau [Fri, 26 Feb 2021 16:39:04 +0000 (17:39 +0100)] 
DOC: scheduler: add a diagram showing the different queues and their usages

The scheduler has become complex over time and the latest updates were a
good opportunity to document it. This diagram shows the time-based wait
queue(s), the priority-based run queue(s), and the class-based tasklet
queues, trying to emphasize what is local-only and what is shared between
threads. The diagram is provided in .fig, .svg, .png, and .pdf.

4 years agoREGTESTS: Add script to test except param for fowardedfor/originalto options
Christopher Faulet [Fri, 26 Feb 2021 11:23:17 +0000 (12:23 +0100)] 
REGTESTS: Add script to test except param for fowardedfor/originalto options

IPv6 support was added for these options. This script test different IPv4
and IPv6 combinations.

4 years agoCLEANUP: hlua: Use net_addr structure internally to parse and compare addresses
Christopher Faulet [Fri, 26 Feb 2021 08:39:05 +0000 (09:39 +0100)] 
CLEANUP: hlua: Use net_addr structure internally to parse and compare addresses

hlua_addr structure may be replaced by net_addr structure to parse and
compare addresses. Both structures are similar.

4 years agoMEDIUM: http-ana: Add IPv6 support for forwardfor and orignialto options
Christopher Faulet [Fri, 26 Feb 2021 08:19:15 +0000 (09:19 +0100)] 
MEDIUM: http-ana: Add IPv6 support for forwardfor and orignialto options

A network may be specified to avoid header addition for "forwardfor" and
"orignialto" option via the "except" parameter. However, only IPv4
networks/addresses are supported. This patch adds the support of IPv6.

To do so, the net_addr structure is used to store the parameter value in the
proxy structure. And ipcmp2net() function is used to perform the comparison.

This patch should fix the issue #1145. It depends on the following commit:

  * c6ce0ab MINOR: tools: Add function to compare an address to a network address
  * 5587287 MINOR: tools: Add net_addr structure describing a network addess

4 years agoMINOR: tools: Add function to compare an address to a network address
Christopher Faulet [Fri, 26 Feb 2021 08:12:50 +0000 (09:12 +0100)] 
MINOR: tools: Add function to compare an address to a network address

ipcmp2net() function may be used to compare an addres (struct
sockaddr_storage) to a network address (struct net_addr). Among other
things, this function will be used to add support of IPv6 for "except"
parameter of "forwardfor" and "originalto" options.

4 years agoMINOR: tools: Add net_addr structure describing a network addess
Christopher Faulet [Thu, 25 Feb 2021 17:50:02 +0000 (18:50 +0100)] 
MINOR: tools: Add net_addr structure describing a network addess

The net_addr structure describes a IPv4 or IPv6 address. Its ip and mask are
represented. Among other things, this structure will be used to add support
of IPv6 for "except" parameter of "forwardfor" and "originalto" options.

4 years agoBUG/MINOR: http-ana: Only consider dst address to process originalto option
Christopher Faulet [Fri, 26 Feb 2021 11:45:56 +0000 (12:45 +0100)] 
BUG/MINOR: http-ana: Only consider dst address to process originalto option

When an except parameter is used for originalto option, only the destination
address must be evaluated. Especially, the address family of the destination
must be tested and not the source one.

This patch must be backported to all stable versions. However be careful,
depending the versions the code may be slightly different.

4 years agoMINOR: task: only limit TL_HEAVY tasks but not others
Willy Tarreau [Fri, 26 Feb 2021 09:18:11 +0000 (10:18 +0100)] 
MINOR: task: only limit TL_HEAVY tasks but not others

The preliminary approach to dealing with heavy tasks forced us to quit
the poller after meeting one. Now instead we process at most one per poll
loop and ignore the next ones, so that we get more bandwidth to process
all other classes.

Doing so further reduced the induced HTTP request latency at 100k req/s
under the stress of 1000 concurrent SSL handshakes in the following
proportions:

            |   default  | low-latency
   ---------+------------+--------------
    before  |   2.75 ms  |   2.0 ms
    after   |   1.38 ms  |   0.98 ms

In both cases, the latency is roughly halved. It's worth noting that
both values are now exactly 10 times better than in 2.4-dev9. Even the
percentiles have much improved. For 16 HTTP connections (1 per thread)
competing with 1000 SSL handshakes, we're seeing these long-tail
latencies (in milliseconds) :

              |  99.5%  |  99.9%  |  100%
   -----------+---------+---------+--------
   2.4-dev9   |  48.4   |  58.1   |  78.5
   previous   |   6.2   |  11.4   |  67.8
   this patch |   2.8   |   2.9   |   6.1

The task latency profiling report now shows this in default mode:
  $ socat - /tmp/sock1 <<< "show profiling"
  Per-task CPU profiling              : on      # set profiling tasks {on|auto|off}
  Tasks activity:
    function                      calls   cpu_tot   cpu_avg   lat_tot   lat_avg
    si_cs_io_cb                 3061966   2.224s    726.0ns   42.03s    13.72us
    h1_io_cb                    3061960   6.418s    2.096us   18.76m    367.6us
    process_stream              3059982   9.137s    2.985us   15.52m    304.3us
    ssl_sock_io_cb               602657   4.265m    424.7us   4.736h    28.29ms
    h1_timeout_task              202973      -         -      6.254s    30.81us
    accept_queue_process         135547   1.179s    8.699us   16.29s    120.1us
    srv_cleanup_toremove_conns       81   15.64ms   193.1us   30.87ms   381.1us
    task_run_applet                  10   758.7us   75.87us   51.77us   5.176us
    srv_cleanup_idle_conns            4   375.3us   93.83us   54.52us   13.63us

And this in low-latency mode, showing that both si_cs_io_cb() and process_stream()
have significantly benefitted from the improvement, with values 50 to 200 times
smaller than 2.4-dev9:
  $ socat - /tmp/sock1 <<< "show profiling"
  Per-task CPU profiling              : on      # set profiling tasks {on|auto|off}
  Tasks activity:
    function                      calls   cpu_tot   cpu_avg   lat_tot   lat_avg
    h1_io_cb                    6407006   11.86s    1.851us   31.14m    291.6us
    process_stream              6403890   18.40s    2.873us   2.134m    20.00us
    si_cs_io_cb                 6403866   4.139s    646.0ns   1.773m    16.61us
    ssl_sock_io_cb               894326   6.407m    429.9us   7.326h    29.49ms
    h1_timeout_task              301189      -         -      8.440s    28.02us
    accept_queue_process         211989   1.691s    7.977us   21.48s    101.3us
    srv_cleanup_toremove_conns      220   23.46ms   106.7us   65.61ms   298.2us
    task_run_applet                  16   1.219ms   76.17us   181.7us   11.36us
    srv_cleanup_idle_conns           12   713.3us   59.44us   168.4us   14.03us

The changes are slightly more invasive than previous ones and depend on
recent patches so they are not likely well suited for backporting.

4 years agoMINOR: task: place the heavy elements in TL_HEAVY
Willy Tarreau [Fri, 26 Feb 2021 09:13:40 +0000 (10:13 +0100)] 
MINOR: task: place the heavy elements in TL_HEAVY

Instead of placing heavy tasklets into the TL_BULK queue, we now place
them into the TL_HEAVY one, which is assigned a default weight of ~1%
load at once. This way heavy tasks will not block TL_BULK anymore.

4 years agoMINOR: task: add one extra tasklet class: TL_HEAVY
Willy Tarreau [Fri, 26 Feb 2021 08:16:22 +0000 (09:16 +0100)] 
MINOR: task: add one extra tasklet class: TL_HEAVY

This class will be used exclusively for heavy processing tasklets. It
will be cleaner than mixing them with the bulk ones. For now it's
allocated ~1% of the CPU bandwidth.

The largest part of the patch consists in re-arranging the fields in the
task_per_thread structure to preserve a clean alignment with one more
list head. Since we're now forced to increase the struct past a second
cache line, it now uses 4 cache lines (for easy multiplying) with the
first two ones being exclusively used by local operations and the third
one mostly by atomic operations. Interestingly, this better arrangement
causes less stress and reduced the response time by 8 microseconds at
1 million requests per second.

4 years agoBUG/MINOR: ssl: potential null pointer dereference in ckchs_dup()
Eric Salama [Tue, 23 Feb 2021 15:50:57 +0000 (16:50 +0100)] 
BUG/MINOR: ssl: potential null pointer dereference in ckchs_dup()

A potential null pointer dereference was reported with an old gcc
version (6.5)

    src/ssl_ckch.c: In function 'cli_parse_set_cert':
    src/ssl_ckch.c:844:7: error: potential null pointer dereference [-Werror=null-dereference]
      if (!ssl_sock_copy_cert_key_and_chain(src->ckch, dst->ckch))
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    src/ssl_ckch.c:844:7: error: potential null pointer dereference [-Werror=null-dereference]
    src/ssl_ckch.c: In function 'ckchs_dup':
    src/ssl_ckch.c:844:7: error: potential null pointer dereference [-Werror=null-dereference]
      if (!ssl_sock_copy_cert_key_and_chain(src->ckch, dst->ckch))
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    src/ssl_ckch.c:844:7: error: potential null pointer dereference [-Werror=null-dereference]

This could happen if ckch_store_new() fails to allocate memory and returns NULL.

This patch must be backported with 8f71298 since it was wrongly fixed and
the bug could happen.

Must be backported as far as 2.2.

4 years agoCLEANUP: server: rename srv_cleanup_{idle,toremove}_connections()
Willy Tarreau [Thu, 25 Feb 2021 23:30:22 +0000 (00:30 +0100)] 
CLEANUP: server: rename srv_cleanup_{idle,toremove}_connections()

These function names are unbearably long, they don't even fit into the
screen in "show profiling", let's trim the "_connections" to "_conns",
which happens to match the name of the lists there.

4 years agoMINOR: ssl: mark the SSL handshake tasklet as heavy
Willy Tarreau [Thu, 25 Feb 2021 14:31:00 +0000 (15:31 +0100)] 
MINOR: ssl: mark the SSL handshake tasklet as heavy

There's a fairness issue between SSL and clear text. A full end-to-end
cleartext connection can require up to ~7.7 wakeups on average, plus 3.3
for the SSL tasklet, one of which is particularly expensive. So if we
accept to process many handshakes taking 1ms each, we significantly
increase the processing time of regular tasks just by adding an extra
delay between their calls. Ideally in order to be fair we should have a
1:18 call ratio, but this requires a bit more accounting. With very little
effort we can mark the SSL handshake tasklet as TASK_HEAVY until the
handshake completes, and remove it once done.

Doing so reduces from 14 to 3.0 ms the total response time experienced
by HTTP clients running in parallel to 1000 SSL clients doing full
handshakes in loops. Better, when tune.sched.low-latency is set to "on",
the latency further drops to 1.8 ms.

The tasks latency distribution explain pretty well what is happening:

Without the patch:
  $ socat - /tmp/sock1 <<< "show profiling"
  Per-task CPU profiling              : on      # set profiling tasks {on|auto|off}
  Tasks activity:
    function                      calls   cpu_tot   cpu_avg   lat_tot   lat_avg
    ssl_sock_io_cb              2785375   19.35m    416.9us   5.401h    6.980ms
    h1_io_cb                    1868949   9.853s    5.271us   4.829h    9.302ms
    process_stream              1864066   7.582s    4.067us   2.058h    3.974ms
    si_cs_io_cb                 1733808   1.932s    1.114us   26.83m    928.5us
    h1_timeout_task              935760      -         -      1.033h    3.975ms
    accept_queue_process         303606   4.627s    15.24us   16.65m    3.291ms
    srv_cleanup_toremove_connections452   64.31ms   142.3us   2.447s    5.415ms
    task_run_applet                  47   5.149ms   109.6us   57.09ms   1.215ms
    srv_cleanup_idle_connections     34   2.210ms   65.00us   87.49ms   2.573ms

With the patch:
  $ socat - /tmp/sock1 <<< "show profiling"
  Per-task CPU profiling              : on      # set profiling tasks {on|auto|off}
  Tasks activity:
    function                      calls   cpu_tot   cpu_avg   lat_tot   lat_avg
    ssl_sock_io_cb              3000365   21.08m    421.6us   20.30h    24.36ms
    h1_io_cb                    2031932   9.278s    4.565us   46.70m    1.379ms
    process_stream              2010682   7.391s    3.675us   22.83m    681.2us
    si_cs_io_cb                 1702070   1.571s    922.0ns   8.732m    307.8us
    h1_timeout_task             1009594      -         -      17.63m    1.048ms
    accept_queue_process         339595   4.792s    14.11us   3.714m    656.2us
    srv_cleanup_toremove_connections779   75.42ms   96.81us   438.3ms   562.6us
    srv_cleanup_idle_connections     48   2.498ms   52.05us   178.1us   3.709us
    task_run_applet                  17   1.738ms   102.3us   11.29ms   663.9us
    other                             1   947.8us   947.8us   202.6us   202.6us

  => h1_io_cb() and process_stream() are divided by 6 while ssl_sock_io_cb() is
     multipled by 4

And with low-latency on:
  $ socat - /tmp/sock1 <<< "show profiling"
  Per-task CPU profiling              : on      # set profiling tasks {on|auto|off}
  Tasks activity:
    function                      calls   cpu_tot   cpu_avg   lat_tot   lat_avg
    ssl_sock_io_cb              3000565   20.96m    419.1us   20.74h    24.89ms
    h1_io_cb                    2019702   9.294s    4.601us   49.22m    1.462ms
    process_stream              2009755   6.570s    3.269us   1.493m    44.57us
    si_cs_io_cb                 1997820   1.566s    783.0ns   2.985m    89.66us
    h1_timeout_task             1009742      -         -      1.647m    97.86us
    accept_queue_process         494509   4.697s    9.498us   1.240m    150.4us
    srv_cleanup_toremove_connections1120   92.32ms   82.43us   463.0ms   413.4us
    srv_cleanup_idle_connections     70   2.703ms   38.61us   204.5us   2.921us
    task_run_applet                  13   1.303ms   100.3us   85.12us   6.548us

  => process_stream() is divided by 100 while ssl_sock_io_cb() is
     multipled by 4

Interestingly, the total HTTPS response time doesn't increase and even very
slightly decreases, with an overall ~1% higher request rate. The net effect
here is a redistribution of the CPU resources between internal tasks, and
in the case of SSL, handshakes wait bit more but everything after completes
faster.

This was made simple enough to be backportable if it helps some users
suffering from high latencies in mixed traffic.

4 years agoMINOR: task: limit the number of subsequent heavy tasks with flag TASK_HEAVY
Willy Tarreau [Thu, 25 Feb 2021 23:25:51 +0000 (00:25 +0100)] 
MINOR: task: limit the number of subsequent heavy tasks with flag TASK_HEAVY

While the scheduler is priority-aware and class-aware, and consistently
tries to maintain fairness between all classes, it doesn't make use of a
fine execution budget to compensate for high-latency tasks such as TLS
handshakes. This can result in many subsequent calls adding multiple
milliseconds of latency between the various steps of other tasklets that
don't even depend on this.

An ideal solution would be to add a 4th queue, have all tasks announce
their estimated cost upfront and let the scheduler maintain an auto-
refilling budget to pick from the most suitable queue.

But it turns out that a very simplified version of this already provides
impressive gains with very tiny changes and could easily be backported.
The principle is to reserve a new task flag "TASK_HEAVY" that indicates
that a task is expected to take a lot of time without yielding (e.g. an
SSL handshake typically takes 700 microseconds of crypto computation).
When the scheduler sees this flag when queuing a tasklet, it will place
it into the bulk queue. And during dequeuing, we accept only one of
these in a full round. This means that the first one will be accepted,
will not prevent other lower priority tasks from running, but if a new
one arrives, then the queue stops here and goes back to the polling.
This will allow to collect more important updates for other tasks that
will be batched before the next call of a heavy task.

Preliminary tests consisting in placing this flag on the SSL handshake
tasklet show that response times under SSL stress fell from 14 ms
before the patch to 3.0 ms with the patch, and even 1.8 ms if
tune.sched.low-latency is set to "on".

4 years agoBUG/MINOR: stats: fix compare of no-maint url suffix
Amaury Denoyelle [Thu, 25 Feb 2021 13:46:08 +0000 (14:46 +0100)] 
BUG/MINOR: stats: fix compare of no-maint url suffix

Only the first 3 characters are compared for ';no-maint' suffix in
http_handle_stats. Fix it by doing a full match over the entire suffix.

As a side effect, the ';norefresh' suffix matched the inaccurate
comparison, so the maintenance servers were always hidden on the stats
page in this case.

no-maint suffix is present since commit
  3e320367014c742814ba494594cdb8340b1161f1
  MINOR: stats: also support a "no-maint" show stat modifier

It should be backported up to 2.3.

This fixes github issue #1147.

4 years agoCLEANUP: muxes: Remove useless if condition in show_fd function
Christopher Faulet [Thu, 25 Feb 2021 09:06:29 +0000 (10:06 +0100)] 
CLEANUP: muxes: Remove useless if condition in show_fd function

In H1, H2 and FCGI muxes, in the show_fd function, there is duplicated test on
the stream's subs field.

This patch fixes the issue #1142. It may be backported as far as 2.2.

4 years agoMINOR: server-state: Don't load server-state file for serverless proxies
Christopher Faulet [Tue, 16 Feb 2021 13:36:06 +0000 (14:36 +0100)] 
MINOR: server-state: Don't load server-state file for serverless proxies

Just a minor improvement. Proxies with no server are now ignored early. It
may happens for listeners for instance.

4 years agoREORG: server-state: Move functions to deal with server-state in its own file
Christopher Faulet [Tue, 16 Feb 2021 12:31:30 +0000 (13:31 +0100)] 
REORG: server-state: Move functions to deal with server-state in its own file

All functions dealing with the server-state files are moved to
server_state.c.

srv_update_state() function was renammed to srv_state_srv_update().

4 years agoREORG: server: Export and rename some functions updating server info
Christopher Faulet [Tue, 16 Feb 2021 11:07:47 +0000 (12:07 +0100)] 
REORG: server: Export and rename some functions updating server info

Some static functions are now exported and renamed to follow the same
pattern of other exported functions. Here is the list :

 * update_server_fqdn: Renamed to srv_update_fqdn and exported
 * update_server_check_addr_port: renamed to srv_update_check_addr_port and exported
 * update_server_agent_addr_port: renamed to srv_update_agent_addr_port and exported
 * update_server_addr: renamed to srv_update_addr
 * update_server_addr_potr: renamed to srv_update_addr_port
 * srv_prepare_for_resolution: exported

This change is mandatory to move all functions dealing with the server-state
files in a separate file.

4 years agoMEDIUM: server: Don't load server-state file if a line is corrupted
Christopher Faulet [Tue, 16 Feb 2021 10:51:12 +0000 (11:51 +0100)] 
MEDIUM: server: Don't load server-state file if a line is corrupted

This change is not huge but may have a visible impact for users. Now, if a
line of a server-state file is corrupted, the whole file is ignored. A
warning is emitted with the corrupted line number.

In fact, there is no way to recover from a corrupted line. A line is
considered as corrupted if it is too long (truncated line) or if it contains
the wrong number of arguments. In both cases, it means the file was forged
(or at least manually edited). It is safer to ignore it.

Note for now, memory allocation errors are not reported and the
corresponding line is silently ignored.

4 years agoMINOR: server: Parse and store server-state lines in a dedicated function
Christopher Faulet [Tue, 16 Feb 2021 10:41:26 +0000 (11:41 +0100)] 
MINOR: server: Parse and store server-state lines in a dedicated function

Now, srv_state_parse_and_store_line() function is used to parse and store a
line in a tree. It is used for global and local server-state files. This
significatly simplies the apply_server_state() function.

4 years agoMEDIUM: server: Use a tree to store local server-state lines
Christopher Faulet [Tue, 16 Feb 2021 10:32:22 +0000 (11:32 +0100)] 
MEDIUM: server: Use a tree to store local server-state lines

Just like for the global server-state file, the line of a local server-state
file are now stored in a tree. This way, the file is fully parsed before
loading the servers state. And with this change, global and local
server-state files are now handled the same way. This will be the
opportunity to factorize the code. It is also a good way to validate the
file before loading any server state.

4 years agoMINOR: server: Move loading state of servers in a dedicated function
Christopher Faulet [Tue, 16 Feb 2021 10:30:47 +0000 (11:30 +0100)] 
MINOR: server: Move loading state of servers in a dedicated function

The loop on the servers of a proxy to load the server states was moved in
the function srv_state_px_update(). This simplify a bit the
apply_server_state() function. It is aslo mandatory to simplify the loading
of local server-state file.

4 years agoMINOR: server: Remove cached line from global server-state tree when found
Christopher Faulet [Tue, 16 Feb 2021 08:58:01 +0000 (09:58 +0100)] 
MINOR: server: Remove cached line from global server-state tree when found

When a server for a given backend is found in the tree containing all lines
of the global server-state file, the node is removed from the tree. It is
useless to keep it longer. It is a small improvement, but it may also be
usefull to track the orphan lines (not used for now).

4 years agoMEDIUM: server: Store parsed params of a server-state line in the tree
Christopher Faulet [Mon, 15 Feb 2021 17:27:35 +0000 (18:27 +0100)] 
MEDIUM: server: Store parsed params of a server-state line in the tree

Parsed parameters are now stored in the tree of server-state lines. This
way, a line from the global server-state file is only parsed once. Before,
it was parsed a first time to store it in the tree and one more time to load
the server state. To do so, the server-state line object must be allocated
before parsing a line. This means its size must no longer depend on the
length of first parsed parameters (backend and server names). Thus the node
type was changed to use a hashed key instead of a string.

4 years agoMINOR: server: Be more strict when reading the version of a server-state file
Christopher Faulet [Mon, 15 Feb 2021 15:24:10 +0000 (16:24 +0100)] 
MINOR: server: Be more strict when reading the version of a server-state file

Now, we read a full line and expects to found an integer only on it. And if
the line is empty or truncated, an error is returned. If the version is not
valid, an error is also returned. This way, the first line is no longer
partially read.

4 years agoCLEANUP: server: Use a local eb-tree to store lines of the global server-state file
Christopher Faulet [Mon, 15 Feb 2021 15:53:51 +0000 (16:53 +0100)] 
CLEANUP: server: Use a local eb-tree to store lines of the global server-state file

There is no reason to use a global variable to store the lines of the global
server-state file. This tree is only used during the file parsing, as a line
cache. Now the eb-tree is declared as a local variable in the
apply_server_state() function.

4 years agoCLEANUP: server: Rename state_line structure into server_state_line
Christopher Faulet [Mon, 15 Feb 2021 16:09:33 +0000 (17:09 +0100)] 
CLEANUP: server: Rename state_line structure into server_state_line

The structure used to store a server-state line in an eb-tree has a too
generic name. Instead of state_line, the structure is renamed as
server_state_line.

4 years agoCLEANUP: server: Rename state_line node to node instead of name_name
Christopher Faulet [Mon, 15 Feb 2021 11:07:40 +0000 (12:07 +0100)] 
CLEANUP: server: Rename state_line node to node instead of name_name

<state_line.name_name> field is a node in an eb-tree. Thus, instead of
"name_name", we now use "node" to name this field. If is a more explicit
name and not too strange.

4 years agoMEDIUM: server: Refactor apply_server_state() to make it more readable
Christopher Faulet [Fri, 12 Feb 2021 18:59:21 +0000 (19:59 +0100)] 
MEDIUM: server: Refactor apply_server_state() to make it more readable

The apply_server_state() function is really hard to read. Thus it was
refactored to be more maintainable. First, an helper function is used to get
the server-state file path. Some useless variables were removed and most of
other variables were renamed to be more readable. The error messages are now
prefixed to know the context (global vs per-proxy). Finally, the loop on the
proxies list was simplified.

This patch may seem a bit huge, but the changes are not so important.

4 years agoMINOR: server: Only fill one array when parsing a server-state line
Christopher Faulet [Fri, 12 Feb 2021 18:02:21 +0000 (19:02 +0100)] 
MINOR: server: Only fill one array when parsing a server-state line

There is no reason to fill two parameter arrays in srv_state_parse_line()
function. Now, only one array is used. The 4th first entries are just
skipped when srv_update_state() is called.

4 years agoMINOR: server: Be more strict on the server-state line parsing
Christopher Faulet [Fri, 12 Feb 2021 17:49:31 +0000 (18:49 +0100)] 
MINOR: server: Be more strict on the server-state line parsing

The srv_state_parse_line() function was rewritten to be more strict. First
of all, it is possible to make the difference between an ignored line and an
malformed one. Then, only blank characters (spaces and tabs) are now allowed
as field separator. An error is reported for truncated lines or for lines
with an unexpected number of arguments regarding the provided version.
However, for now, errors are ignored by the caller, invalid lines are just
skipped.

4 years agoMINOR: task: make tasklet wakeup latency measurements more accurate
Willy Tarreau [Thu, 25 Feb 2021 08:32:58 +0000 (09:32 +0100)] 
MINOR: task: make tasklet wakeup latency measurements more accurate

First, we don't want to measure wakeup times if the call date had not
been set before profiling was enabled at run time. And second, we may
only collect the value before clearing the TASK_IN_LIST bit, otherwise
another wakeup might happen on another thread and replace the call date
we're about to use, hence artificially lower the wakeup times.

4 years agoMINOR: tasks: also compute the tasklet latency when DEBUG_TASK is set
Willy Tarreau [Thu, 25 Feb 2021 07:39:07 +0000 (08:39 +0100)] 
MINOR: tasks: also compute the tasklet latency when DEBUG_TASK is set

It is extremely useful to be able to observe the wakeup latency of some
important I/O operations, so let's accept to inflate the tasklet struct
by 8 extra bytes when DEBUG_TASK is set. With just this we have enough
to get live reports like this:

  $ socat - /tmp/sock1 <<< "show profiling"
  Per-task CPU profiling              : on      # set profiling tasks {on|auto|off}
  Tasks activity:
    function                      calls   cpu_tot   cpu_avg   lat_tot   lat_avg
    si_cs_io_cb                 8099492   4.833s    596.0ns   8.974m    66.48us
    h1_io_cb                    7460365   11.55s    1.548us   2.477m    19.92us
    process_stream              7383828   22.79s    3.086us   18.39m    149.5us
    h1_timeout_task                4157      -         -      348.4ms   83.81us
    srv_cleanup_toremove_connections751   39.70ms   52.86us   10.54ms   14.04us
    srv_cleanup_idle_connections     21   1.405ms   66.89us   30.82us   1.467us
    task_run_applet                  16   1.058ms   66.13us   446.2us   27.89us
    accept_queue_process              7   34.53us   4.933us   333.1us   47.58us

4 years agoMINOR: task: make grq_total atomic to move it outside of the grq_lock
Willy Tarreau [Thu, 25 Feb 2021 06:51:18 +0000 (07:51 +0100)] 
MINOR: task: make grq_total atomic to move it outside of the grq_lock

Instead of decrementing grq_total once per task picked from the global
run queue, let's do it at once after the loop like we do for other
counters. This simplifies the code everywhere. It is not expected to
bring noticeable improvements however, since global tasks tend to be
less common nowadays.

4 years agoCLEANUP: task: re-merge __task_unlink_rq() with task_unlink_rq()
Willy Tarreau [Thu, 25 Feb 2021 06:47:03 +0000 (07:47 +0100)] 
CLEANUP: task: re-merge __task_unlink_rq() with task_unlink_rq()

There's no point keeping the two separate anymore, some tests are
duplicated for no reason.

4 years agoMINOR: task: don't decrement then increment the local run queue
Willy Tarreau [Thu, 25 Feb 2021 06:19:45 +0000 (07:19 +0100)] 
MINOR: task: don't decrement then increment the local run queue

Now we don't need to decrement rq_total when we pick a tack in the tree
to immediately increment it again after installing it into the local
list. Instead, we simply add to the local queue count the number of
globally picked tasks. Avoiding this shows ~0.5% performance gains at
1Mreq/s (2M task switches/s).

4 years agoMINOR: task: do not use __task_unlink_rq() from process_runnable_tasks()
Willy Tarreau [Thu, 25 Feb 2021 06:14:58 +0000 (07:14 +0100)] 
MINOR: task: do not use __task_unlink_rq() from process_runnable_tasks()

As indicated in previous commit, this function tries to guess which tree
the task is in to figure what counters to update, while we already have
that info in the caller. Let's just pick the relevant parts to place them
in the caller.

4 years agoMINOR: task: split the counts of local and global tasks picked
Willy Tarreau [Thu, 25 Feb 2021 06:09:08 +0000 (07:09 +0100)] 
MINOR: task: split the counts of local and global tasks picked

In process_runnable_tasks() we're still calling __task_unlink_rq() to
pick a task, and this function tries to guess where to pick the task
from and which counter to update while the caller's context already
has everything. Worse, the number of local tasks is decremented then
recredited, doubling the operations. In order to avoid this we first
need to keep separate counters for local and global tasks that were
picked. This is what this patch does.

4 years agoBUG/MEDIUM: contrib/prometheus-exporter: fix segfault in listener name dump
William Dauchy [Wed, 24 Feb 2021 23:53:13 +0000 (00:53 +0100)] 
BUG/MEDIUM: contrib/prometheus-exporter: fix segfault in listener name dump

We need to check whether listener is empty before doing anything; in
that case, we were trying to dump listerner name while name is null. So
simply move the counters check above, which validate all possible cases
when the listener is empty. This is very similar to what is done in
stats.c

see also the trace:

  Thread 1 "haproxy" received signal SIGSEGV, Segmentation fault.
  __strlen_sse2 () at ../sysdeps/x86_64/multiarch/../strlen.S:120
  120     ../sysdeps/x86_64/multiarch/../strlen.S: No such file or directory.
  (gdb) bt
  #0  __strlen_sse2 () at ../sysdeps/x86_64/multiarch/../strlen.S:120
  #1  0x00005555555b716b in promex_dump_listener_metrics (htx=0x5555558fadf0, appctx=0x555555926070) at contrib/prometheus-exporter/service-prometheus.c:722
  #2  promex_dump_metrics (htx=0x5555558fadf0, si=0x555555925920, appctx=0x555555926070) at contrib/prometheus-exporter/service-prometheus.c:1200
  #3  promex_appctx_handle_io (appctx=0x555555926070) at contrib/prometheus-exporter/service-prometheus.c:1477
  #4  0x00005555556f0c94 in task_run_applet (t=0x555555926180, context=0x555555926070, state=<optimized out>) at src/applet.c:88
  #5  0x00005555556bc6d8 in run_tasks_from_lists (budgets=budgets@entry=0x7fffffffe374) at src/task.c:548
  #6  0x00005555556bd1a0 in process_runnable_tasks () at src/task.c:750
  #7  0x0000555555696cdd in run_poll_loop () at src/haproxy.c:2870
  #8  0x0000555555697025 in run_thread_poll_loop (data=data@entry=0x0) at src/haproxy.c:3035
  #9  0x0000555555596c90 in main (argc=<optimized out>, argv=0x7fffffffe818) at src/haproxy.c:3723
  quit)

this bug was introduced by commit
e3f7bd5ae9e969cbfe87e4130d06bff7a3e814c6 ("MEDIUM:
contrib/prometheus-exporter: add listen stats"), which is present for
2.4 only, so no backport needed.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
4 years agoDOC: Update the filters guide
Christopher Faulet [Wed, 24 Feb 2021 20:58:43 +0000 (21:58 +0100)] 
DOC: Update the filters guide

The filters guide was totally outdated. Callbacks to filter payload were
changed, especially the HTTP one because of the HTX. All the HTTP legacy
part is removed. This new guide now reflects the reality.

This patch may be backported as far as 2.2.

4 years agoDOC: Update the HTX API documentation
Christopher Faulet [Wed, 24 Feb 2021 10:33:21 +0000 (11:33 +0100)] 
DOC: Update the HTX API documentation

Missing functions have been added. And because the EOM block was removed,
some parts have been adapted to better explain how the end of the message
may be detected.

4 years agoMINOR: htx: Add function to reserve the max possible size for an HTX DATA block
Christopher Faulet [Wed, 3 Feb 2021 11:11:31 +0000 (12:11 +0100)] 
MINOR: htx: Add function to reserve the max possible size for an HTX DATA block

The function htx_reserve_max_data() should be used to get an HTX DATA block
with the max possible size. A current block may be extended or a new one
created, depending on the HTX message state. But the idea is to let the
caller to copy a bunch of data without requesting many new blocks. It is its
responsibility to resize the block at the end, to set the final block size.

This function will be used to parse messages with small chunks. Indeed, we
can have more than 2700 1-byte chunks in a 16Kb of input data. So it is easy
to understand how this function may help to improve the parsing of chunk
messages.

4 years agoDOC: Update the module list in MAINTAINERS file
Christopher Faulet [Tue, 23 Feb 2021 16:51:02 +0000 (17:51 +0100)] 
DOC: Update the module list in MAINTAINERS file

Some missing modules have been added and some others have been updated. The
list is now sorted. It is a bit easier to find something. In addition the
path of files have been updated to reflect recent changes.

4 years agoBUG/MEDIUM: resolvers: Reset address for unresolved servers
Christopher Faulet [Tue, 23 Feb 2021 11:33:17 +0000 (12:33 +0100)] 
BUG/MEDIUM: resolvers: Reset address for unresolved servers

If the DNS resolution failed for a server, its ip address must be
removed. Otherwise, the server is stopped but keeps its ip. This may be
confusing when the servers state are retrieved on the CLI and it may lead to
undefined behavior if HAproxy is configured to load its servers state from a
file.

This patch should be backported as far as 2.0.

4 years agoBUG/MEDIUM: resolvers: Reset server address and port for obselete SRV records
Christopher Faulet [Tue, 23 Feb 2021 11:24:09 +0000 (12:24 +0100)] 
BUG/MEDIUM: resolvers: Reset server address and port for obselete SRV records

When a SRV record expires, the ip/port assigned to the associated server are
now removed. Otherwise, the server is stopped but keeps its ip/port while
the server hostname is removed. It is confusing when the servers state are
retrieve on the CLI and may be a problem if saved in a server-state
file. Because the reload may fail because of this inconsistency.

Here is an example:

 * Declare a server template in a backend, using the resolver <dns>

server-template test 2 _http._tcp.example.com resolvers dns check

 * 2 SRV records are announced with the corresponding additional
   records. Thus, 2 servers are filled. Here is the "show servers state"
   output :

2 frt 1 test1 192.168.1.1 2 64 0 1 2 15 3 4 6 0 0 0 http1.example.com 8001 _http._tcp.example.com 0 0 - - 0
2 frt 2 test2 192.168.1.2 2 64 0 1 1 15 3 4 6 0 0 0 http2.example.com 8002 _http._tcp.example.com 0 0 - - 0

 * Then, one additional record is removed (or a SRV record is removed, the
   result is the same). Here is the new "show servers state" output :

2 frt 1 test1 192.168.1.1 2 64 0 1 38 15 3 4 6 0 0 0 http1.example.com 8001 _http._tcp.example.com 0 0 - - 0
2 frt 2 test2 192.168.1.2 0 96 0 1 19 15 3 0 14 0 0 0 - 8002 _http._tcp.example.com 0 0 - - 0

On reload, if a server-state file is used, this leads to undefined behaviors
depending on the configuration.

This patch should be backported as far as 2.0.

4 years agoBUG/MINOR: resolvers: new callback to properly handle SRV record errors
Baptiste Assmann [Thu, 19 Nov 2020 21:38:33 +0000 (22:38 +0100)] 
BUG/MINOR: resolvers: new callback to properly handle SRV record errors

When a SRV record was created, it used to register the regular server name
resolution callbacks. That said, SRV records and regular server name
resolution don't work the same way, furthermore on error management.

This patch introduces a new call back to manage DNS errors related to
the SRV queries.

this fixes github issue #50.

Backport status: 2.3, 2.2, 2.1, 2.0

4 years agoBUG/MINOR: resolvers: Only renew TTL for SRV records with an additional record
Christopher Faulet [Tue, 23 Feb 2021 11:22:29 +0000 (12:22 +0100)] 
BUG/MINOR: resolvers: Only renew TTL for SRV records with an additional record

If no additional record is associated to a SRV record, its TTL must not be
renewed. Otherwise the entry never expires. Thus once announced a first
time, the entry remains blocked on the same IP/port except if a new announce
replaces the old one.

Now, the TTL is updated if a SRV record is received while a matching
existing one is found with an additional record or when an new additional
record is assigned to an existing SRV record.

This patch should be backported as far as 2.2.

4 years agoBUG/MINOR: resolvers: Fix condition to release received ARs if not assigned
Christopher Faulet [Tue, 23 Feb 2021 10:59:19 +0000 (11:59 +0100)] 
BUG/MINOR: resolvers: Fix condition to release received ARs if not assigned

At the end of resolv_validate_dns_response(), if a received additionnal
record is not assigned to an existing server record, it is released. But the
condition to do so is buggy. If "answer_record" (the received AR) is not
assigned, "tmp_record" is not a valid record object. It is just a dummy
record "representing" the head of the record list.

Now, the condition is far cleaner. This patch must be backported as far as
2.2.

4 years agoBUG/MINOR: fd: properly wait for !running_mask in fd_set_running_excl()
Willy Tarreau [Wed, 24 Feb 2021 18:40:49 +0000 (19:40 +0100)] 
BUG/MINOR: fd: properly wait for !running_mask in fd_set_running_excl()

In fd_set_running_excl() we don't reset the old mask in the CAS loop,
so if we fail on the first round, we'll forcefully take the FD on the
next one.

In practice it's used bu fd_insert() and fd_delete() only, none of which
is supposed to be passed an FD which is still in use since in practice,
given that for now only listeners may be enabled on multiple threads at
once.

This can be backported to 2.2 but shouldn't result in fixing any user
visible bug for now.

4 years agoCLEANUP: task: split the large tasklet_wakeup_on() function in two
Willy Tarreau [Wed, 24 Feb 2021 16:51:38 +0000 (17:51 +0100)] 
CLEANUP: task: split the large tasklet_wakeup_on() function in two

This function has become large with the multi-queue scheduler. We need
to keep the fast path and the debugging parts inlined, but the rest now
moves to task.c just like was done for task_wakeup(). This has reduced
the code size by 6kB due to less inlining of large parts that are always
context-dependent, and as a side effect, has increased the overall
performance by 1%.

4 years agoMINOR: task: move the allocated tasks counter to the per-thread struct
Willy Tarreau [Wed, 24 Feb 2021 16:03:30 +0000 (17:03 +0100)] 
MINOR: task: move the allocated tasks counter to the per-thread struct

The nb_tasks counter was still global and gets incremented and decremented
for each task_new()/task_free(), and was read in process_runnable_tasks().
But it's only used for stats reporting, so doing this this often is
pointless and expensive. Let's move it to the task_per_thread struct and
have the stats sum it when needed.

4 years agoMINOR: task: limit the remote thread wakeup to the global runqueue only
Willy Tarreau [Wed, 24 Feb 2021 15:44:51 +0000 (16:44 +0100)] 
MINOR: task: limit the remote thread wakeup to the global runqueue only

The test in __task_wakeup() to figure if the remote threads are sleeping
doesn't make sense outside of the global runqueue test, since there are
only two possibilities here: local runqueue or global runqueue, hence a
sleeping thread is another one and can only happen when sending to the
global run queue. Let's move the test inside the "if" block.

4 years agoCLEANUP: task: move the tree root detection from __task_wakeup() to task_wakeup()
Willy Tarreau [Wed, 24 Feb 2021 15:41:11 +0000 (16:41 +0100)] 
CLEANUP: task: move the tree root detection from __task_wakeup() to task_wakeup()

Historically we used to call __task_wakeup() with a known tree root but
this is not the case and the code has remained needlessly complicated
with the root calculation in task_wakeup() passed in argument to
__task_wakeup() which compares it again.

Let's get rid of this and just move the detection code there. This
eliminates some ifdefs and allows to simplify the test conditions quite
a bit.

4 years agoCLEANUP: tasks: use a less confusing name for task_list_size
Willy Tarreau [Wed, 24 Feb 2021 13:13:40 +0000 (14:13 +0100)] 
CLEANUP: tasks: use a less confusing name for task_list_size

This one is systematically misunderstood due to its unclear name. It
is in fact the number of tasks in the local tasklet list. Let's call
it "tasks_in_list" to remove some of the confusion.

4 years agoMINOR: tasks: do not maintain the rqueue_size counter anymore
Willy Tarreau [Wed, 24 Feb 2021 15:13:03 +0000 (16:13 +0100)] 
MINOR: tasks: do not maintain the rqueue_size counter anymore

This one is exclusively used as a boolean nowadays and is non-zero only
when the thread-local run queue is not empty. Better check the root tree's
pointer and avoid updating this counter all the time.

4 years agoMEDIUM: task: remove the tasks_run_queue counter and have one per thread
Willy Tarreau [Wed, 24 Feb 2021 14:10:07 +0000 (15:10 +0100)] 
MEDIUM: task: remove the tasks_run_queue counter and have one per thread

This counter is solely used for reporting in the stats and is the hottest
thread contention point to date. Moving it to the scheduler and having a
separate one for the global run queue dramatically improves the performance,
showing a 12% boost on the request rate on 16 threads!

In addition, the thread debugging output which used to rely on rqueue_size
was not totally accurate as it would only report task counts. Now we can
return the exact thread's run queue length.

It is also interesting to note that there are still a few other task/tasklet
counters in the scheduler that are not efficiently updated because some cover
a single area and others cover multiple areas. It looks like having a distinct
counter for each of the following entries would help and would keep the code
a bit cleaner:
  - global run queue (tree)
  - per-thread run queue (tree)
  - per-thread shared tasklets list
  - per-thread local lists

Maybe even splitting the shared tasklets lists between pure tasklets and
tasks instead of having the whole and tasks would simplify the code because
there remain a number of places where several counters have to be updated.

4 years agoBUILD: dns: avoid a build warning when threads are disabled (dss unused)
Willy Tarreau [Wed, 24 Feb 2021 16:38:46 +0000 (17:38 +0100)] 
BUILD: dns: avoid a build warning when threads are disabled (dss unused)

dns_session_release() only uses its struct dns_stream_server to access
the lock, so a warning is emitted when threads are disabled. Let's mark
it __maybe_unused.

4 years agoMEDIUM: streams: do not use the streams lock anymore
Willy Tarreau [Wed, 24 Feb 2021 12:46:12 +0000 (13:46 +0100)] 
MEDIUM: streams: do not use the streams lock anymore

The lock was still used exclusively to deal with the concurrency between
the "show sess" release handler and a stream_new() or stream_free() on
another thread. All other accesses made by "show sess" are already done
under thread isolation. The release handler only requires to unlink its
node when stopping in the middle of a dump (error, timeout etc). Let's
just isolate the thread to deal with this case so that it's compatible
with the dump conditions, and remove all remaining locking on the streams.

This effectively kills the streams lock. The measured gain here is around
1.6% with 4 threads (374krps -> 380k).

4 years agoMINOR: streams: use one list per stream instead of a global one
Willy Tarreau [Wed, 24 Feb 2021 09:37:01 +0000 (10:37 +0100)] 
MINOR: streams: use one list per stream instead of a global one

The global streams list is exclusively used for "show sess", to look up
a stream to shut down, and for the hard-stop. Having all of them in a
single list is extremely expensive in terms of locking when using threads,
with performance losses as high as 7% having been observed just due to
this.

This patch makes the list per-thread, since there's no need to have a
global one in this situation. All call places just iterate over all
threads. The most "invasive" changes was in "show sess" where the end
of list needs to go back to the beginning of next thread's list until
the last thread is seen. For now the lock was maintained to keep the
code auditable but a next commit should get rid of it.

The observed performance gain here with only 4 threads is already 7%
(350krps -> 374krps).

4 years agoMINOR: cli/streams: make "show sess" dump all streams till the new epoch
Willy Tarreau [Wed, 24 Feb 2021 10:53:17 +0000 (11:53 +0100)] 
MINOR: cli/streams: make "show sess" dump all streams till the new epoch

Instead of placing the current stream at the end of the stream list when
issuing a "show sess" on the CLI as was done in 2.2 with commit c6e7a1b8e
("MINOR: cli: make "show sess" stop at the last known session"), now we
compare the listed stream's epoch with the dumping stream's and stop on
more recent ones.

This way we're certain to always only dump known streams at the moment we
issue the dump command without having to modify the list. In theory we
could miss some streams if more than 2^31 "show sess" requests are issued
while an old stream remains present, but that's 68 years at 1 "show sess"
per second and it's unlikely we'll keep a process, let alone a stream, that
long.

It could be verified that the count of dumped streams still matches the
one before this change.

4 years agoMINOR: stream: add an "epoch" to figure which streams appeared when
Willy Tarreau [Wed, 24 Feb 2021 10:29:51 +0000 (11:29 +0100)] 
MINOR: stream: add an "epoch" to figure which streams appeared when

The "show sess" CLI command currently lists all streams and needs to
stop at a given position to avoid dumping forever. Since 2.2 with
commit c6e7a1b8e ("MINOR: cli: make "show sess" stop at the last known
session"), a hack consists in unlinking the stream running the applet
and linking it again at the current end of the list, in order to serve
as a delimiter. But this forces the stream list to be global, which
affects scalability.

This patch introduces an epoch, which is a global 32-bit counter that
is incremented by the "show sess" command, and which is copied by newly
created streams. This way any stream can know whether any other one is
newer or older than itself.

For now it's only stored and not exploited.

4 years agoBUG/MINOR: proxy: wake up all threads when sending the hard-stop signal
Willy Tarreau [Wed, 24 Feb 2021 10:13:59 +0000 (11:13 +0100)] 
BUG/MINOR: proxy: wake up all threads when sending the hard-stop signal

The hard-stop event didn't wake threads up. In the past it wasn't an issue
as the poll timeout was limited to 1 second, but since commit 4f59d3861
("MINOR: time: increase the minimum wakeup interval to 60s") it has become
a problem because old processes can remain live for up to one minute after
the hard-stop-after delay. Let's just wake them up.

This may be backported to older releases, though before 2.4 the extra
delay was only one second.

4 years agoBUG/MEDIUM: cli/shutdown sessions: make it thread-safe
Willy Tarreau [Wed, 24 Feb 2021 10:11:06 +0000 (11:11 +0100)] 
BUG/MEDIUM: cli/shutdown sessions: make it thread-safe

There's no locking around the lookup of a stream nor its shutdown
when issuing "shutdown sessions" over the CLI so the risk of crashing
the process is particularly high.

Let's use a thread_isolate() there which is suitable for this task, and
there are not that many alternatives.

This must be backported to 1.8.

4 years agoBUG/MEDIUM: proxy: use thread-safe stream killing on hard-stop
Willy Tarreau [Wed, 24 Feb 2021 10:08:56 +0000 (11:08 +0100)] 
BUG/MEDIUM: proxy: use thread-safe stream killing on hard-stop

When setting hard-stop-after, hard_stop() is called at the end to kill
last pending streams. Unfortunately there's no locking there while
walking over the streams list nor when shutting them down, so it's
very likely that some old processes have been crashing or gone wild
due to this. Let's use a thread_isolate() call for this as we don't
have much other choice (and it happens once in the process' life,
that's OK).

This must be backported to 1.8.

4 years agoDOC: muxes: add a diagram of the exchanges between muxes and outer world
Willy Tarreau [Wed, 24 Feb 2021 08:07:52 +0000 (09:07 +0100)] 
DOC: muxes: add a diagram of the exchanges between muxes and outer world

Since the muxes API is far from being obvious, let's show a stream being
forwarded between two sides through muxes with their buffers and the
transport layers. The diagram is provided in .fig, .svg, .png, and .pdf.

4 years agoCLEANUP: vars: make smp_fetch_var() to reuse vars_get_by_desc()
Dragan Dosen [Mon, 22 Feb 2021 16:39:02 +0000 (17:39 +0100)] 
CLEANUP: vars: make smp_fetch_var() to reuse vars_get_by_desc()

They both do the same thing, so let's remove unneeded code duplication.

4 years agoBUG/MEDIUM: vars: make functions vars_get_by_{name,desc} thread-safe
Dragan Dosen [Mon, 22 Feb 2021 16:20:01 +0000 (17:20 +0100)] 
BUG/MEDIUM: vars: make functions vars_get_by_{name,desc} thread-safe

This patch adds a lock to functions vars_get_by_name() and
vars_get_by_desc() to protect accesses to the list of variables.

After the variable is fetched, a sample data is duplicated by using
smp_dup() because the variable may be modified by another thread.

This should be backported to all versions supporting vars along with
"BUG/MINOR: sample: secure convs that accept base64 string and var name
as args" which this patch depends on.