Re: [PATCH v2] Set default stack size from program environment

["Carlos O'Donell" <carlos at redhat dot com>]

On 02/05/2013 08:08 AM, Siddhesh Poyarekar wrote:
> This is take 2 to allow default stack size to be controlled
> independently of process stack by using an environment variable so
> that one need not change code for it.  Following my original
> submission[1], Roland suggested that I break down the patch and post
> them, with justifications for each of them.  The original intent of my
> submission was to get the environment variable so that default thread
> stack size can be controlled without having to change program code,
> similar to features that AIX and Solaris provide.  The new API
> functions were eye-candy to make the package complete, so I don't have
> a separate justification for them other than that they 'complete the
> implementation' so to speak.
> 
> As a result, this patch only implements the environment variable
> support.  I will post the API functions patch separately on top of
> this patch if this is accepted; I don't have any use for the API
> functions alone.  Ditto for the manual patch.
> 
> The patch also includes a test case to verify the new feature.  No
> regressions were reported in the testsuite due to this patch.
> 
> Siddhesh
> 
> [1] http://sourceware.org/ml/libc-alpha/2013-01/msg00572.html

Given that we have no easy way to set the thread stack size
default without recompiling everything, I think it is sufficient
rationale to request an environment variable that sets the default.

The counter argument is that users should have been smart enough
to build a parameter reading library that initializes all defaults
for each thread from stored values. So that is well and good, but
what about users that just used the system defaults? The defaults
that we as developers have chosen are, without proof, or documentation
for their choice. I won't say they are bad defaults, just that we
don't provide any proof or description for which workloads they were
tuned.

So we come to a junction where a user, accepting our defaults,
runs into a new situation where they would like to adjust the
defaults *without* needing to recompile all of their code.

The patch does slow-down program startup, but the environment
tables are in memory and no other access is required to initialize
the thread library. I am happy with runtime customization as long
as it doesn't require needing to read system configuration files,
stat files, or other similarly costly operations.

Your patch uses in-memory environment variables to pass runtime
tuning information into the starting application which is useful.

Am I biased? Yes. I am for tuning of runtime behaviour.

> ChangeLog:
> 
> 	* csu/libc-start.c (__pthread_initialize_minimal): Change
> 	function arguments.
> 	* csu/libc-tls.c (__pthread_initialize_minimal): Likewise.
> 
> nptl/ChangeLog:
> 
> 	* Makefile (tests): Add tst-pthread-stack-env.
> 	(tst-pthread-stack-env-ENV): Set environment for test.
> 	* nptl-init.c (set_default_stacksize): New function.
> 	(__pthread_initialize_minimal_internal): Accept ARGC, ARGV and
> 	ENVP.  Initialize __ENVIRON and set __DEFAULT_STACKSIZE.
>         * tst-pthread-stack-env.c: New test case.
> 
> 
> diff --git a/csu/libc-start.c b/csu/libc-start.c
> index d4a135f..edd267c 100644
> --- a/csu/libc-start.c
> +++ b/csu/libc-start.c
> @@ -31,7 +31,7 @@ extern int __libc_multiple_libcs;
>  #include <tls.h>
>  #ifndef SHARED
>  # include <dl-osinfo.h>
> -extern void __pthread_initialize_minimal (void);
> +extern void __pthread_initialize_minimal (int, char **, char **);

OK.

>  # ifndef THREAD_SET_STACK_GUARD
>  /* Only exported for architectures that don't store the stack guard canary
>     in thread local area.  */
> @@ -174,7 +174,7 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>    /* Initialize the thread library at least a bit since the libgcc
>       functions are using thread functions if these are available and
>       we need to setup errno.  */
> -  __pthread_initialize_minimal ();
> +  __pthread_initialize_minimal (argc, argv, __environ);

OK.

>  
>    /* Set up the stack checker's canary.  */
>    uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
> diff --git a/csu/libc-tls.c b/csu/libc-tls.c
> index 90daaa6..f51a711 100644
> --- a/csu/libc-tls.c
> +++ b/csu/libc-tls.c
> @@ -243,7 +243,7 @@ _dl_tls_setup (void)
>     not used.  */
>  void
>  __attribute__ ((weak))
> -__pthread_initialize_minimal (void)
> +__pthread_initialize_minimal (int argc, char **argv, char **envp)

OK.

>  {
>    __libc_setup_tls (TLS_INIT_TCB_SIZE, TLS_INIT_TCB_ALIGN);
>  }
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 6af4b37..1a1ebec 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -251,7 +251,8 @@ tests = tst-typesizes \
>  	tst-exec1 tst-exec2 tst-exec3 tst-exec4 \
>  	tst-exit1 tst-exit2 tst-exit3 \
>  	tst-stdio1 tst-stdio2 \
> -	tst-stack1 tst-stack2 tst-stack3 tst-pthread-getattr \
> +	tst-stack1 tst-stack2 tst-stack3 \
> +	tst-pthread-getattr tst-pthread-stack-env \
>  	tst-unload \
>  	tst-dlsym1 \
>  	tst-sysconf \
> @@ -441,6 +442,8 @@ tst-cancel7-ARGS = --command "exec $(host-test-program-cmd)"
>  tst-cancelx7-ARGS = $(tst-cancel7-ARGS)
>  tst-umask1-ARGS = $(objpfx)tst-umask1.temp
>  
> +tst-pthread-stack-env-ENV = LIBC_PTHREAD_DEFAULT_STACKSIZE_NP=1048576

Please change to `GLIBC_PTHREAD_DEFAULT_STACKSIZE'. After some
more thinking about this I'm of the opinion that we should use the
package name to split up the env-var namespace e.g. GLIBC_* for all
runtime tunables. We don't really have any right to LIBC_* unless
we get these env-vars into some more generic standard. We can drop
the NP because we are using 'GLIBC_' as the prefix which is inherently
non-portable.

OK with GLIBC_PTHREAD_DEFAULT_STACKSIZE.

> +
>  $(objpfx)tst-atfork2: $(libdl) $(shared-thread-library)
>  LDFLAGS-tst-atfork2 = -rdynamic
>  tst-atfork2-ENV = MALLOC_TRACE=$(objpfx)tst-atfork2.mtrace
> diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
> index 19e6616..eb5fe77 100644
> --- a/nptl/nptl-init.c
> +++ b/nptl/nptl-init.c
> @@ -276,8 +276,26 @@ extern void **__libc_dl_error_tsd (void) __attribute__ ((const));
>  /* This can be set by the debugger before initialization is complete.  */
>  static bool __nptl_initial_report_events __attribute_used__;
>  
> +static void
> +set_default_stacksize (size_t stacksize)
> +{
> +  if (stacksize < PTHREAD_STACK_MIN)
> +    stacksize = PTHREAD_STACK_MIN;
> +
> +  /* Make sure it meets the minimum size that allocate_stack
> +     (allocatestack.c) will demand, which depends on the page size.  */
> +  const uintptr_t pagesz = GLRO(dl_pagesize);
> +  const size_t minstack = pagesz + __static_tls_size + MINIMAL_REST_STACK;
> +
> +  if (stacksize < minstack)
> +    stacksize = minstack;
> +
> +  /* Round the resource limit up to page size.  */
> +  stacksize = (stacksize + pagesz - 1) & -pagesz;
> +  __default_stacksize = stacksize;
> +}

OK.

>  void
> -__pthread_initialize_minimal_internal (void)
> +__pthread_initialize_minimal_internal (int argc, char **argv, char **envp)
>  {
>  #ifndef SHARED
>    /* Unlike in the dynamically linked case the dynamic linker has not
> @@ -401,29 +419,41 @@ __pthread_initialize_minimal_internal (void)
>  
>    __static_tls_size = roundup (__static_tls_size, static_tls_align);
>  
> -  /* Determine the default allowed stack size.  This is the size used
> -     in case the user does not specify one.  */
> -  struct rlimit limit;
> -  if (getrlimit (RLIMIT_STACK, &limit) != 0
> -      || limit.rlim_cur == RLIM_INFINITY)
> -    /* The system limit is not usable.  Use an architecture-specific
> -       default.  */
> -    limit.rlim_cur = ARCH_STACK_DEFAULT_SIZE;
> -  else if (limit.rlim_cur < PTHREAD_STACK_MIN)
> -    /* The system limit is unusably small.
> -       Use the minimal size acceptable.  */
> -    limit.rlim_cur = PTHREAD_STACK_MIN;
> +  /* Initialize the environment.  libc.so gets initialized after us due to a
> +     circular dependency and hence __environ is not available otherwise.  */
> +    __environ = envp;
>  
> -  /* Make sure it meets the minimum size that allocate_stack
> -     (allocatestack.c) will demand, which depends on the page size.  */
> -  const uintptr_t pagesz = GLRO(dl_pagesize);
> -  const size_t minstack = pagesz + __static_tls_size + MINIMAL_REST_STACK;
> -  if (limit.rlim_cur < minstack)
> -    limit.rlim_cur = minstack;
> +#ifndef SHARED
> +    __libc_init_secure ();
> +#endif
>  
> -  /* Round the resource limit up to page size.  */
> -  limit.rlim_cur = (limit.rlim_cur + pagesz - 1) & -pagesz;
> -  __default_stacksize = limit.rlim_cur;
> +  size_t stacksize = 0;
> +  char *envval = __libc_secure_getenv ("LIBC_PTHREAD_DEFAULT_STACKSIZE_NP");

s/LIBC_/GLIBC_/g

> +
> +  if (__glibc_unlikely (envval != NULL && envval[0] != '\0'))
> +    {
> +      char *env_conv = envval;
> +      size_t ret = strtoul (envval, &env_conv, 0);
> +
> +      if (*env_conv == '\0')
> +	stacksize = ret;

This is not a correct usage of strtoul, please see the TIMEOUTFACTOR
example in tst-skeleton.c for an example of correct usage. You need
to additionally check `&& env_conv != envval'.

The relevant portion is this:
~~~
If endptr is not NULL, strtoul() stores the address of the 
first invalid character in *endptr.  If there were no digits at all,
strtoul() stores the original value of nptr in *endptr (and returns 0).
In particular, if *nptr is not '\0' but **endptr is '\0' on return, 
the entire string is valid.
~~~

OK with a fix to this to match what is required for strtoul.

> +    }
> +
> +  if (stacksize == 0)
> +    {
> +      /* Determine the default allowed stack size.  This is the size used
> +	 in case the user does not specify one.  */
> +      struct rlimit limit;
> +      if (getrlimit (RLIMIT_STACK, &limit) != 0
> +	  || limit.rlim_cur == RLIM_INFINITY)
> +	/* The system limit is not usable.  Use an architecture-specific
> +	   default.  */
> +	stacksize = ARCH_STACK_DEFAULT_SIZE;
> +      else
> +	stacksize = limit.rlim_cur;
> +    }
> +
> +  set_default_stacksize (stacksize);

OK.

>  #ifdef SHARED
>    /* Transfer the old value from the dynamic linker's internal location.  */
> diff --git a/nptl/tst-pthread-stack-env.c b/nptl/tst-pthread-stack-env.c
> new file mode 100644
> index 0000000..0c7273e
> --- /dev/null
> +++ b/nptl/tst-pthread-stack-env.c
> @@ -0,0 +1,72 @@
> +/* Verify that pthreads uses the default thread stack size set with the
> +   LIBC_PTHREAD_DEFAULT_STACKSIZE_NP environment variable.
> +   Copyright (C) 2013 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <pthread.h>
> +#include <stdio.h>
> +#include <string.h>
> +
> +#define STACKSIZE 1024 * 1024L
> +
> +void *
> +thr (void *u)
> +{
> +  size_t stacksize, guardsize;
> +  pthread_attr_t attr;
> +  pthread_getattr_np (pthread_self (), &attr);
> +
> +  pthread_attr_getstacksize (&attr, &stacksize);
> +  pthread_attr_getguardsize (&attr, &guardsize);
> +
> +  /* FIXME once guardsize is excluded from stacksize.  */
> +  if (stacksize - guardsize != STACKSIZE)
> +    {
> +      printf ("Stack size is %zu, should be %zu\n", stacksize - guardsize,
> +	      STACKSIZE);
> +      return (void *) 1;
> +    }
> +
> +  return NULL;
> +}
> +
> +int
> +do_test (int argc, char **argv)
> +{
> +  pthread_t t;
> +  void *thr_ret;
> +  int ret;
> +
> +  if ((ret = pthread_create (&t, NULL, thr, NULL)) != 0)
> +    {
> +      printf ("thread create failed: %s\n", strerror (ret));
> +      return 1;
> +    }
> +
> +  if ((ret = pthread_join (t, &thr_ret)) != 0)
> +    {
> +      printf ("join failed: %s\n", strerror (ret));
> +      return 1;
> +    }
> +
> +  if (thr_ret != NULL && thr_ret != PTHREAD_CANCELED)
> +    return 1;
> +
> +  return 0;
> +}
> +
> +#include "../test-skeleton.c"

This test returns a false positive PASS if the env-var parsing
code is broken and ARCH_STACK_DEFAULT_SIZE, limit.rlim_cur, or
the minstack match 1048576, which could be plausible.

The only *robust* way to run this test is to fork and exec
yourself in a mode which prints the default stack size. Then
once you know the value add 1MB to it, set the env var, and
fork/exec again, this time looking for a result that matches
your expected +1MB value.

However, given that at present no ARCH_STACK_DEFAULT_SIZE
is less than 2MB; the value of minstack is completely random
because of __static_tls_size, but most likely way less than 1MB
for *this* testcase; and PTHREAD_STACK_MIN is between 16KB and 
192KB; the test should be sufficiently robust even if the QoI
is not as high as it could be.

OK with comment in the test case that explains that there could
be a false positive and how to correct that.

In summary I this patch is OK with the following changes:
- Use GLIBC_PTHREAD_DEFAULT_STACKSIZE.
- Fix strtoul usage.
- Comment in test case about false positive PASS and how to fix it.

Please work on a manual change ASAP to document the new env-var.
We can't expect people to use this without documentation.

Cheers,
Carlos.