diff mbox series

V4 [PATCH] Set tunable value as well as min/max values

Message ID CAMe9rOo9pm2_XQ=wih-f+U3t_LJMLaNCLKTur-dfoC4Ca=1-KQ@mail.gmail.com
State Superseded
Headers
Series V4 [PATCH] Set tunable value as well as min/max values |

Commit Message

H.J. Lu Sept. 29, 2020, 12:30 p.m. UTC 
  On Mon, Sep 28, 2020 at 9:47 PM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
>
> (Sorry, replying to myself)
>
> On 29/09/20 10:15, Siddhesh Poyarekar via Libc-alpha wrote:
> > There should be a check here to ensure that the bounds do not exceed
> > statically set bounds.  That is:
> >
> >     if (minp != NULL && cur->type.min < *((int64_t *) minp))
> >       cur->type.min = *((int64_t *) minp);
> >     if (maxp != NULL && cur->type.max > *((int64_t *) maxp))
> >       cur->type.max = *((int64_t *) maxp);
>
> Also check for min > max type invalid ranges.
>

Here is the updated patch with TUNABLE_SET_BOUNDS_IF_VALID.

OK for master?

Thanks.

Comments

Siddhesh Poyarekar Sept. 29, 2020, 1:50 p.m. UTC | #1 
On 29/09/20 18:00, H.J. Lu wrote:
> Here is the updated patch with TUNABLE_SET_BOUNDS_IF_VALID.
> 
> OK for master?

I don't think TUNABLE_SET_BOUNDS_IF_VALID is doing what it intends to do.

> +#define TUNABLE_SET_BOUNDS_IF_VALID(__cur, __maxp, __minp, __type)	      \

minp and maxp are switched around.

> +({									      \
> +  __type min = __minp ? *((__type *) __minp) : (__cur)->type.min;	      \
> +  __type max = __maxp ? *((__type *) __maxp) : (__cur)->type.max;	      \
> +  if (__minp)								      \
> +    {									      \
> +      if (__maxp)							      \
> +	{								      \
> +	  if (max > min)						      \
> +	    {								      \
> +	      (__cur)->type.min = min;					      \
> +	      (__cur)->type.max = max;					      \
> +	    }								      \

When both minp and maxp are specified, it checks if they're sane with
respect to each other but it should also check whether the range they
describe is a *subset* of (__cur)->type.min and (__cur)->type.max.

> +	}								      \
> +      else if (max > min)						      \
> +	(__cur)->type.min = min;					      \

You also need to make sure that (__cur)->type.min < min to ensure a more
restrictive range.

> +    }									      \
> +  else if (max > min)							      \
> +    (__cur)->type.max = min;						      \

I did not understand this one.

Basically, this is what it should look like:

    if (__minp != NULL
        && *__minp <= *__maxp
        && *__minp >= (__cur)->type.min
        && *__minp <= (__cur)->type.max)
      (__cur)->type.min = *__minp;

    if (__maxp != NULL
        && *__minp <= *_maxp
        && *__maxp >= (__cur)->type.min
        && *__maxp <= (__cur)->type.max)
      (__cur)->type.max = *maxp;

You could collapse some of these conditions if we assume that maxp and
minp are always either NULL or not NULL together.

Siddhesh
diff mbox series

Patch

From aa49996f8e233b081d511441d1f1b557a7fd498f Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Mon, 1 Jun 2020 14:11:32 -0700
Subject: [PATCH] Set tunable value as well as min/max values

Some tunable values and their minimum/maximum values must be determinted
at run-time.  Add TUNABLE_SET_WITH_BOUNDS and TUNABLE_SET_WITH_BOUNDS_FULL
to update tunable value together with minimum and maximum values.
__tunable_set_val is updated to set tunable value as well as min/max
values.
---
 elf/dl-tunables.c      | 33 +++++++++++++++++++++++++++++----
 elf/dl-tunables.h      | 21 +++++++++++++++++++--
 manual/README.tunables | 24 ++++++++++++++++++++++--
 3 files changed, 70 insertions(+), 8 deletions(-)

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index 26e6e26612..6e8ca36fef 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -100,8 +100,30 @@  get_next_env (char **envp, char **name, size_t *namelen, char **val,
     }									      \
 })
 
+#define TUNABLE_SET_BOUNDS_IF_VALID(__cur, __maxp, __minp, __type)	      \
+({									      \
+  __type min = __minp ? *((__type *) __minp) : (__cur)->type.min;	      \
+  __type max = __maxp ? *((__type *) __maxp) : (__cur)->type.max;	      \
+  if (__minp)								      \
+    {									      \
+      if (__maxp)							      \
+	{								      \
+	  if (max > min)						      \
+	    {								      \
+	      (__cur)->type.min = min;					      \
+	      (__cur)->type.max = max;					      \
+	    }								      \
+	}								      \
+      else if (max > min)						      \
+	(__cur)->type.min = min;					      \
+    }									      \
+  else if (max > min)							      \
+    (__cur)->type.max = min;						      \
+})
+
 static void
-do_tunable_update_val (tunable_t *cur, const void *valp)
+do_tunable_update_val (tunable_t *cur, const void *valp,
+		       const void *minp, const void *maxp)
 {
   uint64_t val;
 
@@ -112,16 +134,19 @@  do_tunable_update_val (tunable_t *cur, const void *valp)
     {
     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;
 	}
     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;
 	}
@@ -153,15 +178,15 @@  tunable_initialize (tunable_t *cur, const char *strval)
       cur->initialized = true;
       valp = strval;
     }
-  do_tunable_update_val (cur, valp);
+  do_tunable_update_val (cur, valp, NULL, NULL);
 }
 
 void
-__tunable_set_val (tunable_id_t id, void *valp)
+__tunable_set_val (tunable_id_t id, void *valp, void *minp, void *maxp)
 {
   tunable_t *cur = &tunable_list[id];
 
-  do_tunable_update_val (cur, valp);
+  do_tunable_update_val (cur, valp, minp, maxp);
 }
 
 #if TUNABLES_FRONTEND == TUNABLES_FRONTEND_valstring
diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h
index f05eb50c2f..550b0cc7f4 100644
--- a/elf/dl-tunables.h
+++ b/elf/dl-tunables.h
@@ -70,9 +70,10 @@  typedef struct _tunable tunable_t;
 
 extern void __tunables_init (char **);
 extern void __tunable_get_val (tunable_id_t, void *, tunable_callback_t);
-extern void __tunable_set_val (tunable_id_t, void *);
+extern void __tunable_set_val (tunable_id_t, void *, void *, void *);
 rtld_hidden_proto (__tunables_init)
 rtld_hidden_proto (__tunable_get_val)
+rtld_hidden_proto (__tunable_set_val)
 
 /* Define TUNABLE_GET and TUNABLE_SET in short form if TOP_NAMESPACE and
    TUNABLE_NAMESPACE are defined.  This is useful shorthand to get and set
@@ -82,11 +83,18 @@  rtld_hidden_proto (__tunable_get_val)
   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) \
+  TUNABLE_SET_WITH_BOUNDS_FULL (TOP_NAMESPACE, TUNABLE_NAMESPACE, __id, \
+				__type, __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)
 #endif
 
 /* Get and return a tunable value.  If the tunable was set externally and __CB
@@ -103,7 +111,16 @@  rtld_hidden_proto (__tunable_get_val)
 # define TUNABLE_SET_FULL(__top, __ns, __id, __type, __val) \
 ({									      \
   __tunable_set_val (TUNABLE_ENUM_NAME (__top, __ns, __id),		      \
-			& (__type) {__val});				      \
+		     & (__type) {__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)			      \
+({									      \
+  __tunable_set_val (TUNABLE_ENUM_NAME (__top, __ns, __id),		      \
+		     & (__type) {__val},  & (__type) {__min},		      \
+		     & (__type) {__max});				      \
 })
 
 /* Namespace sanity for callback functions.  Use this macro to keep the
diff --git a/manual/README.tunables b/manual/README.tunables
index f87a31a65e..fff6c2a87e 100644
--- a/manual/README.tunables
+++ b/manual/README.tunables
@@ -67,7 +67,7 @@  The list of allowed attributes are:
 				     non-AT_SECURE subprocesses.
 			NONE: Read all the time.
 
-2. Use TUNABLE_GET/TUNABLE_SET to get and set tunables.
+2. Use TUNABLE_GET/TUNABLE_SET/TUNABLE_SET_WITH_BOUNDS to get and set tunables.
 
 3. OPTIONAL: If tunables in a namespace are being used multiple times within a
    specific module, set the TUNABLE_NAMESPACE macro to reduce the amount of
@@ -112,9 +112,29 @@  form of the macros as follows:
 where 'glibc' is the top namespace, 'cpu' is the tunable namespace and the
 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)
+
+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.
+
+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)
+
+where 'glibc' is the top namespace, 'cpu' is the tunable namespace and the
+remaining arguments are the same as the short form macros.
+
 When TUNABLE_NAMESPACE is not defined in a module, TUNABLE_GET is equivalent to
 TUNABLE_GET_FULL, so you will need to provide full namespace information for
-both macros.  Likewise for TUNABLE_SET and TUNABLE_SET_FULL.
+both macros.  Likewise for TUNABLE_SET, TUNABLE_SET_FULL,
+TUNABLE_SET_WITH_BOUNDS and TUNABLE_SET_WITH_BOUNDS_FULL.
 
 ** IMPORTANT NOTE **
 
-- 
2.26.2