From: Willy Tarreau Date: Wed, 19 Jul 2023 16:17:39 +0000 (+0200) Subject: BUG/MINOR: cpuset: remove the bogus "proc" from the cpu_map struct X-Git-Tag: v2.9-dev2~37 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=151f9a2808fcac5cd69f5ce32a79272f59055d73;p=thirdparty%2Fhaproxy.git BUG/MINOR: cpuset: remove the bogus "proc" from the cpu_map struct We're currently having a problem with the porting from cpu_map from processes to thread-groups as it happened in 2.7 with commit 5b09341c0 ("MEDIUM: cpu-map: replace the process number with the thread group number"), though it seems that it has deeper roots even in 2.0 and that it was progressively made worng over time. The issue stems in the way the per-process and per-thread cpu-sets were employed over time. Originally only processes were supported. Then threads were added after an optional "/" and it was documented that "cpu-map 1" is exactly equivalent to "cpu-map 1/all" (this was clarified in 2.5 by commit 317804d28 ("DOC: update references to process numbers in cpu-map and bind-process"). The reality is different: when processes were still supported, setting "cpu-map 1" would apply the mask to the process itself (and only when run in the background, which is not documented either and is also a bug for another fix), and would be combined with any possible per-thread mask when calculating the threads' affinity, possibly resulting in empty sets. However, "cpu-map 1/all" would only set the mask for the threads and not the process. As such the following: cpu-map 1 odd cpu-map 1/1-8 even would leave no CPU while doing: cpu-map 1/all odd cpu-map 1/1-8 even would allow all CPUs. While such configs are very unlikely to ever be met (which is why this bug is tagged minor), this is becoming quite more visible while testing automatic CPU binding during 2.9 development because due to this bug it's much more common to end up with incorrect bindings. This patch fixes it by simply removing the .proc entry from cpu_map and always setting all threads' maps. The process is no longer arbitrarily bound to the group 1's mask, but in case threads are disabled, we'll use thread 1's mask since it contains the configured CPUs. This fix should be backported at least to 2.6, but no need to insist if it resists as it's easier to break cpu-map than to fix an unlikely issue. --- diff --git a/include/haproxy/cpuset-t.h b/include/haproxy/cpuset-t.h index 2349876ea1..3a479e88a5 100644 --- a/include/haproxy/cpuset-t.h +++ b/include/haproxy/cpuset-t.h @@ -48,7 +48,6 @@ struct hap_cpuset { }; struct cpu_map { - struct hap_cpuset proc; /* list of CPU masks for the whole thread group */ struct hap_cpuset proc_t1 ; /* list of CPU masks for the 1st thread of the group */ struct hap_cpuset thread[MAX_THREADS_PER_GROUP]; /* list of CPU masks for the 32/64 threads of this group */ }; diff --git a/src/cfgparse-global.c b/src/cfgparse-global.c index 30334ba13c..87c597066c 100644 --- a/src/cfgparse-global.c +++ b/src/cfgparse-global.c @@ -1100,7 +1100,10 @@ int cfg_parse_global(const char *file, int linenum, char **args, int kwm) goto out; } *slash = '/'; - } + } else + thread = ~0UL; /* missing '/' = 'all' */ + + /* from now on, thread cannot be NULL anymore */ if (parse_cpu_set((const char **)args+2, &cpus, &errmsg)) { ha_alert("parsing [%s:%d] : %s : %s\n", file, linenum, args[0], errmsg); @@ -1131,35 +1134,21 @@ int cfg_parse_global(const char *file, int linenum, char **args, int kwm) ha_cpuset_assign(&cpus_copy, &cpus); - if (!thread) { - /* no thread set was specified, apply - * the CPU set to the whole group. - */ + /* a thread set is specified, apply the + * CPU set to these threads. + */ + for (j = n = 0; j < MAX_THREADS_PER_GROUP; j++) { + /* No mapping for this thread */ + if (!(thread & (1UL << j))) + continue; + if (!autoinc) - ha_cpuset_assign(&cpu_map[g].proc, &cpus); + ha_cpuset_assign(&cpu_map[g].thread[j], &cpus); else { - ha_cpuset_zero(&cpu_map[g].proc); + ha_cpuset_zero(&cpu_map[g].thread[j]); n = ha_cpuset_ffs(&cpus_copy) - 1; ha_cpuset_clr(&cpus_copy, n); - ha_cpuset_set(&cpu_map[g].proc, n); - } - } else { - /* a thread set is specified, apply the - * CPU set to these threads. - */ - for (j = n = 0; j < MAX_THREADS_PER_GROUP; j++) { - /* No mapping for this thread */ - if (!(thread & (1UL << j))) - continue; - - if (!autoinc) - ha_cpuset_assign(&cpu_map[g].thread[j], &cpus); - else { - ha_cpuset_zero(&cpu_map[g].thread[j]); - n = ha_cpuset_ffs(&cpus_copy) - 1; - ha_cpuset_clr(&cpus_copy, n); - ha_cpuset_set(&cpu_map[g].thread[j], n); - } + ha_cpuset_set(&cpu_map[g].thread[j], n); } } } diff --git a/src/cpuset.c b/src/cpuset.c index 76fad1fb21..4e1c537620 100644 --- a/src/cpuset.c +++ b/src/cpuset.c @@ -127,8 +127,6 @@ int cpu_map_configured(void) int grp, thr; for (grp = 0; grp < MAX_TGROUPS; grp++) { - if (ha_cpuset_count(&cpu_map[grp].proc)) - return 1; for (thr = 0; thr < MAX_THREADS_PER_GROUP; thr++) if (ha_cpuset_count(&cpu_map[grp].thread[thr])) return 1; diff --git a/src/haproxy.c b/src/haproxy.c index b20974199b..993fa39398 100644 --- a/src/haproxy.c +++ b/src/haproxy.c @@ -1533,7 +1533,6 @@ static void init_early(int argc, char **argv) int g, i; for (g = 0; g < MAX_TGROUPS; g++) { - ha_cpuset_zero(&cpu_map[g].proc); ha_cpuset_zero(&cpu_map[g].proc_t1); for (i = 0; i < MAX_THREADS_PER_GROUP; ++i) { ha_cpuset_zero(&cpu_map[g].thread[i]); @@ -3624,14 +3623,14 @@ int main(int argc, char **argv) in_parent = 1; } -#ifdef USE_CPU_AFFINITY - if (!in_parent && ha_cpuset_count(&cpu_map[0].proc)) { /* only do this if the process has a CPU map */ +#if !defined(USE_THREAD) && defined(USE_CPU_AFFINITY) + if (!in_parent && ha_cpuset_count(&cpu_map[0].thread[0])) { /* only do this if the process has a CPU map */ #if defined(CPUSET_USE_CPUSET) || defined(__DragonFly__) - struct hap_cpuset *set = &cpu_map[0].proc; + struct hap_cpuset *set = &cpu_map[0].thread[0]; sched_setaffinity(0, sizeof(set->cpuset), &set->cpuset); #elif defined(__FreeBSD__) - struct hap_cpuset *set = &cpu_map[0].proc; + struct hap_cpuset *set = &cpu_map[0].thread[0]; ret = cpuset_setaffinity(CPU_LEVEL_WHICH, CPU_WHICH_PID, -1, sizeof(set->cpuset), &set->cpuset); #endif } diff --git a/src/thread.c b/src/thread.c index 510a6f51ad..44cbc2e89e 100644 --- a/src/thread.c +++ b/src/thread.c @@ -277,9 +277,6 @@ void set_thread_cpu_affinity() return; /* Now the CPU affinity for all threads */ - if (ha_cpuset_count(&cpu_map[tgid - 1].proc)) - ha_cpuset_and(&cpu_map[tgid - 1].thread[ti->ltid], &cpu_map[tgid - 1].proc); - if (ha_cpuset_count(&cpu_map[tgid - 1].thread[ti->ltid])) {/* only do this if the thread has a THREAD map */ # if defined(__APPLE__) /* Note: this API is limited to the first 32/64 CPUs */