| Message ID | 20160111111719.GA4183@devel.intra.reserved-bit.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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.
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
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.
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.
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)