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.
This commit is contained in:
Willy Tarreau 2023-07-19 18:17:39 +02:00
parent 67c99db0a7
commit 151f9a2808
5 changed files with 19 additions and 37 deletions

View File

@ -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 */
};

View File

@ -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);
}
}
}

View File

@ -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;

View File

@ -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
}

View File

@ -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 */