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

[Jiangli Zhou]

Hi Thomas,

Thanks for the additional comments!

On Thu, Jun 27, 2019 at 10:06 PM Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
>
> Hi Jiangli,
>
> http://cr.openjdk.java.net/~jiangli/8225035/webrev.03/make/test/JtregNativeHotspot.gmk.udiff.html
>
> Do we really need -lpthread?

Not really needed. Removed.

>
> http://cr.openjdk.java.net/~jiangli/8225035/webrev.03/src/hotspot/os/linux/os_linux.cpp.udiff.html
>
> +    tls_size = minstack_size - os::vm_page_size() - PTHREAD_STACK_MIN;
> +    assert(tls_size > 0, "unexpected size");
>
> tls_size is size_t is unsigned, so I do not think this works as intended. You may need to do something like:
>
> if (tls_size > os::vm_page_size() + PTHREAD_STACK_MIN) {
>    tls_size -= os::vm_page_size() - PTHREAD_STACK_MIN;
> }
>

You are absolutely right! Thanks for catching this! Didn't think
thoroughly when adding the assert last night. Your suggestion sounds
ok.

> --
>
> +  // Configure glibc guard page.
> +  pthread_attr_setguardsize(&attr, guard_size);
>
> Can you add a "must happen before calling get_static_tls_area_size()" to the comment ?

Done.

I'll post the updated webrev as part of the response to David's email. Thanks!

Best regards,
Jiangli
>
> ------------------
>
> All the rest looks good. Thank you for the comments, they are well written.
>
> Cheers, Thomas
>
> On Fri, Jun 28, 2019 at 5:58 AM Jiangli Zhou <jianglizhou at google.com> wrote:
>>
>> Updated webrev:
>> http://cr.openjdk.java.net/~jiangli/8225035/webrev.03/
>>
>> Thanks for everyone's contribution on carving out the current workaround!
>>
>> Best regards,
>> Jiangli
>>
>> On Thu, Jun 27, 2019 at 11:42 AM Jiangli Zhou <jianglizhou at google.com> wrote:
>> >
>> > Thank you Thomas and David! Glad to see that we are converging on an
>> > acceptable approach here. I'll try to factor in all the latest inputs
>> > from everyone and send out a new update.
>> >
>> > Thanks and best regards,
>> > Jiangli
>> >
>> > On Thu, Jun 27, 2019 at 11:13 AM David Holmes <david.holmes at oracle.com> wrote:
>> > >
>> > > Trimming ....
>> > >
>> > > On 27/06/2019 12:35 pm, Jiangli Zhou wrote:
>> > > > On Thu, Jun 27, 2019 at 9:23 AM Florian Weimer <fweimer at redhat.com> wrote:
>> > > >> I think you can handle the guard size in this way:
>> > > >>
>> > > >>    pthread_attr_setguardsize(&attr, guard_size);
>> > > >>
>> > > >>    size_t stack_adjust_size = 0;
>> > > >>    if (AdjustStackSizeForTLS) {
>> > > >>      size_t minstack_size = get_minstack(&attr);
>> > > >>      size_t tls_size = minstack_size - vm_page_size() - PTHREAD_STACK_MIN;
>> > > >>      // In glibc before 2.27, tls_size still includes guard_size.
>> > > >>      // In glibc 2.27 and later, guard_size is automatically
>> > > >>      // added to the stack size by pthread_create.
>> > > >>      // In both cases, the guard size is taken into account.
>> > > >>      stack_adjust_size += tls_size;
>> > > >>    } else {
>> > > >>      stack_adjust_size += guard_size;
>> > > >>    }
>> > > >
>> > > > Is the vm_page_size() counted for the dl_pagesize? As long as others
>> > > > are okay with the above suggested adjustment, it looks good to me.
>> > > > Thomas, David and others, any objection?
>> > >
>> > > I find the above acceptable. I've been waiting for the dust to settle.
>> > >
>> > > Thanks,
>> > > David
>> > >
>> > > > Thanks and best regards,
>> > > > Jiangli
>> > > >>
>> > > >> Thanks,
>> > > >> Florian