[PATCH v7 0/9] Add latency priority for CFS class

[PATCH v7 0/9] Add latency priority for CFS class

[Vincent Guittot]

This patchset restarts the work about adding a latency priority to describe
the latency tolerance of cfs tasks.

Patch [1] is a new one that has been added with v6. It fixes an
unfairness for low prio tasks because of wakeup_gran() being bigger
than the maximum vruntime credit that a waking task can keep after
sleeping.

The patches [2-4] have been done by Parth:
https://lore.kernel.org/lkml/20200228090755.22829-1-parth@linux.ibm.com/

I have just rebased and moved the set of latency priority outside the
priority update. I have removed the reviewed tag because the patches
are 2 years old.

This aims to be a generic interface and the following patches is one use
of it to improve the scheduling latency of cfs tasks.

Patch [5] uses latency nice priority to define a latency offset
and then decide if a cfs task can or should preempt the current
running task. The patch gives some tests results with cyclictests and
hackbench to highlight the benefit of latency priority for short
interactive task or long intensive tasks.

Patch [6] adds the support of latency nice priority to task group by
adding a cpu.latency.nice field. The range is [-20:19] as for setting task
latency priority.

Patch [7] makes sched_core taking into account the latency offset.

Patch [8] adds a rb tree to cover some corner cases where the latency
sensitive task (priority < 0) is preempted by high priority task (RT/DL)
or fails to preempt them. This patch ensures that tasks will have at least
a slice of sched_min_granularity in priority at wakeup.

Patch [9] removes useless check after adding a latency rb tree.

I have also backported the patchset on a dragonboard RB3 with an android
mainline kernel based on v5.18 for a quick test. I have used the
TouchLatency app which is part of AOSP and described to be a very good
test to highlight jitter and jank frame sources of a system [1].
In addition to the app, I have added some short running tasks waking-up
regularly (to use the 8 cpus for 4 ms every 37777us) to stress the system
without overloading it (and disabling EAS). The 1st results shows that the
patchset helps to reduce the missed deadline frames from 5% to less than
0.1% when the cpu.latency.nice of task group are set. I haven't rerun the
test with latest version.

I have also tested the patchset with the modified version of the alsa
latency test that has been shared by Tim. The test quickly xruns with
default latency nice priority 0 but is able to run without underuns with
a latency -20 and hackbench running simultaneously.

[1] https://source.android.com/docs/core/debug/eval_perf#touchlatency

Change since v6:
- Fix compilation error for !CONFIG_SCHED_DEBUG

Change since v5:
- Add patch 1 to fix unfairness for low prio task. This has been
  discovered while studying Youssef's tests results with latency nice
  which were hitting the same problem.
- Fixed latency_offset computation to take into account
  GENTLE_FAIR_SLEEPERS. This has diseappeared with v2and has been raised
  by Youssef's tests.
- Reworked and optimized how latency_offset in used to check for
  preempting current task at wakeup and tick. This cover more cases too.
- Add patch 9 to remove check_preempt_from_others() which is not needed
  anymore with the rb tree.

Change since v4:
- Removed permission checks to set latency priority. This enables user
  without elevated privilege like audio application to set their latency
  priority as requested by Tim.
- Removed cpu.latency and replaced it by cpu.latency.nice so we keep a
  generic interface not tied to latency_offset which can be used to
  implement other latency features.
- Added an entry in Documentation/admin-guide/cgroup-v2.rst to describe
  cpu.latency.nice.
- Fix some typos.

Change since v3:
- Fix 2 compilation warnings raised by kernel test robot <lkp@intel.com>

Change since v2:
- Set a latency_offset field instead of saving a weight and computing it
  on the fly.
- Make latency_offset available for task group: cpu.latency
- Fix some corner cases to make latency sensitive tasks schedule first and
  add a rb tree for latency sensitive task.

Change since v1:
- fix typo
- move some codes in the right patch to make bisect happy
- simplify and fixed how the weight is computed
- added support of sched core patch 7

Parth Shah (3):
  sched: Introduce latency-nice as a per-task attribute
  sched/core: Propagate parent task's latency requirements to the child
    task
  sched: Allow sched_{get,set}attr to change latency_nice of the task

Vincent Guittot (6):
  sched/fair: fix unfairness at wakeup
  sched/fair: Take into account latency priority at wakeup
  sched/fair: Add sched group latency support
  sched/core: Support latency priority with sched core
  sched/fair: Add latency list
  sched/fair: remove check_preempt_from_others

 Documentation/admin-guide/cgroup-v2.rst |   8 +
 include/linux/sched.h                   |   5 +
 include/uapi/linux/sched.h              |   4 +-
 include/uapi/linux/sched/types.h        |  19 +++
 init/init_task.c                        |   1 +
 kernel/sched/core.c                     | 105 ++++++++++++
 kernel/sched/debug.c                    |   1 +
 kernel/sched/fair.c                     | 210 ++++++++++++++++++++----
 kernel/sched/sched.h                    |  65 +++++++-
 tools/include/uapi/linux/sched.h        |   4 +-
 10 files changed, 386 insertions(+), 36 deletions(-)

-- 
2.17.1

[PATCH v7 1/9] sched/fair: fix unfairness at wakeup

[Vincent Guittot]

At wake up, the vruntime of a task is updated to not be more older than
a sched_latency period behind the min_vruntime. This prevents long sleeping
task to get unlimited credit at wakeup.
Such waking task should preempt current one to use its CPU bandwidth but
wakeup_gran() can be larger than sched_latency, filter out the
wakeup preemption and as a results steals some CPU bandwidth to
the waking task.

Make sure that a task, which vruntime has been capped, will preempt current
task and use its CPU bandwidth even if wakeup_gran() is in the same range
as sched_latency.

If the waking task failed to preempt current it could to wait up to
sysctl_sched_min_granularity before preempting it during next tick.

Strictly speaking, we should use cfs->min_vruntime instead of
curr->vruntime but it doesn't worth the additional overhead and complexity
as the vruntime of current should be close to min_vruntime if not equal.

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 kernel/sched/fair.c  | 46 ++++++++++++++++++++------------------------
 kernel/sched/sched.h | 30 ++++++++++++++++++++++++++++-
 2 files changed, 50 insertions(+), 26 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 5ffec4370602..eb04c83112a0 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4345,33 +4345,17 @@ place_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int initial)
 {
 	u64 vruntime = cfs_rq->min_vruntime;
 
-	/*
-	 * The 'current' period is already promised to the current tasks,
-	 * however the extra weight of the new task will slow them down a
-	 * little, place the new task so that it fits in the slot that
-	 * stays open at the end.
-	 */
-	if (initial && sched_feat(START_DEBIT))
-		vruntime += sched_vslice(cfs_rq, se);
-
-	/* sleeps up to a single latency don't count. */
-	if (!initial) {
-		unsigned long thresh;
-
-		if (se_is_idle(se))
-			thresh = sysctl_sched_min_granularity;
-		else
-			thresh = sysctl_sched_latency;
-
+	if (!initial)
+		/* sleeps up to a single latency don't count. */
+		vruntime -= get_sched_latency(se_is_idle(se));
+	else if (sched_feat(START_DEBIT))
 		/*
-		 * Halve their sleep time's effect, to allow
-		 * for a gentler effect of sleepers:
+		 * The 'current' period is already promised to the current tasks,
+		 * however the extra weight of the new task will slow them down a
+		 * little, place the new task so that it fits in the slot that
+		 * stays open at the end.
 		 */
-		if (sched_feat(GENTLE_FAIR_SLEEPERS))
-			thresh >>= 1;
-
-		vruntime -= thresh;
-	}
+		vruntime += sched_vslice(cfs_rq, se);
 
 	/* ensure we never gain time by being placed backwards. */
 	se->vruntime = max_vruntime(se->vruntime, vruntime);
@@ -7187,6 +7171,18 @@ wakeup_preempt_entity(struct sched_entity *curr, struct sched_entity *se)
 		return -1;
 
 	gran = wakeup_gran(se);
+
+	/*
+	 * At wake up, the vruntime of a task is capped to not be older than
+	 * a sched_latency period compared to min_vruntime. This prevents long
+	 * sleeping task to get unlimited credit at wakeup. Such waking up task
+	 * has to preempt current in order to not lose its share of CPU
+	 * bandwidth but wakeup_gran() can become higher than scheduling period
+	 * for low priority task. Make sure that long sleeping task will get a
+	 * chance to preempt current.
+	 */
+	gran = min_t(s64, gran, get_latency_max());
+
 	if (vdiff > gran)
 		return 1;
 
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 1fc198be1ffd..14879d429919 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -2432,9 +2432,9 @@ extern void check_preempt_curr(struct rq *rq, struct task_struct *p, int flags);
 extern const_debug unsigned int sysctl_sched_nr_migrate;
 extern const_debug unsigned int sysctl_sched_migration_cost;
 
-#ifdef CONFIG_SCHED_DEBUG
 extern unsigned int sysctl_sched_latency;
 extern unsigned int sysctl_sched_min_granularity;
+#ifdef CONFIG_SCHED_DEBUG
 extern unsigned int sysctl_sched_idle_min_granularity;
 extern unsigned int sysctl_sched_wakeup_granularity;
 extern int sysctl_resched_latency_warn_ms;
@@ -2448,6 +2448,34 @@ extern unsigned int sysctl_numa_balancing_scan_period_max;
 extern unsigned int sysctl_numa_balancing_scan_size;
 #endif
 
+static inline unsigned long  get_sched_latency(bool idle)
+{
+	unsigned long thresh;
+
+	if (idle)
+		thresh = sysctl_sched_min_granularity;
+	else
+		thresh = sysctl_sched_latency;
+
+	/*
+	 * Halve their sleep time's effect, to allow
+	 * for a gentler effect of sleepers:
+	 */
+	if (sched_feat(GENTLE_FAIR_SLEEPERS))
+		thresh >>= 1;
+
+	return thresh;
+}
+
+static inline unsigned long  get_latency_max(void)
+{
+	unsigned long thresh = get_sched_latency(false);
+
+	thresh -= sysctl_sched_min_granularity;
+
+	return thresh;
+}
+
 #ifdef CONFIG_SCHED_HRTICK
 
 /*
-- 
2.17.1

[PATCH v7 2/9] sched: Introduce latency-nice as a per-task attribute

[Vincent Guittot]

From: Parth Shah <parth@linux.ibm.com>

Latency-nice indicates the latency requirements of a task with respect
to the other tasks in the system. The value of the attribute can be within
the range of [-20, 19] both inclusive to be in-line with the values just
like task nice values.

latency_nice = -20 indicates the task to have the least latency as
compared to the tasks having latency_nice = +19.

The latency_nice may affect only the CFS SCHED_CLASS by getting
latency requirements from the userspace.

Additionally, add debugging bits for newly added latency_nice attribute.

Signed-off-by: Parth Shah <parth@linux.ibm.com>
[rebase]
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 include/linux/sched.h |  1 +
 kernel/sched/debug.c  |  1 +
 kernel/sched/sched.h  | 18 ++++++++++++++++++
 3 files changed, 20 insertions(+)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 15e3bd96e4ce..6805f378a9c3 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -783,6 +783,7 @@ struct task_struct {
 	int				static_prio;
 	int				normal_prio;
 	unsigned int			rt_priority;
+	int				latency_nice;
 
 	struct sched_entity		se;
 	struct sched_rt_entity		rt;
diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
index bb3d63bdf4ae..a3f7876217a6 100644
--- a/kernel/sched/debug.c
+++ b/kernel/sched/debug.c
@@ -1042,6 +1042,7 @@ void proc_sched_show_task(struct task_struct *p, struct pid_namespace *ns,
 #endif
 	P(policy);
 	P(prio);
+	P(latency_nice);
 	if (task_has_dl_policy(p)) {
 		P(dl.runtime);
 		P(dl.deadline);
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 14879d429919..4bf9d7777f99 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -125,6 +125,24 @@ extern int sched_rr_timeslice;
  */
 #define NS_TO_JIFFIES(TIME)	((unsigned long)(TIME) / (NSEC_PER_SEC / HZ))
 
+/*
+ * Latency nice is meant to provide scheduler hints about the relative
+ * latency requirements of a task with respect to other tasks.
+ * Thus a task with latency_nice == 19 can be hinted as the task with no
+ * latency requirements, in contrast to the task with latency_nice == -20
+ * which should be given priority in terms of lower latency.
+ */
+#define MAX_LATENCY_NICE	19
+#define MIN_LATENCY_NICE	-20
+
+#define LATENCY_NICE_WIDTH	\
+	(MAX_LATENCY_NICE - MIN_LATENCY_NICE + 1)
+
+/*
+ * Default tasks should be treated as a task with latency_nice = 0.
+ */
+#define DEFAULT_LATENCY_NICE	0
+
 /*
  * Increase resolution of nice-level calculations for 64-bit architectures.
  * The extra resolution improves shares distribution and load balancing of
-- 
2.17.1

[PATCH v7 3/9] sched/core: Propagate parent task's latency requirements to the child task

[Vincent Guittot]

From: Parth Shah <parth@linux.ibm.com>

Clone parent task's latency_nice attribute to the forked child task.

Reset the latency_nice value to default value when the child task is
set to sched_reset_on_fork.

Also, initialize init_task.latency_nice value with DEFAULT_LATENCY_NICE
value

Signed-off-by: Parth Shah <parth@linux.ibm.com>
[rebase]
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 init/init_task.c    | 1 +
 kernel/sched/core.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/init/init_task.c b/init/init_task.c
index ff6c4b9bfe6b..7dd71dd2d261 100644
--- a/init/init_task.c
+++ b/init/init_task.c
@@ -78,6 +78,7 @@ struct task_struct init_task
 	.prio		= MAX_PRIO - 20,
 	.static_prio	= MAX_PRIO - 20,
 	.normal_prio	= MAX_PRIO - 20,
+	.latency_nice	= DEFAULT_LATENCY_NICE,
 	.policy		= SCHED_NORMAL,
 	.cpus_ptr	= &init_task.cpus_mask,
 	.user_cpus_ptr	= NULL,
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 02dc1b8e3cb6..54544353025b 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4559,6 +4559,7 @@ int sched_fork(unsigned long clone_flags, struct task_struct *p)
 		p->prio = p->normal_prio = p->static_prio;
 		set_load_weight(p, false);
 
+		p->latency_nice = DEFAULT_LATENCY_NICE;
 		/*
 		 * We don't need the reset flag anymore after the fork. It has
 		 * fulfilled its duty:
-- 
2.17.1

[PATCH v7 4/9] sched: Allow sched_{get,set}attr to change latency_nice of the task

[Vincent Guittot]

From: Parth Shah <parth@linux.ibm.com>

Introduce the latency_nice attribute to sched_attr and provide a
mechanism to change the value with the use of sched_setattr/sched_getattr
syscall.

Also add new flag "SCHED_FLAG_LATENCY_NICE" to hint the change in
latency_nice of the task on every sched_setattr syscall.

Signed-off-by: Parth Shah <parth@linux.ibm.com>
[rebase and add a dedicated __setscheduler_latency ]
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 include/uapi/linux/sched.h       |  4 +++-
 include/uapi/linux/sched/types.h | 19 +++++++++++++++++++
 kernel/sched/core.c              | 24 ++++++++++++++++++++++++
 tools/include/uapi/linux/sched.h |  4 +++-
 4 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h
index 3bac0a8ceab2..b2e932c25be6 100644
--- a/include/uapi/linux/sched.h
+++ b/include/uapi/linux/sched.h
@@ -132,6 +132,7 @@ struct clone_args {
 #define SCHED_FLAG_KEEP_PARAMS		0x10
 #define SCHED_FLAG_UTIL_CLAMP_MIN	0x20
 #define SCHED_FLAG_UTIL_CLAMP_MAX	0x40
+#define SCHED_FLAG_LATENCY_NICE		0x80
 
 #define SCHED_FLAG_KEEP_ALL	(SCHED_FLAG_KEEP_POLICY | \
 				 SCHED_FLAG_KEEP_PARAMS)
@@ -143,6 +144,7 @@ struct clone_args {
 			 SCHED_FLAG_RECLAIM		| \
 			 SCHED_FLAG_DL_OVERRUN		| \
 			 SCHED_FLAG_KEEP_ALL		| \
-			 SCHED_FLAG_UTIL_CLAMP)
+			 SCHED_FLAG_UTIL_CLAMP		| \
+			 SCHED_FLAG_LATENCY_NICE)
 
 #endif /* _UAPI_LINUX_SCHED_H */
diff --git a/include/uapi/linux/sched/types.h b/include/uapi/linux/sched/types.h
index f2c4589d4dbf..db1e8199e8c8 100644
--- a/include/uapi/linux/sched/types.h
+++ b/include/uapi/linux/sched/types.h
@@ -10,6 +10,7 @@ struct sched_param {
 
 #define SCHED_ATTR_SIZE_VER0	48	/* sizeof first published struct */
 #define SCHED_ATTR_SIZE_VER1	56	/* add: util_{min,max} */
+#define SCHED_ATTR_SIZE_VER2	60	/* add: latency_nice */
 
 /*
  * Extended scheduling parameters data structure.
@@ -98,6 +99,22 @@ struct sched_param {
  * scheduled on a CPU with no more capacity than the specified value.
  *
  * A task utilization boundary can be reset by setting the attribute to -1.
+ *
+ * Latency Tolerance Attributes
+ * ===========================
+ *
+ * A subset of sched_attr attributes allows to specify the relative latency
+ * requirements of a task with respect to the other tasks running/queued in the
+ * system.
+ *
+ * @ sched_latency_nice	task's latency_nice value
+ *
+ * The latency_nice of a task can have any value in a range of
+ * [MIN_LATENCY_NICE..MAX_LATENCY_NICE].
+ *
+ * A task with latency_nice with the value of LATENCY_NICE_MIN can be
+ * taken for a task requiring a lower latency as opposed to the task with
+ * higher latency_nice.
  */
 struct sched_attr {
 	__u32 size;
@@ -120,6 +137,8 @@ struct sched_attr {
 	__u32 sched_util_min;
 	__u32 sched_util_max;
 
+	/* latency requirement hints */
+	__s32 sched_latency_nice;
 };
 
 #endif /* _UAPI_LINUX_SCHED_TYPES_H */
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 54544353025b..b2accc9da4fe 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -7318,6 +7318,14 @@ static void __setscheduler_params(struct task_struct *p,
 	p->rt_priority = attr->sched_priority;
 	p->normal_prio = normal_prio(p);
 	set_load_weight(p, true);
+
+}
+
+static void __setscheduler_latency(struct task_struct *p,
+		const struct sched_attr *attr)
+{
+	if (attr->sched_flags & SCHED_FLAG_LATENCY_NICE)
+		p->latency_nice = attr->sched_latency_nice;
 }
 
 /*
@@ -7460,6 +7468,13 @@ static int __sched_setscheduler(struct task_struct *p,
 			return retval;
 	}
 
+	if (attr->sched_flags & SCHED_FLAG_LATENCY_NICE) {
+		if (attr->sched_latency_nice > MAX_LATENCY_NICE)
+			return -EINVAL;
+		if (attr->sched_latency_nice < MIN_LATENCY_NICE)
+			return -EINVAL;
+	}
+
 	if (pi)
 		cpuset_read_lock();
 
@@ -7494,6 +7509,9 @@ static int __sched_setscheduler(struct task_struct *p,
 			goto change;
 		if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)
 			goto change;
+		if (attr->sched_flags & SCHED_FLAG_LATENCY_NICE &&
+		    attr->sched_latency_nice != p->latency_nice)
+			goto change;
 
 		p->sched_reset_on_fork = reset_on_fork;
 		retval = 0;
@@ -7582,6 +7600,7 @@ static int __sched_setscheduler(struct task_struct *p,
 		__setscheduler_params(p, attr);
 		__setscheduler_prio(p, newprio);
 	}
+	__setscheduler_latency(p, attr);
 	__setscheduler_uclamp(p, attr);
 
 	if (queued) {
@@ -7792,6 +7811,9 @@ static int sched_copy_attr(struct sched_attr __user *uattr, struct sched_attr *a
 	    size < SCHED_ATTR_SIZE_VER1)
 		return -EINVAL;
 
+	if ((attr->sched_flags & SCHED_FLAG_LATENCY_NICE) &&
+	    size < SCHED_ATTR_SIZE_VER2)
+		return -EINVAL;
 	/*
 	 * XXX: Do we want to be lenient like existing syscalls; or do we want
 	 * to be strict and return an error on out-of-bounds values?
@@ -8029,6 +8051,8 @@ SYSCALL_DEFINE4(sched_getattr, pid_t, pid, struct sched_attr __user *, uattr,
 	get_params(p, &kattr);
 	kattr.sched_flags &= SCHED_FLAG_ALL;
 
+	kattr.sched_latency_nice = p->latency_nice;
+
 #ifdef CONFIG_UCLAMP_TASK
 	/*
 	 * This could race with another potential updater, but this is fine
diff --git a/tools/include/uapi/linux/sched.h b/tools/include/uapi/linux/sched.h
index 3bac0a8ceab2..b2e932c25be6 100644
--- a/tools/include/uapi/linux/sched.h
+++ b/tools/include/uapi/linux/sched.h
@@ -132,6 +132,7 @@ struct clone_args {
 #define SCHED_FLAG_KEEP_PARAMS		0x10
 #define SCHED_FLAG_UTIL_CLAMP_MIN	0x20
 #define SCHED_FLAG_UTIL_CLAMP_MAX	0x40
+#define SCHED_FLAG_LATENCY_NICE		0x80
 
 #define SCHED_FLAG_KEEP_ALL	(SCHED_FLAG_KEEP_POLICY | \
 				 SCHED_FLAG_KEEP_PARAMS)
@@ -143,6 +144,7 @@ struct clone_args {
 			 SCHED_FLAG_RECLAIM		| \
 			 SCHED_FLAG_DL_OVERRUN		| \
 			 SCHED_FLAG_KEEP_ALL		| \
-			 SCHED_FLAG_UTIL_CLAMP)
+			 SCHED_FLAG_UTIL_CLAMP		| \
+			 SCHED_FLAG_LATENCY_NICE)
 
 #endif /* _UAPI_LINUX_SCHED_H */
-- 
2.17.1

[PATCH v7 5/9] sched/fair: Take into account latency priority at wakeup

[Vincent Guittot]

Take into account the latency priority of a thread when deciding to
preempt the current running thread. We don't want to provide more CPU
bandwidth to a thread but reorder the scheduling to run latency sensitive
task first whenever possible.

As long as a thread didn't use its bandwidth, it will be able to preempt
the current thread.

At the opposite, a thread with a low latency priority will preempt current
thread at wakeup only to keep fair CPU bandwidth sharing. Otherwise it will
wait for the tick to get its sched slice.

                                   curr vruntime
                                       |
                      sysctl_sched_wakeup_granularity
                                   <-->
----------------------------------|----|-----------------------|---------------
                                  |    |<--------------------->
                                  |    .  sysctl_sched_latency
                                  |    .
default/current latency entity    |    .
                                  |    .
1111111111111111111111111111111111|0000|-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-
se preempts curr at wakeup ------>|<- se doesn't preempt curr -----------------
                                  |    .
                                  |    .
                                  |    .
low latency entity                |    .
                                   ---------------------->|
                               % of sysctl_sched_latency  |
1111111111111111111111111111111111111111111111111111111111|0000|-1-1-1-1-1-1-1-
preempt ------------------------------------------------->|<- do not preempt --
                                  |    .
                                  |    .
                                  |    .
high latency entity               |    .
         |<-----------------------|----.
         | % of sysctl_sched_latency   .
111111111|0000|-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1-1
preempt->|<- se doesn't preempt curr ------------------------------------------

Tests results of nice latency impact on heavy load like hackbench:

hackbench -l (2560 / group) -g group
group        latency 0             latency 19
1            1.378(+/-  1%)      1.337(+/- 1%) + 3%
4            1.393(+/-  3%)      1.312(+/- 3%) + 6%
8            1.308(+/-  2%)      1.279(+/- 1%) + 2%
16           1.347(+/-  1%)      1.317(+/- 1%) + 2%

hackbench -p -l (2560 / group) -g group
group
1            1.836(+/- 17%)      1.148(+/- 5%) +37%
4            1.586(+/-  6%)      1.109(+/- 8%) +30%
8            1.209(+/-  4%)      0.780(+/- 4%) +35%
16           0.805(+/-  5%)      0.728(+/- 4%) +10%

By deacreasing the latency prio, we reduce the number of preemption at
wakeup and help hackbench making progress.

Test results of nice latency impact on short live load like cyclictest
while competing with heavy load like hackbench:

hackbench -l 10000 -g $group &
cyclictest --policy other -D 5 -q -n
        latency 0           latency -20
group   min  avg    max     min  avg    max
0       16    19     29      17   18     29
1       43   299   7359      63   84   3422
4       56   449  14806      45   83    284
8       63   820  51123      63   83    283
16      64  1326  70684      41  157  26852

group = 0 means that hackbench is not running.

The avg is significantly improved with nice latency -20 especially with
large number of groups but min and max remain quite similar. If we add the
histogram parameter to get details of latency, we have :

hackbench -l 10000 -g 16 &
cyclictest --policy other -D 5 -q -n  -H 20000 --histfile data.txt
              latency 0    latency -20
Min Latencies:    64           62
Avg Latencies:  1170          107
Max Latencies: 88069        10417
50% latencies:   122           86
75% latencies:   614           91
85% latencies:   961           94
90% latencies:  1225           97
95% latencies:  6120          102
99% latencies: 18328          159

With percentile details, we see the benefit of nice latency -20 as
only 1% of the latencies are above 159us whereas the default latency
has got 15% around ~1ms or above and 5% over the 6ms.

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 include/linux/sched.h |  4 ++-
 init/init_task.c      |  2 +-
 kernel/sched/core.c   | 38 +++++++++++++++++++++----
 kernel/sched/debug.c  |  2 +-
 kernel/sched/fair.c   | 66 ++++++++++++++++++++++++++++++++++++++-----
 kernel/sched/sched.h  | 12 ++++++++
 6 files changed, 109 insertions(+), 15 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 6805f378a9c3..a74cad08e91e 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -567,6 +567,8 @@ struct sched_entity {
 	/* cached value of my_q->h_nr_running */
 	unsigned long			runnable_weight;
 #endif
+	/* preemption offset in ns */
+	long				latency_offset;
 
 #ifdef CONFIG_SMP
 	/*
@@ -783,7 +785,7 @@ struct task_struct {
 	int				static_prio;
 	int				normal_prio;
 	unsigned int			rt_priority;
-	int				latency_nice;
+	int				latency_prio;
 
 	struct sched_entity		se;
 	struct sched_rt_entity		rt;
diff --git a/init/init_task.c b/init/init_task.c
index 7dd71dd2d261..b8ddf403bc62 100644
--- a/init/init_task.c
+++ b/init/init_task.c
@@ -78,7 +78,7 @@ struct task_struct init_task
 	.prio		= MAX_PRIO - 20,
 	.static_prio	= MAX_PRIO - 20,
 	.normal_prio	= MAX_PRIO - 20,
-	.latency_nice	= DEFAULT_LATENCY_NICE,
+	.latency_prio	= NICE_WIDTH - 20,
 	.policy		= SCHED_NORMAL,
 	.cpus_ptr	= &init_task.cpus_mask,
 	.user_cpus_ptr	= NULL,
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index b2accc9da4fe..caf54e54a74f 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1284,6 +1284,16 @@ static void set_load_weight(struct task_struct *p, bool update_load)
 	}
 }
 
+static void set_latency_offset(struct task_struct *p)
+{
+	long weight = sched_latency_to_weight[p->latency_prio];
+	s64 offset;
+
+	offset = weight * get_sched_latency(false);
+	offset = div_s64(offset, NICE_LATENCY_WEIGHT_MAX);
+	p->se.latency_offset = (long)offset;
+}
+
 #ifdef CONFIG_UCLAMP_TASK
 /*
  * Serializes updates of utilization clamp values
@@ -4559,7 +4569,9 @@ int sched_fork(unsigned long clone_flags, struct task_struct *p)
 		p->prio = p->normal_prio = p->static_prio;
 		set_load_weight(p, false);
 
-		p->latency_nice = DEFAULT_LATENCY_NICE;
+		p->latency_prio = NICE_TO_LATENCY(0);
+		set_latency_offset(p);
+
 		/*
 		 * We don't need the reset flag anymore after the fork. It has
 		 * fulfilled its duty:
@@ -7324,8 +7336,10 @@ static void __setscheduler_params(struct task_struct *p,
 static void __setscheduler_latency(struct task_struct *p,
 		const struct sched_attr *attr)
 {
-	if (attr->sched_flags & SCHED_FLAG_LATENCY_NICE)
-		p->latency_nice = attr->sched_latency_nice;
+	if (attr->sched_flags & SCHED_FLAG_LATENCY_NICE) {
+		p->latency_prio = NICE_TO_LATENCY(attr->sched_latency_nice);
+		set_latency_offset(p);
+	}
 }
 
 /*
@@ -7510,7 +7524,7 @@ static int __sched_setscheduler(struct task_struct *p,
 		if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)
 			goto change;
 		if (attr->sched_flags & SCHED_FLAG_LATENCY_NICE &&
-		    attr->sched_latency_nice != p->latency_nice)
+		    attr->sched_latency_nice != LATENCY_TO_NICE(p->latency_prio))
 			goto change;
 
 		p->sched_reset_on_fork = reset_on_fork;
@@ -8051,7 +8065,7 @@ SYSCALL_DEFINE4(sched_getattr, pid_t, pid, struct sched_attr __user *, uattr,
 	get_params(p, &kattr);
 	kattr.sched_flags &= SCHED_FLAG_ALL;
 
-	kattr.sched_latency_nice = p->latency_nice;
+	kattr.sched_latency_nice = LATENCY_TO_NICE(p->latency_prio);
 
 #ifdef CONFIG_UCLAMP_TASK
 	/*
@@ -11204,6 +11218,20 @@ const u32 sched_prio_to_wmult[40] = {
  /*  15 */ 119304647, 148102320, 186737708, 238609294, 286331153,
 };
 
+/*
+ * latency weight for wakeup preemption
+ */
+const int sched_latency_to_weight[40] = {
+ /* -20 */     -1024,     -973,     -922,      -870,      -819,
+ /* -15 */      -768,     -717,     -666,      -614,      -563,
+ /* -10 */      -512,     -461,     -410,      -358,      -307,
+ /*  -5 */      -256,     -205,     -154,      -102,       -51,
+ /*   0 */         0,       51,      102,       154,       205,
+ /*   5 */       256,      307,      358,       410,       461,
+ /*  10 */       512,      563,      614,       666,       717,
+ /*  15 */       768,      819,      870,       922,       973,
+};
+
 void call_trace_sched_update_nr_running(struct rq *rq, int count)
 {
         trace_sched_update_nr_running_tp(rq, count);
diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
index a3f7876217a6..06aaa0c81d4b 100644
--- a/kernel/sched/debug.c
+++ b/kernel/sched/debug.c
@@ -1042,7 +1042,7 @@ void proc_sched_show_task(struct task_struct *p, struct pid_namespace *ns,
 #endif
 	P(policy);
 	P(prio);
-	P(latency_nice);
+	P(latency_prio);
 	if (task_has_dl_policy(p)) {
 		P(dl.runtime);
 		P(dl.deadline);
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index eb04c83112a0..4299d5108dc7 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4558,6 +4558,8 @@ dequeue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
 		update_idle_cfs_rq_clock_pelt(cfs_rq);
 }
 
+static long wakeup_latency_gran(struct sched_entity *curr, struct sched_entity *se);
+
 /*
  * Preempt the current task with a newly woken task if needed:
  */
@@ -4566,7 +4568,7 @@ check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
 {
 	unsigned long ideal_runtime, delta_exec;
 	struct sched_entity *se;
-	s64 delta;
+	s64 delta, offset;
 
 	ideal_runtime = sched_slice(cfs_rq, curr);
 	delta_exec = curr->sum_exec_runtime - curr->prev_sum_exec_runtime;
@@ -4591,10 +4593,12 @@ check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
 	se = __pick_first_entity(cfs_rq);
 	delta = curr->vruntime - se->vruntime;
 
-	if (delta < 0)
+	offset = wakeup_latency_gran(curr, se);
+	if (delta < offset)
 		return;
 
-	if (delta > ideal_runtime)
+	if ((delta > ideal_runtime) ||
+	    (delta > get_latency_max()))
 		resched_curr(rq_of(cfs_rq));
 }
 
@@ -5716,6 +5720,35 @@ static int sched_idle_cpu(int cpu)
 }
 #endif
 
+static void set_next_buddy(struct sched_entity *se);
+
+static void check_preempt_from_others(struct cfs_rq *cfs, struct sched_entity *se)
+{
+	struct sched_entity *next;
+
+	if (se->latency_offset >= 0)
+		return;
+
+	if (cfs->nr_running <= 1)
+		return;
+	/*
+	 * When waking from another class, we don't need to check to preempt at
+	 * wakeup and don't set next buddy as a candidate for being picked in
+	 * priority.
+	 * In case of simultaneous wakeup when current is another class, the
+	 * latency sensitive tasks lost opportunity to preempt non sensitive
+	 * tasks which woke up simultaneously.
+	 */
+
+	if (cfs->next)
+		next = cfs->next;
+	else
+		next = __pick_first_entity(cfs);
+
+	if (next && wakeup_preempt_entity(next, se) == 1)
+		set_next_buddy(se);
+}
+
 /*
  * The enqueue_task method is called before nr_running is
  * increased. Here we update the fair scheduling stats and
@@ -5802,14 +5835,15 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
 	if (!task_new)
 		update_overutilized_status(rq);
 
+	if (rq->curr->sched_class != &fair_sched_class)
+		check_preempt_from_others(cfs_rq_of(&p->se), &p->se);
+
 enqueue_throttle:
 	assert_list_leaf_cfs_rq(rq);
 
 	hrtick_update(rq);
 }
 
-static void set_next_buddy(struct sched_entity *se);
-
 /*
  * The dequeue_task method is called before nr_running is
  * decreased. We remove the task from the rbtree and
@@ -7128,6 +7162,23 @@ balance_fair(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
 }
 #endif /* CONFIG_SMP */
 
+static long wakeup_latency_gran(struct sched_entity *curr, struct sched_entity *se)
+{
+	long latency_offset = se->latency_offset;
+
+	/*
+	 * A negative latency offset means that the sched_entity has latency
+	 * requirement that needs to be evaluated versus other entity.
+	 * Otherwise, use the latency weight to evaluate how much scheduling
+	 * delay is acceptable by se.
+	 */
+	if ((latency_offset < 0) || (curr->latency_offset < 0))
+		latency_offset -= curr->latency_offset;
+	latency_offset = min_t(long, latency_offset, get_latency_max());
+
+	return latency_offset;
+}
+
 static unsigned long wakeup_gran(struct sched_entity *se)
 {
 	unsigned long gran = sysctl_sched_wakeup_granularity;
@@ -7166,11 +7217,12 @@ static int
 wakeup_preempt_entity(struct sched_entity *curr, struct sched_entity *se)
 {
 	s64 gran, vdiff = curr->vruntime - se->vruntime;
+	s64 offset = wakeup_latency_gran(curr, se);
 
-	if (vdiff <= 0)
+	if (vdiff < offset)
 		return -1;
 
-	gran = wakeup_gran(se);
+	gran = offset + wakeup_gran(se);
 
 	/*
 	 * At wake up, the vruntime of a task is capped to not be older than
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 4bf9d7777f99..99f10b4dc230 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -142,6 +142,17 @@ extern int sched_rr_timeslice;
  * Default tasks should be treated as a task with latency_nice = 0.
  */
 #define DEFAULT_LATENCY_NICE	0
+#define DEFAULT_LATENCY_PRIO	(DEFAULT_LATENCY_NICE + LATENCY_NICE_WIDTH/2)
+
+/*
+ * Convert user-nice values [ -20 ... 0 ... 19 ]
+ * to static latency [ 0..39 ],
+ * and back.
+ */
+#define NICE_TO_LATENCY(nice)	((nice) + DEFAULT_LATENCY_PRIO)
+#define LATENCY_TO_NICE(prio)	((prio) - DEFAULT_LATENCY_PRIO)
+#define NICE_LATENCY_SHIFT	(SCHED_FIXEDPOINT_SHIFT)
+#define NICE_LATENCY_WEIGHT_MAX	(1L << NICE_LATENCY_SHIFT)
 
 /*
  * Increase resolution of nice-level calculations for 64-bit architectures.
@@ -2116,6 +2127,7 @@ static_assert(WF_TTWU == SD_BALANCE_WAKE);
 
 extern const int		sched_prio_to_weight[40];
 extern const u32		sched_prio_to_wmult[40];
+extern const int		sched_latency_to_weight[40];
 
 /*
  * {de,en}queue flags:
-- 
2.17.1

[PATCH v7 6/9] sched/fair: Add sched group latency support

[Vincent Guittot]

Task can set its latency priority with sched_setattr(), which is then used
to set the latency offset of its sched_enity, but sched group entities
still have the default latency offset value.

Add a latency.nice field in cpu cgroup controller to set the latency
priority of the group similarly to sched_setattr(). The latency priority
is then used to set the offset of the sched_entities of the group.

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 Documentation/admin-guide/cgroup-v2.rst |  8 ++++
 kernel/sched/core.c                     | 52 +++++++++++++++++++++++++
 kernel/sched/fair.c                     | 33 ++++++++++++++++
 kernel/sched/sched.h                    |  4 ++
 4 files changed, 97 insertions(+)

diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
index be4a77baf784..d8ae7e411f9c 100644
--- a/Documentation/admin-guide/cgroup-v2.rst
+++ b/Documentation/admin-guide/cgroup-v2.rst
@@ -1095,6 +1095,14 @@ All time durations are in microseconds.
         values similar to the sched_setattr(2). This maximum utilization
         value is used to clamp the task specific maximum utilization clamp.
 
+  cpu.latency.nice
+	A read-write single value file which exists on non-root
+	cgroups.  The default is "0".
+
+	The nice value is in the range [-20, 19].
+
+	This interface file allows reading and setting latency using the
+	same values used by sched_setattr(2).
 
 
 Memory
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index caf54e54a74f..3f42b1f61a7e 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -10890,6 +10890,47 @@ static int cpu_idle_write_s64(struct cgroup_subsys_state *css,
 {
 	return sched_group_set_idle(css_tg(css), idle);
 }
+
+static s64 cpu_latency_nice_read_s64(struct cgroup_subsys_state *css,
+				    struct cftype *cft)
+{
+	int prio, delta, last_delta = INT_MAX;
+	s64 weight;
+
+	weight = css_tg(css)->latency_offset * NICE_LATENCY_WEIGHT_MAX;
+	weight = div_s64(weight, get_sched_latency(false));
+
+	/* Find the closest nice value to the current weight */
+	for (prio = 0; prio < ARRAY_SIZE(sched_latency_to_weight); prio++) {
+		delta = abs(sched_latency_to_weight[prio] - weight);
+		if (delta >= last_delta)
+			break;
+		last_delta = delta;
+	}
+
+	return LATENCY_TO_NICE(prio-1);
+}
+
+static int cpu_latency_nice_write_s64(struct cgroup_subsys_state *css,
+				     struct cftype *cft, s64 nice)
+{
+	s64 latency_offset;
+	long weight;
+	int idx;
+
+	if (nice < MIN_LATENCY_NICE || nice > MAX_LATENCY_NICE)
+		return -ERANGE;
+
+	idx = NICE_TO_LATENCY(nice);
+	idx = array_index_nospec(idx, LATENCY_NICE_WIDTH);
+	weight = sched_latency_to_weight[idx];
+
+	latency_offset = weight * get_sched_latency(false);
+	latency_offset = div_s64(latency_offset, NICE_LATENCY_WEIGHT_MAX);
+
+	return sched_group_set_latency(css_tg(css), latency_offset);
+}
+
 #endif
 
 static struct cftype cpu_legacy_files[] = {
@@ -10904,6 +10945,11 @@ static struct cftype cpu_legacy_files[] = {
 		.read_s64 = cpu_idle_read_s64,
 		.write_s64 = cpu_idle_write_s64,
 	},
+	{
+		.name = "latency.nice",
+		.read_s64 = cpu_latency_nice_read_s64,
+		.write_s64 = cpu_latency_nice_write_s64,
+	},
 #endif
 #ifdef CONFIG_CFS_BANDWIDTH
 	{
@@ -11121,6 +11167,12 @@ static struct cftype cpu_files[] = {
 		.read_s64 = cpu_idle_read_s64,
 		.write_s64 = cpu_idle_write_s64,
 	},
+	{
+		.name = "latency.nice",
+		.flags = CFTYPE_NOT_ON_ROOT,
+		.read_s64 = cpu_latency_nice_read_s64,
+		.write_s64 = cpu_latency_nice_write_s64,
+	},
 #endif
 #ifdef CONFIG_CFS_BANDWIDTH
 	{
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 4299d5108dc7..9583936ce30c 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -11764,6 +11764,7 @@ int alloc_fair_sched_group(struct task_group *tg, struct task_group *parent)
 		goto err;
 
 	tg->shares = NICE_0_LOAD;
+	tg->latency_offset = 0;
 
 	init_cfs_bandwidth(tg_cfs_bandwidth(tg));
 
@@ -11862,6 +11863,9 @@ void init_tg_cfs_entry(struct task_group *tg, struct cfs_rq *cfs_rq,
 	}
 
 	se->my_q = cfs_rq;
+
+	se->latency_offset = tg->latency_offset;
+
 	/* guarantee group entities always have weight */
 	update_load_set(&se->load, NICE_0_LOAD);
 	se->parent = parent;
@@ -11992,6 +11996,35 @@ int sched_group_set_idle(struct task_group *tg, long idle)
 	return 0;
 }
 
+int sched_group_set_latency(struct task_group *tg, s64 latency)
+{
+	int i;
+
+	if (tg == &root_task_group)
+		return -EINVAL;
+
+	if (abs(latency) > sysctl_sched_latency)
+		return -EINVAL;
+
+	mutex_lock(&shares_mutex);
+
+	if (tg->latency_offset == latency) {
+		mutex_unlock(&shares_mutex);
+		return 0;
+	}
+
+	tg->latency_offset = latency;
+
+	for_each_possible_cpu(i) {
+		struct sched_entity *se = tg->se[i];
+
+		WRITE_ONCE(se->latency_offset, latency);
+	}
+
+	mutex_unlock(&shares_mutex);
+	return 0;
+}
+
 #else /* CONFIG_FAIR_GROUP_SCHED */
 
 void free_fair_sched_group(struct task_group *tg) { }
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 99f10b4dc230..95d4be4f3af6 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -407,6 +407,8 @@ struct task_group {
 
 	/* A positive value indicates that this is a SCHED_IDLE group. */
 	int			idle;
+	/* latency constraint of the group. */
+	int			latency_offset;
 
 #ifdef	CONFIG_SMP
 	/*
@@ -517,6 +519,8 @@ extern int sched_group_set_shares(struct task_group *tg, unsigned long shares);
 
 extern int sched_group_set_idle(struct task_group *tg, long idle);
 
+extern int sched_group_set_latency(struct task_group *tg, s64 latency);
+
 #ifdef CONFIG_SMP
 extern void set_task_rq_fair(struct sched_entity *se,
 			     struct cfs_rq *prev, struct cfs_rq *next);
-- 
2.17.1

[PATCH v7 7/9] sched/core: Support latency priority with sched core

[Vincent Guittot]

Take into account wakeup_latency_gran() when ordering the cfs threads.

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 kernel/sched/fair.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 9583936ce30c..a7372f80b1ea 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -11439,6 +11439,9 @@ bool cfs_prio_less(struct task_struct *a, struct task_struct *b, bool in_fi)
 	delta = (s64)(sea->vruntime - seb->vruntime) +
 		(s64)(cfs_rqb->min_vruntime_fi - cfs_rqa->min_vruntime_fi);
 
+	/* Take into account latency prio */
+	delta -= wakeup_latency_gran(sea, seb);
+
 	return delta > 0;
 }
 #else
-- 
2.17.1

[PATCH v7 8/9] sched/fair: Add latency list

[Vincent Guittot]

Add a rb tree for latency sensitive entities so we can schedule the most
sensitive one first even when it failed to preempt current at wakeup or
when it got quickly preempted by another entity of higher priority.

In order to keep fairness, the latency is used once at wakeup to get a
minimum slice and not during the following scheduling slice to prevent
long running entity to got more running time than allocated to his nice
priority.

The rb tree enables to cover the last corner case where latency
sensitive entity can't got schedule quickly after the wakeup.

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 include/linux/sched.h |  2 +
 kernel/sched/fair.c   | 96 +++++++++++++++++++++++++++++++++++++++++--
 kernel/sched/sched.h  |  1 +
 3 files changed, 96 insertions(+), 3 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index a74cad08e91e..0b92674e3664 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -547,6 +547,8 @@ struct sched_entity {
 	/* For load-balancing: */
 	struct load_weight		load;
 	struct rb_node			run_node;
+	struct rb_node			latency_node;
+	unsigned int			on_latency;
 	struct list_head		group_node;
 	unsigned int			on_rq;
 
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index a7372f80b1ea..c28992b7d1a6 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -664,7 +664,77 @@ struct sched_entity *__pick_last_entity(struct cfs_rq *cfs_rq)
 
 	return __node_2_se(last);
 }
+#endif
 
+/**************************************************************
+ * Scheduling class tree data structure manipulation methods:
+ * for latency
+ */
+
+static inline bool latency_before(struct sched_entity *a,
+				struct sched_entity *b)
+{
+	return (s64)(a->vruntime + a->latency_offset - b->vruntime - b->latency_offset) < 0;
+}
+
+#define __latency_node_2_se(node) \
+	rb_entry((node), struct sched_entity, latency_node)
+
+static inline bool __latency_less(struct rb_node *a, const struct rb_node *b)
+{
+	return latency_before(__latency_node_2_se(a), __latency_node_2_se(b));
+}
+
+/*
+ * Enqueue an entity into the latency rb-tree:
+ */
+static void __enqueue_latency(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
+{
+
+	/* Only latency sensitive entity can be added to the list */
+	if (se->latency_offset >= 0)
+		return;
+
+	if (se->on_latency)
+		return;
+
+	/*
+	 * An execution time less than sysctl_sched_min_granularity means that
+	 * the entity has been preempted by a higher sched class or an entity
+	 * with higher latency constraint.
+	 * Put it back in the list so it gets a chance to run 1st during the
+	 * next slice.
+	 */
+	if (!(flags & ENQUEUE_WAKEUP)) {
+		u64 delta_exec = se->sum_exec_runtime - se->prev_sum_exec_runtime;
+
+		if (delta_exec >= sysctl_sched_min_granularity)
+			return;
+	}
+
+	rb_add_cached(&se->latency_node, &cfs_rq->latency_timeline, __latency_less);
+	se->on_latency = 1;
+}
+
+static void __dequeue_latency(struct cfs_rq *cfs_rq, struct sched_entity *se)
+{
+	if (se->on_latency) {
+		rb_erase_cached(&se->latency_node, &cfs_rq->latency_timeline);
+		se->on_latency = 0;
+	}
+}
+
+static struct sched_entity *__pick_first_latency(struct cfs_rq *cfs_rq)
+{
+	struct rb_node *left = rb_first_cached(&cfs_rq->latency_timeline);
+
+	if (!left)
+		return NULL;
+
+	return __latency_node_2_se(left);
+}
+
+#ifdef CONFIG_SCHED_DEBUG
 /**************************************************************
  * Scheduling class statistics methods:
  */
@@ -4439,8 +4509,10 @@ enqueue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
 	check_schedstat_required();
 	update_stats_enqueue_fair(cfs_rq, se, flags);
 	check_spread(cfs_rq, se);
-	if (!curr)
+	if (!curr) {
 		__enqueue_entity(cfs_rq, se);
+		__enqueue_latency(cfs_rq, se, flags);
+	}
 	se->on_rq = 1;
 
 	if (cfs_rq->nr_running == 1) {
@@ -4526,8 +4598,10 @@ dequeue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
 
 	clear_buddies(cfs_rq, se);
 
-	if (se != cfs_rq->curr)
+	if (se != cfs_rq->curr) {
 		__dequeue_entity(cfs_rq, se);
+		__dequeue_latency(cfs_rq, se);
+	}
 	se->on_rq = 0;
 	account_entity_dequeue(cfs_rq, se);
 
@@ -4616,6 +4690,7 @@ set_next_entity(struct cfs_rq *cfs_rq, struct sched_entity *se)
 		 */
 		update_stats_wait_end_fair(cfs_rq, se);
 		__dequeue_entity(cfs_rq, se);
+		__dequeue_latency(cfs_rq, se);
 		update_load_avg(cfs_rq, se, UPDATE_TG);
 	}
 
@@ -4654,7 +4729,7 @@ static struct sched_entity *
 pick_next_entity(struct cfs_rq *cfs_rq, struct sched_entity *curr)
 {
 	struct sched_entity *left = __pick_first_entity(cfs_rq);
-	struct sched_entity *se;
+	struct sched_entity *latency, *se;
 
 	/*
 	 * If curr is set we have to see if its left of the leftmost entity
@@ -4696,6 +4771,12 @@ pick_next_entity(struct cfs_rq *cfs_rq, struct sched_entity *curr)
 		se = cfs_rq->last;
 	}
 
+	/* Check for latency sensitive entity waiting for running */
+	latency = __pick_first_latency(cfs_rq);
+	if (latency && (latency != se) &&
+	    wakeup_preempt_entity(latency, se) < 1)
+		se = latency;
+
 	return se;
 }
 
@@ -4719,6 +4800,7 @@ static void put_prev_entity(struct cfs_rq *cfs_rq, struct sched_entity *prev)
 		update_stats_wait_start_fair(cfs_rq, prev);
 		/* Put 'current' back into the tree. */
 		__enqueue_entity(cfs_rq, prev);
+		__enqueue_latency(cfs_rq, prev, 0);
 		/* in !on_rq case, update occurred at dequeue */
 		update_load_avg(cfs_rq, prev, 0);
 	}
@@ -11712,6 +11794,7 @@ static void set_next_task_fair(struct rq *rq, struct task_struct *p, bool first)
 void init_cfs_rq(struct cfs_rq *cfs_rq)
 {
 	cfs_rq->tasks_timeline = RB_ROOT_CACHED;
+	cfs_rq->latency_timeline = RB_ROOT_CACHED;
 	u64_u32_store(cfs_rq->min_vruntime, (u64)(-(1LL << 20)));
 #ifdef CONFIG_SMP
 	raw_spin_lock_init(&cfs_rq->removed.lock);
@@ -12020,8 +12103,15 @@ int sched_group_set_latency(struct task_group *tg, s64 latency)
 
 	for_each_possible_cpu(i) {
 		struct sched_entity *se = tg->se[i];
+		struct rq *rq = cpu_rq(i);
+		struct rq_flags rf;
+
+		rq_lock_irqsave(rq, &rf);
 
+		__dequeue_latency(se->cfs_rq, se);
 		WRITE_ONCE(se->latency_offset, latency);
+
+		rq_unlock_irqrestore(rq, &rf);
 	}
 
 	mutex_unlock(&shares_mutex);
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 95d4be4f3af6..91ec36c1158b 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -599,6 +599,7 @@ struct cfs_rq {
 #endif
 
 	struct rb_root_cached	tasks_timeline;
+	struct rb_root_cached	latency_timeline;
 
 	/*
 	 * 'curr' points to currently running entity on this cfs_rq.
-- 
2.17.1

[PATCH v7 9/9] sched/fair: remove check_preempt_from_others

[Vincent Guittot]

With the dedicated latency list, we don't have to take care of this special
case anymore as pick_next_entity checks for a runnable latency sensitive
task.

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---
 kernel/sched/fair.c | 34 ++--------------------------------
 1 file changed, 2 insertions(+), 32 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index c28992b7d1a6..9a421b49dbfd 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5802,35 +5802,6 @@ static int sched_idle_cpu(int cpu)
 }
 #endif
 
-static void set_next_buddy(struct sched_entity *se);
-
-static void check_preempt_from_others(struct cfs_rq *cfs, struct sched_entity *se)
-{
-	struct sched_entity *next;
-
-	if (se->latency_offset >= 0)
-		return;
-
-	if (cfs->nr_running <= 1)
-		return;
-	/*
-	 * When waking from another class, we don't need to check to preempt at
-	 * wakeup and don't set next buddy as a candidate for being picked in
-	 * priority.
-	 * In case of simultaneous wakeup when current is another class, the
-	 * latency sensitive tasks lost opportunity to preempt non sensitive
-	 * tasks which woke up simultaneously.
-	 */
-
-	if (cfs->next)
-		next = cfs->next;
-	else
-		next = __pick_first_entity(cfs);
-
-	if (next && wakeup_preempt_entity(next, se) == 1)
-		set_next_buddy(se);
-}
-
 /*
  * The enqueue_task method is called before nr_running is
  * increased. Here we update the fair scheduling stats and
@@ -5917,15 +5888,14 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
 	if (!task_new)
 		update_overutilized_status(rq);
 
-	if (rq->curr->sched_class != &fair_sched_class)
-		check_preempt_from_others(cfs_rq_of(&p->se), &p->se);
-
 enqueue_throttle:
 	assert_list_leaf_cfs_rq(rq);
 
 	hrtick_update(rq);
 }
 
+static void set_next_buddy(struct sched_entity *se);
+
 /*
  * The dequeue_task method is called before nr_running is
  * decreased. We remove the task from the rbtree and
-- 
2.17.1

Re: [PATCH v7 6/9] sched/fair: Add sched group latency support

[Qais Yousef]

On 10/28/22 11:34, Vincent Guittot wrote:
> Task can set its latency priority with sched_setattr(), which is then used
> to set the latency offset of its sched_enity, but sched group entities
> still have the default latency offset value.
> 
> Add a latency.nice field in cpu cgroup controller to set the latency
> priority of the group similarly to sched_setattr(). The latency priority
> is then used to set the offset of the sched_entities of the group.
> 
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
>  Documentation/admin-guide/cgroup-v2.rst |  8 ++++
>  kernel/sched/core.c                     | 52 +++++++++++++++++++++++++
>  kernel/sched/fair.c                     | 33 ++++++++++++++++
>  kernel/sched/sched.h                    |  4 ++
>  4 files changed, 97 insertions(+)
> 
> diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> index be4a77baf784..d8ae7e411f9c 100644
> --- a/Documentation/admin-guide/cgroup-v2.rst
> +++ b/Documentation/admin-guide/cgroup-v2.rst
> @@ -1095,6 +1095,14 @@ All time durations are in microseconds.
>          values similar to the sched_setattr(2). This maximum utilization
>          value is used to clamp the task specific maximum utilization clamp.
>  
> +  cpu.latency.nice
> +	A read-write single value file which exists on non-root
> +	cgroups.  The default is "0".
> +
> +	The nice value is in the range [-20, 19].
> +
> +	This interface file allows reading and setting latency using the
> +	same values used by sched_setattr(2).

I'm still not sure about this [1].

In some scenarios we'd like to get the effective latency_nice of the task. How
will the task inherit the cgroup value or be impacted by it?

For example if there are tasks that belong to a latency sensitive cgroup; and
I'd like to skip some searches in EAS to improve that latency sensitivity - how
would I extract this info in EAS path given these tasks are using default
latency_nice value? And if should happen if their latency_nice is set to
something else other than default?

[1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/


Thanks

--
Qais Yousef

>  
>  
>  Memory
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index caf54e54a74f..3f42b1f61a7e 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -10890,6 +10890,47 @@ static int cpu_idle_write_s64(struct cgroup_subsys_state *css,
>  {
>  	return sched_group_set_idle(css_tg(css), idle);
>  }
> +
> +static s64 cpu_latency_nice_read_s64(struct cgroup_subsys_state *css,
> +				    struct cftype *cft)
> +{
> +	int prio, delta, last_delta = INT_MAX;
> +	s64 weight;
> +
> +	weight = css_tg(css)->latency_offset * NICE_LATENCY_WEIGHT_MAX;
> +	weight = div_s64(weight, get_sched_latency(false));
> +
> +	/* Find the closest nice value to the current weight */
> +	for (prio = 0; prio < ARRAY_SIZE(sched_latency_to_weight); prio++) {
> +		delta = abs(sched_latency_to_weight[prio] - weight);
> +		if (delta >= last_delta)
> +			break;
> +		last_delta = delta;
> +	}
> +
> +	return LATENCY_TO_NICE(prio-1);
> +}
> +
> +static int cpu_latency_nice_write_s64(struct cgroup_subsys_state *css,
> +				     struct cftype *cft, s64 nice)
> +{
> +	s64 latency_offset;
> +	long weight;
> +	int idx;
> +
> +	if (nice < MIN_LATENCY_NICE || nice > MAX_LATENCY_NICE)
> +		return -ERANGE;
> +
> +	idx = NICE_TO_LATENCY(nice);
> +	idx = array_index_nospec(idx, LATENCY_NICE_WIDTH);
> +	weight = sched_latency_to_weight[idx];
> +
> +	latency_offset = weight * get_sched_latency(false);
> +	latency_offset = div_s64(latency_offset, NICE_LATENCY_WEIGHT_MAX);
> +
> +	return sched_group_set_latency(css_tg(css), latency_offset);
> +}
> +
>  #endif
>  
>  static struct cftype cpu_legacy_files[] = {
> @@ -10904,6 +10945,11 @@ static struct cftype cpu_legacy_files[] = {
>  		.read_s64 = cpu_idle_read_s64,
>  		.write_s64 = cpu_idle_write_s64,
>  	},
> +	{
> +		.name = "latency.nice",
> +		.read_s64 = cpu_latency_nice_read_s64,
> +		.write_s64 = cpu_latency_nice_write_s64,
> +	},
>  #endif
>  #ifdef CONFIG_CFS_BANDWIDTH
>  	{
> @@ -11121,6 +11167,12 @@ static struct cftype cpu_files[] = {
>  		.read_s64 = cpu_idle_read_s64,
>  		.write_s64 = cpu_idle_write_s64,
>  	},
> +	{
> +		.name = "latency.nice",
> +		.flags = CFTYPE_NOT_ON_ROOT,
> +		.read_s64 = cpu_latency_nice_read_s64,
> +		.write_s64 = cpu_latency_nice_write_s64,
> +	},
>  #endif
>  #ifdef CONFIG_CFS_BANDWIDTH
>  	{
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 4299d5108dc7..9583936ce30c 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -11764,6 +11764,7 @@ int alloc_fair_sched_group(struct task_group *tg, struct task_group *parent)
>  		goto err;
>  
>  	tg->shares = NICE_0_LOAD;
> +	tg->latency_offset = 0;
>  
>  	init_cfs_bandwidth(tg_cfs_bandwidth(tg));
>  
> @@ -11862,6 +11863,9 @@ void init_tg_cfs_entry(struct task_group *tg, struct cfs_rq *cfs_rq,
>  	}
>  
>  	se->my_q = cfs_rq;
> +
> +	se->latency_offset = tg->latency_offset;
> +
>  	/* guarantee group entities always have weight */
>  	update_load_set(&se->load, NICE_0_LOAD);
>  	se->parent = parent;
> @@ -11992,6 +11996,35 @@ int sched_group_set_idle(struct task_group *tg, long idle)
>  	return 0;
>  }
>  
> +int sched_group_set_latency(struct task_group *tg, s64 latency)
> +{
> +	int i;
> +
> +	if (tg == &root_task_group)
> +		return -EINVAL;
> +
> +	if (abs(latency) > sysctl_sched_latency)
> +		return -EINVAL;
> +
> +	mutex_lock(&shares_mutex);
> +
> +	if (tg->latency_offset == latency) {
> +		mutex_unlock(&shares_mutex);
> +		return 0;
> +	}
> +
> +	tg->latency_offset = latency;
> +
> +	for_each_possible_cpu(i) {
> +		struct sched_entity *se = tg->se[i];
> +
> +		WRITE_ONCE(se->latency_offset, latency);
> +	}
> +
> +	mutex_unlock(&shares_mutex);
> +	return 0;
> +}
> +
>  #else /* CONFIG_FAIR_GROUP_SCHED */
>  
>  void free_fair_sched_group(struct task_group *tg) { }
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 99f10b4dc230..95d4be4f3af6 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -407,6 +407,8 @@ struct task_group {
>  
>  	/* A positive value indicates that this is a SCHED_IDLE group. */
>  	int			idle;
> +	/* latency constraint of the group. */
> +	int			latency_offset;
>  
>  #ifdef	CONFIG_SMP
>  	/*
> @@ -517,6 +519,8 @@ extern int sched_group_set_shares(struct task_group *tg, unsigned long shares);
>  
>  extern int sched_group_set_idle(struct task_group *tg, long idle);
>  
> +extern int sched_group_set_latency(struct task_group *tg, s64 latency);
> +
>  #ifdef CONFIG_SMP
>  extern void set_task_rq_fair(struct sched_entity *se,
>  			     struct cfs_rq *prev, struct cfs_rq *next);
> -- 
> 2.17.1
> 

Re: [PATCH v7 6/9] sched/fair: Add sched group latency support

[Vincent Guittot]

On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote:
>
> On 10/28/22 11:34, Vincent Guittot wrote:
> > Task can set its latency priority with sched_setattr(), which is then used
> > to set the latency offset of its sched_enity, but sched group entities
> > still have the default latency offset value.
> >
> > Add a latency.nice field in cpu cgroup controller to set the latency
> > priority of the group similarly to sched_setattr(). The latency priority
> > is then used to set the offset of the sched_entities of the group.
> >
> > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > ---
> >  Documentation/admin-guide/cgroup-v2.rst |  8 ++++
> >  kernel/sched/core.c                     | 52 +++++++++++++++++++++++++
> >  kernel/sched/fair.c                     | 33 ++++++++++++++++
> >  kernel/sched/sched.h                    |  4 ++
> >  4 files changed, 97 insertions(+)
> >
> > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> > index be4a77baf784..d8ae7e411f9c 100644
> > --- a/Documentation/admin-guide/cgroup-v2.rst
> > +++ b/Documentation/admin-guide/cgroup-v2.rst
> > @@ -1095,6 +1095,14 @@ All time durations are in microseconds.
> >          values similar to the sched_setattr(2). This maximum utilization
> >          value is used to clamp the task specific maximum utilization clamp.
> >
> > +  cpu.latency.nice
> > +     A read-write single value file which exists on non-root
> > +     cgroups.  The default is "0".
> > +
> > +     The nice value is in the range [-20, 19].
> > +
> > +     This interface file allows reading and setting latency using the
> > +     same values used by sched_setattr(2).
>
> I'm still not sure about this [1].

I'm still not sure about what you are trying to say here ...

This is about setting a latency nice prio to a group level.

>
> In some scenarios we'd like to get the effective latency_nice of the task. How
> will the task inherit the cgroup value or be impacted by it?
>
> For example if there are tasks that belong to a latency sensitive cgroup; and
> I'd like to skip some searches in EAS to improve that latency sensitivity - how
> would I extract this info in EAS path given these tasks are using default
> latency_nice value? And if should happen if their latency_nice is set to
> something else other than default?
>
> [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/

Hmm so you are speaking about something that is not part of the patch.
Let focus on the patchset for now

>
>
> Thanks
>
> --
> Qais Yousef
>
> >
> >
> >  Memory
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index caf54e54a74f..3f42b1f61a7e 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -10890,6 +10890,47 @@ static int cpu_idle_write_s64(struct cgroup_subsys_state *css,
> >  {
> >       return sched_group_set_idle(css_tg(css), idle);
> >  }
> > +
> > +static s64 cpu_latency_nice_read_s64(struct cgroup_subsys_state *css,
> > +                                 struct cftype *cft)
> > +{
> > +     int prio, delta, last_delta = INT_MAX;
> > +     s64 weight;
> > +
> > +     weight = css_tg(css)->latency_offset * NICE_LATENCY_WEIGHT_MAX;
> > +     weight = div_s64(weight, get_sched_latency(false));
> > +
> > +     /* Find the closest nice value to the current weight */
> > +     for (prio = 0; prio < ARRAY_SIZE(sched_latency_to_weight); prio++) {
> > +             delta = abs(sched_latency_to_weight[prio] - weight);
> > +             if (delta >= last_delta)
> > +                     break;
> > +             last_delta = delta;
> > +     }
> > +
> > +     return LATENCY_TO_NICE(prio-1);
> > +}
> > +
> > +static int cpu_latency_nice_write_s64(struct cgroup_subsys_state *css,
> > +                                  struct cftype *cft, s64 nice)
> > +{
> > +     s64 latency_offset;
> > +     long weight;
> > +     int idx;
> > +
> > +     if (nice < MIN_LATENCY_NICE || nice > MAX_LATENCY_NICE)
> > +             return -ERANGE;
> > +
> > +     idx = NICE_TO_LATENCY(nice);
> > +     idx = array_index_nospec(idx, LATENCY_NICE_WIDTH);
> > +     weight = sched_latency_to_weight[idx];
> > +
> > +     latency_offset = weight * get_sched_latency(false);
> > +     latency_offset = div_s64(latency_offset, NICE_LATENCY_WEIGHT_MAX);
> > +
> > +     return sched_group_set_latency(css_tg(css), latency_offset);
> > +}
> > +
> >  #endif
> >
> >  static struct cftype cpu_legacy_files[] = {
> > @@ -10904,6 +10945,11 @@ static struct cftype cpu_legacy_files[] = {
> >               .read_s64 = cpu_idle_read_s64,
> >               .write_s64 = cpu_idle_write_s64,
> >       },
> > +     {
> > +             .name = "latency.nice",
> > +             .read_s64 = cpu_latency_nice_read_s64,
> > +             .write_s64 = cpu_latency_nice_write_s64,
> > +     },
> >  #endif
> >  #ifdef CONFIG_CFS_BANDWIDTH
> >       {
> > @@ -11121,6 +11167,12 @@ static struct cftype cpu_files[] = {
> >               .read_s64 = cpu_idle_read_s64,
> >               .write_s64 = cpu_idle_write_s64,
> >       },
> > +     {
> > +             .name = "latency.nice",
> > +             .flags = CFTYPE_NOT_ON_ROOT,
> > +             .read_s64 = cpu_latency_nice_read_s64,
> > +             .write_s64 = cpu_latency_nice_write_s64,
> > +     },
> >  #endif
> >  #ifdef CONFIG_CFS_BANDWIDTH
> >       {
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 4299d5108dc7..9583936ce30c 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -11764,6 +11764,7 @@ int alloc_fair_sched_group(struct task_group *tg, struct task_group *parent)
> >               goto err;
> >
> >       tg->shares = NICE_0_LOAD;
> > +     tg->latency_offset = 0;
> >
> >       init_cfs_bandwidth(tg_cfs_bandwidth(tg));
> >
> > @@ -11862,6 +11863,9 @@ void init_tg_cfs_entry(struct task_group *tg, struct cfs_rq *cfs_rq,
> >       }
> >
> >       se->my_q = cfs_rq;
> > +
> > +     se->latency_offset = tg->latency_offset;
> > +
> >       /* guarantee group entities always have weight */
> >       update_load_set(&se->load, NICE_0_LOAD);
> >       se->parent = parent;
> > @@ -11992,6 +11996,35 @@ int sched_group_set_idle(struct task_group *tg, long idle)
> >       return 0;
> >  }
> >
> > +int sched_group_set_latency(struct task_group *tg, s64 latency)
> > +{
> > +     int i;
> > +
> > +     if (tg == &root_task_group)
> > +             return -EINVAL;
> > +
> > +     if (abs(latency) > sysctl_sched_latency)
> > +             return -EINVAL;
> > +
> > +     mutex_lock(&shares_mutex);
> > +
> > +     if (tg->latency_offset == latency) {
> > +             mutex_unlock(&shares_mutex);
> > +             return 0;
> > +     }
> > +
> > +     tg->latency_offset = latency;
> > +
> > +     for_each_possible_cpu(i) {
> > +             struct sched_entity *se = tg->se[i];
> > +
> > +             WRITE_ONCE(se->latency_offset, latency);
> > +     }
> > +
> > +     mutex_unlock(&shares_mutex);
> > +     return 0;
> > +}
> > +
> >  #else /* CONFIG_FAIR_GROUP_SCHED */
> >
> >  void free_fair_sched_group(struct task_group *tg) { }
> > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > index 99f10b4dc230..95d4be4f3af6 100644
> > --- a/kernel/sched/sched.h
> > +++ b/kernel/sched/sched.h
> > @@ -407,6 +407,8 @@ struct task_group {
> >
> >       /* A positive value indicates that this is a SCHED_IDLE group. */
> >       int                     idle;
> > +     /* latency constraint of the group. */
> > +     int                     latency_offset;
> >
> >  #ifdef       CONFIG_SMP
> >       /*
> > @@ -517,6 +519,8 @@ extern int sched_group_set_shares(struct task_group *tg, unsigned long shares);
> >
> >  extern int sched_group_set_idle(struct task_group *tg, long idle);
> >
> > +extern int sched_group_set_latency(struct task_group *tg, s64 latency);
> > +
> >  #ifdef CONFIG_SMP
> >  extern void set_task_rq_fair(struct sched_entity *se,
> >                            struct cfs_rq *prev, struct cfs_rq *next);
> > --
> > 2.17.1
> >

Re: [PATCH v7 6/9] sched/fair: Add sched group latency support

[Qais Yousef]

On 11/03/22 09:46, Vincent Guittot wrote:
> On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote:
> >
> > On 10/28/22 11:34, Vincent Guittot wrote:
> > > Task can set its latency priority with sched_setattr(), which is then used
> > > to set the latency offset of its sched_enity, but sched group entities
> > > still have the default latency offset value.
> > >
> > > Add a latency.nice field in cpu cgroup controller to set the latency
> > > priority of the group similarly to sched_setattr(). The latency priority
> > > is then used to set the offset of the sched_entities of the group.
> > >
> > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > ---
> > >  Documentation/admin-guide/cgroup-v2.rst |  8 ++++
> > >  kernel/sched/core.c                     | 52 +++++++++++++++++++++++++
> > >  kernel/sched/fair.c                     | 33 ++++++++++++++++
> > >  kernel/sched/sched.h                    |  4 ++
> > >  4 files changed, 97 insertions(+)
> > >
> > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> > > index be4a77baf784..d8ae7e411f9c 100644
> > > --- a/Documentation/admin-guide/cgroup-v2.rst
> > > +++ b/Documentation/admin-guide/cgroup-v2.rst
> > > @@ -1095,6 +1095,14 @@ All time durations are in microseconds.
> > >          values similar to the sched_setattr(2). This maximum utilization
> > >          value is used to clamp the task specific maximum utilization clamp.
> > >
> > > +  cpu.latency.nice
> > > +     A read-write single value file which exists on non-root
> > > +     cgroups.  The default is "0".
> > > +
> > > +     The nice value is in the range [-20, 19].
> > > +
> > > +     This interface file allows reading and setting latency using the
> > > +     same values used by sched_setattr(2).
> >
> > I'm still not sure about this [1].
> 
> I'm still not sure about what you are trying to say here ...
> 
> This is about setting a latency nice prio to a group level.
> 
> >
> > In some scenarios we'd like to get the effective latency_nice of the task. How
> > will the task inherit the cgroup value or be impacted by it?
> >
> > For example if there are tasks that belong to a latency sensitive cgroup; and
> > I'd like to skip some searches in EAS to improve that latency sensitivity - how
> > would I extract this info in EAS path given these tasks are using default
> > latency_nice value? And if should happen if their latency_nice is set to
> > something else other than default?
> >
> > [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/
> 
> Hmm so you are speaking about something that is not part of the patch.
> Let focus on the patchset for now

I am focusing on this patchset. Isn't this an essential part of the design?
Once the interface is out we can't change it. As it stands, I can't see how it
can be used to replace prefer_idle in cgroup as used in Android. I can't see
how this could happen if we don't define how the task will inherit the cgroup
value. If we can, mind elaborating how please?


Thanks

--
Qais Yousef

Re: [PATCH v7 6/9] sched/fair: Add sched group latency support

[Vincent Guittot]

On Thu, 3 Nov 2022 at 15:27, Qais Yousef <qyousef@layalina.io> wrote:
>
> On 11/03/22 09:46, Vincent Guittot wrote:
> > On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote:
> > >
> > > On 10/28/22 11:34, Vincent Guittot wrote:
> > > > Task can set its latency priority with sched_setattr(), which is then used
> > > > to set the latency offset of its sched_enity, but sched group entities
> > > > still have the default latency offset value.
> > > >
> > > > Add a latency.nice field in cpu cgroup controller to set the latency
> > > > priority of the group similarly to sched_setattr(). The latency priority
> > > > is then used to set the offset of the sched_entities of the group.
> > > >
> > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > > ---
> > > >  Documentation/admin-guide/cgroup-v2.rst |  8 ++++
> > > >  kernel/sched/core.c                     | 52 +++++++++++++++++++++++++
> > > >  kernel/sched/fair.c                     | 33 ++++++++++++++++
> > > >  kernel/sched/sched.h                    |  4 ++
> > > >  4 files changed, 97 insertions(+)
> > > >
> > > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> > > > index be4a77baf784..d8ae7e411f9c 100644
> > > > --- a/Documentation/admin-guide/cgroup-v2.rst
> > > > +++ b/Documentation/admin-guide/cgroup-v2.rst
> > > > @@ -1095,6 +1095,14 @@ All time durations are in microseconds.
> > > >          values similar to the sched_setattr(2). This maximum utilization
> > > >          value is used to clamp the task specific maximum utilization clamp.
> > > >
> > > > +  cpu.latency.nice
> > > > +     A read-write single value file which exists on non-root
> > > > +     cgroups.  The default is "0".
> > > > +
> > > > +     The nice value is in the range [-20, 19].
> > > > +
> > > > +     This interface file allows reading and setting latency using the
> > > > +     same values used by sched_setattr(2).
> > >
> > > I'm still not sure about this [1].
> >
> > I'm still not sure about what you are trying to say here ...
> >
> > This is about setting a latency nice prio to a group level.
> >
> > >
> > > In some scenarios we'd like to get the effective latency_nice of the task. How
> > > will the task inherit the cgroup value or be impacted by it?
> > >
> > > For example if there are tasks that belong to a latency sensitive cgroup; and
> > > I'd like to skip some searches in EAS to improve that latency sensitivity - how
> > > would I extract this info in EAS path given these tasks are using default
> > > latency_nice value? And if should happen if their latency_nice is set to
> > > something else other than default?
> > >
> > > [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/
> >
> > Hmm so you are speaking about something that is not part of the patch.
> > Let focus on the patchset for now
>
> I am focusing on this patchset. Isn't this an essential part of the design?
> Once the interface is out we can't change it. As it stands, I can't see how it

So, are you speaking about the interface i.e. setting a value between [-20:19]

> can be used to replace prefer_idle in cgroup as used in Android. I can't see
> how this could happen if we don't define how the task will inherit the cgroup
> value. If we can, mind elaborating how please?

Or how to take into account the value set for a cgroup ?

Regarding the behavior, the rule remains the same that a sched_entity
attached to a cgroup will not get more (latency in this case) than
what has been set for the group entity.

>
>
> Thanks
>
> --
> Qais Yousef

Re: [PATCH v7 6/9] sched/fair: Add sched group latency support

[Joel Fernandes]

On Thu, Nov 3, 2022 at 5:03 PM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Thu, 3 Nov 2022 at 15:27, Qais Yousef <qyousef@layalina.io> wrote:
> >
> > On 11/03/22 09:46, Vincent Guittot wrote:
> > > On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote:
> > > >
> > > > On 10/28/22 11:34, Vincent Guittot wrote:
> > > > > Task can set its latency priority with sched_setattr(), which is then used
> > > > > to set the latency offset of its sched_enity, but sched group entities
> > > > > still have the default latency offset value.
> > > > >
> > > > > Add a latency.nice field in cpu cgroup controller to set the latency
> > > > > priority of the group similarly to sched_setattr(). The latency priority
> > > > > is then used to set the offset of the sched_entities of the group.
> > > > >
> > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > > > ---
> > > > >  Documentation/admin-guide/cgroup-v2.rst |  8 ++++
> > > > >  kernel/sched/core.c                     | 52 +++++++++++++++++++++++++
> > > > >  kernel/sched/fair.c                     | 33 ++++++++++++++++
> > > > >  kernel/sched/sched.h                    |  4 ++
> > > > >  4 files changed, 97 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> > > > > index be4a77baf784..d8ae7e411f9c 100644
> > > > > --- a/Documentation/admin-guide/cgroup-v2.rst
> > > > > +++ b/Documentation/admin-guide/cgroup-v2.rst
> > > > > @@ -1095,6 +1095,14 @@ All time durations are in microseconds.
> > > > >          values similar to the sched_setattr(2). This maximum utilization
> > > > >          value is used to clamp the task specific maximum utilization clamp.
> > > > >
> > > > > +  cpu.latency.nice
> > > > > +     A read-write single value file which exists on non-root
> > > > > +     cgroups.  The default is "0".
> > > > > +
> > > > > +     The nice value is in the range [-20, 19].
> > > > > +
> > > > > +     This interface file allows reading and setting latency using the
> > > > > +     same values used by sched_setattr(2).
> > > >
> > > > I'm still not sure about this [1].
> > >
> > > I'm still not sure about what you are trying to say here ...
> > >
> > > This is about setting a latency nice prio to a group level.
> > >
> > > >
> > > > In some scenarios we'd like to get the effective latency_nice of the task. How
> > > > will the task inherit the cgroup value or be impacted by it?
> > > >
> > > > For example if there are tasks that belong to a latency sensitive cgroup; and
> > > > I'd like to skip some searches in EAS to improve that latency sensitivity - how
> > > > would I extract this info in EAS path given these tasks are using default
> > > > latency_nice value? And if should happen if their latency_nice is set to
> > > > something else other than default?
> > > >
> > > > [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/
> > >
> > > Hmm so you are speaking about something that is not part of the patch.
> > > Let focus on the patchset for now
> >
> > I am focusing on this patchset. Isn't this an essential part of the design?
> > Once the interface is out we can't change it. As it stands, I can't see how it
>
> So, are you speaking about the interface i.e. setting a value between [-20:19]
>
> > can be used to replace prefer_idle in cgroup as used in Android. I can't see
> > how this could happen if we don't define how the task will inherit the cgroup
> > value. If we can, mind elaborating how please?
>
> Or how to take into account the value set for a cgroup ?
>
> Regarding the behavior, the rule remains the same that a sched_entity
> attached to a cgroup will not get more (latency in this case) than
> what has been set for the group entity.

I think the interface solves a different problem which is latency of
task or cgroup wrt other group. Vincent, you are setting this for a
“top app” group in android in your tests, and seeing improvement
correct? AFAICS, this improvement comes because of lower latency
during *same CPU* competition between different groups by juggling
around the wakeup-preemption window -- which maybe is good for
Android.

OTOH, the “prefer idle” flag in android that Qais is referring to,
will need a completely different method as I cannot see how a nice
value can communicate that (that can complement Vincent's changes
here). And it will need to have a per-task interface as well. We have
something in ChromeOS as well, which is a proc knob and also
out-of-tree patch for that [1]. Without [1] we fail Android CTS
testing on a recent ARM64 ChromeOS device.
[1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3884575
The changelog in [1] also has a detailed description of the ChromeOS usecase.

Qais, any other reason you can see why Vincent's change will not be a
good thing for Android? Since you 1 CGroup for the whole user-facing
app (top app), you can just set that to a low "latency_nice" and get
better wake-up latency for that.

(Side rant about latency and CFS -- IMHO a better long term solution
for lower latency is to use RT but don't throttle -- rather demote. Or
break CFS into multiple tiers, and apply demotion. This is in a way
what Vincent is doing, as the task becomes more CPU bound'ish, he's
taking away the latency boost. Vincent/Qais, somebody was working on
the RT demotion vs throttling a while back, any idea on the latest on
that?).

thanks,

 - Joel


>
> >
> >
> > Thanks
> >
> > --
> > Qais Yousef

Re: [PATCH v7 6/9] sched/fair: Add sched group latency support

[Vincent Guittot]

On Fri, 4 Nov 2022 at 11:15, Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Thu, Nov 3, 2022 at 5:03 PM Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> >
> > On Thu, 3 Nov 2022 at 15:27, Qais Yousef <qyousef@layalina.io> wrote:
> > >
> > > On 11/03/22 09:46, Vincent Guittot wrote:
> > > > On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote:
> > > > >
> > > > > On 10/28/22 11:34, Vincent Guittot wrote:
> > > > > > Task can set its latency priority with sched_setattr(), which is then used
> > > > > > to set the latency offset of its sched_enity, but sched group entities
> > > > > > still have the default latency offset value.
> > > > > >
> > > > > > Add a latency.nice field in cpu cgroup controller to set the latency
> > > > > > priority of the group similarly to sched_setattr(). The latency priority
> > > > > > is then used to set the offset of the sched_entities of the group.
> > > > > >
> > > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > > > > ---
> > > > > >  Documentation/admin-guide/cgroup-v2.rst |  8 ++++
> > > > > >  kernel/sched/core.c                     | 52 +++++++++++++++++++++++++
> > > > > >  kernel/sched/fair.c                     | 33 ++++++++++++++++
> > > > > >  kernel/sched/sched.h                    |  4 ++
> > > > > >  4 files changed, 97 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> > > > > > index be4a77baf784..d8ae7e411f9c 100644
> > > > > > --- a/Documentation/admin-guide/cgroup-v2.rst
> > > > > > +++ b/Documentation/admin-guide/cgroup-v2.rst
> > > > > > @@ -1095,6 +1095,14 @@ All time durations are in microseconds.
> > > > > >          values similar to the sched_setattr(2). This maximum utilization
> > > > > >          value is used to clamp the task specific maximum utilization clamp.
> > > > > >
> > > > > > +  cpu.latency.nice
> > > > > > +     A read-write single value file which exists on non-root
> > > > > > +     cgroups.  The default is "0".
> > > > > > +
> > > > > > +     The nice value is in the range [-20, 19].
> > > > > > +
> > > > > > +     This interface file allows reading and setting latency using the
> > > > > > +     same values used by sched_setattr(2).
> > > > >
> > > > > I'm still not sure about this [1].
> > > >
> > > > I'm still not sure about what you are trying to say here ...
> > > >
> > > > This is about setting a latency nice prio to a group level.
> > > >
> > > > >
> > > > > In some scenarios we'd like to get the effective latency_nice of the task. How
> > > > > will the task inherit the cgroup value or be impacted by it?
> > > > >
> > > > > For example if there are tasks that belong to a latency sensitive cgroup; and
> > > > > I'd like to skip some searches in EAS to improve that latency sensitivity - how
> > > > > would I extract this info in EAS path given these tasks are using default
> > > > > latency_nice value? And if should happen if their latency_nice is set to
> > > > > something else other than default?
> > > > >
> > > > > [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/
> > > >
> > > > Hmm so you are speaking about something that is not part of the patch.
> > > > Let focus on the patchset for now
> > >
> > > I am focusing on this patchset. Isn't this an essential part of the design?
> > > Once the interface is out we can't change it. As it stands, I can't see how it
> >
> > So, are you speaking about the interface i.e. setting a value between [-20:19]
> >
> > > can be used to replace prefer_idle in cgroup as used in Android. I can't see
> > > how this could happen if we don't define how the task will inherit the cgroup
> > > value. If we can, mind elaborating how please?
> >
> > Or how to take into account the value set for a cgroup ?
> >
> > Regarding the behavior, the rule remains the same that a sched_entity
> > attached to a cgroup will not get more (latency in this case) than
> > what has been set for the group entity.
>
> I think the interface solves a different problem which is latency of
> task or cgroup wrt other group. Vincent, you are setting this for a
> “top app” group in android in your tests, and seeing improvement
> correct? AFAICS, this improvement comes because of lower latency

Yes Top app and display group

> during *same CPU* competition between different groups by juggling
> around the wakeup-preemption window -- which maybe is good for
> Android.
>
> OTOH, the “prefer idle” flag in android that Qais is referring to,
> will need a completely different method as I cannot see how a nice
> value can communicate that (that can complement Vincent's changes
> here). And it will need to have a per-task interface as well. We have

Why a negative latency_nice value condition can't be used ? or latency -20  ?

> something in ChromeOS as well, which is a proc knob and also
> out-of-tree patch for that [1]. Without [1] we fail Android CTS
> testing on a recent ARM64 ChromeOS device.
> [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3884575
> The changelog in [1] also has a detailed description of the ChromeOS usecase.
>
> Qais, any other reason you can see why Vincent's change will not be a
> good thing for Android? Since you 1 CGroup for the whole user-facing
> app (top app), you can just set that to a low "latency_nice" and get
> better wake-up latency for that.
>
> (Side rant about latency and CFS -- IMHO a better long term solution
> for lower latency is to use RT but don't throttle -- rather demote. Or
> break CFS into multiple tiers, and apply demotion. This is in a way
> what Vincent is doing, as the task becomes more CPU bound'ish, he's
> taking away the latency boost. Vincent/Qais, somebody was working on
> the RT demotion vs throttling a while back, any idea on the latest on
> that?).
>
> thanks,
>
>  - Joel
>
>
> >
> > >
> > >
> > > Thanks
> > >
> > > --
> > > Qais Yousef

Re: [PATCH v7 6/9] sched/fair: Add sched group latency support

[Joel Fernandes]

On Fri, Nov 4, 2022 at 10:37 AM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
[...]
> > during *same CPU* competition between different groups by juggling
> > around the wakeup-preemption window -- which maybe is good for
> > Android.
> >
> > OTOH, the “prefer idle” flag in android that Qais is referring to,
> > will need a completely different method as I cannot see how a nice
> > value can communicate that (that can complement Vincent's changes
> > here). And it will need to have a per-task interface as well. We have
>
> Why a negative latency_nice value condition can't be used ? or latency -20  ?

That's overloading the meaning of a value, the whole nice thing is
supposed to be "relative to something". So you are being nice to
something else. Here -20 means you are not being nice. But in fact you
are, because you are avoiding hurting something else by going to an
idle CPU. So it becomes really weird.

Also, why would -19 or -18 not be a value instead to cause wakeup to
prefer an idle CPU? It confuses the user on how to choose value and we
should refrain from that IMHO.

Re: [PATCH v7 6/9] sched/fair: Add sched group latency support

[Joel Fernandes]

> On Nov 4, 2022, at 6:37 AM, Vincent Guittot <vincent.guittot@linaro.org> wrote:
> 
> On Fri, 4 Nov 2022 at 11:15, Joel Fernandes <joel@joelfernandes.org> wrote:
>> 
>>> On Thu, Nov 3, 2022 at 5:03 PM Vincent Guittot
>>> <vincent.guittot@linaro.org> wrote:
>>> 
>>> On Thu, 3 Nov 2022 at 15:27, Qais Yousef <qyousef@layalina.io> wrote:
>>>> 
>>>> On 11/03/22 09:46, Vincent Guittot wrote:
>>>>> On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote:
>>>>>> 
>>>>>> On 10/28/22 11:34, Vincent Guittot wrote:
>>>>>>> Task can set its latency priority with sched_setattr(), which is then used
>>>>>>> to set the latency offset of its sched_enity, but sched group entities
>>>>>>> still have the default latency offset value.
>>>>>>> 
>>>>>>> Add a latency.nice field in cpu cgroup controller to set the latency
>>>>>>> priority of the group similarly to sched_setattr(). The latency priority
>>>>>>> is then used to set the offset of the sched_entities of the group.
>>>>>>> 
>>>>>>> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
>>>>>>> ---
>>>>>>> Documentation/admin-guide/cgroup-v2.rst |  8 ++++
>>>>>>> kernel/sched/core.c                     | 52 +++++++++++++++++++++++++
>>>>>>> kernel/sched/fair.c                     | 33 ++++++++++++++++
>>>>>>> kernel/sched/sched.h                    |  4 ++
>>>>>>> 4 files changed, 97 insertions(+)
>>>>>>> 
>>>>>>> diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
>>>>>>> index be4a77baf784..d8ae7e411f9c 100644
>>>>>>> --- a/Documentation/admin-guide/cgroup-v2.rst
>>>>>>> +++ b/Documentation/admin-guide/cgroup-v2.rst
>>>>>>> @@ -1095,6 +1095,14 @@ All time durations are in microseconds.
>>>>>>>         values similar to the sched_setattr(2). This maximum utilization
>>>>>>>         value is used to clamp the task specific maximum utilization clamp.
>>>>>>> 
>>>>>>> +  cpu.latency.nice
>>>>>>> +     A read-write single value file which exists on non-root
>>>>>>> +     cgroups.  The default is "0".
>>>>>>> +
>>>>>>> +     The nice value is in the range [-20, 19].
>>>>>>> +
>>>>>>> +     This interface file allows reading and setting latency using the
>>>>>>> +     same values used by sched_setattr(2).
>>>>>> 
>>>>>> I'm still not sure about this [1].
>>>>> 
>>>>> I'm still not sure about what you are trying to say here ...
>>>>> 
>>>>> This is about setting a latency nice prio to a group level.
>>>>> 
>>>>>> 
>>>>>> In some scenarios we'd like to get the effective latency_nice of the task. How
>>>>>> will the task inherit the cgroup value or be impacted by it?
>>>>>> 
>>>>>> For example if there are tasks that belong to a latency sensitive cgroup; and
>>>>>> I'd like to skip some searches in EAS to improve that latency sensitivity - how
>>>>>> would I extract this info in EAS path given these tasks are using default
>>>>>> latency_nice value? And if should happen if their latency_nice is set to
>>>>>> something else other than default?
>>>>>> 
>>>>>> [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/
>>>>> 
>>>>> Hmm so you are speaking about something that is not part of the patch.
>>>>> Let focus on the patchset for now
>>>> 
>>>> I am focusing on this patchset. Isn't this an essential part of the design?
>>>> Once the interface is out we can't change it. As it stands, I can't see how it
>>> 
>>> So, are you speaking about the interface i.e. setting a value between [-20:19]
>>> 
>>>> can be used to replace prefer_idle in cgroup as used in Android. I can't see
>>>> how this could happen if we don't define how the task will inherit the cgroup
>>>> value. If we can, mind elaborating how please?
>>> 
>>> Or how to take into account the value set for a cgroup ?
>>> 
>>> Regarding the behavior, the rule remains the same that a sched_entity
>>> attached to a cgroup will not get more (latency in this case) than
>>> what has been set for the group entity.
>> 
>> I think the interface solves a different problem which is latency of
>> task or cgroup wrt other group. Vincent, you are setting this for a
>> “top app” group in android in your tests, and seeing improvement
>> correct? AFAICS, this improvement comes because of lower latency
> 
> Yes Top app and display group
> 
>> during *same CPU* competition between different groups by juggling
>> around the wakeup-preemption window -- which maybe is good for
>> Android.
>> 
>> OTOH, the “prefer idle” flag in android that Qais is referring to,
>> will need a completely different method as I cannot see how a nice
>> value can communicate that (that can complement Vincent's changes
>> here). And it will need to have a per-task interface as well. We have
> 
> Why a negative latency_nice value condition can't be used ? or latency -20  ?

Ah and forgot to reply about negative.

Maybe, but it’s still a horrible overload of the meaning of the value. I am not terribly against choosing negative value if there is consensus among everyone. Qais?

- Joel


> 
>> something in ChromeOS as well, which is a proc knob and also
>> out-of-tree patch for that [1]. Without [1] we fail Android CTS
>> testing on a recent ARM64 ChromeOS device.
>> [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3884575
>> The changelog in [1] also has a detailed description of the ChromeOS usecase.
>> 
>> Qais, any other reason you can see why Vincent's change will not be a
>> good thing for Android? Since you 1 CGroup for the whole user-facing
>> app (top app), you can just set that to a low "latency_nice" and get
>> better wake-up latency for that.
>> 
>> (Side rant about latency and CFS -- IMHO a better long term solution
>> for lower latency is to use RT but don't throttle -- rather demote. Or
>> break CFS into multiple tiers, and apply demotion. This is in a way
>> what Vincent is doing, as the task becomes more CPU bound'ish, he's
>> taking away the latency boost. Vincent/Qais, somebody was working on
>> the RT demotion vs throttling a while back, any idea on the latest on
>> that?).
>> 
>> thanks,
>> 
>> - Joel
>> 
>> 
>>> 
>>>> 
>>>> 
>>>> Thanks
>>>> 
>>>> --
>>>> Qais Yousef

Re: [PATCH v7 6/9] sched/fair: Add sched group latency support

[Vincent Guittot]

On Fri, 4 Nov 2022 at 11:48, Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Fri, Nov 4, 2022 at 10:37 AM Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> [...]
> > > during *same CPU* competition between different groups by juggling
> > > around the wakeup-preemption window -- which maybe is good for
> > > Android.
> > >
> > > OTOH, the “prefer idle” flag in android that Qais is referring to,
> > > will need a completely different method as I cannot see how a nice
> > > value can communicate that (that can complement Vincent's changes
> > > here). And it will need to have a per-task interface as well. We have
> >
> > Why a negative latency_nice value condition can't be used ? or latency -20  ?
>
> That's overloading the meaning of a value, the whole nice thing is
> supposed to be "relative to something". So you are being nice to
> something else. Here -20 means you are not being nice. But in fact you
> are, because you are avoiding hurting something else by going to an
> idle CPU. So it becomes really weird.

Looking for an idle CPU 1st is already the default behavior of CFS.
Here we speak about an EAS specific behavior where we want to forgot
the "full" EAS policy for some tasks and favor latency by spreading
and looking for an idle cpu

>
> Also, why would -19 or -18 not be a value instead to cause wakeup to
> prefer an idle CPU? It confuses the user on how to choose value and we
> should refrain from that IMHO.

IIRC, the 1st idea was to say any negative value but then using the
lowest one can be seen as an addon to the wakeup preemption

Re: [PATCH v7 6/9] sched/fair: Add sched group latency support

[Qais Yousef]

On 11/03/22 18:02, Vincent Guittot wrote:
> On Thu, 3 Nov 2022 at 15:27, Qais Yousef <qyousef@layalina.io> wrote:
> >
> > On 11/03/22 09:46, Vincent Guittot wrote:
> > > On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote:
> > > >
> > > > On 10/28/22 11:34, Vincent Guittot wrote:
> > > > > Task can set its latency priority with sched_setattr(), which is then used
> > > > > to set the latency offset of its sched_enity, but sched group entities
> > > > > still have the default latency offset value.
> > > > >
> > > > > Add a latency.nice field in cpu cgroup controller to set the latency
> > > > > priority of the group similarly to sched_setattr(). The latency priority
> > > > > is then used to set the offset of the sched_entities of the group.
> > > > >
> > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > > > ---
> > > > >  Documentation/admin-guide/cgroup-v2.rst |  8 ++++
> > > > >  kernel/sched/core.c                     | 52 +++++++++++++++++++++++++
> > > > >  kernel/sched/fair.c                     | 33 ++++++++++++++++
> > > > >  kernel/sched/sched.h                    |  4 ++
> > > > >  4 files changed, 97 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> > > > > index be4a77baf784..d8ae7e411f9c 100644
> > > > > --- a/Documentation/admin-guide/cgroup-v2.rst
> > > > > +++ b/Documentation/admin-guide/cgroup-v2.rst
> > > > > @@ -1095,6 +1095,14 @@ All time durations are in microseconds.
> > > > >          values similar to the sched_setattr(2). This maximum utilization
> > > > >          value is used to clamp the task specific maximum utilization clamp.
> > > > >
> > > > > +  cpu.latency.nice
> > > > > +     A read-write single value file which exists on non-root
> > > > > +     cgroups.  The default is "0".
> > > > > +
> > > > > +     The nice value is in the range [-20, 19].
> > > > > +
> > > > > +     This interface file allows reading and setting latency using the
> > > > > +     same values used by sched_setattr(2).
> > > >
> > > > I'm still not sure about this [1].
> > >
> > > I'm still not sure about what you are trying to say here ...
> > >
> > > This is about setting a latency nice prio to a group level.
> > >
> > > >
> > > > In some scenarios we'd like to get the effective latency_nice of the task. How
> > > > will the task inherit the cgroup value or be impacted by it?
> > > >
> > > > For example if there are tasks that belong to a latency sensitive cgroup; and
> > > > I'd like to skip some searches in EAS to improve that latency sensitivity - how
> > > > would I extract this info in EAS path given these tasks are using default
> > > > latency_nice value? And if should happen if their latency_nice is set to
> > > > something else other than default?
> > > >
> > > > [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/
> > >
> > > Hmm so you are speaking about something that is not part of the patch.
> > > Let focus on the patchset for now
> >
> > I am focusing on this patchset. Isn't this an essential part of the design?
> > Once the interface is out we can't change it. As it stands, I can't see how it
> 
> So, are you speaking about the interface i.e. setting a value between [-20:19]

About how the cgroup and per task interface interact.

How to get the effective value of latency_nice for a task that belongs to
a hierarchy?

If I have a task that has p->latency_nice = 20 but it belongs to a cgroup that
has tg->cpu.latency.nice = -19

And I want to use this interface in EAS; how should I interpret these values?
How should I walk up the hierarchy and decide the _effective_ latency_nice
value?

> 
> > can be used to replace prefer_idle in cgroup as used in Android. I can't see
> > how this could happen if we don't define how the task will inherit the cgroup
> > value. If we can, mind elaborating how please?
> 
> Or how to take into account the value set for a cgroup ?

Yes that. How to calculate effective value in a hierarchy taking parents,
children and task latency_nice values into account.

> 
> Regarding the behavior, the rule remains the same that a sched_entity
> attached to a cgroup will not get more (latency in this case) than
> what has been set for the group entity.

So it behaves like a limit as described in cgroup-v2.rst? Is this enforced in
the series?


Thanks

--
Qais Yousef

> 
> >
> >
> > Thanks
> >
> > --
> > Qais Yousef

Re: [PATCH v7 6/9] sched/fair: Add sched group latency support

[Qais Yousef]

On 11/04/22 10:14, Joel Fernandes wrote:

> I think the interface solves a different problem which is latency of
> task or cgroup wrt other group. Vincent, you are setting this for a
> “top app” group in android in your tests, and seeing improvement
> correct? AFAICS, this improvement comes because of lower latency
> during *same CPU* competition between different groups by juggling
> around the wakeup-preemption window -- which maybe is good for
> Android.
> 
> OTOH, the “prefer idle” flag in android that Qais is referring to,
> will need a completely different method as I cannot see how a nice
> value can communicate that (that can complement Vincent's changes
> here). And it will need to have a per-task interface as well. We have
> something in ChromeOS as well, which is a proc knob and also
> out-of-tree patch for that [1]. Without [1] we fail Android CTS
> testing on a recent ARM64 ChromeOS device.
> [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3884575
> The changelog in [1] also has a detailed description of the ChromeOS usecase.
> 
> Qais, any other reason you can see why Vincent's change will not be a
> good thing for Android? Since you 1 CGroup for the whole user-facing
> app (top app), you can just set that to a low "latency_nice" and get
> better wake-up latency for that.

There are two things to consider here:

	1. The interface and its extensibility.
	2. Current use case of improving wake up latency by manipulating
	   vruntime.

(2) looks promising approach, but it's very hard to know if it can be
considered a replacement. So (1) must be spot on to not block adding other
consumers in EAS or anywhere else in the scheduler for what matters.

> 
> (Side rant about latency and CFS -- IMHO a better long term solution
> for lower latency is to use RT but don't throttle -- rather demote. Or
> break CFS into multiple tiers, and apply demotion. This is in a way
> what Vincent is doing, as the task becomes more CPU bound'ish, he's
> taking away the latency boost. Vincent/Qais, somebody was working on
> the RT demotion vs throttling a while back, any idea on the latest on
> that?).

I can see an appetite in the future for userspace to provide more hints about
the characteristics of the tasks to improve performance and power.

I'm starting to lean towards having a framework to encapsulate such description
where latency_nice and potentially new notion of priority or something else can
be part of.

One aspect of prefer_idle in android for instance is that it disables the
packing behavior of EAS. I've seen Prateek showing similar examples in [1]
(FORK_SPREAD).

I don't have a clear idea in my head, but I'm slowly starting to lean towards
the need for a proper QoS/hint framework to describe the characteristic of the
task or a collection of tasks working together. My pony wish for now I guess
:-)

[1] https://lore.kernel.org/lkml/20220910105326.1797-1-kprateek.nayak@amd.com/


Thanks

--
Qais Yousef

Re: [PATCH v7 6/9] sched/fair: Add sched group latency support

[Qais Yousef]

On 11/04/22 06:55, Joel Fernandes wrote:

> >> I think the interface solves a different problem which is latency of task
> >> or cgroup wrt other group. Vincent, you are setting this for a “top app”
> >> group in android in your tests, and seeing improvement correct? AFAICS,
> >> this improvement comes because of lower latency
> > 
> > Yes Top app and display group
> > 
> >> during *same CPU* competition between different groups by juggling around
> >> the wakeup-preemption window -- which maybe is good for Android.
> >> 
> >> OTOH, the “prefer idle” flag in android that Qais is referring to, will
> >> need a completely different method as I cannot see how a nice value can
> >> communicate that (that can complement Vincent's changes here). And it will
> >> need to have a per-task interface as well. We have
> > 
> > Why a negative latency_nice value condition can't be used ? or latency -20
> > ?
> 
> Ah and forgot to reply about negative.
> 
> Maybe, but it’s still a horrible overload of the meaning of the value. I am
> not terribly against choosing negative value if there is consensus among
> everyone. Qais?

TBH I think the whole notion of 'nice' is confusing. From my experience talking
with some game developers they didn't know how to use nice values or what they
exactly mean. Given their target audience is a large diverse range of devices
with different spec, and that they can only lower it as increasing it requires
privilege; how to decide the right value to work reliably everywhere?

latency_nice might suffer from the same problem. But I don't have a better
alternative to suggest. I think that was the worrying input from Peter and
Thomas; these numbers could appear like magic crystal ball from users
perspective but I haven't seen any new discussion in spite my attempt to stir
it.

As we brought up in another email thread; I think CFS notion of priority could
be improved and it could lead to improve this problem by product. But this
won't address problems like load balance search times that were brought up as
part of this latency discussions in the past.

So short answer is I don't know :) I think the interface and use cases should
be discussed more still.


Thanks

--
Qais Yousef

Re: [PATCH v7 6/9] sched/fair: Add sched group latency support

[Joel Fernandes]

> On Nov 4, 2022, at 7:21 AM, Qais Yousef <qyousef@layalina.io> wrote:
> 
> On 11/03/22 18:02, Vincent Guittot wrote:
>>> On Thu, 3 Nov 2022 at 15:27, Qais Yousef <qyousef@layalina.io> wrote:
>>> 
>>> On 11/03/22 09:46, Vincent Guittot wrote:
>>>> On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote:
>>>>> 
>>>>> On 10/28/22 11:34, Vincent Guittot wrote:
>>>>>> Task can set its latency priority with sched_setattr(), which is then used
>>>>>> to set the latency offset of its sched_enity, but sched group entities
>>>>>> still have the default latency offset value.
>>>>>> 
>>>>>> Add a latency.nice field in cpu cgroup controller to set the latency
>>>>>> priority of the group similarly to sched_setattr(). The latency priority
>>>>>> is then used to set the offset of the sched_entities of the group.
>>>>>> 
>>>>>> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
>>>>>> ---
>>>>>> Documentation/admin-guide/cgroup-v2.rst |  8 ++++
>>>>>> kernel/sched/core.c                     | 52 +++++++++++++++++++++++++
>>>>>> kernel/sched/fair.c                     | 33 ++++++++++++++++
>>>>>> kernel/sched/sched.h                    |  4 ++
>>>>>> 4 files changed, 97 insertions(+)
>>>>>> 
>>>>>> diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
>>>>>> index be4a77baf784..d8ae7e411f9c 100644
>>>>>> --- a/Documentation/admin-guide/cgroup-v2.rst
>>>>>> +++ b/Documentation/admin-guide/cgroup-v2.rst
>>>>>> @@ -1095,6 +1095,14 @@ All time durations are in microseconds.
>>>>>>         values similar to the sched_setattr(2). This maximum utilization
>>>>>>         value is used to clamp the task specific maximum utilization clamp.
>>>>>> 
>>>>>> +  cpu.latency.nice
>>>>>> +     A read-write single value file which exists on non-root
>>>>>> +     cgroups.  The default is "0".
>>>>>> +
>>>>>> +     The nice value is in the range [-20, 19].
>>>>>> +
>>>>>> +     This interface file allows reading and setting latency using the
>>>>>> +     same values used by sched_setattr(2).
>>>>> 
>>>>> I'm still not sure about this [1].
>>>> 
>>>> I'm still not sure about what you are trying to say here ...
>>>> 
>>>> This is about setting a latency nice prio to a group level.
>>>> 
>>>>> 
>>>>> In some scenarios we'd like to get the effective latency_nice of the task. How
>>>>> will the task inherit the cgroup value or be impacted by it?
>>>>> 
>>>>> For example if there are tasks that belong to a latency sensitive cgroup; and
>>>>> I'd like to skip some searches in EAS to improve that latency sensitivity - how
>>>>> would I extract this info in EAS path given these tasks are using default
>>>>> latency_nice value? And if should happen if their latency_nice is set to
>>>>> something else other than default?
>>>>> 
>>>>> [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/
>>>> 
>>>> Hmm so you are speaking about something that is not part of the patch.
>>>> Let focus on the patchset for now
>>> 
>>> I am focusing on this patchset. Isn't this an essential part of the design?
>>> Once the interface is out we can't change it. As it stands, I can't see how it
>> 
>> So, are you speaking about the interface i.e. setting a value between [-20:19]
> 
> About how the cgroup and per task interface interact.
> 
> How to get the effective value of latency_nice for a task that belongs to
> a hierarchy?
> 
> If I have a task that has p->latency_nice = 20 but it belongs to a cgroup that
> has tg->cpu.latency.nice = -19

Just for the task placement signal, One way is to go through the se hierarchy till the root and get the minimum. Then make that the effective value. So In your example that would make it -19 so prefer idle = 1.  We should need a Boolean signal. Not pretty but not the end of the world imho.

> And I want to use this interface in EAS; how should I interpret these values?
> How should I walk up the hierarchy and decide the _effective_ latency_nice
> value

Ah, using a min function?

Joel



>> 
> Thanks
> 
> --
> Qais Yousef
> 
>> 
>>> 
>>> 
>>> Thanks
>>> 
>>> --
>>> Qais Yousef

Re: [PATCH v7 6/9] sched/fair: Add sched group latency support

[Vincent Guittot]

On Fri, 4 Nov 2022 at 12:21, Qais Yousef <qyousef@layalina.io> wrote:
>
> On 11/03/22 18:02, Vincent Guittot wrote:
> > On Thu, 3 Nov 2022 at 15:27, Qais Yousef <qyousef@layalina.io> wrote:
> > >
> > > On 11/03/22 09:46, Vincent Guittot wrote:
> > > > On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote:
> > > > >
> > > > > On 10/28/22 11:34, Vincent Guittot wrote:
> > > > > > Task can set its latency priority with sched_setattr(), which is then used
> > > > > > to set the latency offset of its sched_enity, but sched group entities
> > > > > > still have the default latency offset value.
> > > > > >
> > > > > > Add a latency.nice field in cpu cgroup controller to set the latency
> > > > > > priority of the group similarly to sched_setattr(). The latency priority
> > > > > > is then used to set the offset of the sched_entities of the group.
> > > > > >
> > > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > > > > ---
> > > > > >  Documentation/admin-guide/cgroup-v2.rst |  8 ++++
> > > > > >  kernel/sched/core.c                     | 52 +++++++++++++++++++++++++
> > > > > >  kernel/sched/fair.c                     | 33 ++++++++++++++++
> > > > > >  kernel/sched/sched.h                    |  4 ++
> > > > > >  4 files changed, 97 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> > > > > > index be4a77baf784..d8ae7e411f9c 100644
> > > > > > --- a/Documentation/admin-guide/cgroup-v2.rst
> > > > > > +++ b/Documentation/admin-guide/cgroup-v2.rst
> > > > > > @@ -1095,6 +1095,14 @@ All time durations are in microseconds.
> > > > > >          values similar to the sched_setattr(2). This maximum utilization
> > > > > >          value is used to clamp the task specific maximum utilization clamp.
> > > > > >
> > > > > > +  cpu.latency.nice
> > > > > > +     A read-write single value file which exists on non-root
> > > > > > +     cgroups.  The default is "0".
> > > > > > +
> > > > > > +     The nice value is in the range [-20, 19].
> > > > > > +
> > > > > > +     This interface file allows reading and setting latency using the
> > > > > > +     same values used by sched_setattr(2).
> > > > >
> > > > > I'm still not sure about this [1].
> > > >
> > > > I'm still not sure about what you are trying to say here ...
> > > >
> > > > This is about setting a latency nice prio to a group level.
> > > >
> > > > >
> > > > > In some scenarios we'd like to get the effective latency_nice of the task. How
> > > > > will the task inherit the cgroup value or be impacted by it?
> > > > >
> > > > > For example if there are tasks that belong to a latency sensitive cgroup; and
> > > > > I'd like to skip some searches in EAS to improve that latency sensitivity - how
> > > > > would I extract this info in EAS path given these tasks are using default
> > > > > latency_nice value? And if should happen if their latency_nice is set to
> > > > > something else other than default?
> > > > >
> > > > > [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/
> > > >
> > > > Hmm so you are speaking about something that is not part of the patch.
> > > > Let focus on the patchset for now
> > >
> > > I am focusing on this patchset. Isn't this an essential part of the design?
> > > Once the interface is out we can't change it. As it stands, I can't see how it
> >
> > So, are you speaking about the interface i.e. setting a value between [-20:19]
>
> About how the cgroup and per task interface interact.
>
> How to get the effective value of latency_nice for a task that belongs to
> a hierarchy?

At the common parents level of the 2 entities that you want to compare
and root level if there no other entity to compare with

>
> If I have a task that has p->latency_nice = 20 but it belongs to a cgroup that
> has tg->cpu.latency.nice = -19

according to what i said above, latency_nice = 20 inside the group and
-19 when comparing at the parent level

>
> And I want to use this interface in EAS; how should I interpret these values?
> How should I walk up the hierarchy and decide the _effective_ latency_nice
> value?

The current use of latency_nice doesn't need to walk the hierarchy
because it applies at each scheduling level so the childs
automatically follow parents' latency.

>
> >
> > > can be used to replace prefer_idle in cgroup as used in Android. I can't see
> > > how this could happen if we don't define how the task will inherit the cgroup
> > > value. If we can, mind elaborating how please?
> >
> > Or how to take into account the value set for a cgroup ?
>
> Yes that. How to calculate effective value in a hierarchy taking parents,
> children and task latency_nice values into account.
>
> >
> > Regarding the behavior, the rule remains the same that a sched_entity
> > attached to a cgroup will not get more (latency in this case) than
> > what has been set for the group entity.
>
> So it behaves like a limit as described in cgroup-v2.rst? Is this enforced in
> the series?

It's like a limit and this is enforced in this serie as we are
applying the latency at each level before moving to the next one

>
>
> Thanks
>
> --
> Qais Yousef
>
> >
> > >
> > >
> > > Thanks
> > >
> > > --
> > > Qais Yousef

Re: [PATCH v7 6/9] sched/fair: Add sched group latency support

[Joel Fernandes]

On Fri, Nov 04, 2022 at 03:24:23PM +0100, Vincent Guittot wrote:
> On Fri, 4 Nov 2022 at 12:21, Qais Yousef <qyousef@layalina.io> wrote:
> >
> > On 11/03/22 18:02, Vincent Guittot wrote:
> > > On Thu, 3 Nov 2022 at 15:27, Qais Yousef <qyousef@layalina.io> wrote:
> > > >
> > > > On 11/03/22 09:46, Vincent Guittot wrote:
> > > > > On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote:
> > > > > >
> > > > > > On 10/28/22 11:34, Vincent Guittot wrote:
> > > > > > > Task can set its latency priority with sched_setattr(), which is then used
> > > > > > > to set the latency offset of its sched_enity, but sched group entities
> > > > > > > still have the default latency offset value.
> > > > > > >
> > > > > > > Add a latency.nice field in cpu cgroup controller to set the latency
> > > > > > > priority of the group similarly to sched_setattr(). The latency priority
> > > > > > > is then used to set the offset of the sched_entities of the group.
> > > > > > >
> > > > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > > > > > ---
> > > > > > >  Documentation/admin-guide/cgroup-v2.rst |  8 ++++
> > > > > > >  kernel/sched/core.c                     | 52 +++++++++++++++++++++++++
> > > > > > >  kernel/sched/fair.c                     | 33 ++++++++++++++++
> > > > > > >  kernel/sched/sched.h                    |  4 ++
> > > > > > >  4 files changed, 97 insertions(+)
> > > > > > >
> > > > > > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> > > > > > > index be4a77baf784..d8ae7e411f9c 100644
> > > > > > > --- a/Documentation/admin-guide/cgroup-v2.rst
> > > > > > > +++ b/Documentation/admin-guide/cgroup-v2.rst
> > > > > > > @@ -1095,6 +1095,14 @@ All time durations are in microseconds.
> > > > > > >          values similar to the sched_setattr(2). This maximum utilization
> > > > > > >          value is used to clamp the task specific maximum utilization clamp.
> > > > > > >
> > > > > > > +  cpu.latency.nice
> > > > > > > +     A read-write single value file which exists on non-root
> > > > > > > +     cgroups.  The default is "0".
> > > > > > > +
> > > > > > > +     The nice value is in the range [-20, 19].
> > > > > > > +
> > > > > > > +     This interface file allows reading and setting latency using the
> > > > > > > +     same values used by sched_setattr(2).
> > > > > >
> > > > > > I'm still not sure about this [1].
> > > > >
> > > > > I'm still not sure about what you are trying to say here ...
> > > > >
> > > > > This is about setting a latency nice prio to a group level.
> > > > >
> > > > > >
> > > > > > In some scenarios we'd like to get the effective latency_nice of the task. How
> > > > > > will the task inherit the cgroup value or be impacted by it?
> > > > > >
> > > > > > For example if there are tasks that belong to a latency sensitive cgroup; and
> > > > > > I'd like to skip some searches in EAS to improve that latency sensitivity - how
> > > > > > would I extract this info in EAS path given these tasks are using default
> > > > > > latency_nice value? And if should happen if their latency_nice is set to
> > > > > > something else other than default?
> > > > > >
> > > > > > [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/
> > > > >
> > > > > Hmm so you are speaking about something that is not part of the patch.
> > > > > Let focus on the patchset for now
> > > >
> > > > I am focusing on this patchset. Isn't this an essential part of the design?
> > > > Once the interface is out we can't change it. As it stands, I can't see how it
> > >
> > > So, are you speaking about the interface i.e. setting a value between [-20:19]
> >
> > About how the cgroup and per task interface interact.
> >
> > How to get the effective value of latency_nice for a task that belongs to
> > a hierarchy?
> 
> At the common parents level of the 2 entities that you want to compare
> and root level if there no other entity to compare with
> 
> >
> > If I have a task that has p->latency_nice = 20 but it belongs to a cgroup that
> > has tg->cpu.latency.nice = -19
> 
> according to what i said above, latency_nice = 20 inside the group and
> -19 when comparing at the parent level
> 
> >
> > And I want to use this interface in EAS; how should I interpret these values?
> > How should I walk up the hierarchy and decide the _effective_ latency_nice
> > value?
> 
> The current use of latency_nice doesn't need to walk the hierarchy
> because it applies at each scheduling level so the childs
> automatically follow parents' latency.

Not really, I don't see how that will work that way in the wake up path. The
wake up path (EAS in particular) does not walk through CPU controller group
hierarchy from top level, it only cares about cpuset/affinities and the
"effective" values of tasks.

So when you wake up a task, how will you retrieve the attribute for 'prefer
idle' in the wakeup path using this patchset? The only way is to aggregate
the CGroup hierarchy information to get a per-task effective value; say using
a min function.

If you see uclamp_rq_util_with(), that also is using doing uclamp
aggregation similarly.

So I think Qais is asking about the aggregation function in the EAS wakeup
path.

Thanks.

Re: [PATCH v7 6/9] sched/fair: Add sched group latency support

[Vincent Guittot]

On Fri, 4 Nov 2022 at 15:57, Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Fri, Nov 04, 2022 at 03:24:23PM +0100, Vincent Guittot wrote:
> > On Fri, 4 Nov 2022 at 12:21, Qais Yousef <qyousef@layalina.io> wrote:
> > >
> > > On 11/03/22 18:02, Vincent Guittot wrote:
> > > > On Thu, 3 Nov 2022 at 15:27, Qais Yousef <qyousef@layalina.io> wrote:
> > > > >
> > > > > On 11/03/22 09:46, Vincent Guittot wrote:
> > > > > > On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote:
> > > > > > >
> > > > > > > On 10/28/22 11:34, Vincent Guittot wrote:
> > > > > > > > Task can set its latency priority with sched_setattr(), which is then used
> > > > > > > > to set the latency offset of its sched_enity, but sched group entities
> > > > > > > > still have the default latency offset value.
> > > > > > > >
> > > > > > > > Add a latency.nice field in cpu cgroup controller to set the latency
> > > > > > > > priority of the group similarly to sched_setattr(). The latency priority
> > > > > > > > is then used to set the offset of the sched_entities of the group.
> > > > > > > >
> > > > > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > > > > > > ---
> > > > > > > >  Documentation/admin-guide/cgroup-v2.rst |  8 ++++
> > > > > > > >  kernel/sched/core.c                     | 52 +++++++++++++++++++++++++
> > > > > > > >  kernel/sched/fair.c                     | 33 ++++++++++++++++
> > > > > > > >  kernel/sched/sched.h                    |  4 ++
> > > > > > > >  4 files changed, 97 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> > > > > > > > index be4a77baf784..d8ae7e411f9c 100644
> > > > > > > > --- a/Documentation/admin-guide/cgroup-v2.rst
> > > > > > > > +++ b/Documentation/admin-guide/cgroup-v2.rst
> > > > > > > > @@ -1095,6 +1095,14 @@ All time durations are in microseconds.
> > > > > > > >          values similar to the sched_setattr(2). This maximum utilization
> > > > > > > >          value is used to clamp the task specific maximum utilization clamp.
> > > > > > > >
> > > > > > > > +  cpu.latency.nice
> > > > > > > > +     A read-write single value file which exists on non-root
> > > > > > > > +     cgroups.  The default is "0".
> > > > > > > > +
> > > > > > > > +     The nice value is in the range [-20, 19].
> > > > > > > > +
> > > > > > > > +     This interface file allows reading and setting latency using the
> > > > > > > > +     same values used by sched_setattr(2).
> > > > > > >
> > > > > > > I'm still not sure about this [1].
> > > > > >
> > > > > > I'm still not sure about what you are trying to say here ...
> > > > > >
> > > > > > This is about setting a latency nice prio to a group level.
> > > > > >
> > > > > > >
> > > > > > > In some scenarios we'd like to get the effective latency_nice of the task. How
> > > > > > > will the task inherit the cgroup value or be impacted by it?
> > > > > > >
> > > > > > > For example if there are tasks that belong to a latency sensitive cgroup; and
> > > > > > > I'd like to skip some searches in EAS to improve that latency sensitivity - how
> > > > > > > would I extract this info in EAS path given these tasks are using default
> > > > > > > latency_nice value? And if should happen if their latency_nice is set to
> > > > > > > something else other than default?
> > > > > > >
> > > > > > > [1] https://lore.kernel.org/lkml/20221012160734.hrkb5jcjdq7r23pr@wubuntu/
> > > > > >
> > > > > > Hmm so you are speaking about something that is not part of the patch.
> > > > > > Let focus on the patchset for now
> > > > >
> > > > > I am focusing on this patchset. Isn't this an essential part of the design?
> > > > > Once the interface is out we can't change it. As it stands, I can't see how it
> > > >
> > > > So, are you speaking about the interface i.e. setting a value between [-20:19]
> > >
> > > About how the cgroup and per task interface interact.
> > >
> > > How to get the effective value of latency_nice for a task that belongs to
> > > a hierarchy?
> >
> > At the common parents level of the 2 entities that you want to compare
> > and root level if there no other entity to compare with
> >
> > >
> > > If I have a task that has p->latency_nice = 20 but it belongs to a cgroup that
> > > has tg->cpu.latency.nice = -19
> >
> > according to what i said above, latency_nice = 20 inside the group and
> > -19 when comparing at the parent level
> >
> > >
> > > And I want to use this interface in EAS; how should I interpret these values?
> > > How should I walk up the hierarchy and decide the _effective_ latency_nice
> > > value?
> >
> > The current use of latency_nice doesn't need to walk the hierarchy
> > because it applies at each scheduling level so the childs
> > automatically follow parents' latency.
>
> Not really, I don't see how that will work that way in the wake up path. The
> wake up path (EAS in particular) does not walk through CPU controller group
> hierarchy from top level, it only cares about cpuset/affinities and the
> "effective" values of tasks.

I was explaining the current use of latency_ni i.e. in this patchset,
I'm not speaking about what should be done for other case like EAS

In fact, it's exactly what I explained few lines above :
"> > At the common parents level of the 2 entities that you want to compare
> > and root level if there no other entity to compare with"

>
> So when you wake up a task, how will you retrieve the attribute for 'prefer
> idle' in the wakeup path using this patchset? The only way is to aggregate
> the CGroup hierarchy information to get a per-task effective value; say using
> a min function.
>
> If you see uclamp_rq_util_with(), that also is using doing uclamp
> aggregation similarly.
>
> So I think Qais is asking about the aggregation function in the EAS wakeup
> path.
>
> Thanks.

Re: [PATCH v7 6/9] sched/fair: Add sched group latency support

[Joel Fernandes]

On Fri, Nov 4, 2022 at 3:03 PM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
[...]
> > > > > > On 11/03/22 09:46, Vincent Guittot wrote:
> > > > > > > On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote:
> > > > > > > >
> > > > > > > > On 10/28/22 11:34, Vincent Guittot wrote:
> > > > > > > > > Task can set its latency priority with sched_setattr(), which is then used
> > > > > > > > > to set the latency offset of its sched_enity, but sched group entities
> > > > > > > > > still have the default latency offset value.
> > > > > > > > >
> > > > > > > > > Add a latency.nice field in cpu cgroup controller to set the latency
> > > > > > > > > priority of the group similarly to sched_setattr(). The latency priority
> > > > > > > > > is then used to set the offset of the sched_entities of the group.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > > > > > > > ---
> > > > > > > > >  Documentation/admin-guide/cgroup-v2.rst |  8 ++++
> > > > > > > > >  kernel/sched/core.c                     | 52 +++++++++++++++++++++++++
> > > > > > > > >  kernel/sched/fair.c                     | 33 ++++++++++++++++
> > > > > > > > >  kernel/sched/sched.h                    |  4 ++
> > > > > > > > >  4 files changed, 97 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> > > > > > > > > index be4a77baf784..d8ae7e411f9c 100644
> > > > > > > > > --- a/Documentation/admin-guide/cgroup-v2.rst
> > > > > > > > > +++ b/Documentation/admin-guide/cgroup-v2.rst
> > > > > > > > > @@ -1095,6 +1095,14 @@ All time durations are in microseconds.
> > > > > > > > >          values similar to the sched_setattr(2). This maximum utilization
> > > > > > > > >          value is used to clamp the task specific maximum utilization clamp.
> > > > > > > > >
> > > > > > > > > +  cpu.latency.nice
[...]
> > > > > > >
> > > > > > > Hmm so you are speaking about something that is not part of the patch.
> > > > > > > Let focus on the patchset for now
> > > > > >
> > > > > > I am focusing on this patchset. Isn't this an essential part of the design?
> > > > > > Once the interface is out we can't change it. As it stands, I can't see how it
> > > > >
> > > > > So, are you speaking about the interface i.e. setting a value between [-20:19]
> > > >
> > > > About how the cgroup and per task interface interact.
> > > >
> > > > How to get the effective value of latency_nice for a task that belongs to
> > > > a hierarchy?
> > >
> > > At the common parents level of the 2 entities that you want to compare
> > > and root level if there no other entity to compare with
> > >
> > > >
> > > > If I have a task that has p->latency_nice = 20 but it belongs to a cgroup that
> > > > has tg->cpu.latency.nice = -19
> > >
> > > according to what i said above, latency_nice = 20 inside the group and
> > > -19 when comparing at the parent level
> > >
> > > >
> > > > And I want to use this interface in EAS; how should I interpret these values?
> > > > How should I walk up the hierarchy and decide the _effective_ latency_nice
> > > > value?
> > >
> > > The current use of latency_nice doesn't need to walk the hierarchy
> > > because it applies at each scheduling level so the childs
> > > automatically follow parents' latency.
> >
> > Not really, I don't see how that will work that way in the wake up path. The
> > wake up path (EAS in particular) does not walk through CPU controller group
> > hierarchy from top level, it only cares about cpuset/affinities and the
> > "effective" values of tasks.
>
> I was explaining the current use of latency_ni i.e. in this patchset,
> I'm not speaking about what should be done for other case like EAS

That's fine, but you did mention the negative value of latency_nice
used to mark that a task prefers idle CPU so we should finish that
discussion :-D.  Since that will be one of the potential users of this
patchset.

> In fact, it's exactly what I explained few lines above :
> "> > At the common parents level of the 2 entities that you want to compare
> > > and root level if there no other entity to compare with"

Yes this is a different usecase, but having more real world use cases
will add more purpose to the patchset.

I also want to add -- for ChromeOS, Youssef tried it and the temporary
boost that latency_nice gives is not enough for latency-sensitive
workloads (like the main thread of a ChromeOS web page which is both
CPU bound and handles latency-sensitive user input). So we are also
exploring other ways. However, I have no issue with the patchset if it
helps Android and would love to review further.

Thanks.

Re: [PATCH v7 6/9] sched/fair: Add sched group latency support

[Vincent Guittot]

On Fri, 4 Nov 2022 at 16:12, Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Fri, Nov 4, 2022 at 3:03 PM Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> [...]
> > > > > > > On 11/03/22 09:46, Vincent Guittot wrote:
> > > > > > > > On Tue, 1 Nov 2022 at 20:28, Qais Yousef <qyousef@layalina.io> wrote:
> > > > > > > > >
> > > > > > > > > On 10/28/22 11:34, Vincent Guittot wrote:
> > > > > > > > > > Task can set its latency priority with sched_setattr(), which is then used
> > > > > > > > > > to set the latency offset of its sched_enity, but sched group entities
> > > > > > > > > > still have the default latency offset value.
> > > > > > > > > >
> > > > > > > > > > Add a latency.nice field in cpu cgroup controller to set the latency
> > > > > > > > > > priority of the group similarly to sched_setattr(). The latency priority
> > > > > > > > > > is then used to set the offset of the sched_entities of the group.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > > > > > > > > ---
> > > > > > > > > >  Documentation/admin-guide/cgroup-v2.rst |  8 ++++
> > > > > > > > > >  kernel/sched/core.c                     | 52 +++++++++++++++++++++++++
> > > > > > > > > >  kernel/sched/fair.c                     | 33 ++++++++++++++++
> > > > > > > > > >  kernel/sched/sched.h                    |  4 ++
> > > > > > > > > >  4 files changed, 97 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> > > > > > > > > > index be4a77baf784..d8ae7e411f9c 100644
> > > > > > > > > > --- a/Documentation/admin-guide/cgroup-v2.rst
> > > > > > > > > > +++ b/Documentation/admin-guide/cgroup-v2.rst
> > > > > > > > > > @@ -1095,6 +1095,14 @@ All time durations are in microseconds.
> > > > > > > > > >          values similar to the sched_setattr(2). This maximum utilization
> > > > > > > > > >          value is used to clamp the task specific maximum utilization clamp.
> > > > > > > > > >
> > > > > > > > > > +  cpu.latency.nice
> [...]
> > > > > > > >
> > > > > > > > Hmm so you are speaking about something that is not part of the patch.
> > > > > > > > Let focus on the patchset for now
> > > > > > >
> > > > > > > I am focusing on this patchset. Isn't this an essential part of the design?
> > > > > > > Once the interface is out we can't change it. As it stands, I can't see how it
> > > > > >
> > > > > > So, are you speaking about the interface i.e. setting a value between [-20:19]
> > > > >
> > > > > About how the cgroup and per task interface interact.
> > > > >
> > > > > How to get the effective value of latency_nice for a task that belongs to
> > > > > a hierarchy?
> > > >
> > > > At the common parents level of the 2 entities that you want to compare
> > > > and root level if there no other entity to compare with
> > > >
> > > > >
> > > > > If I have a task that has p->latency_nice = 20 but it belongs to a cgroup that
> > > > > has tg->cpu.latency.nice = -19
> > > >
> > > > according to what i said above, latency_nice = 20 inside the group and
> > > > -19 when comparing at the parent level
> > > >
> > > > >
> > > > > And I want to use this interface in EAS; how should I interpret these values?
> > > > > How should I walk up the hierarchy and decide the _effective_ latency_nice
> > > > > value?
> > > >
> > > > The current use of latency_nice doesn't need to walk the hierarchy
> > > > because it applies at each scheduling level so the childs
> > > > automatically follow parents' latency.
> > >
> > > Not really, I don't see how that will work that way in the wake up path. The
> > > wake up path (EAS in particular) does not walk through CPU controller group
> > > hierarchy from top level, it only cares about cpuset/affinities and the
> > > "effective" values of tasks.
> >
> > I was explaining the current use of latency_ni i.e. in this patchset,
> > I'm not speaking about what should be done for other case like EAS
>
> That's fine, but you did mention the negative value of latency_nice
> used to mark that a task prefers idle CPU so we should finish that
> discussion :-D.  Since that will be one of the potential users of this
> patchset.

We should finish reviewing this patchset 1st. Then, we can discuss if
we should use -19, <0 or something else. This doesn't add any value to
this patchset IMO

>
> > In fact, it's exactly what I explained few lines above :
> > "> > At the common parents level of the 2 entities that you want to compare
> > > > and root level if there no other entity to compare with"
>
> Yes this is a different usecase, but having more real world use cases
> will add more purpose to the patchset.

As long as the policy is defined and I think it is defined, we are
fine and it's not the purpose of discussing implementation details of
potential other use. Once this patchset is merged, I will be more than
happy to prepare another one to make use of latency_nice in EAS and we
can discuss further based on it.


>
> I also want to add -- for ChromeOS, Youssef tried it and the temporary
> boost that latency_nice gives is not enough for latency-sensitive
> workloads (like the main thread of a ChromeOS web page which is both
> CPU bound and handles latency-sensitive user input). So we are also
> exploring other ways. However, I have no issue with the patchset if it
> helps Android and would love to review further.
>
> Thanks.

Re: [PATCH v7 6/9] sched/fair: Add sched group latency support

[Qais Yousef]

On 11/04/22 14:57, Joel Fernandes wrote:

> > The current use of latency_nice doesn't need to walk the hierarchy
> > because it applies at each scheduling level so the childs
> > automatically follow parents' latency.
> 
> Not really, I don't see how that will work that way in the wake up path. The
> wake up path (EAS in particular) does not walk through CPU controller group
> hierarchy from top level, it only cares about cpuset/affinities and the
> "effective" values of tasks.
> 
> So when you wake up a task, how will you retrieve the attribute for 'prefer
> idle' in the wakeup path using this patchset? The only way is to aggregate
> the CGroup hierarchy information to get a per-task effective value; say using
> a min function.
> 
> If you see uclamp_rq_util_with(), that also is using doing uclamp
> aggregation similarly.
> 
> So I think Qais is asking about the aggregation function in the EAS wakeup
> path.

Yes that's what I was trying to say. Thanks for helping to clarify it!


Thanks

--
Qais Yousef

Re: [PATCH v7 6/9] sched/fair: Add sched group latency support

[Qais Yousef]

On 11/04/22 09:13, Joel Fernandes wrote:

> > If I have a task that has p->latency_nice = 20 but it belongs to a cgroup
> > that has tg->cpu.latency.nice = -19
> 
> Just for the task placement signal, One way is to go through the se hierarchy
> till the root and get the minimum. Then make that the effective value. So In
> your example that would make it -19 so prefer idle = 1.  We should need
> a Boolean signal. Not pretty but not the end of the world imho.

It is not hard to hack something. My worry is about consistency; and
maintainers in the future saying that doesn't fit the current design.

I'd love for this to be usable everywhere as-is. That requires the expectations
for both users and consumers are being made clear from the beginning.

What I was asking for is for the documentation to reflect this, and the
implementation of this effective function being made available from the start.


Cheers

--
Qais Yousef

Re: [PATCH v7 1/9] sched/fair: fix unfairness at wakeup

[Chen Yu]

On 2022-10-28 at 11:33:55 +0200, Vincent Guittot wrote:
[snip] 
> +static inline unsigned long  get_latency_max(void)
> +{
> +	unsigned long thresh = get_sched_latency(false);
> +
> +	thresh -= sysctl_sched_min_granularity;
> +
May I know why we substract sysctl_sched_min_granularity above?
I thought thresh -= 1 would be enough to let the waking task preempt
the current one, because:
When a long sleeping task is enqueued on this rq, its vruntime is set
to cfs_rq->min_vtime - latency, so
diff = curr->vtime - cfs_rq->min_vtime + latency,
since (curr->vtime - cfs_rq->min_vtime) >= 0, if we set the thresh to
(latency - 1), the diff is guaranteed to be bigger than thresh and
the waking task can preempt current task.

thanks,
Chenyu
> +	return thresh;
> +}

Re: [PATCH v7 1/9] sched/fair: fix unfairness at wakeup

[Vincent Guittot]

On Sat, 5 Nov 2022 at 15:33, Chen Yu <yu.c.chen@intel.com> wrote:
>
> On 2022-10-28 at 11:33:55 +0200, Vincent Guittot wrote:
> [snip]
> > +static inline unsigned long  get_latency_max(void)
> > +{
> > +     unsigned long thresh = get_sched_latency(false);
> > +
> > +     thresh -= sysctl_sched_min_granularity;
> > +
> May I know why we substract sysctl_sched_min_granularity above?
> I thought thresh -= 1 would be enough to let the waking task preempt
> the current one, because:
> When a long sleeping task is enqueued on this rq, its vruntime is set
> to cfs_rq->min_vtime - latency, so
> diff = curr->vtime - cfs_rq->min_vtime + latency,
> since (curr->vtime - cfs_rq->min_vtime) >= 0, if we set the thresh to
> (latency - 1), the diff is guaranteed to be bigger than thresh and
> the waking task can preempt current task.

If the waking task failed to preempt current it could to wait up to
sysctl_sched_min_granularity before preempting it during next tick.

Vincent

>
> thanks,
> Chenyu
> > +     return thresh;
> > +}

Re: [PATCH v7 0/9] Add latency priority for CFS class

[shrikanth suresh hegde]

> This patchset restarts the work about adding a latency priority to describe
> the latency tolerance of cfs tasks.
>
>
Hi Vincent.
  
Tested the patches on the power10 machine. It is 80 core system with SMT=8. i.e
total of 640 cpus. on the large workload which mainly interacts with the
database there is minor improvement of 3-5%.

the method followed is creating a cgroup, assigning a latency nice value of -20,
-10, 0 and adding the tasks to procs of the cgroup. outside of cgroup, stress-ng
load is running and it is not set any latency value. stress-ng --cpu=768 -l 50

with microbenchmarks, hackbench the values are more or less the same. for large
process pool of 60, there is 10% improvement. schbench tail latencies show
significant improvement with low and medium load upto 256 groups. only 512
groups shows a slight decline.

Hackbench (Iterations or N=50)
Process             6.1_Base        6.1_Latency_Nice
10                      0.13            0.14
20                      0.18            0.18
30                      0.24            0.25
40                      0.34            0.33
50                      0.40            0.41
60                      0.53            0.49

schbench (Iterations or N=5)

Groups: 1
                     6.1_Base        6.1_Latency_Nice
50.0th:                 10.8             9.8
75.0th:                 12.4            11.4
90.0th:                 14.2            13.2
95.0th:                 15.6            14.6
99.0th:                 27.8            19.0
99.5th:                 38.0            21.6
99.9th:                 66.2            25.4

Groups: 2
                     6.1_Base        6.1_Latency_Nice
50.0th:                 11.2            10.8
75.0th:                 13.2            12.4
90.0th:                 15.0            15.0
95.0th:                 16.6            16.6
99.0th:                 22.4            22.8
99.5th:                 23.8            27.8
99.9th:                 30.2            45.6

Groups: 4
                     6.1_Base        6.1_Latency_Nice
50.0th:                 13.8            11.2
75.0th:                 16.0            13.2
90.0th:                 18.6            15.2
95.0th:                 20.4            16.6
99.0th:                 28.8            21.6
99.5th:                 48.8            25.2
99.9th:                900.2            47.0

Groups: 8
                     6.1_Base        6.1_Latency_Nice
50.0th:                 17.8            14.4
75.0th:                 21.8            17.2
90.0th:                 25.4            20.4
95.0th:                 28.0            22.4
99.0th:                 52.8            28.4
99.5th:                156.4            32.6
99.9th:               1990.2            52.0

Groups: 16
                     6.1_Base        6.1_Latency_Nice
50.0th:                 26.0            21.0
75.0th:                 33.0            27.8
90.0th:                 39.6            34.4
95.0th:                 43.4            38.6
99.0th:                 66.8            48.8
99.5th:                170.6            60.6
99.9th:               3308.8           201.6

Groups: 32
                     6.1_Base        6.1_Latency_Nice
50.0th:                 40.8            38.6
75.0th:                 55.4            52.8
90.0th:                 67.0            64.2
95.0th:                 74.2            71.6
99.0th:                106.0            90.0
99.5th:                323.8           133.0
99.9th:               4789.6           459.2

Groups: 64
                     6.1_Base        6.1_Latency_Nice
50.0th:                 72.6            68.2
75.0th:                103.4            97.8
90.0th:                127.6           120.0
95.0th:                141.2           132.0
99.0th:                343.4           158.4
99.5th:               1609.0           180.8
99.9th:               6571.2           686.6

Groups: 128
                     6.1_Base        6.1_Latency_Nice
50.0th:                147.2           147.2
75.0th:                216.4           217.2
90.0th:                268.4           268.2
95.0th:                300.6           294.8
99.0th:               3500.0           638.6
99.5th:               5995.2          2522.8
99.9th:              10390.4          9451.2

Groups: 256
                     6.1_Base        6.1_Latency_Nice
50.0th:                340.8           333.2
75.0th:                551.8           530.2
90.0th:               3528.4          1919.2
95.0th:               7312.8          5558.4
99.0th:              14630.4         12912.0
99.5th:              17955.2         14950.4
99.9th:              23059.2         20230.4

Groups: 512
                     6.1_Base        6.1_Latency_Nice
50.0th:               1021.8           990.6
75.0th:               9545.6         10044.8
90.0th:              20972.8         21638.4
95.0th:              29971.2         30291.2
99.0th:              42355.2         46707.2
99.5th:              48550.4         52057.6
99.9th:              58867.2         60147.2

Tested-by: shrikanth Hegde <sshegde@linux.vnet.ibm.com>

Re: [PATCH v7 5/9] sched/fair: Take into account latency priority at wakeup

[shrikanth suresh hegde]

>   /*
>    * Preempt the current task with a newly woken task if needed:
>    */
> @@ -4566,7 +4568,7 @@ check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
>   {
>   	unsigned long ideal_runtime, delta_exec;
>   	struct sched_entity *se;
> -	s64 delta;
> +	s64 delta, offset;
>   
>   	ideal_runtime = sched_slice(cfs_rq, curr);
>   	delta_exec = curr->sum_exec_runtime - curr->prev_sum_exec_runtime;
> @@ -4591,10 +4593,12 @@ check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
>   	se = __pick_first_entity(cfs_rq);
>   	delta = curr->vruntime - se->vruntime;
>   
> -	if (delta < 0)
> +	offset = wakeup_latency_gran(curr, se);
> +	if (delta < offset)
>   		return;
>   
> -	if (delta > ideal_runtime)
> +	if ((delta > ideal_runtime) ||
> +	    (delta > get_latency_max()))
>   		resched_curr(rq_of(cfs_rq));
>   }
>   
>
Hi Vincent,

I am not sure if i have understood this below change correctly.

wakeup_latency_gran - this function returns difference in latency nice offsets.
Hence the more negative the value, it means se has more latency requirement
compared to current. Hence se should preempt the current here right?
  
we are comparing delta to get_latency_max and ideal_runtime, both of which can
be higher positive value, hence we will not preempt. that is not what we want
right?

Re: [PATCH v7 0/9] Add latency priority for CFS class

[shrikanth suresh hegde]

> This patchset restarts the work about adding a latency priority to describe
> the latency tolerance of cfs tasks.

Hi Vincent.
  
Tested the patches on the power10 machine. It is 80 core system with SMT=8. i.e
total of 640 cpus. on the large workload which mainly interacts with the
database there is minor improvement of 3-5%.

the method followed is creating a cgroup, assigning a latency nice value of -20,
-10, 0 and adding the tasks to procs of the cgroup. outside of cgroup, stress-ng
load is running and it is not set any latency value. stress-ng --cpu=768 -l 50

with microbenchmarks, hackbench the values are more or less the same. for large
process pool of 60, there is 10% improvement. schbench tail latencies show
significant improvement with low and medium load upto 256 groups. only 512
groups shows a slight decline.

Hackbench (Iterations or N=50)
Process             6.1_Base        6.1_Latency_Nice
10                      0.13            0.14
20                      0.18            0.18
30                      0.24            0.25
40                      0.34            0.33
50                      0.40            0.41
60                      0.53            0.49

schbench (Iterations or N=5)

Groups: 1
                     6.1_Base        6.1_Latency_Nice
50.0th:                 10.8             9.8
75.0th:                 12.4            11.4
90.0th:                 14.2            13.2
95.0th:                 15.6            14.6
99.0th:                 27.8            19.0
99.5th:                 38.0            21.6
99.9th:                 66.2            25.4

Groups: 2
                     6.1_Base        6.1_Latency_Nice
50.0th:                 11.2            10.8
75.0th:                 13.2            12.4
90.0th:                 15.0            15.0
95.0th:                 16.6            16.6
99.0th:                 22.4            22.8
99.5th:                 23.8            27.8
99.9th:                 30.2            45.6

Groups: 4
                     6.1_Base        6.1_Latency_Nice
50.0th:                 13.8            11.2
75.0th:                 16.0            13.2
90.0th:                 18.6            15.2
95.0th:                 20.4            16.6
99.0th:                 28.8            21.6
99.5th:                 48.8            25.2
99.9th:                900.2            47.0

Groups: 8
                     6.1_Base        6.1_Latency_Nice
50.0th:                 17.8            14.4
75.0th:                 21.8            17.2
90.0th:                 25.4            20.4
95.0th:                 28.0            22.4
99.0th:                 52.8            28.4
99.5th:                156.4            32.6
99.9th:               1990.2            52.0

Groups: 16
                     6.1_Base        6.1_Latency_Nice
50.0th:                 26.0            21.0
75.0th:                 33.0            27.8
90.0th:                 39.6            34.4
95.0th:                 43.4            38.6
99.0th:                 66.8            48.8
99.5th:                170.6            60.6
99.9th:               3308.8           201.6

Groups: 32
                     6.1_Base        6.1_Latency_Nice
50.0th:                 40.8            38.6
75.0th:                 55.4            52.8
90.0th:                 67.0            64.2
95.0th:                 74.2            71.6
99.0th:                106.0            90.0
99.5th:                323.8           133.0
99.9th:               4789.6           459.2

Groups: 64
                     6.1_Base        6.1_Latency_Nice
50.0th:                 72.6            68.2
75.0th:                103.4            97.8
90.0th:                127.6           120.0
95.0th:                141.2           132.0
99.0th:                343.4           158.4
99.5th:               1609.0           180.8
99.9th:               6571.2           686.6

Groups: 128
                     6.1_Base        6.1_Latency_Nice
50.0th:                147.2           147.2
75.0th:                216.4           217.2
90.0th:                268.4           268.2
95.0th:                300.6           294.8
99.0th:               3500.0           638.6
99.5th:               5995.2          2522.8
99.9th:              10390.4          9451.2

Groups: 256
                     6.1_Base        6.1_Latency_Nice
50.0th:                340.8           333.2
75.0th:                551.8           530.2
90.0th:               3528.4          1919.2
95.0th:               7312.8          5558.4
99.0th:              14630.4         12912.0
99.5th:              17955.2         14950.4
99.9th:              23059.2         20230.4

Groups: 512
                     6.1_Base        6.1_Latency_Nice
50.0th:               1021.8           990.6
75.0th:               9545.6         10044.8
90.0th:              20972.8         21638.4
95.0th:              29971.2         30291.2
99.0th:              42355.2         46707.2
99.5th:              48550.4         52057.6
99.9th:              58867.2         60147.2

Tested-by: shrikanth Hegde <sshegde@linux.vnet.ibm.com>

Re: [PATCH v7 5/9] sched/fair: Take into account latency priority at wakeup

[shrikanth suresh hegde]

>   /*
>    * Preempt the current task with a newly woken task if needed:
>    */
> @@ -4566,7 +4568,7 @@ check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
>   {
>   	unsigned long ideal_runtime, delta_exec;
>   	struct sched_entity *se;
> -	s64 delta;
> +	s64 delta, offset;
>   
>   	ideal_runtime = sched_slice(cfs_rq, curr);
>   	delta_exec = curr->sum_exec_runtime - curr->prev_sum_exec_runtime;
> @@ -4591,10 +4593,12 @@ check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
>   	se = __pick_first_entity(cfs_rq);
>   	delta = curr->vruntime - se->vruntime;
>   
> -	if (delta < 0)
> +	offset = wakeup_latency_gran(curr, se);
> +	if (delta < offset)
>   		return;
>   
> -	if (delta > ideal_runtime)
> +	if ((delta > ideal_runtime) ||
> +	    (delta > get_latency_max()))
>   		resched_curr(rq_of(cfs_rq));
>   }
>   
>
Hi Vincent,

I am not sure if i have understood this below change correctly.

wakeup_latency_gran - this function returns difference in latency nice offsets.
Hence the more negative the value, it means se has more latency requirement
compared to current. Hence se should preempt the current here right?

we are comparing delta to get_latency_max and ideal_runtime, both of which can
be higher positive value, hence we will not preempt. that is not what we want
right?

Re: [PATCH v7 5/9] sched/fair: Take into account latency priority at wakeup

[Vincent Guittot]

On Sun, 13 Nov 2022 at 09:51, shrikanth suresh hegde
<sshegde@linux.vnet.ibm.com> wrote:
>
>
> >   /*
> >    * Preempt the current task with a newly woken task if needed:
> >    */
> > @@ -4566,7 +4568,7 @@ check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
> >   {
> >       unsigned long ideal_runtime, delta_exec;
> >       struct sched_entity *se;
> > -     s64 delta;
> > +     s64 delta, offset;
> >
> >       ideal_runtime = sched_slice(cfs_rq, curr);
> >       delta_exec = curr->sum_exec_runtime - curr->prev_sum_exec_runtime;
> > @@ -4591,10 +4593,12 @@ check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
> >       se = __pick_first_entity(cfs_rq);
> >       delta = curr->vruntime - se->vruntime;
> >
> > -     if (delta < 0)
> > +     offset = wakeup_latency_gran(curr, se);
> > +     if (delta < offset)
> >               return;
> >
> > -     if (delta > ideal_runtime)
> > +     if ((delta > ideal_runtime) ||
> > +         (delta > get_latency_max()))
> >               resched_curr(rq_of(cfs_rq));
> >   }
> >
> >
> Hi Vincent,
>
> I am not sure if i have understood this below change correctly.

I assume you wanted to say above as you removed the end of the patch

>
> wakeup_latency_gran - this function returns difference in latency nice offsets.
> Hence the more negative the value, it means se has more latency requirement
> compared to current. Hence se should preempt the current here right?

yes. the latency nice offset can add a offset in the comparison of (delta < 0)


>
> we are comparing delta to get_latency_max and ideal_runtime, both of which can
> be higher positive value, hence we will not preempt. that is not what we want
> right?

Here we are in the tick and not in the wakeup path. So the wakeup
preemption, if any, should have already happened and we now have to
make sure that if current has not been preempted at wakeup, it will
not get too much run time that will move waiting tasks out of the
sched_latency period

>

Re: [PATCH v7 0/9] Add latency priority for CFS class

[Vincent Guittot]

On Sun, 13 Nov 2022 at 09:51, shrikanth suresh hegde
<sshegde@linux.vnet.ibm.com> wrote:
>
>
> > This patchset restarts the work about adding a latency priority to describe
> > the latency tolerance of cfs tasks.
>
> Hi Vincent.
>
> Tested the patches on the power10 machine. It is 80 core system with SMT=8. i.e
> total of 640 cpus. on the large workload which mainly interacts with the
> database there is minor improvement of 3-5%.
>
> the method followed is creating a cgroup, assigning a latency nice value of -20,
> -10, 0 and adding the tasks to procs of the cgroup. outside of cgroup, stress-ng
> load is running and it is not set any latency value. stress-ng --cpu=768 -l 50
>
> with microbenchmarks, hackbench the values are more or less the same. for large
> process pool of 60, there is 10% improvement. schbench tail latencies show
> significant improvement with low and medium load upto 256 groups. only 512
> groups shows a slight decline.
>
> Hackbench (Iterations or N=50)
> Process             6.1_Base        6.1_Latency_Nice
> 10                      0.13            0.14
> 20                      0.18            0.18
> 30                      0.24            0.25
> 40                      0.34            0.33
> 50                      0.40            0.41
> 60                      0.53            0.49
>
> schbench (Iterations or N=5)
>
> Groups: 1
>                      6.1_Base        6.1_Latency_Nice
> 50.0th:                 10.8             9.8
> 75.0th:                 12.4            11.4
> 90.0th:                 14.2            13.2
> 95.0th:                 15.6            14.6
> 99.0th:                 27.8            19.0
> 99.5th:                 38.0            21.6
> 99.9th:                 66.2            25.4
>
> Groups: 2
>                      6.1_Base        6.1_Latency_Nice
> 50.0th:                 11.2            10.8
> 75.0th:                 13.2            12.4
> 90.0th:                 15.0            15.0
> 95.0th:                 16.6            16.6
> 99.0th:                 22.4            22.8
> 99.5th:                 23.8            27.8
> 99.9th:                 30.2            45.6
>
> Groups: 4
>                      6.1_Base        6.1_Latency_Nice
> 50.0th:                 13.8            11.2
> 75.0th:                 16.0            13.2
> 90.0th:                 18.6            15.2
> 95.0th:                 20.4            16.6
> 99.0th:                 28.8            21.6
> 99.5th:                 48.8            25.2
> 99.9th:                900.2            47.0
>
> Groups: 8
>                      6.1_Base        6.1_Latency_Nice
> 50.0th:                 17.8            14.4
> 75.0th:                 21.8            17.2
> 90.0th:                 25.4            20.4
> 95.0th:                 28.0            22.4
> 99.0th:                 52.8            28.4
> 99.5th:                156.4            32.6
> 99.9th:               1990.2            52.0
>
> Groups: 16
>                      6.1_Base        6.1_Latency_Nice
> 50.0th:                 26.0            21.0
> 75.0th:                 33.0            27.8
> 90.0th:                 39.6            34.4
> 95.0th:                 43.4            38.6
> 99.0th:                 66.8            48.8
> 99.5th:                170.6            60.6
> 99.9th:               3308.8           201.6
>
> Groups: 32
>                      6.1_Base        6.1_Latency_Nice
> 50.0th:                 40.8            38.6
> 75.0th:                 55.4            52.8
> 90.0th:                 67.0            64.2
> 95.0th:                 74.2            71.6
> 99.0th:                106.0            90.0
> 99.5th:                323.8           133.0
> 99.9th:               4789.6           459.2
>
> Groups: 64
>                      6.1_Base        6.1_Latency_Nice
> 50.0th:                 72.6            68.2
> 75.0th:                103.4            97.8
> 90.0th:                127.6           120.0
> 95.0th:                141.2           132.0
> 99.0th:                343.4           158.4
> 99.5th:               1609.0           180.8
> 99.9th:               6571.2           686.6
>
> Groups: 128
>                      6.1_Base        6.1_Latency_Nice
> 50.0th:                147.2           147.2
> 75.0th:                216.4           217.2
> 90.0th:                268.4           268.2
> 95.0th:                300.6           294.8
> 99.0th:               3500.0           638.6
> 99.5th:               5995.2          2522.8
> 99.9th:              10390.4          9451.2
>
> Groups: 256
>                      6.1_Base        6.1_Latency_Nice
> 50.0th:                340.8           333.2
> 75.0th:                551.8           530.2
> 90.0th:               3528.4          1919.2
> 95.0th:               7312.8          5558.4
> 99.0th:              14630.4         12912.0
> 99.5th:              17955.2         14950.4
> 99.9th:              23059.2         20230.4
>
> Groups: 512
>                      6.1_Base        6.1_Latency_Nice
> 50.0th:               1021.8           990.6
> 75.0th:               9545.6         10044.8
> 90.0th:              20972.8         21638.4
> 95.0th:              29971.2         30291.2
> 99.0th:              42355.2         46707.2
> 99.5th:              48550.4         52057.6
> 99.9th:              58867.2         60147.2
>
> Tested-by: shrikanth Hegde <sshegde@linux.vnet.ibm.com>

Thanks for the tests and the results


>