Re: [PATCH] nptl: add TLS/TCB sizes to requested stack size ratherthan subtract

["Carlos O'Donell" <carlos at systemhalted dot org>]

On Fri, Mar 23, 2012 at 4:25 PM, Carlos O'Donell
<carlos@systemhalted.org> wrote:
> On Fri, Mar 23, 2012 at 3:22 PM, Mike Frysinger <vapier@gentoo.org> wrote:
>> From: Ahmad Sharif <asharif@chromium.org>
>>
>> When Chrome is built with -fprofile-generate, pthread_create() fails
>> because the TLS size is larger than the stack size requested. ?This
>> is the same issue as reported in bugzilla a while ago.
>
> It fails because the TLS size is larger than the *default* machine
> stack size (minus the DTV and pthread descriptor)?
>
> ... and because the default machine stack size is not configurable,
> and computing the size of the TLS data is difficult you'd like the
> default behaviour to be that it just works (tm)?
>
> That sounds reasonable.
>
>> URL: http://sourceware.org/bugzilla/show_bug.cgi?id=11787
>> Signed-off-by: Ahmad Sharif <asharif@chromium.org>
>> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>> ---
>> ?nptl/ChangeLog ? ? ? | ? ?7 +++++++
>> ?nptl/allocatestack.c | ? ?6 +++++-
>> ?2 files changed, 12 insertions(+), 1 deletions(-)
>>
>> diff --git a/nptl/ChangeLog b/nptl/ChangeLog
>> index ad345f9..6817d97 100644
>> --- a/nptl/ChangeLog
>> +++ b/nptl/ChangeLog
>> @@ -1,3 +1,10 @@
>> +2012-03-23 ?Ahmad Sharif ?<asharif@chromium.org>
>> +
>> + ? ? ? [BZ #11787]
>> + ? ? ? * allocatestack.c (allocate_stack): Add __static_tls_size to
>> + ? ? ? size, and add TLS_TCB_SIZE to size when TLS_TCB_AT_TP is defined.
>> + ? ? ? Check attr->stacksize against just MINIMAL_REST_STACK.
>> +
>> ?2012-03-19 ?H.J. Lu ?<hongjiu.lu@intel.com>
>>
>> ? ? ? ?* sysdeps/x86_64/pthreaddef.h (CURRENT_STACK_FRAME): Use
>> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
>> index 79c4531..4b5998d 100644
>> --- a/nptl/allocatestack.c
>> +++ b/nptl/allocatestack.c
>> @@ -356,6 +356,10 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
>> ? /* Get the stack size from the attribute if it is set. ?Otherwise we
>> ? ? ?use the default we determined at start time. ?*/
>> ? size = attr->stacksize ?: __default_stacksize;
>> + ?size += __static_tls_size;
>> +#if TLS_TCB_AT_TP
>> + ?size += TLS_TCB_SIZE;
>> +#endif
>
> You must not adjust size here.
>
> Several other asserts depend on it being the original size passed in
> by the user e.g.
> ?375 ? ? ? assert (size > adj + TLS_TCB_SIZE);
> ...
> ?379 ? ? ? assert (size > adj);
> ...
> ?401 ? ? ? /* Remember the stack-related values. ?*/
> ?402 ? ? ? pd->stackblock = (char *) attr->stackaddr - size;
> ?403 ? ? ? pd->stackblock_size = size;
>
> If you need to adjust it do so *only* in the default else clause where
> we handle stack allocation.
>
> You should be using __pthread_get_minstack to compute the minimum
> stack and check to see if it's larger than the default and if so use
> it.
>
> I don't want to increase the default stack size by the static TLS
> block size since doing so could potentially break programs that are at
> the limit of their memory usage. Using __pthread_get_minstack gives
> you a minimum of PTHREAD_STACK_MIN to work with, and beyond that you
> should be allocating custom stacks.
>
> You should not second guess the user though, if attr->stacksize is set
> you should use it and *not* adjust anything.
>
> You'll see that __pthread_get_minstack takes into account __static_tls_size.
>
> Please rework the patch.
>
>> ? /* Get memory for the stack. ?*/
>> ? if (__builtin_expect (attr->flags & ATTR_FLAG_STACKADDR, 0))
>> @@ -365,7 +369,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
>> ? ? ? /* If the user also specified the size of the stack make sure it
>> ? ? ? ? is large enough. ?*/
>> ? ? ? if (attr->stacksize != 0
>> - ? ? ? ? && attr->stacksize < (__static_tls_size + MINIMAL_REST_STACK))
>> + ? ? ? ? && attr->stacksize < (MINIMAL_REST_STACK))
>
> Why are you changing this? It has nothing to do with your current patch.

Additional comments here: http://sourceware.org/bugzilla/show_bug.cgi?id=11787

Cheers,
Carlos.