[1/2] Add framework for tunables

  The tunables framework allows us to uniformly manage and expose global
variables inside glibc as switches to users.  tunables/README has
instructions for glibc developers to add new tunables.

Tunables support can be enabled by passing the --enable-tunables
configure flag to the configure script.  This patch only adds a
framework and does not pose any limitations on how tunable values are
read from the user.  It also adds environment variables used in malloc
behaviour tweaking to the tunables framework as a PoC of the
compatibility interface.

	* manual/install.texi: Add --enable-tunables option.
	* INSTALL: Regenerate.
	* Makeconfig (CPPFLAGS): Define TOP_NAMESPACE.
	(before-compile): Generate dl-tunable-list.h early.
	* config.h.in: Add BUILD_TUNABLES.
	* config.make.in: Add build-tunables.
	* configure.ac: Add --enable-tunables option.
	* configure: Regenerate.
	* malloc/arena.c [BUILD_TUNABLES]: Include dl-tunables.h.
	Define TUNABLE_NAMESPACE.
	(DL_TUNABLE_CALLBACK(set_mallopt_check)): New function.
	(ptmalloc_init): Set tunable values.
	* malloc/tst-malloc-usable-static.c: New test case.
	* malloc/Makefile (tests-static): Add it.
	* csu/init-first.c [BUILD_TUNABLES]: Include dl-tunables.h.
	(__libc_init_first) [!SHARED]: Initialize tunables for static
	binaries.
	* scripts/gen-tunables.awk: New file.
	* README.tunables: New file.
	* elf/Versions (ld): Add __tunable_set_val to GLIBC_PRIVATE
	namespace.
	* elf/dl-tunable-list.h: New auto-generated file.
	* elf/dl-tunables.c: New file.
	* elf/dl-tunables.h: New file.
	* elf/dl-tunables.list: New file.
	* elf/dl-tunable-types.h: New file.
	* elf/rtld.c [BUILD_TUNABLES]: Include dl-tunables.h
	(process_envvars): Call __tunables_init.
---
 INSTALL                           |   6 ++
 Makeconfig                        |  16 ++++
 README.tunables                   |  74 ++++++++++++++++
 config.h.in                       |   3 +
 config.make.in                    |   1 +
 configure                         |  16 ++++
 configure.ac                      |  10 +++
 csu/init-first.c                  |   7 ++
 elf/Makefile                      |   5 ++
 elf/Versions                      |   3 +
 elf/dl-tunable-types.h            |  46 ++++++++++
 elf/dl-tunables.c                 | 182 ++++++++++++++++++++++++++++++++++++++
 elf/dl-tunables.h                 |  76 ++++++++++++++++
 elf/dl-tunables.list              |  50 +++++++++++
 elf/rtld.c                        |   8 ++
 malloc/Makefile                   |   3 +
 malloc/arena.c                    |  35 ++++++++
 malloc/tst-malloc-usable-static.c |   1 +
 manual/install.texi               |   5 ++
 scripts/gen-tunables.awk          | 157 ++++++++++++++++++++++++++++++++
 20 files changed, 704 insertions(+)
 create mode 100644 README.tunables
 create mode 100644 elf/dl-tunable-types.h
 create mode 100644 elf/dl-tunables.c
 create mode 100644 elf/dl-tunables.h
 create mode 100644 elf/dl-tunables.list
 create mode 100644 malloc/tst-malloc-usable-static.c
 create mode 100644 scripts/gen-tunables.awk
  

On 07/09/2016 08:37 PM, Siddhesh Poyarekar wrote:

> +ifeq (yes,$(build-tunables))
> +CPPFLAGS += -DTOP_NAMESPACE=glibc
> +endif

Should probably be TUNABLE_NAMESPACE or TUNABLE_ROOT.

> +- secure_env_alias:	Specify whether the environment variable should be read
> +			for setuid binaries.

This needs to describe the exact meaning of the choices, and the 
default.  I'm not sure if the current/name values are intuitive.

> +/* Compare environment names, bounded by the name hardcoded in glibc.  */
> +static bool
> +is_name (const char *orig, const char *envname)

I don't understand this comment.

> +static bool
> +get_next_env (char ***envp, char **name, size_t *namelen, char **val)

Can you get rid of one indirection somehow?

> +{
> +  char **ev = *envp;
> +
> +  while (ev != NULL && *ev != '\0')

*ev has type char *, so NULL is better than '\0'.  I'm surprised GCC 
does not warn about this.

> +# define TUNABLE_SET_VAL(__id,__val) \
> +({									      \
> +  __tunable_set_val							      \
> +   (TUNABLE_ENUM_NAME (TOP_NAMESPACE, TUNABLE_NAMESPACE, __id), (__val),      \
> +    NULL);								      \
> +})

You do not need __ prefixes on macro arguments.

> +#if BUILD_TUNABLES
> +  /* Ensure initialization/consolidation and do it under a lock so that a
> +     thread attempting to use the arena in parallel waits on us till we
> +     finish.  */
> +  mutex_lock (&main_arena.mutex);
> +  malloc_consolidate (&main_arena);
> +
> +  TUNABLE_SET_VAL_WITH_CALLBACK (check, &check_action, set_mallopt_check);
> +  TUNABLE_SET_VAL (top_pad, &mp_.top_pad);
> +  TUNABLE_SET_VAL (perturb, &perturb_byte);
> +  TUNABLE_SET_VAL (mmap_threshold, &mp_.mmap_threshold);
> +  TUNABLE_SET_VAL (trim_threshold, &mp_.trim_threshold);
> +  TUNABLE_SET_VAL (mmap_max, &mp_.n_mmaps_max);
> +  TUNABLE_SET_VAL (arena_max, &mp_.arena_max);
> +  TUNABLE_SET_VAL (arena_test, &mp_.arena_test);

This doesn't mirror what mallopt does internally, so it alters behavior 
with BUILD_TUNABLES.  In particular, the mmap threshold is still 
dynamically adjusted after that, I think.

This is indeed closer to what I had in mind.  But what I really want is 
something that can patch the tunables directly into a place which malloc 
can use.  My hope is that one day, we have a tunable descriptor inside 
libc.so, and (rarely written) range in the .data segment which contains 
the tunable variables.  Code which relies on tunables directly access 
the tunable range (using relaxed MO loads), and some external tool can 
use the tunables descriptor to update the tunables at run time.

> diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
> new file mode 100644
> index 0000000..1b930e7
> --- /dev/null
> +++ b/scripts/gen-tunables.awk
> @@ -0,0 +1,157 @@
> +# Generate dl-tunable-list.h from dl-tunables.list

Please add the copyright header because the file is non-trivial.

 > +  print "static tunable_t tunable_list[] = {"

There's a const missing there.  Due to function pointer, this should 
really go into .relro.

Thanks,
Florian
  

On Sat, Jul 9, 2016 at 11:37 AM, Siddhesh Poyarekar
<siddhesh@sourceware.org> wrote:
> The tunables framework allows us to uniformly manage and expose global
> variables inside glibc as switches to users.  tunables/README has
> instructions for glibc developers to add new tunables.
>
> Tunables support can be enabled by passing the --enable-tunables
> configure flag to the configure script.  This patch only adds a
> framework and does not pose any limitations on how tunable values are
> read from the user.  It also adds environment variables used in malloc
> behaviour tweaking to the tunables framework as a PoC of the
> compatibility interface.
>
>         * manual/install.texi: Add --enable-tunables option.
>         * INSTALL: Regenerate.
>         * Makeconfig (CPPFLAGS): Define TOP_NAMESPACE.
>         (before-compile): Generate dl-tunable-list.h early.
>         * config.h.in: Add BUILD_TUNABLES.
>         * config.make.in: Add build-tunables.
>         * configure.ac: Add --enable-tunables option.
>         * configure: Regenerate.
>         * malloc/arena.c [BUILD_TUNABLES]: Include dl-tunables.h.
>         Define TUNABLE_NAMESPACE.
>         (DL_TUNABLE_CALLBACK(set_mallopt_check)): New function.
>         (ptmalloc_init): Set tunable values.
>         * malloc/tst-malloc-usable-static.c: New test case.
>         * malloc/Makefile (tests-static): Add it.
>         * csu/init-first.c [BUILD_TUNABLES]: Include dl-tunables.h.
>         (__libc_init_first) [!SHARED]: Initialize tunables for static
>         binaries.
>         * scripts/gen-tunables.awk: New file.
>         * README.tunables: New file.
>         * elf/Versions (ld): Add __tunable_set_val to GLIBC_PRIVATE
>         namespace.
>         * elf/dl-tunable-list.h: New auto-generated file.
>         * elf/dl-tunables.c: New file.
>         * elf/dl-tunables.h: New file.
>         * elf/dl-tunables.list: New file.
>         * elf/dl-tunable-types.h: New file.
>         * elf/rtld.c [BUILD_TUNABLES]: Include dl-tunables.h
>         (process_envvars): Call __tunables_init.

Did you update siddhesh/tunables branch?  It still has the old patches.

H.J.
  

On Mon, Jul 11, 2016 at 06:05:09PM +0200, Florian Weimer wrote:
> On 07/09/2016 08:37 PM, Siddhesh Poyarekar wrote:
> 
> > +ifeq (yes,$(build-tunables))
> > +CPPFLAGS += -DTOP_NAMESPACE=glibc
> > +endif
> 
> Should probably be TUNABLE_NAMESPACE or TUNABLE_ROOT.

OK.

> 
> > +- secure_env_alias:	Specify whether the environment variable should be read
> > +			for setuid binaries.
> 
> This needs to describe the exact meaning of the choices, and the default.
> I'm not sure if the current/name values are intuitive.

It specifies if the env_alias should be read even for setuid binaries.
This is currently only useful for MALLOC_CHECK_ since it is read even
for setuid binaries.  Would env_alias_is_secure be clearer?

> > +/* Compare environment names, bounded by the name hardcoded in glibc.  */
> > +static bool
> > +is_name (const char *orig, const char *envname)
> 
> I don't understand this comment.

The comparison loop is bound by the name that is hardcoded into the
tunables_list structure, i.e. either tunable.name or
tunable.env_alias.

> > +static bool
> > +get_next_env (char ***envp, char **name, size_t *namelen, char **val)
> 
> Can you get rid of one indirection somehow?

I had picked this up verbatim from existing code.  I'll see what I can
do.

> 
> > +{
> > +  char **ev = *envp;
> > +
> > +  while (ev != NULL && *ev != '\0')
> 
> *ev has type char *, so NULL is better than '\0'.  I'm surprised GCC does
> not warn about this.

Ugh, yes, I'll fix it.

> > +# define TUNABLE_SET_VAL(__id,__val) \
> > +({									      \
> > +  __tunable_set_val							      \
> > +   (TUNABLE_ENUM_NAME (TOP_NAMESPACE, TUNABLE_NAMESPACE, __id), (__val),      \
> > +    NULL);								      \
> > +})
> 
> You do not need __ prefixes on macro arguments.

I did it out of habit to avoid namespace collisions, but it's not
necessary here so I'll remove it.

> This doesn't mirror what mallopt does internally, so it alters behavior with
> BUILD_TUNABLES.  In particular, the mmap threshold is still dynamically
> adjusted after that, I think.

Thanks, I missed that.  I'll fix it.

> This is indeed closer to what I had in mind.  But what I really want is
> something that can patch the tunables directly into a place which malloc can

I plan to do it via the tunables structure in the second pass, with
TUNABLE_SET_VAL_WITH_PTR that does the same thing as TUNABLE_SET_VAL
and then stores the pointer (&mp_.mmap_threshold) and callback in the
tunables structure to allow for a live update.

> use.  My hope is that one day, we have a tunable descriptor inside libc.so,
> and (rarely written) range in the .data segment which contains the tunable
> variables.  Code which relies on tunables directly access the tunable range
> (using relaxed MO loads), and some external tool can use the tunables
> descriptor to update the tunables at run time.

This is an interesting idea, but it will still have to live inside
ld.so if it needs to make IFUNC decisions.

> > diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
> > new file mode 100644
> > index 0000000..1b930e7
> > --- /dev/null
> > +++ b/scripts/gen-tunables.awk
> > @@ -0,0 +1,157 @@
> > +# Generate dl-tunable-list.h from dl-tunables.list
> 
> Please add the copyright header because the file is non-trivial.

OK.

> > +  print "static tunable_t tunable_list[] = {"
> 
> There's a const missing there.  Due to function pointer, this should really
> go into .relro.

OK.

Siddhesh
  

On Mon, Jul 11, 2016 at 09:25:07AM -0700, H.J. Lu wrote:
> Did you update siddhesh/tunables branch?  It still has the old patches.

I have now, sorry I keep forgetting.

Siddhesh
  

I am still reviewing the patch, but the question which pops into
my head is whether this is sufficient to initialize tunables
used early on via an init section.

As implied above, my hope is that the initial patches will enable
trivial addition tunables for TLE.  I'll port my old patches and
see how it goes nevertheless.


On 07/09/2016 01:37 PM, Siddhesh Poyarekar wrote:
> The tunables framework allows us to uniformly manage and expose global
> variables inside glibc as switches to users.  tunables/README has
> instructions for glibc developers to add new tunables.
> 
> Tunables support can be enabled by passing the --enable-tunables
> configure flag to the configure script.  This patch only adds a
> framework and does not pose any limitations on how tunable values are
> read from the user.  It also adds environment variables used in malloc
> behaviour tweaking to the tunables framework as a PoC of the
> compatibility interface.
> 
> 	* manual/install.texi: Add --enable-tunables option.
> 	* INSTALL: Regenerate.
> 	* Makeconfig (CPPFLAGS): Define TOP_NAMESPACE.
> 	(before-compile): Generate dl-tunable-list.h early.
> 	* config.h.in: Add BUILD_TUNABLES.
> 	* config.make.in: Add build-tunables.
> 	* configure.ac: Add --enable-tunables option.
> 	* configure: Regenerate.
> 	* malloc/arena.c [BUILD_TUNABLES]: Include dl-tunables.h.
> 	Define TUNABLE_NAMESPACE.
> 	(DL_TUNABLE_CALLBACK(set_mallopt_check)): New function.
> 	(ptmalloc_init): Set tunable values.
> 	* malloc/tst-malloc-usable-static.c: New test case.
> 	* malloc/Makefile (tests-static): Add it.
> 	* csu/init-first.c [BUILD_TUNABLES]: Include dl-tunables.h.
> 	(__libc_init_first) [!SHARED]: Initialize tunables for static
> 	binaries.
> 	* scripts/gen-tunables.awk: New file.
> 	* README.tunables: New file.
> 	* elf/Versions (ld): Add __tunable_set_val to GLIBC_PRIVATE
> 	namespace.
> 	* elf/dl-tunable-list.h: New auto-generated file.
> 	* elf/dl-tunables.c: New file.
> 	* elf/dl-tunables.h: New file.
> 	* elf/dl-tunables.list: New file.
> 	* elf/dl-tunable-types.h: New file.
> 	* elf/rtld.c [BUILD_TUNABLES]: Include dl-tunables.h
> 	(process_envvars): Call __tunables_init.
  

On Mon, Jul 11, 2016 at 9:56 AM, Paul E. Murphy
<murphyp@linux.vnet.ibm.com> wrote:
> I am still reviewing the patch, but the question which pops into
> my head is whether this is sufficient to initialize tunables
> used early on via an init section.

.init section isn't sufficient for IFUNC.

> As implied above, my hope is that the initial patches will enable
> trivial addition tunables for TLE.  I'll port my old patches and
> see how it goes nevertheless.
>
>
> On 07/09/2016 01:37 PM, Siddhesh Poyarekar wrote:
>> The tunables framework allows us to uniformly manage and expose global
>> variables inside glibc as switches to users.  tunables/README has
>> instructions for glibc developers to add new tunables.
>>
>> Tunables support can be enabled by passing the --enable-tunables
>> configure flag to the configure script.  This patch only adds a
>> framework and does not pose any limitations on how tunable values are
>> read from the user.  It also adds environment variables used in malloc
>> behaviour tweaking to the tunables framework as a PoC of the
>> compatibility interface.
>>
>>       * manual/install.texi: Add --enable-tunables option.
>>       * INSTALL: Regenerate.
>>       * Makeconfig (CPPFLAGS): Define TOP_NAMESPACE.
>>       (before-compile): Generate dl-tunable-list.h early.
>>       * config.h.in: Add BUILD_TUNABLES.
>>       * config.make.in: Add build-tunables.
>>       * configure.ac: Add --enable-tunables option.
>>       * configure: Regenerate.
>>       * malloc/arena.c [BUILD_TUNABLES]: Include dl-tunables.h.
>>       Define TUNABLE_NAMESPACE.
>>       (DL_TUNABLE_CALLBACK(set_mallopt_check)): New function.
>>       (ptmalloc_init): Set tunable values.
>>       * malloc/tst-malloc-usable-static.c: New test case.
>>       * malloc/Makefile (tests-static): Add it.
>>       * csu/init-first.c [BUILD_TUNABLES]: Include dl-tunables.h.
>>       (__libc_init_first) [!SHARED]: Initialize tunables for static
>>       binaries.
>>       * scripts/gen-tunables.awk: New file.
>>       * README.tunables: New file.
>>       * elf/Versions (ld): Add __tunable_set_val to GLIBC_PRIVATE
>>       namespace.
>>       * elf/dl-tunable-list.h: New auto-generated file.
>>       * elf/dl-tunables.c: New file.
>>       * elf/dl-tunables.h: New file.
>>       * elf/dl-tunables.list: New file.
>>       * elf/dl-tunable-types.h: New file.
>>       * elf/rtld.c [BUILD_TUNABLES]: Include dl-tunables.h
>>       (process_envvars): Call __tunables_init.
>
  

On 07/09/2016 02:37 PM, Siddhesh Poyarekar wrote:
> The tunables framework allows us to uniformly manage and expose global
> variables inside glibc as switches to users.  tunables/README has
> instructions for glibc developers to add new tunables.
> 
> Tunables support can be enabled by passing the --enable-tunables
> configure flag to the configure script.  This patch only adds a
> framework and does not pose any limitations on how tunable values are
> read from the user.  It also adds environment variables used in malloc
> behaviour tweaking to the tunables framework as a PoC of the
> compatibility interface.

Thank you for doing this work and posting these patches.

Overall the patch looks great.

There is are only two high-level things:

* Callback should be able to return error. I have some immediate
  needs for that. You can ignore this comment if you want.

* Malloc tunables are wrong. See my comments.
 
> 	* manual/install.texi: Add --enable-tunables option.
> 	* INSTALL: Regenerate.
> 	* Makeconfig (CPPFLAGS): Define TOP_NAMESPACE.
> 	(before-compile): Generate dl-tunable-list.h early.
> 	* config.h.in: Add BUILD_TUNABLES.
> 	* config.make.in: Add build-tunables.
> 	* configure.ac: Add --enable-tunables option.
> 	* configure: Regenerate.
> 	* malloc/arena.c [BUILD_TUNABLES]: Include dl-tunables.h.
> 	Define TUNABLE_NAMESPACE.
> 	(DL_TUNABLE_CALLBACK(set_mallopt_check)): New function.
> 	(ptmalloc_init): Set tunable values.
> 	* malloc/tst-malloc-usable-static.c: New test case.
> 	* malloc/Makefile (tests-static): Add it.
> 	* csu/init-first.c [BUILD_TUNABLES]: Include dl-tunables.h.
> 	(__libc_init_first) [!SHARED]: Initialize tunables for static
> 	binaries.
> 	* scripts/gen-tunables.awk: New file.
> 	* README.tunables: New file.
> 	* elf/Versions (ld): Add __tunable_set_val to GLIBC_PRIVATE
> 	namespace.
> 	* elf/dl-tunable-list.h: New auto-generated file.
> 	* elf/dl-tunables.c: New file.
> 	* elf/dl-tunables.h: New file.
> 	* elf/dl-tunables.list: New file.
> 	* elf/dl-tunable-types.h: New file.
> 	* elf/rtld.c [BUILD_TUNABLES]: Include dl-tunables.h
> 	(process_envvars): Call __tunables_init.
> ---
>  INSTALL                           |   6 ++
>  Makeconfig                        |  16 ++++
>  README.tunables                   |  74 ++++++++++++++++
>  config.h.in                       |   3 +
>  config.make.in                    |   1 +
>  configure                         |  16 ++++
>  configure.ac                      |  10 +++
>  csu/init-first.c                  |   7 ++
>  elf/Makefile                      |   5 ++
>  elf/Versions                      |   3 +
>  elf/dl-tunable-types.h            |  46 ++++++++++
>  elf/dl-tunables.c                 | 182 ++++++++++++++++++++++++++++++++++++++
>  elf/dl-tunables.h                 |  76 ++++++++++++++++
>  elf/dl-tunables.list              |  50 +++++++++++
>  elf/rtld.c                        |   8 ++
>  malloc/Makefile                   |   3 +
>  malloc/arena.c                    |  35 ++++++++
>  malloc/tst-malloc-usable-static.c |   1 +
>  manual/install.texi               |   5 ++
>  scripts/gen-tunables.awk          | 157 ++++++++++++++++++++++++++++++++
>  20 files changed, 704 insertions(+)
>  create mode 100644 README.tunables
>  create mode 100644 elf/dl-tunable-types.h
>  create mode 100644 elf/dl-tunables.c
>  create mode 100644 elf/dl-tunables.h
>  create mode 100644 elf/dl-tunables.list
>  create mode 100644 malloc/tst-malloc-usable-static.c
>  create mode 100644 scripts/gen-tunables.awk
> 
> diff --git a/INSTALL b/INSTALL
> index ec3445f..90dc481 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -158,6 +158,12 @@ will be used, and CFLAGS sets optimization options for the compiler.
>       By default for x86_64, the GNU C Library is built with vector math
>       library.  Use this option to disable vector math library.
>  
> +'--enable-tunables'
> +     Tunables support allows additional library parameters to be
> +     customized at runtime for each application.  This is an

s/for each application//g

> +     experimental feature and affects startup time and is thus disabled
> +     by default.
> +
>  '--build=BUILD-SYSTEM'
>  '--host=HOST-SYSTEM'
>       These options are for cross-compiling.  If you specify both options
> diff --git a/Makeconfig b/Makeconfig
> index 58bd3b3..732b870 100644
> --- a/Makeconfig
> +++ b/Makeconfig
> @@ -895,6 +895,11 @@ CPPFLAGS = $(config-extra-cppflags) $(CPPUNDEFS) $(CPPFLAGS-config) \
>  	   $(foreach lib,$(libof-$(basename $(@F))) \
>  			 $(libof-$(<F)) $(libof-$(@F)),$(CPPFLAGS-$(lib))) \
>  	   $(CPPFLAGS-$(<F)) $(CPPFLAGS-$(@F)) $(CPPFLAGS-$(basename $(@F)))
> +
> +ifeq (yes,$(build-tunables))
> +CPPFLAGS += -DTOP_NAMESPACE=glibc
> +endif
> +

OK.

>  override CFLAGS	= -std=gnu11 -fgnu89-inline $(config-extra-cflags) \
>  		  $(filter-out %frame-pointer,$(+cflags)) $(+gccwarn-c) \
>  		  $(sysdep-CFLAGS) $(CFLAGS-$(suffix $@)) $(CFLAGS-$(<F)) \
> @@ -1067,6 +1072,17 @@ $(common-objpfx)libc-modules.stmp: $(..)scripts/gen-libc-modules.awk \
>  
>  endif
>  
> +# Build the tunables list header early since it could be used by any module in
> +# glibc.
> +ifeq (yes,$(build-tunables))
> +before-compile += $(common-objpfx)dl-tunable-list.h
> +
> +$(common-objpfx)dl-tunable-list.h: $(..)scripts/gen-tunables.awk \
> +				   $(..)elf/dl-tunables.list
> +	$(AWK) -f $^ > $@.tmp
> +	mv $@{.tmp,}
> +endif

OK. I like the header list.

> +
>  common-generated += libc-modules.h libc-modules.stmp
>  
>  # The name under which the run-time dynamic linker is installed.
> diff --git a/README.tunables b/README.tunables
> new file mode 100644
> index 0000000..8890249
> --- /dev/null
> +++ b/README.tunables
> @@ -0,0 +1,74 @@
> +			TUNABLE FRAMEWORK
> +			=================
> +
> +The tunable framework allows modules within glibc to register variables that
> +may be tweaked through an environment variable or an API call.  It aims to
> +enforce a strict namespace rule to bring consistency to naming of these tunable
> +environment variables across the project.

The reason for tunables is to allow application authors to alter
the runtime library behaviour to match their workload. We should
say that up front since that notion will guide the choices we make.

See "Why?":
https://sourceware.org/glibc/wiki/TuningLibraryRuntimeBehavior

> +
> +ADDING A NEW TUNABLE
> +--------------------
> +
> +The TOP_NAMESPACE is defined by default as 'glibc' and it may be overridden in
> +distributions for specific tunables if they want to add their own tunables.
> +Downstream implementations are discouraged from using the 'glibc' namespace for
> +tunables they don't already have consensus to push upstream.
> +
> +There are two steps to adding a tunable:
> +
> +1. Add a tunable ID:
> +
> +Modules that wish to use the tunables interface must define the
> +TUNABLE_NAMESPACE macro.  Following this, for each tunable you want to
> +add, make an entry in elf/dl-tunables.list.  The format of the file is as
> +follows:
> +
> +TOP_NAMESPACE {
> +  NAMESPACE1 {
> +    TUNABLE1 {
> +      # tunable attributes, one per line
> +    }
> +    # A tunable with default attributes, i.e. string variable.
> +    TUNABLE2
> +    TUNABLE3 {
> +      # its attributes
> +    }
> +  }
> +  NAMESPACE2 {
> +    ...
> +  }
> +}
> +
> +The list of allowed attributes are:
> +
> +- type:			Data type.  Defaults to STRING.  Allowed types are:
> +			INT_32, SIZE_T.  Note that as of now the STRING type is
> +			neither defined nor supported since no tunables need
> +			it.  It will be in future though, when needed.

OK.

I expect we can add more types as time goes on.

> +
> +- minval:		Optional minimum acceptable value
> +
> +- maxval:		Optional maximum acceptable value
> +

I assume valid only for numeric types and such values as are valid for the type?

Or does minval/maxval indicate minlength/maxlength for a string as an easy form
of filtering?

> +- env_alias:		An alias environment variable
> +
> +- secure_env_alias:	Specify whether the environment variable should be read
> +			for setuid binaries.

We should make a few things clear here:

* The tunable may still be ignored in AT_SECURE if the non-default value
  fails to pass some level of runtime checking e.g. value could break the
  application.

* The 'secure_env_alias' should really mean "Specify whether the environment
  variable should be read for setuid binaries, 0 (default) means they are not
  read and 1 means they are. Even if the variable is read it may be ignored."

> +
> +2. Call either the TUNABLE_SET_VALUE and pass into it the tunable name and a
> +   pointer to the variable that should be set with the tunable value.
> +   If additional work needs to be done after setting the value, use the
> +   TUNABLE_SET_VALUE_WITH_CALLBACK instead and additionally pass a pointer to
> +   the function that should be called if the tunable value has been set.
> +

Are there any restrictions on the callback? I assume the foreign function (callback)
is called from _any_ context so you have to be very very very careful about what
functions you call?

> +FUTURE WORK
> +-----------
> +
> +The framework currently only allows a one-time initialization of variables
> +through environment variables and in some cases, modification of variables via
> +an API call.  A future goals for this project include:
> +
> +- Setting system-wide and user-wide defaults for tunables through some
> +  mechanism like a configuration file.
> +
> +- Allow tweaking of some tunables at runtime

OK.

> diff --git a/config.h.in b/config.h.in
> index 856ef6a..6112782 100644
> --- a/config.h.in
> +++ b/config.h.in
> @@ -240,4 +240,7 @@
>  /* PowerPC32 uses fctidz for floating point to long long conversions.  */
>  #define HAVE_PPC_FCTIDZ 0
>  
> +/* Build glibc with tunables support.  */
> +#define BUILD_TUNABLES 0
> +

Suggest 'HAVE_TUNABLES' following the other HAVE_* defines.

>  #endif
> diff --git a/config.make.in b/config.make.in
> index 95c6f36..653ef6f 100644
> --- a/config.make.in
> +++ b/config.make.in
> @@ -91,6 +91,7 @@ use-nscd = @use_nscd@
>  build-hardcoded-path-in-tests= @hardcoded_path_in_tests@
>  build-pt-chown = @build_pt_chown@
>  enable-lock-elision = @enable_lock_elision@
> +build-tunables = @build_tunables@

Since tunables is less about "building" and more about "having"
them, again total bikeshed, have-tunables.

>  
>  # Build tools.
>  CC = @CC@
> diff --git a/configure b/configure
> index 19a4829..1534ba7 100755
> --- a/configure
> +++ b/configure
> @@ -659,6 +659,7 @@ multi_arch
>  base_machine
>  add_on_subdirs
>  add_ons
> +build_tunables
>  build_pt_chown
>  build_nscd
>  link_obsolete_rpc
> @@ -773,6 +774,7 @@ enable_systemtap
>  enable_build_nscd
>  enable_nscd
>  enable_pt_chown
> +enable_tunables
>  enable_mathvec
>  with_cpu
>  '
> @@ -1440,6 +1442,7 @@ Optional Features:
>    --disable-build-nscd    disable building and installing the nscd daemon
>    --disable-nscd          library functions will not contact the nscd daemon
>    --enable-pt_chown       Enable building and installing pt_chown
> +  --enable-tunables       Enable tunables support
>    --enable-mathvec        Enable building and installing mathvec [default
>                            depends on architecture]
>  
> @@ -3646,6 +3649,19 @@ if test "$build_pt_chown" = yes; then
>  
>  fi
>  
> +# Check whether --enable-tunables was given.
> +if test "${enable_tunables+set}" = set; then :
> +  enableval=$enable_tunables; build_tunables=$enableval
> +else
> +  build_tunables=no
> +fi
> +
> +
> +if test "$build_tunables" = yes; then
> +  $as_echo "#define BUILD_TUNABLES 1" >>confdefs.h
> +
> +fi
> +
>  # The abi-tags file uses a fairly simplistic model for name recognition that
>  # can't distinguish i486-pc-linux-gnu fully from i486-pc-gnu.  So we mutate a
>  # $host_os of `gnu*' here to be `gnu-gnu*' just so that it can tell.
> diff --git a/configure.ac b/configure.ac
> index 123f0d2..b2aaf9b 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -395,6 +395,16 @@ if test "$build_pt_chown" = yes; then
>    AC_DEFINE(HAVE_PT_CHOWN)
>  fi
>  
> +AC_ARG_ENABLE([tunables],
> +	      [AS_HELP_STRING([--enable-tunables],
> +	       [Enable tunables support])],
> +	      [build_tunables=$enableval],
> +	      [build_tunables=no])
> +AC_SUBST(build_tunables)
> +if test "$build_tunables" = yes; then
> +  AC_DEFINE(BUILD_TUNABLES)
> +fi
> +
>  # The abi-tags file uses a fairly simplistic model for name recognition that
>  # can't distinguish i486-pc-linux-gnu fully from i486-pc-gnu.  So we mutate a
>  # $host_os of `gnu*' here to be `gnu-gnu*' just so that it can tell.
> diff --git a/csu/init-first.c b/csu/init-first.c
> index 77c6e1c..7427121 100644
> --- a/csu/init-first.c
> +++ b/csu/init-first.c
> @@ -28,6 +28,9 @@
>  #include <libc-internal.h>
>  
>  #include <ldsodefs.h>
> +#if BUILD_TUNABLES
> +# include <elf/dl-tunables.h>
> +#endif

OK.

>  
>  /* Set nonzero if we have to be prepared for more than one libc being
>     used in the process.  Safe assumption if initializer never runs.  */
> @@ -74,6 +77,10 @@ _init (int argc, char **argv, char **envp)
>  #ifndef SHARED
>    __libc_init_secure ();
>  
> +#if BUILD_TUNABLES
> +  __tunables_init (envp);

OK, very early initialization.

> +#endif
> +
>    /* First the initialization which normally would be done by the
>       dynamic linker.  */
>    _dl_non_dynamic_init ();
> diff --git a/elf/Makefile b/elf/Makefile
> index 210dde9..c634d120 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -35,6 +35,11 @@ dl-routines	= $(addprefix dl-,load lookup object reloc deps hwcaps \
>  ifeq (yes,$(use-ldconfig))
>  dl-routines += dl-cache
>  endif
> +
> +ifeq (yes,$(build-tunables))
> +dl-routines += dl-tunables
> +endif

OK.

Can we unit test the interface to make sure the API works as designed?

https://sourceware.org/ml/libc-alpha/2016-07/msg00110.html

> +
>  all-dl-routines = $(dl-routines) $(sysdep-dl-routines)
>  # But they are absent from the shared libc, because that code is in ld.so.
>  elide-routines.os = $(all-dl-routines) dl-support enbl-secure dl-origin \
> diff --git a/elf/Versions b/elf/Versions
> index 23deda9..1734697 100644
> --- a/elf/Versions
> +++ b/elf/Versions
> @@ -64,5 +64,8 @@ ld {
>  
>      # Pointer protection.
>      __pointer_chk_guard;
> +
> +    # Set value of a tunable.
> +    __tunable_set_val;

OK.

>    }
>  }
> diff --git a/elf/dl-tunable-types.h b/elf/dl-tunable-types.h
> new file mode 100644
> index 0000000..d1591b6
> --- /dev/null
> +++ b/elf/dl-tunable-types.h
> @@ -0,0 +1,46 @@
> +/* Tunable type information.
> +
> +   Copyright (C) 2016 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/>.  */
> +
> +#ifndef _TUNABLE_TYPES_H_
> +# define _TUNABLE_TYPES_H_
> +#include <stddef.h>
> +
> +typedef void (*tunable_callback_t) (void *);

Can we pass back a void* to return success/failure like the
pthreads API does? I can see a short term need to know if the
foreign function call failed.

> +
> +typedef enum
> +{
> +  TUNABLE_TYPE_INT_32,
> +  TUNABLE_TYPE_SIZE_T,
> +  TUNABLE_TYPE_STRING
> +} tunable_type_code_t;

OK.

> +
> +typedef struct
> +{
> +  tunable_type_code_t type_code;
> +  int64_t min;
> +  int64_t max;
> +} tunable_type_t;

OK.

> +
> +typedef union
> +{
> +  int64_t numval;
> +  const char *strval;
> +} tunable_val_t;
> +
> +#endif
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> new file mode 100644
> index 0000000..ee8c2f0
> --- /dev/null
> +++ b/elf/dl-tunables.c
> @@ -0,0 +1,182 @@
> +/* The tunable framework.  See the README to know how to use the tunable in
> +   a glibc module.
> +
> +   Copyright (C) 2016 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 <stdint.h>
> +#include <stdbool.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +
> +#define TUNABLES_INTERNAL 1
> +#include "dl-tunables.h"
> +
> +/* Compare environment names, bounded by the name hardcoded in glibc.  */
> +static bool
> +is_name (const char *orig, const char *envname)
> +{
> +  for (;*orig != '\0' && *envname != '\0'; envname++, orig++)
> +    if (*orig != *envname)
> +      break;
> +
> +  /* The ENVNAME is immediately followed by a value.  */
> +  if (*orig == '\0' && *envname == '=')
> +    return true;
> +  else
> +    return false;
> +}

OK.

> +
> +static bool
> +get_next_env (char ***envp, char **name, size_t *namelen, char **val)
> +{
> +  char **ev = *envp;
> +
> +  while (ev != NULL && *ev != '\0')
> +    {
> +      char *envline = *ev;
> +      int len = 0;
> +
> +      while (envline[len] != '\0' && envline[len] != '=')
> +	len++;
> +
> +      /* Just the name and no value, go to the next one.  */
> +      if (envline[len] == '\0')
> +	continue;
> +
> +      *name = envline;
> +      *namelen = len;
> +      *val = &envline[len + 1];
> +      *envp = ++ev;
> +
> +      return true;
> +    }
> +
> +  return false;
> +}

OK.

> +
> +
> +#define RETURN_IF_INVALID_RANGE(__cur, __val) \
> +({									      \
> +  __typeof ((__cur)->type.min) min = (__cur)->type.min;			      \
> +  __typeof ((__cur)->type.max) max = (__cur)->type.max;			      \
> +  if (min != max && ((__val) < min || (__val) > max))			      \
> +    return;								      \
> +})

As mentioned above, might be nice to have min/max limit string min
and max length.

> +
> +/* Validate range of the input value and initialize the tunable CUR if it looks
> +   good.  */
> +static void
> +tunable_initialize (tunable_t *cur)
> +{
> +  switch (cur->type.type_code)
> +    {
> +    case TUNABLE_TYPE_INT_32:
> +	{
> +	  int32_t val = (int32_t) __strtoul_internal (cur->strval, NULL, 0, 0);
> +	  RETURN_IF_INVALID_RANGE (cur, val);
> +	  cur->val.numval = (int64_t) val;
> +	  break;
> +	}
> +    case TUNABLE_TYPE_SIZE_T:
> +	{
> +	  size_t val = (size_t) __strtoul_internal (cur->strval, NULL, 0, 0);
> +	  RETURN_IF_INVALID_RANGE (cur, val);
> +	  cur->val.numval = (int64_t) val;
> +	  break;
> +	}
> +    case TUNABLE_TYPE_STRING:
> +	{
> +	  cur->val.strval = cur->strval;
> +	  break;
> +	}
> +    default:
> +      __builtin_unreachable ();
> +    }
> +}

OK.

> +
> +/* Initialize the tunables list from the environment.  For now we only use the
> +   ENV_ALIAS to find values.  Later we will also use the tunable names to find
> +   values.  */
> +void
> +__tunables_init (char **envp)
> +{
> +  char *envname = NULL;
> +  char *envval = NULL;
> +  size_t len = 0;
> +
> +  while (get_next_env (&envp, &envname, &len, &envval))
> +    {
> +      for (int i = 0; i < sizeof (tunable_list) / sizeof (tunable_t); i++)
> +	{
> +	  tunable_t *cur = &tunable_list[i];
> +
> +	  /* Skip over tunables that have either been set already or should be
> +	     skipped.  */
> +	  if (cur->strval != NULL || cur->env_alias == NULL
> +	      || (__libc_enable_secure && !cur->secure_env_alias))
> +	    continue;
> +
> +	  const char *name = cur->env_alias;
> +
> +	  /* We have a match.  Initialize and move on to the next line.  */
> +	  if (is_name (name, envname))
> +	    {
> +	      cur->strval = envval;
> +	      tunable_initialize (cur);
> +	      break;
> +	    }
> +	}
> +    }
> +}
> +

OK. We can accelerate this a bit if we used the TOP_NAMESPACE[0] char
to reject env entries.

> +/* Set the tunable value.  This is called by the module that the tunable exists
> +   in. */
> +void
> +__tunable_set_val (tunable_id_t id, void *valp, tunable_callback_t callback)
> +{
> +  tunable_t *cur = &tunable_list[id];
> +
> +  /* Sanity check: don't do anything if our tunable was not set during
> +     initialization.  */
> +  if (cur->strval == NULL)
> +    return;
> +
> +  switch (cur->type.type_code)
> +    {
> +    case TUNABLE_TYPE_INT_32:
> +	{
> +	  *((int32_t *) valp) = (int32_t) cur->val.numval;
> +	  break;
> +	}
> +    case TUNABLE_TYPE_SIZE_T:
> +	{
> +	  *((size_t *) valp) = (size_t) cur->val.numval;
> +	  break;
> +	}
> +    case TUNABLE_TYPE_STRING:
> +	{
> +	  *((const char **)valp) = cur->strval;
> +	  break;
> +	}
> +    default:
> +      __builtin_unreachable ();
> +    }
> +
> +  if (callback)
> +    callback (&cur->val);
> +}

OK.

> diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h
> new file mode 100644
> index 0000000..c4ae97d
> --- /dev/null
> +++ b/elf/dl-tunables.h
> @@ -0,0 +1,76 @@
> +/* The tunable framework.  See the README to know how to use the tunable in
> +   a glibc module.
> +
> +   Copyright (C) 2016 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/>.  */
> +
> +#ifndef _TUNABLES_H_
> +# define _TUNABLES_H_
> +# include <stddef.h>
> +# include "dl-tunable-types.h"
> +
> +/* A tunable.  */
> +struct _tunable
> +{
> +  const char *name;			/* Internal name of the tunable.  */
> +  tunable_type_t type;			/* Data type of the tunable.  */
> +  tunable_val_t val;			/* The value.  */
> +  const char *strval;			/* The string containing the value,
> +					   points into envp.  */
> +  /* Compatibility elements.  */
> +  const char *env_alias;		/* The compatibility environment
> +					   variable name.  */
> +  bool secure_env_alias;		/* Whether the environment variable
> +					   must be read even for setuid
> +					   binaries.  */

Add "May still be ignored if value is dangerous."

> +};
> +
> +typedef struct _tunable tunable_t;
> +
> +/* Full name for a tunable is top_ns.tunable_ns.id.  */
> +#define TUNABLE_NAME_S(top,ns,id) #top "." #ns "." #id
> +
> +# define TUNABLE_ENUM_NAME(top,ns,id) TUNABLE_ENUM_NAME1 (top,ns,id)
> +# define TUNABLE_ENUM_NAME1(top,ns,id) top ## _ ## ns ## _ ## id
> +
> +#include "dl-tunable-list.h"
> +
> +extern void __tunables_init (char **);
> +extern void __tunable_set_val (tunable_id_t, void *, tunable_callback_t);
> +
> +/* Check if the tunable has been set to a non-default value and if it is, copy
> +   it over into __VAL.  */
> +# define TUNABLE_SET_VAL(__id,__val) \
> +({									      \
> +  __tunable_set_val							      \
> +   (TUNABLE_ENUM_NAME (TOP_NAMESPACE, TUNABLE_NAMESPACE, __id), (__val),      \
> +    NULL);								      \
> +})
> +
> +/* Same as TUNABLE_SET_VAL, but also set the callback function to __CB and call
> +   it.  */
> +# define TUNABLE_SET_VAL_WITH_CALLBACK(__id,__val,__cb) \
> +({									      \
> +  __tunable_set_val							      \
> +   (TUNABLE_ENUM_NAME (TOP_NAMESPACE, TUNABLE_NAMESPACE, __id), (__val),      \
> +    DL_TUNABLE_CALLBACK (__cb));					      \
> +})
> +
> +/* Namespace sanity for callback functions.  Use this macro to keep the
> +   namespace of the modules clean.  */
> +#define DL_TUNABLE_CALLBACK(__name) _dl_tunable_ ## __name
> +#endif

OK.

> diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
> new file mode 100644
> index 0000000..342e3d5
> --- /dev/null
> +++ b/elf/dl-tunables.list
> @@ -0,0 +1,50 @@
> +# Allowed attributes for tunables:
> +#
> +# type: Defaults to STRING
> +# minval: Optional minimum acceptable value
> +# maxval: Optional maximum acceptable value
> +# env_alias: An alias environment variable
> +# secure_env_alias: Specify whether the environment variable should be read for
> +# setuid binaries.
> +
> +glibc {
> +  malloc {
> +    check {
> +      type: INT_32
> +      minval: 0
> +      maxval: 3
> +      env_alias: MALLOC_CHECK_
> +      secure_env_alias: true
> +    }
> +    top_pad {
> +      type: SIZE_T
> +      env_alias: MALLOC_TOP_PAD_
> +    }
> +    perturb {
> +      type: INT_32
> +      minval: 0
> +      maxval: 0xff
> +      env_alias: MALLOC_PERTURB_
> +    }
> +    mmap_threshold {
> +      type: SIZE_T
> +      env_alias: MALLOC_MMAP_THRESHOLD_
> +    }
> +    trim_threshold {
> +      type: SIZE_T
> +      env_alias: MALLOC_TRIM_THRESHOLD_
> +    }
> +    mmap_max {
> +      type: INT_32
> +      env_alias: MALLOC_MMAP_MAX_
> +    }
> +    arena_max {
> +      type: SIZE_T
> +      env_alias: MALLOC_ARENA_MAX
> +    }
> +    arena_test {
> +      type: SIZE_T
> +      env_alias: MALLOC_ARENA_TEST
> +    }
> +  }
> +}

Nice. We need M_MXFAST on the list! ;-)

> diff --git a/elf/rtld.c b/elf/rtld.c
> index 647661c..263723a 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -44,6 +44,10 @@
>  
>  #include <assert.h>
>  
> +#if BUILD_TUNABLES
> +# include <dl-tunables.h>
> +#endif
> +
>  /* Avoid PLT use for our local calls at startup.  */
>  extern __typeof (__mempcpy) __mempcpy attribute_hidden;
>  
> @@ -2346,6 +2350,10 @@ process_envvars (enum mode *modep)
>    enum mode mode = normal;
>    char *debug_output = NULL;
>  
> +#if BUILD_TUNABLES
> +  __tunables_init (_environ);
> +#endif

OK.

> +
>    /* This is the default place for profiling data file.  */
>    GLRO(dl_profile_output)
>      = &"/var/tmp\0/var/profile"[__libc_enable_secure ? 9 : 0];
> diff --git a/malloc/Makefile b/malloc/Makefile
> index fa1730e..23ab2f4 100644
> --- a/malloc/Makefile
> +++ b/malloc/Makefile
> @@ -31,6 +31,8 @@ tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
>  	 tst-malloc-backtrace tst-malloc-thread-exit \
>  	 tst-malloc-thread-fail tst-malloc-fork-deadlock \
>  	 tst-mallocfork2
> +tests-static := tst-malloc-usable-static
> +tests += $(tests-static)

OK.

>  test-srcs = tst-mtrace
>  
>  routines = malloc morecore mcheck mtrace obstack \
> @@ -138,6 +140,7 @@ endif
>  
>  tst-mcheck-ENV = MALLOC_CHECK_=3
>  tst-malloc-usable-ENV = MALLOC_CHECK_=3
> +tst-malloc-usable-static-ENV = $(tst-malloc-usable-ENV)
>  
>  # Uncomment this for test releases.  For public releases it is too expensive.
>  #CPPFLAGS-malloc.o += -DMALLOC_DEBUG=1
> diff --git a/malloc/arena.c b/malloc/arena.c
> index 229783f..7de4caf 100644
> --- a/malloc/arena.c
> +++ b/malloc/arena.c
> @@ -19,6 +19,11 @@
>  
>  #include <stdbool.h>
>  
> +#if BUILD_TUNABLES
> +# define TUNABLE_NAMESPACE malloc
> +# include <elf/dl-tunables.h>
> +#endif
> +
>  /* Compile-time constants.  */
>  
>  #define HEAP_MIN_SIZE (32 * 1024)
> @@ -204,6 +209,15 @@ __malloc_fork_unlock_child (void)
>    mutex_init (&list_lock);
>  }
>  
> +#if BUILD_TUNABLES
> +void
> +DL_TUNABLE_CALLBACK (set_mallopt_check) (void *unused)
> +{
> +  if (check_action != 0)
> +    __malloc_check_init ();
> +}
> +
> +#else
>  /* Initialization routine. */
>  #include <string.h>
>  extern char **_environ;
> @@ -238,6 +252,7 @@ next_env_entry (char ***position)
>  
>    return result;
>  }
> +#endif
>  
>  
>  #ifdef SHARED
> @@ -272,6 +287,24 @@ ptmalloc_init (void)
>  #endif
>  
>    thread_arena = &main_arena;
> +
> +#if BUILD_TUNABLES
> +  /* Ensure initialization/consolidation and do it under a lock so that a
> +     thread attempting to use the arena in parallel waits on us till we
> +     finish.  */
> +  mutex_lock (&main_arena.mutex);
> +  malloc_consolidate (&main_arena);
> +
> +  TUNABLE_SET_VAL_WITH_CALLBACK (check, &check_action, set_mallopt_check);
> +  TUNABLE_SET_VAL (top_pad, &mp_.top_pad);
> +  TUNABLE_SET_VAL (perturb, &perturb_byte);
> +  TUNABLE_SET_VAL (mmap_threshold, &mp_.mmap_threshold);
> +  TUNABLE_SET_VAL (trim_threshold, &mp_.trim_threshold);
> +  TUNABLE_SET_VAL (mmap_max, &mp_.n_mmaps_max);
> +  TUNABLE_SET_VAL (arena_max, &mp_.arena_max);
> +  TUNABLE_SET_VAL (arena_test, &mp_.arena_test);

Two things are wrong.

Missing systemtap probes, _and_ wrong settings.

For example setting top_pad should disable dynamic thresholding.

Therefore you need to do the following:

(a) Take __libc_mallopt from malloc/malloc.c and split out the
    setting of each variable into a "callback"

(b) Have __libc_mallopt call the callbacks.

(c) Have TUNABLE_SET_VAL call the callback via the tunables
    mechanism.

That way you have code in one place for what has to happen
when setting the variable.

> +  mutex_unlock (&main_arena.mutex);
> +#else
>    const char *s = NULL;
>    if (__glibc_likely (_environ != NULL))
>      {
> @@ -340,6 +373,8 @@ ptmalloc_init (void)
>        if (check_action != 0)
>          __malloc_check_init ();
>      }
> +#endif
> +
>  #if HAVE_MALLOC_INIT_HOOK
>    void (*hook) (void) = atomic_forced_read (__malloc_initialize_hook);
>    if (hook != NULL)

OK.

> diff --git a/malloc/tst-malloc-usable-static.c b/malloc/tst-malloc-usable-static.c
> new file mode 100644
> index 0000000..8907db0
> --- /dev/null
> +++ b/malloc/tst-malloc-usable-static.c
> @@ -0,0 +1 @@
> +#include <malloc/tst-malloc-usable.c>
> diff --git a/manual/install.texi b/manual/install.texi
> index 79ee45f..f9b4784 100644
> --- a/manual/install.texi
> +++ b/manual/install.texi
> @@ -189,6 +189,11 @@ configure with @option{--disable-werror}.
>  By default for x86_64, @theglibc{} is built with vector math library.
>  Use this option to disable vector math library.
>  
> +@item --enable-tunables
> +Tunables support allows additional library parameters to be customized at
> +runtime for each application.  This is an experimental feature and affects
> +startup time and is thus disabled by default.
> +
>  @item --build=@var{build-system}
>  @itemx --host=@var{host-system}
>  These options are for cross-compiling.  If you specify both options and
> diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
> new file mode 100644
> index 0000000..1b930e7
> --- /dev/null
> +++ b/scripts/gen-tunables.awk
> @@ -0,0 +1,157 @@
> +# Generate dl-tunable-list.h from dl-tunables.list
> +
> +BEGIN {
> +  tunable=""
> +  ns=""
> +  top_ns=""
> +}
> +
> +# Skip over blank lines and comments.
> +/^#/ {
> +  next
> +}
> +
> +/^[ \t]*$/ {
> +  next
> +}
> +
> +# Beginning of either a top namespace, tunable namespace or a tunable, decided
> +# on the current value of TUNABLE, NS or TOP_NS.
> +$2 == "{" {
> +  if (top_ns == "") {
> +    top_ns = $1
> +  }
> +  else if (ns == "") {
> +    ns = $1
> +  }
> +  else if (tunable == "") {
> +    tunable = $1
> +  }
> +  else {
> +    printf ("Unexpected occurrence of '{': %s:%d\n", FILENAME, FNR)
> +    exit 1
> +  }
> +
> +  next
> +}
> +
> +# End of either a top namespace, tunable namespace or a tunable.
> +$1 == "}" {
> +  if (tunable != "") {
> +    # Tunables definition ended, now fill in default attributes.
> +    if (!types[top_ns][ns][tunable]) {
> +      types[top_ns][ns][tunable] = "STRING"
> +    }
> +    if (!minvals[top_ns][ns][tunable]) {
> +      minvals[top_ns][ns][tunable] = "0"
> +    }
> +    if (!maxvals[top_ns][ns][tunable]) {
> +      maxvals[top_ns][ns][tunable] = "0"
> +    }
> +    if (!env_alias[top_ns][ns][tunable]) {
> +      env_alias[top_ns][ns][tunable] = "NULL"
> +    }
> +    if (!secure_env_alias[top_ns][ns][tunable]) {
> +      secure_env_alias[top_ns][ns][tunable] = "false"
> +    }
> +
> +    tunable = ""
> +  }
> +  else if (ns != "") {
> +    ns = ""
> +  }
> +  else if (top_ns != "") {
> +    top_ns = ""
> +  }
> +  else {
> +    printf ("syntax error: extra }: %s:%d\n", FILENAME, FNR)
> +    exit 1
> +  }
> +  next
> +}
> +
> +# Everything else, which could either be a tunable without any attributes or a
> +# tunable attribute.
> +{
> +  if (ns == "") {
> +    printf("Line %d: Invalid tunable outside a namespace: %s\n", NR, $0)
> +    exit 1
> +  }
> +
> +  if (tunable == "") {
> +    # We encountered a tunable without any attributes, so note it with a
> +    # default.
> +    types[top_ns][ns][$1] = "STRING"
> +    next
> +  }
> +
> +  # Otherwise, we have encountered a tunable attribute.
> +  split($0, arr, ":")
> +  attr = gensub(/^[ \t]+|[ \t]+$/, "", "g", arr[1])
> +  val = gensub(/^[ \t]+|[ \t]+$/, "", "g", arr[2])
> +
> +  if (attr == "type") {
> +    types[top_ns][ns][tunable] = val
> +  }
> +  else if (attr == "minval") {
> +    minvals[top_ns][ns][tunable] = val
> +  }
> +  else if (attr == "maxval") {
> +    maxvals[top_ns][ns][tunable] = val
> +  }
> +  else if (attr == "env_alias") {
> +    env_alias[top_ns][ns][tunable] = sprintf("\"%s\"", val)
> +  }
> +  else if (attr == "secure_env_alias") {
> +    if (val == "true" || val == "false") {
> +      secure_env_alias[top_ns][ns][tunable] = val
> +    }
> +    else {
> +      printf("Line %d: Invalid value (%s) for secure_env_alias: %s, ", NR, val,
> +	     $0)
> +      print("Allowed values are 'true' or 'false'")
> +      exit 1
> +    }
> +  }
> +}
> +
> +END {
> +  if (ns != "") {
> +    print "Unterminated namespace.  Is a closing brace missing?"
> +    exit 1
> +  }
> +
> +  print "/* AUTOGENERATED by gen-tunables.awk.  */"
> +  print "#ifndef _TUNABLES_H_"
> +  print "# error \"Do not include this file directly.\""
> +  print "# error \"Include tunables.h instead.\""
> +  print "#endif"
> +
> +  # Now, the enum names
> +  print "\ntypedef enum"
> +  print "{"
> +  for (t in types) {
> +    for (n in types[t]) {
> +      for (m in types[t][n]) {
> +        printf ("  TUNABLE_ENUM_NAME(%s, %s, %s),\n", t, n, m);
> +      }
> +    }
> +  }
> +  print "} tunable_id_t;\n"
> +
> +  # Finally, the tunable list.
> +  print "\n#ifdef TUNABLES_INTERNAL"
> +  print "static tunable_t tunable_list[] = {"
> +  for (t in types) {
> +    for (n in types[t]) {
> +      for (m in types[t][n]) {
> +        printf ("  {TUNABLE_NAME_S(%s, %s, %s)", t, n, m)
> +        printf (", {TUNABLE_TYPE_%s, %s, %s}, {.numval = 0}, NULL, %s, %s},\n",
> +		types[t][n][m], minvals[t][n][m], maxvals[t][n][m],
> +		env_alias[t][n][m], secure_env_alias[t][n][m]);
> +      }
> +    }
> +  }
> +  print "};"
> +  print "#endif"
> +}
> 

OK.

Cheers,
Carlos.
  

Siddhesh Poyarekar <siddhesh@sourceware.org> writes:
>> You do not need __ prefixes on macro arguments.
>
> I did it out of habit to avoid namespace collisions, but it's not
> necessary here so I'll remove it.

I've been burned by unprotected parameter names in headers too (although
not macros); it's a good habit to include the underscores, even if not
needed.
  

On 07/11/2016 06:05 PM, Florian Weimer wrote:
> This is indeed closer to what I had in mind.  But what I really want is
> something that can patch the tunables directly into a place which malloc
> can use.  My hope is that one day, we have a tunable descriptor inside
> libc.so, and (rarely written) range in the .data segment which contains
> the tunable variables.  Code which relies on tunables directly access
> the tunable range (using relaxed MO loads), and some external tool can
> use the tunables descriptor to update the tunables at run time.

Just to clarify: I don't see this an obstacle to applying the present 
patch.  I merely hope we can agree this is a direction we want to move in.

The main problem I see with the patch is the lack of equivalence with 
the mallopt-based approach.

Thanks,
Florian
  

On 07/11/2016 06:39 PM, Siddhesh Poyarekar wrote:
>>> > > +- secure_env_alias:	Specify whether the environment variable should be read
>>> > > +			for setuid binaries.
>> >
>> > This needs to describe the exact meaning of the choices, and the default.
>> > I'm not sure if the current/name values are intuitive.
> It specifies if the env_alias should be read even for setuid binaries.
> This is currently only useful for MALLOC_CHECK_ since it is read even
> for setuid binaries.  Would env_alias_is_secure be clearer?

No, because I think the sense is reverted:

+    check {
+      type: INT_32
+      minval: 0
+      maxval: 3
+      env_alias: MALLOC_CHECK_
+      secure_env_alias: true
+    }

That's why I said you need to document what the options are and what 
they do in AT_SECURE mode, and the default value.

Thanks,
Florian
  

On Mon, Jul 11, 2016 at 9:41 AM, Siddhesh Poyarekar
<siddhesh@sourceware.org> wrote:
> On Mon, Jul 11, 2016 at 09:25:07AM -0700, H.J. Lu wrote:
>> Did you update siddhesh/tunables branch?  It still has the old patches.
>
> I have now, sorry I keep forgetting.

On x86-64, I got

FAIL: malloc/tst-malloc-usable-static-tunables
FAIL: malloc/tst-malloc-usable-tunables

[hjl@gnu-6 build-x86_64-linux]$ cat malloc/tst-malloc-usable-static-tunables.out
malloc_usable_size: expected 7 but got 24
[hjl@gnu-6 build-x86_64-linux]$ cat malloc/tst-malloc-usable-tunables.out
malloc_usable_size: expected 7 but got 24
[hjl@gnu-6 build-x86_64-linux]$

I used

BUILD_CC="gcc" CC="gcc" CXX="g++" CFLAGS="-O3 -g"
/export/gnu/import/git/sources/glibc/configure --prefix=/usr
--without-cvs --without-selinux --target=x86_64-linux
--build=x86_64-linux --host=x86_64-linux
--enable-hardcoded-path-in-tests --enable-large-benchtests

to configure glibc.
  

On Wed, Jul 13, 2016 at 07:43:22AM -0700, H.J. Lu wrote:
> On x86-64, I got
> 
> FAIL: malloc/tst-malloc-usable-static-tunables
> FAIL: malloc/tst-malloc-usable-tunables
> 
> [hjl@gnu-6 build-x86_64-linux]$ cat malloc/tst-malloc-usable-static-tunables.out
> malloc_usable_size: expected 7 but got 24
> [hjl@gnu-6 build-x86_64-linux]$ cat malloc/tst-malloc-usable-tunables.out
> malloc_usable_size: expected 7 but got 24
> [hjl@gnu-6 build-x86_64-linux]$
> 
> I used
> 
> BUILD_CC="gcc" CC="gcc" CXX="g++" CFLAGS="-O3 -g"
> /export/gnu/import/git/sources/glibc/configure --prefix=/usr
> --without-cvs --without-selinux --target=x86_64-linux
> --build=x86_64-linux --host=x86_64-linux
> --enable-hardcoded-path-in-tests --enable-large-benchtests

I test on x86_64, so that's odd, maybe due to
--enable-hardcoded-path-in-tests.  I'll take a look.

Thanks,
Siddhesh
  

On Wed, Jul 13, 2016 at 7:49 AM, Siddhesh Poyarekar
<siddhesh@sourceware.org> wrote:
> On Wed, Jul 13, 2016 at 07:43:22AM -0700, H.J. Lu wrote:
>> On x86-64, I got
>>
>> FAIL: malloc/tst-malloc-usable-static-tunables
>> FAIL: malloc/tst-malloc-usable-tunables
>>
>> [hjl@gnu-6 build-x86_64-linux]$ cat malloc/tst-malloc-usable-static-tunables.out
>> malloc_usable_size: expected 7 but got 24
>> [hjl@gnu-6 build-x86_64-linux]$ cat malloc/tst-malloc-usable-tunables.out
>> malloc_usable_size: expected 7 but got 24
>> [hjl@gnu-6 build-x86_64-linux]$
>>
>> I used
>>
>> BUILD_CC="gcc" CC="gcc" CXX="g++" CFLAGS="-O3 -g"
>> /export/gnu/import/git/sources/glibc/configure --prefix=/usr
>> --without-cvs --without-selinux --target=x86_64-linux
>> --build=x86_64-linux --host=x86_64-linux
>> --enable-hardcoded-path-in-tests --enable-large-benchtests
>
> I test on x86_64, so that's odd, maybe due to
> --enable-hardcoded-path-in-tests.  I'll take a look.
>

This scheme doesn't work well for IFUNC.  cpu_features must be
updated very early after init_cpu_features is called.  But __tunables_init
is called too late.  Can it be moved before init_cpu_features so that
cpu_features can be updated at the end of init_cpu_features?
  

On Wed, Jul 13, 2016 at 10:37:56AM -0700, H.J. Lu wrote:
> This scheme doesn't work well for IFUNC.  cpu_features must be
> updated very early after init_cpu_features is called.  But __tunables_init
> is called too late.  Can it be moved before init_cpu_features so that
> cpu_features can be updated at the end of init_cpu_features?

Sure, I'll move it further up.

Siddhesh
  

On Wed, Jul 13, 2016 at 10:55 AM, Siddhesh Poyarekar
<siddhesh@sourceware.org> wrote:
> On Wed, Jul 13, 2016 at 10:37:56AM -0700, H.J. Lu wrote:
>> This scheme doesn't work well for IFUNC.  cpu_features must be
>> updated very early after init_cpu_features is called.  But __tunables_init
>> is called too late.  Can it be moved before init_cpu_features so that
>> cpu_features can be updated at the end of init_cpu_features?
>
> Sure, I'll move it further up.

Please make sure that it is called before init_cpu_features for both
static and dynamic executables.
  

On Wed, Jul 13, 2016 at 10:59 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Jul 13, 2016 at 10:55 AM, Siddhesh Poyarekar
> <siddhesh@sourceware.org> wrote:
>> On Wed, Jul 13, 2016 at 10:37:56AM -0700, H.J. Lu wrote:
>>> This scheme doesn't work well for IFUNC.  cpu_features must be
>>> updated very early after init_cpu_features is called.  But __tunables_init
>>> is called too late.  Can it be moved before init_cpu_features so that
>>> cpu_features can be updated at the end of init_cpu_features?
>>
>> Sure, I'll move it further up.
>
> Please make sure that it is called before init_cpu_features for both
> static and dynamic executables.
>

Another thing, please add TUNABLE_WITH_CALLBACK:

TUNABLE_WITH_CALLBACK (ifunc, ifunc);

so that I can have a callback without setting a value.
  

On Wed, Jul 13, 2016 at 11:03:02AM -0700, H.J. Lu wrote:
> Another thing, please add TUNABLE_WITH_CALLBACK:
> 
> TUNABLE_WITH_CALLBACK (ifunc, ifunc);
> 
> so that I can have a callback without setting a value.

You can set the val as NULL, I'll add a NULL check.

Siddhesh
  

On Mon, Jul 11, 2016 at 03:06:34PM -0400, Carlos O'Donell wrote:
> > +The list of allowed attributes are:
> > +
> > +- type:			Data type.  Defaults to STRING.  Allowed types are:
> > +			INT_32, SIZE_T.  Note that as of now the STRING type is
> > +			neither defined nor supported since no tunables need
> > +			it.  It will be in future though, when needed.
> 
> OK.
> 
> I expect we can add more types as time goes on.

Yes, and update this document when we do, which reminds me to update
the fact that STRING is supported.

> > +
> > +- minval:		Optional minimum acceptable value
> > +
> > +- maxval:		Optional maximum acceptable value
> > +
> 
> I assume valid only for numeric types and such values as are valid for the type?
> 
> Or does minval/maxval indicate minlength/maxlength for a string as an easy form
> of filtering?

It was only for numbers but I can overload that to mean string size
limits too.  As for string size checks, I'll add with the first string
tunable that actually needs it.

> > +- env_alias:		An alias environment variable
> > +
> > +- secure_env_alias:	Specify whether the environment variable should be read
> > +			for setuid binaries.
> 
> We should make a few things clear here:
> 
> * The tunable may still be ignored in AT_SECURE if the non-default value
>   fails to pass some level of runtime checking e.g. value could break the
>   application.
> 
> * The 'secure_env_alias' should really mean "Specify whether the environment
>   variable should be read for setuid binaries, 0 (default) means they are not
>   read and 1 means they are. Even if the variable is read it may be ignored."

OK, I'll extend the meaning of that for the tunable in general, not
just its alias.  That way, the meaning does not change between the
alias and the tunable.

> Are there any restrictions on the callback? I assume the foreign function (callback)
> is called from _any_ context so you have to be very very very careful about what
> functions you call?

The callback is currently only called from one context: the module
initialization.  In future it may get called later, i.e. when a
tunable is changed at runtime.  I prefer to think of the latter case
later if that is OK.

> Can we unit test the interface to make sure the API works as designed?
> 
> https://sourceware.org/ml/libc-alpha/2016-07/msg00110.html

Let me see if I can do that, but not as part of this patch.

> Can we pass back a void* to return success/failure like the
> pthreads API does? I can see a short term need to know if the
> foreign function call failed.

I can, but can you elaborate on why we would need that?  The call is
effectively going to happen in the same context in which the callback
is defined, i.e. we won't have a case where the callback is in one
module and the initialization is in another.

> > +#define RETURN_IF_INVALID_RANGE(__cur, __val) \
> > +({									      \
> > +  __typeof ((__cur)->type.min) min = (__cur)->type.min;			      \
> > +  __typeof ((__cur)->type.max) max = (__cur)->type.max;			      \
> > +  if (min != max && ((__val) < min || (__val) > max))			      \
> > +    return;								      \
> > +})
> 
> As mentioned above, might be nice to have min/max limit string min
> and max length.

I'll add string validation when we have an actual tunable that needs it.

> Add "May still be ignored if value is dangerous."

How about "May still be ignored if the value does not meet criteria
for allowed values for the tunable."?  That seems more precise since
we don't really have a way to identify dangerous values beyond the
validation we have in place.

> Two things are wrong.
> 
> Missing systemtap probes, _and_ wrong settings.

I skipped the probes on purpose thinking that it doesn't make sense to
hit them anyway, but I guess it is required since it changes
behaviour.  I'll add it.

> 
> For example setting top_pad should disable dynamic thresholding.

Yeah, I missed that.  I'll review all of the options and fix up.

> 
> Therefore you need to do the following:
> 
> (a) Take __libc_mallopt from malloc/malloc.c and split out the
>     setting of each variable into a "callback"
> 
> (b) Have __libc_mallopt call the callbacks.
> 
> (c) Have TUNABLE_SET_VAL call the callback via the tunables
>     mechanism.
> 
> That way you have code in one place for what has to happen
> when setting the variable.

That's a good idea.

Thanks,
Siddhesh