#51151 Issue 51072 - Set the default minimum worker threads
Closed 3 years ago by spichugi. Opened 3 years ago by mreynolds.
mreynolds/389-ds-base issue51072  into  master

@@ -5905,11 +5905,6 @@ 

          retVal = util_get_hardware_threads();

      }

  

-     /* We *still* can't detect hardware threads. Okay, return 30 :( */

-     if (retVal <= 0) {

-         retVal = 30;

-     }

- 

      return retVal;

  }

  

file modified
+31 -14
@@ -26,6 +26,7 @@ 

  #include "snmp_collator.h"

  #include <sys/time.h>

  #include <sys/resource.h>

+ #include <errno.h>

  

  #define UTIL_ESCAPE_NONE 0

  #define UTIL_ESCAPE_HEX 1
@@ -40,6 +41,9 @@ 

  #define FILTER_BUF 128 /* initial buffer size for attr value */

  #define BUF_INCR 16    /* the amount to increase the FILTER_BUF once it fills up */

  

+ #define MIN_THREADS 16

+ #define MAX_THREADS 512

+ 

  /* slapi-private contains the pal. */

  #include <slapi-private.h>

  
@@ -1487,26 +1491,39 @@ 

  long

  util_get_hardware_threads(void)

  {

+     long threads = MIN_THREADS;

  #ifdef LINUX

      long hw_threads = sysconf(_SC_NPROCESSORS_ONLN);

-     long threads = 0;

-     slapi_log_err(SLAPI_LOG_TRACE, "util_get_hardware_threads", "Detected %lu hardware threads\n", threads);

-     if (hw_threads == 0) {

-         /* Error! */

-         threads = -1;

-     } else if (hw_threads < 512) {

-         threads = hw_threads;

-     } else {

-         /* Cap at 512 for now ... */

-         threads = 512;

+ 

+     slapi_log_err(SLAPI_LOG_TRACE, "util_get_hardware_threads",

+              "Detected %ld hardware threads\n", hw_threads);

+ 

+     if (hw_threads == -1) {

+         /* sysconf failed, use MIN_THREADS */

+         slapi_log_err(SLAPI_LOG_ERR, "util_get_hardware_threads",

+                 "Failed to get hardware threads.  Error (%d) %s\n",

+                 errno, slapd_system_strerror(errno));

+     } else if (hw_threads == 0) {

+         /* sysconf failed to find any processors, use MIN_THREADS */

+         slapi_log_err(SLAPI_LOG_ERR, "util_get_hardware_threads",

+                 "Cannot detect any hardware threads.\n");

+     } else if (hw_threads > MIN_THREADS) {

+         if (hw_threads < MAX_THREADS) {

+             threads = hw_threads;

+         } else {

+             /* Cap at 512 for now ... */

+             threads = MAX_THREADS;

+         }

      }

-     slapi_log_err(SLAPI_LOG_INFO, "util_get_hardware_threads", "Automatically configuring %lu threads\n", threads);

+ 

+     slapi_log_err(SLAPI_LOG_INFO, "util_get_hardware_threads", "Automatically configuring %ld threads\n", threads);

  

      return threads;

  #else

-     slapi_log_err(SLAPI_LOG_ERR, "util_get_hardware_threads", "ERROR: Cannot detect hardware threads on this platform. This is probably a bug!\n");

-     /* Can't detect threads on this platform! */

-     return -1;

+     slapi_log_err(SLAPI_LOG_ERR, "util_get_hardware_threads",

+             "ERROR: Cannot detect hardware threads on this platform. This is probably a bug!\n");

+     /* Can't detect threads on this platform!  Return the default thread number */

+     return threads;

  #endif

  }

  

Description:

Testing has shown that using current number of CPU cores to set the thread number gives the best performance, but when there are expensive operations total throughput drops.

We still need a minimum number of workers threads to handle a wide range of operations. We decided for now that the minimum should be 16 workers.

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

Only comment is to make 16 a #define MIN_THREADS because we could always keep this so that if we lower it later that we have say MIN_THREADS = 1, then we avoid the -1 case if we can't detect.

Could we also have a case to handle hw_threads == 0/negative which returns a min too, and gives the proper error?

Beside that little implementation detail it's good. Thanks :) Happy to re-read if you tweak it.

Only comment is to make 16 a #define MIN_THREADS because we could always keep this so that if we lower it later that we have say MIN_THREADS = 1, then we avoid the -1 case if we can't detect.

Sure

Could we also have a case to handle hw_threads == 0/negative which returns a min too, and gives the proper error?

It already handles this case. If the syscall fails it returns -1, but in the code, if its less than 16 we set it to 16 anyway. So this handles all the cases correctly. So the function will never return a value less than 16 to the caller. That's how I was able to remove the checks for -1

I think it's more, let's say we get the -1. Today we return min_threads, but I think we should have "on -1 make noise and return some error level of threads like 16 because we might be on a real server", vs "we have 4 cores, okay, let's set min to 16".

Imagine we knock min threads down to say ... 2? Then on a -1 we have a bad-time awaiting, so I want to make angry loud noises in this case :)

Thoughts? Perhaps just on the hw_threads < 1 case, we should raise an error, and move on with our lives?

Shouldn't be tested against 'threads' value ?
I agree with the use of a define MIN_THREADS or MIN_THREADNUMBER

1 new commit added

  • Define MAX and MIN threads, and improve logging
3 years ago

2 new commits added

  • Define MAX and MIN threads, and improve logging
  • Issue 51072 - Set the default minimum worker threads
3 years ago

Changes applied, please review...

rebased onto dc7bf4a

3 years ago

Pull-Request has been merged by mreynolds

3 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/4204

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi

3 years ago