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

[Thomas Stüfe]

On Thu, Jun 27, 2019 at 6:36 PM Jiangli Zhou <jianglizhou at google.com> wrote:

> 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?
>

Not exactly happy but if others are okay with this I do not want to hold up
this patch.

Can we factor this out and comment it clearly? E.g. like this:

// Returns the size of the TLS area glibc puts on thread stacks.
size_t os::Linux::get_tls_area_size() {
  // blabla... comment about what we do here - calling an undocumented
glibc function
  //  and reverting all operations it did to arrive at the tls_size result,
maybe with a link
  //  to the glibc sources
  return pthread_get_minstack() - pagesize - PTHREAD_STACK_MIN
}

Cheers, Thomas


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