[PATCH 1/2] Add framework for tunables

[Florian Weimer]

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