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/[email protected]/
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.
While preparing this version 8, I have evaluated the benefit of using an
augmented rbtree instead of adding a rbtree for latency sensitive entities,
which was a relevant suggestion done by PeterZ. Although the augmented
rbtree enables to sort additional information in the tree with a limited
overhead, it has more impact on legacy use cases (latency_nice >= 0)
because the augmented callbacks are always called to maintain this
additional information even when there is no sensitive tasks. In such
cases, the dedicated rbtree remains empty and the overhead is reduced to
loading a cached null node pointer. Nevertheless, we might want to
reconsider the augmented rbtree once the use of negative latency_nice will
be more widlely deployed. At now, the different tests that I have done,
have not shown improvements with augmented rbtree.
Below are some hackbench results:
2 rbtrees augmented rbtree augmented rbtree
sorted by vruntime sorted by wakeup_vruntime
sched pipe
avg 26311,000 25976,667 25839,556
stdev 0,15 % 0,28 % 0,24 %
vs tip 0,50 % -0,78 % -1,31 %
hackbench 1 group
avg 1,315 1,344 1,359
stdev 0,88 % 1,55 % 1,82 %
vs tip -0,47 % -2,68 % -3,87 %
hackbench 4 groups
avg 1,339 1,365 1,367
stdev 2,39 % 2,26 % 3,58 %
vs tip -0,08 % -2,01 % -2,22 %
hackbench 8 groups
avg 1,233 1,286 1,301
stdev 0,74 % 1,09 % 1,52 %
vs tip 0,29 % -4,05 % -5,27 %
hackbench 16 groups
avg 1,268 1,313 1,319
stdev 0,85 % 1,60 % 0,68 %
vs tip -0,02 % -3,56 % -4,01 %
[1] https://source.android.com/docs/core/debug/eval_perf#touchlatency
Change since v7:
- Replaced se->on_latency by using RB_CLEAR_NODE() and RB_EMPTY_NODE()
- Clarify the limit behavior fo the cgroup cpu.latenyc_nice
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 <[email protected]>
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 | 10 ++
include/linux/sched.h | 4 +
include/uapi/linux/sched.h | 4 +-
include/uapi/linux/sched/types.h | 19 +++
init/init_task.c | 1 +
kernel/sched/core.c | 106 ++++++++++++
kernel/sched/debug.c | 1 +
kernel/sched/fair.c | 209 ++++++++++++++++++++----
kernel/sched/sched.h | 65 +++++++-
tools/include/uapi/linux/sched.h | 4 +-
10 files changed, 387 insertions(+), 36 deletions(-)
--
2.17.1
Take into account wakeup_latency_gran() when ordering the cfs threads.
Signed-off-by: Vincent Guittot <[email protected]>
---
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
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 <[email protected]>
---
include/linux/sched.h | 1 +
kernel/sched/core.c | 1 +
kernel/sched/fair.c | 95 +++++++++++++++++++++++++++++++++++++++++--
kernel/sched/sched.h | 1 +
4 files changed, 95 insertions(+), 3 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index a74cad08e91e..45b8c36e64cc 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -547,6 +547,7 @@ struct sched_entity {
/* For load-balancing: */
struct load_weight load;
struct rb_node run_node;
+ struct rb_node latency_node;
struct list_head group_node;
unsigned int on_rq;
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 3f42b1f61a7e..c6c67677d71f 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4342,6 +4342,7 @@ static void __sched_fork(unsigned long clone_flags, struct task_struct *p)
p->se.nr_migrations = 0;
p->se.vruntime = 0;
INIT_LIST_HEAD(&p->se.group_node);
+ RB_CLEAR_NODE(&p->se.latency_node);
#ifdef CONFIG_FAIR_GROUP_SCHED
p->se.cfs_rq = NULL;
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index a7372f80b1ea..fb4973a87f25 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -664,7 +664,76 @@ 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 (!RB_EMPTY_NODE(&se->latency_node))
+ 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);
+}
+
+static void __dequeue_latency(struct cfs_rq *cfs_rq, struct sched_entity *se)
+{
+ if (!RB_EMPTY_NODE(&se->latency_node)) {
+ rb_erase_cached(&se->latency_node, &cfs_rq->latency_timeline);
+ RB_CLEAR_NODE(&se->latency_node);
+ }
+}
+
+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 +4508,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 +4597,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 +4689,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 +4728,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 +4770,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 +4799,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 +11793,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 +12102,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
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 <[email protected]>
---
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 fb4973a87f25..c2c75d531612 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5801,35 +5801,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
@@ -5916,15 +5887,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
From: Parth Shah <[email protected]>
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 <[email protected]>
[rebase]
Signed-off-by: Vincent Guittot <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
Documentation/admin-guide/cgroup-v2.rst | 10 +++++
kernel/sched/core.c | 52 +++++++++++++++++++++++++
kernel/sched/fair.c | 33 ++++++++++++++++
kernel/sched/sched.h | 4 ++
4 files changed, 99 insertions(+)
diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
index be4a77baf784..a4866cd4e58c 100644
--- a/Documentation/admin-guide/cgroup-v2.rst
+++ b/Documentation/admin-guide/cgroup-v2.rst
@@ -1095,6 +1095,16 @@ 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). The latency_nice of a group is
+ used to limit the impact of the latency_nice of a task outside the
+ group.
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
Hi Vincent,
On Thu, Nov 10, 2022 at 06:50:01PM +0100, Vincent Guittot wrote:
> 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.
Just a thought: one can argue that this also hurts the running task because
wakeup_gran() is expected to not preempt the running task for a certain
minimum amount of time right?
So for example, if I set sysctl_sched_wakeup_granularity to a high value, I
expect the current task to not be preempted for that long, even if the
sched_latency cap in place_entity() makes the delta smaller than
wakeup_gran(). The place_entity() in current code is used to cap the sleep
credit, it does not really talk about preemption.
I don't mind this change, but it does change the meaning a bit of
sysctl_sched_wakeup_granularity I think.
> 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.
nit: I would prefer we say, instead of "is in the same range", "is greater
than". Because it got confusing a bit for me.
> 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.
Could we add here,
Reported-by: Youssef Esmat <[email protected]>
> Signed-off-by: Vincent Guittot <[email protected]>
Just a few more comments below:
> ---
> 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());
> +
Can we move this to wakeup_gran(se)? IMO, it belongs there because you are
adjusting the wakeup_gran().
> 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)
> +{
IMO, since there are other users of sysctl_sched_latency, it would be better
to call this get_max_sleep_credit() or something.
> + 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;
Could you clarify, why are you subtracting sched_min_granularity here? Could
you add some comments here to make it clear?
thanks,
- Joel
> +
> + return thresh;
> +}
> +
> #ifdef CONFIG_SCHED_HRTICK
>
> /*
> --
> 2.17.1
>
On Mon, 14 Nov 2022 at 04:06, Joel Fernandes <[email protected]> wrote:
>
> Hi Vincent,
>
> On Thu, Nov 10, 2022 at 06:50:01PM +0100, Vincent Guittot wrote:
> > 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.
>
> Just a thought: one can argue that this also hurts the running task because
> wakeup_gran() is expected to not preempt the running task for a certain
> minimum amount of time right?
No because you should not make wakeup_gran() higher than sched_latency.
>
> So for example, if I set sysctl_sched_wakeup_granularity to a high value, I
> expect the current task to not be preempted for that long, even if the
> sched_latency cap in place_entity() makes the delta smaller than
> wakeup_gran(). The place_entity() in current code is used to cap the sleep
> credit, it does not really talk about preemption.
But one should never set such nonsense values.
>
> I don't mind this change, but it does change the meaning a bit of
> sysctl_sched_wakeup_granularity I think.
>
> > 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.
>
> nit: I would prefer we say, instead of "is in the same range", "is greater
> than". Because it got confusing a bit for me.
I prefer keeping current description because the sentence below gives
the reason why it's not strictly greater than
>
> > 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.
>
> Could we add here,
> Reported-by: Youssef Esmat <[email protected]>
yes
>
> > Signed-off-by: Vincent Guittot <[email protected]>
>
> Just a few more comments below:
>
> > ---
> > 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());
> > +
>
> Can we move this to wakeup_gran(se)? IMO, it belongs there because you are
> adjusting the wakeup_gran().
I prefer keep current code because patch 8 adds offset in the equation
>
> > 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)
> > +{
>
> IMO, since there are other users of sysctl_sched_latency, it would be better
> to call this get_max_sleep_credit() or something.
get_sleep_latency()
>
> > + 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;
>
> Could you clarify, why are you subtracting sched_min_granularity here? Could
> you add some comments here to make it clear?
If the waking task failed to preempt current it could to wait up to
sysctl_sched_min_granularity before preempting it during next tick.
>
> thanks,
>
> - Joel
>
>
> > +
> > + return thresh;
> > +}
> > +
> > #ifdef CONFIG_SCHED_HRTICK
> >
> > /*
> > --
> > 2.17.1
> >
Hi Vincent!
On 10-Nov 18:50, Vincent Guittot wrote:
[...]
> 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)
^^^^^^^^^^^^^^^^^
This can be confusing since it's not always returning the sysctl_sched_latency
value. It's also being used to tune the vruntime at wakeup time.
Thus, what about renaming this to something more close to what's used for, e.g.
get_wakeup_latency(se)
?
Also, in the following patches we call this always with a false parametr.
Thus, perhaps in a following patch, we can better add something like:
#define max_wakeup_latency get_wakeup_latency(false)
?
> +{
> + 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)
^^^^^^^^^^^^^^^
This is always used to cap some form of vruntime deltas in:
- check_preempt_tick()
- wakeup_latency_gran()
- wakeup_preempt_entity()
It's always smaller then the max_wakeup_latency (as defined above).
Thus, does not seems something like:
wakeup_latency_threshold()
a better documenting naming?
> +{
> + unsigned long thresh = get_sched_latency(false);
> +
> + thresh -= sysctl_sched_min_granularity;
> +
> + return thresh;
> +}
[...]
Best,
Patrick
--
#include <best/regards.h>
Patrick Bellasi
Hi Vincent,
On 10-Nov 18:50, Vincent Guittot wrote:
[...]
> diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> index be4a77baf784..a4866cd4e58c 100644
> --- a/Documentation/admin-guide/cgroup-v2.rst
> +++ b/Documentation/admin-guide/cgroup-v2.rst
> @@ -1095,6 +1095,16 @@ 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). The latency_nice of a group is
> + used to limit the impact of the latency_nice of a task outside the
> + group.
This control model is not clear to me.
It does not seem matching what we have for uclamp, where the cgroup values are
used to restrict how much a task can ask or give (in terms of latency here).
in the uclamp's requested-vs-effective values model:
A) a task can "request" (or give up) latency as much as it likes
B) the cgroup in which the task is in any moment limits wthe maximum
latency a task can "request" (or give up)
C) the system wide knob set the "request" limit for the root cgroup an any task
not in a cgroup.
This model seems to be what we should use here too.
IOW, for each task compute an "effective" latency_nice value which is defined
starting for a task "requested" latency value and by restricting this value
based on the (B) cgroup value and the (C) system wide value.
That's what we do in uclamp_eff_get():
https://elixir.bootlin.com/linux/v6.0/source/kernel/sched/core.c#L1484
Why such a model cannot be used for latency_nice too?
Am I missing something?
Best,
patrick
--
#include <best/regards.h>
Patrick Bellasi
On Mon, 14 Nov 2022 at 17:20, Patrick Bellasi
<[email protected]> wrote:
>
> Hi Vincent,
>
> On 10-Nov 18:50, Vincent Guittot wrote:
>
> [...]
>
> > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> > index be4a77baf784..a4866cd4e58c 100644
> > --- a/Documentation/admin-guide/cgroup-v2.rst
> > +++ b/Documentation/admin-guide/cgroup-v2.rst
> > @@ -1095,6 +1095,16 @@ 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). The latency_nice of a group is
> > + used to limit the impact of the latency_nice of a task outside the
> > + group.
>
> This control model is not clear to me.
>
> It does not seem matching what we have for uclamp, where the cgroup values are
> used to restrict how much a task can ask or give (in terms of latency here).
>
> in the uclamp's requested-vs-effective values model:
>
> A) a task can "request" (or give up) latency as much as it likes
>
> B) the cgroup in which the task is in any moment limits wthe maximum
> latency a task can "request" (or give up)
>
> C) the system wide knob set the "request" limit for the root cgroup an any task
> not in a cgroup.
>
> This model seems to be what we should use here too.
>
> IOW, for each task compute an "effective" latency_nice value which is defined
> starting for a task "requested" latency value and by restricting this value
> based on the (B) cgroup value and the (C) system wide value.
>
> That's what we do in uclamp_eff_get():
> https://elixir.bootlin.com/linux/v6.0/source/kernel/sched/core.c#L1484
>
> Why such a model cannot be used for latency_nice too?
> Am I missing something?
Have you read the previous email thread on the subject ?
As I mentioned previously we don't need an effective latency for this
patchset because the current use of cgroup latency_nice is done at
each scheduling level just like the cgroup weight is used at each
level.
Regards,
Vincent
>
>
> Best,
> patrick
>
> --
> #include <best/regards.h>
>
> Patrick Bellasi
On Mon, 14 Nov 2022 at 17:20, Patrick Bellasi
<[email protected]> wrote:
>
> Hi Vincent!
>
> On 10-Nov 18:50, Vincent Guittot wrote:
>
> [...]
>
> > 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)
> ^^^^^^^^^^^^^^^^^
>
> This can be confusing since it's not always returning the sysctl_sched_latency
> value. It's also being used to tune the vruntime at wakeup time.
>
> Thus, what about renaming this to something more close to what's used for, e.g.
> get_wakeup_latency(se)
> ?
>
> Also, in the following patches we call this always with a false parametr.
> Thus, perhaps in a following patch, we can better add something like:
> #define max_wakeup_latency get_wakeup_latency(false)
> ?
I'm going to rename get_wakeup_latency by get_sleep_latency() as
proposed earlier.
I don't see the benefit of adding a macro of top so will keep the parameter
>
> > +{
> > + 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)
> ^^^^^^^^^^^^^^^
>
> This is always used to cap some form of vruntime deltas in:
> - check_preempt_tick()
> - wakeup_latency_gran()
> - wakeup_preempt_entity()
> It's always smaller then the max_wakeup_latency (as defined above).
>
> Thus, does not seems something like:
> wakeup_latency_threshold()
> a better documenting naming?
>
> > +{
> > + unsigned long thresh = get_sched_latency(false);
> > +
> > + thresh -= sysctl_sched_min_granularity;
> > +
> > + return thresh;
> > +}
>
> [...]
>
> Best,
> Patrick
>
> --
> #include <best/regards.h>
>
> Patrick Bellasi
>
On 10/11/2022 18:50, Vincent Guittot wrote:
> 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.
Looks like that gran can be nuch higher than sched_latency for extreme
cases?
>
> 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.
^^^ Does this related to the `if (vdiff > gran) return 1` condition in
wakeup_preempt_entity()?
[...]
> @@ -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
low priority task or taskgroup with low cpu.shares, right?
6 CPUs
sysctl_sched
.sysctl_sched_latency : 18.000000
.sysctl_sched_min_granularity : 2.250000
.sysctl_sched_idle_min_granularity : 0.750000
.sysctl_sched_wakeup_granularity : 3.000000
...
p1 & p2 affine to CPUX
'/'
/\
p1 p2
p1 & p2 nice=0 - vdiff=9ms gran=3ms lat_max=6.75ms
p1 & p2 nice=4 - vdiff=9ms gran=7.26ms lat_max=6.75ms
p1 & p2 nice=19 - vdiff=9ms gran=204.79ms lat_max=6.75ms
'/'
/\
A B
/ \
p1 p2
A & B cpu.shares=1024 - vdiff=9ms gran=3ms lat_max=6.75ms
A & B cpu.shares=448 - vdiff=9ms gran=6.86ms lat_max=6.75ms
A & B cpu.shares=2 - vdiff=9ms gran=1536ms lat_max=6.75ms
> + * chance to preempt current.
> + */
> + gran = min_t(s64, gran, get_latency_max());
> +
[...]
> @@ -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)
^^
2 white-spaces
[...]
> +
> +static inline unsigned long get_latency_max(void)
^^
[...]
On Mon, 14 Nov 2022 at 20:13, Dietmar Eggemann <[email protected]> wrote:
>
> On 10/11/2022 18:50, Vincent Guittot wrote:
> > 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.
>
> Looks like that gran can be nuch higher than sched_latency for extreme
> cases?
It's not that extreme, all tasks with nice prio 5 and above will face
the problem
>
> >
> > 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.
>
> ^^^ Does this related to the `if (vdiff > gran) return 1` condition in
> wakeup_preempt_entity()?
yes
>
> [...]
>
> > @@ -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
>
> low priority task or taskgroup with low cpu.shares, right?
yes
>
> 6 CPUs
>
> sysctl_sched
> .sysctl_sched_latency : 18.000000
> .sysctl_sched_min_granularity : 2.250000
> .sysctl_sched_idle_min_granularity : 0.750000
> .sysctl_sched_wakeup_granularity : 3.000000
> ...
>
> p1 & p2 affine to CPUX
>
> '/'
> /\
> p1 p2
>
> p1 & p2 nice=0 - vdiff=9ms gran=3ms lat_max=6.75ms
> p1 & p2 nice=4 - vdiff=9ms gran=7.26ms lat_max=6.75ms
p1 & p2 nice = 5 - vdiff=9ms gran=9.17ms lat_max=6.75ms
> p1 & p2 nice=19 - vdiff=9ms gran=204.79ms lat_max=6.75ms
>
>
> '/'
> /\
> A B
> / \
> p1 p2
>
> A & B cpu.shares=1024 - vdiff=9ms gran=3ms lat_max=6.75ms
> A & B cpu.shares=448 - vdiff=9ms gran=6.86ms lat_max=6.75ms
> A & B cpu.shares=2 - vdiff=9ms gran=1536ms lat_max=6.75ms
>
> > + * chance to preempt current.
> > + */
> > + gran = min_t(s64, gran, get_latency_max());
> > +
>
> [...]
>
> > @@ -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)
> ^^
> 2 white-spaces
ok
>
> [...]
>
> > +
> > +static inline unsigned long get_latency_max(void)
> ^^
ok
>
> [...]
Hi Vincent,
On Mon, Nov 14, 2022 at 11:05 AM Vincent Guittot
<[email protected]> wrote:
[...]
> >
> > On Thu, Nov 10, 2022 at 06:50:01PM +0100, Vincent Guittot wrote:
> > > 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.
> >
> > Just a thought: one can argue that this also hurts the running task because
> > wakeup_gran() is expected to not preempt the running task for a certain
> > minimum amount of time right?
>
> No because you should not make wakeup_gran() higher than sched_latency.
>
> >
> > So for example, if I set sysctl_sched_wakeup_granularity to a high value, I
> > expect the current task to not be preempted for that long, even if the
> > sched_latency cap in place_entity() makes the delta smaller than
> > wakeup_gran(). The place_entity() in current code is used to cap the sleep
> > credit, it does not really talk about preemption.
>
> But one should never set such nonsense values.
It is not about the user setting nonsense sysctl value. Even if you do
not change sysctl_sched_wakeup_granularity, wakeup_gran() can be large
due to NICE scaling.
wakeup_gran() scales the sysctl by the ratio of the nice-load of the
se, with the NICE_0_LOAD.
On my system, by default sysctl_sched_wakeup_granularity is 3ms, and
sysctl_sched_latency is 18ms.
However, if you set the task to nice +10, the wakeup_gran() scaling
can easily make the gran exceed sysctl_sched_latency. And also, just
to note (per my experience) sysctl_sched_latency does not really hold
anyway when nice values are not default. IOW, all tasks are not
guaranteed to run within the sched_latency window always.
Again, like I said I don't mind this change (and I think it is OK to
do) but I was just preparing you/us for someone who might say they
don't much like the aggressive preemption.
> > I don't mind this change, but it does change the meaning a bit of
> > sysctl_sched_wakeup_granularity I think.
> >
> > > 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.
> >
> > nit: I would prefer we say, instead of "is in the same range", "is greater
> > than". Because it got confusing a bit for me.
>
> I prefer keeping current description because the sentence below gives
> the reason why it's not strictly greater than
Honestly saying "is in the same range" is ambiguous and confusing. I
prefer the commit messages to be clear, but I leave it up to you.
> > Just a few more comments below:
[...]
> > > +
> > > + /*
> > > + * 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());
> > > +
> >
> > Can we move this to wakeup_gran(se)? IMO, it belongs there because you are
> > adjusting the wakeup_gran().
>
> I prefer keep current code because patch 8 adds offset in the equation
Ack.
> > > 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)
> > > +{
> >
> > IMO, since there are other users of sysctl_sched_latency, it would be better
> > to call this get_max_sleep_credit() or something.
>
> get_sleep_latency()
Ack.
> >
> > > + 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;
> >
> > Could you clarify, why are you subtracting sched_min_granularity here? Could
> > you add some comments here to make it clear?
>
> If the waking task failed to preempt current it could to wait up to
> sysctl_sched_min_granularity before preempting it during next tick.
Ok, makes sense, thanks.
Reviewed-by: Joel Fernandes (Google) <[email protected]>
- Joel
On Mon, Nov 14, 2022 at 12:05:18PM +0100, Vincent Guittot wrote:
> On Mon, 14 Nov 2022 at 04:06, Joel Fernandes <[email protected]> wrote:
> >
> > Hi Vincent,
> >
> > On Thu, Nov 10, 2022 at 06:50:01PM +0100, Vincent Guittot wrote:
... ...
> > > +static inline unsigned long get_latency_max(void)
> > > +{
> > > + unsigned long thresh = get_sched_latency(false);
> > > +
> > > + thresh -= sysctl_sched_min_granularity;
> >
> > Could you clarify, why are you subtracting sched_min_granularity here? Could
> > you add some comments here to make it clear?
>
> If the waking task failed to preempt current it could to wait up to
> sysctl_sched_min_granularity before preempting it during next tick.
check_preempt_tick() compares vdiff/delta between the leftmost se and
curr against curr's ideal_runtime, it doesn't use thresh here or the
adjusted wakeup_gran, so I don't see why reducing thresh here can help
se to preempt curr during next tick if it failed to preempt curr in its
wakeup path.
I can see reducing thresh here with whatever value can help the waking
se to preempt curr in wakeup_preempt_entity() though, because most
likely the waking se's vruntime is cfs_rq->min_vruntime -
sysctl_sched_latency/2 and curr->vruntime is near cfs_rq->min_vruntime
so vdiff is about sysctl_sched_latency/2, which is the same value as
get_sched_latency(false) and when thresh is reduced some bit, then vdiff
in wakeup_preempt_entity() will be larger than gran and make it possible
to preempt.
So I'm confused by your comment or I might misread the code.
On Wed, 16 Nov 2022 at 09:26, Aaron Lu <[email protected]> wrote:
>
> On Mon, Nov 14, 2022 at 12:05:18PM +0100, Vincent Guittot wrote:
> > On Mon, 14 Nov 2022 at 04:06, Joel Fernandes <[email protected]> wrote:
> > >
> > > Hi Vincent,
> > >
> > > On Thu, Nov 10, 2022 at 06:50:01PM +0100, Vincent Guittot wrote:
>
> ... ...
>
> > > > +static inline unsigned long get_latency_max(void)
> > > > +{
> > > > + unsigned long thresh = get_sched_latency(false);
> > > > +
> > > > + thresh -= sysctl_sched_min_granularity;
> > >
> > > Could you clarify, why are you subtracting sched_min_granularity here? Could
> > > you add some comments here to make it clear?
> >
> > If the waking task failed to preempt current it could to wait up to
> > sysctl_sched_min_granularity before preempting it during next tick.
>
> check_preempt_tick() compares vdiff/delta between the leftmost se and
> curr against curr's ideal_runtime, it doesn't use thresh here or the
> adjusted wakeup_gran, so I don't see why reducing thresh here can help
> se to preempt curr during next tick if it failed to preempt curr in its
> wakeup path.
If waking task doesn't preempt curr, it will wait for the next
check_preempt_tick(), but check_preempt_tick() ensures a minimum
runtime of sysctl_sched_min_granularity before comparing the vruntime.
Thresh doesn't help in check_preempt_tick() but anticipate the fact
that if it fails to preempt now, current can get an additional
sysctl_sched_min_granularity runtime before being preempted.
>
> I can see reducing thresh here with whatever value can help the waking
> se to preempt curr in wakeup_preempt_entity() though, because most
> likely the waking se's vruntime is cfs_rq->min_vruntime -
> sysctl_sched_latency/2 and curr->vruntime is near cfs_rq->min_vruntime
> so vdiff is about sysctl_sched_latency/2, which is the same value as
> get_sched_latency(false) and when thresh is reduced some bit, then vdiff
> in wakeup_preempt_entity() will be larger than gran and make it possible
> to preempt.
>
> So I'm confused by your comment or I might misread the code.