RFR: 8225035: Thread stack size issue caused by large TLS size

[Jiangli Zhou]

Hi David,

On Tue, Jun 11, 2019 at 12:56 AM David Holmes <david.holmes at oracle.com> wrote:
>
> Hi Jiangli,
>
> Some initial feedback ...
>
> On 11/06/2019 8:29 am, Jiangli Zhou wrote:
> > Hi David and Florian,
> >
> > This the a follow up of the discussion regarding the TLS (Thread Local
> > Storage) issue:
> > http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2019-May/034459.html.
> > The following is an updated patch with your suggestions incorporated:
> >
> > 1) Use __pthread_get_minstack
> > 2) By default only adjust the thread stack size if the
> > minstack-to-stacksize ratio is >= 10%
> > 3) Introduce a new command-line option (Linux only),
> > AdjustWithMinStackAt, which can be used to change the default 10%
> > ratio for triggering stack size adjustment
>
> I don't understand why the flag range is 0.0 to 50.0. If 0.1 = 10% then
> 50 = 500%, which means the minstack is 5x greater than the actual
> requested stack! The flag has to be limited between 0 and 100% AFAICS
> else the requested stack size is too small to accommodate the "min
> stack" and the attempt to create the thread will presumably fail. ??

The minstack size is added on top of the current calculated size and
the resulting value is passed to pthread_attr_setstacksize before the
thread creation. We will be able to create a thread with stack size
that's large enough. I initially thought about using [0%, 100%] range,
which is 0 to 1 as well. However, that doesn't apply to the cases
where the minstack size is greater then the requested size. Not sure
how rare those cases are in reality, it is fairly easy to create a
test case with a minstack > request_size. In the added jtreg test, the
minstack is 540k with the TLS values.

I picked 50.0, but I'm not too certain about using it as the upper
bound. We probably want to use a larger value to cover more cases.

>
>   827 static void pthread_get_minstack_init() {
>   828   // The code below uses the static symbol if available.
>   829   // If not, it will use dlsym to search for the symbol at runtime.
>   830   p_pthread_get_minstack = &__pthread_get_minstack;
>
> I don't understand this part. If at runtime the "static" symbol doesn't
> exist, won't this generate an error from the loader and prevent the VM
> from starting?

I see Florian has provided detailed information on this question (thanks!).

>
> Style nit: p_pthread_get_minstack
>
> Can you drop the "p" prefix and just use _pthread_get_minstack similar
> to other dynamically determined functions please.

Done.

>
> +     min_stack_size = (*p_pthread_get_minstack)(attr);
>
> When you have a function pointer you don't need to dereference it to
> invoke it, you can just do:
>
>    min_stack_size = p_pthread_get_minstack(attr);

Dropped dereferencing.

>
> +   if (p_pthread_get_minstack) {
>
> Style nit: Please avoid implicit booleans and check != NULL.

Done.

>
> I don't quite see what the test is doing - why does the Java code run
> "echo" ??

The TLS issue can be easily demonstrated by starting a process using
ProcessBuilder. It could be any valid command that can be passed to
ProcessBuilder. "echo" is inherited from the original test case
attached to JDK-8130425.

> I expected to see different values of AdjustWithMinStackAt
> being used and threads then checking whether their stacks have been
> adjusted or not; and in some cases to get errors. ??

It's a bit tricky, as we may not know the exact failure behavior
caused by insufficient stack space in different environments...

>
> > CSR: https://bugs.openjdk.java.net/browse/JDK-8225498
>
> I'm still thinking about the right name, type and range for the flag,
> which will impact the CSR.

Thanks! Please let me know your suggestions, I'll be more than happy
to incorporate them.

Best regards,
Jiangli

>
> Thanks,
> David
> -----
>
> > webrev: http://cr.openjdk.java.net/~jiangli/8225035/webrev.00/
> > bug: https://bugs.openjdk.java.net/browse/JDK-8225035
> >
> > Best regards,
> >
> > Jiangli
> >