[Openmp-commits] [PATCH] D62488: Added propagation of not big initial stack size of master thread to workers.

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added inline comments.


================
Comment at: runtime/src/z_Linux_util.cpp:1844
+    if (__kmp_stksize > KMP_DEFAULT_STKSIZE * 64) // just a heuristics...
+      __kmp_stksize = KMP_DEFAULT_STKSIZE;
+    __kmp_check_stksize(&__kmp_stksize); // check value just in case
----------------
AndreyChurbanov wrote:
> jdoerfert wrote:
> > 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.
> 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.
> 
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 |




================
Comment at: runtime/test/misc_bugs/stack-propagate.c:62
+#endif // _WIN32
+  return 0;
+}
----------------
AndreyChurbanov wrote:
> jdoerfert wrote:
> > Can you `return 1` on failure?
> Sure, I will fix the test.
If the return values is checked then this change should be sufficient.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62488/new/

https://reviews.llvm.org/D62488