DEBUG: counters: add the ability to enable/disable updating the COUNT_IF counters

These counters can have a noticeable cost on large machines, though not
dramatic. There's no single good choice to keep them enabled or disabled.
This commit adds multiple choices:
  - DEBUG_COUNTERS set to 2 will automatically enable them by default, while
    1 will disable them by default
  - the global "debug.counters on/off" will allow to change the setting at
    boot, regardless of DEBUG_COUNTERS as long as it was at least 1.
  - the CLI "debug counters on/off" will also allow to change the value at
    run time, allowing to observe a phenomenon while it's happening, or to
    disable counters if it's suspected that their cost is too high

Finally, the "debug counters" command will append "(stopped)" at the end
of the CNT lines when these counters are stopped.

Not that the whole mechanism would easily support being extended to all
counter types by specifying the types to apply to, but it doesn't seem
useful at all and would require the user to also type "cnt" on debug
lines. This may easily be changed in the future if it's found relevant.

diff --git a/Makefile b/Makefile
index 6b8fa5bca90df3021bcfb01d23d803903bf6cd5f..b87a0af7d04d0f85fdbb37e3ea8a29d4c3a8c39b 100644 (file)
--- a/Makefile
+++ b/Makefile
@@ -263,7 +263,7 @@ endif
 # DEBUG_NO_POOLS, DEBUG_FAIL_ALLOC, DEBUG_STRICT_ACTION=[0-3], DEBUG_HPACK,
 # DEBUG_AUTH, DEBUG_SPOE, DEBUG_UAF, DEBUG_THREAD, DEBUG_STRICT, DEBUG_DEV,
 # DEBUG_TASK, DEBUG_MEMORY_POOLS, DEBUG_POOL_TRACING, DEBUG_QPACK, DEBUG_LIST,
-# DEBUG_COUNTERS=[0-1], DEBUG_STRESS, DEBUG_UNIT.
+# DEBUG_COUNTERS=[0-2], DEBUG_STRESS, DEBUG_UNIT.
 DEBUG =
 
 #### Trace options

diff --git a/doc/configuration.txt b/doc/configuration.txt
index f5e98cb3865818d0618880e9f7808e10b12dadc8..9b4636a64501a230a80c1fa431bda4f3fa388c74 100644 (file)
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -1741,6 +1741,7 @@ The following keywords are supported in the "global" section :
 
  * Debugging
    - anonkey
+   - debug.counters
    - force-cfg-parser-pause
    - quiet
    - warn-blocked-traffic-after
@@ -4815,6 +4816,19 @@ anonkey <key>
   from the CLI command "set anon global-key". See also command line argument
   "-dC" in the management manual.
 
+debug.counters { on | off }
+  Enables ('on') or disables ('off') the updating of event counters in the
+  code. These are the counters reported under the type "CNT" in the CLI command
+  "debug counters". These counters are only available when the code was build
+  with DEBUG_COUNTERS set to a value 1 or above. With the value 1, the counters
+  are not updated by default ("debug.counters off"), and with value 2, they are
+  updated by default ("debug.counters on"). There is normally no reason to
+  change this setting unless a developer requests it, or unless it is suspected
+  to consume abnormal amounts of CPU (in which case a report to developers is
+  necessary with a dump of the counters). It is also possible to change this
+  status at run time using the "debug counters" CLI command. Please consult the
+  management manual.
+
 force-cfg-parser-pause <timeout>
   This command is pausing the configuration parser for <timeout> milliseconds.
   This is useful for development or for testing timeouts of init scripts,

diff --git a/doc/management.txt b/doc/management.txt
index 191c0c49937a22af06bdc1c806b2cbab98bcfedc..24636850022af1cba2eddb383976b8a6fea2827f 100644 (file)
--- a/doc/management.txt
+++ b/doc/management.txt
@@ -1999,7 +1999,7 @@ commit ssl crl-file <crlfile>
   See also "new ssl crl-file", "set ssl crl-file", "abort ssl crl-file" and
   "add ssl crt-list".
 
-debug counters [reset|show|all|bug|chk|cnt|glt|?]*
+debug counters [reset|show|on|off|all|bug|chk|cnt|glt|?]*
   List internal counters placed in the code, which may vary depending on some
   build options. Some of them depend on DEBUG_STRICT, others on DEBUG_COUNTERS.
   The command takes a combination of multiple arguments, some defining actions
@@ -2009,6 +2009,8 @@ debug counters [reset|show|all|bug|chk|cnt|glt|?]*
     - chk     enables listing the counters for CHECK_IF() statements
     - glt     enables listing the counters for COUNT_GLITCH() statements
     - all     enables showing counters that never triggered (value 0)
+    - off     action: disables updating of the COUNT_IF() counters
+    - on      action: enables updating of the COUNT_IF() counters
     - reset   action: resets all specified counters
     - show    action: shows all specified counters
 

diff --git a/include/haproxy/bug.h b/include/haproxy/bug.h
index f583e093dd3fa070e03a2a865a67e379713e1211..71d302e879c2cd8e4c81c691ff1cd3776d22e06c 100644 (file)
--- a/include/haproxy/bug.h
+++ b/include/haproxy/bug.h
@@ -161,6 +161,8 @@ static __attribute__((noinline,noreturn,unused)) void abort_with_line(uint line)
  * COUNT_IF() invocation requires a special section ("dbg_cnt") hence a modern
  * linker.
  */
+extern unsigned int debug_enable_counters;
+
 #if !defined(USE_OBSOLETE_LINKER)
 
 /* type of checks that can be verified. We cannot really distinguish between
@@ -225,17 +227,21 @@ extern __attribute__((__weak__)) struct debug_count __stop_dbg_cnt  HA_SECTION_S
 
 /* Matrix for DEBUG_COUNTERS:
  *    0 : only BUG_ON() and CHECK_IF() are reported (super rare)
- *    1 : COUNT_GLITCH() and COUNT_IF() are also reported (rare)
+ *    1 : COUNT_GLITCH() are also reported (rare)
+ *        COUNT_IF() are also reported only if debug_enable_counters was set
+ *    2 : COUNT_IF() are also reported unless debug_enable_counters was reset
  */
 
 /* Core of the COUNT_IF() macro, checks the condition and counts one hit if
- * true. It's only enabled at DEBUG_COUNTERS >= 1.
+ * true. It's only enabled at DEBUG_COUNTERS >= 1, and enabled by default if
+ * DEBUG_COUNTERS >= 2.
  */
 # if defined(DEBUG_COUNTERS) && (DEBUG_COUNTERS >= 1)
-#  define _COUNT_IF(cond, file, line, ...)                                     \
-       (unlikely(cond) ? ({                                                    \
-               __DBG_COUNT(cond, file, line, DBG_COUNT_IF, __VA_ARGS__);       \
-               1; /* let's return the true condition */                        \
+#  define _COUNT_IF(cond, file, line, ...)                                       \
+       (unlikely(cond) ? ({                                                      \
+               if (debug_enable_counters)                                        \
+                       __DBG_COUNT(cond, file, line, DBG_COUNT_IF, __VA_ARGS__); \
+               1; /* let's return the true condition */                          \
        }) : 0)
 # else
 #  define _COUNT_IF(cond, file, line, ...) DISGUISE(unlikely(cond) ? 1 : 0)

diff --git a/src/debug.c b/src/debug.c
index d3229e6b96d35861a46d722ba9be2a0fef2dd26b..ebf79cb7b102aba74893434b84468f6bf930c1b0 100644 (file)
--- a/src/debug.c
+++ b/src/debug.c
@@ -31,6 +31,7 @@
 #include <haproxy/api.h>
 #include <haproxy/applet.h>
 #include <haproxy/buf.h>
+#include <haproxy/cfgparse.h>
 #include <haproxy/cli.h>
 #include <haproxy/clock.h>
 #include <haproxy/debug.h>
@@ -163,6 +164,7 @@ struct post_mortem {
 
 unsigned int debug_commands_issued = 0;
 unsigned int warn_blocked_issued = 0;
+unsigned int debug_enable_counters = (DEBUG_COUNTERS >= 2);
 
 /* dumps a backtrace of the current thread that is appended to buffer <buf>.
  * Lines are prefixed with the string <prefix> which may be empty (used for
@@ -2254,6 +2256,14 @@ static int debug_parse_cli_counters(char **args, char *payload, struct appctx *a
                        action = 1;
                        continue;
                }
+               else if (strcmp(args[arg], "off") == 0) {
+                       action = 2;
+                       continue;
+               }
+               else if (strcmp(args[arg], "on") == 0) {
+                       action = 3;
+                       continue;
+               }
                else if (strcmp(args[arg], "all") == 0) {
                        ctx->show_all = 1;
                        continue;
@@ -2279,7 +2289,7 @@ static int debug_parse_cli_counters(char **args, char *payload, struct appctx *a
                        continue;
                }
                else
-                       return cli_err(appctx, "Expects an optional action ('reset','show'), optional types ('bug','chk','cnt','glt') and optionally 'all' to even dump null counters.\n");
+                       return cli_err(appctx, "Expects an optional action ('reset','show','on','off'), optional types ('bug','chk','cnt','glt') and optionally 'all' to even dump null counters.\n");
        }
 
 #if (DEBUG_STRICT > 0) || (DEBUG_COUNTERS > 0)
@@ -2299,6 +2309,12 @@ static int debug_parse_cli_counters(char **args, char *payload, struct appctx *a
                }
                return 1;
        }
+       else if (action == 2 || action == 3) { // off/on
+               if (!cli_has_level(appctx, ACCESS_LVL_ADMIN))
+                       return 1;
+               HA_ATOMIC_STORE(&debug_enable_counters, action == 3);
+               return 0;
+       }
 
        /* OK it's a show, let's dump relevant counters */
        return 0;
@@ -2340,10 +2356,11 @@ static int debug_iohandler_counters(struct appctx *appctx)
                }
 
                if (ptr->type < DBG_COUNTER_TYPES)
-                       chunk_appendf(&trash, "%-10u %3s %s:%d %s()%s%s\n",
+                       chunk_appendf(&trash, "%-10u %3s %s:%d %s()%s%s%s\n",
                                      ptr->count, bug_type[ptr->type],
                                      name, ptr->line, ptr->func,
-                                     *ptr->desc ? ": " : "", ptr->desc);
+                                     *ptr->desc ? ": " : "", ptr->desc,
+                                     (ptr->type == DBG_COUNT_IF && !debug_enable_counters) ? " (stopped)" : "");
 
                if (applet_putchk(appctx, &trash) == -1) {
                        ctx->start = ptr;
@@ -2807,6 +2824,36 @@ void list_unittests()
 
 #endif
 
+#if DEBUG_STRICT > 1
+/* config parser for global "debug.counters", accepts "on" or "off" */
+static int cfg_parse_debug_counters(char **args, int section_type, struct proxy *curpx,
+                                    const struct proxy *defpx, const char *file, int line,
+                                    char **err)
+{
+       if (too_many_args(1, args, err, NULL))
+               return -1;
+
+       if (strcmp(args[1], "on") == 0) {
+               HA_ATOMIC_STORE(&debug_enable_counters, 1);
+       }
+       else if (strcmp(args[1], "off") == 0)
+               HA_ATOMIC_STORE(&debug_enable_counters, 0);
+       else {
+               memprintf(err, "'%s' expects either 'on' or 'off' but got '%s'.", args[0], args[1]);
+               return -1;
+       }
+       return 0;
+}
+#endif
+
+/* config keyword parsers */
+static struct cfg_kw_list cfg_kws = {ILH, {
+#if DEBUG_STRICT > 1
+       { CFG_GLOBAL, "debug.counters",         cfg_parse_debug_counters      },
+#endif
+       { 0, NULL, NULL }
+}};
+INITCALL1(STG_REGISTER, cfg_register_keywords, &cfg_kws);
 
 /* register cli keywords */
 static struct cli_kw_list cli_kws = {{ },{