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 Mon, Jul 11, 2016 at 06:05:09PM +0200, Florian Weimer wrote:
> 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.

OK.

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

It specifies if the env_alias should be read even for setuid binaries.
This is currently only useful for MALLOC_CHECK_ since it is read even
for setuid binaries.  Would env_alias_is_secure be clearer?

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

The comparison loop is bound by the name that is hardcoded into the
tunables_list structure, i.e. either tunable.name or
tunable.env_alias.

> > +static bool
> > +get_next_env (char ***envp, char **name, size_t *namelen, char **val)
> 
> Can you get rid of one indirection somehow?

I had picked this up verbatim from existing code.  I'll see what I can
do.

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

Ugh, yes, I'll fix it.

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

I did it out of habit to avoid namespace collisions, but it's not
necessary here so I'll remove it.

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

Thanks, I missed that.  I'll fix it.

> 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

I plan to do it via the tunables structure in the second pass, with
TUNABLE_SET_VAL_WITH_PTR that does the same thing as TUNABLE_SET_VAL
and then stores the pointer (&mp_.mmap_threshold) and callback in the
tunables structure to allow for a live update.

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

This is an interesting idea, but it will still have to live inside
ld.so if it needs to make IFUNC decisions.

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

OK.

> > +  print "static tunable_t tunable_list[] = {"
> 
> There's a const missing there.  Due to function pointer, this should really
> go into .relro.

OK.

Siddhesh


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