[PATCH 0/3] TUNABLE_SET fixes

[PATCH 0/3] TUNABLE_SET fixes

[Siddhesh Poyarekar]

This series fixes the setting of tunable values and ranges from within
glibc through the TUNABLE_SET* macros.  The current way will result in
overreads when reading 4 byte values since do_tunable_update_val always
reads an 8 byte value.  This also results in simplification of the
TUNABLE_SET* macros since they no longer need a type to be passed.

Siddhesh Poyarekar (3):
  Fix casts when setting tunable range
  tunables: Remove C type arg from TUNABLE_SET* macros
  x86: Use SIZE_MAX instead of (long int)-1 for tunable range value

 elf/dl-tunables.c                             | 10 +++----
 elf/dl-tunables.h                             | 29 +++++++++----------
 manual/README.tunables                        | 16 +++++-----
 .../unix/sysv/linux/aarch64/cpu-features.c    |  2 +-
 sysdeps/x86/dl-cacheinfo.h                    | 19 +++++-------
 5 files changed, 34 insertions(+), 42 deletions(-)

-- 
2.29.2

[PATCH 1/3] Fix casts when setting tunable range

[Siddhesh Poyarekar]

Cast tunable min and max to target type before comparison so that we
don't mix comparison of signed and unsigned types.
---
 elf/dl-tunables.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index b1a50b8469..488be4bbf1 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -107,6 +107,8 @@ get_next_env (char **envp, char **name, size_t *namelen, char **val,
 
 #define TUNABLE_SET_BOUNDS_IF_VALID(__cur, __minp, __maxp, __type)	      \
 ({									      \
+  __type curmin = (__cur)->type.min;				      \
+  __type curmax = (__cur)->type.max;				      \
   if (__minp != NULL)							      \
     {									      \
       /* MIN is specified.  */						      \
@@ -115,15 +117,13 @@ get_next_env (char **envp, char **name, size_t *namelen, char **val,
 	{								      \
 	   /* Both MIN and MAX are specified.  */			      \
 	    __type max = *((__type *) __maxp);				      \
-	  if (max >= min						      \
-	      && max <= (__cur)->type.max				      \
-	      && min >= (__cur)->type.min)				      \
+	  if (max >= min && max <= curmax && min >= curmin)		      \
 	    {								      \
 	      (__cur)->type.min = min;					      \
 	      (__cur)->type.max = max;					      \
 	    }								      \
 	}								      \
-      else if (min > (__cur)->type.min && min <= (__cur)->type.max)	      \
+      else if (min > curmin && min <= curmax)				      \
 	{								      \
 	  /* Only MIN is specified.  */					      \
 	  (__cur)->type.min = min;					      \
@@ -133,7 +133,7 @@ get_next_env (char **envp, char **name, size_t *namelen, char **val,
     {									      \
       /* Only MAX is specified.  */					      \
       __type max = *((__type *) __maxp);				      \
-      if (max < (__cur)->type.max && max >= (__cur)->type.min)		      \
+      if (max < curmax && max >= curmin)				      \
 	(__cur)->type.max = max;					      \
     }									      \
 })
-- 
2.29.2

Re: [PATCH 1/3] Fix casts when setting tunable range

[Adhemerval Zanella]

On 03/02/2021 14:34, Siddhesh Poyarekar via Libc-alpha wrote:
> Cast tunable min and max to target type before comparison so that we
> don't mix comparison of signed and unsigned types.
> ---
>  elf/dl-tunables.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index b1a50b8469..488be4bbf1 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -107,6 +107,8 @@ get_next_env (char **envp, char **name, size_t *namelen, char **val,
>  
>  #define TUNABLE_SET_BOUNDS_IF_VALID(__cur, __minp, __maxp, __type)	      \
>  ({									      \
> +  __type curmin = (__cur)->type.min;				      \
> +  __type curmax = (__cur)->type.max;				      \
>    if (__minp != NULL)							      \
>      {									      \
>        /* MIN is specified.  */						      \
> @@ -115,15 +117,13 @@ get_next_env (char **envp, char **name, size_t *namelen, char **val,
>  	{								      \
>  	   /* Both MIN and MAX are specified.  */			      \
>  	    __type max = *((__type *) __maxp);				      \
> -	  if (max >= min						      \
> -	      && max <= (__cur)->type.max				      \
> -	      && min >= (__cur)->type.min)				      \
> +	  if (max >= min && max <= curmax && min >= curmin)		      \
>  	    {								      \
>  	      (__cur)->type.min = min;					      \
>  	      (__cur)->type.max = max;					      \
>  	    }								      \
>  	}								      \
> -      else if (min > (__cur)->type.min && min <= (__cur)->type.max)	      \
> +      else if (min > curmin && min <= curmax)				      \
>  	{								      \
>  	  /* Only MIN is specified.  */					      \
>  	  (__cur)->type.min = min;					      \
> @@ -133,7 +133,7 @@ get_next_env (char **envp, char **name, size_t *namelen, char **val,
>      {									      \
>        /* Only MAX is specified.  */					      \
>        __type max = *((__type *) __maxp);				      \
> -      if (max < (__cur)->type.max && max >= (__cur)->type.min)		      \
> +      if (max < curmax && max >= curmin)				      \
>  	(__cur)->type.max = max;					      \
>      }									      \
>  })
> 

Wouldn't be better to just get rid of this macro mess (which has cause
some issue on the internal syscall mechanism where macro arguments were
not correctly evaluated) and use proper functions instead?

Something like (not sure about the function generation macros, open coded
them might be clear since I don't foresee we adding more different type):

---

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index b1a50b8469..fa9bccfb29 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -93,50 +93,52 @@ get_next_env (char **envp, char **name, size_t *namelen, char **val,
   return NULL;
 }
 
-#define TUNABLE_SET_VAL_IF_VALID_RANGE(__cur, __val, __type)		      \
-({									      \
-  __type min = (__cur)->type.min;					      \
-  __type max = (__cur)->type.max;					      \
-									      \
-  if ((__type) (__val) >= min && (__type) (__val) <= max)		      \
-    {									      \
-      (__cur)->val.numval = (__val);					      \
-      (__cur)->initialized = true;					      \
-    }									      \
-})
-
-#define TUNABLE_SET_BOUNDS_IF_VALID(__cur, __minp, __maxp, __type)	      \
-({									      \
-  if (__minp != NULL)							      \
-    {									      \
-      /* MIN is specified.  */						      \
-      __type min = *((__type *) __minp);				      \
-      if (__maxp != NULL)						      \
-	{								      \
-	   /* Both MIN and MAX are specified.  */			      \
-	    __type max = *((__type *) __maxp);				      \
-	  if (max >= min						      \
-	      && max <= (__cur)->type.max				      \
-	      && min >= (__cur)->type.min)				      \
-	    {								      \
-	      (__cur)->type.min = min;					      \
-	      (__cur)->type.max = max;					      \
-	    }								      \
-	}								      \
-      else if (min > (__cur)->type.min && min <= (__cur)->type.max)	      \
-	{								      \
-	  /* Only MIN is specified.  */					      \
-	  (__cur)->type.min = min;					      \
-	}								      \
-    }									      \
-  else if (__maxp != NULL)						      \
-    {									      \
-      /* Only MAX is specified.  */					      \
-      __type max = *((__type *) __maxp);				      \
-      if (max < (__cur)->type.max && max >= (__cur)->type.min)		      \
-	(__cur)->type.max = max;					      \
-    }									      \
-})
+#define DEFINE_TUNABLE_SET_VAL_IF_VALID_RANGE(__type)			\
+static void								\
+do_tunable_set_val_if_valid_range_##__type(tunable_t *cur, __type val)	\
+{									\
+  __type min = cur->type.min;						\
+  __type max = cur->type.max;						\
+  if (val >= min && val <= max)						\
+    {									\
+      cur->val.numval = val;						\
+      cur->initialized = true;						\
+    }									\
+}
+DEFINE_TUNABLE_SET_VAL_IF_VALID_RANGE (int64_t)
+DEFINE_TUNABLE_SET_VAL_IF_VALID_RANGE (uint64_t)
+
+#define DEFINE_TUNABLE_SET_BOUNDS_IF_VALID(__type) 			\
+static void								\
+do_tunable_update_val_##__type (tunable_t *cur, const __type *minp,	\
+				const __type *maxp) 			\
+{									\
+  if (minp != NULL)							\
+    {									\
+      __type min = *minp;						\
+      if (maxp != NULL)							\
+	{								\
+	  __type max = *maxp;						\
+	  if (max >= min						\
+	      && max <= cur->type.max					\
+	      && min >= cur->type.min)					\
+	    {								\
+	      cur->type.min = min;					\
+	      cur->type.max = max;					\
+	    }								\
+	}								\
+      else if (min > cur->type.min && min <= cur->type.max)		\
+	cur->type.min = min;						\
+    }									\
+  else if (maxp != NULL)						\
+    {									\
+      __type max = *maxp;						\
+      if (max < cur->type.max && max >= cur->type.min)			\
+	cur->type.max = max;						\
+    }									\
+}
+DEFINE_TUNABLE_SET_BOUNDS_IF_VALID (int64_t)
+DEFINE_TUNABLE_SET_BOUNDS_IF_VALID (uint64_t)
 
 static void
 do_tunable_update_val (tunable_t *cur, const void *valp,
@@ -150,28 +152,17 @@ do_tunable_update_val (tunable_t *cur, const void *valp,
   switch (cur->type.type_code)
     {
     case TUNABLE_TYPE_INT_32:
-	{
-	  TUNABLE_SET_BOUNDS_IF_VALID (cur, minp, maxp, int64_t);
-	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, int64_t);
-	  break;
-	}
+      do_tunable_update_val_int64_t (cur, minp, maxp);
+      do_tunable_set_val_if_valid_range_int64_t (cur, val);
+      break;
     case TUNABLE_TYPE_UINT_64:
-	{
-	  TUNABLE_SET_BOUNDS_IF_VALID (cur, minp, maxp, uint64_t);
-	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, uint64_t);
-	  break;
-	}
     case TUNABLE_TYPE_SIZE_T:
-	{
-	  TUNABLE_SET_BOUNDS_IF_VALID (cur, minp, maxp, uint64_t);
-	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, uint64_t);
-	  break;
-	}
+      do_tunable_update_val_uint64_t (cur, minp, maxp);
+      do_tunable_set_val_if_valid_range_uint64_t (cur, val);
+      break;
     case TUNABLE_TYPE_STRING:
-	{
-	  cur->val.strval = valp;
-	  break;
-	}
+      cur->val.strval = valp;
+      break;
     default:
       __builtin_unreachable ();
     }

Re: [PATCH 1/3] Fix casts when setting tunable range

[Siddhesh Poyarekar]

On 2/4/21 7:14 PM, Adhemerval Zanella wrote:
> Wouldn't be better to just get rid of this macro mess (which has cause
> some issue on the internal syscall mechanism where macro arguments were
> not correctly evaluated) and use proper functions instead?
> 
> Something like (not sure about the function generation macros, open coded
> them might be clear since I don't foresee we adding more different type):

Yeah it's actually just signed vs unsigned, I can definitely make this 
macro-less.  I'll post an update for this patch tomorrow.

Thanks,
Siddhesh

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

[Siddhesh Poyarekar]

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.
---
 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
@@ -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;
-- 
2.29.2

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

[Adhemerval Zanella]

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.

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

[Siddhesh Poyarekar]

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

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

[Adhemerval Zanella]

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

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

[H.J. Lu]

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



-- 
H.J.

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

[Siddhesh Poyarekar]

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

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

[Siddhesh Poyarekar]

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

[PATCH 3/3] x86: Use SIZE_MAX instead of (long int)-1 for tunable range value

[Siddhesh Poyarekar]

The tunable types are SIZE_T, so set the ranges to the correct maximum
value, i.e. SIZE_MAX.
---
 sysdeps/x86/dl-cacheinfo.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sysdeps/x86/dl-cacheinfo.h b/sysdeps/x86/dl-cacheinfo.h
index e0a72568d8..6f91651f0d 100644
--- a/sysdeps/x86/dl-cacheinfo.h
+++ b/sysdeps/x86/dl-cacheinfo.h
@@ -917,14 +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, data, 0, (long int) -1);
-  TUNABLE_SET_WITH_BOUNDS (x86_shared_cache_size, shared, 0, (long int) -1);
+  TUNABLE_SET_WITH_BOUNDS (x86_data_cache_size, data, 0, SIZE_MAX);
+  TUNABLE_SET_WITH_BOUNDS (x86_shared_cache_size, shared, 0, SIZE_MAX);
   TUNABLE_SET_WITH_BOUNDS (x86_non_temporal_threshold, non_temporal_threshold,
-			   0, (long int) -1);
+			   0, SIZE_MAX);
   TUNABLE_SET_WITH_BOUNDS (x86_rep_movsb_threshold, rep_movsb_threshold,
-			   minimum_rep_movsb_threshold, (long int) -1);
+			   minimum_rep_movsb_threshold, SIZE_MAX);
   TUNABLE_SET_WITH_BOUNDS (x86_rep_stosb_threshold, rep_stosb_threshold, 1,
-			   (long int) -1);
+			   SIZE_MAX);
 #endif
 
   cpu_features->data_cache_size = data;
-- 
2.29.2

Re: [PATCH 3/3] x86: Use SIZE_MAX instead of (long int)-1 for tunable range value

[Adhemerval Zanella]

On 03/02/2021 14:34, Siddhesh Poyarekar via Libc-alpha wrote:
> The tunable types are SIZE_T, so set the ranges to the correct maximum
> value, i.e. SIZE_MAX.


LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  sysdeps/x86/dl-cacheinfo.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/sysdeps/x86/dl-cacheinfo.h b/sysdeps/x86/dl-cacheinfo.h
> index e0a72568d8..6f91651f0d 100644
> --- a/sysdeps/x86/dl-cacheinfo.h
> +++ b/sysdeps/x86/dl-cacheinfo.h
> @@ -917,14 +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, data, 0, (long int) -1);
> -  TUNABLE_SET_WITH_BOUNDS (x86_shared_cache_size, shared, 0, (long int) -1);
> +  TUNABLE_SET_WITH_BOUNDS (x86_data_cache_size, data, 0, SIZE_MAX);
> +  TUNABLE_SET_WITH_BOUNDS (x86_shared_cache_size, shared, 0, SIZE_MAX);
>    TUNABLE_SET_WITH_BOUNDS (x86_non_temporal_threshold, non_temporal_threshold,
> -			   0, (long int) -1);
> +			   0, SIZE_MAX);
>    TUNABLE_SET_WITH_BOUNDS (x86_rep_movsb_threshold, rep_movsb_threshold,
> -			   minimum_rep_movsb_threshold, (long int) -1);
> +			   minimum_rep_movsb_threshold, SIZE_MAX);
>    TUNABLE_SET_WITH_BOUNDS (x86_rep_stosb_threshold, rep_stosb_threshold, 1,
> -			   (long int) -1);
> +			   SIZE_MAX);
>  #endif
>  
>    cpu_features->data_cache_size = data;
>