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.
(cherry picked from commit ecc473b5299ccd0007a73ec4b4af099ac65e9c43)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>
(cherry picked from commit e70a3ba91c1c0c24b7c651a133836e5ab2b7ee9f)
Signed-off-by: Willy Tarreau <w@1wt.eu>
diff --git a/include/haproxy/compiler.h b/include/haproxy/compiler.h
index 352da8e..b975412 100644
--- a/include/haproxy/compiler.h
+++ b/include/haproxy/compiler.h
@@ -203,6 +203,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
@@ -277,6 +288,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 200385f..8882388 100644
--- a/include/haproxy/server-t.h
+++ b/include/haproxy/server-t.h
@@ -275,7 +275,7 @@
/* 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 */
@@ -290,14 +290,14 @@
/* 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 nbpend; /* number of pending connections */
@@ -307,7 +307,7 @@
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 */