This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 1/2] Add framework for tunables


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

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

Should probably be TUNABLE_NAMESPACE or TUNABLE_ROOT.

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

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

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

I don't understand this comment.

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

Can you get rid of one indirection somehow?

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

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

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

You do not need __ prefixes on macro arguments.

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

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

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

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

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

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

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

Thanks,
Florian


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]