The backend conn-stream is no longer released on connection retry. This
means the conn-stream is detached from the underlying connection but not
released. Thus, during connection retries, the stream has always an
allocated conn-stream with no connection. All previous changes were made to
make this possible.
Note that .attach() mux callback function was changed to get the conn-stream
as argument. The muxes are no longer responsible to create the conn-stream
when a server connection is attached to a stream.
si_attach_conn() function should be used to attach a connection to a
stream-interface. It created a conn-stream if necessary. This function is
mandatory to be able to keep the backend conn-stream during connection
retries.
si_reset_endpoint() function may be used to reset the SI's endpoint without
releasing the conn-stream if the endpoint is a connection. If the endpoint
is an appctx, it is released. This change is mandatory to merge the SI and
the CS and keep the backend conn-stream attached to the stream during
connection retries.
cs_detach() function is added to detach a conn-stream from the underlying
connection. This part will evovle to handle applets too. Concretely,
cs_destroy() is split to detach the conn-stream from its endpoint, via
cs_detach(), and then, the conn-stream is released, via cs_free().
In the same way the previous commit, when a stream is created, the appctx
case is now handled before the conn-stream one. The purpose of this change
is to limit bugs during the SI/CS refactoring.
The conn-stream will progressively replace the stream-interface. Thus, a
stream will have to allocate the backend conn-stream during its
creation. This means it will be possible to have a conn-stream with no
connection. To prepare this change, we test the conn-stream's connection
when we retrieve it.
Stream-interfaces will be moved in the conn-stream and the appctx will be
moved at the same level than the muxes. Idea is to merge the
stream-interface and the conn-stream and have a better symmetry between the
muxes and the applets. To limit bugs during this refactoring, when the SI
endpoint is released, the appctx case is handled first.
The pools currently have plenty of options (and some usefull ones were
even lost with the modern design), but most of them could be categorized
along a few use cases, namely, performance, reliability, debuggability.
This document explores various ways to try to combine them and their
effect in a less complex way for the long term.
This enables DEBUG_MEMORY_POOLS and DEBUG_POOL_INTEGRITY so that by
default the tests run under stricter checks, which are likely to
catch more bugs. Note that these ones are permanently used in prod
on haproxy.org.
The first one will enable all currently deployed BUG_ON() checks. These
ones are safe from a performance perspective and from a reliability
perspective. New ones may be added later with different categories
(hot path, detection of uncertain events, etc).
DEBUG_MEMORY_POOLS enables the "tag" pool debugging option by default,
so that pools may be better traced in dumps. This one alone results in
almost imperceptible performance difference, and 8 extra bytes per
allocated object.
Both options are safe for production use (they're among those enabled
all the time on haproxy.org) and allow to produce much more trustable
bug reports which should save a few round trips with the reporters.
The 9 currently available debugging options may now be checked, set, or
cleared using -dM. The directive now takes a comma-delimited list of
options after the optional poisonning byte. With "help", the list of
available options is displayed with a short help and their current
status.
The management doc was updated.
New function pool_parse_debugging() is now dedicated to parsing options
of -dM. For now it only handles the optional memory poisonning byte, but
the function may already return an informative message to be printed for
help, a warning or an error. This way we'll reuse it for the settings
that will be needed for configurable debugging options.
The argument parser runs too late, we'll soon need it before creating
pools, hence just after init_early(). No visible change is expected but
this part is sensitive enough to be placed into its own commit for easier
bisection later if needed.
The cmdline argument parsing was performed quite late, which prevents
from retrieving elements that can be used to initialize the pools and
certain sensitive areas. The goal is to improve this by parsing command
line arguments right after the early init stage. This is possible
because the cmdline parser already does very little beyond retrieving
config elements that are used later.
Doing so requires to move the parser code to a separate function and
to externalize a few variables out of the function as they're used
later in the boot process, in the original function.
This patch creates init_args() but doesn't move it upfront yet, it's
still executed just before init(), which essentially corresponds to
what was done before (only the trash buffers, ACLs and Lua were
initialized earlier and are not needed for this).
The rest is not modified and as expected no change is observed.
Note that the diff doesn't to justice to the change as it makes it
look like the early init() code was moved to a new function after
the function was renamed, while in fact it's clearly the parser
itself which moved.
There are some delicate chicken-and-egg situations in the initialization
code, because the init() function currently does way too much (it goes
as far as parsing the config) and due to this it must be started very
late. But it's also in charge of initializing a number of variables that
are needed in early boot (e.g. hostname/pid for error reporting, or
entropy for random generators).
This patch carefully extracts all the early code that depends on
absolutely nothing, and places it immediately after the STG_LOCK init
stage. The only possible failures at this stage are only allocation
errors and they continue to provoke an immediate exit().
Some environment variables, hostname, date, pid etc are retrieved at
this stage. The program's arguments are also copied there since they're
needed to be kept intact for the master process.
The STG_REGISTER init level is used to register known keywords and
protocol stacks. It must be called earlier because some of the init
code already relies on it to be known. For example, "haproxy -vv"
for now is constrained to start very late only because of this.
This patch moves it between STG_LOCK and STG_ALLOC, which is fine as
it's used for static registration.
Now -dM will set POOL_DBG_POISON for consistency with the rest of the
pool debugging options. As such now we only check for the new flag,
which allows the default value to be preset.
This option used to allow to store a marker at the end of the area, which
was used as a canary and detection against wrong freeing while the object
is used, and as a pointer to the last pool_free() caller when back in cache.
Now that we can compute the offsets at runtime, let's check it at run time
and continue the code simplification.
This option used to allow to store a pointer to the caller of the last
pool_alloc() or pool_free() at the end of the area. Now that we can
compute the offsets at runtime, let's check it at run time and continue
the code simplification. In __pool_alloc() we now always calculate the
return address (which is quite cheap), and the POOL_DEBUG_TRACE_CALLER()
calls are conditionned on the status of debugging option.
This macro is build-time dependent and is almost unused, yet where it
cannot easily be avoided. Now that we store the distinction between
pool->size and pool->alloc_sz, we don't need to maintain it and we
can instead compute it on the fly when creating a pool. This is what
this patch does. The variables are for now pretty static, but this is
sufficient to kill the macro and will allow to set them more dynamically.
The allocated size is the visible size plus the extra storage. Since
for now we can store up to two extra elements (mark and tracer), it's
convenient because now we know that the mark is always stored at
->size, and the tracer is always before ->alloc_sz.
Like previous patches, this replaces the build-time code paths that were
conditionned by CONFIG_HAP_POOLS with runtime paths conditionned by
!POOL_DBG_NO_CACHE. One trivial test had to be added in the hot path in
__pool_alloc() to refrain from calling pool_get_from_cache(), and another
one in __pool_free() to avoid calling pool_put_to_cache().
All cache-specific functions were instrumented with a BUG_ON() to make
sure we never call them with cache disabled. Additionally the cache[]
array was not initialized (remains NULL) so that we can later drop it
if not needed. It's particularly huge and should be turned to dynamic
with a pointer to a per-thread area where all the objects are located.
This will solve the memory usage issue and will improve locality, or
even help better deal with NUMA machines once each thread uses its own
arena.
There were very few functions left that were specific to global pools,
and even the checks they used to participate to are not directly on the
most critical path so they can suffer an extra "if".
What's done now is that pool_releasable() always returns 0 when global
pools are disabled (like the one before) so that pool_evict_last_items()
never tries to place evicted objects there. As such there will never be
any object in the free list. However pool_refill_local_from_shared() is
bypassed when global pools are disabled so that we even avoid the atomic
loads from this function.
The default global setting is still adjusted based on the original
CONFIG_NO_GLOBAL_POOLS that is set depending on threads and the allocator.
The global executable only grew by 1.1kB by keeping this code enabled,
and the code is simplified and will later support runtime options.
The test to decide whether or not to enforce integrity checks on cached
objects is now enabled at runtime and conditionned by this new debugging
flag. While previously it was not a concern to inflate the code size by
keeping the two functions static, they were moved to pool.c to limit the
impact. In pool_get_from_cache(), the fast code path remains fast by
having both flags tested at once to open a slower branch when either
POOL_DBG_COLD_FIRST or POOL_DBG_INTEGRITY are set.
When enabling pools integrity checks, we usually prefer to allocate cold
objects first in order to maximize the time the objects spend in the
cache. In order to make this configurable at runtime, let's introduce
a new debugging flag to control this allocation order. It is currently
preset by the DEBUG_POOL_INTEGRITY build-time setting.
This test used to appear at a single location in create_pool() to
enable a check on the pool name or unconditionally merge similarly
sized pools.
This patch introduces POOL_DBG_DONT_MERGE and conditions the test on
this new runtime flag, that is preset according to the aforementioned
debugging option.
The fail-alloc test used to be enabled/disabled at build time using
the DEBUG_FAIL_ALLOC macro, but it happens that the cost of the test
is quite cheap and that it can be enabled as one of the pool_debugging
options.
This patch thus introduces the first POOL_DBG_FAIL_ALLOC option, whose
default value depends on DEBUG_FAIL_ALLOC. The mem_should_fail() function
is now always built, but it was made static since it's never used outside.
This read-mostly variable will be used at runtime to enable/disable
certain pool-debugging features and will be set by the command-line
parser. A future option -dP will take a number of debugging features
as arguments to configure this variable's contents.
The poisonning performed on pool_free() used to help a little bit with
use-after-free detection, but usually did more harm than good in that
it was never possible to perform post-mortem analysis on released
objects once poisonning was enabled on allocation. Now that there is
a dedicated DEBUG_POOL_INTEGRITY, let's get rid of this annoyance
which is not even documented in the management manual.
There's no point keeping the vars_init_head() call in init() when we
already have a vars_init() registered at the right time to do that,
and it complexifies the boot sequence, so let's move it there.
Let's not use a trash there anymore. The function is called at very
early boot (for "haproxy -vv"), and the need for a trash prevents the
arguments from being parsed earlier. Moreover, the function only uses
a FILE* on output with fprintf(), so there's not even any benefit in
using chunk_printf() on an intermediary variable, emitting the output
directly is both clearer and safer.
REGISTER is meant to only assemble static lists, not to initialize
code that may depend on some elements possibly initialized at this
level. For example the init code currently looks up transport protocols
such as XPRT_RAW and XPRT_SSL which ought to be themselves registered
from at REGISTER stage, and which currently work only because they're
still registered directly from a constructor. INIT is perfectly suited
for this level.
Add the ability to set a "server timeout" on the httpclient with either
the httpclient_set_timeout() API or the timeout argument in a request.
Issue #1470.
In process_stream(), we force the response buffer allocation before any
processing to be able to return an error message. It is important because,
when an error is triggered, the stream is immediately closed. Thus we cannot
wait for the response buffer allocation.
When the allocation fails, the stream analysis is stopped and the expiration
date of the stream's task is updated before exiting process_stream(). But if
the stream was woken up because of a connection or an analysis timeout, the
expiration date remains blocked in the past. This means the stream is woken
up in loop as long as the response buffer is not properly allocated.
Alone, this behavior is already a bug. But because the mechanism to handle
buffer allocation failures is totally broken since a while, this bug becomes
more problematic. Because, most of time, the watchdog will kill HAProxy in
this case because it will detect a spinning loop.
To fix it, at least temporarily, an allocation failure at this stage is now
reported as an error and the processing is aborted. It's not satisfying but
it is better than nothing. If the buffers allocation mechanism is
refactored, this part will be reviewed.
This patch must be backported, probably as far as 2.0. It may be perceived
as a regression, but the actual behavior is probably even worse. And
because it was not reported, it is probably not a common situation.
This one started to randomly fail on me again and I could figure the
problem. It mixes one checked server with one unchecked on in each
backend, and tries to make sure that each checked server receives
exactly one request. But that doesn't work and is entirely time-
dependent because if the check starts before the client, a pure
TCP check is sent to the server, which sees an aborted connection
and makes the whole check fail.
Here what is done is that we make sure that only the second server
and not the first one is checked. The traffic is delivered to all
first servers, and each HTTP server must always receive a valid HTTP
request. In parallel, checks must not fail as they're delivered to
dummy servers. The check doesn't fail anymore, even when started on
a single thread at nice +5 while 8 processes are fighting on the same
core to inject HTTP traffic at 25 Gbps, which used to systematically
make it fail previously.
Since it took more than one hour to fix the "expect" line for the stats
output, I did it using a small script that I pasted into the vtc file
in case it's needed later. The relevance of this test is questionable
once its complexity is factored in. Let's keep it as long as it works
without too much effort.
This function was renderred obsolete by commit a0b5831ee ("MEDIUM: pools:
centralize cache eviction in a common function") which replaced its last
call inside the loop with a single call out of the loop to pool_releasable()
as introduced by commit 91a8e28f9 ("MINOR: pool: add a function to estimate
how many may be released at once"). Let's remove it before it becomes wrong
and used again.
The mem_poison_byte, mem_fail_rate, using_default_allocator and the
pools list are all only set once at boot time and never changed later,
while they're heavily used at run time. Let's optimize their usage from
all threads by marking them read-mostly so that them reside in a shared
cache line.
The recent changes was not complete.
d1c76f24fd
MINOR: quic: do not modify offset node if quic_rx_strm_frm in tree
The frame length and data pointer should incremented after the data
copy. A BUG_ON statement has been added to detect an incorrect decrement
operaiton.
Some variables were only checked via BUG_ON macro. If compiling without
DEBUG_STRICT, this instruction is a noop. Fix this by using an explicit
condition + ABORT_NOW.
This should fix the github issue #1549.
qc_rx_strm_frm_cpy is unsafe because it updates the offset field of the
frame. This is not safe as the frame is inserted in the tree when
calling this function and offset serves as the key node.
To fix this, the API is modified so that qc_rx_strm_frm_cpy does not
update the frame parameter. The caller is responsible to update
offset/length in case of a partial copy.
The impact of this bug is not known. It can only happened with received
STREAM frames out-of-order. This might be triggered with large h3 POST
requests.
In si_cs_recv(), the mux must never set CS_FL_WANT_ROOM flag on the
conn-stream if the input buffer is empty and nothing was copied. It is
important because, there is nothing the app layer can do in this case to
make some room. If this happens, this will most probably lead to a ping-pong
loop between the mux and the stream.
With this BUG_ON(), it will be easier to spot such bugs.
If a parsing error is detected and the corresponding HTX flag is set
(HTX_FL_PARSING_ERROR), we must be sure to always report it to the app
layer. It is especially important when the error occurs during the response
parsing, on the server side. In this case, the RX buffer contains an empty
HTX message to carry the flag. And it remains in this state till the info is
reported to the app layer. This must be done otherwise, on the conn-stream,
the CS_FL_ERR_PENDING flag cannot be switched to CS_FL_ERROR and the
CS_FL_WANT_ROOM flag is always set when h2_rcv_buf() is called. The result
is a ping-pong loop between the mux and the stream.
Note that this patch fixes a bug. But it also reveals a design issue. The
error must not be reported at the HTX level. The error is already carried by
the conn-stream. There is no reason to duplicate it. In addition, it is
errorprone to have an empty HTX message only to report the error to the app
layer.
This patch should fix the issue #1561. It must be backported as far as 2.0
but the bug only affects HAProxy >= 2.4.
