From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x230.google.com (mail-oi1-x230.google.com [IPv6:2607:f8b0:4864:20::230]) by sourceware.org (Postfix) with ESMTPS id 2F0913857831; Thu, 4 Feb 2021 19:43:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 2F0913857831 Received: by mail-oi1-x230.google.com with SMTP id w124so4948081oia.6; Thu, 04 Feb 2021 11:43:28 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=5J6h71KwLkCXsuE4KsFtCGc6UGMMbQUs1L2NqbvF82E=; b=ZZ+/1hbyccKqklWjg4oC6eSGr7bSEpYJEFdp85KGnse5tn4DtWsSzxs7tze2XMtPm0 GiOAJt96iad+QLxIazGn34vEPnowJ/EjTLYPAof7tG6pnJ5PVtZtGzoV46w8R1+SxcNw jVBzsNwHRAK+fIbyT1A/58ivVt6ZJwHWKxAS2JseQmweuQJtQ7lmAz8Zu86kBN/xLCpL P8P49Gi1VlK7r6fLtrGoNMahnZI+FmUWPjjC8LVnvHcSoe3E9g+4IUaIjsV2Rajco3NR G+m93l5pttmYaLfqncKIWf16BoAp7xaLjYwyXJg2zIrDCi/Y0woV5sL2fLPTNtMHwD5J ePWg== X-Gm-Message-State: AOAM533ERA+zsWwlHyZVW1bUH17oW7AU2GjWABM+4JYTOBj6QT14ezin je0+52uwwEjSoetDemKvNtNS1zRy0lxghrXhFrqikME8+P8= X-Google-Smtp-Source: ABdhPJxPMwXKJCnc3gQwCcMg/NsiYVeZO7ut2sQnbCywwUs+cZXKdt2TI4rIqiofbsczN4ZxnzxMlI1CdQYf5aNpxt0= X-Received: by 2002:aca:f503:: with SMTP id t3mr761865oih.79.1612467807553; Thu, 04 Feb 2021 11:43:27 -0800 (PST) MIME-Version: 1.0 References: <20210203173406.2075480-1-siddhesh@sourceware.org> <20210203173406.2075480-3-siddhesh@sourceware.org> <0cfe3977-d42f-3a10-ec29-62105df235af@linaro.org> <267d379b-91b1-ce89-8319-30a6169744c9@sourceware.org> <26862155-2ae2-4e1b-5f3d-92c39e23ffb0@linaro.org> In-Reply-To: <26862155-2ae2-4e1b-5f3d-92c39e23ffb0@linaro.org> From: "H.J. Lu" Date: Thu, 4 Feb 2021 11:42:51 -0800 Message-ID: Subject: Re: [PATCH 2/3] tunables: Remove C type arg from TUNABLE_SET* macros To: Adhemerval Zanella Cc: Siddhesh Poyarekar , GNU C Library , Florian Weimer Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-3036.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, 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 19:43:31 -0000 On Thu, Feb 4, 2021 at 11:37 AM Adhemerval Zanella wrote: > > > > On 04/02/2021 15:02, Siddhesh Poyarekar wrote: > > 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 d= o > >>> to that type is wrong; it should just cast to int64_t since that's ho= w > >>> it gets dereferenced in do_tunable_update_val. > >> > >> Wouldn't be better to then use intmax_t instead of int64_t for the tun= ables, > >> 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 bo= unds to be set; they need to be defined in dl-tunables.list or specified wi= th TUNABLE_SET_WITH_BOUNDS otherwise bounds based on the underlying type ar= e 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, __m= ax) > >>> #endif > >>> /* Get and return a tunable value. If the tunable was set extern= ally 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, __v= al, \ > >>> - __min, __max) \ > >>> +# define TUNABLE_SET_WITH_BOUNDS_FULL(__top, __ns, __id, __val, __mi= n, __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 ke= ep 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 passe= d 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 =3D TUNABLE_GET_FULL (glibc, cpu, hwcap_mask, uint64_t, NUL= L) > >>> - 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 namespac= e and the > >>> remaining arguments are the same as the short form macros. > >>> @@ -116,18 +115,17 @@ remaining arguments are the same as the short f= orm macros. > >>> The minimum and maximum values can updated together with the tunabl= e 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 m= aximum > >>> -values of the tunable. > >>> +where 'check' is the tunable name, 'val' is a value of same type, 'm= in' 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 =3D TUNABLE_GET_FULL (glibc, cpu, hwcap_mask, uint64_t, NUL= L) > >>> - 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, ma= x) > >>> where 'glibc' is the top namespace, 'cpu' is the tunable namespac= e 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=3Dglibc.malloc.mmap_max=3D-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 re= ad as an unsigned value, which is why things must be going awry. > > Yes, could you fix it on next iteration as well? > > > > >> 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 wro= ng if it needs to always be a non-negative value. > > > > Indeed, so maybe it should be change to UINT_32 type instead. > > >> Another thing is there is no way to actually debug, so maybe also > >> add a LD_DEBUG=3Dtunables. > > > > Ah yes, wasn't HJ working on it? If not, I'll work on it. > > I don't recall any work from him related to this. I opened: https://sourceware.org/bugzilla/show_bug.cgi?id=3D27069 It should be fixed. > > > > Let me rework this one too; I need to rethink all of the type handling. > > Thanks. > > > > > Siddhesh --=20 H.J.