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.

diff --git a/include/haproxy/compiler.h b/include/haproxy/compiler.h
index f18b5fa5cd1a5425dd30f09c06c925f58b3a49e5..3943fe6459a03ec6ecc27c790d3a1bc554cb357d 100644 (file)
--- 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 09f39333d67ed4387b66c7a7780bee0bb5342ab8..e53d6121b422f0edcb5e84665205fccc9da41111 100644 (file)
--- 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 */