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 2349876ea119319f120c2c16d40873d42df386ec..3a479e88a5d306f8e2bcbee2a12c5efcc603a336 100644 (file)
--- 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 30334ba13cbbe90cec0285989e491fccc8f3ad07..87c597066c8008c5b5e3c936cc0f9ca16ce62e0d 100644 (file)
--- 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 76fad1fb211ebff8ee0518edb3fa30828480e90c..4e1c537620a718fd072067858d6628ad2538f522 100644 (file)
--- 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 b20974199bf65e48a7614f0b2f77e00336eb7fd4..993fa3939822f230a082948a88b4b43062f4f350 100644 (file)
--- 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 510a6f51adb789accc1be5c4c83680b9e97f2053..44cbc2e89e63db7716410512ad8202fa48214b0c 100644 (file)
--- 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 */