From aa622b822bcb834405e251d007525f49dc8bfd42 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Thu, 28 Jan 2021 21:44:22 +0100 Subject: [PATCH] MINOR: activity: make profiling more manageable In 2.0, commit d2d3348ac ("MINOR: activity: enable automatic profiling turn on/off") introduced an automatic mode to enable/disable profiling. The problem is that the automatic mode automatically changes to on/off, which implied that the forced on/off modes aren't sticky anymore. It's annoying when debugging because as soon as the load decreases, profiling stops. This makes a small change which ought to have been done first, which consists in having two states for "auto" (auto-on, auto-off) to distinguish them from the forced states. Setting to "auto" in the config defaults to "auto-off" as before, and setting it on the CLI switches to auto but keeps the current operating state. This is simple enough to be backported to older releases if needed. --- include/haproxy/activity-t.h | 5 +++-- include/haproxy/activity.h | 22 ++++++++++------------ src/activity.c | 17 ++++++++++++----- 3 files changed, 25 insertions(+), 19 deletions(-) diff --git a/include/haproxy/activity-t.h b/include/haproxy/activity-t.h index 5301ceed6..072b9cac2 100644 --- a/include/haproxy/activity-t.h +++ b/include/haproxy/activity-t.h @@ -27,8 +27,9 @@ /* bit fields for the "profiling" global variable */ #define HA_PROF_TASKS_OFF 0x00000000 /* per-task CPU profiling forced disabled */ -#define HA_PROF_TASKS_AUTO 0x00000001 /* per-task CPU profiling automatic */ -#define HA_PROF_TASKS_ON 0x00000002 /* per-task CPU profiling forced enabled */ +#define HA_PROF_TASKS_AOFF 0x00000001 /* per-task CPU profiling off (automatic) */ +#define HA_PROF_TASKS_AON 0x00000002 /* per-task CPU profiling on (automatic) */ +#define HA_PROF_TASKS_ON 0x00000003 /* per-task CPU profiling forced enabled */ #define HA_PROF_TASKS_MASK 0x00000003 /* per-task CPU profiling mask */ /* per-thread activity reports. It's important that it's aligned on cache lines diff --git a/include/haproxy/activity.h b/include/haproxy/activity.h index 05ee77384..1a7361979 100644 --- a/include/haproxy/activity.h +++ b/include/haproxy/activity.h @@ -69,24 +69,22 @@ static inline void activity_count_runtime() } run_time = (before_poll.tv_sec - after_poll.tv_sec) * 1000000U + (before_poll.tv_usec - after_poll.tv_usec); - swrate_add(&activity[tid].avg_loop_us, TIME_STATS_SAMPLES, run_time); + run_time = swrate_add(&activity[tid].avg_loop_us, TIME_STATS_SAMPLES, run_time); - /* reaching the "up" threshold on average switches profiling to "on" - * when automatic, and going back below the "down" threshold switches - * to off. + /* In automatic mode, reaching the "up" threshold on average switches + * profiling to "on" when automatic, and going back below the "down" + * threshold switches to off. The forced modes don't check the load. */ if (!(task_profiling_mask & tid_bit)) { if (unlikely((profiling & HA_PROF_TASKS_MASK) == HA_PROF_TASKS_ON || - ((profiling & HA_PROF_TASKS_MASK) == HA_PROF_TASKS_AUTO && run_time >= up))) { - if (swrate_avg(activity[tid].avg_loop_us, TIME_STATS_SAMPLES) >= up) - _HA_ATOMIC_OR(&task_profiling_mask, tid_bit); - } + ((profiling & HA_PROF_TASKS_MASK) == HA_PROF_TASKS_AON && + swrate_avg(run_time, TIME_STATS_SAMPLES) >= up))) + _HA_ATOMIC_OR(&task_profiling_mask, tid_bit); } else { if (unlikely((profiling & HA_PROF_TASKS_MASK) == HA_PROF_TASKS_OFF || - ((profiling & HA_PROF_TASKS_MASK) == HA_PROF_TASKS_AUTO && run_time <= down))) { - if (swrate_avg(activity[tid].avg_loop_us, TIME_STATS_SAMPLES) <= down) - _HA_ATOMIC_AND(&task_profiling_mask, ~tid_bit); - } + ((profiling & HA_PROF_TASKS_MASK) == HA_PROF_TASKS_AOFF && + swrate_avg(run_time, TIME_STATS_SAMPLES) <= down))) + _HA_ATOMIC_AND(&task_profiling_mask, ~tid_bit); } } diff --git a/src/activity.c b/src/activity.c index 53721efcd..79aad3e1c 100644 --- a/src/activity.c +++ b/src/activity.c @@ -21,7 +21,7 @@ /* bit field of profiling options. Beware, may be modified at runtime! */ -unsigned int profiling = HA_PROF_TASKS_AUTO; +unsigned int profiling = HA_PROF_TASKS_AOFF; unsigned long task_profiling_mask = 0; /* One struct per thread containing all collected measurements */ @@ -49,7 +49,7 @@ static int cfg_parse_prof_tasks(char **args, int section_type, struct proxy *cur if (strcmp(args[1], "on") == 0) profiling = (profiling & ~HA_PROF_TASKS_MASK) | HA_PROF_TASKS_ON; else if (strcmp(args[1], "auto") == 0) - profiling = (profiling & ~HA_PROF_TASKS_MASK) | HA_PROF_TASKS_AUTO; + profiling = (profiling & ~HA_PROF_TASKS_MASK) | HA_PROF_TASKS_AOFF; else if (strcmp(args[1], "off") == 0) profiling = (profiling & ~HA_PROF_TASKS_MASK) | HA_PROF_TASKS_OFF; else { @@ -75,8 +75,14 @@ static int cli_parse_set_profiling(char **args, char *payload, struct appctx *ap } else if (strcmp(args[3], "auto") == 0) { unsigned int old = profiling; - while (!_HA_ATOMIC_CAS(&profiling, &old, (old & ~HA_PROF_TASKS_MASK) | HA_PROF_TASKS_AUTO)) - ; + unsigned int new; + + do { + if ((old & HA_PROF_TASKS_MASK) >= HA_PROF_TASKS_AON) + new = (old & ~HA_PROF_TASKS_MASK) | HA_PROF_TASKS_AON; + else + new = (old & ~HA_PROF_TASKS_MASK) | HA_PROF_TASKS_AOFF; + } while (!_HA_ATOMIC_CAS(&profiling, &old, new)); } else if (strcmp(args[3], "off") == 0) { unsigned int old = profiling; @@ -103,7 +109,8 @@ static int cli_io_handler_show_profiling(struct appctx *appctx) chunk_reset(&trash); switch (profiling & HA_PROF_TASKS_MASK) { - case HA_PROF_TASKS_AUTO: str="auto"; break; + case HA_PROF_TASKS_AOFF: str="auto-off"; break; + case HA_PROF_TASKS_AON: str="auto-on"; break; case HA_PROF_TASKS_ON: str="on"; break; default: str="off"; break; }