This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
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