]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
8 months agoOPTIM: mux-h2: make h2_send() report more accurate wake up conditions
Willy Tarreau [Fri, 11 Oct 2024 16:53:59 +0000 (18:53 +0200)] 
OPTIM: mux-h2: make h2_send() report more accurate wake up conditions

h2_send() used to report non-zero every time any data were sent, and
this was used from h2_snd_buf() or h2_done_ff() to trigger a wakeup,
which possibly can do nothing. Restricting this wakeup to either a
successful send() combined with the ability to demux, or an error.

Doing this makes the number of h2_io_cb() wakeups drop from 422k to
245k for 1000 1MB objects delivered over 100 streams between two H2
proxies, without any behavior change nor performance change. In
practice, most send() calls do not result in a wakeup anymore but
synchronous errors still do.

A local test downloading 10k 1MB objects from an H1 server with a single
connection shows this change:

   before      after    caller
   1547        1467     h2_process_demux()
   2138           0     h2_done_ff()       <---
     38        1453     ssl_sock_io_cb()   <---
     18           0     h2_snd_buf()
      1           1     h2_init()
   3742        2921     -- total --

In practice the ssl_sock_io_cb() wakeups are those notifying about
SUB_RETRY_RECV, which are not accounted for when h2_done_ff() performs
the wakeup because the tasklet is already queued (a counter placed
there shows that it's nonetheless called). So there's no transfer and
h2_done_ff() was only hiding the other one.

Another test involving 4 connections with 10 concurrent streams each
and 20000 1MB objects total shows a total disparition of the wakeups
from h2_snd_buf and h2_done_ff, which used to account together for
50% of the wakeups, resulting in effectively halving the number of
wakeups which, based on their avg process time, were not doing
anything:

Before:
  function   calls     cpu_tot   cpu_avg
  h2_io_cb   2571208   7.406s    2.880us <- h2c_restart_reading@src/mux_h2.c:940 tasklet_wakeup
  h2_io_cb   2536949   251.4ms   99.00ns <- h2_snd_buf@src/mux_h2.c:7573 tasklet_wakeup ###
  h2_io_cb     41100   5.622ms   136.0ns <- h2_done_ff@src/mux_h2.c:7779 tasklet_wakeup ###
  h2_io_cb     38979   852.8ms   21.88us <- sock_conn_iocb@src/sock.c:1007 tasklet_wakeup
  h2_io_cb     12519   90.28ms   7.211us <- ssl_sock_io_cb@src/ssl_sock.c:5721 tasklet_wakeup
  h2_io_cb         1   13.81us   13.81us <- sock_conn_iocb@src/sock.c:986 tasklet_wakeup
  h2_io_cb   5200756   8.606s    1.654us --total--

After:
  h2_io_cb   2562340   8.157s    3.183us <- h2c_restart_reading@src/mux_h2.c:957 tasklet_wakeup
  h2_io_cb     30109   840.9ms   27.93us <- sock_conn_iocb@src/sock.c:1007 tasklet_wakeup
  h2_io_cb     16105   106.4ms   6.607us <- ssl_sock_io_cb@src/ssl_sock.c:5721 tasklet_wakeup
  h2_io_cb         1   11.75us   11.75us <- sock_conn_iocb@src/sock.c:986 tasklet_wakeup
  h2_io_cb   2608555   9.104s    3.490us --total--

8 months agoMEDIUM: mux-h2: rework h2_restart_reading() to differentiate recv and demux
Willy Tarreau [Fri, 11 Oct 2024 15:24:02 +0000 (17:24 +0200)] 
MEDIUM: mux-h2: rework h2_restart_reading() to differentiate recv and demux

From the beginning, h2_restart_reading() has always been confusing because
it decides whether or not to wake the tasklet handler up or not. This
tasklet handler does two things, one is receiving from the socket to the
demux buf, and one is demuxing from the demux buf to the streams' rxbufs.

The conditions are governed by h2_recv_allowed(), which is also called at
a few places to decide whether or not to actually receive from the socket.
It starts to be visible that this leaves some difficulties regarding what
to do with possibly pending data.

In 2.0 with commit 3ca18bf0b ("BUG/MEDIUM: h2: Don't attempt to recv from
h2_process_demux if we subscribed."), we even had to address a special
case where it was possibly to endlessly wake up because the conditions
would rely on the demux buffer's contents, though the solution consisted
in passing a flag to decide whether or not to consider the buffer's
contents.

In 2.5 commit b5f7b5296 ("BUG/MEDIUM: mux-h2: Handle remaining read0 cases
on partial frames") introduced a new flag H2_CF_DEM_SHORT_READ which
indicates that the demux had to stop in the middle of a frame and cannot
make progress without more data. More adaptations later came in based on
this but this actually reflected exactly what was needed to solve this
painful situation: a state indicating whether to receive or parse.

Now's about time to definitely address this by reworking h2_restart_reading()
to check two completely independent things:
  - the ability to receive more data into the demux buffer, which is
    based on its allocation/fill state and the socket's errors
  - the ability to demux such data, which is based on the presence of
    enough data (i.e. no stuck short read), and ability to find an rx
    buf to continue the processing.

Now the conditions are much more understandable, and it's also visible
that the consider_buffer argument, whose value was not trivial for
callers, is not used anymore.

Tests stacking two layers of H2 show strictly no change to the wakeup
cause distributions nor counts.

8 months agoDOC: design-thoughts: add diagrams illustrating an rx win groth
Willy Tarreau [Mon, 19 Aug 2024 04:57:35 +0000 (06:57 +0200)] 
DOC: design-thoughts: add diagrams illustrating an rx win groth

Let's just see on a diagram how the receiver can detect that the
window is large enough for the remote sender to fill the link. Here
it seems that a first criterion is that data are accumulating in
the rxbuf, indicating that the next hop doesn't consume them fast
enough. On the diagram it's visible when blue arrows (incoming data)
are more frequent than the magenta ones on average (outgoing data),
which happens when silence moments are less frequent and don't allow
the reader to catch up. It's also visible that there are two phases
alternating in the transfer:
  - measure round trip time (i.e. how long it takes to restart
    sending after a WU was sent after a long silence)

  - measure the lowest rxbuf size during the previous round trip

It's worth noting that a window size change only has *observable* effect
after two RTT: the first RTT is to restart sending (opening or enlarging
the window), the second RTT to measure the lowest rxbuf size over the
period.

By turning the advertised window into an offset and comparing it to
the received quantity, it's possible to measure the RTT of the whole
chain (including the client possibly producing the data). Note that
when multiple streams compete for BW this can become tricky. Limiting
the window to available buffers and counting the number of sending
streams on a connection could work (i.e. split total buffers into
1+#senders, first one being used for tx).

8 months agoMEDIUM: mux-h2: change the default initial window to 16kB
Willy Tarreau [Wed, 9 Oct 2024 06:43:31 +0000 (08:43 +0200)] 
MEDIUM: mux-h2: change the default initial window to 16kB

Now that we're using all available rx buffers for transfers, there's
no point anymore in advertising more than the minimum value we can
safely buffer. Let's be conservative and only rely on the dynamic
buffers to improve speed beyond the configured value, and make sure
than many streams will no longer cause unfairness.

Interestingly, the total number of wakeups has further shrunk down, but
with a different distribution. From 128k for 1000 1M transfers, it went
down to 119k, with 96k from restart_reading, 10k from done_ff and 2.6k
from snd_buf. done_ff went up by 30% and restart_reading went down by
30%.

8 months agoMINOR: mux-h2: add tune.h2.be.rxbuf and tune.h2.fe.rxbuf global settings
Willy Tarreau [Wed, 9 Oct 2024 06:30:38 +0000 (08:30 +0200)] 
MINOR: mux-h2: add tune.h2.be.rxbuf and tune.h2.fe.rxbuf global settings

These settings allow to change the total buffer size allocated to the
backend and frontend respectively. This way it's no longer necessary to
play with tune.bufsize nor increase the number of streams to benefit from
more buffers.

Setting tune.h2.fe.rxbuf to 4m to match a sender's max tcp_wmem resulted
in 257 Mbps for a single stream at 103ms vs 121 Mbps default (or 5.1 Mbps
with a single buffer and 64kB window).

8 months agoMAJOR: mux-h2: make the rxbuf allocation algorithm a bit smarter
Willy Tarreau [Tue, 8 Oct 2024 12:17:54 +0000 (14:17 +0200)] 
MAJOR: mux-h2: make the rxbuf allocation algorithm a bit smarter

Without using bandwidth estimates, we can already use up to the number
of allocatable rxbufs and share them evenly between receiving streams.
In practice we reserve one buffer for any non-receiving stream, plus
1 per 8 possible new streams, and divide the rest between the number
of receiving streams.

Finally, for front streams, this is rounded up to the buffer size while
for back streams we round it down. The rationale here is that front to
back is very fast to flush and slow to refill so we want to optimise
upload bandwidth regardless of the number of streams, while it's the
opposite in the other way so we try to minimize HoL.

That shows good results with a single stream being able to send at 121
Mbps at 103ms using 1.4 MB buffer with default settings, or 8 streams
sharing the bandwidth at 180kB each. Previously the limit was approx
5.1 Mbps per stream.

It also enables better sharing of backend connections: a slow (100 Mbps)
and a fast (1 Gbps) clients were both downloading 2 100MB files each over
a shared H2 connection. The fast one used to show 6.86 to 20.74s with an
avg of 11.45s and an stddev of 5.81s before the patch, and went to a
much more respectable 6.82 to 7.73s with 7.08s avg and 0.336s stddev.

We don't try to increase the window past the remaining content length.
First, this is pointless (though harmless), but in addition it causes
needless emission of WINDOW_UPDATE frames on small uploads that are
smaller than a window, and beyond being useless, it upsets vtest which
expects an RST on some tests. The scheduling is not reliable enough to
insert an expect for a window update first, so in the end wich that
extra check we save a few useless frames on small uploads and please
vtest.

A new setting should be added to allow to increase the number of buffers
without having to change the number of streams. At this point it's not
done.

8 months agoMAJOR: mux-h2: permit a stream to allocate as many buffers as desired
Willy Tarreau [Thu, 3 Oct 2024 14:09:18 +0000 (16:09 +0200)] 
MAJOR: mux-h2: permit a stream to allocate as many buffers as desired

Now we don't enforce allocation limits in h2s_get_rxbuf(), since there
is no benefit in not processing pending data, it would still cause HoL
for no saving. The only reason for not allocating is if there are no
buffers available for the connection.

In theory this should not change anything except that it excerts code
paths that support reallocating multiple buffers, which could possibly
uncover a sleeping bug. This is why it's placed in a separate commit.

And one observation worth noting is that it almost cut in half the number
of iocb wakeups: for 1000 1MB transfers over 100 concurrent streams of a
single connection, we used to observe 208k wakeups (110 from restart_reading,
80 from snd_buf, 11 from done_ff), and now we're observing 128k (113 from
restart_reading, 2.4 from snd_buf, 6.9k from done_ff), which seems to
indicate that pretty often the demuxing was blocked on a buffer full due
to the default advertised window of 64k.

8 months agoMAJOR: mux-h2: make streams use the connection's buffers
Willy Tarreau [Wed, 2 Oct 2024 15:46:32 +0000 (17:46 +0200)] 
MAJOR: mux-h2: make streams use the connection's buffers

For now it seems to work as before, and even when artificially inflating
the number of allocatable buffers per stream. The number of allocated
slots is always the same as the max number of streams, which guarantees
that each stream will find one buffer. we only grant one buffer per
stream at this point, since the goal was to replace the existing single
rxbuf.

A new demux blocking flag, H2_CF_DEM_RXBUF, was added to indicate
a failure to get an rxbuf slot from the connection. It was lightly
tested (by forcing bl_init() to a lower number of buffers). It is not
yet certain whether it's more useful to have a new flag or to reuse
the existing H2_CF_DEM_SFULL which indicates the rxbuf is full,
but at least the new flag more accurately translates the condition,
that may make a difference in the future. However, given that when
RXBUF is set, most of the time it results in a failure to find more
room to demux and it sets SFULL, for now we have to always clear
SFULL when clearing RXBUF as well. This means that most of the time
we'll see 3 combinations:
  - none: everything's OK
  - SFULL: the unique rx buffer is full
  - RXBUF || (RXBUF|SFULL): cannot allocate more entries

Note that we need to be super careful in h2_frt_transfer_data() because
the htx_free_data_space() function doesn't guarantee that the room is
usable, so htx_add_data() may still fail despite an apparent room. For
this reason, h2_frt_transfer_data() maintains a "full" flag to indicate
that a transfer attempt failed and that a new buffer is required.

8 months agoMINOR: mux-h2: clear up H2_CF_DEM_DFULL and H2_CF_DEM_SHORT_READ ambiguity
Willy Tarreau [Wed, 9 Oct 2024 18:00:07 +0000 (20:00 +0200)] 
MINOR: mux-h2: clear up H2_CF_DEM_DFULL and H2_CF_DEM_SHORT_READ ambiguity

Since commit 485da0b05 ("BUG/MEDIUM: mux_h2: Handle others remaining
read0 cases on partial frames"), H2_CF_DEM_SHORT_READ is set when there
is no blocking flags. However, it checks H2_CF_DEM_BLOCK_ANY which does
not include H2_CF_DEM_DFULL. This results in many cases where both
H2_CF_DEM_DFULL and H2_CF_DEM_SHORT_READ are set together, which makes
no sense, since one says the demux buffer is full while the other one
says an incomplete read was done. This doesn't permit to properly
decide whether to restart reading or processing.

Let's make sure to clear DFULL in h2_process_demux() whenever we
consume incoming data from the dbuf, and check for DFULL before
setting SHORT_READ.

This could probably be considered as a bug fix but it's hard to say if
it has any impact on the current code, probably at worst it might cause
a few useless wakeups, so until there's any proof that it needs to be
backported, better not do it.

8 months agoMINOR: mux-h2: simplify the wake up code in h2_rcv_buf()
Willy Tarreau [Fri, 11 Oct 2024 12:49:45 +0000 (14:49 +0200)] 
MINOR: mux-h2: simplify the wake up code in h2_rcv_buf()

The code used to decide when to restart reading is far from being trivial
and will cause trouble after the forthcoming changes: it checks if the
current stream is the same that is being demuxed, and only if so, wakes
the demux to restart reading. Once streams will start to use multiple
buffers, this condition will make no sense anymore. Actually the real
reason is split into two steps:
  - detect if the demux is currently blocked on the current stream, and
    if so remove SFULL
  - detect if any demux blocking flags were removed during the operations,
    and if so, wake demuxing.

For now this doesn't change anything.

8 months agoMINOR: mux-h2: simplify the exit code in h2_rcv_buf()
Willy Tarreau [Wed, 2 Oct 2024 15:13:28 +0000 (17:13 +0200)] 
MINOR: mux-h2: simplify the exit code in h2_rcv_buf()

The code used to decide what to tell to the upper layer and when to free
the rxbuf is a bit convoluted and difficult to adapt to dynamic rxbufs.
We first need to deal with memory management (b_free) and only then to
decide what to report upwards. Right now it does it the other way around.

This should not change anything.

8 months agoMINOR: mux-h2: move H2_CF_WAIT_IN_LIST flag away from the demux flags
Willy Tarreau [Wed, 2 Oct 2024 12:13:24 +0000 (14:13 +0200)] 
MINOR: mux-h2: move H2_CF_WAIT_IN_LIST flag away from the demux flags

It's not convenient to have this flag in the middle of the demux flags,
it easily hides other ones that need to be added. Let's move it after
the other ones.

8 months agoMINOR: mux-h2: add rxbuf head/tail/count management for h2s
Willy Tarreau [Tue, 1 Oct 2024 16:16:51 +0000 (18:16 +0200)] 
MINOR: mux-h2: add rxbuf head/tail/count management for h2s

Now the h2s get their rx_head, rx_tail and rx_count associated with the
shared rxbufs. A few functions are provided to manipulate all this,
essentially allocate/release a buffer for the stream, return a buffer
pointer to the head/tail, counting allocated buffers for the stream
and reporting if a stream may still allocate.

For now this code is not used.

8 months agoMINOR: mux-h2: allocate the array of shared rx bufs in the h2c
Willy Tarreau [Tue, 1 Oct 2024 09:00:49 +0000 (11:00 +0200)] 
MINOR: mux-h2: allocate the array of shared rx bufs in the h2c

In preparation for having a shared list of rx bufs, we're now allocating
the array of shared rx bufs in the h2c. The pool is created at the max
size between the front and back max streams for now, and the array is not
used yet.

8 months agoMINOR: mux-h2: count within a connection, how many streams are receiving data
Willy Tarreau [Wed, 28 Aug 2024 09:38:32 +0000 (11:38 +0200)] 
MINOR: mux-h2: count within a connection, how many streams are receiving data

A stream is receiving data from after the HEADERS frame missing END_STREAM,
to the end of the stream or HREM (the presence of END_STREAM). We're now
adding a flag to the stream that indicates this state, as well as a counter
in the connection of streams currently receiving data. The purpose will be
to gauge at any instant the number of streams that might have to share the
available bandwidth and buffers count in order not to allocate too much flow
control to any single stream. For now the counter is kept up to date, and is
reported in "show fd".

8 months agoMEDIUM: mux-h2: start to introduce the window size in the offset calculation
Willy Tarreau [Tue, 27 Aug 2024 18:22:08 +0000 (20:22 +0200)] 
MEDIUM: mux-h2: start to introduce the window size in the offset calculation

Instead of incrementing the last_max_ofs by the amount of received bytes,
we now start from the new current offset to which we add the static window
size. The result is exactly the same but it prepares the code to use a
window size combined with an offset instead of just refilling the budget
from what was received.

It was even verified that changing h2_fe_settings_initial_window_size in
the middle of a transfer using gdb does indeed allow the transfer speed
to adapt accordingly.

8 months agoMEDIUM: mux-h2: start to update stream when sending WU
Willy Tarreau [Tue, 27 Aug 2024 17:33:25 +0000 (19:33 +0200)] 
MEDIUM: mux-h2: start to update stream when sending WU

The rationale here is that we don't absolutely need to update the
stream offset live, there's already the rcvd_s counter to remind
us we've received data. So we can continue to exploit the current
check points for this.

Now we know that rcvd_s indicates the amount of newly received bytes
for the stream since last call to h2c_send_strm_wu() so we can update
our stream offsets within that function. The wu_s counter is set to
the difference between next_adv_ofs and last_adv_ofs, which are
resynchronized once the frame is sent.

If the stream suddenly disappears with unacked data (aborted upload),
the presence of the last update in h2c->wu_s is sufficient to let the
connection ack the data alone, and upon subsequent calls with new
rcvd_s, the received counter will be used to ack, like before. We
don't need to do more anyway since the goal is to let the client
abort ASAP when it gets an RST.

At this point, the stream knows its current rx offset, the computed
max offset and the last advertised one.

8 months agoMINOR: mux-h2: create and initialize an rx offset per stream
Willy Tarreau [Tue, 27 Aug 2024 16:57:45 +0000 (18:57 +0200)] 
MINOR: mux-h2: create and initialize an rx offset per stream

In H2, everything is accounted as budget. But if we want to moderate
the rcv window that's not very convenient, and we'd rather have offsets
instead so that we know where we are in the stream. Let's first add
the fields to the struct and initialize them. The curr_rx_ofs indicates
the position in the stream where next incoming bytes will be stored.
last_adv_ofs tells what's the offset that was last advertised as the
window limit, and next_max_ofs is the one that will need to be
advertised, which is curr_rx_ofs plus the current window. next_max_ofs
will have to cause a WINDOW_UPDATE to be emitted when it's higher than
last_adv_ofs, and once the WU is sent, its value will have to be copied
over last_adv_ofs.

The problem is, for now wherever we emit a stream WU, we have no notion
of stream (the stream might even not exist anymore, e.g. after aborting
an upload), because we currently keep a counter of stream window to be
acked for the current stream ID (h2c->dsi) in the connection (rcvd_s).
Similarly there are a few places early in the frame header processing
where rcvd_s is incremented without knowing the stream yet. Thus, lookups
will be needed for that, unless such a connection-level counter remains
used and poured into the stream's count once known (delicate).

Thus for now this commit only creates the fields and initializes them.

8 months agoMINOR: mux-h2: split the amount of rx data from the amount to ack
Willy Tarreau [Tue, 27 Aug 2024 17:40:57 +0000 (19:40 +0200)] 
MINOR: mux-h2: split the amount of rx data from the amount to ack

We'll need to keep track of the total amount of data received for the
current stream, and the amount of data to ack for the current stream,
which might soon diverge as soon as we'll have to update the stream's
offset with received data, which are different from those to be ACKed.
One reason is that in case a stream doesn't exist anymore (e.g. aborted
an upload), the rcvd_s info might get lost after updating the stream,
so we do need to have an in-connection counter for that.

What's done here is that the rcvd_s count is transferred to wu_s in
h2c_send_strm_wu(), to be used as the counter to send, and both are
considered as sufficient when non-null to call the function.

8 months agoMINOR: buffer: add a buffer list type with functions
Willy Tarreau [Wed, 28 Aug 2024 16:27:51 +0000 (18:27 +0200)] 
MINOR: buffer: add a buffer list type with functions

The buffer ring is problematic in multiple aspects, one of which being
that it is only usable by one entity. With multiplexed protocols, we need
to have shared buffers used by many entities (streams and connection),
and the only way to use the buffer ring model in this case is to have
each entity store its own array, and keep a shared counter on allocated
entries. But even with the default 32 buf and 100 streams per HTTP/2
connection, we're speaking about 32*101*32 bytes = 103424 bytes per H2
connection, just to store up to 32 shared buffers, spread randomly in
these tables. Some users might want to achieve much higher than default
rates over high speed links (e.g. 30-50 MB/s at 100ms), which is 3 to 5
MB storage per connection, hence 180 to 300 buffers. There it starts to
cost a lot, up to 1 MB per connection, just to store buffer indexes.

Instead this patch introduces a variant which we call a buffer list.
That's basically just a free list encoded in an array. Each cell
contains a buffer structure, a next index, and a few flags. The index
could be reduced to 16 bits if needed, in order to make room for a new
struct member. The design permits initializing a whole freelist at once
using memset(0).

The list pointer is stored at a single location (e.g. the connection)
and all users (the streams) will just have indexes referencing their
first and last assigned entries (head and tail). This means that with
a single table we can now have all our buffers shared between multiple
streams, irrelevant to the number of potential streams which would want
to use them. Now the 180 to 300 entries array only costs 7.2 to 12 kB,
or 80 times less.

Two large functions (bl_deinit() & bl_get()) were implemented in buf.c.
A basic doc was added to explain how it works.

8 months agoREORG: buffers: move some of the heavy functions from buf.h to buf.c
Willy Tarreau [Mon, 30 Sep 2024 13:23:43 +0000 (15:23 +0200)] 
REORG: buffers: move some of the heavy functions from buf.h to buf.c

Over time, some of the buffer management functions grew quite a bit,
and were still forced to remain inlined since all defined in buf.h.
Let's create buf.c and move the heaviest ones there. All those moved
here were above 200 bytes.

8 months agoCLEANUP: muxes: remove useless inclusion of ebmbtree.h
Willy Tarreau [Tue, 27 Aug 2024 17:17:25 +0000 (19:17 +0200)] 
CLEANUP: muxes: remove useless inclusion of ebmbtree.h

Since 2.7 with commit 8522348482 ("BUG/MAJOR: conn-idle: fix hash indexing
issues on idle conns"), we've been using eb64 trees and not ebmb trees
anymore, and later we dropped all that to centralize the operations in
the server. Let's remove the ebmbtree.h includes from the muxes that do
not use them.

8 months agoMINOR: mux-h2/traces: print the size of the DATA frames
Willy Tarreau [Thu, 3 Oct 2024 14:04:04 +0000 (16:04 +0200)] 
MINOR: mux-h2/traces: print the size of the DATA frames

DATA frames produce a special trace with the amount of transferred data
in arg4, but this was not reported by h2_trace(). This commit just adds
it.

8 months agoBUG/MINOR: mux-h2/traces: present the correct buffer for trailers errors traces
Willy Tarreau [Wed, 2 Oct 2024 13:39:52 +0000 (15:39 +0200)] 
BUG/MINOR: mux-h2/traces: present the correct buffer for trailers errors traces

The local "rxbuf" buffer was passed to the trace instead of h2s->rxbuf
that is used when decoding trailers. The impact is essentially the
impossibility to present some buffer contents in some rare cases. It
may be backported but it's unlikely that anyone will ever notice the
difference.

8 months agoBUILD: cache: silence an uninitialized warning at -Og with gcc-12.2
Willy Tarreau [Thu, 10 Oct 2024 04:59:12 +0000 (06:59 +0200)] 
BUILD: cache: silence an uninitialized warning at -Og with gcc-12.2

Building with gcc-12.2 -Og yields this incorrect warning in cache.c:

  In function 'release_entry_unlocked',
      inlined from 'http_action_store_cache' at src/cache.c:1449:4:
  src/cache.c:330:9: warning: 'object' may be used uninitialized [-Wmaybe-uninitialized]
    330 |         release_entry(cache, entry, 1);
        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  src/cache.c: In function 'http_action_store_cache':
  src/cache.c:1200:29: note: 'object' was declared here
   1200 |         struct cache_entry *object, *old;
        |                             ^~~~~~

This is wrong, the only way to reach the function is with first!=NULL
and the gotos that reach there are all those made with first==NULL.
Let's just preset object to NULL to silence it.

9 months agoMINOR: cfgparse: simulate long configuration parsing with force-cfg-parser-pause
William Lallemand [Fri, 11 Oct 2024 15:38:46 +0000 (17:38 +0200)] 
MINOR: cfgparse: simulate long configuration parsing with force-cfg-parser-pause

This command is pausing the configuration parser for <timeout>
milliseconds. This is useful for development or for testing timeouts of
init scripts, particularly to simulate a very long reload. It requires
the expose-experimental-directives to be set.

9 months agoBUG/MEDIUM: mux-quic: ensure timeout server is active for short requests
Amaury Denoyelle [Thu, 10 Oct 2024 15:07:36 +0000 (17:07 +0200)] 
BUG/MEDIUM: mux-quic: ensure timeout server is active for short requests

If a small request is received on QUIC MUX frontend, it can be
transmitted directly with the FIN on attach operation. rcv_buf is
skipped by the stream layer. Thus, it is necessary to ensure that there
is similar behavior when FIN is reported either on attach or rcv_buf.

One difference was that se_expect_data() was called only for rcv_buf but
not on attach. This most obvious effect is that stream timeout was
deactivated for this request : client timeout was disabled on EOI but
server one not armed due to previous se_expect_no_data(). This prevents
the early closure of too long requests.

To fix this, add an invokation of se_expect_data() on attach operation.

This bug can simply be detected using httpterm with delay request (for
example /?t=10000) and using smaller client/server timeouts. The bug is
present if the request is not aborted on timeout but instead continue
until its proper HTTP 200 termination.

This has been introduced by the following commit :
  85eabfbf672c57e4ed082da1b96c95348b331320
  MEDIUM: mux-quic: Don't expect data from server as long as request is unfinished

This must be backported up to 2.8.

9 months agoMINOR: sample: postresolve sink names in debug() converter
Aurelien DARRAGON [Thu, 10 Oct 2024 09:37:36 +0000 (11:37 +0200)] 
MINOR: sample: postresolve sink names in debug() converter

debug() converter used to resolve sink names during parsing time. Because
of this, we were unable to specify sink names that were defined after
the debug() converter was placed.

Like in the previous commit, let's implement proper postparsing for the
debug() converter, in order to be able to use sink names that are about
to be defined later in the config file.

9 months agoMINOR: trace: postresolve sink names
Aurelien DARRAGON [Tue, 8 Oct 2024 17:02:20 +0000 (19:02 +0200)] 
MINOR: trace: postresolve sink names

A previous known limitation about traces was that parsing was performed on
the fly, meaning that when using "sink" keyword, only sinks that were
either internal or previously defined in the config could be used. Indeed,
it was not possible to use a ring section defined AFTER the traces section
when using the 'sink' keyword from traces.

This limitation was also mentioned in the config file.

Let's get rid of that limitation by implementing proper postparsing for
the sink parameter in traces section. To do this, make use of the new
sink_find_early() helper to start referencing sink by their names even
if they don't exist yet (if they are about to be defined later in the
config)

Traces commands on the cli are not concerned by this change.

9 months agoMEDIUM: sink: implement sink_find_early()
Aurelien DARRAGON [Thu, 10 Oct 2024 09:31:59 +0000 (11:31 +0200)] 
MEDIUM: sink: implement sink_find_early()

sink_find_early() is a convenient function that can be used instead of
sink_find() during parsing time in order to try to find a matching
sink even if the sink is not defined yet.

Indeed, if the sink is not defined, sink_find_early() will try to create
it and mark it as forward-declared. It will also save informations from
the caller to better identify it in case of errors.

If the sink happens to be found in the config, it will transition from
forward-declared type to its final type. Else, it means that the sink
was not found in the config, in this case, during postresolve, we raise
an error to indicate that the sink was not found in the configuration.

It should help solve postresolving issue with rings, because for now only
log targets implement proper ring postresolving.. but rings may be used
at different places in the code, such as debug() converter or in "traces"
section.

9 months agoMINOR: ssl: disable server side default CRL check with WolfSSL
Damien Claisse [Tue, 8 Oct 2024 13:32:49 +0000 (13:32 +0000)] 
MINOR: ssl: disable server side default CRL check with WolfSSL

Patch 64a77e3ea5 disabled CRL check when no CRL file was provided, but
it only did it on bind side. Add the same fix in server context
initialization side.
This allows to enable peer verification (verify required) on a server
using TLS, without having to provide a CRL file.

9 months agoBUG/MEDIUM: quic: properly decount out-of-order ACK on stream release
Amaury Denoyelle [Wed, 9 Oct 2024 09:59:32 +0000 (11:59 +0200)] 
BUG/MEDIUM: quic: properly decount out-of-order ACK on stream release

Out-of-order STREAM ACK are buffered in its related streambuf tree. On
insertion, overlapping or contiguous ranges are merged together. The
total size of buffered ack range is stored in <room> streambuf member
and reported to QUIC MUX layer on streambuf release. The objective is to
ensure QUIC MUX layer can allocate Tx buffers conveniently to preserve a
good transfer throughput.

Streamdesc is the overall container of many streambufs. It may also been
released when its upper QCS instance is freed, after all stream data
have been emitted. In this case, the active streambuf is also released
via custom code. However, in this code path, <room> was not reported to
the QUIC MUX layer.

This bug caused wrong estimation for the QUIC MUX txbuf window, with
bytes reamining even after all ACK reception. This may cause transfer
freeze on other connection streams, with RESET_STREAM emission on
timeout client.

To fix this, reuse the existing qc_stream_buf_release() function on
streamdesc release. This ensures that notify_room is correctly used.

No need to backport.

9 months agoBUG/MINOR: quic: fix discarding of already stored out-of-order ACK
Amaury Denoyelle [Wed, 9 Oct 2024 10:03:36 +0000 (12:03 +0200)] 
BUG/MINOR: quic: fix discarding of already stored out-of-order ACK

To properly decount out-of-order acked data range, contiguous or
overlapping ranges are first merged before their insertion in a tree.

The first step ensure that a newly reported range is not completely
covered by the existing tree ranges. However, one of the condition was
incorrect. Fix this to ensure that the final range tree does not contain
duplicated entry.

The impact of this bug is unknown. However, it may have allowed the
insertion of overlapping ranges, which could in turn cause an error in
QUIC MUX txbuf window, with a possible transfer freeze.

No need to backport.

9 months agoBUG/MEDIUM: hlua: properly handle sample func errors in hlua_run_sample_{fetch,conv}()
Aurelien DARRAGON [Tue, 8 Oct 2024 09:42:14 +0000 (11:42 +0200)] 
BUG/MEDIUM: hlua: properly handle sample func errors in hlua_run_sample_{fetch,conv}()

To execute sample fetches and converters from lua. hlua API leverages the
sample API. Prior to executing the sample func, the arg checker is called
from hlua_run_sample_{fetch,conv}() to detect potential errors.

However, hlua_run_sample_{fetch,conv}() both pass NULL as <err> argument,
but it is wrong for two reasons. First we miss an opportunity to report
precise error messages to help the user know what went wrong during the
check.. and more importantly, some val check functions consider that the
<err> pointer is never NULL. This is the case for example with
check_crypto_hmac(). Because of this, when such val check functions
encounter an error, they will crash the process because they will try
to de-reference NULL.

This bug was discovered and reported by GH user @JB0925 on #2745.

Perhaps val check functions should make sure that the provided <err>
pointer is != NULL prior to de-referencing it. But since there are
multiple occurences found in the code and the API isn't clear about that,
it is easier to fix the hlua part (caller) for now.

To fix the issue, let's always provide a valid <err> pointer when
leveraging val_arg() check function pointer, and make use of it in case
or error to report relevant message to the user before freeing it.

It should be backported to all stable versions.

9 months agoBUG/MEDIUM: hlua: make hlua_ctx_renew() safe
Aurelien DARRAGON [Tue, 8 Oct 2024 09:34:10 +0000 (11:34 +0200)] 
BUG/MEDIUM: hlua: make hlua_ctx_renew() safe

hlua_ctx_renew() is called from unsafe places where the caller doesn't
expect it to LJMP.. however hlua_ctx_renew() makes use of Lua library
function that could potentially raise errors, such as lua_newthread(),
and it does nothing to catch errors. Because of this, haproxy could
unexpectedly crash. This was discovered and reported by GH user
@JB0925 on #2745.

To fix the issue, let's simply make hlua_ctx_renew() safe by applying
the same logic implemented for hlua_ctx_init() or hlua_ctx_destroy(),
which is catching Lua errors by leveraging SET_SAFE_LJMP_PARENT() helper.

It should be backported to all stable versions.

9 months agoREGTESTS: add some tests for 'do-log' action
Aurelien DARRAGON [Thu, 3 Oct 2024 07:26:21 +0000 (09:26 +0200)] 
REGTESTS: add some tests for 'do-log' action

Now that 'do-log' action may be used for all existing action contexts,
let's add some tests in reg-tests/log/log_profile.vtc to ensure it works
as expected. quic-ini is not tested as it may not be builtin depending on
build options..

9 months agoMINOR: action: add do-log action
Aurelien DARRAGON [Fri, 20 Sep 2024 07:34:13 +0000 (09:34 +0200)] 
MINOR: action: add do-log action

Thanks to the two previous commits, we can now expose the do-log action
on all available action contexts, including the new quic-init context.

Each context is responsible for exposing the do-log action by registering
the relevant log steps, saving the idendifier, and then store it in the
rule's context so that do_log_action() automatically uses it to produce
the log during runtime.

To use the feature, it is simply needed to use "do-log" (without argument)
on an action directive, example:

   tcp-request connection do-log

As mentioned before, each context where the action is exposed has its own
log step identifier. Currently known identifiers are:

  quic-initial:           quic-init
  tcp-request connection: tcp-req-conn
  tcp-request session:    tcp-req-sess
  tcp-request content:    tcp-req-cont
  tcp-response content:   tcp-res-cont
  http-request:           http-req
  http-response:          http-res
  http-after-response:    http-after-res

Thus, these "additional" logging steps can be used as-is under log-profile
section (after "on" keyword). However, although the parser will accept
them, it makes no sense to use them with the "log-steps" proxy keyword,
since the only path for these origins to trigger a log generation is
through the explicit use of "do-log" action.

This need was described in GH #401, it should help to conditionally
trigger logs using ACL at specific key points.. and may either be used
alone or combined with "log-steps" to add additional log "trackers" during
transaction handling.

Documentation was updated and some examples were added.

9 months agoMINOR: log: add do_log_parse_act() helper func
Aurelien DARRAGON [Tue, 1 Oct 2024 15:55:55 +0000 (17:55 +0200)] 
MINOR: log: add do_log_parse_act() helper func

Function may be used from places where per-context actions are usually
registered (tcp_act.c, http_act.c, quic_rules.c.. to name a few) in
order to expose the do_log() action.

9 months agoMINOR: log: add do_log() logging helper
Aurelien DARRAGON [Thu, 26 Sep 2024 08:09:09 +0000 (10:09 +0200)] 
MINOR: log: add do_log() logging helper

do_log() is quite similar to sess_log() or strm_log(), excepts that it
may be called at any time during session handling in an opportunistic
way as long as the session exists (the stream may or may not exist).

Also, it will try to emit the log as INFO by default, unless set-log-level
is used on the stream, or error origin flag is set.

9 months agoMEDIUM: quic: decount out-of-order ACK data range for MUX txbuf window
Amaury Denoyelle [Wed, 2 Oct 2024 12:44:41 +0000 (14:44 +0200)] 
MEDIUM: quic: decount out-of-order ACK data range for MUX txbuf window

This commit is the last one of a serie whose objective is to restore
QUIC transfer throughput performance to the state prior to the recent
QUIC MUX buffer allocator rework.

This gain is obtained by reporting received out-of-order ACK data range
to the QUIC MUX which can then decount room in its txbuf window. This is
implemented in QUIC streamdesc layer by adding a new invokation of
notify_room callback. This is done into qc_stream_buf_store_ack() which
handle out-of-order ACK data range.

Previous commit has introduced merging of overlapping ACK data range. As
such, it's easy to only report the newly acknowledged data range.

As with in-order ACKs, this new notification is only performed on
released streambuf. As such, when a streambuf instance is released,
notify_room notification now also reports the total length of
out-of-order ACK data range currently stored. This value is stored in a
new streambuf member <room> to avoid unnecessary tree lookup.

This <room> member also serves on in-order ACK notification to reduce
the notified room. This prevents to report invalid values when overlap
ranges are treated first out-of-order and then in-order, which would
cause an invalid QUIC MUX txbuf window value.

After this change has been implemented, performance has been
significantly improved, both with ngtcp2-client rate usage and on
interop goodput test. These values are now similar to the rate observed
on older haproxy version before QUIC MUX buffer allocator rework.

9 months agoMEDIUM: quic: merge contiguous/overlapping buffered ack stream range
Amaury Denoyelle [Wed, 2 Oct 2024 08:23:21 +0000 (10:23 +0200)] 
MEDIUM: quic: merge contiguous/overlapping buffered ack stream range

Transfer throughput was deteriorated since recent rework of QUIC MUX
txbuf allocator. This was partially restorated with the commit to
decount individual in-order ACK from the MUX buffer window.

To fully retrieve the old performance level, all ACKs must be decounted
when handled by QUIC streamdesc layer, event out-of-order ranges.
However, this is not easily implemented as several ranges may exist in
parallel with overlap on the underlying data. It would cause
miscalculation for QUIC MUX buffer window if such ranges were blindly
reported.

The proper solution is to first implement merge of contiguous or
overlapping ACK data ranges to reduce the number of stored ranges to the
minimal. This is the purpose of this patch. This is implemented in a new
static function named qc_stream_buf_store_ack() into streamdesc layer.

The merge algorithm is simple enough. First, it ensures the newly added
range is not already fully covered by a preexisting entry. Then, it
checks if there is contiguity/overlap with one or several ranges
starting at the same of a greater offset. If true, the newly added entry
is extended to cover them all, and all contiguous/overlapped ranges are
removed. Finally, if there is contiguity or overlap with an entry
starting at a smaller offset, no new range is instantiated and instead
the smaller offset is extended.

Now that contiguous or overlapped ranges cannot exits anymore, ACK data
ranges tree instiatiation can used EB_ROOT_UNIQUE.

Outside of the longer term objective which is to decount out-of-order
ACKs from MUX txbuf window, this commit could also improve some
performance and/or memory usage for connections where stream data
fragmentation and packet reording is high.

9 months agoMINOR: quic: implement dedicated type for out-of-order stream ACK
Amaury Denoyelle [Tue, 1 Oct 2024 15:34:55 +0000 (17:34 +0200)] 
MINOR: quic: implement dedicated type for out-of-order stream ACK

QUIC streamdesc layer is responsible to handle reception of ACK for
streams. It removes stream data from the underlying buffers on ACK
reception.

Streamdesc layer treats ACK in order at the stream level. Out of order
ACKs are buffered in a tree until they can be handled on older data
acknowledgement reception. Previously, qf_stream instance which comes
from the quic_tx_packet was used as tree node to buffer such ranges.

Introduce a new type dedicated to represent out of order stream ack data
range. This type is named qc_stream_ack. It contains minimal infos only
relative to the acknowledged stream data range.

This allows to reduce size of frequently used quic_frame with the
removal of tree node from qf_stream. Another side effect of this change
is that now quic_frame are always released immediately on ACK reception,
both in-order and out-of-order. This allows to also release the
quic_tx_packet instance which should reduce memory consumption.

The drawback of this change is that qc_stream_ack instance must be
allocated on out-of-order ACK reception. As such, qc_stream_desc_ack()
may fail if an error happens on allocation. For the moment, such error
is silenly recovered up to qc_treat_rx_pkts() with the dropping of the
received packet containing the ACK frame. In the future, it may be
useful to close the connection as this error may only happens on low
memory usage.

9 months agoMEDIUM: quic: decount acknowledged data for MUX txbuf window
Amaury Denoyelle [Wed, 18 Sep 2024 08:32:39 +0000 (10:32 +0200)] 
MEDIUM: quic: decount acknowledged data for MUX txbuf window

Recently, a new allocation mechanism was implemented for Tx buffers used
by QUIC MUX. Now, underlying congestion window size is used to determine
if it is still possible or not to allocate a new buffer when necessary.

This mechanism has render the QUIC stack more flexible. However, it also
has brought some performance degradation, with transfer time longer in
certain environment. It was first discovered on the measurement results
of the interop. It can also easily be reproduced using the following
ngtcp2-client example which forces a very small congestion window due to
frequent loss :

 $ ngtcp2-client -q --no-quic-dump --no-http-dump --exit-on-all-streams-close -r 0.1 127.0.0.1 20443 "https://[::]:20443/?s=10m"

This performance decrease is caused by the allocator which is now too
strict. It may cause buffer underrun frequently at the MUX layer when
the congestion window is too small, as new buffers cannot be allocated
until the current one is fully acknowledged. This resuls in transfers
with very bad throughput utilisation. The objective of this new serie of
patches is to relax some restrictions to permit QUIC MUX to allocate new
buffers more quickly, while preserving the initial limitation based on
congestion window size.

An interesting method for this is to notify QUIC MUX about newly
available room on individual ACK reception, without waiting for the full
bffer acknowledgement. This is easily implemented by adding a new
notify_room invokation in QUIC streamdesc layer on ACK reception.
However, ACK reception are handled in-order at the stream level. Out of
order ACKs are buffered and are not decounted for now. This will be
implemented in a future commit.

Note that for a single buffer instance, data can in parallel be written
by QUIC MUX and removed on ACK reception. This could cause room
notification to QUIC MUX layer to report invalid values. As such, ACK
reception are only accounted for released buffers. This ensures that
such buffers won't received any new data. In the same time, buffer room
is notified on release operation as it does not need acknowledgement.

This commit has permit to improve performance for the ngtcp2-client
scenario above. However, it is not yet sufficient enough for interop
goodput test.

9 months agoMINOR: quic: strengthen qc_release_frm()
Amaury Denoyelle [Thu, 3 Oct 2024 16:11:08 +0000 (18:11 +0200)] 
MINOR: quic: strengthen qc_release_frm()

quic_frame is the type used to represent frames emitted in a QUIC Tx
packet. Each frame is attached to a packet, and can also be linked to
other frames from the the same packet, or duplicated frames for
retransmission. As such, quic_frame free operation is a tedious process.

qc_release_frm() has been implemented to ensure quic_frame is always
properly freed after detaching from all its list attach point. One
particular point is to ensure that when a frame is released, the frame
origin and all origin copies, including the current <frm> are flagged as
acked and detached from the reflist. Add a BUG_ON() to ensure this loop
is properly conducted when dealing with the current <frm> instance.

9 months agoBUG/MINOR: stats: Fix the name for the total number of streams created
Christopher Faulet [Fri, 4 Oct 2024 13:44:39 +0000 (15:44 +0200)] 
BUG/MINOR: stats: Fix the name for the total number of streams created

Because of a copy/paste error, CurrStreams was reused by mistake. It should
be "CumStreams"

No backports needed.

9 months agoBUG/MAJOR: mux-quic: do not crash on empty STREAM frame emission
Amaury Denoyelle [Fri, 4 Oct 2024 08:10:37 +0000 (10:10 +0200)] 
BUG/MAJOR: mux-quic: do not crash on empty STREAM frame emission

Most of the time STREAM frames emitted by QUIC MUX have some data in it.
However, it is possible to use an empty frame when a delayed FIN must be
transferred.

Recently, QUIC MUX send callback notification has been refactored. Now,
this callback is blindly called by quic_conn lower layer each time a
STREAM frame is built into a newly Tx packet. QUIC MUX is responsible to
ensure the notified frame corresponds to newly emitted data or
retransmission. Offsets are used for this comparison, but this requires
special care for empty FIN frames.

Sadly, the comparison written to determine if an empty FIN frame was
sent for the first time or retransmitted is not correct. This caused
such frame to always be dismissed as retransmission in QUIC MUX sent
callback. This prevented the related QCS instance to be removed from the
send_list, causing qcc_io_send() to retry a new emission. This was
finally interrupted by the BUG_ON() assertion to prevent an infinite
loop.

Fix this crash by updating the condition in QUIC MUX send callback. For
empty STREAM frame, it is sufficient to check if QC_SF_FIN_STREAM was
already removed or not to detect a retransmission. Indeed, empty STREAM
frames are never used outside of delayed FIN reporting.

No need to backport. This crash was introduced in the current dev branch
by the following commit.
  d7f4e5abf0b7129329d0ea716c104474fd934bc6
  MEDIUM: quic: strengthen MUX send notification

9 months ago[RELEASE] Released version 3.1-dev9 v3.1-dev9
Willy Tarreau [Thu, 3 Oct 2024 15:47:33 +0000 (17:47 +0200)] 
[RELEASE] Released version 3.1-dev9

Released version 3.1-dev9 with the following main changes :
    - MINOR: tools: add minimal file name management
    - CLEANUP: stick-table: make the file location point to a global file name
    - MINOR: proxy: use the global file names for conf->file
    - CLEANUP: cfgparse: factor proxy vs log-forward collisions
    - BUG/MINOR: cfgparse: detect another uncaught case of duplicate defaults
    - MINOR: proxy: add a list of orphaned defaults sections
    - MEDIUM: cfgparse: drop duplicate named defaults sections after use
    - OPTIM: cfgparse: speed up duplicate server detection
    - MEDIUM: cfgparse: warn about deprecated use of duplicate server names
    - BUG/MINOR: server: shut down streams under thread isolation
    - BUG/MINOR: proxy: also make the cli and resolvers use the global name
    - REGTESTS: log: fix log-profile.vtc
    - MEDIUM: mailers: warn about deprecated legacy mailers
    - BUG/MEDIUM: cli: Be sure to catch immediate client abort
    - DEV: flags/applet: decode appctx flags
    - BUG/MEDIUM: cli: Deadlock when setting frontend maxconn
    - MINOR: log: fix indent in strm_log()
    - MINOR: log: introduce extra log profile steps
    - MINOR: log: handle extra log origins in _process_send_log_override()
    - MINOR: log: introduce log_orig flags
    - MINOR: log: explicitly handle extra log origins as error when relevant
    - MINOR: log: support extra log origins for '%OG' alias
    - MINOR: proxy: add log_steps struct member
    - MINOR: log: introduce "log-steps" proxy keyword
    - MINOR: log: add log_orig_proxy() helper function
    - MEDIUM: log: consider log-steps proxy setting for existing log origins
    - DOC: config: document proxy "log-steps" keyword
    - REGTESTS: add a test for proxy "log-steps"
    - Revert "BUG/MINOR: server: shut down streams under thread isolation"
    - MINOR: task: define two new one-shot events for use with WOKEN_OTHER or MSG
    - BUG/MEDIUM: stream: make stream_shutdown() async-safe
    - BUG/MINOR: server: make sure the HMAINT state is part of MAINT
    - BUG/MINOR: queue: make sure that maintenance redispatches server queue
    - MINOR: server: make srv_shutdown_sessions() call pendconn_redistribute()
    - BUILD: tools: only include execinfo.h for the real backtrace() function
    - MINOR: tools: do not attempt to use backtrace() on linux without glibc
    - OPTIM: channel: speed up co_getline()'s search of the end of line
    - OPTIM: stconn: Don't pretend mux have more data to deliver on EOI/EOS/ERROR
    - BUG/MINOR: mcli: Pretend the mux have more data to deliver between two commands
    - MINOR: action: Export release_expr_int_action() release function
    - MINOR: stream: Rely on a per-stream max connection retries value
    - MINOR: stream: Support dynamic changes of the number of connection retries
    - MINOR: stream/stats: Expose the current number of streams in stats
    - MINOR: stream/stats: Expose the total number of streams ever created in stats
    - BUG/MINOR: cfgparse-global: fix allowed args number for setenv
    - MINOR: cfgparse-global: add dedicated parser for *env keywords
    - MINOR: mux-quic: complete Tx infos for QCS dump
    - MINOR: quic: ensure txbuf realloc is only performed on empty buffer
    - MINOR: mux-quic: strengthen qcs_send_metadata() usage
    - MINOR: quic: remove unneeded notification of txbuf room
    - MINOR: quic: refactor MUX send notification
    - MEDIUM: quic: strengthen MUX send notification
    - MINOR: quic: refactor STREAM room notification
    - MINOR: quic: do not remove qc_stream_desc automatically on ACK handling
    - MINOR: quic: store streambuf in a streamdesc tree
    - MINOR: quic: move buffered ACK to streambuf
    - MEDIUM: quic: handle out-of-order ACK at streamdesc layer
    - MEDIUM: quic: refactor buffered STREAM ACK consuming
    - BUG/MEDIUM: queue: always dequeue the backend when redistributing the last server
    - MINOR: config/trace: Add a 'traces' section to declare debug traces
    - MINOR: trace: Be able to chain commands for a source in one line
    - MINOR: tcpcheck: Add support for an option host header value for httpchk option
    - BUG/MINOR: mux-h1: Fix condition to set EOI on SE during zero-copy forwarding
    - MINOR: mux-h1: Use a dedicated function to conditionnaly set EOI flag on SE
    - BUG/MINOR: http-ana: Disable fast-fwd for unfinished req waiting for upgrade
    - BUG/MINOR: mux-quic: fix crash on qcc_init() early return
    - BUG/MINOR: quic: fix trace on releasing STREAM frame after ack

9 months agoBUG/MINOR: quic: fix trace on releasing STREAM frame after ack
Amaury Denoyelle [Wed, 2 Oct 2024 15:04:19 +0000 (17:04 +0200)] 
BUG/MINOR: quic: fix trace on releasing STREAM frame after ack

Fix NULL argument pass to qc_release_frm(). This allows to give more
context on the traces inside it. Note that no crash occured as QUIC
traces always check validity on first arg before derefencing it.

No backport needed.

9 months agoBUG/MINOR: mux-quic: fix crash on qcc_init() early return
Amaury Denoyelle [Wed, 2 Oct 2024 08:21:02 +0000 (10:21 +0200)] 
BUG/MINOR: mux-quic: fix crash on qcc_init() early return

qcc_release() may be used in case qcc_init() cannot complete. In this
case, connection instance is NULL. As such, it cannot be dereferenced
without testing it first.

This should fix github coverity report #2739.

No backport needed.

9 months agoBUG/MINOR: http-ana: Disable fast-fwd for unfinished req waiting for upgrade
Christopher Faulet [Wed, 2 Oct 2024 07:57:34 +0000 (09:57 +0200)] 
BUG/MINOR: http-ana: Disable fast-fwd for unfinished req waiting for upgrade

If a request is waiting for a protocol upgrade but it is not finished, the
data fast-forwarding is disabled. Otherwise, the request analyzers will miss
the end of the message.

This case is possible since the commit 01fb1a54 ("BUG/MEDIUM: mux-h1/mux-h2:
Reject upgrades with payload on H2 side only"). Indeed, before, a protocol
upgrade was not allowed for request with payload. But it is now possible and
this comes with a side-effect. It is not really satisfying but for now there
is no other way to sync the muxes and the applicative stream. It seems to be
a reasonnable fix for now, waiting for a deeper refactoring.

This patch must be backported with the commit above.

9 months agoMINOR: mux-h1: Use a dedicated function to conditionnaly set EOI flag on SE
Christopher Faulet [Wed, 2 Oct 2024 07:36:15 +0000 (09:36 +0200)] 
MINOR: mux-h1: Use a dedicated function to conditionnaly set EOI flag on SE

The same conditions are evaluated in h1_process_demux() and h1_fastfwd() to
know if SE_FL_EOI flag must be set or not on the sedesc. So now, a dedicated
function is used.

9 months agoBUG/MINOR: mux-h1: Fix condition to set EOI on SE during zero-copy forwarding
Christopher Faulet [Wed, 2 Oct 2024 07:25:28 +0000 (09:25 +0200)] 
BUG/MINOR: mux-h1: Fix condition to set EOI on SE during zero-copy forwarding

During zero-copy data forwarding, the producer must set the EOI flag on the SE
when end of the message is reached. It is already done but there is a case where
this flag is set while it should not. When a request wants to perform a protocol
upgrade and it is waiting for the server response, the flag must not be set
because the HTTP message is finished but some data are possibly still expected,
depending on the server response. On a 101-switching-protocol, more data will be
sent because the producer is switch to TUNNEL state.

So, now, the right condition is used. In DONE state, SE_FL_EOI flag is set on the sedesc iff:

  - it is the response
  - it is the request and the response is also in DONNE state
  - it is a request but no a protocol upgrade nor a CONNECT

This patch must be backported as far as 2.9.

9 months agoMINOR: tcpcheck: Add support for an option host header value for httpchk option
Christopher Faulet [Tue, 1 Oct 2024 16:40:40 +0000 (18:40 +0200)] 
MINOR: tcpcheck: Add support for an option host header value for httpchk option

Support for headers and body hidden in the version for the "option httpchk"
directive was removed. However a Host header is mandatory for HTTP/1.1
requests and some servers may return an error if it is not set. For now, to
add it, an "http-check send" rule must be added. But it is not really handy
to use an extra config line for this purpose.

So now, it is possible to set the host header value, a log-format string, as
extra argument to "option httpchk" directive. It must be the fourth argument:

  option httpchk GET / HTTP/1.1 www.srv.com

While this patch is not a bug fix, it is simple enough to be backported if
necessary. On 2.9 and older, lf_init_expr() does not exist and LIST_INIT() must
be used instead.

9 months agoMINOR: trace: Be able to chain commands for a source in one line
Christopher Faulet [Tue, 1 Oct 2024 09:05:48 +0000 (11:05 +0200)] 
MINOR: trace: Be able to chain commands for a source in one line

In the configuration file or on the CLI, configuring traces for a specific
source is a bit painful because this must be done in several lines. Thanks
to this patch, it is now possible to fully configure traces for a source in
one line. For instance, the following on the CLI:

  trace h1 sink stderr; trace h1 level developer; trace h1 verbosity complete; trace h1 start now

can now be replaced by:

  trace h1 sink stderr level developer verbosity complete start now

The same is true for the 'trace' directives in the configuration file.

9 months agoMINOR: config/trace: Add a 'traces' section to declare debug traces
Christopher Faulet [Tue, 1 Oct 2024 06:48:38 +0000 (08:48 +0200)] 
MINOR: config/trace: Add a 'traces' section to declare debug traces

It is no longer supported to declare debug traces, via 'trace' directive, in
a global section. A 'traces' directive must be used instead. The syntax of
the 'trace' directive in these sections remains the same. But it is no
longer experimental.

The main reason for this change is to avoid to have a ring section defined
before a global one. Indeed, for now, forward declarations of ring sections
are not supported. So to configure traces, you had to add a ring section
before the global one defining the traces. Most of time, that meant to have
two global sections :

  global
    [...] # global settings

  ring <name>
    [...]

  global
    [...] # trace config

In addition, it will be possible to easily extend the traces section by
adding some new directives.

9 months agoBUG/MEDIUM: queue: always dequeue the backend when redistributing the last server
Willy Tarreau [Tue, 1 Oct 2024 16:57:51 +0000 (18:57 +0200)] 
BUG/MEDIUM: queue: always dequeue the backend when redistributing the last server

An interesting bug was revealed by commit 5541d4995d ("BUG/MEDIUM: queue:
deal with a rare TOCTOU in assign_server_and_queue()"). When shutting
down a server to redistribute its connections, no check is made on the
backend's queue. If we're turning off the last server and the backend
has pending connections, these ones will wait there till the queue
timeout. But worse, since the commit above, we can enter an endless loop
in the following situation:

  - streams are present in the backend's queue
  - streams are purged on the last server via srv_shutdown_streams()
  - that one calls pendconn_redistribute(srv) which does not purge
    the backend's pendconns
  - a stream performs some load balancing and enters assign_server_and_queue()
  - assign_server() is called in turn
  - the LB algo is non-deterministic and there are entries in the
    backend's queue. The function notices it and returns SRV_STATUS_FULL
  - assign_server_and_queue() calls pendconn_add() to add the connection
    to the backend's queue
  - on return, pendconn_must_try_again() is called, it figures there's
    no stream served anymore on the server nor the proxy, so it removes
    the pendconn from the queue and returns 1
  - assign_server_and_queue() loops back to the beginning to try again,
    while the conditions have not changed, resulting in an endless loop.

Ideally a change count should be used in the queues so that it's possible
to detect that some dequeuing happened and/or that a last stream has left.
But that wouldn't completely solve the problem that is that we must never
ever add to a queue when there's no server streams to dequeue the new
entries.

The current solution consists in making pendconn_redistribute() take care
of the proxy after the server in case there's no more server available on
the proxy. It at least ensures that no pending streams are left in the
backend's queue when shutting streams down or when the last server goes
down. The try_again loop remains necessary to deal with inevitable races
during pendconn additions. It could be limited to a few rounds, though,
but it should never trigger if the conditions are sufficient to permit
it to converge.

One way to reproduce the issue is to run a config with a single server
with maxconn 1 and plenty of threads, then run in loops series of:

 "disable server px/s;shutdown sessions server px/s;
  wait 100ms server-removable px/s; show servers conn px;
  enable server px/s"

on the CLI at ~10/s while injecting with around 40 concurrent conns at
40-100k RPS. In this case in 10s - 1mn the crash can appear with a
backtrace like this one for at least 1 thread:

  #0  pendconn_add (strm=strm@entry=0x17f2ce0) at src/queue.c:487
  #1  0x000000000064797d in assign_server_and_queue (s=s@entry=0x17f2ce0) at src/backend.c:1064
  #2  0x000000000064a928 in srv_redispatch_connect (s=s@entry=0x17f2ce0) at src/backend.c:1962
  #3  0x000000000064ac54 in back_handle_st_req (s=s@entry=0x17f2ce0) at src/backend.c:2287
  #4  0x00000000005ae1d5 in process_stream (t=t@entry=0x17f4ab0, context=0x17f2ce0, state=<optimized out>) at src/stream.c:2336

It's worth noting that other threads may often appear waiting after the
poller and one in server_atomic_sync() waiting for isolation, because
the event that is processed when shutting the server down is consumed
under isolation, and having less threads available to dequeue remaining
requests increases the probability to trigger the problem, though it is
not at all necessary (some less common traces never show them).

This should carefully be backported wherever the commit above was
backported.

9 months agoMEDIUM: quic: refactor buffered STREAM ACK consuming
Amaury Denoyelle [Mon, 30 Sep 2024 07:48:29 +0000 (09:48 +0200)] 
MEDIUM: quic: refactor buffered STREAM ACK consuming

For the moment, streamdesc layer can only deal with in-order ACK at the
stream level. Received out-of-order ACKs are buffered in a tree attached
to a streambuf instance.

Previously, caller of qc_stream_desc_ack() was responsible to implement
consumption of these buffered ACKs. Refactor this by implementing it
directly at the streamdesc layer within qc_stream_desc_ack(). This
simplifies quic_rx ACK handling and ensure buffered ACKs are consumed as
soon as possible.

9 months agoMEDIUM: quic: handle out-of-order ACK at streamdesc layer
Amaury Denoyelle [Mon, 30 Sep 2024 07:21:10 +0000 (09:21 +0200)] 
MEDIUM: quic: handle out-of-order ACK at streamdesc layer

qc_stream_desc_ack() is the entrypoint for streamdesc layer to handle a
new acknowledgement of previously emitted STREAM data.

Previously, it was only able to deal with in-order ACK offset. The
caller was responsible to buffer out-of-order ACKs. Change this by
dealing with the latter case directly in qc_stream_desc_ack(). This
notably simplify ACK handling in quic_rx module.

9 months agoMINOR: quic: move buffered ACK to streambuf
Amaury Denoyelle [Tue, 1 Oct 2024 09:13:41 +0000 (11:13 +0200)] 
MINOR: quic: move buffered ACK to streambuf

QUIC streamdesc layer is used to manage QUIC MUX stream txbuf data
storage until acknowledgment. Currently, it only supports in-order
acknowledgment at the stream level. This requires to be able to buffer
out-of-order ACKs until they can be handled.

Previously, these ACKs were stored in a tree to the streamdesc instance.
Move this indexed storage at the streambuf instance.

This commit is purely an architecture change. However, it will allow to
extend ACK management in future patches, such as the ability to merge
overlapping out-of-order ACKs.

9 months agoMINOR: quic: store streambuf in a streamdesc tree
Amaury Denoyelle [Tue, 1 Oct 2024 09:27:37 +0000 (11:27 +0200)] 
MINOR: quic: store streambuf in a streamdesc tree

qc_stream_desc layer is used by QUIC MUX to store emitted STREAM data
until their acknowledgement. Each stream with Tx capability can allocate
its own qc_stream_desc. In turn, each stream desc can have one or
multiple data buffers. This is useful when a MUX stream releases a
buffer and allocate a new one, to preserve bandwith without waiting to
receive all acknowledgement of the previous buffer.

Each buffer is encapsulated in a qc_stream_buf structure. Previously, it
was stored as a list into qc_stream_desc. Change this storage to use a
tree instead. Each buffer is indexed by their offset.

This commit does not introduce functional changes. However, this
rearchitecture will be necessary for future commit to extend ACK
management which require fetching individual buffer instance, not just
the first or last element of a streamdesc, by their offset.

9 months agoMINOR: quic: do not remove qc_stream_desc automatically on ACK handling
Amaury Denoyelle [Thu, 26 Sep 2024 14:14:40 +0000 (16:14 +0200)] 
MINOR: quic: do not remove qc_stream_desc automatically on ACK handling

qc_stream_desc_ack() is used to handle ACK received for STREAM frame. It
removes acknowledged data from their underlying buffer.

If all data were removed after ACK handling, qc_stream_desc instance
would automatically be freed at the end of qc_stream_desc_ack().
However, this renders the function complicated to use. Simplify this by
removing this automatic removal. Now, caller is responsible to check
after ACK handling if qc_stream_desc instance can be removed. This is
easily done using qc_stream_desc_done() helper.

9 months agoMINOR: quic: refactor STREAM room notification
Amaury Denoyelle [Wed, 25 Sep 2024 16:25:08 +0000 (18:25 +0200)] 
MINOR: quic: refactor STREAM room notification

qc_stream_desc is an intermediary layer between QUIC MUX and quic_conn.
It is a facility which permits to store data to emit and keep them for
retransmission until acknowledgment. This layer is responsible to notify
QUIC MUX each time a buffer is freed. This is necessary as MUX buffer
allocation is limited by the underlying congestion window size.

Refactor this to use a mechanism similar to send notification. A new
callback notify_room can now be registered to qc_stream_desc instance.
This is set by QUIC MUX to qmux_ctrl_room(). On MUX QUIC free, special
care is now taken to reset notify_room callback to NULL.

Thanks to this refactoring, further adjustment have been made to refine
the architecture. One of them is the removal of qc_stream_desc
QC_SD_FL_OOB_BUF, which is now converted to a MUX layer flag
QC_SF_TXBUF_OOB.

9 months agoMEDIUM: quic: strengthen MUX send notification
Amaury Denoyelle [Mon, 30 Sep 2024 12:39:15 +0000 (14:39 +0200)] 
MEDIUM: quic: strengthen MUX send notification

Previous commit implement a refactor of MUX send notification from
quic_conn layer. With this new architecture, a proper callback is
defined for each qc_stream_desc instance.

This architecture change allows to simplify notification from quic_conn
layer. First, ensure the MUX callback to properly ignore retransmission
of an already emitted frame. Luckily, this can be handled easily by
comparing offsets and FIN status. Also, each QCS instance can now be
unregistered from send notification just prior qc_stream_desc releasing.
This ensures a QCS is never manipulated from quic_conn after its
emission ending. Both these changes render the send notification more
robust. As a nice effect, flag QUIC_FL_CONN_TX_MUX_CONTEXT can be
removed as it is now unneeded.

9 months agoMINOR: quic: refactor MUX send notification
Amaury Denoyelle [Wed, 25 Sep 2024 15:55:10 +0000 (17:55 +0200)] 
MINOR: quic: refactor MUX send notification

For STREAM emission, MUX QUIC generates one or several frames and emit
them via qc_send_mux(). Lower layer may use them as-is, or split them to
lower chunk to fit in a QUIC packet. It is then responsible to notify
the MUX to report the amount of data sent.

Previously, this was done via a direct call from quic_conn to MUX using
qcc_streams_sent_done(). Modify this to have a better isolation accross
layers. Define a send callback handled by the qc_stream_desc instance.
This allows the MUX to register each QCS instance individually to the
renamved qmux_ctrl_send() which replaces qcc_streams_sent_done().

At quic_conn layer, qc_stream_desc_send() can be used now. This is a
wrapper to qc_stream_desc layer to invoke the send callback if
registered.

This mechanism of qc_stream_desc callback should be extended later to
implement other notifications accross the QUIC stack.

9 months agoMINOR: quic: remove unneeded notification of txbuf room
Amaury Denoyelle [Fri, 27 Sep 2024 13:31:21 +0000 (15:31 +0200)] 
MINOR: quic: remove unneeded notification of txbuf room

When a stream buffer is freed, qc_stream_desc notify MUX. This is useful
if MUX is waiting for Tx buffer allocation.

Remove this notification in qc_stream_desc(). This is because the
function is called when all stream data have been acknowledged and thus
notified. This function can also be called with some data
unacknowledged, but in this case this is only true just before
connection closure. As such, it is useful to notify the MUX in this
condition.

9 months agoMINOR: mux-quic: strengthen qcs_send_metadata() usage
Amaury Denoyelle [Tue, 1 Oct 2024 14:17:03 +0000 (16:17 +0200)] 
MINOR: mux-quic: strengthen qcs_send_metadata() usage

This function is reserved for QCS instance where no data was emitted.
A BUG_ON() ensures this by checking that streamdesc buf_list is empty.

However, this condition would not be enough if data were previously
emitted but already fully acknowledged. Thus, extend the condition by
also checking the streamdesc ack_offset is 0.

9 months agoMINOR: quic: ensure txbuf realloc is only performed on empty buffer
Amaury Denoyelle [Tue, 1 Oct 2024 08:55:40 +0000 (10:55 +0200)] 
MINOR: quic: ensure txbuf realloc is only performed on empty buffer

QUIC application protocol layer has the ability to either allocate a
standard buffer or a smaller one. The latter is useful when only small
data are transferred to prevent consuming too much of the QUIC MUX
buffer window.

This operation is performed using qc_stream_buf_realloc(). Add a new
BUG_ON() in it to ensure no data is present in the buffer. Indeed, this
would cause to data loss, or even crash when trying to acknowledge data.

Note that for the moment qc_stream_buf_realloc() is only use for HTTP/3
headers transmission, and this usage is conform to the new BUG_ON. This
commit is thus not a bug fix, but only to strengthen the API.

9 months agoMINOR: mux-quic: complete Tx infos for QCS dump
Amaury Denoyelle [Thu, 26 Sep 2024 08:49:54 +0000 (10:49 +0200)] 
MINOR: mux-quic: complete Tx infos for QCS dump

Complete debug info when a QCS instance is dumped either on traces or
show quic. Display the value of Tx offset both soft and real, along with
the current flow-control limit.

9 months agoMINOR: cfgparse-global: add dedicated parser for *env keywords
Valentine Krasnobaeva [Mon, 30 Sep 2024 12:27:08 +0000 (14:27 +0200)] 
MINOR: cfgparse-global: add dedicated parser for *env keywords

This commit prepares the config parser to support MODE_DISCOVERY and, thus,
refactored master-worker mode. The latter implies, that master process reads
only the 'DISCOVERY' tagged keywords from the global section and it must call
for this an appropriate keyword parser.

So, let's move the code, which parses *env keywords, from the global section
parser to its own keyword registered parser.

9 months agoBUG/MINOR: cfgparse-global: fix allowed args number for setenv
Valentine Krasnobaeva [Mon, 30 Sep 2024 13:29:47 +0000 (15:29 +0200)] 
BUG/MINOR: cfgparse-global: fix allowed args number for setenv

Keywords setenv and presetenv take 2 arguments: variable name and value.
So, the total number, that should be passed to alertif_too_many_args is 2
("setenv <name> <value>") instead of 3. For alertif_too_many_args the first
argument index is 0.

This should be backported in all stable versions.

9 months agoMINOR: stream/stats: Expose the total number of streams ever created in stats
Christopher Faulet [Fri, 27 Sep 2024 15:16:00 +0000 (17:16 +0200)] 
MINOR: stream/stats: Expose the total number of streams ever created in stats

A shared counter is added in the thread context to track the total number of
streams created on the thread. This number is then reported in stats. It
will be a useful information to diagnose some bugs.

9 months agoMINOR: stream/stats: Expose the current number of streams in stats
Christopher Faulet [Wed, 25 Sep 2024 07:59:11 +0000 (09:59 +0200)] 
MINOR: stream/stats: Expose the current number of streams in stats

A shared counter is added in the thread context to track the current number
of streams. This number is then reported in stats. It will be a useful
information to diagnose some bugs.

9 months agoMINOR: stream: Support dynamic changes of the number of connection retries
Christopher Faulet [Wed, 25 Sep 2024 13:10:08 +0000 (15:10 +0200)] 
MINOR: stream: Support dynamic changes of the number of connection retries

Thanks to the previous patch, it is now possible to add an action to
dynamically change the maxumum number of connection retires for a stream.
"set-retries" action may now be used to do so, from a "tcp-request content"
or a "http-request" rule. This action accepts an expression or an integer
between 0 and 100. The integer value is checked during the configuration
parsing and leads to an error if it is not in the expected range. However,
for the expression, the value is retrieve at runtime. So, invalid value are
just ignored.

Too high value is forbidden to avoid any trouble. 100 retries seems already
be an amazingly hight value. In addition, the option is only available on
backend or listen sections.

Because the max retries is limited to 100 at most, it can be stored as a
unsigned short. This save some space in the stream structure.

9 months agoMINOR: stream: Rely on a per-stream max connection retries value
Christopher Faulet [Wed, 25 Sep 2024 13:05:07 +0000 (15:05 +0200)] 
MINOR: stream: Rely on a per-stream max connection retries value

Instead of directly relying on the backend parameter to limit the number of
connection retries, we now use a per-stream value. This value is by default
inherited from the backend value when it is set. So for now, there is no
change except the stream value is used instead of the backend value. But
thanks to this change, it will be possible to dynamically change this value.

9 months agoMINOR: action: Export release_expr_int_action() release function
Christopher Faulet [Wed, 25 Sep 2024 13:03:43 +0000 (15:03 +0200)] 
MINOR: action: Export release_expr_int_action() release function

This function was only used by TCP actions and was private to tcp_act.c
file. However, it make sense to make it public to be used by any action
relying on an int-or-expression argument.

9 months agoBUG/MINOR: mcli: Pretend the mux have more data to deliver between two commands
Christopher Faulet [Fri, 27 Sep 2024 16:14:33 +0000 (18:14 +0200)] 
BUG/MINOR: mcli: Pretend the mux have more data to deliver between two commands

Since the commit "OPTIM: stconn: Don't pretend mux have more data to deliver
on EOI/EOS/ERROR", the SC no longer pretend its mux have more data to
deliver when one of EOI/EOS/ERROR flags are set on its sedesc.

However, for the master cli, it is an issue because any EOI/EOS at the end
of a command is in fact detected on the attempt to get the next command. To
do so, the stream is reset. Because if the commit above, the next received
is never performed. To fix the issue, when the stream is reset, the front SC
pretend its mux have more data to deliver.

This patch must only be bacported if the commit above is backported.

9 months agoOPTIM: stconn: Don't pretend mux have more data to deliver on EOI/EOS/ERROR
Christopher Faulet [Fri, 27 Sep 2024 13:05:11 +0000 (15:05 +0200)] 
OPTIM: stconn: Don't pretend mux have more data to deliver on EOI/EOS/ERROR

Doing some benchs on the 3.0, we encountered a small loss on requests/sec on
small objects compared to the 2.8 . After bisecting the issue, it appeared
that this was introduced when the mux-to-mux zero-copy data forwarding was
implemented in 2.9-dev8. Extra subscribes on receives at the end of the
message were responsible of the loss.

A basic configuration, sending H2 requests to a H1 server returning
responses without payload is enough to observe the issue. With the following
command, we can observe a huge increase of epoll_ctl calls on 2.9/3.x:

  h2load -c 100 -m 10 -n 100000 http://...

On 2.8 we have around 3200 calls to epoll_ctl against more than 20k on 3.1.

The fix seems obvious. After a receive, there is no reason to state a mux
have more data to deliver if EOI/EOS/ERROR flag was set on the
stream-endpoint descriptor. With this change, extra calls to epoll_ctl
disappear. However it is a sensitive part so it is important to keep an eye
on it and to not backport it.

Thanks to Willy and Emeric to have spot the issue.

9 months agoOPTIM: channel: speed up co_getline()'s search of the end of line
Willy Tarreau [Mon, 30 Sep 2024 09:19:20 +0000 (11:19 +0200)] 
OPTIM: channel: speed up co_getline()'s search of the end of line

Previously, co_getline() was essentially used for occasional parsing
in peers's banner or Lua, so it could afford to read one character at
a time. However now it's also used on the TCP log path, where it can
consume up to 40% CPU as mentioned in GH issue #2731. Let's speed it
up by using memchr() to look for the LF, and copying the data at once
using memcpy().

Previously it would take 2.44s to consume 1 GB of log on a single
thread of a Core i7-8650U, now it takes 1.56s (-36%).

9 months agoMINOR: tools: do not attempt to use backtrace() on linux without glibc
Willy Tarreau [Sun, 29 Sep 2024 07:46:10 +0000 (09:46 +0200)] 
MINOR: tools: do not attempt to use backtrace() on linux without glibc

The function is provided by glibc. Nothing prevents us from using our
own outside of glibc there (tested on aarch64 with musl). We still do
not enable it by default as we don't yet know if all archs work well,
but it's sufficient to pass USE_BACKTRACE=1 when building with musl to
verify it's OK.

9 months agoBUILD: tools: only include execinfo.h for the real backtrace() function
Willy Tarreau [Sun, 29 Sep 2024 07:37:16 +0000 (09:37 +0200)] 
BUILD: tools: only include execinfo.h for the real backtrace() function

No need to include this possibly non-existing file when using our own
backtrace() implementation, it's only needed for the libc-provided one.
Because of this it's currently not possible to build musl with backtrace
enabled.

9 months agoMINOR: server: make srv_shutdown_sessions() call pendconn_redistribute()
Willy Tarreau [Fri, 27 Sep 2024 17:01:38 +0000 (19:01 +0200)] 
MINOR: server: make srv_shutdown_sessions() call pendconn_redistribute()

When shutting down server sessions, the queue was not considered, which
is a problem if some element reached the queue at the moment the server
was going down, because there will be no more requests to kick them out
of it. Let's always make sure we scan the queue to kick these streams
out of it and that they can possibly find a more suitable server. This
may make a difference in the time it takes to shut down a server on the
CLI when lots of servers are in the queue.

It might be interesting to backport this to 3.0 but probably not much
further.

9 months agoBUG/MINOR: queue: make sure that maintenance redispatches server queue
Willy Tarreau [Fri, 27 Sep 2024 16:54:07 +0000 (18:54 +0200)] 
BUG/MINOR: queue: make sure that maintenance redispatches server queue

Turning a server to maintenance currently doesn't redispatch the server
queue unless there's an explicit "option redispatch" and no "option
persist", while the former has never really been the purpose of this
test. Better refine this so that forced maintenance also causes the
queue to be flushed, and possibly redispatched unless the proxy has
option persist. This way now when turning a server to maintenance,
the queue is immediately flushed and streams can decide what to do.

This can be backported, though there's no need to go far since it was
never directly reported and only noticed as part of debugging some
rare "shutdown sessions" strangeness, which it might participate to.

9 months agoBUG/MINOR: server: make sure the HMAINT state is part of MAINT
Willy Tarreau [Fri, 27 Sep 2024 16:38:35 +0000 (18:38 +0200)] 
BUG/MINOR: server: make sure the HMAINT state is part of MAINT

In 1.8 when adding "set server fqdn" with commit b418c1228c ("MINOR:
server: cli: Add server FQDNs to server-state file and stats socket."),
the HMAINT flag was not made part of the MAINT ones, so technically
speaking when changing the FQDN, the server is not completely considered
as in maintenance mode.

In its defense, the code location around that was completely messy, with
the aggregator flag being hidden between other values and purposely but
discretely ignoring one of the flags, so the comments were updated to
make the intent clearer (particularly regarding CMAINT which looked like
it was also forgotten while it was on purpose).

This can be backported anywhere.

9 months agoBUG/MEDIUM: stream: make stream_shutdown() async-safe
Willy Tarreau [Thu, 26 Sep 2024 16:30:36 +0000 (18:30 +0200)] 
BUG/MEDIUM: stream: make stream_shutdown() async-safe

The solution found in commit b500e84e24 ("BUG/MINOR: server: shut down
streams under thread isolation") to deal with inter-thread stream
shutdown doesn't work fine because there exists code paths involving
a server lock which can then deadlock on thread_isolate(). A better
solution then consists in deferring the shutdown to the stream itself
and just wake it up for that.

The only thing is that TASK_WOKEN_OTHER is a bit too generic and we
need to pass at least 2 types of events (SF_ERR_DOWN and SF_ERR_KILLED),
so we're now leveraging the new TASK_F_UEVT1 and _UEVT2 flags on the
task's state to convey these info. The caller only needs to wake the
task up with these flags set, and the stream handler will then finish
the job locally using stream_shutdown_self().

This needs to be carefully backported to all branches affected by the
dequeuing issue and containing any of the 5541d4995d ("BUG/MEDIUM:
queue: deal with a rare TOCTOU in assign_server_and_queue()"), and/or
b11495652e ("BUG/MEDIUM: queue: implement a flag to check for the
dequeuing").

9 months agoMINOR: task: define two new one-shot events for use with WOKEN_OTHER or MSG
Willy Tarreau [Thu, 26 Sep 2024 16:19:10 +0000 (18:19 +0200)] 
MINOR: task: define two new one-shot events for use with WOKEN_OTHER or MSG

TASK_WOKEN_MSG only says "someone sent you a message" but doesn't convey
any info about the message. TASK_WOKEN_OTHER says "you're woken for another
reason" but doesn't tell which one. Most often they're used as-is by the
task handlers to report very specific situations.

For some important control notifications, having the ability to modulate
the message a little bit is useful, so let's define two user event types
UEVT1 and UEVT2 to be used in conjunction with TASK_WOKEN_MSG or _OTHER
so that the application can know that a specific condition was explicitly
requested. It will be used this way:

  task_wakeup(s->task, TASK_WOKEN_MSG | TASK_F_UEVT1);
or:
  task_wakeup(s->task, TASK_WOKEN_OTHER | TASK_F_UEVT2);

Since events are cumulative, keep in mind not to consider a 3rd value
as the combination of EVT1+EVT2; these really mean that the two events
appeared (though in unspecified order).

9 months agoRevert "BUG/MINOR: server: shut down streams under thread isolation"
Willy Tarreau [Thu, 26 Sep 2024 16:36:17 +0000 (18:36 +0200)] 
Revert "BUG/MINOR: server: shut down streams under thread isolation"

This reverts commit b500e84e24fd19ccbcdf4fae5165aeb07e46bd67.

Thread isolation does not work well for this, there exists code paths
which already hold the server's lock and result in a deadlock. Let's
revert that and address it better without isolation.

9 months agoREGTESTS: add a test for proxy "log-steps"
Aurelien DARRAGON [Thu, 5 Sep 2024 14:48:09 +0000 (16:48 +0200)] 
REGTESTS: add a test for proxy "log-steps"

Now that proxy "log-steps" keyword was implemented and is usable since
("MEDIUM: log: consider log-steps proxy setting for existing log origins")
let's add some tests for it in reg-tests/log/log_profile.vtc.

9 months agoDOC: config: document proxy "log-steps" keyword
Aurelien DARRAGON [Thu, 5 Sep 2024 14:20:00 +0000 (16:20 +0200)] 
DOC: config: document proxy "log-steps" keyword

Now that "log-steps" proxy keyword is functional, let's add some
documentation and usage examples for it.

9 months agoMEDIUM: log: consider log-steps proxy setting for existing log origins
Aurelien DARRAGON [Wed, 4 Sep 2024 13:03:46 +0000 (15:03 +0200)] 
MEDIUM: log: consider log-steps proxy setting for existing log origins

During tcp/http transaction processing, haproxy may produce logs at
different steps during the processing (accept, connect, request,
response, close). But the behavior is hardly configurable because
haproxy will only emit a single log per transaction, and by default
it will try to produce the log once all log aliases or fetches used
in the logformat could be satisfied, which means the log is often
emitted during connection teardown, unless "option logasap" is used.

We were often asked to have a way to emit multiple logs for a single
transaction, like for instance emit log during accept, then request,
response and close for instance, see GH #401 for more context.

Thanks to "log-steps" keyword introduced by commit "MINOR: log:
introduce "log-steps" proxy keyword", it is now possible to explictly
configure when logs should be generated by haproxy when processing a
transaction. This commit adds the required checks so that log-steps
proxy option is properly considered for existing logs generated by
haproxy. If "log-steps" is not specified on the proxy, the old behavior
is preserved.

Note: a slight cpu overhead should only be visible when "log-steps"
keyword will be used due to the implementation relying on eb32 lookup
instead of basic bitfield check as described in "MINOR: proxy: add
log_steps struct member". However, the default behavior shouldn't be
affected.

When combining log-steps with log-profiles, user has the ability to
explicitly control how and when haproxy should generate logs during
requests handling.

9 months agoMINOR: log: add log_orig_proxy() helper function
Aurelien DARRAGON [Wed, 4 Sep 2024 10:15:21 +0000 (12:15 +0200)] 
MINOR: log: add log_orig_proxy() helper function

Function may be used on proxy where log-steps are used to check if a given
log origin should be handled or not.

9 months agoMINOR: log: introduce "log-steps" proxy keyword
Aurelien DARRAGON [Wed, 31 Jul 2024 15:06:57 +0000 (17:06 +0200)] 
MINOR: log: introduce "log-steps" proxy keyword

For now it is only available for proxies with frontend capability because
log-steps are only evaluated under sess_log() or strm_log() which
essentially focus on the frontend side when it comes to log settings so
it's better to keep it this way for better consistency, at least for now.

For now the setting does nothing (it is not considered during runtime),
it will be implemented and documented in upcoming commits.

9 months agoMINOR: proxy: add log_steps struct member
Aurelien DARRAGON [Tue, 20 Aug 2024 16:29:06 +0000 (18:29 +0200)] 
MINOR: proxy: add log_steps struct member

add proxy->conf.log_steps eb32 root tree which will be used to store the
log origin identifiers that should result in haproxy emitting a log as
configured by the user using upcoming "log-steps" proxy keyword.

It was chosen to use eb32 tree instead of simple bitfield because despite
the slight overhead it is more future-proof given that we already
implemented the prerequisites for seamless custom log origins registration
that will also be usable from "log-steps" proxy keyword.

9 months agoMINOR: log: support extra log origins for '%OG' alias
Aurelien DARRAGON [Tue, 10 Sep 2024 13:39:23 +0000 (15:39 +0200)] 
MINOR: log: support extra log origins for '%OG' alias

Following previous commits, let's improve log_orig_to_str() so that
extra log origins (registered through log_orig_register()) can be
translated to string from origin ID.

For that, it is required to add eb_32 tree node to log_origin struct in
order to enable quick integer lookup during runtime. Slow name lookup
using the list is acceptable for config parsing, but it is not the case
during runtime when log_orig_to_str() is expected to be used. Also, to
prevent duplicated info, get rid of ->id field and use ->tree.key instead

9 months agoMINOR: log: explicitly handle extra log origins as error when relevant
Aurelien DARRAGON [Wed, 31 Jul 2024 12:10:52 +0000 (14:10 +0200)] 
MINOR: log: explicitly handle extra log origins as error when relevant

Thanks to previous commit, we can know check for log_orig optional flags
in functions taking struct log_orig as parameter. Let's take this
opportunity to add the LOG_ORIG_FL_ERROR flag and check this flag at a
few places to handle the log message differently because if the flag is
set then the caller expects the log to be handled as an error explicitly.

e.g.: in _process_send_log_override(), if the flag is set, use the error
log format instead of the dedicated one.

9 months agoMINOR: log: introduce log_orig flags
Aurelien DARRAGON [Mon, 23 Sep 2024 15:22:45 +0000 (17:22 +0200)] 
MINOR: log: introduce log_orig flags

Rename 'enum log_orig' to 'enum log_orig_id', since this enum specifically
contains the log origin ids.

Add 'struct log_orig' which wraps 'enum log_orig' with optional flags
(no flags defined for now).

Add log_orig() helper func that takes id and flags as parameter and
returns log_orig struct initialized with input arguments.

Update functions taking log origin as parameter so they explicitly take
log orig id or log orig wrapper as argument depending on the level of
context expected by the function.

9 months agoMINOR: log: handle extra log origins in _process_send_log_override()
Aurelien DARRAGON [Wed, 31 Jul 2024 12:45:32 +0000 (14:45 +0200)] 
MINOR: log: handle extra log origins in _process_send_log_override()

Thanks to the previous commit, it is now possible to register additional
log origins that may be used from log-profile section as 'on' steps.

As such, let's make _process_send_log_override() function aware of them
by trying to lookup in the tree of extra logging steps in the default
switch-case catchall. If the log origin id matches with the id of the
extra logging step, we use the associated log format instead of the
"any" log format.

9 months agoMINOR: log: introduce extra log profile steps
Aurelien DARRAGON [Wed, 31 Jul 2024 09:15:13 +0000 (11:15 +0200)] 
MINOR: log: introduce extra log profile steps

add a way to register additional log origins using log_origin_register()
that may be used as log profile steps from log profile sections.

For now this does nothing as no extra origins are registered and extra log
origins are not yet considered for runtime logging paths.

When specifying an extra logging step for on <step> under log-profile
section, the logging step is stored within a binary tree for efficient
lookup during runtime. No performance impact should be expected if extra
log origins are not being used, and slight performance impact if extra
log origins are used.

Don't forget to update the documentation when new log origins are added
(both %OG log alias and on <step> log-profile keyword are concerned.

9 months agoMINOR: log: fix indent in strm_log()
Aurelien DARRAGON [Thu, 26 Sep 2024 08:11:41 +0000 (10:11 +0200)] 
MINOR: log: fix indent in strm_log()

8f34320e15 ("MINOR: log: provide log origin in logformat expressions
using '%OG'") caused wrong indent in strm_log()

9 months agoBUG/MEDIUM: cli: Deadlock when setting frontend maxconn
Oliver Dala [Wed, 25 Sep 2024 09:37:25 +0000 (11:37 +0200)] 
BUG/MEDIUM: cli: Deadlock when setting frontend maxconn

The proxy lock state isn't passed down to relax_listener
through dequeue_proxy_listeners, which causes a deadlock
in relax_listener when it tries to get that lock.

Backporting: Older versions didn't have relax_listener and directly called
resume_listener in dequeue_proxy_listeners. lpx should just be passed directly
to resume_listener then.

The bug was introduced in commit 001328873c352e5e4b1df0dcc8facaf2fc1408aa

[cf: This patch should fix the issue #2726. It must be backported as far as
2.4]

9 months agoDEV: flags/applet: decode appctx flags
Christopher Faulet [Tue, 24 Sep 2024 16:26:36 +0000 (18:26 +0200)] 
DEV: flags/applet: decode appctx flags

Decode APPCTX flags via appctx_show_flags() function.