From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24047 invoked by alias); 14 Jul 2016 11:27:13 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 24025 invoked by uid 89); 14 Jul 2016 11:27:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_NEUTRAL autolearn=no version=3.3.2 spammy=H*F:U*siddhesh, HX-Envelope-From:sk:siddhes, criteria, careful X-HELO: homiemail-a52.g.dreamhost.com Date: Thu, 14 Jul 2016 11:27:00 -0000 From: Siddhesh Poyarekar To: Carlos O'Donell Cc: libc-alpha@sourceware.org Subject: Re: [PATCH 1/2] Add framework for tunables Message-ID: <20160714112654.GG22705@devel.intra.reserved-bit.com> References: <1468089478-10085-1-git-send-email-siddhesh@sourceware.org> <1468089478-10085-2-git-send-email-siddhesh@sourceware.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.1 (2016-04-27) X-SW-Source: 2016-07/txt/msg00425.txt.bz2 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