A number of tests are now performed in low-level buffer management
functions to verify that we're not appending data to a full buffer
for example, or that the buffer passed in argument is consistent in
that its data don't outweigh its size. The few functions that already
involve memcpy() or memmove() instead got a BUG_ON() that will always
be enabled, since the overhead remains minimalist.
The buffer ring management functions br_* were all stuffed with BUG_ON()
statements that never triggered and that are on some fast paths (e.g. in
mux_h2). Let's turn them to BUG_ON_HOT() instead.
Two new BUG_ON variants, BUG_ON_HOT() and CHECK_IF_HOT() are introduced
to debug hot paths (such as low-level API functions). These ones must
not be enabled by default as they would significantly affect performance
but they may be enabled by setting DEBUG_STRICT to a value above 1. In
this case, DEBUG_STRICT_ACTION is mostly respected with a small change,
which is that the no_crash variant of BUG_ON() isn't turned to a regular
warning but to a one-time warning so as not to spam with warnings in a
hot path. It is for this reason that there is no WARN_ON_HOT().
We used to have DEBUG_STRICT_NOCRASH to disable crashes on BUG_ON().
Now we have other levels (WARN_ON(), CHECK_IF()) so we need something
finer-grained.
This patch introduces DEBUG_STRICT_ACTION which takes an integer value.
0 disables crashes and is the equivalent of DEBUG_STRICT_NOCRASH. 1 is
the default and only enables crashes on BUG_ON(). 2 also enables crashes
on WARN_ON(), and 3 also enables warnings on CHECK_IF(), and is suited
to developers and CI.
Now we'll explicitly mention if the test was a bug/warn/check, and
"FATAL" is only displayed when the process crashes. The non-crashing
BUG_ON() also suggests to report to developers.
The only reason for warning once is to check if a condition really
happens. Let's use a term that better translates the intent, that's
important when reading the code.
Simplify the data manipulation of STREAM frames on TX. Only stream data
and len field are used to generate a valid STREAM frames from the
buffer. Do not use the offset field, which required that a single buffer
instance should be shared for every frames on a single stream.
This one will maintain a static counter per call place and will only
emit the warning on the first call. It may be used to invite users to
report an unexpected event without spamming them with messages.
This is the same as BUG_ON() except that it never crashes and only emits
a warning and a backtrace, inviting users to report the problem. This will
be usable for non-fatal issues that should not happen and need to be fixed.
This way the BUG_ON() when using DEBUG_STRICT_NOCRASH is effectively an
equivalent of WARN_ON().
The purpose is to make the program die at this point, so let's help the
compiler optimise the code (especially in sensitive areas) by telling it
that ABORT_NOW() does not return. This reduces the overall code size by
~0.5%.
The BUG_ON() macro handling is complicated because it relies on a
conditional CRASH_NOW() macro whose definition depends on DEBUG_STRICT
and DEBUG_STRICT_NOCRASH. Let's rethink the whole thing differently,
and instead make the underlying _BUG_ON() macro take a crash argument
to decide whether to crash or not, as well as a prefix and a suffix for
the message, that will allow to distinguish between variants. Now the
suffix is set to a message explaining we don't crash when needed.
This also allows to get rid of the CRASH_NOW() macro and to define
much simpler new macros.
The functions needed to manipulate the "tainted" flags were located in
too high a level to be callable from the lower code layers. Let's move
them to bug.h.
Since recent changes related to the conn-stream/stream-interface
refactoring, GCC reports potential null pointer dereferences when we get the
appctx, the stream or the stream-interface from the conn-strem. Of course,
depending on the time, these entities may be null. But at many places, we
know they are defined and it is safe to get them without any check. Thus, we
use ALREADY_CHECKED() macro to silent these warnings.
Note that the refactoring is unfinished, so it is not a real issue for now.
cs_detach_app() function is added to detach an app from a conn-stream. And
now, both cs_detach_app() and cs_detach_endp() release the conn-stream when
both the app and the endpoint are detached.
Thanks to all previous changes, it is now possible to move the
stream-interface into the conn-stream. To do so, some SI functions are
removed and their conn-stream counterparts are added. In addition, the
conn-stream is now responsible to create and release the
stream-interface. While the stream-interfaces were inlined in the stream
structure, there is now a pointer in the conn-stream. stream-interfaces are
now dynamically allocated. Thus a dedicated pool is added. It is a temporary
change because, at the end, the stream-interface structure will most
probably disappear.
Because cs_detach() is releated to the endpoint only, the function is
renamed. The main purpose of this patch is to be able to add a function to
detach the conn-stream from the application.
To be able to move the stream-interface from the stream to the conn-stream,
all access to the SI is done via the conn-stream. This patch is limited to
the stream part.
To be able to move the stream-interface from the stream to the conn-stream,
all access to the SI is done via the conn-stream. This patch is limited to
the stream-interface part.
frontend and backend conn-streams are now directly accesible from the
stream. This way, and with some other changes, it will be possible to remove
the stream-interfaces from the stream structure.
In the same way the conn-stream has a pointer to the stream endpoint , this
patch adds a pointer to the application entity in the conn-stream
structure. For now, it is a stream or a health-check. It is mandatory to
merge the stream-interface with the conn-stream.
Because appctx is now an endpoint of the conn-stream, there is no reason to
still have the stream-interface as appctx owner. Thus, the conn-stream is
now the appctx owner.
Thanks to previous changes, it is now possible to set an appctx as endpoint
for a conn-stream. This means the appctx is no longer linked to the
stream-interface but to the conn-stream. Thus, a pointer to the conn-stream
is explicitly stored in the stream-interface. The endpoint (connection or
appctx) can be retrieved via the conn-stream.
To be able to handle applets as a conn-stream endpoint, we must be prepared
to handle different types of endpoints. First of all, the conn-strream's
connection must no longer be used directly.
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().
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.
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 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.
