Re: [PATCH 2/3] tunables: Remove C type arg from TUNABLE_SET* macros

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

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 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.

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 wrong 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=tunables.
> 
> 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.

> 
> Let me rework this one too; I need to rethink all of the type handling.

Thanks.

> 
> Siddhesh