From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from quail.birch.relay.mailchannels.net (quail.birch.relay.mailchannels.net [23.83.209.151]) by sourceware.org (Postfix) with ESMTPS id 6BC013874C3E for ; Thu, 4 Feb 2021 18:02:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 6BC013874C3E X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id F14354821A4; Thu, 4 Feb 2021 18:02:45 +0000 (UTC) Received: from pdx1-sub0-mail-a34.g.dreamhost.com (100-96-16-7.trex.outbound.svc.cluster.local [100.96.16.7]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 32F26482DEC; Thu, 4 Feb 2021 18:02:45 +0000 (UTC) X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from pdx1-sub0-mail-a34.g.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384) by 100.96.16.7 (trex/6.0.2); Thu, 04 Feb 2021 18:02:45 +0000 X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|siddhesh@gotplt.org X-MailChannels-Auth-Id: dreamhost X-Interest-Reaction: 42c5fcf43aefdd3f_1612461765572_3881522624 X-MC-Loop-Signature: 1612461765571:3614105466 X-MC-Ingress-Time: 1612461765571 Received: from pdx1-sub0-mail-a34.g.dreamhost.com (localhost [127.0.0.1]) by pdx1-sub0-mail-a34.g.dreamhost.com (Postfix) with ESMTP id AA40B8B8DF; Thu, 4 Feb 2021 10:02:44 -0800 (PST) Received: from [192.168.86.152] (unknown [103.199.173.250]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: siddhesh@gotplt.org) by pdx1-sub0-mail-a34.g.dreamhost.com (Postfix) with ESMTPSA id DA17C7E0A8; Thu, 4 Feb 2021 10:02:40 -0800 (PST) Subject: Re: [PATCH 2/3] tunables: Remove C type arg from TUNABLE_SET* macros To: Adhemerval Zanella , libc-alpha@sourceware.org, Florian Weimer References: <20210203173406.2075480-1-siddhesh@sourceware.org> <20210203173406.2075480-3-siddhesh@sourceware.org> <0cfe3977-d42f-3a10-ec29-62105df235af@linaro.org> X-DH-BACKEND: pdx1-sub0-mail-a34 From: Siddhesh Poyarekar Message-ID: <267d379b-91b1-ce89-8319-30a6169744c9@sourceware.org> Date: Thu, 4 Feb 2021 23:32:29 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <0cfe3977-d42f-3a10-ec29-62105df235af@linaro.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-1171.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_NONE, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NEUTRAL, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 04 Feb 2021 18:02:49 -0000 On 2/4/21 10:54 PM, Adhemerval Zanella wrote: > > > On 03/02/2021 14:34, Siddhesh Poyarekar via Libc-alpha wrote: >> The C type argument is unnecessary because the cast that the macros do >> to that type is wrong; it should just cast to int64_t since that's how >> it gets dereferenced in do_tunable_update_val. > > Wouldn't be better to then use intmax_t instead of int64_t for the tunables, > now that if value has bounds the API requires you explicit set it? intmax_t is a good idea; I'll do it. The API doesn't always require bounds to be set; they need to be defined in dl-tunables.list or specified with TUNABLE_SET_WITH_BOUNDS otherwise bounds based on the underlying type are chosen in the generated dl-tunables-list.h. >> --- >> elf/dl-tunables.h | 29 +++++++++---------- >> manual/README.tunables | 16 +++++----- >> .../unix/sysv/linux/aarch64/cpu-features.c | 2 +- >> sysdeps/x86/dl-cacheinfo.h | 15 ++++------ >> 4 files changed, 27 insertions(+), 35 deletions(-) >> >> diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h >> index 971376ba8d..e0bd290e28 100644 >> --- a/elf/dl-tunables.h >> +++ b/elf/dl-tunables.h >> @@ -64,20 +64,18 @@ rtld_hidden_proto (__tunable_set_val) >> #if defined TOP_NAMESPACE && defined TUNABLE_NAMESPACE >> # define TUNABLE_GET(__id, __type, __cb) \ >> TUNABLE_GET_FULL (TOP_NAMESPACE, TUNABLE_NAMESPACE, __id, __type, __cb) >> -# define TUNABLE_SET(__id, __type, __val) \ >> - TUNABLE_SET_FULL (TOP_NAMESPACE, TUNABLE_NAMESPACE, __id, __type, __val) >> -# define TUNABLE_SET_WITH_BOUNDS(__id, __type, __val, __min, __max) \ >> +# define TUNABLE_SET(__id, __val) \ >> + TUNABLE_SET_FULL (TOP_NAMESPACE, TUNABLE_NAMESPACE, __id, __val) >> +# define TUNABLE_SET_WITH_BOUNDS(__id, __val, __min, __max) \ >> TUNABLE_SET_WITH_BOUNDS_FULL (TOP_NAMESPACE, TUNABLE_NAMESPACE, __id, \ >> - __type, __val, __min, __max) >> + __val, __min, __max) >> #else >> # define TUNABLE_GET(__top, __ns, __id, __type, __cb) \ >> TUNABLE_GET_FULL (__top, __ns, __id, __type, __cb) >> -# define TUNABLE_SET(__top, __ns, __id, __type, __val) \ >> - TUNABLE_SET_FULL (__top, __ns, __id, __type, __val) >> -# define TUNABLE_SET_WITH_BOUNDS(__top, __ns, __id, __type, __val, \ >> - __min, __max) \ >> - TUNABLE_SET_WITH_BOUNDS_FULL (__top, __ns, __id, __type, __val, \ >> - __min, __max) >> +# define TUNABLE_SET(__top, __ns, __id, __val) \ >> + TUNABLE_SET_FULL (__top, __ns, __id, __val) >> +# define TUNABLE_SET_WITH_BOUNDS(__top, __ns, __id, __val, __min, __max) \ >> + TUNABLE_SET_WITH_BOUNDS_FULL (__top, __ns, __id, __val, __min, __max) >> #endif >> >> /* Get and return a tunable value. If the tunable was set externally and __CB > > Ok. > >> @@ -91,19 +89,18 @@ rtld_hidden_proto (__tunable_set_val) >> }) >> >> /* Set a tunable value. */ >> -# define TUNABLE_SET_FULL(__top, __ns, __id, __type, __val) \ >> +# define TUNABLE_SET_FULL(__top, __ns, __id, __val) \ >> ({ \ >> __tunable_set_val (TUNABLE_ENUM_NAME (__top, __ns, __id), \ >> - & (__type) {__val}, NULL, NULL); \ >> + & (int64_t) {__val}, NULL, NULL); \ >> }) >> >> /* Set a tunable value together with min/max values. */ >> -# define TUNABLE_SET_WITH_BOUNDS_FULL(__top, __ns, __id, __type, __val, \ >> - __min, __max) \ >> +# define TUNABLE_SET_WITH_BOUNDS_FULL(__top, __ns, __id, __val, __min, __max) \ >> ({ \ >> __tunable_set_val (TUNABLE_ENUM_NAME (__top, __ns, __id), \ >> - & (__type) {__val}, & (__type) {__min}, \ >> - & (__type) {__max}); \ >> + & (int64_t) {__val}, & (int64_t) {__min}, \ >> + & (int64_t) {__max}); \ >> }) >> >> /* Namespace sanity for callback functions. Use this macro to keep the > > Ok. > >> diff --git a/manual/README.tunables b/manual/README.tunables >> index d8c768abcc..5f3e9d9403 100644 >> --- a/manual/README.tunables >> +++ b/manual/README.tunables >> @@ -98,17 +98,16 @@ where it can expect the tunable value to be passed in VALP. >> >> Tunables in the module can be updated using: >> >> - TUNABLE_SET (check, int32_t, val) >> + TUNABLE_SET (check, val) >> >> -where 'check' is the tunable name, 'int32_t' is the C type of the tunable and >> -'val' is a value of same type. >> +where 'check' is the tunable name, and 'val' is a value of same type. >> >> To get and set tunables in a different namespace from that module, use the full >> form of the macros as follows: >> >> val = TUNABLE_GET_FULL (glibc, cpu, hwcap_mask, uint64_t, NULL) >> >> - TUNABLE_SET_FULL (glibc, cpu, hwcap_mask, uint64_t, val) >> + TUNABLE_SET_FULL (glibc, cpu, hwcap_mask, val) >> >> where 'glibc' is the top namespace, 'cpu' is the tunable namespace and the >> remaining arguments are the same as the short form macros. >> @@ -116,18 +115,17 @@ remaining arguments are the same as the short form macros. >> The minimum and maximum values can updated together with the tunable value >> using: >> >> - TUNABLE_SET_WITH_BOUNDS (check, int32_t, val, min, max) >> + TUNABLE_SET_WITH_BOUNDS (check, val, min, max) >> >> -where 'check' is the tunable name, 'int32_t' is the C type of the tunable, >> -'val' is a value of same type, 'min' and 'max' are the minimum and maximum >> -values of the tunable. >> +where 'check' is the tunable name, 'val' is a value of same type, 'min' and >> +'max' are the minimum and maximum values of the tunable. >> >> To set the minimum and maximum values of tunables in a different namespace >> from that module, use the full form of the macros as follows: >> >> val = TUNABLE_GET_FULL (glibc, cpu, hwcap_mask, uint64_t, NULL) >> >> - TUNABLE_SET_WITH_BOUNDS_FULL (glibc, cpu, hwcap_mask, uint64_t, val, min, max) >> + TUNABLE_SET_WITH_BOUNDS_FULL (glibc, cpu, hwcap_mask, val, min, max) >> >> where 'glibc' is the top namespace, 'cpu' is the tunable namespace and the >> remaining arguments are the same as the short form macros. > > Ok. Something related to this patch that I noted is for TUNABLE_SET > without bounds is there is no range check. So, for instance, with > mmap_max if we set: > > GLIBC_TUNABLES=glibc.malloc.mmap_max=-1 program > > The value set will be '-4294967295' (since tunable val is a 64-bit) > which depending of the algorithm might not a valid value. Hmm, the value is always validated against the range. However, it's read as an unsigned value, which is why things must be going awry. > I think we should set the default range depending of the underlying > the 'type', so INT_32 will have an implicit range of [0, INT32_MAX]. > It will also simplify the code, since type.min/type.max will always > be set and valid. The range set for glibc.malloc.mmap_max in dl-tunables.list is also wrong if it needs to always be a non-negative value. > Another thing is there is no way to actually debug, so maybe also > add a LD_DEBUG=tunables. Ah yes, wasn't HJ working on it? If not, I'll work on it. Let me rework this one too; I need to rethink all of the type handling. Siddhesh