More dynamic debugging [was: [ANNOUNCE] haproxy-2.5.2]

[Willy Tarreau]

Hi,

On Wed, Feb 16, 2022 at 10:26:08PM +0100, Willy Tarreau wrote:
> Maybe we'll figure reasonable ways to turn some options to dynamic
> in the future (thinking about what's done with pools, I'm pretty sure
> that would be possible for almost half of the options, this would
> solve the problem).

I managed to work on this to turn almost all pool-related debug options
to boot-time options.

For this I extended the "-dM" command-line argument to also support
enabling/disabling/listing debug settings. The code was arranged so
that disabled options have no impact and the most common options have
almost no measurable impact. There's a "help" option which lists the
current settings (which are still preset based on DEBUG_* so that it
remains possible to set the desired default settings at build time):

  $ ./haproxy -dM,help
  -dM alone enables memory poisonning with byte 0x50 on allocation. A numeric
  value may be appended immediately after -dM to use another value (0 
supported).
  Then an optional list of comma-delimited keywords may be appended to set or
  clear some debugging options ('*' marks the current setting):
  
  set   cleardescription

-+-+-
  fail   |* no-fail| randomly fail allocations
* no-merge   |  merge  | disable merging of similar pools
  cold-first |* hot-first  | pick cold objects first
  integrity  |* no-integrity   | enable cache integrity checks
* no-global  |  global | disable global shared cache
  no-cache   |* cache  | disable thread-local cache
  caller |* no-caller  | save caller information in cache
* tag|  no-tag | add tag at end of allocated objects
* poison |  no-poison  | poison newly allocated objects

As it was easier to make measurements here, I could verify that, as
expected, poisonning, integrity checks and disabling the cache have an
important impact (halves the max req rate), and that the "tag" which
catches most misuses of pool_free() is almost non-measurable beyond
eating 8 extra bytes per object. I still need to mark the buffers
non-checkable (as there's no benefit in doing that and it's heavy).

Thus I updated the makefile to enable this by default (by setting
DEBUG_MEMORY_POOL), and DEBUG_STRICT (which enables BUG_ON()). I have
plans to improve the pool debugging, which may result in less options
but easier ones (i.e. instead of knowing how it's implemetend inside,
we would simply configure based on the use case). I don't know if this
will be done for 2.6. Regarding DEBUG_STRICT I would really like to
improve this to stuff many more of them in more sensitive areas but
they would not be enabled in default builds, only in CI and developers'.

Now asking reporters for more info will just be a matter of asking to
restart with "-dMno-merge,cold-first,integrity,caller,tag" instead of
rebuilding, and that will provide the highest level of detail we
currently support.

Unexpectedly, the main difficulty in this patch set was to re-arrange
the init code so that it was possible to move the command line parsing
earlier, and that we know pools configuration before they are created.

Thus, while I initially considered that it would be a childs game that
could be trivially backported, now I know it is not as trivial anymore.
I do think it remains trivially backportable to 2.5 and might eventually
be done, but at least I want to leave that under observation for some
time before engaging into this.

Thanks again for the discussion, it was useful ;-)
Willy

Re: [ANNOUNCE] haproxy-2.5.2

[William Lallemand]

Hi,

On Thu, Feb 17, 2022 at 08:25:39AM +0100, Willy Tarreau wrote:
> By the way Vincent, William figured that I missed a few patches during my
> long backport session yesterday. I'll have to check with him if they
> warrant another release or if they can wait for next one. Hence no need
> to rush on new packages yet ;-) I'll keep you updated whatever the
> outcome.
> 

I'll probably emit a 2.5.3 this evening or tomorrow, some of the
forgotten fixes could be bothersome for people trying to migrate in 2.5.

-- 
William Lallemand

Re: [ANNOUNCE] haproxy-2.5.2

[Willy Tarreau]

On Thu, Feb 17, 2022 at 07:57:30AM +0100, Vincent Bernat wrote:
>  ? 16 February 2022 22:15 +01, Willy Tarreau:
> 
> > That's exactly the sense behind the word "maybe" above, to open the
> > discussion :-)  Those with large buffers can definitely see a
> > difference. I've seen configs with WAF analysis using 1MB buffers,
> > and there the extra CPU usage will be noticeable, maybe 5-10%. My
> > impression is that the vast majority of users rely on distro packages
> > and are not sensitive to performance (typically sites like haproxy.org
> > where enabling everything has no measurable impact, when I'm lucky I
> > see 1% CPU). Those who deal with high levels of traffic tend to be
> > forced to follow stable updates more often, they'll typically build
> > from the Git tree, and are also more at ease with debugging options.
> > That was my reasoning, it may be wrong, and I perfectly understand
> > your point which is equally valid. And I'm not even asking for a
> > change, just saying "maybe it would be even better if".
> 
> For Debian, being a binary distribution, we cannot be flexible with the
> users. In the past, we were often told we were less performant than a
> source distribution because we didn't enable this or this optimization.
> Also, 1% CPU increase could also translate to increased latency.

I agree. I know that the vast majority of us do not care, but I know
a few places where that matters. Those who have to manage 100 LBs
definitely don't want to go to 101 just because we changed an option
(but arguably when performing major version upgrades, variations are
larger than that in both directions).

> As a comparison, we did not have memory cgroups in our kernels until the
> overhead was reduced quite significantly when not using them. On our
> side, we believe everyone is using Debian packages. ;-)

Oh I'm not surprised! I'll work more on the runtime configuration of
most of these settings, as I think the most expensive hence controversial
ones are the ones which should easily support adding a runtime test. For
the most sensitive parts (e.g. BUG_ON() in scheduler), that should still
be addressed at build time but on a case-by-case basis. I'll come back
trying to propose a better long-term solution for all this.

By the way Vincent, William figured that I missed a few patches during my
long backport session yesterday. I'll have to check with him if they
warrant another release or if they can wait for next one. Hence no need
to rush on new packages yet ;-) I'll keep you updated whatever the
outcome.

Cheers,
Willy

Re: [ANNOUNCE] haproxy-2.5.2

[Vincent Bernat]

 ❦ 16 February 2022 22:15 +01, Willy Tarreau:

> That's exactly the sense behind the word "maybe" above, to open the
> discussion :-)  Those with large buffers can definitely see a
> difference. I've seen configs with WAF analysis using 1MB buffers,
> and there the extra CPU usage will be noticeable, maybe 5-10%. My
> impression is that the vast majority of users rely on distro packages
> and are not sensitive to performance (typically sites like haproxy.org
> where enabling everything has no measurable impact, when I'm lucky I
> see 1% CPU). Those who deal with high levels of traffic tend to be
> forced to follow stable updates more often, they'll typically build
> from the Git tree, and are also more at ease with debugging options.
> That was my reasoning, it may be wrong, and I perfectly understand
> your point which is equally valid. And I'm not even asking for a
> change, just saying "maybe it would be even better if".

For Debian, being a binary distribution, we cannot be flexible with the
users. In the past, we were often told we were less performant than a
source distribution because we didn't enable this or this optimization.
Also, 1% CPU increase could also translate to increased latency.

As a comparison, we did not have memory cgroups in our kernels until the
overhead was reduced quite significantly when not using them. On our
side, we believe everyone is using Debian packages. ;-)
-- 
Be careful of reading health books, you might die of a misprint.
-- Mark Twain

Re: [ANNOUNCE] haproxy-2.5.2

[Willy Tarreau]

On Wed, Feb 16, 2022 at 09:57:45PM +0100, Christian Ruppert wrote:
> On 2022-02-16 19:08, Vincent Bernat wrote:
> > ? 16 February 2022 16:27 +01, Willy Tarreau:
> > 
> > > Maybe that would even be a nice improvement for distros to provide
> > > these
> > > by default starting with 2.6 or maybe even 2.5.
> > 
> > Why not enabling them directly on your side then? Are there some numbers
> > on the performance impact of these options? I am a bit uncomfortable
> > providing packages that perform slower than an upstream build.
> 
> Do you want all those options to be enabled in distro packages or just some
> specific?

I don't know, as I mentioned in my previous response, it could be just
some or even none for now, waiting for finer granularity.

> Esp. for the ones that make up to 1-2% CPU usage I'd second
> Vicent's idea of enabling it by default. So anybody has the option to
> disable it, if 1-2% or perhaps some ns/µs delay really matters that much.

The difficulty is that the ratio can vary based on some use cases (esp
with buffer sizes), and we need to keep a sweet spot between performance
and difficulty of deploying something for a particular user case. But
once these are split and re-arranged, it could become easier to decide.

I agree with Vincent in general about the fact that the distro should
not deviate much from the original setup, and we've even changed some
default options in the past to preserve this sane principle. For now
I'm just trying to gauge interest and starting to put the focus on these
possibilities for those who know they can easily afford a small perf
drop and who think that it will not change anything for them,
particularly if it shortens the life of the bugs they're facing. The
granularity remains a bit too coarse right now to ask users to decide
before testing, and for us to decide for all of them.

Maybe we'll figure reasonable ways to turn some options to dynamic
in the future (thinking about what's done with pools, I'm pretty sure
that would be possible for almost half of the options, this would
solve the problem).

I'm still interested in this discussion and your opinions on this
(and do not hesitate to violently disagree with me, my goal is to
figure what's best for most users, while avoiding traps for newcomers).

Cheers,
Willy

Re: [ANNOUNCE] haproxy-2.5.2

[Willy Tarreau]

Hi Vincent,

On Wed, Feb 16, 2022 at 07:08:38PM +0100, Vincent Bernat wrote:
>  ? 16 February 2022 16:27 +01, Willy Tarreau:
> 
> > Maybe that would even be a nice improvement for distros to provide these
> > by default starting with 2.6 or maybe even 2.5.
> 
> Why not enabling them directly on your side then? Are there some numbers
> on the performance impact of these options? I am a bit uncomfortable
> providing packages that perform slower than an upstream build.

That's exactly the sense behind the word "maybe" above, to open the
discussion :-)  Those with large buffers can definitely see a
difference. I've seen configs with WAF analysis using 1MB buffers,
and there the extra CPU usage will be noticeable, maybe 5-10%. My
impression is that the vast majority of users rely on distro packages
and are not sensitive to performance (typically sites like haproxy.org
where enabling everything has no measurable impact, when I'm lucky I
see 1% CPU). Those who deal with high levels of traffic tend to be
forced to follow stable updates more often, they'll typically build
from the Git tree, and are also more at ease with debugging options.
That was my reasoning, it may be wrong, and I perfectly understand
your point which is equally valid. And I'm not even asking for a
change, just saying "maybe it would be even better if".

What I'd like to do for 2.6 and beyond would be to have multiple levels
of protection/debugging classified by impacts. The vast majority of the
BUG_ON() we have have absolutely zero impact. Some in the past were
placed long after the code was written just to confirm that what was
understood was correct. Thus we couldn't enable them by default. Then
we started to place a lot more like plain assert() but still disabled
by default to avoid affecting performance. And due to this raising
concern about performance we don't put any into very sensitive places
where it could help for the vast majority of users. So my goal would
be to enable by default all those which have no visible impact, and
let users easily change them in case of trouble. Similarly some of the
DEBUG options will likely be enabled by default when the impact is
tiny. Nowadays for example I think we can afford to lose 8 bytes in
an allocated area to store the pointer to the last caller (especially
for free). This might possibly save one week to one month of round
trips in an issue, depending on the frequency of crashes for a given
report.

Once we manage to establish a balanced set of protection mechanisms
and debugging options, we can better document the ones that can save
the last few percent of performance or memory consumption, and the
ones that improve the accuracy of bug reports. In this case maybe
some users will more naturally enable some of them to get more solid
reports (we all prefer to produce undisputable bug reports, as there's
nothing more irritating than a developer expressing doubts about their
validity).

The options I mentioned today do not yet have this level of granularity,
they will have an impact, albeit a small one, hence why I'd prefer to
ask on a voluntary basis only. With some of the usual reporters, this is
something that is regularly done when asked, and I think that openly
indicating the costs and benefits around this allows us to progressively
get out of a debug-centric model and start to look into the direction of
a more generally proactive model.

There will always be exceptions anyway, but finer grained control is
necessary to enable such stuff by default in its current form.

Cheers,
Willy

Re: [ANNOUNCE] haproxy-2.5.2

[Vincent Bernat]

 ❦ 16 February 2022 16:27 +01, Willy Tarreau:

> Maybe that would even be a nice improvement for distros to provide these
> by default starting with 2.6 or maybe even 2.5.

Why not enabling them directly on your side then? Are there some numbers
on the performance impact of these options? I am a bit uncomfortable
providing packages that perform slower than an upstream build.
-- 
Don't stop with your first draft.
- The Elements of Programming Style (Kernighan & Plauger)

[ANNOUNCE] haproxy-2.5.2

[Willy Tarreau]

Hi,

HAProxy 2.5.2 was released on 2022/02/16. It added 44 new commits
after version 2.5.1.

This version addresses a few long-term bugs that have been keeping us
quite busy for far too long, but ultimately it's satisfying to know that
these ones are gone and that they won't be casting a doubt over every
single bug report.

The main issues fixed in this version are:
  - a tiny race condition in the scheduler affecting the rare multi-
threaded tasks. In some cases, a task could be finishing to run
on one thread and expiring on another one, just in the process
of being requeued to the position being in the process of being
calculated by the thread finishing with it. The most likely case
was the peers task disabling the expiration while waiting for other
peers to be locked, causing such a non-expirable task to be queued
and to block all other timers from expiring (typically health checks,
peers and resolvers, but others were affected). This could only
happen at high peers traffic rate but it definitely did. When built
with the suitable options such as DEBUG_STRICT it would immediately
crash (which is how it was detected). This bug was present since 2.0.

  - a bug in the Set-Cookie2 response parser may result in an infinite
loop triggering the watchdog if a server sends this while it belongs
to a backend configured with cookie persistence. Usually cookie-based
persistence is not used with untrusted servers, but if that was the
case, the following rule would be usable as a workaround for the time
it takes to upgrade:

 http-response del-header Set-Cookie2 

It reminded us that 2.5 years ago we were discussing about completely
dropping Set-Cookie2 which never succeeded in field, Tim has opened an
issue so that we don't forget to remove it after 2.6. This issue was
diagnosed, reported and fixed by Andrew McDermott and Grant Spence.
This bug was there since 1.9.

  - a bug in the SPOE error handling. When a connection to an agent dies,
there may still be requests pending that are tied to this connection.
The list of such requests is scanned so that they can be aborted,
except that the condition to scan the list was incorrect, and when
these requests were finally aborted upon processing timeout, they were
updating the memory area they used to point to, which could have been
reused for anything, causing random crashes very commonly seen in
libc's malloc/free va openssl, or haproxy pools with corrupted pointers.
In short, anyone using SPOE must absolutely update to apply the fix
otherwise any bug they face cannot be trusted as we know there's a rare
but real case of memory corruption there. This bug was present since
1.8.

  - there was a possible race condition on the listeners where it was
sometimes possible to wake up a temporarily paused listener just after
it had failed to rebind upon a failed attempt to reload. This would
access fdtab[-1] causing memory corruption or crashes. It's been there
since 2.2 but really started to have an effect with 2.3.

  - the master CLI could remain stuck forever if extra characters followed
by a shutdown were sent before the end of a response. In this case, each
such connection would remain unusable, and a script doing this would
face a connection failure after the 10th attempt (master's maxconn). A
few related issues could also cause it to loop forever (e.g. too long
pipelined requests, and empty buffers after wrapping).

  - the connection stopping list introduced in 2.4 to deal with idle
frontend connection on reloads missed a deletion, and could leave link
elements in the list after their containing structure was freed, causing
occasional crashes of the old process upon reload.

  - there is an ambiguity in the definition of dynamic table size updates
between the HTTP/2 spec (RFC7540) and the HPACK spec (RFC7541) which
can be read two ways. HAProxy and a few servers interpret it one way
and a few clients and other servers interpret it another way (and
generally clients win, as usual). One client, nghttp, enforces it
strictly, causing interoperability issues with haproxy and a few other
ones when the table size is set below 4096. We had a long discussion
with other participants of the HTTP working group to find the best
path forward that resulted in a nice update of the H2 spec that 
preserves the best interoperability with existing components while
clarifying all points. This update is present in this version and
will be progressively backported to older ones after some time (I
managed to mess up with the first attempt).

  - the HTTP client might not always start to send requests which were
ready in advance (before the connection is requested), it used to work
most of the time thanks to the scheduling of events but was a bit
fragile