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 03:06:34PM -0400, Carlos O'Donell wrote:
> > +The list of allowed attributes are:
> > +
> > +- type:			Data type.  Defaults to STRING.  Allowed types are:
> > +			INT_32, SIZE_T.  Note that as of now the STRING type is
> > +			neither defined nor supported since no tunables need
> > +			it.  It will be in future though, when needed.
> 
> OK.
> 
> I expect we can add more types as time goes on.

Yes, and update this document when we do, which reminds me to update
the fact that STRING is supported.

> > +
> > +- minval:		Optional minimum acceptable value
> > +
> > +- maxval:		Optional maximum acceptable value
> > +
> 
> I assume valid only for numeric types and such values as are valid for the type?
> 
> Or does minval/maxval indicate minlength/maxlength for a string as an easy form
> of filtering?

It was only for numbers but I can overload that to mean string size
limits too.  As for string size checks, I'll add with the first string
tunable that actually needs it.

> > +- env_alias:		An alias environment variable
> > +
> > +- secure_env_alias:	Specify whether the environment variable should be read
> > +			for setuid binaries.
> 
> We should make a few things clear here:
> 
> * The tunable may still be ignored in AT_SECURE if the non-default value
>   fails to pass some level of runtime checking e.g. value could break the
>   application.
> 
> * The 'secure_env_alias' should really mean "Specify whether the environment
>   variable should be read for setuid binaries, 0 (default) means they are not
>   read and 1 means they are. Even if the variable is read it may be ignored."

OK, I'll extend the meaning of that for the tunable in general, not
just its alias.  That way, the meaning does not change between the
alias and the tunable.

> Are there any restrictions on the callback? I assume the foreign function (callback)
> is called from _any_ context so you have to be very very very careful about what
> functions you call?

The callback is currently only called from one context: the module
initialization.  In future it may get called later, i.e. when a
tunable is changed at runtime.  I prefer to think of the latter case
later if that is OK.

> Can we unit test the interface to make sure the API works as designed?
> 
> https://sourceware.org/ml/libc-alpha/2016-07/msg00110.html

Let me see if I can do that, but not as part of this patch.

> Can we pass back a void* to return success/failure like the
> pthreads API does? I can see a short term need to know if the
> foreign function call failed.

I can, but can you elaborate on why we would need that?  The call is
effectively going to happen in the same context in which the callback
is defined, i.e. we won't have a case where the callback is in one
module and the initialization is in another.

> > +#define RETURN_IF_INVALID_RANGE(__cur, __val) \
> > +({									      \
> > +  __typeof ((__cur)->type.min) min = (__cur)->type.min;			      \
> > +  __typeof ((__cur)->type.max) max = (__cur)->type.max;			      \
> > +  if (min != max && ((__val) < min || (__val) > max))			      \
> > +    return;								      \
> > +})
> 
> As mentioned above, might be nice to have min/max limit string min
> and max length.

I'll add string validation when we have an actual tunable that needs it.

> Add "May still be ignored if value is dangerous."

How about "May still be ignored if the value does not meet criteria
for allowed values for the tunable."?  That seems more precise since
we don't really have a way to identify dangerous values beyond the
validation we have in place.

> Two things are wrong.
> 
> Missing systemtap probes, _and_ wrong settings.

I skipped the probes on purpose thinking that it doesn't make sense to
hit them anyway, but I guess it is required since it changes
behaviour.  I'll add it.

> 
> For example setting top_pad should disable dynamic thresholding.

Yeah, I missed that.  I'll review all of the options and fix up.

> 
> Therefore you need to do the following:
> 
> (a) Take __libc_mallopt from malloc/malloc.c and split out the
>     setting of each variable into a "callback"
> 
> (b) Have __libc_mallopt call the callbacks.
> 
> (c) Have TUNABLE_SET_VAL call the callback via the tunables
>     mechanism.
> 
> That way you have code in one place for what has to happen
> when setting the variable.

That's a good idea.

Thanks,
Siddhesh


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