From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x82a.google.com (mail-qt1-x82a.google.com [IPv6:2607:f8b0:4864:20::82a]) by sourceware.org (Postfix) with ESMTPS id 5BB21385780C for ; Thu, 4 Feb 2021 17:24:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 5BB21385780C Received: by mail-qt1-x82a.google.com with SMTP id c1so2977414qtc.1 for ; Thu, 04 Feb 2021 09:24:15 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:to:references:from:autocrypt:subject:message-id :date:user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=Kt82sOqvfso85puEnjllWG/7pfjnoLHEQKEW4zp1Nfw=; b=H8VuvH9/gJ0XPuWZyzYeQTDOXogBM7tiAWTGk4LX4oWE+Fp5dsnLeDDkEKAdgJ2m4a QHS140Ny8r1KjjYYqKhTvp4uAerV03YjXrXYvFnOf8sFNX6BF3pfgPm+d8AyWNUWjiAC URSFZlJGe1DBHsloIPFu+3fGFcJsXmRAR0ZOsGxGjMZzCQKxyBLjWh3RB6okJUq6Qxi8 qGtgkiXS+4eS6qpv45hpA8AV2SAws69S4HefAUdBHqn3jJYVxAZc6ezLYzSCklNjwuqj q1iEF1UQ1bExkepcSkcIowQ4WGuIapWUfBu+aDgKruDWM5bmTthaWRD5xkh9DFw1NZ3U iDZg== X-Gm-Message-State: AOAM533akD/2Pz/OZw4HK1o1Ji56dQ24xeI+mdeJgnDFLvqWpWbVik6q SAtFntFA7oWQeWATxR9a0KscSw== X-Google-Smtp-Source: ABdhPJxueuQTT2UJpVd9akPlYn6+JVxdIDvAK3h0hlUXNLrApcwrnKvdPOOQ+js5sAYy5bQ7USmQNg== X-Received: by 2002:ac8:5dc8:: with SMTP id e8mr549984qtx.249.1612459454775; Thu, 04 Feb 2021 09:24:14 -0800 (PST) Received: from [192.168.1.4] ([177.194.48.209]) by smtp.googlemail.com with ESMTPSA id d9sm5632263qko.84.2021.02.04.09.24.13 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 04 Feb 2021 09:24:14 -0800 (PST) To: libc-alpha@sourceware.org, Siddhesh Poyarekar , Florian Weimer References: <20210203173406.2075480-1-siddhesh@sourceware.org> <20210203173406.2075480-3-siddhesh@sourceware.org> From: Adhemerval Zanella Autocrypt: addr=adhemerval.zanella@linaro.org; prefer-encrypt=mutual; keydata= mQINBFcVGkoBEADiQU2x/cBBmAVf5C2d1xgz6zCnlCefbqaflUBw4hB/bEME40QsrVzWZ5Nq 8kxkEczZzAOKkkvv4pRVLlLn/zDtFXhlcvQRJ3yFMGqzBjofucOrmdYkOGo0uCaoJKPT186L NWp53SACXguFJpnw4ODI64ziInzXQs/rUJqrFoVIlrPDmNv/LUv1OVPKz20ETjgfpg8MNwG6 iMizMefCl+RbtXbIEZ3TE/IaDT/jcOirjv96lBKrc/pAL0h/O71Kwbbp43fimW80GhjiaN2y WGByepnkAVP7FyNarhdDpJhoDmUk9yfwNuIuESaCQtfd3vgKKuo6grcKZ8bHy7IXX1XJj2X/ BgRVhVgMHAnDPFIkXtP+SiarkUaLjGzCz7XkUn4XAGDskBNfbizFqYUQCaL2FdbW3DeZqNIa nSzKAZK7Dm9+0VVSRZXP89w71Y7JUV56xL/PlOE+YKKFdEw+gQjQi0e+DZILAtFjJLoCrkEX w4LluMhYX/X8XP6/C3xW0yOZhvHYyn72sV4yJ1uyc/qz3OY32CRy+bwPzAMAkhdwcORA3JPb kPTlimhQqVgvca8m+MQ/JFZ6D+K7QPyvEv7bQ7M+IzFmTkOCwCJ3xqOD6GjX3aphk8Sr0dq3 4Awlf5xFDAG8dn8Uuutb7naGBd/fEv6t8dfkNyzj6yvc4jpVxwARAQABtElBZGhlbWVydmFs IFphbmVsbGEgTmV0dG8gKExpbmFybyBWUE4gS2V5KSA8YWRoZW1lcnZhbC56YW5lbGxhQGxp bmFyby5vcmc+iQI3BBMBCAAhBQJXFRpKAhsDBQsJCAcDBRUKCQgLBRYCAwEAAh4BAheAAAoJ EKqx7BSnlIjv0e8P/1YOYoNkvJ+AJcNUaM5a2SA9oAKjSJ/M/EN4Id5Ow41ZJS4lUA0apSXW NjQg3VeVc2RiHab2LIB4MxdJhaWTuzfLkYnBeoy4u6njYcaoSwf3g9dSsvsl3mhtuzm6aXFH /Qsauav77enJh99tI4T+58rp0EuLhDsQbnBic/ukYNv7sQV8dy9KxA54yLnYUFqH6pfH8Lly sTVAMyi5Fg5O5/hVV+Z0Kpr+ZocC1YFJkTsNLAW5EIYSP9ftniqaVsim7MNmodv/zqK0IyDB GLLH1kjhvb5+6ySGlWbMTomt/or/uvMgulz0bRS+LUyOmlfXDdT+t38VPKBBVwFMarNuREU2 69M3a3jdTfScboDd2ck1u7l+QbaGoHZQ8ZNUrzgObltjohiIsazqkgYDQzXIMrD9H19E+8fw kCNUlXxjEgH/Kg8DlpoYJXSJCX0fjMWfXywL6ZXc2xyG/hbl5hvsLNmqDpLpc1CfKcA0BkK+ k8R57fr91mTCppSwwKJYO9T+8J+o4ho/CJnK/jBy1pWKMYJPvvrpdBCWq3MfzVpXYdahRKHI ypk8m4QlRlbOXWJ3TDd/SKNfSSrWgwRSg7XCjSlR7PNzNFXTULLB34sZhjrN6Q8NQZsZnMNs TX8nlGOVrKolnQPjKCLwCyu8PhllU8OwbSMKskcD1PSkG6h3r0AquQINBFcVGkoBEACgAdbR Ck+fsfOVwT8zowMiL3l9a2DP3Eeak23ifdZG+8Avb/SImpv0UMSbRfnw/N81IWwlbjkjbGTu oT37iZHLRwYUFmA8fZX0wNDNKQUUTjN6XalJmvhdz9l71H3WnE0wneEM5ahu5V1L1utUWTyh VUwzX1lwJeV3vyrNgI1kYOaeuNVvq7npNR6t6XxEpqPsNc6O77I12XELic2+36YibyqlTJIQ V1SZEbIy26AbC2zH9WqaKyGyQnr/IPbTJ2Lv0dM3RaXoVf+CeK7gB2B+w1hZummD21c1Laua +VIMPCUQ+EM8W9EtX+0iJXxI+wsztLT6vltQcm+5Q7tY+HFUucizJkAOAz98YFucwKefbkTp eKvCfCwiM1bGatZEFFKIlvJ2QNMQNiUrqJBlW9nZp/k7pbG3oStOjvawD9ZbP9e0fnlWJIsj 6c7pX354Yi7kxIk/6gREidHLLqEb/otuwt1aoMPg97iUgDV5mlNef77lWE8vxmlY0FBWIXuZ yv0XYxf1WF6dRizwFFbxvUZzIJp3spAao7jLsQj1DbD2s5+S1BW09A0mI/1DjB6EhNN+4bDB SJCOv/ReK3tFJXuj/HbyDrOdoMt8aIFbe7YFLEExHpSk+HgN05Lg5TyTro8oW7TSMTk+8a5M kzaH4UGXTTBDP/g5cfL3RFPl79ubXwARAQABiQIfBBgBCAAJBQJXFRpKAhsMAAoJEKqx7BSn lIjvI/8P/jg0jl4Tbvg3B5kT6PxJOXHYu9OoyaHLcay6Cd+ZrOd1VQQCbOcgLFbf4Yr+rE9l mYsY67AUgq2QKmVVbn9pjvGsEaz8UmfDnz5epUhDxC6yRRvY4hreMXZhPZ1pbMa6A0a/WOSt AgFj5V6Z4dXGTM/lNManr0HjXxbUYv2WfbNt3/07Db9T+GZkpUotC6iknsTA4rJi6u2ls0W9 1UIvW4o01vb4nZRCj4rni0g6eWoQCGoVDk/xFfy7ZliR5B+3Z3EWRJcQskip/QAHjbLa3pml xAZ484fVxgeESOoaeC9TiBIp0NfH8akWOI0HpBCiBD5xaCTvR7ujUWMvhsX2n881r/hNlR9g fcE6q00qHSPAEgGr1bnFv74/1vbKtjeXLCcRKk3Ulw0bY1OoDxWQr86T2fZGJ/HIZuVVBf3+ gaYJF92GXFynHnea14nFFuFgOni0Mi1zDxYH/8yGGBXvo14KWd8JOW0NJPaCDFJkdS5hu0VY 7vJwKcyHJGxsCLU+Et0mryX8qZwqibJIzu7kUJQdQDljbRPDFd/xmGUFCQiQAncSilYOcxNU EMVCXPAQTteqkvA+gNqSaK1NM9tY0eQ4iJpo+aoX8HAcn4sZzt2pfUB9vQMTBJ2d4+m/qO6+ cFTAceXmIoFsN8+gFN3i8Is3u12u8xGudcBPvpoy4OoG Subject: Re: [PATCH 2/3] tunables: Remove C type arg from TUNABLE_SET* macros Message-ID: <0cfe3977-d42f-3a10-ec29-62105df235af@linaro.org> Date: Thu, 4 Feb 2021 14:24:11 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20210203173406.2075480-3-siddhesh@sourceware.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-14.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, 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 17:24:17 -0000 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? > --- > 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. 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. Another thing is there is no way to actually debug, so maybe also add a LD_DEBUG=tunables. > diff --git a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c > index fe52b6308e..db6aa3516c 100644 > --- a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c > +++ b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c > @@ -104,7 +104,7 @@ init_cpu_features (struct cpu_features *cpu_features) > cpu_features->mte_state = (GLRO (dl_hwcap2) & HWCAP2_MTE) ? mte_state : 0; > /* If we lack the MTE feature, disable the tunable, since it will > otherwise cause instructions that won't run on this CPU to be used. */ > - TUNABLE_SET (glibc, mem, tagging, unsigned, cpu_features->mte_state); > + TUNABLE_SET (glibc, mem, tagging, cpu_features->mte_state); > # endif > > if (cpu_features->mte_state & 2) Ok. > diff --git a/sysdeps/x86/dl-cacheinfo.h b/sysdeps/x86/dl-cacheinfo.h > index a31fa0783a..e0a72568d8 100644 > --- a/sysdeps/x86/dl-cacheinfo.h > +++ b/sysdeps/x86/dl-cacheinfo.h > @@ -917,17 +917,14 @@ dl_init_cacheinfo (struct cpu_features *cpu_features) > rep_stosb_threshold = TUNABLE_GET (x86_rep_stosb_threshold, > long int, NULL); > > - TUNABLE_SET_WITH_BOUNDS (x86_data_cache_size, long int, data, > + TUNABLE_SET_WITH_BOUNDS (x86_data_cache_size, data, 0, (long int) -1); > + TUNABLE_SET_WITH_BOUNDS (x86_shared_cache_size, shared, 0, (long int) -1); > + TUNABLE_SET_WITH_BOUNDS (x86_non_temporal_threshold, non_temporal_threshold, > 0, (long int) -1); > - TUNABLE_SET_WITH_BOUNDS (x86_shared_cache_size, long int, shared, > - 0, (long int) -1); > - TUNABLE_SET_WITH_BOUNDS (x86_non_temporal_threshold, long int, > - non_temporal_threshold, 0, (long int) -1); > - TUNABLE_SET_WITH_BOUNDS (x86_rep_movsb_threshold, long int, > - rep_movsb_threshold, > + TUNABLE_SET_WITH_BOUNDS (x86_rep_movsb_threshold, rep_movsb_threshold, > minimum_rep_movsb_threshold, (long int) -1); > - TUNABLE_SET_WITH_BOUNDS (x86_rep_stosb_threshold, long int, > - rep_stosb_threshold, 1, (long int) -1); > + TUNABLE_SET_WITH_BOUNDS (x86_rep_stosb_threshold, rep_stosb_threshold, 1, > + (long int) -1); > #endif > > cpu_features->data_cache_size = data; > Ok.