There's no point using ha_random32() which is heavy and uses shared
variables to calculate a random timer when we have statistical_prng()
which does the same and was made exactly for this.
In the past we've seen "show servers state" dump some internal bits for
the check states, that were causing regtests to fail. The relevant bits
have been added to the doc to fix the public API and make sure they do
not change by accident, but the output doesn't take care of masking the
undesired ones, causing regtests (and possibly user programs) to fail
when new bits are added. Let's add the mask for the only documented ones
(0x0F for check and 0x1F for agent respectively).
This could be backported wherever the server state is present, though
there's a tiny risk that some undocumented bits might have already
leaked to some user scripts, so it might be wise to wait a bit before
doing that or even not to backport too far.
In h1_process_demux(), aborts for incomplete messages were not properly
handled. It was not an issue because the abort was detected later in
h1_process(). But it will be an issue to perform the aborts refoctoring.
First, when a read0 was detected, the SE_FL_EOI flag was set for messages in
DONE or TUNNEL state or for messages without known length (so responses in
close mode). The last statement is not accurate. The message must also be in
DATA state. Otherwise, SE_FL_EOI flag may be set on incomplete message.
Then, an error was reported, via SE_FL_ERROR flag, only when an incomplete
message was detected on the payload parsing. It must also be reported if
headers are incomplete. Here again, the error is detected later for now. But
it could be an issue later.
There is no reason to backport this patch.
There is no error handling when we read or write on a pipe. There error is
caught later, in the mux I/O handler. But there is no reason to not do so
here.
There is no reason to backport it because no issue was reported for now
because of this "bug". In all cases, it must be evaluated first.
The new functions fconn_show_flags() and fstrm_show_flags() decode the flags
state into a string, and are used by dev/flags:
$ /dev/flags/flags fconn 0x3100
fconn->flags = FCGI_CF_GET_VALUES | FCGI_CF_KEEP_CONN | FCGI_CF_MPXS_CONNS
./dev/flags/flags fstrm 0x3300
fstrm->flags = FCGI_SF_WANT_SHUTW | FCGI_SF_WANT_SHUTR | FCGI_SF_OUTGOING_DATA | FCGI_SF_BEGIN_SENT
The same was performed for the H2 and H1 multiplexers. FCGI connection and
stream flags are moved in a dedicated header file. It will be mainly used to
be able to decode mux-fcgi flags from the flags utility.
In this patch, we move the flags and enums to mux_fcgi-t.h, as well as the
two state decoding inline functions.
Compilation is broken with DEBUG_THREAD since the following patch
76642223f0
MEDIUM: stick-table: switch the table lock to rwlock
Fix this by updating a legacy HA_SPIN_INIT() to HA_RWLOCK_INIT().
No backport needed unless the mentionned patch is backported.
We don't need to call stktable_requeue_exp() with the table's lock
held anymore, so let's move it out. It should slightly reduce the
contention on the write lock, though it is now already quite low.
We're generalizing the change performed in previous commit "MEDIUM:
stick-table: requeue the expiration task out of the exclusive lock"
to stktable_requeue_exp() so that it can also be used by callers of
__stktable_store(). At the moment there's still no visible change
since it's still called under the write lock. However, the previous
code in stitable_touch_with_exp() was updated to use this function.
__staktable_store() performs two distinct things, one is to insert a key
and the other one is to requeue the task's expiration date. Since the
latter might be done without a lock, let's first split the function in
two halves. For now this has no impact.
With 48 threads, a heavily loaded table with plenty of trackers and
rules and a short expiration timer of 10ms saturates the CPU at 232k
rps. By carefully using atomic ops we can make sure that t->exp_next
and t->task->expire converge to the earliest next expiration date and
that all of this can be performed under atomic ops without any lock.
That's what this patch is doing in stktable_touch_with_exp(). This is
sufficient to double the performance and reach 470k rps.
It's worth noting that __stktable_store() uses a mix of eb32_insert()
and task_queue, and that the second part of it could possibly benefit
from this, even though sometimes it's called under a lock that was
already held.
On a 24-core machine having some "stick-store response" rules, a lot of
time is spent in the write lock in stktable_set_entry(). Let's apply the
same mechanism as for the stktable_get_entry() consisting in looking up
the value under the read lock and upgrading it to a write lock only to
perform modifications. Here we even have the luxury of upgrading the
lock since there are no alloc/free in the path. All this increases the
performance by 40% (from 363k to 510k rps).
We don't need to be protected by the table's lock when touching t->current
if we do it using atomics, and that's great because it allows us to have
a cleaner stksess_new() that doesn't require a lock either, and to avoid
manipulating pools under a lock.
That's another 1% performance gain from 2.07 to 2.10M req/s under 48
threads.
On a 24-core machine doing lots of track-sc, it was found that the lock
in stktable_get_entry() was responsible for 25% of the CPU alone. It's
sad because most of its job is to protect the table during the lookup.
Here we're taking a slightly different approach: the lock is first taken
for reads during the lookup, and only in case of failure we switch it for
a write lock. We don't even perform an upgrade here since an allocation
is needed between the two, it would be wasted to do it under the lock,
and is generally not a good idea, so better release the read lock and
try again.
Here the performance under 48 threads with 3 trackers on the same table
jumped from 455k to 2.07M, or 4.55x! Note that the same approach should
be possible for stktable_set_entry().
These functions do not modify anything in the the table except the refcount
on success. Let's just lock the table for shared accesses and make use of
atomic ops to update the refcount. This brings a nice gain from 425k to
455k under 48 threads (7%), but some contention remains on the exclusive
locks in other parts.
Note that the refcount continues to be updated under the lock because it's
not yet certain whether there are races between it and some of the exclusive
lock on the table. The difference is marginal and we prefer to stay on the
safe side for now.
In __stktable_get_entry() now we're planning for the possibility that the
call to __stktable_store() doesn't add the newly allocated entry and instead
finds a previously inserted one. At the moment this doesn't exist because
the lookup + insert passes are made under the same lock. But it will soon
change.
This function is used to create an entry in the table. But it doesn't
consider the possibility that the entry already exists, because right
now it's only called in situations where it was verified under a lock
that it doesn't exist. Since we'll soon need to break that assumption
we need it to verify that the requested entry was added and to return
a pointer to the one in the tree so that the caller can detect any
possible conflict. At the moment this is not used.
stream_store_counters() calls stksess_kill_if_expired() for each active
counter. And this one takes an exclusive lock on the table before
checking if it has any work to do (hint: it almost never has since it
only wants to delete expired entries). However a lock is still neeed for
now to protect the ref_cnt, but we can do it atomically under the read
lock.
Let's change the mechanism. Now what we do is to check out of the lock
if the entry is expired. If it is, we take the write lock, expire it,
and decrement the refcount. Otherwise we just decrement the refcount
under a read lock. With this change alone, the config based on 3
trackers without the previous patches saw a 2.6x improvement, but here
it doesn't yet change anything because some heavy contention remains
on the lookup part.
As previously mentioned, this function currently holds an exclusive lock
on the table during all the time it take to check if the entry needs to
be updated and synchronized with peers. The reality is that many setups
do not use peers and that on highly loaded setups, the same entries are
hammered all the time so the key's expiration doesn't change between a
number of consecutive accesses.
With this patch we take a different approach. The function starts
without taking the lock, and will take it only if needed, keeping track
of it. This way we can avoid it most of the time, or even entirely.
Finally if the decrefcnt argument requires that the refcount is
decremented, we either do it using a non-atomic op if the table was
locked (since no other entry may touch it) or via an atomic under the
read lock only.
With this change alone, a 48-thread test with 3 trackers increased
from 193k req/s to 425k req/s, which is a 2.2x factor.
Taking the write lock prior to entering that function is a problem
because this function is full of conditions that most of the time can
lead to eliminating the lock.
This commit first moves the write lock inside the function and passes
the extra argument required to implement stktable_touch_remote() and
stktable_touch_local(). It also renames the function to remove the
underscores since there's no other variant and it's exported under
this name (probably an old rename that was not propagated). The code
was stressed under 48 threads using 3 trackers on the same table. It
already shows a tiny 3% improvement from 187k to 193k rps.
At plenty of places we decrement ts->ref_cnt under the write lock
because it's held. We don't technically need it to be done that way
if there's contention and an atomic could suffice. However until all
places are turned to atomic, we at least need to do that under a
read lock for now, so that we don't mix atomic and non-atomic uses.
Regardless it already brings ~1.5% req rate improvement with 3 trackers
on the same table under 48 threads at 184k->187k rps.
Right now a spinlock is used, but most accesses are for reads, so let's
switch the lock to an rwlock and switch all accesses to exclusive locks
for now. There should be no visible difference at this point.
Right now when dealing with freq_ctr updates, we're using the process-
wide monotinic time, and accessing it is expensive since every thread
needs to update it, so this adds some contention. However we don't need
it all the time, the thread's local time is most of the time strictly
equal to the global time, and may be off by one millisecond when the
global time is switched to the next one by another thread, and in this
case we don't want to use the local time because it would risk to cause
a rotation of the counter. But that's precisely the condition we're
already relying on for the slow path!
What this patch does is to add a check for the period against the
local time prior to anything else, and immediately return after
updating the counter if still within the period, otherwise fall back
to the existing code. Given that the function starts to inflate a bit,
it was split between s very short inline part that does the hot path,
and the slower fallback that's in a cold function. It was measured that
on a 24-CPU machine it was called ~0.003% of the time.
The resulting improvement sits between 2 and 3% at 500k req/s tracking
an http_req_rate counter.
The new macro PLOCK_DISABLE_EBO may be defined to disable exponential
backoff. This can be useful to more easily spot functions that cause
contention. In this case the CPU will be spent inside the functions
themselves instead of the pl_wait_unlock_{long,int}() functions, making
them easier to spot using "perf top" even if that causes a significant
degradation of the thread scalability.
Since commit 03cdf55e6 ("MINOR: stream: Stickiness server lookup by name.")
in 2.0-dev6, server names may be used instead of their IDs, in order to
perform stickiness. However the commit above may end up trying to insert
an empty server name in the dictionary when the server is an applet
instead, resulting in an immediate segfault. This is typically what
happens when a "stick-store" rule is present in a backend featuring a
"stats" directive. As there doesn't seem to be an easy way around it,
it seems to imply that "stick-store" is not much used anymore.
The solution here is to only try to insert non-null keys into the
dictionary. The patch moves the check of the key type before the
first lock so that the test on the key can be performed under the lock
instead of locking twice (the patch is more readable with diff -b).
Note that before 2.4, there's no <key> variable there as it was
introduced by commit 92149f9a8 ("MEDIUM: stick-tables: Add srvkey
option to stick-table"), but the __objt_server(s->target)->id still
needs to be tested.
This needs to be backported as far as 2.0.
In cd341d531, I added a FIXME comment because I noticed a
lua_pushvalue with 0 index, whereas lua doc states that 0 is never
an acceptable index.
After reviewing and testing the hlua_applet_http_send_response() code,
it turns out that this pushvalue is not even needed.
So it's safer to remove it as it could lead to undefined
behavior (since it is not supported by Lua API) and it grows lua stack
by 1 for no reason.
No backport needed.
An example given for tcp-request content rule with lua
was missing 'if' keyword. Using it "as is" makes haproxy unhappy.
The example was introduced with 579d83b05.
So it may be backported as far as 1.6, but it is a really minor typo.
In hlua code, we mark every function that may longjump using
MAY_LJMP macro so it's easier to identify them by reading the code.
However, some luaL_checktypes() were performed without the MAY_LJMP.
According to lua doc:
Functions called luaL_check* always raise an error if
the check is not satisfied.
-> Adding the missing MAY_LJMP for those luaLchecktypes() calls.
No backport needed.
Move code which activates IP_PKTINFO socket option (or affiliated
options) from sock_inet_bind_receiver() to quic_bind_listener()
function. This change is useful for two reasons :
* first, and the most important one : this activates IP_PKTINFO only for
QUIC receivers. The previous version impacted all datagram receivers,
used for example by log-forwarder. This should reduce memory usage for
these datagram sockets which do not need this option.
* second, USE_QUIC preprocessor statements are removed from
src/sock_inet.c which clean up the code.
IP_PKTINFO was introduced recently by the following patch :
97ecc7a8ea (quic-dev/qns)
MEDIUM: quic: retrieve frontend destination address
For the moment, this does not impact any stable release. However, as
previous patch is scheduled for 2.6 backporting, the current change must
also be backported to the same versions.
The tx_qrings[] and tx_qring_list in the receiver are not used
anymore since commit f2476053f ("MINOR: quic: replace custom buf on Tx
by default struct buffer"), the only place where they're referenced
was in quic_alloc_tx_rings_listener(), which by the way implies that
these were not even freed on exit.
Let's just remove them. This should be backported to 2.6 since the
commit above also was.
Retrieve the frontend destination address for a QUIC connection. This
address is retrieve from the first received datagram and then stored in
the associated quic-conn.
This feature relies on IP_PKTINFO or affiliated flags support on the
socket. This flag is set for each QUIC listeners in
sock_inet_bind_receiver(). To retrieve the destination address,
recvfrom() has been replaced by recvmsg() syscall. This operation and
parsing of msghdr structure has been extracted in a wrapper quic_recv().
This change is useful to finalize the implementation of 'dst' sample
fetch. As such, quic_sock_get_dst() has been edited to return local
address from the quic-conn. As a best effort, if local address is not
available due to kernel non-support of IP_PKTINFO, address of the
listener is returned instead.
This should be backported up to 2.6.
Inspect return code of qc_send_mux(). If quic-conn layer reports an
error, this will interrupt the current emission process.
This should be backported up to 2.6.
Continue on the cleanup of QUIC stack and components.
quic_conn uses internally a ssl_sock_ctx to handle mandatory TLS QUIC
integration. However, this is merely as a convenience, and it is not
equivalent to stackable ssl xprt layer in the context of HTTP1 or 2.
To better emphasize this, ssl_sock_ctx usage in quic_conn has been
removed wherever it is not necessary : namely in functions not related
to TLS. quic_conn struct now contains its own wait_event for tasklet
quic_conn_io_cb().
This should be backported up to 2.6.
Channel.insert(channel, string, [,offset]):
When no offset is provided, hlua_channel_insert_data() inserts
string at the end of incoming data.
This behavior conflicts with the documentation that explicitly says
that the default behavior is to insert the string in front of incoming data.
This patch fixes hlua_channel_insert_data() behavior so that it fully
complies with the documentation.
Thanks to Smackd0wn for noticing it.
This could be backported to 2.6 and 2.5
Gcc 4.x, 5.x and 6.x report this when compiling http_fetch.c:
src/http_fetch.c: In function 'smp_fetch_meth':
src/http_fetch.c:357:6: warning: 'htx' may be used uninitialized in this function [-Wmaybe-uninitialized]
sl = http_get_stline(htx);
That's quite weird since there's no such code path, but presetting the
htx variable to NULL during declaration is enough to shut it up.
This may be backported to any version that has dbbdb25f1 ("BUG/MINOR:
http-fetch: Use integer value when possible in "method" sample fetch")
as it's the one that triggered this warning (hence at least 2.0).
In smp_fetch_meth(), smp_prefetch_htx() function may be called to parse the
HTX message and update the HTTP transaction accordingly. In this case, in
smp_fetch_metch() and on success, we must update "meth" variable. Otherwise,
the variable is still equal to HTTP_METH_OTHER and the string version is
always used instead of the enum for known methods.
This patch must be backported as far as 2.0.
As seen in issue #1866, some environments will not allow to change the
current FD limit, and actually we don't need to do it, we only do it as
a byproduct of adjusting the limit to the one that fits. Here we're
replacing calls to setrlimit() with calls to raise_rlim_nofile(), which
will avoid making the setrlimit() syscall in case the desired value is
lower than the current process' one.
This depends on previous commit "MINOR: fd: add a new function to only
raise RLIMIT_NOFILE" and may need to be backported to 2.6, possibly
earlier, depending on users' experience in such environments.
In issue #1866 an issue was reported under docker, by which a user cannot
lower the number of FD needed. It looks like a restriction imposed in this
environment, but it results in an error while it ought not have to in the
case of shrinking.
This patch adds a new function raise_rlim_nofile() that takes the desired
new setting, compares it to the current one, and only calls setrlimit() if
one of the values in the new setting is larger than the older one. As such
it will continue to emit warnings and errors in case of failure to raise
the limit but will never shrink it.
This patch is only preliminary to another one, but will have to be
backported where relevant (likely only 2.6).
Building h1.c with gcc-4.7 -Os produces the following warning:
src/h1.c: In function 'h1_headers_to_hdr_list':
src/h1.c:1101:36: warning: 'ptr' may be used uninitialized in this function [-Wmaybe-uninitialized]
In fact ptr may be taken from sl.rq.u.ptr which is only initialized after
passing through the relevant states, but gcc doesn't know which states
are visited. Adding an ALREADY_CHECKED() statement there is sufficient to
shut it up and doesn't affect the emitted code.
This may be backported to stable versions to make sure that builds on older
distros and systems is clean.
In hlua_lua2arg_check(), we allow for the first argument to not be
provided, if it has a type we know, this is true for frontend, backend,
and stick table. However, the stick table code was changed. It used
to be deduced from the proxy, but it is now directly provided in struct
args. So setting the proxy there no longer work, and we have to
explicitely set the stick table.
Not doing so will lead the code do use the proxy pointer as a stick
table pointer, which will likely cause crashes.
This should be backported up to 2.0.
In hlua_lua2arg_check(), on failure, before calling free_argp(), make
sure to always mark the failed argument as ARGT_STOP. We only want to
free argument prior to that point, because we did not allocate the
strings after this one, and so we don't want to free them.
This should be backported up to 2.2.
It is possible to receive a STOP_SENDING frame for a locally closed
stream. This was not properly managed as this would result in a BUG_ON()
crash from qcs_idle_open() call under qcc_recv_stop_sending().
Now, STOP_SENDING frames are ignored when received on streams already
locally closed. This has two consequences depending on the reason of
closure :
* if a RESET_STREAM was already emitted and closed the stream, this
patch prevents to emit a new RESET_STREAM. This behavior is thus
better.
* if stream was closed due to all data transmitted, no RESET_STREAM will
be built. This is contrary to the RFC 9000 which advice to transmit
it, even on "Data Sent" state. However, this is not mandatory so the
new behavior is acceptable, even if it could be improved.
This crash has been detected on haproxy.org. This can be artifically
reproduced by adding the following snippet at the end of qc_send_mux()
when doing a request with a small payload response :
qcc_recv_stop_sending(qc->qcc, 0, 0);
This must be backported up to 2.6.
xprt_quic module was too large and did not reflect the true architecture
by contrast to the other protocols in haproxy.
Extract code related to XPRT layer and keep it under xprt_quic module.
This code should only contains a simple API to communicate between QUIC
lower layer and connection/MUX.
The vast majority of the code has been moved into a new module named
quic_conn. This module is responsible to the implementation of QUIC
lower layer. Conceptually, it overlaps with TCP kernel implementation
when comparing QUIC and HTTP1/2 stacks of haproxy.
This should be backported up to 2.6.
There was some identical code between xprt_quic and quic_enc modules.
This concerns helper on QUIC varint type. Keep only the version in
quic_enc file : this should help to reduce dependency on xprt_quic
module.
Note that quic_max_int_by_size() has been removed and is replaced by the
identical quic_max_int().
This should be backported up to 2.6.
Clean up quic sources by adjusting headers list included depending
on the actual dependency of each source file.
On some occasion, xprt_quic.h was removed from included list. This is
useful to help reducing the dependency on this single file and cleaning
up QUIC haproxy architecture.
This should be backported up to 2.6.
Two prototypes in quic_tls module were not identical to the actual
function definition.
* quic_tls_decrypt2() : the second argument const attribute is not
present, to be able to use it with EVP_CIPHER_CTX_ctlr(). As a
consequence of this change, token field of quic_rx_packet is now
declared as non-const.
* quic_tls_generate_retry_integrity_tag() : the second argument type
differ between the two. Adjust this by fixing it to as unsigned char
to match EVP_EncryptUpdate() SSL function.
This situation did not seem to have any visible effect. However, this is
clearly an undefined behavior and should be treated as a bug.
This should be backported up to 2.6.
Willy Tarreau
Christopher Faulet
Amaury Denoyelle
Aurelien DARRAGON
Tim Duesterhus
Olivier Houchard