[PATCH v5 0/2] sched: Optionally skip uclamp logic in fast path

[PATCH v5 0/2] sched: Optionally skip uclamp logic in fast path

[Qais Yousef]

This series attempts to address the report that uclamp logic could be expensive
sometimes and shows a regression in netperf UDP_STREAM under certain
conditions.

The first patch is a fix for how struct uclamp_rq is initialized which is
required by the 2nd patch which contains the real 'fix'.

Worth noting that the root cause of the overhead is believed to be system
specific or related to potential certain code/data layout issues, leading to
worse I/D $ performance.

Different systems exhibited different behaviors and the regression did
disappear in certain kernel version while attempting to reporoduce.

More info can be found here:

https://lore.kernel.org/lkml/20200616110824.dgkkbyapn3io6wik@e107158-lin/

Having the static key seemed the best thing to do to ensure the effect of
uclamp is minimized for kernels that compile it in but don't have a userspace
that uses it, which will allow distros to distribute uclamp capable kernels by
default without having to compromise on performance for some systems that could
be affected.

Changes in v5:
	* Fix a race that could happen when order of enqueue/dequeue of tasks
	  A and B is not done in order, and sched_uclamp_used is enabled in
	  between.
	* Add more comments explaining the race and the behavior of
	  uclamp_rq_util_with() which is now protected with a static key to be
	  a NOP. When no uclamp aggregation at rq level is done, this function
	  can't do much.

Changes in v4:
	* Fix broken boosting of RT tasks when static key is disabled.

Changes in v3:
	* Avoid double negatives and rename the static key to uclamp_used
	* Unconditionally enable the static key through any of the paths where
	  the user can modify the default uclamp value.
	* Use C99 named struct initializer for struct uclamp_rq which is easier
	  to read than the memset().

Changes in v2:
	* Add more info in the commit message about the result of perf diff to
	  demonstrate that the activate/deactivate_task pressure is reduced in
	  the fast path.

	* Fix sparse warning reported by the test robot.

	* Add an extra commit about using static_branch_likely() instead of
	  static_branc_unlikely().

Thanks

--
Qais Yousef

Cc: Juri Lelli <juri.lelli@redhat.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ben Segall <bsegall@google.com>
Cc: Mel Gorman <mgorman@suse.de>
CC: Patrick Bellasi <patrick.bellasi@matbug.net>
Cc: Chris Redpath <chris.redpath@arm.com>
Cc: Lukasz Luba <lukasz.luba@arm.com>
Cc: linux-kernel@vger.kernel.org


Qais Yousef (2):
  sched/uclamp: Fix initialization of struct uclamp_rq
  sched/uclamp: Protect uclamp fast path code with static key

 kernel/sched/core.c              | 86 +++++++++++++++++++++++++++++---
 kernel/sched/cpufreq_schedutil.c |  2 +-
 kernel/sched/sched.h             | 39 ++++++++++++++-
 3 files changed, 117 insertions(+), 10 deletions(-)

-- 
2.17.1

[PATCH v5 1/2] sched/uclamp: Fix initialization of struct uclamp_rq

[Qais Yousef]

struct uclamp_rq was zeroed out entirely in assumption that in the first
call to uclamp_rq_inc() they'd be initialized correctly in accordance to
default settings.

But when next patch introduces a static key to skip
uclamp_rq_{inc,dec}() until userspace opts in to use uclamp, schedutil
will fail to perform any frequency changes because the
rq->uclamp[UCLAMP_MAX].value is zeroed at init and stays as such. Which
means all rqs are capped to 0 by default.

Fix it by making sure we do proper initialization at init without
relying on uclamp_rq_inc() doing it later.

Fixes: 69842cba9ace ("sched/uclamp: Add CPU's clamp buckets refcounting")
Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
Signed-off-by: Qais Yousef <qais.yousef@arm.com>
Cc: Juri Lelli <juri.lelli@redhat.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ben Segall <bsegall@google.com>
Cc: Mel Gorman <mgorman@suse.de>
CC: Patrick Bellasi <patrick.bellasi@matbug.net>
Cc: Chris Redpath <chris.redpath@arm.com>
Cc: Lukasz Luba <lukasz.luba@arm.com>
Cc: linux-kernel@vger.kernel.org
---
 kernel/sched/core.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 8fe2ac910bed..235b2cae00a0 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1248,6 +1248,20 @@ static void uclamp_fork(struct task_struct *p)
 	}
 }
 
+static void __init init_uclamp_rq(struct rq *rq)
+{
+	enum uclamp_id clamp_id;
+	struct uclamp_rq *uc_rq = rq->uclamp;
+
+	for_each_clamp_id(clamp_id) {
+		uc_rq[clamp_id] = (struct uclamp_rq) {
+			.value = uclamp_none(clamp_id)
+		};
+	}
+
+	rq->uclamp_flags = 0;
+}
+
 static void __init init_uclamp(void)
 {
 	struct uclamp_se uc_max = {};
@@ -1256,11 +1270,8 @@ static void __init init_uclamp(void)
 
 	mutex_init(&uclamp_mutex);
 
-	for_each_possible_cpu(cpu) {
-		memset(&cpu_rq(cpu)->uclamp, 0,
-				sizeof(struct uclamp_rq)*UCLAMP_CNT);
-		cpu_rq(cpu)->uclamp_flags = 0;
-	}
+	for_each_possible_cpu(cpu)
+		init_uclamp_rq(cpu_rq(cpu));
 
 	for_each_clamp_id(clamp_id) {
 		uclamp_se_set(&init_task.uclamp_req[clamp_id],
-- 
2.17.1

[PATCH v5 2/2] sched/uclamp: Protect uclamp fast path code with static key

[Qais Yousef]

There is a report that when uclamp is enabled, a netperf UDP test
regresses compared to a kernel compiled without uclamp.

https://lore.kernel.org/lkml/20200529100806.GA3070@suse.de/

While investigating the root cause, there were no sign that the uclamp
code is doing anything particularly expensive but could suffer from bad
cache behavior under certain circumstances that are yet to be
understood.

https://lore.kernel.org/lkml/20200616110824.dgkkbyapn3io6wik@e107158-lin/

To reduce the pressure on the fast path anyway, add a static key that is
by default will skip executing uclamp logic in the
enqueue/dequeue_task() fast path until it's needed.

As soon as the user start using util clamp by:

	1. Changing uclamp value of a task with sched_setattr()
	2. Modifying the default sysctl_sched_util_clamp_{min, max}
	3. Modifying the default cpu.uclamp.{min, max} value in cgroup

We flip the static key now that the user has opted to use util clamp.
Effectively re-introducing uclamp logic in the enqueue/dequeue_task()
fast path. It stays on from that point forward until the next reboot.

This should help minimize the effect of util clamp on workloads that
don't need it but still allow distros to ship their kernels with uclamp
compiled in by default.

SCHED_WARN_ON() in uclamp_rq_dec_id() was removed since now we can end
up with unbalanced call to uclamp_rq_dec_id() if we flip the key while
a task is running in the rq. Since we know it is harmless we just
quietly return if we attempt a uclamp_rq_dec_id() when
rq->uclamp[].bucket[].tasks is 0.

In schedutil, we introduce a new uclamp_is_enabled() helper which takes
the static key into account to ensure RT boosting behavior is retained.

The following results demonstrates how this helps on 2 Sockets Xeon E5
2x10-Cores system.

                                   nouclamp                 uclamp      uclamp-static-key
Hmean     send-64         162.43 (   0.00%)      157.84 *  -2.82%*      163.39 *   0.59%*
Hmean     send-128        324.71 (   0.00%)      314.78 *  -3.06%*      326.18 *   0.45%*
Hmean     send-256        641.55 (   0.00%)      628.67 *  -2.01%*      648.12 *   1.02%*
Hmean     send-1024      2525.28 (   0.00%)     2448.26 *  -3.05%*     2543.73 *   0.73%*
Hmean     send-2048      4836.14 (   0.00%)     4712.08 *  -2.57%*     4867.69 *   0.65%*
Hmean     send-3312      7540.83 (   0.00%)     7425.45 *  -1.53%*     7621.06 *   1.06%*
Hmean     send-4096      9124.53 (   0.00%)     8948.82 *  -1.93%*     9276.25 *   1.66%*
Hmean     send-8192     15589.67 (   0.00%)    15486.35 *  -0.66%*    15819.98 *   1.48%*
Hmean     send-16384    26386.47 (   0.00%)    25752.25 *  -2.40%*    26773.74 *   1.47%*

The perf diff between nouclamp and uclamp-static-key when uclamp is
disabled in the fast path:

     8.73%     -1.55%  [kernel.kallsyms]        [k] try_to_wake_up
     0.07%     +0.04%  [kernel.kallsyms]        [k] deactivate_task
     0.13%     -0.02%  [kernel.kallsyms]        [k] activate_task

The diff between nouclamp and uclamp-static-key when uclamp is enabled
in the fast path:

     8.73%     -0.72%  [kernel.kallsyms]        [k] try_to_wake_up
     0.13%     +0.39%  [kernel.kallsyms]        [k] activate_task
     0.07%     +0.38%  [kernel.kallsyms]        [k] deactivate_task

Reported-by: Mel Gorman <mgorman@suse.de>
Tested-by: Lukasz Luba <lukasz.luba@arm.com>
Fixes: 69842cba9ace ("sched/uclamp: Add CPU's clamp buckets refcounting")
Signed-off-by: Qais Yousef <qais.yousef@arm.com>
Cc: Juri Lelli <juri.lelli@redhat.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ben Segall <bsegall@google.com>
Cc: Mel Gorman <mgorman@suse.de>
CC: Patrick Bellasi <patrick.bellasi@matbug.net>
Cc: Chris Redpath <chris.redpath@arm.com>
Cc: Lukasz Luba <lukasz.luba@arm.com>
Cc: linux-kernel@vger.kernel.org
---

This takes a different approach to PSI which introduces a config option

```
      CONFIG_PSI_DEFAULT_DISABLED

        Require boot parameter to enable pressure stall information
        tracking (NEW)

      boot param psi
```

via commit e0c274472d5d "psi: make disabling/enabling easier for vendor kernels"

uclamp has a clearer points of entry when userspace would like to use it so we
can automatically flip the switch if the kernel is running on a userspace that
wants to user utilclamp without any extra userspace visible switches.

I wanted to make this dependent on schedutil being the governor too, but beside
the complexity, uclamp is used for capacity awareness. We could certainly
construct a more complex condition, but I'm not sure it's worth it. Open to
hear more opinions and points of views on this :)


 kernel/sched/core.c              | 65 +++++++++++++++++++++++++++++++-
 kernel/sched/cpufreq_schedutil.c |  2 +-
 kernel/sched/sched.h             | 39 ++++++++++++++++++-
 3 files changed, 101 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 235b2cae00a0..8d80d6091d86 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -794,6 +794,26 @@ unsigned int sysctl_sched_uclamp_util_max = SCHED_CAPACITY_SCALE;
 /* All clamps are required to be less or equal than these values */
 static struct uclamp_se uclamp_default[UCLAMP_CNT];
 
+/*
+ * This static key is used to reduce the uclamp overhead in the fast path. It
+ * primarily disables the call to uclamp_rq_{inc, dec}() in
+ * enqueue/dequeue_task().
+ *
+ * This allows users to continue to enable uclamp in their kernel config with
+ * minimum uclamp overhead in the fast path.
+ *
+ * As soon as userspace modifies any of the uclamp knobs, the static key is
+ * enabled, since we have an actual users that make use of uclamp
+ * functionality.
+ *
+ * The knobs that would enable this static key are:
+ *
+ *   * A task modifying its uclamp value with sched_setattr().
+ *   * An admin modifying the sysctl_sched_uclamp_{min, max} via procfs.
+ *   * An admin modifying the cgroup cpu.uclamp.{min, max}
+ */
+DEFINE_STATIC_KEY_FALSE(sched_uclamp_used);
+
 /* Integer rounded range for each bucket */
 #define UCLAMP_BUCKET_DELTA DIV_ROUND_CLOSEST(SCHED_CAPACITY_SCALE, UCLAMP_BUCKETS)
 
@@ -994,9 +1014,30 @@ static inline void uclamp_rq_dec_id(struct rq *rq, struct task_struct *p,
 	lockdep_assert_held(&rq->lock);
 
 	bucket = &uc_rq->bucket[uc_se->bucket_id];
-	SCHED_WARN_ON(!bucket->tasks);
+
+	/*
+	 * bucket->tasks could be zero if sched_uclamp_used was enabled while
+	 * the current task was running, hence we could end up with unbalanced
+	 * call to uclamp_rq_dec_id().
+	 *
+	 * Need to be careful of the following enqeueue/dequeue order
+	 * problem too
+	 *
+	 *	enqueue(taskA)
+	 *	// sched_uclamp_used gets enabled
+	 *	enqueue(taskB)
+	 *	dequeue(taskA)
+	 *	// bucket->tasks is now 0
+	 *	dequeue(taskB)
+	 *
+	 * where we could end up with uc_se->active of the task set to true and
+	 * the wrong bucket[uc_se->bucket_id].value.
+	 *
+	 * Hence always make sure we reset things properly.
+	 */
 	if (likely(bucket->tasks))
 		bucket->tasks--;
+
 	uc_se->active = false;
 
 	/*
@@ -1032,6 +1073,13 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
 {
 	enum uclamp_id clamp_id;
 
+	/*
+	 * Avoid any overhead until uclamp is actually used by the userspace.
+	 * Including the branch if we use static_branch_likely()
+	 */
+	if (!static_branch_unlikely(&sched_uclamp_used))
+		return;
+
 	if (unlikely(!p->sched_class->uclamp_enabled))
 		return;
 
@@ -1047,6 +1095,13 @@ static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
 {
 	enum uclamp_id clamp_id;
 
+	/*
+	 * Avoid any overhead until uclamp is actually used by the userspace.
+	 * Including the branch if we use static_branch_likely()
+	 */
+	if (!static_branch_unlikely(&sched_uclamp_used))
+		return;
+
 	if (unlikely(!p->sched_class->uclamp_enabled))
 		return;
 
@@ -1155,8 +1210,10 @@ int sysctl_sched_uclamp_handler(struct ctl_table *table, int write,
 		update_root_tg = true;
 	}
 
-	if (update_root_tg)
+	if (update_root_tg) {
+		static_branch_enable(&sched_uclamp_used);
 		uclamp_update_root_tg();
+	}
 
 	/*
 	 * We update all RUNNABLE tasks only when task groups are in use.
@@ -1221,6 +1278,8 @@ static void __setscheduler_uclamp(struct task_struct *p,
 	if (likely(!(attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)))
 		return;
 
+	static_branch_enable(&sched_uclamp_used);
+
 	if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MIN) {
 		uclamp_se_set(&p->uclamp_req[UCLAMP_MIN],
 			      attr->sched_util_min, true);
@@ -7387,6 +7446,8 @@ static ssize_t cpu_uclamp_write(struct kernfs_open_file *of, char *buf,
 	if (req.ret)
 		return req.ret;
 
+	static_branch_enable(&sched_uclamp_used);
+
 	mutex_lock(&uclamp_mutex);
 	rcu_read_lock();
 
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 7fbaee24c824..3f4e296ccb67 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -210,7 +210,7 @@ unsigned long schedutil_cpu_util(int cpu, unsigned long util_cfs,
 	unsigned long dl_util, util, irq;
 	struct rq *rq = cpu_rq(cpu);
 
-	if (!IS_BUILTIN(CONFIG_UCLAMP_TASK) &&
+	if (!uclamp_is_enabled() &&
 	    type == FREQUENCY_UTIL && rt_rq_is_runnable(&rq->rt)) {
 		return max;
 	}
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 1d4e94c1e5fe..e700a70008e5 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -862,6 +862,8 @@ struct uclamp_rq {
 	unsigned int value;
 	struct uclamp_bucket bucket[UCLAMP_BUCKETS];
 };
+
+DECLARE_STATIC_KEY_FALSE(sched_uclamp_used);
 #endif /* CONFIG_UCLAMP_TASK */
 
 /*
@@ -2349,12 +2351,35 @@ static inline void cpufreq_update_util(struct rq *rq, unsigned int flags) {}
 #ifdef CONFIG_UCLAMP_TASK
 unsigned long uclamp_eff_value(struct task_struct *p, enum uclamp_id clamp_id);
 
+/**
+ * uclamp_rq_util_with - clamp @util with @rq and @p effective uclamp values.
+ * @rq:		The rq to clamp against. Must not be NULL.
+ * @util:	The util value to clamp.
+ * @p:		The task to clamp against. Can be NULL if you want to clamp
+ *		against @rq only.
+ *
+ * Clamps the passed @util to the max(@rq, @p) effective uclamp values.
+ *
+ * If sched_uclamp_used static key is disabled, then just return the util
+ * without any clamping since uclamp aggregation at the rq level in the fast
+ * path is disabled, rendering this operation a NOP.
+ *
+ * Use uclamp_eff_value() if you don't care about uclamp values at rq level. It
+ * will return the correct effective uclamp value of the task even if the
+ * static key is disabled.
+ */
 static __always_inline
 unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util,
 				  struct task_struct *p)
 {
-	unsigned long min_util = READ_ONCE(rq->uclamp[UCLAMP_MIN].value);
-	unsigned long max_util = READ_ONCE(rq->uclamp[UCLAMP_MAX].value);
+	unsigned long min_util;
+	unsigned long max_util;
+
+	if (!static_branch_likely(&sched_uclamp_used))
+		return util;
+
+	min_util = READ_ONCE(rq->uclamp[UCLAMP_MIN].value);
+	max_util = READ_ONCE(rq->uclamp[UCLAMP_MAX].value);
 
 	if (p) {
 		min_util = max(min_util, uclamp_eff_value(p, UCLAMP_MIN));
@@ -2371,6 +2396,11 @@ unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util,
 
 	return clamp(util, min_util, max_util);
 }
+
+static inline bool uclamp_is_enabled(void)
+{
+	return static_branch_likely(&sched_uclamp_used);
+}
 #else /* CONFIG_UCLAMP_TASK */
 static inline
 unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util,
@@ -2378,6 +2408,11 @@ unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util,
 {
 	return util;
 }
+
+static inline bool uclamp_is_enabled(void)
+{
+	return false;
+}
 #endif /* CONFIG_UCLAMP_TASK */
 
 #ifdef arch_scale_freq_capacity
-- 
2.17.1

Re: [PATCH v5 2/2] sched/uclamp: Protect uclamp fast path code with static key

[Patrick Bellasi]

Hi Qais,
here are some more 2c from me...

On Mon, Jun 29, 2020 at 18:26:33 +0200, Qais Yousef <qais.yousef@arm.com> wrote...

[...]

> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 235b2cae00a0..8d80d6091d86 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -794,6 +794,26 @@ unsigned int sysctl_sched_uclamp_util_max = SCHED_CAPACITY_SCALE;
>  /* All clamps are required to be less or equal than these values */
>  static struct uclamp_se uclamp_default[UCLAMP_CNT];
>  
> +/*
> + * This static key is used to reduce the uclamp overhead in the fast path. It
> + * primarily disables the call to uclamp_rq_{inc, dec}() in
> + * enqueue/dequeue_task().
> + *
> + * This allows users to continue to enable uclamp in their kernel config with
> + * minimum uclamp overhead in the fast path.
> + *
> + * As soon as userspace modifies any of the uclamp knobs, the static key is
> + * enabled, since we have an actual users that make use of uclamp
> + * functionality.
> + *
> + * The knobs that would enable this static key are:
> + *
> + *   * A task modifying its uclamp value with sched_setattr().
> + *   * An admin modifying the sysctl_sched_uclamp_{min, max} via procfs.
> + *   * An admin modifying the cgroup cpu.uclamp.{min, max}

I guess this list can be obtained with a grep or git changelog, moreover
this text will require maintenance.

What about replacing this full comment with something shorted like:

---8<---
      Static key to reduce uclamp overhead in the fast path by disabling
      calls to uclamp_rq_{inc, dec}().
---8<---

> + */
> +DEFINE_STATIC_KEY_FALSE(sched_uclamp_used);
> +
>  /* Integer rounded range for each bucket */
>  #define UCLAMP_BUCKET_DELTA DIV_ROUND_CLOSEST(SCHED_CAPACITY_SCALE, UCLAMP_BUCKETS)
>  
> @@ -994,9 +1014,30 @@ static inline void uclamp_rq_dec_id(struct rq *rq, struct task_struct *p,
>  	lockdep_assert_held(&rq->lock);
>  
>  	bucket = &uc_rq->bucket[uc_se->bucket_id];
> -	SCHED_WARN_ON(!bucket->tasks);
> +
> +	/*
> +	 * bucket->tasks could be zero if sched_uclamp_used was enabled while
> +	 * the current task was running, hence we could end up with unbalanced
> +	 * call to uclamp_rq_dec_id().
> +	 *
> +	 * Need to be careful of the following enqeueue/dequeue order
> +	 * problem too
> +	 *
> +	 *	enqueue(taskA)
> +	 *	// sched_uclamp_used gets enabled
> +	 *	enqueue(taskB)
> +	 *	dequeue(taskA)
> +	 *	// bucket->tasks is now 0
> +	 *	dequeue(taskB)
> +	 *
> +	 * where we could end up with uc_se->active of the task set to true and
> +	 * the wrong bucket[uc_se->bucket_id].value.
> +	 *
> +	 * Hence always make sure we reset things properly.
> +	 */
>  	if (likely(bucket->tasks))
>  		bucket->tasks--;
> +
>  	uc_se->active = false;

Better than v4, what about just using this active flag?

---8<---
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 8f360326861e..465a7645713b 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -990,6 +990,13 @@ static inline void uclamp_rq_dec_id(struct rq *rq, struct task_struct *p,
 
        lockdep_assert_held(&rq->lock);
 
+       /*
+        * If a task was already enqueue at uclamp enable time
+        * nothing has been accounted for it.
+        */
+       if (unlikely(!uc_se->active))
+               return;
+
        bucket = &uc_rq->bucket[uc_se->bucket_id];
        SCHED_WARN_ON(!bucket->tasks);
        if (likely(bucket->tasks))
---8<---

This will allow also to keep in all the ref count checks we have,
e.g. the SChed_WARN_ON().


>  	/*
> @@ -1032,6 +1073,13 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
>  {
>  	enum uclamp_id clamp_id;
>  
> +	/*
> +	 * Avoid any overhead until uclamp is actually used by the userspace.
> +	 * Including the branch if we use static_branch_likely()

I still find this last sentence hard to parse, but perhaps it's just me
still missing a breakfast :)

> +	 */
> +	if (!static_branch_unlikely(&sched_uclamp_used))
> +		return;

I'm also still wondering if the optimization is still working when we
have that ! in front.

Had a check at:

   https://elixir.bootlin.com/linux/latest/source/include/linux/jump_label.h#L399

and AFAIU, it all boils down to cook a __branch_check()'s compiler hint,
and ISTR that those are "anti-patterns"?

That said we do have some usages for this pattern too:

$ git grep '!static_branch_unlikely' | wc -l       36
$ git grep 'static_branch_unlikely' | wc -l       220

?

> +
>  	if (unlikely(!p->sched_class->uclamp_enabled))
>  		return;
>  

[...]

> +/**
> + * uclamp_rq_util_with - clamp @util with @rq and @p effective uclamp values.
> + * @rq:		The rq to clamp against. Must not be NULL.
> + * @util:	The util value to clamp.
> + * @p:		The task to clamp against. Can be NULL if you want to clamp
> + *		against @rq only.
> + *
> + * Clamps the passed @util to the max(@rq, @p) effective uclamp values.
> + *
> + * If sched_uclamp_used static key is disabled, then just return the util
> + * without any clamping since uclamp aggregation at the rq level in the fast
> + * path is disabled, rendering this operation a NOP.
> + *
> + * Use uclamp_eff_value() if you don't care about uclamp values at rq level. It
> + * will return the correct effective uclamp value of the task even if the
> + * static key is disabled.

Well, if you don't care about rq, you don't call a uclamp_rq_* method.

I would say that the above paragraph is redundant, moreover it adds some
cross-reference to a different method (name) which required maintenance.

What about removing it?

> + */
>  static __always_inline
>  unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util,
>  				  struct task_struct *p)
>  {
> -	unsigned long min_util = READ_ONCE(rq->uclamp[UCLAMP_MIN].value);
> -	unsigned long max_util = READ_ONCE(rq->uclamp[UCLAMP_MAX].value);
> +	unsigned long min_util;
> +	unsigned long max_util;
> +
> +	if (!static_branch_likely(&sched_uclamp_used))
> +		return util;
> +
> +	min_util = READ_ONCE(rq->uclamp[UCLAMP_MIN].value);
> +	max_util = READ_ONCE(rq->uclamp[UCLAMP_MAX].value);

I think moving the initialization is not required, the compiler should
be smart enough to place theme where's better.

>  	if (p) {
>  		min_util = max(min_util, uclamp_eff_value(p, UCLAMP_MIN));
> @@ -2371,6 +2396,11 @@ unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util,
>  
>  	return clamp(util, min_util, max_util);
>  }
> +
> +static inline bool uclamp_is_enabled(void)
> +{
> +	return static_branch_likely(&sched_uclamp_used);
> +}

Looks like here we mix up terms, which can be confusing.
AFAIKS, we use:
- *_enabled for the sched class flags (compile time)
- *_used    for the user-space opting in (run time)

Thus, perhaps we can just use the same pattern used by the
sched_numa_balancing static key:

  $ git grep sched_numa_balancing
  kernel/sched/core.c:DEFINE_STATIC_KEY_FALSE(sched_numa_balancing);
  kernel/sched/core.c:            static_branch_enable(&sched_numa_balancing);
  kernel/sched/core.c:            static_branch_disable(&sched_numa_balancing);
  kernel/sched/core.c:    int state = static_branch_likely(&sched_numa_balancing);
  kernel/sched/fair.c:    if (!static_branch_likely(&sched_numa_balancing))
  kernel/sched/fair.c:    if (!static_branch_likely(&sched_numa_balancing))
  kernel/sched/fair.c:    if (!static_branch_likely(&sched_numa_balancing))
  kernel/sched/fair.c:    if (static_branch_unlikely(&sched_numa_balancing))
  kernel/sched/sched.h:extern struct static_key_false sched_numa_balancing;

IOW: unconditionally define sched_uclamp_used as non static in core.c,
and use it directly on schedutil too.

>  #else /* CONFIG_UCLAMP_TASK */
>  static inline
>  unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util,
> @@ -2378,6 +2408,11 @@ unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util,
>  {
>  	return util;
>  }
> +
> +static inline bool uclamp_is_enabled(void)
> +{
> +	return false;
> +}
>  #endif /* CONFIG_UCLAMP_TASK */
>  
>  #ifdef arch_scale_freq_capacity

Best,
Patrick

Re: [PATCH v5 2/2] sched/uclamp: Protect uclamp fast path code with static key

[Valentin Schneider]

On 30/06/20 09:11, Patrick Bellasi wrote:
> Hi Qais,
> here are some more 2c from me...
>
> On Mon, Jun 29, 2020 at 18:26:33 +0200, Qais Yousef <qais.yousef@arm.com> wrote...
>
> [...]
>
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index 235b2cae00a0..8d80d6091d86 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -794,6 +794,26 @@ unsigned int sysctl_sched_uclamp_util_max = SCHED_CAPACITY_SCALE;
>>  /* All clamps are required to be less or equal than these values */
>>  static struct uclamp_se uclamp_default[UCLAMP_CNT];
>>
>> +/*
>> + * This static key is used to reduce the uclamp overhead in the fast path. It
>> + * primarily disables the call to uclamp_rq_{inc, dec}() in
>> + * enqueue/dequeue_task().
>> + *
>> + * This allows users to continue to enable uclamp in their kernel config with
>> + * minimum uclamp overhead in the fast path.
>> + *
>> + * As soon as userspace modifies any of the uclamp knobs, the static key is
>> + * enabled, since we have an actual users that make use of uclamp
>> + * functionality.
>> + *
>> + * The knobs that would enable this static key are:
>> + *
>> + *   * A task modifying its uclamp value with sched_setattr().
>> + *   * An admin modifying the sysctl_sched_uclamp_{min, max} via procfs.
>> + *   * An admin modifying the cgroup cpu.uclamp.{min, max}
>
> I guess this list can be obtained with a grep or git changelog, moreover
> this text will require maintenance.
>
> What about replacing this full comment with something shorted like:
>
> ---8<---
>       Static key to reduce uclamp overhead in the fast path by disabling
>       calls to uclamp_rq_{inc, dec}().
> ---8<---
>

Having some sense of when that key gets flipped is worthwhile IMO; though
it may not have to be exhaustive list.

>> + */
>> +DEFINE_STATIC_KEY_FALSE(sched_uclamp_used);
>> +
>>  /* Integer rounded range for each bucket */
>>  #define UCLAMP_BUCKET_DELTA DIV_ROUND_CLOSEST(SCHED_CAPACITY_SCALE, UCLAMP_BUCKETS)
>>
[...]
>> @@ -1032,6 +1073,13 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
>>  {
>>      enum uclamp_id clamp_id;
>>
>> +	/*
>> +	 * Avoid any overhead until uclamp is actually used by the userspace.
>> +	 * Including the branch if we use static_branch_likely()
>
> I still find this last sentence hard to parse, but perhaps it's just me
> still missing a breakfast :)
>

AIUI this tries to explain why we use 'unlikely' rather than 'likely' -
it's to prevent !uclamp users from having the branch overhead (see
include/linux/jump_label.h, there's a nice fat comment just above
static_branch_likely()).

IMO that point is already covered by the first sentence, as I blabbered on
some previous version.

>> +	 */
>> +	if (!static_branch_unlikely(&sched_uclamp_used))
>> +		return;
>
> I'm also still wondering if the optimization is still working when we
> have that ! in front.
>
> Had a check at:
>
>    https://elixir.bootlin.com/linux/latest/source/include/linux/jump_label.h#L399
>
> and AFAIU, it all boils down to cook a __branch_check()'s compiler hint,
> and ISTR that those are "anti-patterns"?
>
> That said we do have some usages for this pattern too:
>
> $ git grep '!static_branch_unlikely' | wc -l       36
> $ git grep 'static_branch_unlikely' | wc -l       220
>
> ?
>

We use it for e.g. the sched_asym_cpucapacity key, and that works (and I've
been down into the asm a few times).

>> +
>>      if (unlikely(!p->sched_class->uclamp_enabled))
>>              return;
>>
>
> [...]
>
>> +/**
>> + * uclamp_rq_util_with - clamp @util with @rq and @p effective uclamp values.
>> + * @rq:		The rq to clamp against. Must not be NULL.
>> + * @util:	The util value to clamp.
>> + * @p:		The task to clamp against. Can be NULL if you want to clamp
>> + *		against @rq only.
>> + *
>> + * Clamps the passed @util to the max(@rq, @p) effective uclamp values.
>> + *
>> + * If sched_uclamp_used static key is disabled, then just return the util
>> + * without any clamping since uclamp aggregation at the rq level in the fast
>> + * path is disabled, rendering this operation a NOP.
>> + *
>> + * Use uclamp_eff_value() if you don't care about uclamp values at rq level. It
>> + * will return the correct effective uclamp value of the task even if the
>> + * static key is disabled.
>
> Well, if you don't care about rq, you don't call a uclamp_rq_* method.
>
> I would say that the above paragraph is redundant, moreover it adds some
> cross-reference to a different method (name) which required maintenance.
>
> What about removing it?
>
>> + */
>>  static __always_inline
>>  unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util,
>>                                struct task_struct *p)
>>  {
>> -	unsigned long min_util = READ_ONCE(rq->uclamp[UCLAMP_MIN].value);
>> -	unsigned long max_util = READ_ONCE(rq->uclamp[UCLAMP_MAX].value);
>> +	unsigned long min_util;
>> +	unsigned long max_util;
>> +
>> +	if (!static_branch_likely(&sched_uclamp_used))
>> +		return util;
>> +
>> +	min_util = READ_ONCE(rq->uclamp[UCLAMP_MIN].value);
>> +	max_util = READ_ONCE(rq->uclamp[UCLAMP_MAX].value);
>
> I think moving the initialization is not required, the compiler should
> be smart enough to place theme where's better.
>

Not so sure with the READ_ONCE() & the volatile underneath; a quick
compiler test with a volatile read before a branch tells me we still do the
read before the branch, even if the value is only used after the branch.

>>      if (p) {
>>              min_util = max(min_util, uclamp_eff_value(p, UCLAMP_MIN));
>> @@ -2371,6 +2396,11 @@ unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util,
>>
>>      return clamp(util, min_util, max_util);
>>  }
>> +
>> +static inline bool uclamp_is_enabled(void)
>> +{
>> +	return static_branch_likely(&sched_uclamp_used);
>> +}
>
> Looks like here we mix up terms, which can be confusing.
> AFAIKS, we use:
> - *_enabled for the sched class flags (compile time)
> - *_used    for the user-space opting in (run time)
>
> Thus, perhaps we can just use the same pattern used by the
> sched_numa_balancing static key:
>
>   $ git grep sched_numa_balancing
>   kernel/sched/core.c:DEFINE_STATIC_KEY_FALSE(sched_numa_balancing);
>   kernel/sched/core.c:            static_branch_enable(&sched_numa_balancing);
>   kernel/sched/core.c:            static_branch_disable(&sched_numa_balancing);
>   kernel/sched/core.c:    int state = static_branch_likely(&sched_numa_balancing);
>   kernel/sched/fair.c:    if (!static_branch_likely(&sched_numa_balancing))
>   kernel/sched/fair.c:    if (!static_branch_likely(&sched_numa_balancing))
>   kernel/sched/fair.c:    if (!static_branch_likely(&sched_numa_balancing))
>   kernel/sched/fair.c:    if (static_branch_unlikely(&sched_numa_balancing))
>   kernel/sched/sched.h:extern struct static_key_false sched_numa_balancing;
>
> IOW: unconditionally define sched_uclamp_used as non static in core.c,
> and use it directly on schedutil too.
>
>>  #else /* CONFIG_UCLAMP_TASK */
>>  static inline
>>  unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util,
>> @@ -2378,6 +2408,11 @@ unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util,
>>  {
>>      return util;
>>  }
>> +
>> +static inline bool uclamp_is_enabled(void)
>> +{
>> +	return false;
>> +}
>>  #endif /* CONFIG_UCLAMP_TASK */
>>
>>  #ifdef arch_scale_freq_capacity
>
> Best,
> Patrick

Re: [PATCH v5 2/2] sched/uclamp: Protect uclamp fast path code with static key

[Qais Yousef]

Hi Patrick

On 06/30/20 10:11, Patrick Bellasi wrote:
> 
> Hi Qais,
> here are some more 2c from me...
> 
> On Mon, Jun 29, 2020 at 18:26:33 +0200, Qais Yousef <qais.yousef@arm.com> wrote...
> 
> [...]
> 
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 235b2cae00a0..8d80d6091d86 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -794,6 +794,26 @@ unsigned int sysctl_sched_uclamp_util_max = SCHED_CAPACITY_SCALE;
> >  /* All clamps are required to be less or equal than these values */
> >  static struct uclamp_se uclamp_default[UCLAMP_CNT];
> >  
> > +/*
> > + * This static key is used to reduce the uclamp overhead in the fast path. It
> > + * primarily disables the call to uclamp_rq_{inc, dec}() in
> > + * enqueue/dequeue_task().
> > + *
> > + * This allows users to continue to enable uclamp in their kernel config with
> > + * minimum uclamp overhead in the fast path.
> > + *
> > + * As soon as userspace modifies any of the uclamp knobs, the static key is
> > + * enabled, since we have an actual users that make use of uclamp
> > + * functionality.
> > + *
> > + * The knobs that would enable this static key are:
> > + *
> > + *   * A task modifying its uclamp value with sched_setattr().
> > + *   * An admin modifying the sysctl_sched_uclamp_{min, max} via procfs.
> > + *   * An admin modifying the cgroup cpu.uclamp.{min, max}
> 
> I guess this list can be obtained with a grep or git changelog, moreover
> this text will require maintenance.
> 
> What about replacing this full comment with something shorted like:
> 
> ---8<---
>       Static key to reduce uclamp overhead in the fast path by disabling
>       calls to uclamp_rq_{inc, dec}().
> ---8<---

If you don't mind, I rather more verbose info. As a relatively new comer, lack
of comments about expectation of some functions is still a challenge.

> 
> > + */
> > +DEFINE_STATIC_KEY_FALSE(sched_uclamp_used);
> > +
> >  /* Integer rounded range for each bucket */
> >  #define UCLAMP_BUCKET_DELTA DIV_ROUND_CLOSEST(SCHED_CAPACITY_SCALE, UCLAMP_BUCKETS)
> >  
> > @@ -994,9 +1014,30 @@ static inline void uclamp_rq_dec_id(struct rq *rq, struct task_struct *p,
> >  	lockdep_assert_held(&rq->lock);
> >  
> >  	bucket = &uc_rq->bucket[uc_se->bucket_id];
> > -	SCHED_WARN_ON(!bucket->tasks);
> > +
> > +	/*
> > +	 * bucket->tasks could be zero if sched_uclamp_used was enabled while
> > +	 * the current task was running, hence we could end up with unbalanced
> > +	 * call to uclamp_rq_dec_id().
> > +	 *
> > +	 * Need to be careful of the following enqeueue/dequeue order
> > +	 * problem too
> > +	 *
> > +	 *	enqueue(taskA)
> > +	 *	// sched_uclamp_used gets enabled
> > +	 *	enqueue(taskB)
> > +	 *	dequeue(taskA)
> > +	 *	// bucket->tasks is now 0
> > +	 *	dequeue(taskB)
> > +	 *
> > +	 * where we could end up with uc_se->active of the task set to true and
> > +	 * the wrong bucket[uc_se->bucket_id].value.
> > +	 *
> > +	 * Hence always make sure we reset things properly.
> > +	 */
> >  	if (likely(bucket->tasks))
> >  		bucket->tasks--;
> > +
> >  	uc_se->active = false;
> 
> Better than v4, what about just using this active flag?
> 
> ---8<---
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 8f360326861e..465a7645713b 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -990,6 +990,13 @@ static inline void uclamp_rq_dec_id(struct rq *rq, struct task_struct *p,
>  
>         lockdep_assert_held(&rq->lock);
>  
> +       /*
> +        * If a task was already enqueue at uclamp enable time
> +        * nothing has been accounted for it.
> +        */
> +       if (unlikely(!uc_se->active))
> +               return;
> +
>         bucket = &uc_rq->bucket[uc_se->bucket_id];
>         SCHED_WARN_ON(!bucket->tasks);
>         if (likely(bucket->tasks))
> ---8<---
> 
> This will allow also to keep in all the ref count checks we have,
> e.g. the SChed_WARN_ON().

Works for me. Though I'd like to expand on the comment more just because there
were few things that were caught out and worth documenting IMO.

> 
> 
> >  	/*
> > @@ -1032,6 +1073,13 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
> >  {
> >  	enum uclamp_id clamp_id;
> >  
> > +	/*
> > +	 * Avoid any overhead until uclamp is actually used by the userspace.
> > +	 * Including the branch if we use static_branch_likely()
> 
> I still find this last sentence hard to parse, but perhaps it's just me
> still missing a breakfast :)

It used to be

	 * Including the JMP if we use static_branch_likely()

Note s/branch/JMP/

Effectively the condition is written such that we produce a NOP when uclamp is
not used. I'll rephrase.

> 
> > +	 */
> > +	if (!static_branch_unlikely(&sched_uclamp_used))
> > +		return;
> 
> I'm also still wondering if the optimization is still working when we
> have that ! in front.

It does. I looked at the generated code before posting.

> 
> Had a check at:
> 
>    https://elixir.bootlin.com/linux/latest/source/include/linux/jump_label.h#L399
> 
> and AFAIU, it all boils down to cook a __branch_check()'s compiler hint,
> and ISTR that those are "anti-patterns"?
> 
> That said we do have some usages for this pattern too:
> 
> $ git grep '!static_branch_unlikely' | wc -l       36
> $ git grep 'static_branch_unlikely' | wc -l       220
> 
> ?
> 
> > +
> >  	if (unlikely(!p->sched_class->uclamp_enabled))
> >  		return;
> >  
> 
> [...]
> 
> > +/**
> > + * uclamp_rq_util_with - clamp @util with @rq and @p effective uclamp values.
> > + * @rq:		The rq to clamp against. Must not be NULL.
> > + * @util:	The util value to clamp.
> > + * @p:		The task to clamp against. Can be NULL if you want to clamp
> > + *		against @rq only.
> > + *
> > + * Clamps the passed @util to the max(@rq, @p) effective uclamp values.
> > + *
> > + * If sched_uclamp_used static key is disabled, then just return the util
> > + * without any clamping since uclamp aggregation at the rq level in the fast
> > + * path is disabled, rendering this operation a NOP.
> > + *
> > + * Use uclamp_eff_value() if you don't care about uclamp values at rq level. It
> > + * will return the correct effective uclamp value of the task even if the
> > + * static key is disabled.
> 
> Well, if you don't care about rq, you don't call a uclamp_rq_* method.
> 
> I would say that the above paragraph is redundant, moreover it adds some
> cross-reference to a different method (name) which required maintenance.
> 
> What about removing it?

I'd rather keep this one too. It helps explaining what the expected way to use
this code. I don't think the maintenance is a big issue? We have to maintain
the code anyway?

> 
> > + */
> >  static __always_inline
> >  unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util,
> >  				  struct task_struct *p)
> >  {
> > -	unsigned long min_util = READ_ONCE(rq->uclamp[UCLAMP_MIN].value);
> > -	unsigned long max_util = READ_ONCE(rq->uclamp[UCLAMP_MAX].value);
> > +	unsigned long min_util;
> > +	unsigned long max_util;
> > +
> > +	if (!static_branch_likely(&sched_uclamp_used))
> > +		return util;
> > +
> > +	min_util = READ_ONCE(rq->uclamp[UCLAMP_MIN].value);
> > +	max_util = READ_ONCE(rq->uclamp[UCLAMP_MAX].value);
> 
> I think moving the initialization is not required, the compiler should
> be smart enough to place theme where's better.

I did look at the generated code before posting. The compiler doesn't optimize
the reads. Likely because of the READ_ONCE() which implies volatile access.

> 
> >  	if (p) {
> >  		min_util = max(min_util, uclamp_eff_value(p, UCLAMP_MIN));
> > @@ -2371,6 +2396,11 @@ unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util,
> >  
> >  	return clamp(util, min_util, max_util);
> >  }
> > +
> > +static inline bool uclamp_is_enabled(void)
> > +{
> > +	return static_branch_likely(&sched_uclamp_used);
> > +}
> 
> Looks like here we mix up terms, which can be confusing.
> AFAIKS, we use:
> - *_enabled for the sched class flags (compile time)
> - *_used    for the user-space opting in (run time)

I wanted to add a comment here.

I can rename it to uclamp_is_used() if you want.

Thanks

--
Qais Yousef

> 
> Thus, perhaps we can just use the same pattern used by the
> sched_numa_balancing static key:
> 
>   $ git grep sched_numa_balancing
>   kernel/sched/core.c:DEFINE_STATIC_KEY_FALSE(sched_numa_balancing);
>   kernel/sched/core.c:            static_branch_enable(&sched_numa_balancing);
>   kernel/sched/core.c:            static_branch_disable(&sched_numa_balancing);
>   kernel/sched/core.c:    int state = static_branch_likely(&sched_numa_balancing);
>   kernel/sched/fair.c:    if (!static_branch_likely(&sched_numa_balancing))
>   kernel/sched/fair.c:    if (!static_branch_likely(&sched_numa_balancing))
>   kernel/sched/fair.c:    if (!static_branch_likely(&sched_numa_balancing))
>   kernel/sched/fair.c:    if (static_branch_unlikely(&sched_numa_balancing))
>   kernel/sched/sched.h:extern struct static_key_false sched_numa_balancing;
> 
> IOW: unconditionally define sched_uclamp_used as non static in core.c,
> and use it directly on schedutil too.
> 
> >  #else /* CONFIG_UCLAMP_TASK */
> >  static inline
> >  unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util,
> > @@ -2378,6 +2408,11 @@ unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util,
> >  {
> >  	return util;
> >  }
> > +
> > +static inline bool uclamp_is_enabled(void)
> > +{
> > +	return false;
> > +}
> >  #endif /* CONFIG_UCLAMP_TASK */
> >  
> >  #ifdef arch_scale_freq_capacity
> 
> Best,
> Patrick

Re: [PATCH v5 2/2] sched/uclamp: Protect uclamp fast path code with static key

[Patrick Bellasi]

Hi Qais,
sorry for commenting on v5 with a v6 already posted, but...
... I cannot keep up with your re-spinning rate ;)

More importantly, perhaps you missed to comment on one of my previous
points.

Will have a better look at the rest of v6 later today.

Cheers,
Patrick

On Tue, Jun 30, 2020 at 11:46:24 +0200, Qais Yousef <qais.yousef@arm.com> wrote...
> On 06/30/20 10:11, Patrick Bellasi wrote:
>> On Mon, Jun 29, 2020 at 18:26:33 +0200, Qais Yousef <qais.yousef@arm.com> wrote...

[...]

>> > +
>> > +static inline bool uclamp_is_enabled(void)
>> > +{
>> > +	return static_branch_likely(&sched_uclamp_used);
>> > +}
>> 
>> Looks like here we mix up terms, which can be confusing.
>> AFAIKS, we use:
>> - *_enabled for the sched class flags (compile time)
>> - *_used    for the user-space opting in (run time)
>
> I wanted to add a comment here.
>
> I can rename it to uclamp_is_used() if you want.

In my previous message I was mostly asking about this:

>> Thus, perhaps we can just use the same pattern used by the
>> sched_numa_balancing static key:
>> 
>>   $ git grep sched_numa_balancing
>>   kernel/sched/core.c:DEFINE_STATIC_KEY_FALSE(sched_numa_balancing);
>>   kernel/sched/core.c:            static_branch_enable(&sched_numa_balancing);
>>   kernel/sched/core.c:            static_branch_disable(&sched_numa_balancing);
>>   kernel/sched/core.c:    int state = static_branch_likely(&sched_numa_balancing);
>>   kernel/sched/fair.c:    if (!static_branch_likely(&sched_numa_balancing))
>>   kernel/sched/fair.c:    if (!static_branch_likely(&sched_numa_balancing))
>>   kernel/sched/fair.c:    if (!static_branch_likely(&sched_numa_balancing))
>>   kernel/sched/fair.c:    if (static_branch_unlikely(&sched_numa_balancing))
>>   kernel/sched/sched.h:extern struct static_key_false sched_numa_balancing;
>> 
>> IOW: unconditionally define sched_uclamp_used as non static in core.c,
>> and use it directly on schedutil too.

So, what about this instead of adding the (renamed) method above?

Re: [PATCH v5 2/2] sched/uclamp: Protect uclamp fast path code with static key

[Qais Yousef]

Hi Patrick

On 06/30/20 16:55, Patrick Bellasi wrote:
> 
> Hi Qais,
> sorry for commenting on v5 with a v6 already posted, but...
> ... I cannot keep up with your re-spinning rate ;)

I classified that as a nit really and doesn't affect correctness. We have
different subjective view on what is better here. I did all the work in the
past 2 weeks and I think as the author of this patch I have the right to keep
my preference on subjective matters. I did consider your feedback and didn't
ignore it and improved the naming and added a comment to make sure there's no
confusion.

We could nitpick the best name forever, but is it really that important?

I really don't see any added value for one approach or another here to start
a long debate about it.

The comments were small enough that I didn't see any controversy that
warrants holding the patches longer. I agreed with your proposal to use
uc_se->active and clarified why your other suggestions don't hold.

You pointed that uclamp_is_enabled() confused you; and I responded that I'll
change the name. Sorry for not being explicit about answering the below, but
I thought my answer implied that I don't prefer it.

> 
> >> Thus, perhaps we can just use the same pattern used by the
> >> sched_numa_balancing static key:
> >> 
> >>   $ git grep sched_numa_balancing
> >>   kernel/sched/core.c:DEFINE_STATIC_KEY_FALSE(sched_numa_balancing);
> >>   kernel/sched/core.c:            static_branch_enable(&sched_numa_balancing);
> >>   kernel/sched/core.c:            static_branch_disable(&sched_numa_balancing);
> >>   kernel/sched/core.c:    int state = static_branch_likely(&sched_numa_balancing);
> >>   kernel/sched/fair.c:    if (!static_branch_likely(&sched_numa_balancing))
> >>   kernel/sched/fair.c:    if (!static_branch_likely(&sched_numa_balancing))
> >>   kernel/sched/fair.c:    if (!static_branch_likely(&sched_numa_balancing))
> >>   kernel/sched/fair.c:    if (static_branch_unlikely(&sched_numa_balancing))
> >>   kernel/sched/sched.h:extern struct static_key_false sched_numa_balancing;
> >> 
> >> IOW: unconditionally define sched_uclamp_used as non static in core.c,
> >> and use it directly on schedutil too.
> 
> So, what about this instead of adding the (renamed) method above?

I am sorry there's no written rule that says one should do it in a specific
way. And AFAIK both way are implemented in the kernel. I appreciate your
suggestion but as the person who did all the hard work, I think my preference
matters here too.

And actually with my approach when uclamp is not compiled in there's no need to
define an extra variable; and since uclamp_is_used() is defined as false for
!CONFIG_UCLAMP_TASK, it'll help with DCE, so less likely to end up with dead
code that'll never run in the final binary.

Thanks a lot for all of your comments and feedback anyway!

--
Qais Yousef

Re: [PATCH v5 2/2] sched/uclamp: Protect uclamp fast path code with static key

[Patrick Bellasi]

On Tue, Jun 30, 2020 at 17:40:34 +0200, Qais Yousef <qais.yousef@arm.com> wrote...

> Hi Patrick
>
> On 06/30/20 16:55, Patrick Bellasi wrote:
>> 
>> Hi Qais,
>> sorry for commenting on v5 with a v6 already posted, but...
>> ... I cannot keep up with your re-spinning rate ;)
>
> I classified that as a nit really and doesn't affect correctness. We have
> different subjective view on what is better here. I did all the work in the
> past 2 weeks and I think as the author of this patch I have the right to keep
> my preference on subjective matters. I did consider your feedback and didn't
> ignore it and improved the naming and added a comment to make sure there's no
> confusion.
>
> We could nitpick the best name forever, but is it really that important?

Which leans toward confirming the impression I had while reading your
previous response, i.e. you stopped reading at the name change
observation, which would be _just_ a nit-picking, although still worth
IMHO.

Instead, I went further and asked you to consider a different approach:
not adding a new kernel symbol to represent a concept already there.

> I really don't see any added value for one approach or another here to start
> a long debate about it.

Then you could have just called out that instead of silently ignoring
the comment/proposal.

> The comments were small enough that I didn't see any controversy that
> warrants holding the patches longer. I agreed with your proposal to use
> uc_se->active and clarified why your other suggestions don't hold.
>
> You pointed that uclamp_is_enabled() confused you; and I responded that I'll
> change the name.

Perhaps it would not confuse only me having 'something_enabled()'
referring to 'something_used'.

> Sorry for not being explicit about answering the below, but
> I thought my answer implied that I don't prefer it.

Your answer was about a name change, don't see correlation with a
different approach... but should be just me.

>> >> Thus, perhaps we can just use the same pattern used by the
>> >> sched_numa_balancing static key:
>> >> 
>> >>   $ git grep sched_numa_balancing
>> >>   kernel/sched/core.c:DEFINE_STATIC_KEY_FALSE(sched_numa_balancing);
>> >>   kernel/sched/core.c:            static_branch_enable(&sched_numa_balancing);
>> >>   kernel/sched/core.c:            static_branch_disable(&sched_numa_balancing);
>> >>   kernel/sched/core.c:    int state = static_branch_likely(&sched_numa_balancing);
>> >>   kernel/sched/fair.c:    if (!static_branch_likely(&sched_numa_balancing))
>> >>   kernel/sched/fair.c:    if (!static_branch_likely(&sched_numa_balancing))
>> >>   kernel/sched/fair.c:    if (!static_branch_likely(&sched_numa_balancing))
>> >>   kernel/sched/fair.c:    if (static_branch_unlikely(&sched_numa_balancing))
>> >>   kernel/sched/sched.h:extern struct static_key_false sched_numa_balancing;
>> >> 
>> >> IOW: unconditionally define sched_uclamp_used as non static in core.c,
>> >> and use it directly on schedutil too.
>> 
>> So, what about this instead of adding the (renamed) method above?
>
> I am sorry there's no written rule that says one should do it in a specific
> way. And AFAIK both way are implemented in the kernel. I appreciate your
> suggestion but as the person who did all the hard work, I think my preference
> matters here too.

You sure know that sometime reviewing code can be an "hard work" too, so I
would not go down that way at all with the discussion. Quite likely I
have a different "subjective" view on how Open Source development works.

> And actually with my approach when uclamp is not compiled in there's no need to
> define an extra variable; and since uclamp_is_used() is defined as false for
> !CONFIG_UCLAMP_TASK, it'll help with DCE, so less likely to end up with dead
> code that'll never run in the final binary.

Good, this is the simple and small reply I've politely asked for.

Best,
Patrick

Re: [PATCH v5 2/2] sched/uclamp: Protect uclamp fast path code with static key

[Qais Yousef]

On 06/30/20 19:44, Patrick Bellasi wrote:

[...]

> > I am sorry there's no written rule that says one should do it in a specific
> > way. And AFAIK both way are implemented in the kernel. I appreciate your
> > suggestion but as the person who did all the hard work, I think my preference
> > matters here too.
> 
> You sure know that sometime reviewing code can be an "hard work" too, so I
> would not go down that way at all with the discussion. Quite likely I
> have a different "subjective" view on how Open Source development works.
> 
> > And actually with my approach when uclamp is not compiled in there's no need to
> > define an extra variable; and since uclamp_is_used() is defined as false for
> > !CONFIG_UCLAMP_TASK, it'll help with DCE, so less likely to end up with dead
> > code that'll never run in the final binary.
> 
> Good, this is the simple and small reply I've politely asked for.

I am sorry if I offended you. I took all your comments seriously and answered
them to the best of my ability. All of your comments and suggestions were
highly appreciated too. If the wrong message reached across, rest assured it
wasn't the intended one.

Thanks

--
Qais Yousef