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/9] Add vectorized getenv for glibc use


On Thu, May 02, 2013 at 04:27:24PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> This adds a general "vectorized getenv" for glibc internal use.
> The motivation is to allow subsystems to access environment variables
> cheaply without having to rescan the environment completely.

Please see my review comments inline.  I like the essence of the
change; I intend to use it for my library tunables support work if
this patch makes it in.

We could add cache refresh support for some variables by making setenv
update this list whenever a GLIBC_* environ is updated.  This is not
necessary (or even advisable I guess) for the variables you intend to
implement, so that could be deferred as a separate change.

> The dynamic linker already walks the environment to look for its
> LD_* variables. Extend this code to look for a number of
> pre-registered GLIBC_* variables too. This can be done at basically
> no additional cost. The only two variables currently pre-registered
> are for the lock elision code.
> 
> For static builds which do not use the dynamic linker a similar
> environment walking function is called at init time.
> 
> The variable values are saved in a global array that can be directly
> accessed by libc and related libraries like libpthread.
> 
> 2013-05-02  Andi Kleen  <ak@linux.intel.com>
> 
> 	* .gitignore: Don't exclude glibc-var*
> 	* elf/Makefile (glibc-var.o): Add new file.
> 	* elf/Versions (_dl_glibc_var, __glibc_var_init): Export new symbols
> 	as GLIBC_PRIVATE.
> 	* elf/dl-environ.c (_dl_next_ld_env_entry): Look for GLIBC_ prefixes
> 	too. Return first character in new argument.
> 	* elf/glibc-var.c (struct glibc_var): Define global array.
> 	(__record_glibc_var): New function to save glibc variables.
> 	(next_env_entry): Function to walk environment.
> 	(__glibc_var_init): Fallback function for static builds.
> 	* elf/rtld.c: Update comment.
> 	(process_envvars): Handle GLIBC_ variables and call __record_glibc_var.
> 	* include/glibc-var.h: New file.
> 	* sysdeps/generic/ldsodefs.h (_dl_next_ld_env_entry): Update prototype.
> ---
>  .gitignore                 |    1 +
>  elf/Makefile               |    1 +
>  elf/Versions               |    2 +
>  elf/dl-environ.c           |   19 +++++++-
>  elf/glibc-var.c            |  107 ++++++++++++++++++++++++++++++++++++++++++++
>  elf/rtld.c                 |   17 ++++++-
>  include/glibc-var.h        |   48 ++++++++++++++++++++
>  sysdeps/generic/ldsodefs.h |    2 +-
>  8 files changed, 192 insertions(+), 5 deletions(-)
>  create mode 100644 elf/glibc-var.c
>  create mode 100644 include/glibc-var.h
> 
> diff --git a/.gitignore b/.gitignore
> index ee8b6dd..66b51cd 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -13,6 +13,7 @@ AUTHORS
>  copyr-*
>  copying.*
>  glibc-*
> +!glibc-var.*
>  
>  configparms
> 

If you rename glibc-var.[ch] to dl-glibc-var.[ch] then you won't need
this.  Additionally, it maintains the file naming convention in elf/.

> diff --git a/elf/Makefile b/elf/Makefile
> index c01ca9e..bce2a20 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -35,6 +35,7 @@ dl-routines	= $(addprefix dl-,load lookup object reloc deps hwcaps \
>  ifeq (yes,$(use-ldconfig))
>  dl-routines += dl-cache
>  endif
> +dl-routines += glibc-var
>  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 2383992..7190ccf 100644
> --- a/elf/Versions
> +++ b/elf/Versions
> @@ -61,5 +61,7 @@ ld {
>  
>      # Pointer protection.
>      __pointer_chk_guard;
> +    _dl_glibc_var;
> +    __glibc_var_init;

These should be sorted.

>    }
>  }
> diff --git a/elf/dl-environ.c b/elf/dl-environ.c
> index b971569..bdc6c49 100644
> --- a/elf/dl-environ.c
> +++ b/elf/dl-environ.c
> @@ -22,10 +22,10 @@
>  #include <ldsodefs.h>
>  
>  /* Walk through the environment of the process and return all entries
> -   starting with `LD_'.  */
> +   starting with `LD_' or 'GLIBC_'.  */
>  char *
>  internal_function
> -_dl_next_ld_env_entry (char ***position)
> +_dl_next_ld_env_entry (char ***position, char *first)
>  {
>    char **current = *position;
>    char *result = NULL;
> @@ -35,6 +35,7 @@ _dl_next_ld_env_entry (char ***position)
>        if (__builtin_expect ((*current)[0] == 'L', 0)
>  	  && (*current)[1] == 'D' && (*current)[2] == '_')
>  	{
> +	  *first = (*current)[0];
>  	  result = &(*current)[3];
>  
>  	  /* Save current position for next visit.  */
> @@ -43,6 +44,20 @@ _dl_next_ld_env_entry (char ***position)
>  	  break;
>  	}
>  
> +      if (__builtin_expect ((*current)[0] == 'G', 0)
> +	  && (*current)[1] == 'L' && (*current)[2] == 'I'
> +	  && (*current)[3] == 'B' && (*current)[4] == 'C'
> +	  && (*current)[5] == '_')
> +        {
> +	  *first = (*current)[0];
> +	  result = &(*current)[6];
> +
> +	  /* Save current position for next visit.  */
> +	  *position = ++current;
> +
> +	  break;
> +        }
> +
>        ++current;
>      }
>  
> diff --git a/elf/glibc-var.c b/elf/glibc-var.c
> new file mode 100644
> index 0000000..dc511f8
> --- /dev/null
> +++ b/elf/glibc-var.c
> @@ -0,0 +1,107 @@
> +/* Fast access to GLIBC_* environment variables, without having to walk
> +   the environment multiple times.
> +   Copyright (C) 2013 Free Software Foundation, Inc.
> +
> +   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 <string.h>
> +#include <glibc-var.h>
> +
> +struct glibc_var _dl_glibc_var[] = 
> +{
> +  [GLIBC_VAR_MUTEX]  = { "MUTEX",  5, NULL },
> +  [GLIBC_VAR_RWLOCK] = { "RWLOCK", 6, NULL },
> +  /* Add more GLIBC_ variables here */

A period and two spaces to end the comment.

> +  [GLIBC_VAR_MAX] =    { NULL, 0, NULL }
> +};
> +
> +internal_function void
> +__record_glibc_var (char *name, int len, char *val)
> +{
> +  int i;
> +
> +  for (i = 0; _dl_glibc_var[i].name; i++)

i < GLIBC_VAR_MAX may be a better termination check since it should
help unroll this loop.

> +    {
> +      struct glibc_var *v = &_dl_glibc_var[i];
> +
> +      if (len == v->len && !memcmp (v->name, name, v->len))
> +        {
> +	  v->val = val;
> +	  break;
> +	}
> +    }
> +  /* Ignore unknown GLIBC_ variables. */

Two spaces after the period.

> +}
> +
> +#ifndef SHARED
> +
> +/* If SHARED the env walk is shared with rtld.c */

Period and two spaces to end the comment.

> +
> +static char *
> +next_env_entry (char first, char ***position)
> +{
> +  char **current = *position;
> +  char *result = NULL;
> +
> +  while (*current != NULL)
> +    {
> +      if ((*current)[0] == first)
> +	{
> +	  result = *current;
> +	  *position = ++current;
> +	  break;
> +	}
> +
> +      ++current;
> +    }
> +
> +  return result;
> +}
> +
> +/* May be called from libpthread */

Period and two spaces to end the comment.

> +
> +void
> +__glibc_var_init (int argc __attribute__ ((unused)),
> +		  char **argv  __attribute__ ((unused)),
> +		  char **environ)
> +{
> +  char *envline;
> +  static int initialized;
> +
> +  if (initialized)
> +    return;
> +  initialized = 1;
> +
> +  while ((envline = next_env_entry ('G', &environ)) != NULL)
> +    {
> +      if (envline[1] == 'L' && envline[2] == 'I' && envline[3] == 'B'
> +	  && envline[4] == 'C' && envline[5] == '_')
> +	{
> +	  char *e = envline + 6;
> +	  while (*e && *e != '=')
> +	    e++;
> +	  if (*e == 0)
> +	    continue;
> +	  __record_glibc_var (envline + 6, e - (envline + 6), e + 1);
> +	}
> +    }
> +}
> +
> +void (*const __glibc_var_init_array []) (int, char **, char **)
> +  __attribute__ ((section (".preinit_array"), aligned (sizeof (void *)))) =
> +{
> +  &__glibc_var_init
> +};
> +#endif
> diff --git a/elf/rtld.c b/elf/rtld.c
> index 23238ad..b83387b 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -41,6 +41,7 @@
>  #include <tls.h>
>  #include <stap-probe.h>
>  #include <stackinfo.h>
> +#include <glibc-var.h>
>  
>  #include <assert.h>
>  
> @@ -2476,7 +2477,9 @@ process_dl_audit (char *str)
>  
>  /* Process all environments variables the dynamic linker must recognize.
>     Since all of them start with `LD_' we are a bit smarter while finding
> -   all the entries.  */
> +   all the entries.  In addition we also save a bunch of GLIBC_ variables
> +   used by other parts of glibc, so that each startup only has to walk the
> +   environment once. */

Two spaces after the period.

>  extern char **_environ attribute_hidden;
>  
>  
> @@ -2487,12 +2490,13 @@ process_envvars (enum mode *modep)
>    char *envline;
>    enum mode mode = normal;
>    char *debug_output = NULL;
> +  char first;
>  
>    /* This is the default place for profiling data file.  */
>    GLRO(dl_profile_output)
>      = &"/var/tmp\0/var/profile"[INTUSE(__libc_enable_secure) ? 9 : 0];
>  
> -  while ((envline = _dl_next_ld_env_entry (&runp)) != NULL)
> +  while ((envline = _dl_next_ld_env_entry (&runp, &first)) != NULL)
>      {
>        size_t len = 0;
>  
> @@ -2505,6 +2509,15 @@ process_envvars (enum mode *modep)
>  	   invalid memory below.  */
>  	continue;
>  
> +      /* Must be for GLIBC_ */
> +      if (first == 'G')
> + 	{
> +	  __record_glibc_var (envline, len, envline + len + 1);
> +	  continue;
> +	}
> +
> +      /* Must be for LD_ */
> +
>        switch (len)
>  	{
>  	case 4:
> diff --git a/include/glibc-var.h b/include/glibc-var.h
> new file mode 100644
> index 0000000..19c7c84
> --- /dev/null
> +++ b/include/glibc-var.h
> @@ -0,0 +1,48 @@
> +/* Fast access to GLIBC_* environment variables, without having to walk
> +   the environment. Register new ones in in elf/glibc-var.c
> +   Copyright (C) 2013 Free Software Foundation, Inc.
> +
> +   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 _GLIBC_VAR_H
> +#define _GLIBC_VAR_H 1
> +
> +#include <libc-symbols.h>
> +
> +enum
> +{
> + GLIBC_VAR_MUTEX,
> + GLIBC_VAR_RWLOCK,
> + GLIBC_VAR_MAX
> +};
> +

We might want to enforce namespaces from the beginning here.  How
about GLIBC_PTHREAD_MUTEX_TYPE or similar?  I don't want to bikeshed
on the actual name; I'm fine as long as a good namespace convention is
established.

It might also be a good idea to typedef the enum as glibc_env_t (or
similar) and enforce that on users of the function instead of
accepting any int.

> +struct glibc_var
> +{
> +  const char *name;
> +  int len; 
> +  char *val;
> +};
> +
> +extern struct glibc_var _dl_glibc_var[];
> +extern void __record_glibc_var (char *name, int len, char *val) internal_function;
> +
> +/* Call this if you're in a constructor that may run before glibc-var's */

Period and two spaces at the end of the comment.

> +#ifndef SHARED
> +extern void __glibc_var_init (int ac, char **av, char **env);
> +#else
> +/* For shared this is always done in the dynamic linker early enough. */
> +#define __glibc_var_init(a,b,c) do {} while(0)
> +#endif

Space before define and after while.

> +
> +#endif
> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index 41684f3..fc5c0f5 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -891,7 +891,7 @@ extern void _dl_show_auxv (void) internal_function;
>  
>  /* Return all environment variables starting with `LD_', one after the
>     other.  */
> -extern char *_dl_next_ld_env_entry (char ***position) internal_function;
> +extern char *_dl_next_ld_env_entry (char ***position, char *first) internal_function;

Fix comment to reflect new reality.

>  
>  /* Return an array with the names of the important hardware capabilities.  */
>  extern const struct r_strlenpair *_dl_important_hwcaps (const char *platform,
> -- 
> 1.7.7.6
> 


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