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

Cheers,
Carlos.