public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 1/2] Add framework for tunables
  2016-07-02 17:13 [PATCH 0/2] tunables for glibc Siddhesh Poyarekar
@ 2016-07-02 17:13 ` Siddhesh Poyarekar
  2016-07-03 14:30   ` H.J. Lu
                     ` (2 more replies)
  2016-07-02 17:13 ` [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable Siddhesh Poyarekar
  2016-07-03  0:24 ` [PATCH 0/2] tunables for glibc H.J. Lu
  2 siblings, 3 replies; 29+ messages in thread
From: Siddhesh Poyarekar @ 2016-07-02 17:13 UTC (permalink / raw)
  To: libc-alpha

The tunables framework allows us to uniformly manage and expose global
variables inside glibc as switches to users.  README.tunables 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.
	* 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            |  45 ++++++++++
 elf/dl-tunables.c                 | 172 ++++++++++++++++++++++++++++++++++++++
 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, 693 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
+     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 901e253..70ab097 100644
--- a/Makeconfig
+++ b/Makeconfig
@@ -883,6 +883,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
+
 override CFLAGS	= -std=gnu11 -fgnu89-inline $(config-extra-cflags) \
 		  $(filter-out %frame-pointer,$(+cflags)) $(+gccwarn-c) \
 		  $(sysdep-CFLAGS) $(CFLAGS-$(suffix $@)) $(CFLAGS-$(<F)) \
@@ -1054,6 +1059,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
+
 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.
+
+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.
+
+- 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.
+
+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.
+
+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
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
+
 #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@
 
 # 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
 
 /* 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);
+#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
+
 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;
   }
 }
diff --git a/elf/dl-tunable-types.h b/elf/dl-tunable-types.h
new file mode 100644
index 0000000..ecbb197
--- /dev/null
+++ b/elf/dl-tunable-types.h
@@ -0,0 +1,45 @@
+/* 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);
+
+typedef enum
+{
+  TUNABLE_TYPE_INT_32,
+  TUNABLE_TYPE_SIZE_T
+} tunable_type_code_t;
+
+typedef struct
+{
+  tunable_type_code_t type_code;
+  int64_t min;
+  int64_t max;
+} tunable_type_t;
+
+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..8a26dfc
--- /dev/null
+++ b/elf/dl-tunables.c
@@ -0,0 +1,172 @@
+/* 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;
+}
+
+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;
+}
+
+
+#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;								      \
+})
+
+/* 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;
+	}
+    default:
+      __builtin_unreachable ();
+    }
+}
+
+/* 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;
+	    }
+	}
+    }
+}
+
+/* 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, void (*callback) (void))
+{
+  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;
+	}
+    default:
+      __builtin_unreachable ();
+    }
+
+  if (callback)
+    callback ();
+}
diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h
new file mode 100644
index 0000000..93bcad7
--- /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.  */
+};
+
+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 *, void (*) (void));
+
+/* 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
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
+    }
+  }
+}
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
+
   /* 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..34816f2 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)
 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 = MALLOC_CHECK_=3
 
 # 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..9cc7099 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)
+{
+  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);
+  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)
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"
+}
-- 
2.5.5

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable
  2016-07-02 17:13 [PATCH 0/2] tunables for glibc Siddhesh Poyarekar
  2016-07-02 17:13 ` [PATCH 1/2] Add framework for tunables Siddhesh Poyarekar
@ 2016-07-02 17:13 ` Siddhesh Poyarekar
  2016-07-03 14:39   ` H.J. Lu
  2016-07-03 15:53   ` H.J. Lu
  2016-07-03  0:24 ` [PATCH 0/2] tunables for glibc H.J. Lu
  2 siblings, 2 replies; 29+ messages in thread
From: Siddhesh Poyarekar @ 2016-07-02 17:13 UTC (permalink / raw)
  To: libc-alpha

Read tunables values from the users using the GLIBC_TUNABLES
environment variable.  The value of this variable is a colon-separated
list of name=value pairs.  So a typical string would look like this:

GLIBC_TUNABLES=glibc.malloc.mmap_threshold=2048:glibc.malloc.trim_threshold=1024

	* elf/dl-tunables.c: Include string.h.
	(parse_tunables): New function.
	(GLIBC_TUNABLES): New macro.
	(__tunables_init): Use it.
---
 elf/dl-tunables.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index 8a26dfc..04d1d52 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -22,10 +22,13 @@
 #include <stdbool.h>
 #include <unistd.h>
 #include <stdlib.h>
+#include <string.h>
 
 #define TUNABLES_INTERNAL 1
 #include "dl-tunables.h"
 
+#define GLIBC_TUNABLES "GLIBC_TUNABLES"
+
 /* Compare environment names, bounded by the name hardcoded in glibc.  */
 static bool
 is_name (const char *orig, const char *envname)
@@ -104,6 +107,66 @@ tunable_initialize (tunable_t *cur)
     }
 }
 
+static void
+parse_tunables (char *tunestr)
+{
+  if (tunestr == NULL || *tunestr == '\0')
+    return;
+
+  char *p = tunestr;
+
+  while (true)
+    {
+      char *name = p;
+      size_t len = 0;
+
+      /* First, find where the name ends.  */
+      while (p[len] != '=' && p[len] != ':' && p[len] != '\0')
+	len++;
+
+      /* If we reach the end of the string before getting a valid name-value
+	 pair, bail out.  */
+      if (p[len] == '\0')
+	return;
+
+      /* We did not find a valid name-value pair before encountering the
+	 colon.  */
+      if (p[len]== ':')
+	{
+	  p += len + 1;
+	  continue;
+	}
+
+      p += len + 1;
+
+      char *value = p;
+      len = 0;
+
+      while (p[len] != ':' && p[len] != '\0')
+	len++;
+
+      char end = p[len];
+      p[len] = '\0';
+
+      /* Add the tunable if it exists.  */
+      for (size_t i = 0; i < sizeof (tunable_list) / sizeof (tunable_t); i++)
+	{
+	  if (is_name (tunable_list[i].name, name))
+	    {
+	      /* Tunable values take precedence over the env_alias.  */
+	      tunable_list[i].strval = value;
+	      tunable_initialize (&tunable_list[i]);
+	      break;
+	    }
+	}
+
+      if (end == ':')
+	p += len + 1;
+      else
+	return;
+    }
+}
+
 /* 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.  */
@@ -116,6 +179,12 @@ __tunables_init (char **envp)
 
   while (get_next_env (&envp, &envname, &len, &envval))
     {
+      if (is_name (GLIBC_TUNABLES, envname))
+	{
+	  parse_tunables (strdup (envval));
+	  continue;
+	}
+
       for (int i = 0; i < sizeof (tunable_list) / sizeof (tunable_t); i++)
 	{
 	  tunable_t *cur = &tunable_list[i];
-- 
2.5.5

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 0/2] tunables for glibc
@ 2016-07-02 17:13 Siddhesh Poyarekar
  2016-07-02 17:13 ` [PATCH 1/2] Add framework for tunables Siddhesh Poyarekar
                   ` (2 more replies)
  0 siblings, 3 replies; 29+ messages in thread
From: Siddhesh Poyarekar @ 2016-07-02 17:13 UTC (permalink / raw)
  To: libc-alpha

Hi,

Here's another stab at the tunables patchset.  This patch now gets tunables
working with static binaries as well.

The tunables framework aims to provide a simple and consistent interface for
the library to expose tuning switches in a much more coherent manner to
userspace.  In its initial form, the framework will allow users to tweak
certain parts of the library using an environment variable (or set of
environment variables, depending on where the consensus sends us).  In future,
this could be extended to per-user or even systemwide tuning switches.

The first patch implements a bare framework that moves the environment
variables in the malloc module to the tunables framework without adding any new
user-visible functionality.  The environment variables that were implemented
earlier should work as is.

The second patch adds support for a GLIBC_TUNABLES environment variable, which
users can use to set up tunables instead of using the old environment
variables.  The alternative approach to this is to expose namespace-consistent
environment variables for each tunable for userspace to take advantage of
instead of the earlier environment variables.

Siddhesh Poyarekar (2):
  Add framework for tunables
  Initialize tunable list with the GLIBC_TUNABLES environment variable

 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            |  45 +++++++
 elf/dl-tunables.c                 | 241 ++++++++++++++++++++++++++++++++++++++
 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, 762 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

-- 
2.5.5

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 0/2] tunables for glibc
  2016-07-02 17:13 [PATCH 0/2] tunables for glibc Siddhesh Poyarekar
  2016-07-02 17:13 ` [PATCH 1/2] Add framework for tunables Siddhesh Poyarekar
  2016-07-02 17:13 ` [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable Siddhesh Poyarekar
@ 2016-07-03  0:24 ` H.J. Lu
  2016-07-03  3:08   ` Siddhesh Poyarekar
  2 siblings, 1 reply; 29+ messages in thread
From: H.J. Lu @ 2016-07-03  0:24 UTC (permalink / raw)
  To: Siddhesh Poyarekar; +Cc: GNU C Library

On Sat, Jul 2, 2016 at 10:12 AM, Siddhesh Poyarekar
<siddhesh@sourceware.org> wrote:
> Hi,
>
> Here's another stab at the tunables patchset.  This patch now gets tunables
> working with static binaries as well.
>
> The tunables framework aims to provide a simple and consistent interface for
> the library to expose tuning switches in a much more coherent manner to
> userspace.  In its initial form, the framework will allow users to tweak
> certain parts of the library using an environment variable (or set of
> environment variables, depending on where the consensus sends us).  In future,
> this could be extended to per-user or even systemwide tuning switches.
>
> The first patch implements a bare framework that moves the environment
> variables in the malloc module to the tunables framework without adding any new
> user-visible functionality.  The environment variables that were implemented
> earlier should work as is.
>
> The second patch adds support for a GLIBC_TUNABLES environment variable, which
> users can use to set up tunables instead of using the old environment
> variables.  The alternative approach to this is to expose namespace-consistent
> environment variables for each tunable for userspace to take advantage of
> instead of the earlier environment variables.
>
> Siddhesh Poyarekar (2):
>   Add framework for tunables
>   Initialize tunable list with the GLIBC_TUNABLES environment variable
>
>  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            |  45 +++++++
>  elf/dl-tunables.c                 | 241 ++++++++++++++++++++++++++++++++++++++
>  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, 762 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
>

Can you push you new patches to siddhesh/tunables branch so
that I can give it a try?

Thanks.

-- 
H.J.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 0/2] tunables for glibc
  2016-07-03  0:24 ` [PATCH 0/2] tunables for glibc H.J. Lu
@ 2016-07-03  3:08   ` Siddhesh Poyarekar
  0 siblings, 0 replies; 29+ messages in thread
From: Siddhesh Poyarekar @ 2016-07-03  3:08 UTC (permalink / raw)
  To: H.J. Lu; +Cc: GNU C Library

On Sat, Jul 02, 2016 at 05:24:40PM -0700, H.J. Lu wrote:
> Can you push you new patches to siddhesh/tunables branch so
> that I can give it a try?

Oops, I thought I had already done that.  I've pushed it now.

Thanks,
Siddhesh

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 1/2] Add framework for tunables
  2016-07-02 17:13 ` [PATCH 1/2] Add framework for tunables Siddhesh Poyarekar
@ 2016-07-03 14:30   ` H.J. Lu
  2016-07-03 17:08     ` Siddhesh Poyarekar
  2016-07-03 14:44   ` H.J. Lu
  2016-07-03 15:13   ` H.J. Lu
  2 siblings, 1 reply; 29+ messages in thread
From: H.J. Lu @ 2016-07-03 14:30 UTC (permalink / raw)
  To: Siddhesh Poyarekar; +Cc: GNU C Library

On Sat, Jul 2, 2016 at 10:12 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.  README.tunables 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.
>         * 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.
>
> 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
>
>  /* 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);
> +#endif
> +
>    /* First the initialization which normally would be done by the
>       dynamic linker.  */
>    _dl_non_dynamic_init ();

Shouldn't __tunables_init be called from _dl_non_dynamic_init?

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

Shouldn't __tunables_init be called from _dl_sysdep_start?

>    /* This is the default place for profiling data file.  */
>    GLRO(dl_profile_output)
>      = &"/var/tmp\0/var/profile"[__libc_enable_secure ? 9 : 0];

I


-- 
H.J.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable
  2016-07-02 17:13 ` [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable Siddhesh Poyarekar
@ 2016-07-03 14:39   ` H.J. Lu
  2016-07-03 17:15     ` Siddhesh Poyarekar
  2016-07-03 15:53   ` H.J. Lu
  1 sibling, 1 reply; 29+ messages in thread
From: H.J. Lu @ 2016-07-03 14:39 UTC (permalink / raw)
  To: Siddhesh Poyarekar; +Cc: GNU C Library

On Sat, Jul 2, 2016 at 10:12 AM, Siddhesh Poyarekar
<siddhesh@sourceware.org> wrote:
> Read tunables values from the users using the GLIBC_TUNABLES
> environment variable.  The value of this variable is a colon-separated
> list of name=value pairs.  So a typical string would look like this:
>
> GLIBC_TUNABLES=glibc.malloc.mmap_threshold=2048:glibc.malloc.trim_threshold=1024
>
>         * elf/dl-tunables.c: Include string.h.
>         (parse_tunables): New function.
>         (GLIBC_TUNABLES): New macro.
>         (__tunables_init): Use it.
> ---
>  elf/dl-tunables.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
>
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index 8a26dfc..04d1d52 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -22,10 +22,13 @@
>  #include <stdbool.h>
>  #include <unistd.h>
>  #include <stdlib.h>
> +#include <string.h>
>
>  #define TUNABLES_INTERNAL 1
>  #include "dl-tunables.h"
>
> +#define GLIBC_TUNABLES "GLIBC_TUNABLES"
> +
>  /* Compare environment names, bounded by the name hardcoded in glibc.  */
>  static bool
>  is_name (const char *orig, const char *envname)
> @@ -104,6 +107,66 @@ tunable_initialize (tunable_t *cur)
>      }
>  }
>
> +static void
> +parse_tunables (char *tunestr)
> +{
> +  if (tunestr == NULL || *tunestr == '\0')
> +    return;
> +
> +  char *p = tunestr;
> +
> +  while (true)
> +    {
> +      char *name = p;
> +      size_t len = 0;
> +
> +      /* First, find where the name ends.  */
> +      while (p[len] != '=' && p[len] != ':' && p[len] != '\0')
> +       len++;
> +
> +      /* If we reach the end of the string before getting a valid name-value
> +        pair, bail out.  */
> +      if (p[len] == '\0')
> +       return;
> +
> +      /* We did not find a valid name-value pair before encountering the
> +        colon.  */
> +      if (p[len]== ':')
> +       {
> +         p += len + 1;
> +         continue;
> +       }
> +
> +      p += len + 1;
> +
> +      char *value = p;
> +      len = 0;
> +
> +      while (p[len] != ':' && p[len] != '\0')
> +       len++;
> +
> +      char end = p[len];
> +      p[len] = '\0';
> +
> +      /* Add the tunable if it exists.  */
> +      for (size_t i = 0; i < sizeof (tunable_list) / sizeof (tunable_t); i++)
> +       {
> +         if (is_name (tunable_list[i].name, name))

Can you add the length of name to tunable_list field and check
if the length matches first?

> +           {
> +             /* Tunable values take precedence over the env_alias.  */
> +             tunable_list[i].strval = value;
> +             tunable_initialize (&tunable_list[i]);
> +             break;
> +           }
> +       }
> +
> +      if (end == ':')
> +       p += len + 1;
> +      else
> +       return;
> +    }
> +}
> +
>  /* 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.  */
> @@ -116,6 +179,12 @@ __tunables_init (char **envp)
>
>    while (get_next_env (&envp, &envname, &len, &envval))
>      {
> +      if (is_name (GLIBC_TUNABLES, envname))
> +       {
> +         parse_tunables (strdup (envval));
> +         continue;
> +       }
> +
>        for (int i = 0; i < sizeof (tunable_list) / sizeof (tunable_t); i++)
>         {
>           tunable_t *cur = &tunable_list[i];
> --
> 2.5.5
>



-- 
H.J.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 1/2] Add framework for tunables
  2016-07-02 17:13 ` [PATCH 1/2] Add framework for tunables Siddhesh Poyarekar
  2016-07-03 14:30   ` H.J. Lu
@ 2016-07-03 14:44   ` H.J. Lu
  2016-07-03 17:14     ` Siddhesh Poyarekar
  2016-07-03 15:13   ` H.J. Lu
  2 siblings, 1 reply; 29+ messages in thread
From: H.J. Lu @ 2016-07-03 14:44 UTC (permalink / raw)
  To: Siddhesh Poyarekar; +Cc: GNU C Library

On Sat, Jul 2, 2016 at 10:12 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.  README.tunables 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.
>         * 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.

> +
> +/* 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;
> +}

Can you add the length parameter to is_name and make is_name
processor specific so that misaligned short/int/long long load can
be used, similar to equal in

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/x86/cpu-features.c;h=3ae44cfb86e39b41c43903e403325541476b37bf;hb=refs/heads/hjl/ifunc/master

H.J.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 1/2] Add framework for tunables
  2016-07-02 17:13 ` [PATCH 1/2] Add framework for tunables Siddhesh Poyarekar
  2016-07-03 14:30   ` H.J. Lu
  2016-07-03 14:44   ` H.J. Lu
@ 2016-07-03 15:13   ` H.J. Lu
  2016-07-03 17:43     ` H.J. Lu
  2 siblings, 1 reply; 29+ messages in thread
From: H.J. Lu @ 2016-07-03 15:13 UTC (permalink / raw)
  To: Siddhesh Poyarekar; +Cc: GNU C Library

On Sat, Jul 2, 2016 at 10:12 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.  README.tunables 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.
>         * 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            |  45 ++++++++++
>  elf/dl-tunables.c                 | 172 ++++++++++++++++++++++++++++++++++++++
>  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, 693 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
>


> +/* 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

For IFUNC, or any processor specific tunables, a different callback is
needed:

void
ifunc_callback (const char *p)
{
...
}

where p is the string after "GLIBC_IFUNC=".

H.J.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable
  2016-07-02 17:13 ` [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable Siddhesh Poyarekar
  2016-07-03 14:39   ` H.J. Lu
@ 2016-07-03 15:53   ` H.J. Lu
  2016-07-03 17:18     ` Siddhesh Poyarekar
  1 sibling, 1 reply; 29+ messages in thread
From: H.J. Lu @ 2016-07-03 15:53 UTC (permalink / raw)
  To: Siddhesh Poyarekar; +Cc: GNU C Library

On Sat, Jul 2, 2016 at 10:12 AM, Siddhesh Poyarekar
<siddhesh@sourceware.org> wrote:
> Read tunables values from the users using the GLIBC_TUNABLES
> environment variable.  The value of this variable is a colon-separated
> list of name=value pairs.  So a typical string would look like this:
>
> GLIBC_TUNABLES=glibc.malloc.mmap_threshold=2048:glibc.malloc.trim_threshold=1024
>
>         * elf/dl-tunables.c: Include string.h.
>         (parse_tunables): New function.
>         (GLIBC_TUNABLES): New macro.
>         (__tunables_init): Use it.

I didn't see any tests for GLIBC_TUNABLES=glibc.malloc and I don't think
it works for static executable:

gdb) bt
#0  __tunable_set_val (id=glibc_malloc_check, valp=0x6b67d0 <check_action>,
    callback=0x414460 <_dl_tunable_set_mallopt_check>) at dl-tunables.c:221
#1  0x0000000000413cda in ptmalloc_init () at arena.c:299
#2  0x0000000000414dfe in ptmalloc_init () at hooks.c:29
#3  malloc_hook_ini (sz=66, caller=<optimized out>) at hooks.c:31
#4  0x0000000000418b2a in __strdup (
    s=0x7fffffffef66
"glibc.malloc.mmap_threshold=2048:glibc.malloc.trim_threshold=1024")
at strdup.c:42
#5  0x0000000000437e01 in __tunables_init (envp=0x7fffffffe088,
    envp@entry=0x7fffffffdeb8) at dl-tunables.c:184
#6  0x000000000043908b in __libc_init_first (argc=argc@entry=1,
    argv=argv@entry=0x7fffffffdea8, envp=0x7fffffffdeb8)
    at ../csu/init-first.c:81
#7  0x00000000004012d5 in generic_start_main (main=0x400550 <main>, argc=1,
    argv=argv@entry=0x7fffffffdea8,
    init=init@entry=0x401b20 <__libc_csu_init>,
    fini=0x401bb0 <__libc_csu_fini>, rtld_fini=0x0, stack_end=0x7fffffffde98)
    at ../csu/libc-start.c:225
#8  0x00000000004015c7 in __libc_start_main (main=<optimized out>,
    argc=<optimized out>, argv=0x7fffffffdea8,
    init=0x401b20 <__libc_csu_init>, fini=0x401bb0 <__libc_csu_fini>,
    rtld_fini=<optimized out>, stack_end=0x7fffffffde98)
    at ../sysdeps/x86/libc-start.c:38
---Type <return> to continue, or q <return> to quit---
#9  0x0000000000400e1a in _start () at ../sysdeps/x86_64/start.S:120

     if (is_name (GLIBC_TUNABLES, envname))
        {
          parse_tunables (strdup (envval));
                                ^^^^^^^^^  Which calls malloc and
other library functions.
          continue;
        }

Since tunable code is called very early, it can't use any C library functions.

-- 
H.J.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 1/2] Add framework for tunables
  2016-07-03 14:30   ` H.J. Lu
@ 2016-07-03 17:08     ` Siddhesh Poyarekar
  0 siblings, 0 replies; 29+ messages in thread
From: Siddhesh Poyarekar @ 2016-07-03 17:08 UTC (permalink / raw)
  To: H.J. Lu; +Cc: GNU C Library

On Sun, Jul 03, 2016 at 07:30:30AM -0700, H.J. Lu wrote:
> Shouldn't __tunables_init be called from _dl_non_dynamic_init?
> 
<snip>
> 
> Shouldn't __tunables_init be called from _dl_sysdep_start?

It needs to be earlier.  Eventually I intend to put dynamic linker
envvars also into the tunables.

Siddhesh

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 1/2] Add framework for tunables
  2016-07-03 14:44   ` H.J. Lu
@ 2016-07-03 17:14     ` Siddhesh Poyarekar
  2016-07-03 17:41       ` H.J. Lu
  0 siblings, 1 reply; 29+ messages in thread
From: Siddhesh Poyarekar @ 2016-07-03 17:14 UTC (permalink / raw)
  To: H.J. Lu; +Cc: GNU C Library

On Sun, Jul 03, 2016 at 07:44:25AM -0700, H.J. Lu wrote:
> > +
> > +/* 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;
> > +}
> 
> Can you add the length parameter to is_name and make is_name
> processor specific so that misaligned short/int/long long load can
> be used, similar to equal in

Will it really make that much of an impact?  These will be really
short comparisons (less than 16 bytes in most cases) and will really
only happen once during startup.

Maybe we could do it as an addon patch if the tunables initialization
really turns out to be that much of a bottleneck.

Siddhesh

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable
  2016-07-03 14:39   ` H.J. Lu
@ 2016-07-03 17:15     ` Siddhesh Poyarekar
  0 siblings, 0 replies; 29+ messages in thread
From: Siddhesh Poyarekar @ 2016-07-03 17:15 UTC (permalink / raw)
  To: H.J. Lu; +Cc: GNU C Library

On Sun, Jul 03, 2016 at 07:38:54AM -0700, H.J. Lu wrote:
> Can you add the length of name to tunable_list field and check
> if the length matches first?

Similar to passing the length in is_name, can we do this as an addon
if we see a significant performance impact?

Siddhesh

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable
  2016-07-03 15:53   ` H.J. Lu
@ 2016-07-03 17:18     ` Siddhesh Poyarekar
  0 siblings, 0 replies; 29+ messages in thread
From: Siddhesh Poyarekar @ 2016-07-03 17:18 UTC (permalink / raw)
  To: H.J. Lu; +Cc: GNU C Library

On Sun, Jul 03, 2016 at 08:53:12AM -0700, H.J. Lu wrote:
> I didn't see any tests for GLIBC_TUNABLES=glibc.malloc and I don't think
> it works for static executable:

Hmm, right, I need to fix this.  I had manually run the
tst-malloc-usable patch with GLIBC_TUNABLES but didn't do it for
static case again.  I'll fix this up, write those tests and repost.

Thanks,
Siddhesh

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 1/2] Add framework for tunables
  2016-07-03 17:14     ` Siddhesh Poyarekar
@ 2016-07-03 17:41       ` H.J. Lu
  0 siblings, 0 replies; 29+ messages in thread
From: H.J. Lu @ 2016-07-03 17:41 UTC (permalink / raw)
  To: Siddhesh Poyarekar; +Cc: GNU C Library

On Sun, Jul 3, 2016 at 10:13 AM, Siddhesh Poyarekar
<siddhesh@sourceware.org> wrote:
> On Sun, Jul 03, 2016 at 07:44:25AM -0700, H.J. Lu wrote:
>> > +
>> > +/* 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;
>> > +}
>>
>> Can you add the length parameter to is_name and make is_name
>> processor specific so that misaligned short/int/long long load can
>> be used, similar to equal in
>
> Will it really make that much of an impact?  These will be really
> short comparisons (less than 16 bytes in most cases) and will really
> only happen once during startup.
>
> Maybe we could do it as an addon patch if the tunables initialization
> really turns out to be that much of a bottleneck.
>

Sure.

-- 
H.J.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 1/2] Add framework for tunables
  2016-07-03 15:13   ` H.J. Lu
@ 2016-07-03 17:43     ` H.J. Lu
  2016-07-03 18:20       ` Siddhesh Poyarekar
  0 siblings, 1 reply; 29+ messages in thread
From: H.J. Lu @ 2016-07-03 17:43 UTC (permalink / raw)
  To: Siddhesh Poyarekar; +Cc: GNU C Library

On Sun, Jul 3, 2016 at 8:13 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Sat, Jul 2, 2016 at 10:12 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.  README.tunables 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.
>>         * 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.
>

>
>> +/* 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
>
> For IFUNC, or any processor specific tunables, a different callback is
> needed:
>
> void
> ifunc_callback (const char *p)
> {
> ...
> }
>
> where p is the string after "GLIBC_IFUNC=".
>
> H.J.

This should be an option for __tunables_init not to parse the
string, but instead pass the string to callback directly.

-- 
H.J.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 1/2] Add framework for tunables
  2016-07-03 17:43     ` H.J. Lu
@ 2016-07-03 18:20       ` Siddhesh Poyarekar
  0 siblings, 0 replies; 29+ messages in thread
From: Siddhesh Poyarekar @ 2016-07-03 18:20 UTC (permalink / raw)
  To: H.J. Lu; +Cc: GNU C Library

On Sun, Jul 03, 2016 at 10:43:36AM -0700, H.J. Lu wrote:
> This should be an option for __tunables_init not to parse the
> string, but instead pass the string to callback directly.

If the tunable type is string then right now it won't do any parsing,
just assign the value in the tunable_val_t.  I could modify the
callback to:

    void (*callback) (void *)

That way the value is sent back via the callback regardless of the
type.  The callback can then cast the value appropriately and use it.

Siddhesh

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable
  2016-08-15 20:05 [PATCHv4 " Siddhesh Poyarekar
@ 2016-08-15 20:05 ` Siddhesh Poyarekar
  0 siblings, 0 replies; 29+ messages in thread
From: Siddhesh Poyarekar @ 2016-08-15 20:05 UTC (permalink / raw)
  To: libc-alpha; +Cc: carlos, fweimer

Read tunables values from the users using the GLIBC_TUNABLES
environment variable.  The value of this variable is a colon-separated
list of name=value pairs.  So a typical string would look like this:

GLIBC_TUNABLES=glibc.malloc.mmap_threshold=2048:glibc.malloc.trim_threshold=1024

	* elf/dl-tunables.c: Include mman.h and libc-internals.h.
	(tunables_strdup): New function.
	(parse_tunables): New function.
	(GLIBC_TUNABLES): New macro.
	(__tunables_init): Use the new functions and macro.
	* malloc/tst-malloc-usable-tunables.c: New test case.
	* malloc/tst-malloc-usable-static-tunables.c: New test case.
	* malloc/Makefile (tests, tests-static): Add tests.
---
 elf/dl-tunables.c                          | 99 ++++++++++++++++++++++++++++++
 malloc/Makefile                            |  6 +-
 malloc/tst-malloc-usable-static-tunables.c |  1 +
 malloc/tst-malloc-usable-tunables.c        |  1 +
 4 files changed, 105 insertions(+), 2 deletions(-)
 create mode 100644 malloc/tst-malloc-usable-static-tunables.c
 create mode 100644 malloc/tst-malloc-usable-tunables.c

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index b8f8fc0..ff71e9d 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -22,10 +22,14 @@
 #include <stdbool.h>
 #include <unistd.h>
 #include <stdlib.h>
+#include <sys/mman.h>
+#include <libc-internal.h>
 
 #define TUNABLES_INTERNAL 1
 #include "dl-tunables.h"
 
+#define GLIBC_TUNABLES "GLIBC_TUNABLES"
+
 /* Compare environment names, bounded by the name hardcoded in glibc.  */
 static bool
 is_name (const char *orig, const char *envname)
@@ -41,6 +45,27 @@ is_name (const char *orig, const char *envname)
     return false;
 }
 
+static char *tunables_strdup (const char *in)
+{
+  size_t i = 0;
+
+  while (in[i++]);
+
+  char *out = __mmap (NULL, ALIGN_UP (i, __getpagesize ()),
+		      PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, -1,
+		      0);
+
+  if (out == MAP_FAILED)
+    return NULL;
+
+  i--;
+
+  while (i-- > 0)
+    out[i] = in[i];
+
+  return out;
+}
+
 static char **
 get_next_env (char **envp, char **name, size_t *namelen, char **val)
 {
@@ -109,6 +134,72 @@ tunable_initialize (tunable_t *cur, const char *strval)
     }
 }
 
+static void
+parse_tunables (char *tunestr)
+{
+  if (tunestr == NULL || *tunestr == '\0')
+    return;
+
+  char *p = tunestr;
+
+  while (true)
+    {
+      char *name = p;
+      size_t len = 0;
+
+      /* First, find where the name ends.  */
+      while (p[len] != '=' && p[len] != ':' && p[len] != '\0')
+	len++;
+
+      /* If we reach the end of the string before getting a valid name-value
+	 pair, bail out.  */
+      if (p[len] == '\0')
+	return;
+
+      /* We did not find a valid name-value pair before encountering the
+	 colon.  */
+      if (p[len]== ':')
+	{
+	  p += len + 1;
+	  continue;
+	}
+
+      p += len + 1;
+
+      char *value = p;
+      len = 0;
+
+      while (p[len] != ':' && p[len] != '\0')
+	len++;
+
+      char end = p[len];
+      p[len] = '\0';
+
+      /* Add the tunable if it exists.  */
+      for (size_t i = 0; i < sizeof (tunable_list) / sizeof (tunable_t); i++)
+	{
+	  tunable_t *cur = &tunable_list[i];
+
+	  /* If we are in a secure context (AT_SECURE) then ignore the tunable
+	     unless it is explicitly marked as secure.  Tunable values take
+	     precendence over their envvar aliases.  */
+	  if (__libc_enable_secure && !cur->is_secure)
+	    continue;
+
+	  if (is_name (cur->name, name))
+	    {
+	      tunable_initialize (cur, value);
+	      break;
+	    }
+	}
+
+      if (end == ':')
+	p += len + 1;
+      else
+	return;
+    }
+}
+
 /* 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.  */
@@ -121,6 +212,14 @@ __tunables_init (char **envp)
 
   while ((envp = get_next_env (envp, &envname, &len, &envval)) != NULL)
     {
+      if (is_name (GLIBC_TUNABLES, envname))
+	{
+	  char *val = tunables_strdup (envval);
+	  if (val != NULL)
+	    parse_tunables (val);
+	  continue;
+	}
+
       for (int i = 0; i < sizeof (tunable_list) / sizeof (tunable_t); i++)
 	{
 	  tunable_t *cur = &tunable_list[i];
diff --git a/malloc/Makefile b/malloc/Makefile
index de9aafe..599ae9d 100644
--- a/malloc/Makefile
+++ b/malloc/Makefile
@@ -30,8 +30,8 @@ tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
 	 tst-pvalloc tst-memalign tst-mallopt tst-scratch_buffer \
 	 tst-malloc-backtrace tst-malloc-thread-exit \
 	 tst-malloc-thread-fail tst-malloc-fork-deadlock \
-	 tst-mallocfork2
-tests-static := tst-malloc-usable-static
+	 tst-mallocfork2 tst-malloc-usable-tunables
+tests-static := tst-malloc-usable-static tst-malloc-usable-static-tunables
 tests += $(tests-static)
 test-srcs = tst-mtrace
 
@@ -141,6 +141,8 @@ endif
 tst-mcheck-ENV = MALLOC_CHECK_=3
 tst-malloc-usable-ENV = MALLOC_CHECK_=3
 tst-malloc-usable-static-ENV = $(tst-malloc-usable-ENV)
+tst-malloc-usable-tunables-ENV = GLIBC_TUNABLES=glibc.malloc.check=3
+tst-malloc-usable-static-tunables-ENV = $(tst-malloc-usable-tunables-ENV)
 
 # Uncomment this for test releases.  For public releases it is too expensive.
 #CPPFLAGS-malloc.o += -DMALLOC_DEBUG=1
diff --git a/malloc/tst-malloc-usable-static-tunables.c b/malloc/tst-malloc-usable-static-tunables.c
new file mode 100644
index 0000000..8907db0
--- /dev/null
+++ b/malloc/tst-malloc-usable-static-tunables.c
@@ -0,0 +1 @@
+#include <malloc/tst-malloc-usable.c>
diff --git a/malloc/tst-malloc-usable-tunables.c b/malloc/tst-malloc-usable-tunables.c
new file mode 100644
index 0000000..8907db0
--- /dev/null
+++ b/malloc/tst-malloc-usable-tunables.c
@@ -0,0 +1 @@
+#include <malloc/tst-malloc-usable.c>
-- 
2.7.4

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable
  2016-07-09 18:49 [PATCHv3 0/2] tunables for glibc Siddhesh Poyarekar
@ 2016-07-09 18:49 ` Siddhesh Poyarekar
  0 siblings, 0 replies; 29+ messages in thread
From: Siddhesh Poyarekar @ 2016-07-09 18:49 UTC (permalink / raw)
  To: libc-alpha

Read tunables values from the users using the GLIBC_TUNABLES
environment variable.  The value of this variable is a colon-separated
list of name=value pairs.  So a typical string would look like this:

GLIBC_TUNABLES=glibc.malloc.mmap_threshold=2048:glibc.malloc.trim_threshold=1024

	* elf/dl-tunables.c: Include mman.h and libc-internals.h.
	(tunables_strdup): New function.
	(parse_tunables): New function.
	(GLIBC_TUNABLES): New macro.
	(__tunables_init): Use the new functions and macro.
	* malloc/tst-malloc-usable-tunables.c: New test case.
	* malloc/tst-malloc-usable-static-tunables.c: New test case.
	* malloc/Makefile (tests, tests-static): Add tests.
---
 elf/dl-tunables.c                          | 93 ++++++++++++++++++++++++++++++
 malloc/Makefile                            |  6 +-
 malloc/tst-malloc-usable-static-tunables.c |  1 +
 malloc/tst-malloc-usable-tunables.c        |  1 +
 4 files changed, 99 insertions(+), 2 deletions(-)
 create mode 100644 malloc/tst-malloc-usable-static-tunables.c
 create mode 100644 malloc/tst-malloc-usable-tunables.c

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index ee8c2f0..8ed66c8 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -22,10 +22,14 @@
 #include <stdbool.h>
 #include <unistd.h>
 #include <stdlib.h>
+#include <sys/mman.h>
+#include <libc-internal.h>
 
 #define TUNABLES_INTERNAL 1
 #include "dl-tunables.h"
 
+#define GLIBC_TUNABLES "GLIBC_TUNABLES"
+
 /* Compare environment names, bounded by the name hardcoded in glibc.  */
 static bool
 is_name (const char *orig, const char *envname)
@@ -41,6 +45,27 @@ is_name (const char *orig, const char *envname)
     return false;
 }
 
+static char *tunables_strdup (const char *in)
+{
+  size_t i = 0;
+
+  while (in[i++]);
+
+  char *out = __mmap (NULL, ALIGN_UP (i, __getpagesize ()),
+		      PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, -1,
+		      0);
+
+  if (out == MAP_FAILED)
+    return NULL;
+
+  i--;
+
+  while (i-- > 0)
+    out[i] = in[i];
+
+  return out;
+}
+
 static bool
 get_next_env (char ***envp, char **name, size_t *namelen, char **val)
 {
@@ -109,6 +134,66 @@ tunable_initialize (tunable_t *cur)
     }
 }
 
+static void
+parse_tunables (char *tunestr)
+{
+  if (tunestr == NULL || *tunestr == '\0')
+    return;
+
+  char *p = tunestr;
+
+  while (true)
+    {
+      char *name = p;
+      size_t len = 0;
+
+      /* First, find where the name ends.  */
+      while (p[len] != '=' && p[len] != ':' && p[len] != '\0')
+	len++;
+
+      /* If we reach the end of the string before getting a valid name-value
+	 pair, bail out.  */
+      if (p[len] == '\0')
+	return;
+
+      /* We did not find a valid name-value pair before encountering the
+	 colon.  */
+      if (p[len]== ':')
+	{
+	  p += len + 1;
+	  continue;
+	}
+
+      p += len + 1;
+
+      char *value = p;
+      len = 0;
+
+      while (p[len] != ':' && p[len] != '\0')
+	len++;
+
+      char end = p[len];
+      p[len] = '\0';
+
+      /* Add the tunable if it exists.  */
+      for (size_t i = 0; i < sizeof (tunable_list) / sizeof (tunable_t); i++)
+	{
+	  if (is_name (tunable_list[i].name, name))
+	    {
+	      /* Tunable values take precedence over the env_alias.  */
+	      tunable_list[i].strval = value;
+	      tunable_initialize (&tunable_list[i]);
+	      break;
+	    }
+	}
+
+      if (end == ':')
+	p += len + 1;
+      else
+	return;
+    }
+}
+
 /* 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.  */
@@ -121,6 +206,14 @@ __tunables_init (char **envp)
 
   while (get_next_env (&envp, &envname, &len, &envval))
     {
+      if (is_name (GLIBC_TUNABLES, envname))
+	{
+	  char *val = tunables_strdup (envval);
+	  if (val != NULL)
+	    parse_tunables (val);
+	  continue;
+	}
+
       for (int i = 0; i < sizeof (tunable_list) / sizeof (tunable_t); i++)
 	{
 	  tunable_t *cur = &tunable_list[i];
diff --git a/malloc/Makefile b/malloc/Makefile
index 23ab2f4..ba34da1 100644
--- a/malloc/Makefile
+++ b/malloc/Makefile
@@ -30,8 +30,8 @@ tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
 	 tst-pvalloc tst-memalign tst-mallopt tst-scratch_buffer \
 	 tst-malloc-backtrace tst-malloc-thread-exit \
 	 tst-malloc-thread-fail tst-malloc-fork-deadlock \
-	 tst-mallocfork2
-tests-static := tst-malloc-usable-static
+	 tst-mallocfork2 tst-malloc-usable-tunables
+tests-static := tst-malloc-usable-static tst-malloc-usable-static-tunables
 tests += $(tests-static)
 test-srcs = tst-mtrace
 
@@ -141,6 +141,8 @@ endif
 tst-mcheck-ENV = MALLOC_CHECK_=3
 tst-malloc-usable-ENV = MALLOC_CHECK_=3
 tst-malloc-usable-static-ENV = $(tst-malloc-usable-ENV)
+tst-malloc-usable-tunables-ENV = GLIBC_TUNABLES=glibc.malloc.check=3
+tst-malloc-usable-static-tunables-ENV = $(tst-malloc-usable-tunables-ENV)
 
 # Uncomment this for test releases.  For public releases it is too expensive.
 #CPPFLAGS-malloc.o += -DMALLOC_DEBUG=1
diff --git a/malloc/tst-malloc-usable-static-tunables.c b/malloc/tst-malloc-usable-static-tunables.c
new file mode 100644
index 0000000..8907db0
--- /dev/null
+++ b/malloc/tst-malloc-usable-static-tunables.c
@@ -0,0 +1 @@
+#include <malloc/tst-malloc-usable.c>
diff --git a/malloc/tst-malloc-usable-tunables.c b/malloc/tst-malloc-usable-tunables.c
new file mode 100644
index 0000000..8907db0
--- /dev/null
+++ b/malloc/tst-malloc-usable-tunables.c
@@ -0,0 +1 @@
+#include <malloc/tst-malloc-usable.c>
-- 
2.5.5

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable
  2016-07-01 18:31 [PATCH 0/2] Tunables for glibc Siddhesh Poyarekar
@ 2016-07-01 18:31 ` Siddhesh Poyarekar
  0 siblings, 0 replies; 29+ messages in thread
From: Siddhesh Poyarekar @ 2016-07-01 18:31 UTC (permalink / raw)
  To: libc-alpha

Read tunables values from the users using the GLIBC_TUNABLES
environment variable.  The value of this variable is a colon-separated
list of name=value pairs.  So a typical string would look like this:

GLIBC_TUNABLES=glibc.malloc.mmap_threshold=2048:glibc.malloc.trim_threshold=1024

	* elf/dl-tunables.c: Include string.h.
	(parse_tunables): New function.
	(GLIBC_TUNABLES): New macro.
	(__tunables_init): Use it.
---
 elf/dl-tunables.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index 8a26dfc..04d1d52 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -22,10 +22,13 @@
 #include <stdbool.h>
 #include <unistd.h>
 #include <stdlib.h>
+#include <string.h>
 
 #define TUNABLES_INTERNAL 1
 #include "dl-tunables.h"
 
+#define GLIBC_TUNABLES "GLIBC_TUNABLES"
+
 /* Compare environment names, bounded by the name hardcoded in glibc.  */
 static bool
 is_name (const char *orig, const char *envname)
@@ -104,6 +107,66 @@ tunable_initialize (tunable_t *cur)
     }
 }
 
+static void
+parse_tunables (char *tunestr)
+{
+  if (tunestr == NULL || *tunestr == '\0')
+    return;
+
+  char *p = tunestr;
+
+  while (true)
+    {
+      char *name = p;
+      size_t len = 0;
+
+      /* First, find where the name ends.  */
+      while (p[len] != '=' && p[len] != ':' && p[len] != '\0')
+	len++;
+
+      /* If we reach the end of the string before getting a valid name-value
+	 pair, bail out.  */
+      if (p[len] == '\0')
+	return;
+
+      /* We did not find a valid name-value pair before encountering the
+	 colon.  */
+      if (p[len]== ':')
+	{
+	  p += len + 1;
+	  continue;
+	}
+
+      p += len + 1;
+
+      char *value = p;
+      len = 0;
+
+      while (p[len] != ':' && p[len] != '\0')
+	len++;
+
+      char end = p[len];
+      p[len] = '\0';
+
+      /* Add the tunable if it exists.  */
+      for (size_t i = 0; i < sizeof (tunable_list) / sizeof (tunable_t); i++)
+	{
+	  if (is_name (tunable_list[i].name, name))
+	    {
+	      /* Tunable values take precedence over the env_alias.  */
+	      tunable_list[i].strval = value;
+	      tunable_initialize (&tunable_list[i]);
+	      break;
+	    }
+	}
+
+      if (end == ':')
+	p += len + 1;
+      else
+	return;
+    }
+}
+
 /* 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.  */
@@ -116,6 +179,12 @@ __tunables_init (char **envp)
 
   while (get_next_env (&envp, &envname, &len, &envval))
     {
+      if (is_name (GLIBC_TUNABLES, envname))
+	{
+	  parse_tunables (strdup (envval));
+	  continue;
+	}
+
       for (int i = 0; i < sizeof (tunable_list) / sizeof (tunable_t); i++)
 	{
 	  tunable_t *cur = &tunable_list[i];
-- 
2.5.5

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable
  2016-01-13  8:38         ` Andreas Schwab
@ 2016-01-13 15:37           ` Carlos O'Donell
  0 siblings, 0 replies; 29+ messages in thread
From: Carlos O'Donell @ 2016-01-13 15:37 UTC (permalink / raw)
  To: Andreas Schwab, Siddhesh Poyarekar
  Cc: libc-alpha, roland, Paul E. Murphy, Andi Kleen

On 01/13/2016 03:38 AM, Andreas Schwab wrote:
> Siddhesh Poyarekar <sid@reserved-bit.com> writes:
> 
>> A single load (and constructor call) does not matter since there's
>> nothing to race against.  The case of concurrent constructor calls I
>> mentioned in the following paragraph; the load lock is sufficient to
>> protect it.
> 
> PR19448 is about removing that lock.

If the code has comments that it needs protection from load lock then
such code will be reviewed as we fix BZ19448. Which is why I suggested
comments to that extent. The removal of the load lock won't be easy
and will require quite a bit of review.

Cheers,
Carlos.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable
  2016-01-13  3:27       ` Siddhesh Poyarekar
@ 2016-01-13  8:38         ` Andreas Schwab
  2016-01-13 15:37           ` Carlos O'Donell
  0 siblings, 1 reply; 29+ messages in thread
From: Andreas Schwab @ 2016-01-13  8:38 UTC (permalink / raw)
  To: Siddhesh Poyarekar
  Cc: Carlos O'Donell, libc-alpha, roland, Paul E. Murphy, Andi Kleen

Siddhesh Poyarekar <sid@reserved-bit.com> writes:

> A single load (and constructor call) does not matter since there's
> nothing to race against.  The case of concurrent constructor calls I
> mentioned in the following paragraph; the load lock is sufficient to
> protect it.

PR19448 is about removing that lock.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable
  2016-01-13  2:44     ` Carlos O'Donell
@ 2016-01-13  3:27       ` Siddhesh Poyarekar
  2016-01-13  8:38         ` Andreas Schwab
  0 siblings, 1 reply; 29+ messages in thread
From: Siddhesh Poyarekar @ 2016-01-13  3:27 UTC (permalink / raw)
  To: Carlos O'Donell
  Cc: Andreas Schwab, libc-alpha, roland, Paul E. Murphy, Andi Kleen

On Tue, Jan 12, 2016 at 09:44:51PM -0500, Carlos O'Donell wrote:
> Not true if you link statically and dlopen, at which point you could
> have multiple threads, and you call dlopen which loads 
> libc.so/libpthread.so's constructors?

A single load (and constructor call) does not matter since there's
nothing to race against.  The case of concurrent constructor calls I
mentioned in the following paragraph; the load lock is sufficient to
protect it.

> > Even in case the constructors do get called in parallel in different
> > threads, they should get synchronized by the dynamic linker load lock,
> > so you'd never have concurrent calls to __tunables_init that race on
> > the value of initialized.
> 
> Please include a concurrency note there then, that this is protected
> by the load lock?

OK.

Thanks,
Siddhesh

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable
  2016-01-11 14:45   ` Siddhesh Poyarekar
@ 2016-01-13  2:44     ` Carlos O'Donell
  2016-01-13  3:27       ` Siddhesh Poyarekar
  0 siblings, 1 reply; 29+ messages in thread
From: Carlos O'Donell @ 2016-01-13  2:44 UTC (permalink / raw)
  To: Siddhesh Poyarekar, Andreas Schwab
  Cc: libc-alpha, roland, Paul E. Murphy, Andi Kleen

On 01/11/2016 09:45 AM, Siddhesh Poyarekar wrote:
> On Mon, Jan 11, 2016 at 02:51:36PM +0100, Andreas Schwab wrote:
>> Siddhesh Poyarekar <sid@reserved-bit.com> writes:
>>
>>>  void
>>>  __tunables_init (char **envp)
>>>  {
>>> -  /* Empty for now.  */
>>> +  static bool initialized = false;
>>> +
>>> +  if (__glibc_likely (initialized))
>>> +    return;
>>
>> Is this supposed to be thread-safe?
> 
> This is called only from the libc.so and libpthread.so constructors.
> So the first run will always happen exclusively in the main thread
> through either library constructor.

Not true if you link statically and dlopen, at which point you could
have multiple threads, and you call dlopen which loads 
libc.so/libpthread.so's constructors?

You can write a simple static test case that creates N threads and
calls dlopen on some DSO to test this.

> Even in case the constructors do get called in parallel in different
> threads, they should get synchronized by the dynamic linker load lock,
> so you'd never have concurrent calls to __tunables_init that race on
> the value of initialized.

Please include a concurrency note there then, that this is protected
by the load lock?

Cheers,
Carlos.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable
  2016-01-11 21:26 ` Paul E. Murphy
@ 2016-01-12 12:28   ` Siddhesh Poyarekar
  0 siblings, 0 replies; 29+ messages in thread
From: Siddhesh Poyarekar @ 2016-01-12 12:28 UTC (permalink / raw)
  To: Paul E. Murphy; +Cc: libc-alpha, roland, carlos, Andi Kleen

On Mon, Jan 11, 2016 at 03:26:43PM -0600, Paul E. Murphy wrote:
> > +#define GLIBC_TUNABLES "GLIBC_TUNABLES"
> 
> Should this be "GLIBC_TUNABLES=" to prevent matching a bogus prefix?
> 

Thanks for pointing out the problem.  I'm fixing it differently
though, by implementing the string comparison as strcmp instead, so
that it returns a mismatch if string lengths are different.  I chose
this approach because the other uses of t_strncmp also had the same
problem you pointed out.

Siddhesh

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable
  2016-01-11 11:17 Siddhesh Poyarekar
  2016-01-11 13:51 ` Andreas Schwab
@ 2016-01-11 21:26 ` Paul E. Murphy
  2016-01-12 12:28   ` Siddhesh Poyarekar
  1 sibling, 1 reply; 29+ messages in thread
From: Paul E. Murphy @ 2016-01-11 21:26 UTC (permalink / raw)
  To: Siddhesh Poyarekar, libc-alpha; +Cc: roland, carlos, Andi Kleen

[-- Attachment #1: Type: text/plain, Size: 1580 bytes --]


I ran into a few minor issues while testing this, noted below.

On 01/11/2016 05:17 AM, Siddhesh Poyarekar wrote:
> Read tunables values from the users using the GLIBC_TUNABLES
> environment variable.  The value of this variable is a colon-separated
> list of name=value pairs.  So a typical string would look like this:
> 
> GLIBC_TUNABLES=glibc.malloc.mmap_threshold=2048:glibc.malloc.trim_threshold=1024
> 
> 	* tunables/tunables.c: Include sys/mman.h and libc-internals.h.
> 	(GLIBC_TUNABLES): New macro.
> 	(t_strdup): New function.
> 	(__tunables_init): Implement initializer.
> ---
>  tunables/tunables.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 98 insertions(+), 5 deletions(-)
> 
> diff --git a/tunables/tunables.c b/tunables/tunables.c
> index 2ae1050..ddd1934 100644
> --- a/tunables/tunables.c
> +++ b/tunables/tunables.c
> @@ -24,12 +24,16 @@
>  #include <stdbool.h>
>  #include <unistd.h>
>  #include <sys/param.h>
> +#include <sys/mman.h>
> +#include <libc-internal.h>
> 
>  extern char **__environ;
> 
>  #define TUNABLES_INTERNAL 1
>  #include "tunables.h"
> 
> +#define GLIBC_TUNABLES "GLIBC_TUNABLES"

Should this be "GLIBC_TUNABLES=" to prevent matching a bogus prefix?


> +
> +  while (get_next_env (&evp, &envname, &envnamelen, &envval))
> +    {
> +      if (!t_strncmp (envname, GLIBC_TUNABLES, sizeof (GLIBC_TUNABLES)))

"sizeof (GLIBC_TUNABLES) - 1"?


With those changes, I was able to use the dynamic TLE patch. Attached is my
updated dynamic TLE patch for those interested. It did not require much
change.

Paul

[-- Attachment #2: 0001-Add-elision-tunables.patch --]
[-- Type: text/x-patch, Size: 12007 bytes --]

From 51967c99dbcea3e13bface4c6f13c1cfa3cb5709 Mon Sep 17 00:00:00 2001
From: "Paul E. Murphy" <murphyp@linux.vnet.ibm.com>
Date: Wed, 23 Sep 2015 15:16:41 -0500
Subject: [PATCH] Add elision tunables

This patch adds several new tunables to control the behavior of
elision on supported platforms.  This also disables elision
by default on powerpc.

The tunable names are slightly munged to remove architecture
specific terms.  Otherwise, the elision implementation is
roughly identical on all supported platforms.

2016-01-11  Paul E. Murphy  <murphyp@linux.vnet.ibm.com>

	* nptl/elision-tunables.c: New file.
	* sysdeps/unix/sysv/linux/powerpc/elision-conf.c:
	(ELISION_ENABLE): New macro.
	(ELISION_SKIP_LOCK_BUSY): Likewise.
	(ELISION_SKIP_LOCK_INTERNAL_ABORT): Likewise.
	(ELISION_SKIP_LOCK_AFTER_RETRIES): Likewise.
	(ELISION_TRIES): Likewise.
	(ELISION_TRYLOCK_INTERNAL_ABORT): Likewise.
	(ELISION_CAN_ENABLE): Likewise.
	(elision_init): Add hook to tunable function.
	* sysdeps/unix/sysv/linux/s390/elision-conf.c:
	Likewise.
	* sysdeps/unix/sysv/linux/x86/elision-conf.c:
	(ELISION_ENABLE): New macro.
	(ELISION_SKIP_LOCK_BUSY): Likewise.
	(ELISION_SKIP_LOCK_INTERNAL_ABORT): Likewise.
	(ELISION_TRIES): Likewise.
	(ELISION_TRYLOCK_INTERNAL_ABORT): Likewise.
	(ELISION_CAN_ENABLE): Likewise.
	(elision_init): Add hook to tunable function.
	* tunables/tunable-list.h (tunable_register): Regenerate.
	* tunables/tunable.list: Add elision parameters.
---
 nptl/elision-tunables.c                        | 82 ++++++++++++++++++++++++++
 sysdeps/unix/sysv/linux/powerpc/elision-conf.c | 18 +++++-
 sysdeps/unix/sysv/linux/s390/elision-conf.c    | 19 +++++-
 sysdeps/unix/sysv/linux/x86/elision-conf.c     | 12 ++++
 tunables/tunable-list.h                        | 30 +++++++---
 tunables/tunables.list                         |  8 +++
 6 files changed, 157 insertions(+), 12 deletions(-)
 create mode 100644 nptl/elision-tunables.c

diff --git a/nptl/elision-tunables.c b/nptl/elision-tunables.c
new file mode 100644
index 0000000..efaabdd
--- /dev/null
+++ b/nptl/elision-tunables.c
@@ -0,0 +1,82 @@
+/* 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/>.  */
+
+#if BUILD_TUNABLES
+
+#define TUNABLE_NAMESPACE pthread
+#include <tunables/tunables.h>
+
+/* Define a function to parse an integral tunable value and
+   set the pointer this value.  */
+#define _ELISION_TUNABLE_INT_FUNC(tunable, ptr) \
+  void \
+  _elision_set_ ## tunable (const char * value) \
+  { \
+    *ptr = atoi (value); \
+  }
+
+/* Define some helper macros to register a tunable, and
+   define the setter function.  Note, this defines a
+   nested function, and the tunable macro likely resolves
+   to a function call.  */
+#define _ELISION_TUNABLE_REGISTER(name, mname) \
+  _ELISION_TUNABLE_INT_FUNC (name, mname); \
+  TUNABLE_REGISTER (pthread, elision_ ## name, \
+  			     &_elision_set_ ## name)
+
+/* Initialize the tunables based on what the platform has defined as
+   being available.  */
+static void __attribute__((unused))
+elision_init_tunables (char **envp)
+{
+  /* Don't attempt to do anything if elision isn't supported */
+  if (!ELISION_CAN_ENABLE || __libc_enable_secure)
+    return;
+
+  _ELISION_TUNABLE_REGISTER (enable, ELISION_ENABLE);
+
+#ifdef ELISION_SKIP_LOCK_BUSY
+  _ELISION_TUNABLE_REGISTER (skip_lock_busy, ELISION_SKIP_LOCK_BUSY);
+#endif
+
+#ifdef ELISION_SKIP_LOCK_INTERNAL_ABORT
+  _ELISION_TUNABLE_REGISTER (skip_lock_internal_abort,
+  			     ELISION_SKIP_LOCK_INTERNAL_ABORT);
+#endif
+
+#ifdef ELISION_SKIP_LOCK_AFTER_RETRIES
+  _ELISION_TUNABLE_REGISTER (skip_lock_after_retries,
+  			     ELISION_SKIP_LOCK_AFTER_RETRIES);
+#endif
+
+#ifdef ELISION_TRIES
+  _ELISION_TUNABLE_REGISTER (tries, ELISION_TRIES);
+#endif
+
+#ifdef ELISION_SKIP_TRYLOCK_INTERNAL_ABORT
+  _ELISION_TUNABLE_REGISTER (skip_trylock_internal_abort,
+  			     ELISION_SKIP_TRYLOCK_INTERNAL_ABORT);
+#endif
+
+  tunables_init (envp);
+}
+
+#else
+
+#define elision_init_tunables(x)
+
+#endif /* BUILD_TUNABLES */
diff --git a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
index 21c3afd..b5249c3 100644
--- a/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
+++ b/sysdeps/unix/sysv/linux/powerpc/elision-conf.c
@@ -22,6 +22,19 @@
 #include <unistd.h>
 #include <dl-procinfo.h>
 
+#define ELISION_ENABLE (&__pthread_force_elision)
+#define ELISION_SKIP_LOCK_BUSY (&__elision_aconf.skip_lock_busy)
+#define ELISION_SKIP_LOCK_INTERNAL_ABORT \
+  (&__elision_aconf.skip_lock_internal_abort)
+#define ELISION_SKIP_LOCK_AFTER_RETRIES \
+  (&__elision_aconf.skip_lock_out_of_tbegin_retries)
+#define ELISION_TRIES (&__elision_aconf.try_tbegin)
+#define ELISION_SKIP_TRYLOCK_INTERNAL_ABORT \
+  (&__elision_aconf.skip_trylock_internal_abort)
+#define ELISION_CAN_ENABLE ((GLRO (dl_hwcap2) & PPC_FEATURE2_HAS_HTM))
+
+#include "elision-tunables.c"
+
 /* Reasonable initial tuning values, may be revised in the future.
    This is a conservative initial value.  */
 
@@ -60,8 +73,9 @@ elision_init (int argc __attribute__ ((unused)),
 	      char **environ)
 {
 #ifdef ENABLE_LOCK_ELISION
-  int elision_available = (GLRO (dl_hwcap2) & PPC_FEATURE2_HAS_HTM) ? 1 : 0;
-  __pthread_force_elision = __libc_enable_secure ? 0 : elision_available;
+  /* Note, elision must be explicitly turned on by setting the
+     appropriate tunable on a supported platform.  */
+  elision_init_tunables (environ);
 #endif
   if (!__pthread_force_elision)
     /* Disable elision on rwlocks.  */
diff --git a/sysdeps/unix/sysv/linux/s390/elision-conf.c b/sysdeps/unix/sysv/linux/s390/elision-conf.c
index 4441fd9..aa2b2de 100644
--- a/sysdeps/unix/sysv/linux/s390/elision-conf.c
+++ b/sysdeps/unix/sysv/linux/s390/elision-conf.c
@@ -22,6 +22,19 @@
 #include <unistd.h>
 #include <dl-procinfo.h>
 
+#define ELISION_ENABLE (&__pthread_force_elision)
+#define ELISION_SKIP_LOCK_BUSY (&__elision_aconf.skip_lock_busy)
+#define ELISION_SKIP_LOCK_INTERNAL_ABORT \
+  (&__elision_aconf.skip_lock_internal_abort)
+#define ELISION_SKIP_LOCK_AFTER_RETRIES \
+  (&__elision_aconf.skip_lock_out_of_tbegin_retries)
+#define ELISION_TRIES (&__elision_aconf.try_tbegin)
+#define ELISION_SKIP_TRYLOCK_INTERNAL_ABORT \
+  (&__elision_aconf.skip_trylock_internal_abort)
+#define ELISION_CAN_ENABLE (GLRO (dl_hwcap) & HWCAP_S390_TE)
+
+#include "elision-tunables.c"
+
 /* Reasonable initial tuning values, may be revised in the future.
    This is a conservative initial value.  */
 
@@ -62,9 +75,13 @@ elision_init (int argc __attribute__ ((unused)),
 {
   /* Set when the CPU and the kernel supports transactional execution.
      When false elision is never attempted.  */
-  int elision_available = (GLRO (dl_hwcap) & HWCAP_S390_TE) ? 1 : 0;
+  int elision_available = ELISION_SHOULD_ENABLE ? 1 : 0;
 
+  /* Enable elision, if available, by default.  */
   __pthread_force_elision = __libc_enable_secure ? 0 : elision_available;
+
+  /* Update any tunables as desired.  */
+  elision_init_tunables (environ);
 }
 
 #ifdef SHARED
diff --git a/sysdeps/unix/sysv/linux/x86/elision-conf.c b/sysdeps/unix/sysv/linux/x86/elision-conf.c
index 0d98133..b307db8 100644
--- a/sysdeps/unix/sysv/linux/x86/elision-conf.c
+++ b/sysdeps/unix/sysv/linux/x86/elision-conf.c
@@ -22,6 +22,17 @@
 #include <elision-conf.h>
 #include <unistd.h>
 
+#define ELISION_ENABLE (&__pthread_force_elision)
+#define ELISION_SKIP_LOCK_BUSY (&__elision_aconf.skip_lock_busy)
+#define ELISION_SKIP_LOCK_INTERNAL_ABORT \
+  (&__elision_aconf.skip_lock_internal_abort)
+#define ELISION_TRIES (&__elision_aconf.retry_try_xbegin)
+#define ELISION_SKIP_TRYLOCK_INTERNAL_ABORT \
+  (&__elision_aconf.skip_trylock_internal_abort)
+#define ELISION_CAN_ENABLE (HAS_CPU_FEATURE (RTM))
+
+#include "elision-tunables.c"
+
 /* Reasonable initial tuning values, may be revised in the future.
    This is a conservative initial value.  */
 
@@ -65,6 +76,7 @@ elision_init (int argc __attribute__ ((unused)),
   __elision_available = HAS_CPU_FEATURE (RTM);
 #ifdef ENABLE_LOCK_ELISION
   __pthread_force_elision = __libc_enable_secure ? 0 : __elision_available;
+  elision_init_tunables (environ);
 #endif
   if (!HAS_CPU_FEATURE (RTM))
     __elision_aconf.retry_try_xbegin = 0; /* Disable elision on rwlocks */
diff --git a/tunables/tunable-list.h b/tunables/tunable-list.h
index 2b1caa4..08b07a6 100644
--- a/tunables/tunable-list.h
+++ b/tunables/tunable-list.h
@@ -8,25 +8,37 @@
 
 typedef enum
 {
-  TUNABLE_ENUM_NAME(glibc, malloc, check),
-  TUNABLE_ENUM_NAME(glibc, malloc, top_pad),
-  TUNABLE_ENUM_NAME(glibc, malloc, perturb),
-  TUNABLE_ENUM_NAME(glibc, malloc, mmap_threshold),
+  TUNABLE_ENUM_NAME(glibc, pthread, elision_tries),
+  TUNABLE_ENUM_NAME(glibc, pthread, elision_skip_trylock_internal_abort),
+  TUNABLE_ENUM_NAME(glibc, pthread, elision_enable),
+  TUNABLE_ENUM_NAME(glibc, pthread, elision_skip_lock_busy),
+  TUNABLE_ENUM_NAME(glibc, pthread, elision_skip_lock_internal_abort),
+  TUNABLE_ENUM_NAME(glibc, pthread, elision_skip_lock_after_retries),
   TUNABLE_ENUM_NAME(glibc, malloc, trim_threshold),
   TUNABLE_ENUM_NAME(glibc, malloc, mmap_max),
   TUNABLE_ENUM_NAME(glibc, malloc, arena_max),
   TUNABLE_ENUM_NAME(glibc, malloc, arena_test),
+  TUNABLE_ENUM_NAME(glibc, malloc, check),
+  TUNABLE_ENUM_NAME(glibc, malloc, top_pad),
+  TUNABLE_ENUM_NAME(glibc, malloc, perturb),
+  TUNABLE_ENUM_NAME(glibc, malloc, mmap_threshold),
 } tunable_id_t;
 
 #ifdef TUNABLES_INTERNAL
 static tunable_t tunable_list[] = {
-  {TUNABLE_NAME_S(glibc, malloc, check), NULL, NULL, false},
-  {TUNABLE_NAME_S(glibc, malloc, top_pad), NULL, NULL, false},
-  {TUNABLE_NAME_S(glibc, malloc, perturb), NULL, NULL, false},
-  {TUNABLE_NAME_S(glibc, malloc, mmap_threshold), NULL, NULL, false},
+  {TUNABLE_NAME_S(glibc, pthread, elision_tries), NULL, NULL, false},
+  {TUNABLE_NAME_S(glibc, pthread, elision_skip_trylock_internal_abort), NULL, NULL, false},
+  {TUNABLE_NAME_S(glibc, pthread, elision_enable), NULL, NULL, false},
+  {TUNABLE_NAME_S(glibc, pthread, elision_skip_lock_busy), NULL, NULL, false},
+  {TUNABLE_NAME_S(glibc, pthread, elision_skip_lock_internal_abort), NULL, NULL, false},
+  {TUNABLE_NAME_S(glibc, pthread, elision_skip_lock_after_retries), NULL, NULL, false},
   {TUNABLE_NAME_S(glibc, malloc, trim_threshold), NULL, NULL, false},
   {TUNABLE_NAME_S(glibc, malloc, mmap_max), NULL, NULL, false},
   {TUNABLE_NAME_S(glibc, malloc, arena_max), NULL, NULL, false},
   {TUNABLE_NAME_S(glibc, malloc, arena_test), NULL, NULL, false},
+  {TUNABLE_NAME_S(glibc, malloc, check), NULL, NULL, false},
+  {TUNABLE_NAME_S(glibc, malloc, top_pad), NULL, NULL, false},
+  {TUNABLE_NAME_S(glibc, malloc, perturb), NULL, NULL, false},
+  {TUNABLE_NAME_S(glibc, malloc, mmap_threshold), NULL, NULL, false},
 };
-#endif
+#endif
\ No newline at end of file
diff --git a/tunables/tunables.list b/tunables/tunables.list
index f1335cc..d7a677b 100644
--- a/tunables/tunables.list
+++ b/tunables/tunables.list
@@ -9,4 +9,12 @@ glibc {
     arena_max
     arena_test
   }
+  pthread {
+    elision_enable
+    elision_skip_lock_busy
+    elision_skip_lock_internal_abort
+    elision_skip_lock_after_retries
+    elision_tries
+    elision_skip_trylock_internal_abort
+  }
 }
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable
  2016-01-11 13:51 ` Andreas Schwab
@ 2016-01-11 14:45   ` Siddhesh Poyarekar
  2016-01-13  2:44     ` Carlos O'Donell
  0 siblings, 1 reply; 29+ messages in thread
From: Siddhesh Poyarekar @ 2016-01-11 14:45 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: libc-alpha, roland, carlos, Paul E. Murphy, Andi Kleen

On Mon, Jan 11, 2016 at 02:51:36PM +0100, Andreas Schwab wrote:
> Siddhesh Poyarekar <sid@reserved-bit.com> writes:
> 
> >  void
> >  __tunables_init (char **envp)
> >  {
> > -  /* Empty for now.  */
> > +  static bool initialized = false;
> > +
> > +  if (__glibc_likely (initialized))
> > +    return;
> 
> Is this supposed to be thread-safe?

This is called only from the libc.so and libpthread.so constructors.
So the first run will always happen exclusively in the main thread
through either library constructor.

Even in case the constructors do get called in parallel in different
threads, they should get synchronized by the dynamic linker load lock,
so you'd never have concurrent calls to __tunables_init that race on
the value of initialized.

Siddhesh

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable
  2016-01-11 11:17 Siddhesh Poyarekar
@ 2016-01-11 13:51 ` Andreas Schwab
  2016-01-11 14:45   ` Siddhesh Poyarekar
  2016-01-11 21:26 ` Paul E. Murphy
  1 sibling, 1 reply; 29+ messages in thread
From: Andreas Schwab @ 2016-01-11 13:51 UTC (permalink / raw)
  To: Siddhesh Poyarekar; +Cc: libc-alpha, roland, carlos, Paul E. Murphy, Andi Kleen

Siddhesh Poyarekar <sid@reserved-bit.com> writes:

>  void
>  __tunables_init (char **envp)
>  {
> -  /* Empty for now.  */
> +  static bool initialized = false;
> +
> +  if (__glibc_likely (initialized))
> +    return;

Is this supposed to be thread-safe?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable
@ 2016-01-11 11:17 Siddhesh Poyarekar
  2016-01-11 13:51 ` Andreas Schwab
  2016-01-11 21:26 ` Paul E. Murphy
  0 siblings, 2 replies; 29+ messages in thread
From: Siddhesh Poyarekar @ 2016-01-11 11:17 UTC (permalink / raw)
  To: libc-alpha; +Cc: roland, carlos, Paul E. Murphy, Andi Kleen

Read tunables values from the users using the GLIBC_TUNABLES
environment variable.  The value of this variable is a colon-separated
list of name=value pairs.  So a typical string would look like this:

GLIBC_TUNABLES=glibc.malloc.mmap_threshold=2048:glibc.malloc.trim_threshold=1024

	* tunables/tunables.c: Include sys/mman.h and libc-internals.h.
	(GLIBC_TUNABLES): New macro.
	(t_strdup): New function.
	(__tunables_init): Implement initializer.
---
 tunables/tunables.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 98 insertions(+), 5 deletions(-)

diff --git a/tunables/tunables.c b/tunables/tunables.c
index 2ae1050..ddd1934 100644
--- a/tunables/tunables.c
+++ b/tunables/tunables.c
@@ -24,12 +24,16 @@
 #include <stdbool.h>
 #include <unistd.h>
 #include <sys/param.h>
+#include <sys/mman.h>
+#include <libc-internal.h>
 
 extern char **__environ;
 
 #define TUNABLES_INTERNAL 1
 #include "tunables.h"
 
+#define GLIBC_TUNABLES "GLIBC_TUNABLES"
+
 static int
 t_strncmp (const char *a, const char *b, size_t len)
 {
@@ -46,6 +50,31 @@ t_strncmp (const char *a, const char *b, size_t len)
   return 0;
 }
 
+static char *
+t_strdup (const char *in)
+{
+  size_t len = 0;
+
+  while (in[len] != '\0')
+    len++;
+
+  /* Allocate enough number of pages.  Given the number of tunables this should
+     not exceed a single page but we err on the conservative side and try to
+     allocate space as needed.  */
+  size_t alloclen = ALIGN_UP (len + 1, __getpagesize ());
+
+  char *out = __mmap (NULL, alloclen, PROT_READ | PROT_WRITE,
+		      MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
+
+  if (__glibc_unlikely (out == MAP_FAILED))
+    return NULL;
+  else
+    {
+      memcpy (out, in, len);
+      return out;
+    }
+}
+
 static bool
 get_next_env (char ***envp, char **name, size_t *namelen, char **val)
 {
@@ -74,14 +103,78 @@ get_next_env (char ***envp, char **name, size_t *namelen, char **val)
   return false;
 }
 
-/* This is where tunables will be read in from either an environment variable,
-   a set of environment variables or some other source and then initialized.
-   Caller should pass it the environment variable; __environ may not be
-   reliable if it is called earlier than libc.so initialization.  */
+/* Initialize tunables from the GLIBC_TUNABLES environment variable.  The
+   variable is set as colon separated name=value pairs.  */
 void
 __tunables_init (char **envp)
 {
-  /* Empty for now.  */
+  static bool initialized = false;
+
+  if (__glibc_likely (initialized))
+    return;
+
+  char **evp = envp;
+  char *p = NULL;
+
+  char *envname;
+  size_t envnamelen;
+  char *envval;
+
+  while (get_next_env (&evp, &envname, &envnamelen, &envval))
+    {
+      if (!t_strncmp (envname, GLIBC_TUNABLES, sizeof (GLIBC_TUNABLES)))
+	{
+	  p = t_strdup (envval);
+	  break;
+	}
+    }
+
+  if (p == NULL || *p == '\0')
+    goto out;
+
+  while (true)
+    {
+      char *name = p;
+      size_t len = 0;
+
+      /* First, find where the name ends.  */
+      while (p[len] != '=' && p[len] != '\0')
+	len++;
+
+      /* If we reach the end of the string before getting a valid name-value
+	 pair, bail out.  */
+      if (p[len] == '\0')
+	goto out;
+
+      p[len] = '\0';
+      p += len + 1;
+
+      char *value = p;
+      len = 0;
+
+      while (p[len] != ':' && p[len] != '\0')
+	len++;
+
+      char end = p[len];
+      p[len] = '\0';
+
+      /* Add the tunable if it exists.  */
+      for (size_t i = 0; i < sizeof (tunable_list) / sizeof (tunable_t); i++)
+	{
+	  if (t_strncmp (name, tunable_list[i].name, SIZE_MAX) == 0)
+	    {
+	      tunable_list[i].val = value;
+	      break;
+	    }
+	}
+
+      if (end == ':')
+	p += len + 1;
+      else
+	goto out;
+    }
+out:
+  initialized = true;
 }
 strong_alias (__tunables_init, tunables_init)
 
-- 
2.5.0

^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, other threads:[~2016-08-15 20:05 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-02 17:13 [PATCH 0/2] tunables for glibc Siddhesh Poyarekar
2016-07-02 17:13 ` [PATCH 1/2] Add framework for tunables Siddhesh Poyarekar
2016-07-03 14:30   ` H.J. Lu
2016-07-03 17:08     ` Siddhesh Poyarekar
2016-07-03 14:44   ` H.J. Lu
2016-07-03 17:14     ` Siddhesh Poyarekar
2016-07-03 17:41       ` H.J. Lu
2016-07-03 15:13   ` H.J. Lu
2016-07-03 17:43     ` H.J. Lu
2016-07-03 18:20       ` Siddhesh Poyarekar
2016-07-02 17:13 ` [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable Siddhesh Poyarekar
2016-07-03 14:39   ` H.J. Lu
2016-07-03 17:15     ` Siddhesh Poyarekar
2016-07-03 15:53   ` H.J. Lu
2016-07-03 17:18     ` Siddhesh Poyarekar
2016-07-03  0:24 ` [PATCH 0/2] tunables for glibc H.J. Lu
2016-07-03  3:08   ` Siddhesh Poyarekar
  -- strict thread matches above, loose matches on Subject: below --
2016-08-15 20:05 [PATCHv4 " Siddhesh Poyarekar
2016-08-15 20:05 ` [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable Siddhesh Poyarekar
2016-07-09 18:49 [PATCHv3 0/2] tunables for glibc Siddhesh Poyarekar
2016-07-09 18:49 ` [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable Siddhesh Poyarekar
2016-07-01 18:31 [PATCH 0/2] Tunables for glibc Siddhesh Poyarekar
2016-07-01 18:31 ` [PATCH 2/2] Initialize tunable list with the GLIBC_TUNABLES environment variable Siddhesh Poyarekar
2016-01-11 11:17 Siddhesh Poyarekar
2016-01-11 13:51 ` Andreas Schwab
2016-01-11 14:45   ` Siddhesh Poyarekar
2016-01-13  2:44     ` Carlos O'Donell
2016-01-13  3:27       ` Siddhesh Poyarekar
2016-01-13  8:38         ` Andreas Schwab
2016-01-13 15:37           ` Carlos O'Donell
2016-01-11 21:26 ` Paul E. Murphy
2016-01-12 12:28   ` Siddhesh Poyarekar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).