More dynamic debugging [was: [ANNOUNCE] haproxy-2.5.2]
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
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
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
❦ 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
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
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
❦ 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
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 a