Message ID | 20210203173406.2075480-3-siddhesh@sourceware.org |
---|---|
State | New |
Headers | show |
Series | TUNABLE_SET fixes | expand |
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.
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
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
On Thu, Feb 4, 2021 at 11:37 AM Adhemerval Zanella <adhemerval.zanella@linaro.org> 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 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. I opened: https://sourceware.org/bugzilla/show_bug.cgi?id=27069 It should be fixed. > > > > Let me rework this one too; I need to rethink all of the type handling. > > Thanks. > > > > > Siddhesh
On 2/5/21 1:12 AM, H.J. Lu via Libc-alpha wrote: > I opened: > > https://sourceware.org/bugzilla/show_bug.cgi?id=27069 > > It should be fixed. There, I remember your name associated with this somehow :) I'll add it to my list of TODOs. Siddhesh
On 2/5/21 1:07 AM, Adhemerval Zanella via Libc-alpha wrote: >> 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? Yes, I will include a fix for this. Siddhesh
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 @@ -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 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. 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) 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;