diff mbox series

[2/4] Set tunable value as well as min/max values

Message ID 20200918160709.949608-3-hjl.tools@gmail.com
State Superseded
Headers
Series [1/4] x86: Initialize CPU info via IFUNC relocation [BZ 26203] |

Commit Message

H.J. Lu Sept. 18, 2020, 4:07 p.m. UTC 
  Some tunable values and their minimum/maximum values must be determinted
at run-time.  Add TUNABLE_SET_ALL and TUNABLE_SET_ALL_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      | 17 ++++++++++++-----
 elf/dl-tunables.h      | 18 ++++++++++++++++--
 manual/README.tunables | 24 ++++++++++++++++++++++--
 3 files changed, 50 insertions(+), 9 deletions(-)

Comments

Florian Weimer Sept. 28, 2020, 1:35 p.m. UTC | #1 
* H. J. Lu via Libc-alpha:

> Some tunable values and their minimum/maximum values must be determinted
> at run-time.  Add TUNABLE_SET_ALL and TUNABLE_SET_ALL_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.

I'm not sure if this change is philosophically correct as far as the
tunables framework is concerned.  I had thought the limits should be
something static, so that they are consistent across systems.

Maybe Siddhesh can comment on that aspect?

What is supposed to happen if you specify an out-of-range value?

Thanks,
Florian
H.J. Lu Sept. 28, 2020, 1:53 p.m. UTC | #2 
On Mon, Sep 28, 2020 at 6:35 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu via Libc-alpha:
>
> > Some tunable values and their minimum/maximum values must be determinted
> > at run-time.  Add TUNABLE_SET_ALL and TUNABLE_SET_ALL_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.
>
> I'm not sure if this change is philosophically correct as far as the
> tunables framework is concerned.  I had thought the limits should be
> something static, so that they are consistent across systems.

Some x86 tunables ranges are dynamic.

> Maybe Siddhesh can comment on that aspect?
>
> What is supposed to happen if you specify an out-of-range value?

It should be rejected.  Otherwise programs will crash.
Florian Weimer Sept. 28, 2020, 2:03 p.m. UTC | #3 
* H. J. Lu:

> On Mon, Sep 28, 2020 at 6:35 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * H. J. Lu via Libc-alpha:
>>
>> > Some tunable values and their minimum/maximum values must be determinted
>> > at run-time.  Add TUNABLE_SET_ALL and TUNABLE_SET_ALL_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.
>>
>> I'm not sure if this change is philosophically correct as far as the
>> tunables framework is concerned.  I had thought the limits should be
>> something static, so that they are consistent across systems.
>
> Some x86 tunables ranges are dynamic.
>
>> Maybe Siddhesh can comment on that aspect?
>>
>> What is supposed to happen if you specify an out-of-range value?
>
> It should be rejected.  Otherwise programs will crash.

You could still do this outside the tunables framework, I think.

Let's wait for Siddhesh's comments.

Thanks,
Florian
Siddhesh Poyarekar Sept. 28, 2020, 5:30 p.m. UTC | #4 
On 28/09/20 19:05, Florian Weimer via Libc-alpha wrote:
> I'm not sure if this change is philosophically correct as far as the
> tunables framework is concerned.  I had thought the limits should be
> something static, so that they are consistent across systems.

It seems like a good idea to support dynamic limits if they will always
be more restrictive than the most restrictive static limit one could
come up with for the tunable.  I didn't exclude dynamic limits from a
design perspective; it's just that the tunables implemented at that time
didn't need them.

There is a case to always have static bounds (at the minimum to ensure
that values don't overflow the underlying types) but that shouldn't
preclude more restrictive dynamic limits IMO.

Bikeshed: maybe the macro should be called TUNABLE_SET_WITH_BOUNDS()
instead of TUNABLE_SET_ALL.

Siddhesh
diff mbox series

Patch

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index 26e6e26612..b44174fe71 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -101,12 +101,19 @@  get_next_env (char **envp, char **name, size_t *namelen, char **val,
 })
 
 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;
 
   if (cur->type.type_code != TUNABLE_TYPE_STRING)
-    val = *((int64_t *) valp);
+    {
+      val = *((int64_t *) valp);
+      if (minp)
+	cur->type.min = *((int64_t *) minp);
+      if (maxp)
+	cur->type.max = *((int64_t *) maxp);
+    }
 
   switch (cur->type.type_code)
     {
@@ -153,15 +160,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..b1add3184f 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,16 @@  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_ALL(__id, __type, __val, __min, __max) \
+  TUNABLE_SET_ALL_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_ALL(__top, __ns, __id, __type, __val, __min, __max) \
+  TUNABLE_SET_ALL_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 +109,15 @@  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_ALL_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..db6f6bae8d 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_ALL 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_ALL (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_ALL_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_ALL
+and TUNABLE_SET_ALL_FULL.
 
 ** IMPORTANT NOTE **