From: Paul Turner <redacted>
CPU scheduler marks need_resched flag to signal a schedule() on a
particular CPU. But, schedule() may not happen immediately in cases
where the current task is executing in the kernel mode (no
preemption state) for extended periods of time.
This patch adds a warn_on if need_resched is pending for more than the
time specified in sysctl resched_latency_warn_ms. If it goes off, it is
likely that there is a missing cond_resched() somewhere. Monitoring is
done via the tick and the accuracy is hence limited to jiffy scale. This
also means that we won't trigger the warning if the tick is disabled.
This feature is default disabled. It can be toggled on using sysctl
resched_latency_warn_enabled.
Signed-off-by: Paul Turner <redacted>
Signed-off-by: Josh Don <redacted>
---
Delta from v1:
- separate sysctl for enabling/disabling and triggering warn_once
behavior
- add documentation
- static branch for the enable
Documentation/admin-guide/sysctl/kernel.rst | 23 ++++++
include/linux/sched/sysctl.h | 4 ++
kernel/sched/core.c | 78 ++++++++++++++++++++-
kernel/sched/debug.c | 10 +++
kernel/sched/sched.h | 10 +++
kernel/sysctl.c | 24 +++++++
6 files changed, 148 insertions(+), 1 deletion(-)
@@ -1077,6 +1077,29 @@ ROM/Flash boot loader. Maybe to tell it what to do after rebooting. ???+resched_latency_warn_enabled+============================++Enables/disables a warning that will trigger if need_resched is set for+longer than sysctl ``resched_latency_warn_ms``. This warning likely+indicates a kernel bug, such as a failure to call cond_resched().++Requires ``CONFIG_SCHED_DEBUG``.+++resched_latency_warn_ms+=======================++See ``resched_latency_warn_enabled``.+++resched_latency_warn_once+=========================++If set, ``resched_latency_warn_enabled`` will only trigger one warning+per boot.++ sched_energy_aware ==================
@@ -4520,6 +4534,58 @@ unsigned long long task_sched_runtime(struct task_struct *p)returnns;}+#ifdef CONFIG_SCHED_DEBUG+staticu64resched_latency_check(structrq*rq)+{+intlatency_warn_ms=READ_ONCE(sysctl_resched_latency_warn_ms);+u64need_resched_latency,now=rq_clock(rq);+staticboolwarned_once;++if(sysctl_resched_latency_warn_once&&warned_once)+return0;++if(!need_resched()||WARN_ON_ONCE(latency_warn_ms<2))+return0;++/* Disable this warning for the first few mins after boot */+if(now<resched_boot_quiet_sec*NSEC_PER_SEC)+return0;++if(!rq->last_seen_need_resched_ns){+rq->last_seen_need_resched_ns=now;+rq->ticks_without_resched=0;+return0;+}++rq->ticks_without_resched++;+need_resched_latency=now-rq->last_seen_need_resched_ns;+if(need_resched_latency<=latency_warn_ms*NSEC_PER_MSEC)+return0;++warned_once=true;++returnneed_resched_latency;+}++staticint__initsetup_resched_latency_warn_ms(char*str)+{+longval;++if((kstrtol(str,0,&val))){+pr_warn("Unable to set resched_latency_warn_ms\n");+return1;+}++sysctl_resched_latency_warn_ms=val;+return1;+}+__setup("resched_latency_warn_ms=",setup_resched_latency_warn_ms);+#else+staticinlineu64resched_latency_check(structrq*rq){return0;}+#endif /* CONFIG_SCHED_DEBUG */++DEFINE_STATIC_KEY_FALSE(resched_latency_warn_enabled);+/**Thisfunctiongetscalledbythetimercode,withHZfrequency.*Wecallitwithinterruptsdisabled.
From: Peter Zijlstra <peterz@infradead.org> Date: 2021-03-24 09:39:34
On Mon, Mar 22, 2021 at 08:57:06PM -0700, Josh Don wrote:
quoted hunk
From: Paul Turner <redacted>
CPU scheduler marks need_resched flag to signal a schedule() on a
particular CPU. But, schedule() may not happen immediately in cases
where the current task is executing in the kernel mode (no
preemption state) for extended periods of time.
This patch adds a warn_on if need_resched is pending for more than the
time specified in sysctl resched_latency_warn_ms. If it goes off, it is
likely that there is a missing cond_resched() somewhere. Monitoring is
done via the tick and the accuracy is hence limited to jiffy scale. This
also means that we won't trigger the warning if the tick is disabled.
This feature is default disabled. It can be toggled on using sysctl
resched_latency_warn_enabled.
Signed-off-by: Paul Turner <redacted>
Signed-off-by: Josh Don <redacted>
---
Delta from v1:
- separate sysctl for enabling/disabling and triggering warn_once
behavior
- add documentation
- static branch for the enable
Documentation/admin-guide/sysctl/kernel.rst | 23 ++++++
include/linux/sched/sysctl.h | 4 ++
kernel/sched/core.c | 78 ++++++++++++++++++++-
kernel/sched/debug.c | 10 +++
kernel/sched/sched.h | 10 +++
kernel/sysctl.c | 24 +++++++
6 files changed, 148 insertions(+), 1 deletion(-)
@@ -1077,6 +1077,29 @@ ROM/Flash boot loader. Maybe to tell it what to do after rebooting. ???+resched_latency_warn_enabled+============================++Enables/disables a warning that will trigger if need_resched is set for+longer than sysctl ``resched_latency_warn_ms``. This warning likely+indicates a kernel bug, such as a failure to call cond_resched().++Requires ``CONFIG_SCHED_DEBUG``.+++resched_latency_warn_ms+=======================++See ``resched_latency_warn_enabled``.+++resched_latency_warn_once+=========================++If set, ``resched_latency_warn_enabled`` will only trigger one warning+per boot.++ sched_energy_aware ==================
Should we perhaps take out all SCHED_DEBUG sysctls and move them to
/debug/sched/ ? (along with the existing /debug/sched_{debug,features,preemp}
files)
Having all that in sysctl and documented gives them far too much sheen
of ABI.
Not saying this patch should do that, just as a general observation.
From: Peter Zijlstra <peterz@infradead.org> Date: 2021-03-24 10:57:18
On Wed, Mar 24, 2021 at 10:37:43AM +0100, Peter Zijlstra wrote:
Should we perhaps take out all SCHED_DEBUG sysctls and move them to
/debug/sched/ ? (along with the existing /debug/sched_{debug,features,preemp}
files)
Having all that in sysctl and documented gives them far too much sheen
of ABI.
... a little something like this ...
---
Subject: sched: Move SCHED_DEBUG to debugfs
From: Peter Zijlstra <peterz@infradead.org>
Date: Wed Mar 24 11:43:21 CET 2021
Stop polluting sysctl with undocumented knobs that really are debug
only, move them all to /debug/sched/.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
include/linux/sched/sysctl.h | 12 ---
kernel/sched/core.c | 48 +-----------
kernel/sched/debug.c | 169 +++++++++++++++++++++++++++++++++++++++++--
kernel/sched/fair.c | 9 --
kernel/sysctl.c | 116 -----------------------------
5 files changed, 174 insertions(+), 180 deletions(-)
From: Peter Zijlstra <peterz@infradead.org> Date: 2021-03-24 10:57:51
On Wed, Mar 24, 2021 at 11:54:24AM +0100, Peter Zijlstra wrote:
On Wed, Mar 24, 2021 at 10:37:43AM +0100, Peter Zijlstra wrote:
quoted
Should we perhaps take out all SCHED_DEBUG sysctls and move them to
/debug/sched/ ? (along with the existing /debug/sched_{debug,features,preemp}
files)
Having all that in sysctl and documented gives them far too much sheen
of ABI.
... a little something like this ...
And then the parent post becomes something like this..
---
Subject: sched: Warn on long periods of pending need_resched
From: Paul Turner <redacted>
Date: Mon, 22 Mar 2021 20:57:06 -0700
From: Paul Turner <redacted>
CPU scheduler marks need_resched flag to signal a schedule() on a
particular CPU. But, schedule() may not happen immediately in cases
where the current task is executing in the kernel mode (no
preemption state) for extended periods of time.
This patch adds a warn_on if need_resched is pending for more than the
time specified in sysctl resched_latency_warn_ms. If it goes off, it is
likely that there is a missing cond_resched() somewhere. Monitoring is
done via the tick and the accuracy is hence limited to jiffy scale. This
also means that we won't trigger the warning if the tick is disabled.
This feature is default disabled.
Signed-off-by: Paul Turner <redacted>
Signed-off-by: Josh Don <redacted>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20210323035706.572953-1-joshdon@google.com
---
include/linux/sched/sysctl.h | 3 +
kernel/sched/core.c | 75 ++++++++++++++++++++++++++++++++++++++++++-
kernel/sched/debug.c | 13 +++++++
kernel/sched/features.h | 2 +
kernel/sched/sched.h | 10 +++++
5 files changed, 102 insertions(+), 1 deletion(-)
@@ -4485,6 +4499,56 @@ unsigned long long task_sched_runtime(streturnns;}+#ifdef CONFIG_SCHED_DEBUG+staticu64resched_latency_check(structrq*rq)+{+intlatency_warn_ms=READ_ONCE(sysctl_resched_latency_warn_ms);+u64need_resched_latency,now=rq_clock(rq);+staticboolwarned_once;++if(sysctl_resched_latency_warn_once&&warned_once)+return0;++if(!need_resched()||WARN_ON_ONCE(latency_warn_ms<2))+return0;++/* Disable this warning for the first few mins after boot */+if(now<resched_boot_quiet_sec*NSEC_PER_SEC)+return0;++if(!rq->last_seen_need_resched_ns){+rq->last_seen_need_resched_ns=now;+rq->ticks_without_resched=0;+return0;+}++rq->ticks_without_resched++;+need_resched_latency=now-rq->last_seen_need_resched_ns;+if(need_resched_latency<=latency_warn_ms*NSEC_PER_MSEC)+return0;++warned_once=true;++returnneed_resched_latency;+}++staticint__initsetup_resched_latency_warn_ms(char*str)+{+longval;++if((kstrtol(str,0,&val))){+pr_warn("Unable to set resched_latency_warn_ms\n");+return1;+}++sysctl_resched_latency_warn_ms=val;+return1;+}+__setup("resched_latency_warn_ms=",setup_resched_latency_warn_ms);+#else+staticinlineu64resched_latency_check(structrq*rq){return0;}+#endif /* CONFIG_SCHED_DEBUG */+/**Thisfunctiongetscalledbythetimercode,withHZfrequency.*Wecallitwithinterruptsdisabled.
On Mon, Mar 22, 2021 at 08:57:06PM -0700, Josh Don wrote:
quoted hunk
From: Paul Turner <redacted>
CPU scheduler marks need_resched flag to signal a schedule() on a
particular CPU. But, schedule() may not happen immediately in cases
where the current task is executing in the kernel mode (no
preemption state) for extended periods of time.
This patch adds a warn_on if need_resched is pending for more than the
time specified in sysctl resched_latency_warn_ms. If it goes off, it is
likely that there is a missing cond_resched() somewhere. Monitoring is
done via the tick and the accuracy is hence limited to jiffy scale. This
also means that we won't trigger the warning if the tick is disabled.
This feature is default disabled. It can be toggled on using sysctl
resched_latency_warn_enabled.
Signed-off-by: Paul Turner <redacted>
Signed-off-by: Josh Don <redacted>
---
Delta from v1:
- separate sysctl for enabling/disabling and triggering warn_once
behavior
- add documentation
- static branch for the enable
Documentation/admin-guide/sysctl/kernel.rst | 23 ++++++
include/linux/sched/sysctl.h | 4 ++
kernel/sched/core.c | 78 ++++++++++++++++++++-
kernel/sched/debug.c | 10 +++
kernel/sched/sched.h | 10 +++
kernel/sysctl.c | 24 +++++++
6 files changed, 148 insertions(+), 1 deletion(-)
@@ -1077,6 +1077,29 @@ ROM/Flash boot loader. Maybe to tell it what to do after rebooting. ???+resched_latency_warn_enabled+============================++Enables/disables a warning that will trigger if need_resched is set for+longer than sysctl ``resched_latency_warn_ms``. This warning likely+indicates a kernel bug, such as a failure to call cond_resched().++Requires ``CONFIG_SCHED_DEBUG``.+
I'm not a fan of the name. I know other sysctls have _enabled in the
name but it's redundant. If you say the name out loud, it sounds weird.
I would suggest an alternative but see below.
+
+resched_latency_warn_ms
+=======================
+
+See ``resched_latency_warn_enabled``.
+
+
+resched_latency_warn_once
+=========================
+
+If set, ``resched_latency_warn_enabled`` will only trigger one warning
+per boot.
+
I suggest semantics and naming similar to hung_task_warnings
because it's sortof similar. resched_latency_warnings would combine
resched_latency_warn_enabled and resched_latency_warn_once. 0 would mean
"never warn", -1 would mean always warn and any positive value means
"warn X number of times".
Internally, you could still use the static label
resched_latency_warn_enabled, it would simply be false if
resched_latency_warnings == 0.
Obviously though sysctl_resched_latency_warn_once would need to change.
@@ -58,7 +58,21 @@ const_debug unsigned int sysctl_sched_features =#include"features.h"0;#undef SCHED_FEAT-#endif++/*+*Printawarningifneed_reschedissetforthegivenduration(if+*resched_latency_warn_enabledisset).+*+*Ifsysctl_resched_latency_warn_onceisset,onlyonewarningwillbeshown+*perboot.+*+*Reschedlatencywillbeignoredforthefirstresched_boot_quiet_sec,to+*reducefalsealarms.+*/+intsysctl_resched_latency_warn_ms=100;+intsysctl_resched_latency_warn_once=1;
Use __read_mostly
+const long resched_boot_quiet_sec = 600;
This seems arbitrary but could also be a #define. More on this later
+#endif /* CONFIG_SCHED_DEBUG */
quoted hunk
/*
* Number of tasks to iterate in a single balance run.
@@ -4520,6 +4534,58 @@ unsigned long long task_sched_runtime(struct task_struct *p) return ns; }+#ifdef CONFIG_SCHED_DEBUG+static u64 resched_latency_check(struct rq *rq)+{+ int latency_warn_ms = READ_ONCE(sysctl_resched_latency_warn_ms);+ u64 need_resched_latency, now = rq_clock(rq);+ static bool warned_once;++ if (sysctl_resched_latency_warn_once && warned_once)+ return 0;+
That is a global variable that can be modified in parallel and I do not
think it's properly locked (scheduler_tick is holding rq lock which does
not protect this).
Consider making resched_latency_warnings atomic and use
atomic_dec_if_positive. If it drops to zero in this path, disable the
static branch.
That said, it may be overkill. hung_task_warnings does not appear to have
special protection that prevents it going to -1 or lower values by accident
either. Maybe it can afford to be a bit more relaxed because a system that
is spamming hung task warnings is probably dead or might as well be dead.
The naming need_resched_latency implies it's a boolean but it's not.
Maybe just resched_latency?
Similarly, resched_latency_check implies it returns a boolean but it
returns an excessive latency value. At this point I've been reading the
patch for a long time so I've ran out of naming suggestions :)
I note that you split when a warning is needed and printing the warning
but it's not clear why. Sure you are under the RQ lock but there are other
places that warn under the RQ lock. I suppose for consistency it could
use SCHED_WARN_ON even though all this code is under SCHED_DEBUG already.
quoted hunk
+static int __init setup_resched_latency_warn_ms(char *str)
+{
+ long val;
+
+ if ((kstrtol(str, 0, &val))) {
+ pr_warn("Unable to set resched_latency_warn_ms\n");
+ return 1;
+ }
+
+ sysctl_resched_latency_warn_ms = val;
+ return 1;
+}
+__setup("resched_latency_warn_ms=", setup_resched_latency_warn_ms);
+#else
+static inline u64 resched_latency_check(struct rq *rq) { return 0; }
+#endif /* CONFIG_SCHED_DEBUG */
+
+DEFINE_STATIC_KEY_FALSE(resched_latency_warn_enabled);
+
/*
* This function gets called by the timer code, with HZ frequency.
* We call it with interrupts disabled.
I don't see the need to split latency detection with the display of the
warning. As resched_latency_check is static with a single caller, it should
be inlined so you can move all the logic, including the static branch
check there. Maybe to be on the safe side, explicitly mark it inline.
That allows you to delete resched_latency_warn and avoid advertising it
through sched.h
--
Mel Gorman
SUSE Labs
On Wed, Mar 24, 2021 at 11:54:24AM +0100, Peter Zijlstra wrote:
On Wed, Mar 24, 2021 at 10:37:43AM +0100, Peter Zijlstra wrote:
quoted
Should we perhaps take out all SCHED_DEBUG sysctls and move them to
/debug/sched/ ? (along with the existing /debug/sched_{debug,features,preemp}
files)
Having all that in sysctl and documented gives them far too much sheen
of ABI.
... a little something like this ...
I did not read this particularly carefully or boot it to check but some
of the sysctls moved are expected to exist and should never should have
been under SCHED_DEBUG.
For example, I'm surprised that numa_balancing is under the SCHED_DEBUG
sysctl because there are legimiate reasons to disable that at runtime.
For example, HPC clusters running various workloads may disable NUMA
balancing globally for particular jobs without wanting to reboot and
reenable it when finished.
Moving something like sched_min_granularity_ns will break a number of
tuning guides as well as the "tuned" tool which ships by default with
some distros and I believe some of the default profiles used for tuned
tweak kernel.sched_min_granularity_ns
Whether there are legimiate reasons to modify those values or not,
removing them may generate fun bug reports.
--
Mel Gorman
SUSE Labs
From: Peter Zijlstra <peterz@infradead.org> Date: 2021-03-24 12:16:12
On Wed, Mar 24, 2021 at 11:42:24AM +0000, Mel Gorman wrote:
On Wed, Mar 24, 2021 at 11:54:24AM +0100, Peter Zijlstra wrote:
quoted
On Wed, Mar 24, 2021 at 10:37:43AM +0100, Peter Zijlstra wrote:
quoted
Should we perhaps take out all SCHED_DEBUG sysctls and move them to
/debug/sched/ ? (along with the existing /debug/sched_{debug,features,preemp}
files)
Having all that in sysctl and documented gives them far too much sheen
of ABI.
... a little something like this ...
I did not read this particularly carefully or boot it to check but some
of the sysctls moved are expected to exist and should never should have
been under SCHED_DEBUG.
For example, I'm surprised that numa_balancing is under the SCHED_DEBUG
sysctl because there are legimiate reasons to disable that at runtime.
For example, HPC clusters running various workloads may disable NUMA
balancing globally for particular jobs without wanting to reboot and
reenable it when finished.
Yeah, lets say I was pleasantly surprised to find it there :-)
Moving something like sched_min_granularity_ns will break a number of
tuning guides as well as the "tuned" tool which ships by default with
some distros and I believe some of the default profiles used for tuned
tweak kernel.sched_min_granularity_ns
Yeah, can't say I care. I suppose some people with PREEMPT=n kernels
increase that to make their server workloads 'go fast'. But I'll
absolutely suck rock on anything desktop.
These knobs really shouldn't have been as widely available as they are.
And guides, well, the writes have to earn a living too, right.
Whether there are legimiate reasons to modify those values or not,
removing them may generate fun bug reports.
Which I'll close with -EDONTCARE, userspace has to cope with
SCHED_DEBUG=n in any case.
On Wed, Mar 24, 2021 at 01:12:16PM +0100, Peter Zijlstra wrote:
On Wed, Mar 24, 2021 at 11:42:24AM +0000, Mel Gorman wrote:
quoted
On Wed, Mar 24, 2021 at 11:54:24AM +0100, Peter Zijlstra wrote:
quoted
On Wed, Mar 24, 2021 at 10:37:43AM +0100, Peter Zijlstra wrote:
quoted
Should we perhaps take out all SCHED_DEBUG sysctls and move them to
/debug/sched/ ? (along with the existing /debug/sched_{debug,features,preemp}
files)
Having all that in sysctl and documented gives them far too much sheen
of ABI.
... a little something like this ...
I did not read this particularly carefully or boot it to check but some
of the sysctls moved are expected to exist and should never should have
been under SCHED_DEBUG.
For example, I'm surprised that numa_balancing is under the SCHED_DEBUG
sysctl because there are legimiate reasons to disable that at runtime.
For example, HPC clusters running various workloads may disable NUMA
balancing globally for particular jobs without wanting to reboot and
reenable it when finished.
Yeah, lets say I was pleasantly surprised to find it there :-)
Minimally, lets move that out before it gets kicked out. Patch below.
quoted
Moving something like sched_min_granularity_ns will break a number of
tuning guides as well as the "tuned" tool which ships by default with
some distros and I believe some of the default profiles used for tuned
tweak kernel.sched_min_granularity_ns
Yeah, can't say I care. I suppose some people with PREEMPT=n kernels
increase that to make their server workloads 'go fast'. But I'll
absolutely suck rock on anything desktop.
Broadly speaking yes and despite the lack of documentation, enough people
think of that parameter when tuning for throughput vs latency depending on
the expected use of the machine. kernel.sched_wakeup_granularity_ns might
get tuned if preemption is causing overscheduling. Same potentially with
kernel.sched_min_granularity_ns and kernel.sched_latency_ns. That said, I'm
struggling to think of an instance where I've seen tuning recommendations
properly quantified other than the impact on microbenchmarks but I
think there will be complaining if they disappear. I suspect that some
recommended tuning is based on "I tried a number of different values and
this seemed to work reasonably well".
kernel.sched_schedstats probably should not depend in SCHED_DEBUG because
it has value for workload analysis which is not necessarily about debugging
per-se. It might simply be informing whether another variable should be
tuned or useful for debugging applications rather than the kernel.
The others I'm less concerned with. kernel.sched_tunable_scaling is very
specific. sysctl_sched_migration_cost is subtle because it affects lots
of things including whether tasks are cache hot and load balancing and
is best left alone. I wonder how many people can accurately predict how
workloads will behave when that is tuned? sched_nr_migrate is also a hard
one to tune in a sensible fashion.
As an aside, I wonder how often SCHED_DEBUG has been enabled simply
because LATENCYTOP selects it -- no idea offhand why LATENCYTOP even
needs SCHED_DEBUG.
These knobs really shouldn't have been as widely available as they are.
Probably not. Worse, some of the tuning is probably based on "this worked
for workload X 10 years ago so I'll just keep doing that"
And guides, well, the writes have to earn a living too, right.
For most of the guides I've seen they either specify values without
explaining why or just describe roughly what the parameter does and it's
not always that accurate a description.
quoted
Whether there are legimiate reasons to modify those values or not,
removing them may generate fun bug reports.
Which I'll close with -EDONTCARE, userspace has to cope with
SCHED_DEBUG=n in any case.
True but removing the throughput vs latency parameters is likely to
generate a lot of noise even if the reasons for tuning are bad ones.
Some definitely should not be depending on SCHED_DEBUG, others may
need to be moved to debugfs one patch at a time so they can be reverted
individually if complaining is excessive and there is a legiminate reason
why it should be tuned. It's possible that complaining will be based on
a workload regression that really depended on tuned changing parameters.
Anyway, I definitely want to save kernel.numa_balancing from the firing
line so....
--8<--
sched/numa: Allow runtime enabling/disabling of NUMA balance without SCHED_DEBUG
From: Mel Gorman <mgorman@suse.de>
The ability to enable/disable NUMA balancing is not a debugging feature
and should not depend on CONFIG_SCHED_DEBUG. For example, machines within
a HPC cluster may disable NUMA balancing temporarily for some jobs and
re-enable it for other jobs without needing to reboot.
This patch removes the dependency on CONFIG_SCHED_DEBUG for
kernel.numa_balancing sysctl. The other numa balancing related sysctls
are left as-is because if they need to be tuned then it is more likely
that NUMA balancing needs to be fixed instead.
Signed-off-by: Mel Gorman <mgorman@suse.de>
---
kernel/sysctl.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Peter Zijlstra <peterz@infradead.org> Date: 2021-03-24 14:37:07
On Wed, Mar 24, 2021 at 01:39:16PM +0000, Mel Gorman wrote:
quoted
Yeah, lets say I was pleasantly surprised to find it there :-)
Minimally, lets move that out before it gets kicked out. Patch below.
OK, stuck that in front.
quoted
quoted
Moving something like sched_min_granularity_ns will break a number of
tuning guides as well as the "tuned" tool which ships by default with
some distros and I believe some of the default profiles used for tuned
tweak kernel.sched_min_granularity_ns
Yeah, can't say I care. I suppose some people with PREEMPT=n kernels
increase that to make their server workloads 'go fast'. But I'll
absolutely suck rock on anything desktop.
Broadly speaking yes and despite the lack of documentation, enough people
think of that parameter when tuning for throughput vs latency depending on
the expected use of the machine. kernel.sched_wakeup_granularity_ns might
get tuned if preemption is causing overscheduling. Same potentially with
kernel.sched_min_granularity_ns and kernel.sched_latency_ns. That said, I'm
struggling to think of an instance where I've seen tuning recommendations
properly quantified other than the impact on microbenchmarks but I
think there will be complaining if they disappear. I suspect that some
recommended tuning is based on "I tried a number of different values and
this seemed to work reasonably well".
Right, except that due to that scaling thing, you'd have to re-evaluate
when you change machine.
Also, do you have any inclination on the perf difference we're talking
about? (I should probably ask Google and not you...)
kernel.sched_schedstats probably should not depend in SCHED_DEBUG because
it has value for workload analysis which is not necessarily about debugging
per-se. It might simply be informing whether another variable should be
tuned or useful for debugging applications rather than the kernel.
Dubious, if you're that far down the rabit hole, you're dang near
debugging.
As an aside, I wonder how often SCHED_DEBUG has been enabled simply
because LATENCYTOP selects it -- no idea offhand why LATENCYTOP even
needs SCHED_DEBUG.
Perhaps schedstats used to rely on debug? I can't remember. I don't
think I've used latencytop in at least 10 years. ftrace and perf sorta
killed the need for it.
quoted
These knobs really shouldn't have been as widely available as they are.
Probably not. Worse, some of the tuning is probably based on "this worked
for workload X 10 years ago so I'll just keep doing that"
That sounds like an excellent reason to disrupt ;-)
quoted
And guides, well, the writes have to earn a living too, right.
For most of the guides I've seen they either specify values without
explaining why or just describe roughly what the parameter does and it's
not always that accurate a description.
Another good reason.
quoted
quoted
Whether there are legimiate reasons to modify those values or not,
removing them may generate fun bug reports.
Which I'll close with -EDONTCARE, userspace has to cope with
SCHED_DEBUG=n in any case.
True but removing the throughput vs latency parameters is likely to
generate a lot of noise even if the reasons for tuning are bad ones.
Some definitely should not be depending on SCHED_DEBUG, others may
need to be moved to debugfs one patch at a time so they can be reverted
individually if complaining is excessive and there is a legiminate reason
why it should be tuned. It's possible that complaining will be based on
a workload regression that really depended on tuned changing parameters.
The way I've done it, you can simply re-instate the systl table entry
and it'll work again, except for the entries that had a custom handler.
I'm ready to disrupt :-)
On Wed, Mar 24, 2021 at 03:36:14PM +0100, Peter Zijlstra wrote:
On Wed, Mar 24, 2021 at 01:39:16PM +0000, Mel Gorman wrote:
quoted
quoted
Yeah, lets say I was pleasantly surprised to find it there :-)
Minimally, lets move that out before it gets kicked out. Patch below.
OK, stuck that in front.
Thanks.
quoted
quoted
quoted
Moving something like sched_min_granularity_ns will break a number of
tuning guides as well as the "tuned" tool which ships by default with
some distros and I believe some of the default profiles used for tuned
tweak kernel.sched_min_granularity_ns
Yeah, can't say I care. I suppose some people with PREEMPT=n kernels
increase that to make their server workloads 'go fast'. But I'll
absolutely suck rock on anything desktop.
Broadly speaking yes and despite the lack of documentation, enough people
think of that parameter when tuning for throughput vs latency depending on
the expected use of the machine. kernel.sched_wakeup_granularity_ns might
get tuned if preemption is causing overscheduling. Same potentially with
kernel.sched_min_granularity_ns and kernel.sched_latency_ns. That said, I'm
struggling to think of an instance where I've seen tuning recommendations
properly quantified other than the impact on microbenchmarks but I
think there will be complaining if they disappear. I suspect that some
recommended tuning is based on "I tried a number of different values and
this seemed to work reasonably well".
Right, except that due to that scaling thing, you'd have to re-evaluate
when you change machine.
Yes although in practice I've rarely seen that happen. What I have seen
is tuning parameters being copied across machines or kernel versions that
turned out to be the source of the "regression" because something changed
in the scheduler that invalidated the tuning.
Also, do you have any inclination on the perf difference we're talking
about? (I should probably ask Google and not you...)
I don't have good data on hand and I don't trust Google for performance
data. However, I know for certain that there are "Enterprise Applications"
whose tuning relies on modifying kernel.sched_min_granularity_ns and
kernel.sched_wakeup_granularity_ns at the very least (might be others,
I'd have to check). The issue was severe enough to fail acceptance testing
for OS upgrades and it generated bugs.
I did not see the raw data but even if I had, it would have been based on
a battery of tests across multiple platforms and generations so at best I
would have a vague range. For the vendors in question, it is unlikely they
would release detailed information because it can be seen as commercially
sensitive. I don't really agree that this is useful behaviour but it is
the reality so don't shoot the messenger :(
The last I checked, hackbench figures could be changed in the 10-15%
range either direction depending on group counts but in itself, that is
not useful.
quoted
kernel.sched_schedstats probably should not depend in SCHED_DEBUG because
it has value for workload analysis which is not necessarily about debugging
per-se. It might simply be informing whether another variable should be
tuned or useful for debugging applications rather than the kernel.
Dubious, if you're that far down the rabit hole, you're dang near
debugging.
Yes, but not necessarily the kernel. For example, the workload analysis
might be to see if the maximum number of threads in a worker pool should
be tuned (either up or down).
quoted
As an aside, I wonder how often SCHED_DEBUG has been enabled simply
because LATENCYTOP selects it -- no idea offhand why LATENCYTOP even
needs SCHED_DEBUG.
Perhaps schedstats used to rely on debug? I can't remember. I don't
think I've used latencytop in at least 10 years. ftrace and perf sorta
killed the need for it.
I don't think schedstats used to rely on SCHED_DEBUG. LATENCYTOP appears
to build even if SCHED_DEBUG is disabled so it was either was an
accident or it's no longer necessary.
quoted
quoted
These knobs really shouldn't have been as widely available as they are.
Probably not. Worse, some of the tuning is probably based on "this worked
for workload X 10 years ago so I'll just keep doing that"
That sounds like an excellent reason to disrupt ;-)
The same logic applies for all tuning unfortunately :P
quoted
quoted
quoted
Whether there are legimiate reasons to modify those values or not,
removing them may generate fun bug reports.
Which I'll close with -EDONTCARE, userspace has to cope with
SCHED_DEBUG=n in any case.
True but removing the throughput vs latency parameters is likely to
generate a lot of noise even if the reasons for tuning are bad ones.
Some definitely should not be depending on SCHED_DEBUG, others may
need to be moved to debugfs one patch at a time so they can be reverted
individually if complaining is excessive and there is a legiminate reason
why it should be tuned. It's possible that complaining will be based on
a workload regression that really depended on tuned changing parameters.
The way I've done it, you can simply re-instate the systl table entry
and it'll work again, except for the entries that had a custom handler.
On Wed, Mar 24, 2021 at 4:27 AM Mel Gorman [off-list ref] wrote:
I'm not a fan of the name. I know other sysctls have _enabled in the
name but it's redundant. If you say the name out loud, it sounds weird.
I would suggest an alternative but see below.
Now using the version rebased by Peter; this control has gone away and
we have simply a scheduling feature "LATENCY WARN"
I suggest semantics and naming similar to hung_task_warnings
because it's sortof similar. resched_latency_warnings would combine
resched_latency_warn_enabled and resched_latency_warn_once. 0 would mean
"never warn", -1 would mean always warn and any positive value means
"warn X number of times".
See above. I'm happy with the enabled bit being toggled separately by
a sched feature; the warn_once behavior is not overloaded with the
enabling/disabling. Also, I don't see value in "warn X number of
times", given the warning is rate limited anyway.
That is a global variable that can be modified in parallel and I do not
think it's properly locked (scheduler_tick is holding rq lock which does
not protect this).
Consider making resched_latency_warnings atomic and use
atomic_dec_if_positive. If it drops to zero in this path, disable the
static branch.
That said, it may be overkill. hung_task_warnings does not appear to have
special protection that prevents it going to -1 or lower values by accident
either. Maybe it can afford to be a bit more relaxed because a system that
is spamming hung task warnings is probably dead or might as well be dead.
There's no real issue if we race over modification to that sysctl.
This is intentionally not more strongly synchronized for that reason.
The naming need_resched_latency implies it's a boolean but it's not.
Maybe just resched_latency?
Similarly, resched_latency_check implies it returns a boolean but it
returns an excessive latency value. At this point I've been reading the
patch for a long time so I've ran out of naming suggestions :)
The "need_" part does confuse it a bit; I reworded these to hopefully
make it more clear.
I note that you split when a warning is needed and printing the warning
but it's not clear why. Sure you are under the RQ lock but there are other
places that warn under the RQ lock. I suppose for consistency it could
use SCHED_WARN_ON even though all this code is under SCHED_DEBUG already.
We had seen a circular lock dependency warning (console_sem, pi lock,
rq lock), since printing might need to wake a waiter. However, I do
see plenty of warns under rq->lock, so maybe I missed a patch to
address this?
On Wed, Mar 24, 2021 at 01:39:16PM +0000, Mel Gorman wrote:
I'm not going to NAK because I do not have hard data that shows they must
exist. However, I won't ACK either because I bet a lot of tasty beverages
the next time we meet that the following parameters will generate reports
if removed.
kernel.sched_latency_ns
kernel.sched_migration_cost_ns
kernel.sched_min_granularity_ns
kernel.sched_wakeup_granularity_ns
I know they are altered by tuned for different profiles and some people do
go the effort to create custom profiles for specific applications. They
also show up in "Official Benchmarking" such as SPEC CPU 2017 and
some vendors put a *lot* of effort into SPEC CPU results for bragging
rights. They show up in technical books and best practice guids for
applications. Finally they show up in Google when searching for "tuning
sched_foo". I'm not saying that any of these are even accurate or a good
idea, just that they show up near the top of the results and they are
sufficiently popular that they might as well be an ABI.
+1, these seem like sufficiently well-known scheduler tunables, and
not really SCHED_DEBUG.
From: Peter Zijlstra <peterz@infradead.org> Date: 2021-03-26 08:59:48
On Thu, Mar 25, 2021 at 02:58:52PM -0700, Josh Don wrote:
quoted
On Wed, Mar 24, 2021 at 01:39:16PM +0000, Mel Gorman wrote:
I'm not going to NAK because I do not have hard data that shows they must
exist. However, I won't ACK either because I bet a lot of tasty beverages
the next time we meet that the following parameters will generate reports
if removed.
kernel.sched_latency_ns
kernel.sched_migration_cost_ns
kernel.sched_min_granularity_ns
kernel.sched_wakeup_granularity_ns
I know they are altered by tuned for different profiles and some people do
go the effort to create custom profiles for specific applications. They
also show up in "Official Benchmarking" such as SPEC CPU 2017 and
some vendors put a *lot* of effort into SPEC CPU results for bragging
rights. They show up in technical books and best practice guids for
applications. Finally they show up in Google when searching for "tuning
sched_foo". I'm not saying that any of these are even accurate or a good
idea, just that they show up near the top of the results and they are
sufficiently popular that they might as well be an ABI.
+1, these seem like sufficiently well-known scheduler tunables, and
not really SCHED_DEBUG.
So we've never made any guarantees on their behaviour, nor am I willing
to make any.
In fact, I propose we merge the below along with the debugfs move. Just
to make absolutely sure any 'tuning' is broken.
---
Subject: sched,fair: Alternative sched_slice()
From: Peter Zijlstra <peterz@infradead.org>
Date: Thu Mar 25 13:44:46 CET 2021
The current sched_slice() seems to have issues; there's two possible
things that could be improved:
- the 'nr_running' used for __sched_period() is daft when cgroups are
considered. Using the RQ wide h_nr_running seems like a much more
consistent number.
- (esp) cgroups can slice it real fine (pun intendend), which makes for
easy over-scheduling, ensure min_gran is what the name says.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
kernel/sched/fair.c | 15 ++++++++++++++-
kernel/sched/features.h | 3 +++
2 files changed, 17 insertions(+), 1 deletion(-)
Peter,
Since you've already pulled the need_resched warning patch into your
tree, I'm including just the diff based on that patch (in response to
Mel's comments) below. This should be squashed into the original
patch.
Thanks,
Josh
---
From 85796b4d299b1cf3f99bde154a356ce1061221b7 Mon Sep 17 00:00:00 2001
From: Josh Don <redacted>
Date: Mon, 22 Mar 2021 20:57:06 -0700
Subject: [PATCH] fixup: sched: Warn on long periods of pending need_resched
---
kernel/sched/core.c | 29 ++++++++++++-----------------
1 file changed, 12 insertions(+), 17 deletions(-)
From: Peter Zijlstra <peterz@infradead.org> Date: 2021-04-16 15:06:49
On Tue, Mar 30, 2021 at 03:44:12PM -0700, Josh Don wrote:
Peter,
Since you've already pulled the need_resched warning patch into your
tree, I'm including just the diff based on that patch (in response to
Mel's comments) below. This should be squashed into the original
patch.
Sorry, I seem to have missed this. The patch is completely whitespace
mangled through.
I've attached my latest copy of the patch and held it back for now,
please resubmit.
---
Subject: sched: Warn on long periods of pending need_resched
From: Paul Turner <redacted>
Date: Mon, 22 Mar 2021 20:57:06 -0700
From: Paul Turner <redacted>
CPU scheduler marks need_resched flag to signal a schedule() on a
particular CPU. But, schedule() may not happen immediately in cases
where the current task is executing in the kernel mode (no
preemption state) for extended periods of time.
This patch adds a warn_on if need_resched is pending for more than the
time specified in sysctl resched_latency_warn_ms. If it goes off, it is
likely that there is a missing cond_resched() somewhere. Monitoring is
done via the tick and the accuracy is hence limited to jiffy scale. This
also means that we won't trigger the warning if the tick is disabled.
This feature is default disabled. It can be toggled on using sysctl
resched_latency_warn_enabled.
Signed-off-by: Paul Turner <redacted>
Signed-off-by: Josh Don <redacted>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20210323035706.572953-1-joshdon@google.com
---
include/linux/sched/sysctl.h | 3 +
kernel/sched/core.c | 75 ++++++++++++++++++++++++++++++++++++++++++-
kernel/sched/debug.c | 13 +++++++
kernel/sched/features.h | 2 +
kernel/sched/sched.h | 10 +++++
5 files changed, 102 insertions(+), 1 deletion(-)
@@ -4527,6 +4541,56 @@ unsigned long long task_sched_runtime(streturnns;}+#ifdef CONFIG_SCHED_DEBUG+staticu64resched_latency_check(structrq*rq)+{+intlatency_warn_ms=READ_ONCE(sysctl_resched_latency_warn_ms);+u64need_resched_latency,now=rq_clock(rq);+staticboolwarned_once;++if(sysctl_resched_latency_warn_once&&warned_once)+return0;++if(!need_resched()||WARN_ON_ONCE(latency_warn_ms<2))+return0;++/* Disable this warning for the first few mins after boot */+if(now<resched_boot_quiet_sec*NSEC_PER_SEC)+return0;++if(!rq->last_seen_need_resched_ns){+rq->last_seen_need_resched_ns=now;+rq->ticks_without_resched=0;+return0;+}++rq->ticks_without_resched++;+need_resched_latency=now-rq->last_seen_need_resched_ns;+if(need_resched_latency<=latency_warn_ms*NSEC_PER_MSEC)+return0;++warned_once=true;++returnneed_resched_latency;+}++staticint__initsetup_resched_latency_warn_ms(char*str)+{+longval;++if((kstrtol(str,0,&val))){+pr_warn("Unable to set resched_latency_warn_ms\n");+return1;+}++sysctl_resched_latency_warn_ms=val;+return1;+}+__setup("resched_latency_warn_ms=",setup_resched_latency_warn_ms);+#else+staticinlineu64resched_latency_check(structrq*rq){return0;}+#endif /* CONFIG_SCHED_DEBUG */+/**Thisfunctiongetscalledbythetimercode,withHZfrequency.*Wecallitwithinterruptsdisabled.
On Fri, Apr 16, 2021 at 8:05 AM Peter Zijlstra [off-list ref] wrote:
On Tue, Mar 30, 2021 at 03:44:12PM -0700, Josh Don wrote:
quoted
Peter,
Since you've already pulled the need_resched warning patch into your
tree, I'm including just the diff based on that patch (in response to
Mel's comments) below. This should be squashed into the original
patch.
Sorry, I seem to have missed this. The patch is completely whitespace
mangled through.
I've attached my latest copy of the patch and held it back for now,
please resubmit.
Yikes, sorry about that. I've squashed the fixup and sent the updated
patch (with trimmed cc list): https://lkml.org/lkml/2021/4/16/1125
Thanks,
Josh
For future reference, please use: https://lkml.kernel.org/r/MSGID
Then I can search for MSGID in my local mailer (mutt: ~h) and instantly
find the actual message (now I'll search for all email from you and it
should obviously be easily found as well).