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

[Jiangli Zhou]

Hi Thomas,

Thanks for the feedback! I enjoyed reading the in-depth discussions from
everyone in this email thread this morning (and the follow-up discussion as
well). I'll address the comments from you, David and others in details.

Best regards,
Jiangli

On Wed, Jun 12, 2019 at 1:46 PM Thomas Stüfe <thomas.stuefe at gmail.com>
wrote:

> Hi Jiangli,
>
> after learning a lot of interesting things today about TLS :-) I took a
> look at your change.
>
> First another question, just to be sure I understand this: From what think
> I understood about TLS, and from the way you wrote your test (writing an
> own launcher and all) I assume the problem only happens for TLS of modules
> which are loaded at program start, right? So Libjvm.so and dynamically
> loaded JNI libs can allocate as many __thread space as they like, they
> would not go on the thread stacks? And this is only a problem when you
> embed the libjvm into other launchers which use a lot of TLS?
>
> ---
>
> I do not understand why the complicated dynamic lookup of
> __pthread_get_minstack. What is the advantage of this two-level approach vs
> just dlsym'ing the pointer like we do for all other dynamically loaded
> functions? Also, would this not cause problems when building for other libc
> implementations, e.g. muslc?
>
> And why RTLD_NEXT, why not RTLD_DEFAULT? Any special reason? Since that
> seems to be how we normally load dynamic symbols from the libc.
>
> About the fkt pointer, in contrast to David I would prefer to see it is a
> pointer, not a function, so your first name which I assume was "p_.." I
> would have liked more.
>
> ---
>
> +  size_t min_stack_size = align_up(pthread_get_minstack(&attr),
> vm_page_size());
>
> Why the align up? The only condition we have to meet is for the total
> stack size to be page aligned. On some platforms page size can be large (I
> think we have 64K pages on ppc) and TLS size could be smaller than that.
> Could you move the align up do right before the call to
> pthread_attr_setstacksize() (and probably remove the assert there)?
>
> ---
>
> What is the logic behind giving attr to pthread_get_minstack? If Florian
> is right and this:
> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2019-May/034593.html
> is the implementation, that parameter seems not to be used. In that case,
> just to be sure, I would feed it a dummy attr.
>
> ---
>
> I am not sure, but is adding the TLS sized buffer to just small thread
> stacks the right thing to do? Why it is okay for thread stacks slightly
> larger than the cut-off point to live without this additional space? Will
> they not as well be hurt by the large TLS subtraction from their stack
> sizes?
>
> Thank you!
>
> Cheers, Thomas
>
>
>
>
> On Tue, Jun 11, 2019 at 12:30 AM Jiangli Zhou <jianglizhou at google.com>
> 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
>>
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8225498
>> webrev: http://cr.openjdk.java.net/~jiangli/8225035/webrev.00/
>> bug: https://bugs.openjdk.java.net/browse/JDK-8225035
>>
>> Best regards,
>>
>> Jiangli
>>
>