Added propagation of not big initial stack size of master thread to workers.

[AndreyChurbanov]

Summary

Fix for https://bugs.llvm.org/show_bug.cgi?id=26540.

Currently implemented only for non-Windows 64-bit platforms (not requested on Windows so far;
32-bit systems have small memory, so it is tricky to find reasonable threshold to limit propagation).

For 64-bit the limit of 256 MB chosen (almost arbitrary, can be debatable...).

[AndreyChurbanov]

Moved declaration of rlim under #if to avoid "unused variable" warning.

[jdoerfert]

Generally, I think this is the right thing to do, minor questions though.

Could you change the commit subject to describe the change, the bug reference should be in the commit message.

I'm not familiar with the test system here but is it checking the output of the test looking for "passed" or how does it determine if the test succeeded?

runtime/src/z_Linux_util.cpp

1844 ↗

(On Diff #202424)

I would have assumed we have a limit and we propagate min(MASTER_STACK_SIZE, LIMIT_STACK_SIZE) so if you are a bit over the "limit" you don't fall back to the potentially way lower default.

runtime/test/misc_bugs/stack-propagate.c

62 ↗

(On Diff #202424)

Can you return 1 on failure?

[AndreyChurbanov]

runtime/src/z_Linux_util.cpp

1844 ↗

(On Diff #202424)

Not sure I got the question here.

Couple more details anyway. We've got request to propagate 8MB, because it was default limit on user's system and we had 4MB internal default in the library, that caused crash. Then we can as well propagate 16MB, 20MB, etc. But we cannot propagate 3GB because it slows down a lot of programs. So we have to stop propagation at some stack size limit. I haven't investigated what is the perfect value for upper bound we want to propagate. Would guess it can depend on a lot of factors... So just (arbitrary) chosen 256MB (haven't seen slowdowns with this value), and "a bit over" means "do not propagate" for me. Anyway we have standard external control, so if people need particular stack size for worker threads they can explicitly express that, same as set any stack size for master thread using system means and compiler/linker.

runtime/test/misc_bugs/stack-propagate.c

62 ↗

(On Diff #202424)

Sure, I will fix the test.

[AndreyChurbanov]

Test fixed: added "return 1" on possible failure of the test.

[AndreyChurbanov]

Regarding test system,

how does it determine if the test succeeded?

I am pretty sure it checks return code, 0 - success.

[jlpeyton]

I'm not familiar with the test system here but is it checking the output of the test looking for "passed" or how does it determine if the test succeeded?

For most of the OpenMP runtime tests, the LIT test system will check the return value of the test. If the // RUN: line (at the top) of a test has FileCheck in it then it is checking both the return values and the output for certain streams of tokens, this can be seen in the OMPT tests. Technically you can pipe the output into any tool but LLVM uses FileCheck for this usually.

[jdoerfert]

runtime/src/z_Linux_util.cpp

1844 ↗

(On Diff #202424)

My point is the odd behavior shown in the table below, assuming KMP_DEFAULT_STKSIZE * 64 = 256MB.

master stack size	thread stack size
255MB	255MB
256MB	256MB
257MB	4MB

If you change:

__kmp_stksize = rlim.rlim_cur;
// if system stack size is too big then don't propagate it to workers
if (__kmp_stksize > KMP_DEFAULT_STKSIZE * 64) // just a heuristics...
  __kmp_stksize = KMP_DEFAULT_STKSIZE;

to

// Limit the propagated stack size at a empirically choosen value.
__kmp_stksize = min(rlim.rlim_cur, KMP_DEFAULT_STKSIZE * 64);

you will get:

master stack size	thread stack size
255MB	255MB
256MB	256MB
257MB	256MB

runtime/test/misc_bugs/stack-propagate.c

62 ↗

(On Diff #202424)

If the return values is checked then this change should be sufficient.

[AndreyChurbanov]

runtime/src/z_Linux_util.cpp

1844 ↗

(On Diff #202424)

Thanks for explanation, got your point now.
I'll think a bit of this change.

[AndreyChurbanov]

Addressed Johannes' comment. At the same time I've reduced the upper bound of the size to be propagated to 64MB (from initial 256MB), because with this change more applications will be affected (those require big stack for master and small stack for workers).

[jdoerfert]

Thanks for the update. I inlined two more comments.

runtime/test/misc_bugs/stack-propagate.c

42 ↗

(On Diff #202919)

Do we need to print passed here?

[jdoerfert]

runtime/src/kmp_settings.cpp

294 ↗

(On Diff #202919)

[My initial comment got somehow lost:( ]

I have the same problem with this logic as I had before. I would like us to saturate at the edges and not jump to an arbitrary default value suddenly.

Assuming I got the min/max right, I would like:

*val = min(KMP_MAX_STKSIZE, max(KMP_MIN_STKSIZE, *val));

If there is a problem with that, please describe it, preferably also in the code above.

[AndreyChurbanov]

Review comments addressed:

• removed printf("passed") when the test is skipped;
• changed default size to lower/upper limit for consistency during size adjustment.

[jdoerfert]

LGTM, thanks.

openmp/trunk/runtime/src/kmp.h

 extern void __kmp_set_schedule(int gtid, kmp_sched_t new_sched, int chunk);
 extern void __kmp_get_schedule(int gtid, kmp_sched_t *sched, int *chunk);
 
 extern unsigned short __kmp_get_random(kmp_info_t *thread);
 extern void __kmp_init_random(kmp_info_t *thread);
 
 extern kmp_r_sched_t __kmp_get_schedule_global(void);
 extern void __kmp_adjust_num_threads(int new_nproc);
+extern void __kmp_check_stksize(size_t *val);
 
 extern void *___kmp_allocate(size_t size KMP_SRC_LOC_DECL);
 extern void *___kmp_page_allocate(size_t size KMP_SRC_LOC_DECL);
 extern void ___kmp_free(void *ptr KMP_SRC_LOC_DECL);
 #define __kmp_allocate(size) ___kmp_allocate((size)KMP_SRC_LOC_CURR)
 #define __kmp_page_allocate(size) ___kmp_page_allocate((size)KMP_SRC_LOC_CURR)
 #define __kmp_free(ptr) ___kmp_free((ptr)KMP_SRC_LOC_CURR)
 

openmp/trunk/runtime/src/kmp_settings.cpp

static void __kmp_stg_parse_bool(char const *name, char const *value,
   } else if (__kmp_str_match_false(value)) {
     *out = FALSE;
   } else {
     __kmp_msg(kmp_ms_warning, KMP_MSG(BadBoolValue, name, value),
               KMP_HNT(ValidBoolValues), __kmp_msg_null);
   }
 } // __kmp_stg_parse_bool
 
+// placed here in order to use __kmp_round4k static function
+void __kmp_check_stksize(size_t *val) {
+  // if system stack size is too big then limit the size for worker threads
+  if (*val > KMP_DEFAULT_STKSIZE * 16) // just a heuristics...
+    *val = KMP_DEFAULT_STKSIZE * 16;
+  if (*val < KMP_MIN_STKSIZE)
+    *val = KMP_MIN_STKSIZE;
+  if (*val > KMP_MAX_STKSIZE)
+    *val = KMP_MAX_STKSIZE; // dead code currently, but may work in future
+#if KMP_OS_DARWIN
+  *val = __kmp_round4k(*val);
+#endif // KMP_OS_DARWIN
+}
+
 static void __kmp_stg_parse_size(char const *name, char const *value,
                                  size_t size_min, size_t size_max,
                                  int *is_specified, size_t *out,
                                  size_t factor) {
   char const *msg = NULL;
 #if KMP_OS_DARWIN
   size_min = __kmp_round4k(size_min);
   size_max = __kmp_round4k(size_max);

openmp/trunk/runtime/src/z_Linux_util.cpp

 #if (KMP_ARCH_X86 || KMP_ARCH_X86_64)
   if (!__kmp_cpuinfo.initialized) {
     __kmp_query_cpuid(&__kmp_cpuinfo);
   }
 #endif /* KMP_ARCH_X86 || KMP_ARCH_X86_64 */
 
   __kmp_xproc = __kmp_get_xproc();
 
+#if ! KMP_32_BIT_ARCH
+  struct rlimit rlim;
+  // read stack size of calling thread, save it as default for worker threads;
+  // this should be done before reading environment variables
+  status = getrlimit(RLIMIT_STACK, &rlim);
+  if (status == 0) { // success?
+    __kmp_stksize = rlim.rlim_cur;
+    __kmp_check_stksize(&__kmp_stksize); // check value and adjust if needed
+  }
+#endif /* KMP_32_BIT_ARCH */
+
   if (sysconf(_SC_THREADS)) {
 
     /* Query the maximum number of threads */
     __kmp_sys_max_nth = sysconf(_SC_THREAD_THREADS_MAX);
     if (__kmp_sys_max_nth == -1) {
       /* Unlimited threads for NPTL */
       __kmp_sys_max_nth = INT_MAX;
     } else if (__kmp_sys_max_nth <= 1) {

openmp/trunk/runtime/test/misc_bugs/stack-propagate.c

+// RUN: %libomp-compile-and-run
+
+// https://bugs.llvm.org/show_bug.cgi?id=26540 requested
+// stack size to be propagated from master to workers.
+// Library implements propagation of not too big stack
+// for Linux x86_64 platform (skipped Windows for now).
+//
+// The test checks that workers can use more than 4MB
+// of stack (4MB - was historical default for
+// stack size of worker thread in runtime library).
+
+#include <stdio.h>
+#include <omp.h>
+#if !defined(_WIN32)
+#include <sys/resource.h> // getrlimit
+#endif
+
+#define STK 4800000
+
+double foo(int n, int th)
+{
+  double arr[n];
+  int i;
+  double res = 0.0;
+  for (i = 0; i < n; ++i) {
+    arr[i] = (double)i / (n + 2);
+  }
+  for (i = 0; i < n; ++i) {
+    res += arr[i] / n;
+  }
+  return res;
+}
+
+int main(int argc, char *argv[])
+{
+#if defined(_WIN32)
+  // don't test Windows
+  printf("stack propagation not implemented, skipping test...\n");
+  return 0;
+#else
+  int status;
+  double val = 0.0;
+  int m = STK / 8; // > 4800000 bytes per thread
+  // read stack size of calling thread, save it as default
+  struct rlimit rlim;
+  status = getrlimit(RLIMIT_STACK, &rlim);
+  if (sizeof(void *) > 4 &&                 // do not test 32-bit systems,
+      status == 0 && rlim.rlim_cur > STK) { // or small initial stack size
+#pragma omp parallel reduction(+:val)
+    {
+      val += foo(m, omp_get_thread_num());
+    }
+  } else {
+    printf("too small stack size limit (needs about 8MB), skipping test...\n");
+    return 0;
+  }
+  if (val > 0.1) {
+    printf("passed\n");
+    return 0;
+  } else {
+    printf("failed, val = %f\n", val);
+    return 1;
+  }
+#endif // _WIN32
+}