From 2be9d1b4332d3b9b55a2d285e9610813100e235f Mon Sep 17 00:00:00 2001
From: Mark Reynolds <mreynolds@redhat.com>
Date: Mon, 22 Jun 2020 17:49:10 -0400
Subject: [PATCH] Issue 49256 - log warning when thread number is very
 different from autotuned value

Description:  To help prevent customers from setting incorrect values for
              the thread number it would be useful to warn them that the
              configured value is either way too low or way too high.

relates: https://pagure.io/389-ds-base/issue/49256

Reviewed by: firstyear(Thanks!)
---
 .../tests/suites/config/autotuning_test.py    | 28 +++++++++++++++
 ldap/servers/slapd/libglobs.c                 | 34 ++++++++++++++++++-
 ldap/servers/slapd/slap.h                     |  3 ++
 3 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/dirsrvtests/tests/suites/config/autotuning_test.py b/dirsrvtests/tests/suites/config/autotuning_test.py
index d1c751444..540761250 100644
--- a/dirsrvtests/tests/suites/config/autotuning_test.py
+++ b/dirsrvtests/tests/suites/config/autotuning_test.py
@@ -43,6 +43,34 @@ def test_threads_basic(topo):
     assert topo.standalone.config.get_attr_val_int("nsslapd-threadnumber") > 0
 
 
+def test_threads_warning(topo):
+    """Check that we log a warning if the thread number is too high or low
+
+    :id: db92412b-2812-49de-84b0-00f452cd254f
+    :setup: Standalone Instance
+    :steps:
+        1. Get autotuned thread number
+        2. Set threads way higher than hw threads, and find a warning in the log
+        3. Set threads way lower than hw threads, and find a warning in the log
+    :expectedresults:
+        1. Success
+        2. Success
+        3. Success
+    """
+    topo.standalone.config.set("nsslapd-threadnumber", "-1")
+    autotuned_value = topo.standalone.config.get_attr_val_utf8("nsslapd-threadnumber")
+
+    topo.standalone.config.set("nsslapd-threadnumber", str(int(autotuned_value) * 4))
+    time.sleep(.5)
+    assert topo.standalone.ds_error_log.match('.*higher.*hurt server performance.*')
+
+    if int(autotuned_value) > 1:
+        # If autotuned is 1, there isn't anything to test here
+        topo.standalone.config.set("nsslapd-threadnumber", "1")
+        time.sleep(.5)
+        assert topo.standalone.ds_error_log.match('.*lower.*hurt server performance.*')
+
+
 @pytest.mark.parametrize("invalid_value", ('-2', '0', 'invalid'))
 def test_threads_invalid_value(topo, invalid_value):
     """Check nsslapd-threadnumber for an invalid values
diff --git a/ldap/servers/slapd/libglobs.c b/ldap/servers/slapd/libglobs.c
index fbf90d92d..88676a303 100644
--- a/ldap/servers/slapd/libglobs.c
+++ b/ldap/servers/slapd/libglobs.c
@@ -4374,6 +4374,7 @@ config_set_threadnumber(const char *attrname, char *value, char *errorbuf, int a
 {
     int retVal = LDAP_SUCCESS;
     int32_t threadnum = 0;
+    int32_t hw_threadnum = 0;
     char *endp = NULL;
 
     slapdFrontendConfig_t *slapdFrontendConfig = getFrontendConfig();
@@ -4386,8 +4387,39 @@ config_set_threadnumber(const char *attrname, char *value, char *errorbuf, int a
     threadnum = strtol(value, &endp, 10);
 
     /* Means we want to re-run the hardware detection. */
+    hw_threadnum = util_get_hardware_threads();
     if (threadnum == -1) {
-        threadnum = util_get_hardware_threads();
+        threadnum = hw_threadnum;
+    } else {
+        /*
+         * Log a message if the user defined thread number is very different
+         * from the hardware threads as this is probably not the optimal
+         * value.
+         */
+        if (threadnum >= hw_threadnum) {
+            if (threadnum > MIN_THREADS && threadnum / hw_threadnum >= 4) {
+                /* We're over the default minimum and way higher than the hw
+                 * threads. */
+                slapi_log_err(SLAPI_LOG_NOTICE, "config_set_threadnumber",
+                        "The configured thread number (%d) is significantly "
+                        "higher than the number of hardware threads (%d).  "
+                        "This can potentially hurt server performance.  If "
+                        "you are unsure how to tune \"nsslapd-threadnumber\" "
+                        "then set it to \"-1\" and the server will tune it "
+                        "according to the system hardware\n",
+                        threadnum, hw_threadnum);
+            }
+        } else if (threadnum < MIN_THREADS) {
+            /* The thread number should never be less than the minimum and
+             * hardware threads. */
+            slapi_log_err(SLAPI_LOG_WARNING, "config_set_threadnumber",
+                    "The configured thread number (%d) is lower than the number "
+                    "of hardware threads (%d).  This will hurt server performance.  "
+                    "If you are unsure how to tune \"nsslapd-threadnumber\" then "
+                    "set it to \"-1\" and the server will tune it according to the "
+                    "system hardware\n",
+                    threadnum, hw_threadnum);
+            }
     }
 
     if (*endp != '\0' || errno == ERANGE || threadnum < 1 || threadnum > 65535) {
diff --git a/ldap/servers/slapd/slap.h b/ldap/servers/slapd/slap.h
index 8e76393c3..894efd29c 100644
--- a/ldap/servers/slapd/slap.h
+++ b/ldap/servers/slapd/slap.h
@@ -403,6 +403,9 @@ typedef void (*VFPV)(); /* takes undefined arguments */
 #define SLAPD_DEFAULT_PW_MAX_CLASS_CHARS_ATTRIBUTE 0
 #define SLAPD_DEFAULT_PW_MAX_CLASS_CHARS_ATTRIBUTE_STR "0"
 
+#define MIN_THREADS 16
+#define MAX_THREADS 512
+
 
 /* Default password values. */
 
-- 
2.26.2