BUG/MEDIUM: thread: consider secondary threads as idle+harmless during boot
idle and harmless bits in the tgroup_ctx structure were not explicitly
set during boot.
| struct tgroup_ctx ha_tgroup_ctx[MAX_TGROUPS] = { };
As the structure is first statically initialized,
.threads_harmless and .threads_idle are automatically zero-
initialized by the compiler.
Unfortulately, this means that such threads are not considered idle
nor harmless by thread_isolate(_full)() functions until they enter
the polling loop (thread_harmless_now() and thread_idle_now() are
respectively called before entering the polling loop)
Because of this, any attempt to call thread_isolate() or thread_isolate_full()
during a startup phase with nbthreads >= 2 will cause thread_isolate to
loop until every secondary threads make it through their first polling loop.
If the startup phase is aborted during boot (ie: "-c" option to check the
configuration), secondary threads may be initialized but will never be started
(ie: they won't enter the polling loop), thus thread_isolate()
could would loop forever in such cases.
We can easily reveal the bug with this patch reproducer:
| diff --git a/src/haproxy.c b/src/haproxy.c
| index e91691658..0b733f6ee 100644
| --- a/src/haproxy.c
| +++ b/src/haproxy.c
| @@ -2317,6 +2317,10 @@ static void init(int argc, char **argv)
| if (pr || px) {
| /* At least one peer or one listener has been found */
| qfprintf(stdout, "Configuration file is valid\n");
| + printf("haproxy will loop...\n");
| + thread_isolate();
| + printf("we will never reach this\n");
| + thread_release();
| deinit_and_exit(0);
| }
| qfprintf(stdout, "Configuration file has no error but will not start (no listener) => exit(2).\n");
Now we start haproxy with a valid config:
$> haproxy -c -f valid.conf
Configuration file is valid
haproxy will loop...
^C
------------------------------------------------------------------------------
This did not cause any issue so far because no early deinit paths require
full thread isolation. But this may change when new features or requirements
are introduced, so we should fix this before it becomes a real issue.
To fix this, we explicitly assign .threads_harmless and .threads_idle
to .threads_enabled value in thread_map_to_groups() function during boot.
This is the proper place to do this since as long as .threads_enabled is not
explicitly set, its default value is also 0 (zero-initialized by the compiler)
code snippet from thread_isolate() function:
ulong te = _HA_ATOMIC_LOAD(&ha_tgroup_info[tgrp].threads_enabled);
ulong th = _HA_ATOMIC_LOAD(&ha_tgroup_ctx[tgrp].threads_harmless);
if ((th & te) == te)
break;
Thus thread_isolate(_full()) won't be looping forever in thread_isolate()
even if it were to be used before thread_map_to_groups() is executed.
No backport needed unless this is a requirement.
diff --git a/src/haproxy.c b/src/haproxy.c
index 3cf57e4..3c78285 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -3020,6 +3020,9 @@
ha_set_thread(data);
set_thread_cpu_affinity();
clock_set_local_source();
+ /* thread is started, from now on it is not idle nor harmless */
+ thread_harmless_end();
+ thread_idle_end();
/* Now, initialize one thread init at a time. This is better since
* some init code is a bit tricky and may release global resources
diff --git a/src/thread.c b/src/thread.c
index 00d9f9f..241377e 100644
--- a/src/thread.c
+++ b/src/thread.c
@@ -1199,6 +1199,16 @@
m = 0;
for (g = 0; g < global.nbtgroups; g++) {
ha_tgroup_info[g].threads_enabled = nbits(ha_tgroup_info[g].count);
+ /* for now, additional threads are not started, so we should
+ * consider them as harmless and idle.
+ * This will get automatically updated when such threads are
+ * started in run_thread_poll_loop()
+ * Without this, thread_isolate() and thread_isolate_full()
+ * will fail to work as long as secondary threads did not enter
+ * the polling loop at least once.
+ */
+ ha_tgroup_ctx[g].threads_harmless = ha_tgroup_info[g].threads_enabled;
+ ha_tgroup_ctx[g].threads_idle = ha_tgroup_info[g].threads_enabled;
if (!ha_tgroup_info[g].count)
continue;
m |= 1UL << g;