diff options
| -rw-r--r-- | net-proxy/haproxy/ecc473b5299ccd0007a73ec4b4af099ac65e9c43.patch | 132 |
1 files changed, 132 insertions, 0 deletions
diff --git a/net-proxy/haproxy/ecc473b5299ccd0007a73ec4b4af099ac65e9c43.patch b/net-proxy/haproxy/ecc473b5299ccd0007a73ec4b4af099ac65e9c43.patch new file mode 100644 index 0000000..19acb57 --- /dev/null +++ b/net-proxy/haproxy/ecc473b5299ccd0007a73ec4b4af099ac65e9c43.patch @@ -0,0 +1,132 @@ +From ecc473b5299ccd0007a73ec4b4af099ac65e9c43 Mon Sep 17 00:00:00 2001 +From: Willy Tarreau <w@1wt.eu> +Date: Thu, 27 Jan 2022 15:46:19 +0100 +Subject: [PATCH] BUG/MAJOR: compiler: relax alignment constraints on certain + structures + +In github bug #1517, Mike Lothian reported instant crashes on startup +on RHEL8 + gcc-11 that appeared with 2.4 when allocating a proxy. + +The analysis brought us down to the THREAD_ALIGN() entries that were +placed inside the "server" struct to avoid false sharing of cache lines. +It turns out that some modern gcc make use of aligned vector operations +to manipulate some fields (e.g. memset() etc) and that these structures +allocated using malloc() are not necessarily aligned, hence the crash. + +The compiler is allowed to do that because the structure claims to be +aligned. The problem is in fact that the alignment propagates to other +structures that embed it. While most of these structures are used as +statically allocated variables, some are dynamic and cannot use that. +A deeper analysis showed that struct server does this, propagates to +struct proxy, which propagates to struct spoe_config, all of which +are allocated using malloc/calloc. + +A better approach would consist in usins posix_memalign(), but this one +is not available everywhere and will either need to be reimplemented +less efficiently (by always wasting 64 bytes before the area), or a +few functions will have to be specifically written to deal with the +few structures that are dynamically allocated. + +But the deeper problem that remains is that it is difficult to track +structure alignment, as there's no available warning to check this. +For the long term we'll probably have to create a macro such as +"struct_malloc()" etc which takes a type and enforces an alignment +based on the one of this type. This also means propagating that to +pools as well, and it's not a tiny task. + +For now, let's get rid of the forced alignment in struct server, and +replace it with extra padding. By punching 63-byte holes, we can keep +areas on separate cache lines. Doing so moderately increases the size +of the "server" structure (~+6%) but that's the best short-term option +and it's easily backportable. + +This will have to be backported as far as 2.4. + +Thanks to Mike for the detailed report. +--- + include/haproxy/compiler.h | 25 +++++++++++++++++++++++++ + include/haproxy/server-t.h | 8 ++++---- + 2 files changed, 29 insertions(+), 4 deletions(-) + +diff --git a/include/haproxy/compiler.h b/include/haproxy/compiler.h +index f18b5fa5cd..3943fe6459 100644 +--- a/include/haproxy/compiler.h ++++ b/include/haproxy/compiler.h +@@ -220,6 +220,17 @@ + #define HA_HAVE_CAS_DW + #endif + ++/*********************** IMPORTANT NOTE ABOUT ALIGNMENT **********************\ ++ * Alignment works fine for variables. It also works on types and struct * ++ * members by propagating the alignment to the container struct itself, * ++ * but this requires that variables of the affected type are properly * ++ * aligned themselves. While regular variables will always abide, those * ++ * allocated using malloc() will not! Most platforms provide posix_memalign()* ++ * for this, but it's not available everywhere. As such one ought not to use * ++ * these alignment declarations inside structures that are dynamically * ++ * allocated. If the purpose is only to avoid false sharing of cache lines * ++ * for multi_threading, see THREAD_PAD() below. * ++\*****************************************************************************/ + + /* sets alignment for current field or variable */ + #ifndef ALIGNED +@@ -294,6 +305,20 @@ + #endif + #endif + ++/* add optional padding of the specified size between fields in a structure, ++ * only when threads are enabled. This is used to avoid false sharing of cache ++ * lines for dynamically allocated structures which cannot guarantee alignment. ++ */ ++#ifndef THREAD_PAD ++# ifdef USE_THREAD ++# define __THREAD_PAD(x,l) char __pad_##l[x] ++# define _THREAD_PAD(x,l) __THREAD_PAD(x, l) ++# define THREAD_PAD(x) _THREAD_PAD(x, __LINE__) ++# else ++# define THREAD_PAD(x) ++# endif ++#endif ++ + /* The THREAD_LOCAL type attribute defines thread-local storage and is defined + * to __thread when threads are enabled or empty when disabled. + */ +diff --git a/include/haproxy/server-t.h b/include/haproxy/server-t.h +index 09f39333d6..e53d6121b4 100644 +--- a/include/haproxy/server-t.h ++++ b/include/haproxy/server-t.h +@@ -273,7 +273,7 @@ struct server { + /* The elements below may be changed on every single request by any + * thread, and generally at the same time. + */ +- ALWAYS_ALIGN(64); ++ THREAD_PAD(63); + struct eb32_node idle_node; /* When to next do cleanup in the idle connections */ + unsigned int curr_idle_conns; /* Current number of orphan idling connections, both the idle and the safe lists */ + unsigned int curr_idle_nb; /* Current number of connections in the idle list */ +@@ -288,14 +288,14 @@ struct server { + /* Element below are usd by LB algorithms and must be doable in + * parallel to other threads reusing connections above. + */ +- ALWAYS_ALIGN(64); ++ THREAD_PAD(63); + __decl_thread(HA_SPINLOCK_T lock); /* may enclose the proxy's lock, must not be taken under */ + unsigned npos, lpos; /* next and last positions in the LB tree, protected by LB lock */ + struct eb32_node lb_node; /* node used for tree-based load balancing */ + struct server *next_full; /* next server in the temporary full list */ + + /* usually atomically updated by any thread during parsing or on end of request */ +- ALWAYS_ALIGN(64); ++ THREAD_PAD(63); + int cur_sess; /* number of currently active sessions (including syn_sent) */ + int served; /* # of active sessions currently being served (ie not pending) */ + int consecutive_errors; /* current number of consecutive errors */ +@@ -303,7 +303,7 @@ struct server { + struct be_counters counters; /* statistics counters */ + + /* Below are some relatively stable settings, only changed under the lock */ +- ALWAYS_ALIGN(64); ++ THREAD_PAD(63); + + struct eb_root *lb_tree; /* we want to know in what tree the server is */ + struct tree_occ *lb_nodes; /* lb_nodes_tot * struct tree_occ */ |