lkml.org 
[lkml]   [2021]   [Sep]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 2/2] sched/fair: Scale wakeup granularity relative to nr_running
On Wed, 22 Sept 2021 at 16:15, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Wed, 22 Sept 2021 at 15:20, Mel Gorman <mgorman@techsingularity.net> wrote:
> >
> > On Wed, Sep 22, 2021 at 07:22:20AM +0200, Mike Galbraith wrote:
> > > On Tue, 2021-09-21 at 11:36 +0100, Mel Gorman wrote:
> > > > On Tue, Sep 21, 2021 at 05:52:32AM +0200, Mike Galbraith wrote:
> > > >
> > > >
> > > > > Preemption does rapidly run into diminishing return as load climbs for
> > > > > a lot of loads, but as you know, it's a rather sticky wicket because
> > > > > even when over-committed, preventing light control threads from slicing
> > > > > through (what can be a load's own work crew of) hogs can seriously
> > > > > injure performance.
> > > > >
> > > >
> > > > Turning this into a classic Rob Peter To Pay Paul problem. We don't know
> > > > if there is a light control thread that needs to run or not that affects
> > > > overall performance. It all depends on whether that control thread needs
> > > > to make progress for the overall workload or whether there are a mix of
> > > > workloads resulting in overloading.
> > >
> > > WRT overload, and our good buddies Peter and Paul :) I added...
> > > if (gran >= sysctl_sched_latency >> 1)
> > > trace_printk("runnable:%d preempt disabled\n",cfs_rq->nr_running);
> > > ...to watch, and met the below when I.. logged in.
> > >
> > > homer:..debug/tracing # tail -20 trace
> > > X-2229 [002] d..5. 60.690322: wakeup_gran: runnable:9 preempt disabled
> > > X-2229 [002] d..5. 60.690325: wakeup_gran: runnable:10 preempt disabled
> > > X-2229 [002] d..5. 60.690330: wakeup_gran: runnable:11 preempt disabled
> > > X-2229 [002] d..5. 60.690363: wakeup_gran: runnable:13 preempt disabled
> > > X-2229 [002] d..5. 60.690377: wakeup_gran: runnable:14 preempt disabled
> > > X-2229 [002] d..5. 60.690390: wakeup_gran: runnable:15 preempt disabled
> > > X-2229 [002] d..5. 60.690404: wakeup_gran: runnable:16 preempt disabled
> > > X-2229 [002] d..5. 60.690425: wakeup_gran: runnable:9 preempt disabled
> > > ksmserver-2694 [003] d..3. 60.690432: wakeup_gran: runnable:6 preempt disabled
> > > ksmserver-2694 [003] d..3. 60.690436: wakeup_gran: runnable:7 preempt disabled
> > > X-2229 [002] d..5. 60.690451: wakeup_gran: runnable:6 preempt disabled
> > > X-2229 [002] d..5. 60.690465: wakeup_gran: runnable:7 preempt disabled
> > > kmix-2736 [000] d..3. 60.690491: wakeup_gran: runnable:6 preempt disabled
> > > X-2229 [004] d..5. 92.889635: wakeup_gran: runnable:6 preempt disabled
> > > X-2229 [004] d..5. 92.889675: wakeup_gran: runnable:6 preempt disabled
> > > X-2229 [004] d..5. 92.889863: wakeup_gran: runnable:6 preempt disabled
> > > X-2229 [004] d..5. 92.889944: wakeup_gran: runnable:6 preempt disabled
> > > X-2229 [004] d..5. 92.889957: wakeup_gran: runnable:7 preempt disabled
> > > X-2229 [004] d..5. 92.889968: wakeup_gran: runnable:8 preempt disabled
> > > QXcbEventQueue-2740 [000] d..4. 92.890025: wakeup_gran: runnable:6 preempt disabled
> > > homer:..debug/tracing
> > >
> > > Watching 'while sleep 1; do clear;tail trace; done' with nothing but a
> > > kbuild running is like watching top. There's enough stacking during
> > > routine use of my desktop box that it runs into the tick granularity
> > > wall pretty much continuously, so 'overload' may want redefining.
> > >
> >
> > Ok, that's pretty convincing. You didn't mention if there were
> > interactivity glitches but it's possible. This is what I'm currently
> > testing but have no results for yet. It caps wakeup_gran at
> > sysctl_sched_latency.
> >
> > ---8<---
> > sched/fair: Scale wakeup granularity relative to nr_running
> >
> > Commit 8a99b6833c88 ("sched: Move SCHED_DEBUG sysctl to debugfs") moved
> > the kernel.sched_wakeup_granularity_ns sysctl under debugfs. One of the
> > reasons why this sysctl may be used may be for "optimising for throughput",
> > particularly when overloaded. The tool TuneD sometimes alters this for two
> > profiles e.g. "mssql" and "throughput-performance". At least version 2.9
> > does but it changed in master where it also will poke at debugfs instead.
> >
> > Internal parameters like sysctl_sched_wakeup_granularity are scaled
> > based on the number of CPUs due to sysctl_sched_tunable_scaling. For
> > simplicity, the timing figures in this changelog are based on
> > SCHED_TUNABLESCALING_NONE.
>
> This is a bit misleading because the platform that you used to
> highlight the problem has a 7ms sysctl_sched_wakeup_granularity. which
> is far more than your tick which should be 1ms
>
> >
> > During task migration or wakeup, a decision is made on whether
> > to preempt the current task or not. To limit over-scheduled,
> > sysctl_sched_wakeup_granularity delays the preemption to allow at least 1ms
> > of runtime before preempting. However, when a domain is heavily overloaded
> > (e.g. hackbench), the degree of over-scheduling is still severe. This is
> > problematic as time is wasted rescheduling tasks that could instead be
> > used by userspace tasks.
> >
> > However, care must be taken. Even if a system is overloaded, there may
> > be high priority threads that must still be able to run. Mike Galbraith
> > explained the contraints as follows;
> >
> > CFS came about because the O1 scheduler was unfair to the
> > point it had starvation problems. People pretty much across the
> > board agreed that a fair scheduler was a much way better way
> > to go, and CFS was born. It didn't originally have the sleep
> > credit business, but had to grow it to become _short term_ fair.
> > Ingo cut the sleep credit in half because of overscheduling, and
> > that has worked out pretty well all told.. but now you're pushing
> > it more in the unfair direction, all the way to extremely unfair
> > for anything and everything very light.
> >
> > Fairness isn't the holy grail mind you, and at some point, giving
> > up on short term fairness certainly isn't crazy, as proven by your
> > hackbench numbers and other numbers we've seen over the years,
> > but taking bites out of the 'CF' in the CFS that was born to be a
> > corner-case killer is.. worrisome. The other shoe will drop.. it
> > always does :)
> >
> > This patch scales the wakeup granularity based on the number of running
> > tasks on the CPU relative to
> >
> > sched_nr_disable_gran = sysctl_sched_latency / sysctl_sched_wakeup_granularity
> >
> > By default, this will allow the wakeup_gran to scale from
> > sysctl_sched_wakeup_granularity up to sysctl_sched_wakeup_granularity up to
> > sysctl_sched_latency depending on the number of running tasks on a cfs_rq.
> > By default, the limit is 6ms.
> >
> > Note that the TuneD throughput-performance profile allows up to 15ms
> > for sysctl_sched_latency (ignoring scaling) but there is no explanation
> > why such a long period was necessary or why sched_latency_ns is also
> > not adjusted. The intent may have been to disable wakeup preemption
> > or it might be an oversight. An internet search for instances where
> > sysctl_sched_wakeup_granularity parameter are tuned to high values offer
> > either no explanation or a broken one.
> >
> > TODO: Results positive or negative
> >
> > Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> > ---
> > kernel/sched/fair.c | 64 ++++++++++++++++++++++++++++++++++++++++++-------
> > kernel/sched/features.h | 6 +++++
> > 2 files changed, 61 insertions(+), 9 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index ff69f245b939..5ec3b12039d6 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -84,6 +84,14 @@ static unsigned int normalized_sysctl_sched_wakeup_granularity = 1000000UL;
> >
> > const_debug unsigned int sysctl_sched_migration_cost = 500000UL;
> >
> > +/*
> > + * This value is kept at sysctl_sched_latency / sysctl_sched_wakeup_granularity
> > + *
> > + * This influences the decision on whether a waking task can preempt a running
> > + * task.
> > + */
> > +static unsigned int sched_nr_disable_gran = 6;
> > +
> > int sched_thermal_decay_shift;
> > static int __init setup_sched_thermal_decay_shift(char *str)
> > {
> > @@ -627,6 +635,9 @@ int sched_update_scaling(void)
> > sched_nr_latency = DIV_ROUND_UP(sysctl_sched_latency,
> > sysctl_sched_min_granularity);
> >
> > + sched_nr_disable_gran = DIV_ROUND_UP(sysctl_sched_latency,
> > + sysctl_sched_wakeup_granularity);
> > +
> > #define WRT_SYSCTL(name) \
> > (normalized_sysctl_##name = sysctl_##name / (factor))
> > WRT_SYSCTL(sched_min_granularity);
> > @@ -4511,7 +4522,8 @@ set_next_entity(struct cfs_rq *cfs_rq, struct sched_entity *se)
> > }
> >
> > static int
> > -wakeup_preempt_entity(struct sched_entity *curr, struct sched_entity *se);
> > +wakeup_preempt_entity(struct cfs_rq *cfs_rq, struct sched_entity *curr,
> > + struct sched_entity *se);
> >
> > /*
> > * Pick the next process, keeping these things in mind, in this order:
> > @@ -4550,16 +4562,16 @@ pick_next_entity(struct cfs_rq *cfs_rq, struct sched_entity *curr)
> > second = curr;
> > }
> >
> > - if (second && wakeup_preempt_entity(second, left) < 1)
> > + if (second && wakeup_preempt_entity(NULL, second, left) < 1)
>
> Why not applying the same policy here ? the tick can also prevent
> current task to move forward
>
> > se = second;
> > }
> >
> > - if (cfs_rq->next && wakeup_preempt_entity(cfs_rq->next, left) < 1) {
> > + if (cfs_rq->next && wakeup_preempt_entity(NULL, cfs_rq->next, left) < 1) {
> > /*
> > * Someone really wants this to run. If it's not unfair, run it.
> > */
> > se = cfs_rq->next;
> > - } else if (cfs_rq->last && wakeup_preempt_entity(cfs_rq->last, left) < 1) {
> > + } else if (cfs_rq->last && wakeup_preempt_entity(NULL, cfs_rq->last, left) < 1) {
> > /*
> > * Prefer last buddy, try to return the CPU to a preempted task.
> > */
> > @@ -7044,9 +7056,42 @@ balance_fair(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
> > }
> > #endif /* CONFIG_SMP */
> >
> > -static unsigned long wakeup_gran(struct sched_entity *se)
> > +static unsigned long
> > +select_wakeup_gran(struct cfs_rq *cfs_rq)
> > +{
> > + unsigned int nr_running, threshold;
> > +
> > + if (!cfs_rq || !sched_feat(SCALE_WAKEUP_GRAN))
> > + return sysctl_sched_wakeup_granularity;
> > +
> > + /* !GENTLE_FAIR_SLEEPERS has one overload threshold. */
> > + if (!sched_feat(GENTLE_FAIR_SLEEPERS)) {
> > + if (cfs_rq->nr_running <= sched_nr_disable_gran)
>
> cfs_rq->nr_running reflects the number of sched entities in the cfs
> but not the number of running tasks which reflected in h_nr_running
>
> Also do you want to take into account only tasks in this cfs and its
> children or on all cfs on this rq ?
>
> > + return sysctl_sched_wakeup_granularity;
> > +
> > + return sysctl_sched_latency;
> > + }
> > +
> > + /* GENTLE_FAIR_SLEEPER has two overloaded thresholds. */
> > + nr_running = cfs_rq->nr_running;
> > + threshold = sched_nr_disable_gran >> 1;
> > +
> > + /* No overload. */
> > + if (nr_running <= threshold)
> > + return sysctl_sched_wakeup_granularity;
>
> TBH I don't like these "no overload", "light overload" ... They don't
> have any real meaning apart from that it might work for your platform
> and your hackbench test.
> We already had have people complaining that small cfs task does not
> preempt fast enough curr task as an example
>
> There is no explanation why these values are the correct ones and not
> but are just some random heuristic results and we are trying to remove
> platform heuristic and to not add new
>
> > +
> > + /* Light overload. */
> > + if (nr_running <= sched_nr_disable_gran)
> > + return sysctl_sched_latency >> 1;
> > +
> > + /* Heavy overload. */
> > + return sysctl_sched_latency;
>
> Why should a thread without any relationship with the curr, not
> preempt it because there are already a lot of running threads ? In
> your case, you want hackbench threads to not preempt each others
> because they tries to use same resources so it's probably better to
> let the current one to move forward but that's not a universal policy.
>
> side question: Have you try to change the nice priority which also
> impact whether a thread can preempt curr ?

There were also some discussions about adding a nice_latency prio so a
task/group can be set if they want to be nice with others from a
scheduling latency pov. One open point was to define what it mean to
be nice from a latency pov
>
> > +}
> > +
> > +static unsigned long
> > +wakeup_gran(struct cfs_rq *cfs_rq, struct sched_entity *se)
> > {
> > - unsigned long gran = sysctl_sched_wakeup_granularity;
> > + unsigned long gran = select_wakeup_gran(cfs_rq);
> >
> > /*
> > * Since its curr running now, convert the gran from real-time
> > @@ -7079,14 +7124,15 @@ static unsigned long wakeup_gran(struct sched_entity *se)
> > *
> > */
> > static int
> > -wakeup_preempt_entity(struct sched_entity *curr, struct sched_entity *se)
> > +wakeup_preempt_entity(struct cfs_rq *cfs_rq, struct sched_entity *curr,
> > + struct sched_entity *se)
> > {
> > s64 gran, vdiff = curr->vruntime - se->vruntime;
> >
> > if (vdiff <= 0)
> > return -1;
> >
> > - gran = wakeup_gran(se);
> > + gran = wakeup_gran(cfs_rq, se);
> > if (vdiff > gran)
> > return 1;
> >
> > @@ -7191,7 +7237,7 @@ static void check_preempt_wakeup(struct rq *rq, struct task_struct *p, int wake_
> > return;
> >
> > update_curr(cfs_rq_of(se));
> > - if (wakeup_preempt_entity(se, pse) == 1) {
> > + if (wakeup_preempt_entity(cfs_rq, se, pse) == 1) {
>
> like for update_curr above, cfs_rq can be wrong because se could have changed
>
> > /*
> > * Bias pick_next to pick the sched entity that is
> > * triggering this preemption.
> > diff --git a/kernel/sched/features.h b/kernel/sched/features.h
> > index 7f8dace0964c..d041d7023029 100644
> > --- a/kernel/sched/features.h
> > +++ b/kernel/sched/features.h
> > @@ -95,3 +95,9 @@ SCHED_FEAT(LATENCY_WARN, false)
> >
> > SCHED_FEAT(ALT_PERIOD, true)
> > SCHED_FEAT(BASE_SLICE, true)
> > +
> > +/*
> > + * Scale sched_wakeup_granularity dynamically based on the number of running
> > + * tasks up to a cap of sysctl_sched_latency.
> > + */
> > +SCHED_FEAT(SCALE_WAKEUP_GRAN, true)

\
 
 \ /
  Last update: 2021-09-22 17:06    [W:0.126 / U:0.508 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site