From: Tim Chen <tim.c.chen@linux.intel.com>
To: Peter Zijlstra <peterz@infradead.org>,
mingo@kernel.org, vincent.guittot@linaro.org
Cc: linux-kernel@vger.kernel.org, juri.lelli@redhat.com,
dietmar.eggemann@arm.com, rostedt@goodmis.org,
bsegall@google.com, mgorman@suse.de, bristot@redhat.com,
corbet@lwn.net, qyousef@layalina.io, chris.hyser@oracle.com,
patrick.bellasi@matbug.net, pjt@google.com, pavel@ucw.cz,
qperret@google.com, joshdon@google.com, timj@gnu.org,
kprateek.nayak@amd.com, yu.c.chen@intel.com,
youssefesmat@chromium.org, joel@joelfernandes.org
Subject: Re: [PATCH 08/10] sched/fair: Add lag based placement
Date: Thu, 16 Mar 2023 15:49:31 -0700 [thread overview]
Message-ID: <75730c46f226722b71dd1ec9634a1eb90017f116.camel@linux.intel.com> (raw)
In-Reply-To: <20230306141502.691294694@infradead.org>
On Mon, 2023-03-06 at 14:25 +0100, Peter Zijlstra wrote:
> With the introduction of avg_vruntime, it is possible to approximate
> lag (the entire purpose of introducing it in fact). Use this to do lag
> based placement over sleep+wake.
>
> Specifically, the FAIR_SLEEPERS thing places things too far to the
> left and messes up the deadline aspect of EEVDF.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> include/linux/sched.h | 1
> kernel/sched/core.c | 1
> kernel/sched/fair.c | 63 +++++++++++++++++++++++++++---------------------
> kernel/sched/features.h | 8 ++++++
> 4 files changed, 46 insertions(+), 27 deletions(-)
>
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -555,6 +555,7 @@ struct sched_entity {
> u64 sum_exec_runtime;
> u64 vruntime;
> u64 prev_sum_exec_runtime;
> + s64 lag;
>
> u64 nr_migrations;
>
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -4436,6 +4436,7 @@ static void __sched_fork(unsigned long c
> p->se.prev_sum_exec_runtime = 0;
> p->se.nr_migrations = 0;
> p->se.vruntime = 0;
> + p->se.lag = 0;
> INIT_LIST_HEAD(&p->se.group_node);
>
> set_latency_offset(p);
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4749,39 +4749,45 @@ static void
> place_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int initial)
> {
> u64 vruntime = avg_vruntime(cfs_rq);
> - u64 sleep_time;
>
> - /* 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 (sched_feat(FAIR_SLEEPERS)) {
> + u64 sleep_time;
> +
> + /* 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;
> +
> + /*
> + * Halve their sleep time's effect, to allow
> + * for a gentler effect of sleepers:
> + */
> + if (sched_feat(GENTLE_FAIR_SLEEPERS))
> + thresh >>= 1;
> +
> + vruntime -= thresh;
> + }
>
> /*
> - * Halve their sleep time's effect, to allow
> - * for a gentler effect of sleepers:
> + * Pull vruntime of the entity being placed to the base level of
> + * cfs_rq, to prevent boosting it if placed backwards. If the entity
> + * slept for a long time, don't even try to compare its vruntime with
> + * the base as it may be too far off and the comparison may get
> + * inversed due to s64 overflow.
> */
> - if (sched_feat(GENTLE_FAIR_SLEEPERS))
> - thresh >>= 1;
> -
> - vruntime -= thresh;
> + sleep_time = rq_clock_task(rq_of(cfs_rq)) - se->exec_start;
> + if ((s64)sleep_time < 60LL * NSEC_PER_SEC)
> + vruntime = max_vruntime(se->vruntime, vruntime);
> }
>
> - /*
> - * Pull vruntime of the entity being placed to the base level of
> - * cfs_rq, to prevent boosting it if placed backwards. If the entity
> - * slept for a long time, don't even try to compare its vruntime with
> - * the base as it may be too far off and the comparison may get
> - * inversed due to s64 overflow.
> - */
> - sleep_time = rq_clock_task(rq_of(cfs_rq)) - se->exec_start;
> - if ((s64)sleep_time > 60LL * NSEC_PER_SEC)
> - se->vruntime = vruntime;
> - else
> - se->vruntime = max_vruntime(se->vruntime, vruntime);
> + if (sched_feat(PRESERVE_LAG))
> + vruntime -= se->lag;
> +
> + se->vruntime = vruntime;
I was going to say that
when we migrate a task to a new runqueue, we subtract vruntime
by old queue's min_vruntime and that math needs update.
But then I saw you did that in
https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/commit/kernel/sched?h=sched/eevdf
With this new lag based placement, I think it should properly fix
the starvation issues we have seen caused by quickly cpu hopping tasks.
Tim
> }
>
> static void check_enqueue_throttle(struct cfs_rq *cfs_rq);
> @@ -4949,6 +4955,9 @@ dequeue_entity(struct cfs_rq *cfs_rq, st
>
> clear_buddies(cfs_rq, se);
>
> + if (sched_feat(PRESERVE_LAG) && (flags & DEQUEUE_SLEEP))
> + se->lag = avg_vruntime(cfs_rq) - se->vruntime;
> +
> if (se != cfs_rq->curr)
> __dequeue_entity(cfs_rq, se);
> se->on_rq = 0;
> --- a/kernel/sched/features.h
> +++ b/kernel/sched/features.h
> @@ -1,12 +1,20 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> +
> /*
> * Only give sleepers 50% of their service deficit. This allows
> * them to run sooner, but does not allow tons of sleepers to
> * rip the spread apart.
> */
> +SCHED_FEAT(FAIR_SLEEPERS, false)
> SCHED_FEAT(GENTLE_FAIR_SLEEPERS, true)
>
> /*
> + * Using the avg_vruntime, do the right thing and preserve lag
> + * across sleep+wake cycles.
> + */
> +SCHED_FEAT(PRESERVE_LAG, true)
> +
> +/*
> * Prefer to schedule the task we woke last (assuming it failed
> * wakeup-preemption), since its likely going to consume data we
> * touched, increases cache locality.
>
>
next prev parent reply other threads:[~2023-03-16 22:49 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-06 13:25 [PATCH 00/10] sched: EEVDF using latency-nice Peter Zijlstra
2023-03-06 13:25 ` [PATCH 01/10] sched: Introduce latency-nice as a per-task attribute Peter Zijlstra
2023-03-06 13:25 ` [PATCH 02/10] sched/core: Propagate parent tasks latency requirements to the child task Peter Zijlstra
2023-03-06 13:25 ` [PATCH 03/10] sched: Allow sched_{get,set}attr to change latency_nice of the task Peter Zijlstra
2023-03-06 13:25 ` [PATCH 04/10] sched/fair: Add latency_offset Peter Zijlstra
2023-03-06 13:25 ` [PATCH 05/10] sched/fair: Add sched group latency support Peter Zijlstra
2023-03-06 13:25 ` [PATCH 06/10] sched/fair: Add avg_vruntime Peter Zijlstra
2023-03-21 13:58 ` Chen Yu
2023-03-21 16:04 ` Peter Zijlstra
2023-03-24 7:12 ` Chen Yu
2023-03-24 10:03 ` Peter Zijlstra
2023-03-06 13:25 ` [PATCH 07/10] sched/fair: Remove START_DEBIT Peter Zijlstra
2023-03-06 13:25 ` [PATCH 08/10] sched/fair: Add lag based placement Peter Zijlstra
2023-03-16 22:49 ` Tim Chen [this message]
2023-03-06 13:25 ` [PATCH 09/10] rbtree: Add rb_add_augmented_cached() helper Peter Zijlstra
2023-03-06 13:25 ` [PATCH 10/10] sched/fair: Implement an EEVDF like policy Peter Zijlstra
2023-03-08 8:39 ` Mike Galbraith
2023-03-08 9:26 ` Mike Galbraith
2023-03-08 13:36 ` Mike Galbraith
2023-03-09 4:23 ` Mike Galbraith
2023-03-10 20:38 ` Peter Zijlstra
2023-03-11 5:53 ` Mike Galbraith
2023-03-11 7:56 ` Mike Galbraith
2023-03-09 9:06 ` Peter Zijlstra
2023-03-09 12:44 ` Peter Zijlstra
2023-03-09 15:29 ` Peter Zijlstra
2023-03-09 15:39 ` Peter Zijlstra
2023-03-09 16:24 ` Mike Galbraith
2023-03-09 16:42 ` Peter Zijlstra
2023-03-07 10:27 ` [PATCH 00/10] sched: EEVDF using latency-nice Vincent Guittot
2023-03-07 13:08 ` Peter Zijlstra
2023-03-08 15:13 ` Shrikanth Hegde
2023-03-22 6:49 ` K Prateek Nayak
2023-03-22 9:38 ` K Prateek Nayak
2023-03-23 11:53 ` Pavel Machek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=75730c46f226722b71dd1ec9634a1eb90017f116.camel@linux.intel.com \
--to=tim.c.chen@linux.intel.com \
--cc=bristot@redhat.com \
--cc=bsegall@google.com \
--cc=chris.hyser@oracle.com \
--cc=corbet@lwn.net \
--cc=dietmar.eggemann@arm.com \
--cc=joel@joelfernandes.org \
--cc=joshdon@google.com \
--cc=juri.lelli@redhat.com \
--cc=kprateek.nayak@amd.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mgorman@suse.de \
--cc=mingo@kernel.org \
--cc=patrick.bellasi@matbug.net \
--cc=pavel@ucw.cz \
--cc=peterz@infradead.org \
--cc=pjt@google.com \
--cc=qperret@google.com \
--cc=qyousef@layalina.io \
--cc=rostedt@goodmis.org \
--cc=timj@gnu.org \
--cc=vincent.guittot@linaro.org \
--cc=youssefesmat@chromium.org \
--cc=yu.c.chen@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).