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

[Jiangli Zhou]

Hi Florian,

On Thu, Jun 27, 2019 at 9:23 AM Florian Weimer <fweimer at redhat.com> wrote:
>
> * Jiangli Zhou:
>
> > Hi Florian,
> >
> > On Thu, Jun 27, 2019 at 8:14 AM Florian Weimer <fweimer at redhat.com> wrote:
> >>
> >> * Jiangli Zhou:
> >>
> >> > On Thu, Jun 27, 2019 at 12:57 AM Florian Weimer <fweimer at redhat.com> wrote:
> >> >>
> >> >> * Thomas Stüfe:
> >> >>
> >> >> >> I think
> >> >> >>
> >> >> >>   stack_adjust_size += guard_size;
> >> >> >>
> >> >> >> is redundant with the get_minstack-based adjustment because it is either
> >> >> >> included in it directly in the get_minstack value, or indirectly in the
> >> >> >> updated stack size computation in glibc (where it adds the guard size
> >> >> >> internally).
> >> >> >
> >> >> > Please do not change this!
> >> >> >
> >> >> > To me and I guess many others it is very important that nothing at all
> >> >> > changes if this workaround is off.
> >> >>
> >> >> That has already happened because recent glibc adds the guard size to
> >> >> the stack size internally now.
> >> >>
> >> >> But what I meant: only perform the += adjustment above for the
> >> >> !AdjustStackSizeForTLS case.
> >> >
> >> > I'm reluctant to do that, because it adds additional dependencies on
> >> > the glibc implementation. Subtracting the non-TLS size from the value
> >> > returned by __pthread_get_minstack may not be forward compatible in
> >> > the future when __pthread_get_minstack changes. I agree with Thomas to
> >> > keep the workaround as simple as possible.
> >>
> >> Even though it is an internal interface, __pthread_get_minstack will not
> >> change its competition.  It will be removed if glibc switches to
> >> allocating the TCB and TLS data off the stack.  But that that point, the
> >> size adjustment for TLS will no loger be necessary.
> >>
> >> PTHREAD_MIN_STACK could theoretically change (it is rather small on some
> >> architectures), but we have been extremely reluctant to make that
> >> change, due to dependencies like this one.  Rather, we have made the
> >> effective available stack size larger by other means, for the same value
> >> of PTHREAD_MIN_STACK.  (By not subtracting the guard size, in
> >> particular, and we also avoid calls in to the dynamic loader in more
> >> cases, which tend to require lots of stack space on several
> >> architectures.)
> >
> > I think the issue is if we subtract PTHREAD_STACK_MIN, do we want to
> > also subtract the _dl_pagesize? For glibc implementation older than
> > 2.27, do we need to handle the case for the guardsize separately? If
> > we do that, we would need conditional code to handle different glibc
> > versions. That seems to be worse than the approach of going with
> > _dl_get_tls_static_info??
>
> 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?

Thanks and best regards,
Jiangli
>
> Thanks,
> Florian