* Re: [PATCH v4 5/8] sched/fair: Take into account latency priority at wakeup
[not found] <20220916080305.29574-6-vincent.guittot@linaro.org>
@ 2022-09-16 12:02 ` Hillf Danton
2022-09-16 13:36 ` Vincent Guittot
0 siblings, 1 reply; 6+ messages in thread
From: Hillf Danton @ 2022-09-16 12:02 UTC (permalink / raw)
To: Vincent Guittot
Cc: peterz, mgorman, valentin.schneider, parth, linux-mm, linux-kernel
Hello Vincent
On 16 Sep 2022 10:03:02 +0200 Vincent Guittot <vincent.guittot@linaro.org> wrote:
>
> @@ -4606,6 +4608,7 @@ check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
>
> se = __pick_first_entity(cfs_rq);
> delta = curr->vruntime - se->vruntime;
> + delta -= wakeup_latency_gran(curr, se);
>
> if (delta < 0)
> return;
What is derived from the latency nice you added is the runtime granulaity
which has a role in preempting the current task.
Given the same defination of latency nice as the nice, the runtime granularity
can be computed without introducing the latency nice.
Only for thoughts now.
Hillf
+++ b/kernel/sched/fair.c
@@ -4569,7 +4569,7 @@ dequeue_entity(struct cfs_rq *cfs_rq, st
static void
check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
{
- unsigned long ideal_runtime, delta_exec;
+ unsigned long ideal_runtime, delta_exec, granu;
struct sched_entity *se;
s64 delta;
@@ -4594,6 +4594,14 @@ check_preempt_tick(struct cfs_rq *cfs_rq
return;
se = __pick_first_entity(cfs_rq);
+
+ granu = sysctl_sched_min_granularity +
+ (ideal_runtime - sysctl_sched_min_granularity) *
+ (se->latency_nice + 20) / LATENCY_NICE_WIDTH;
+
+ if (delta_exec < granu)
+ return;
+
delta = curr->vruntime - se->vruntime;
if (delta < 0)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 5/8] sched/fair: Take into account latency priority at wakeup
2022-09-16 12:02 ` [PATCH v4 5/8] sched/fair: Take into account latency priority at wakeup Hillf Danton
@ 2022-09-16 13:36 ` Vincent Guittot
2022-09-17 22:58 ` Hillf Danton
0 siblings, 1 reply; 6+ messages in thread
From: Vincent Guittot @ 2022-09-16 13:36 UTC (permalink / raw)
To: Hillf Danton
Cc: peterz, mgorman, valentin.schneider, parth, linux-mm, linux-kernel
Hi Hillf,
On Fri, 16 Sept 2022 at 14:03, Hillf Danton <hdanton@sina.com> wrote:
>
> Hello Vincent
>
> On 16 Sep 2022 10:03:02 +0200 Vincent Guittot <vincent.guittot@linaro.org> wrote:
> >
> > @@ -4606,6 +4608,7 @@ check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
> >
> > se = __pick_first_entity(cfs_rq);
> > delta = curr->vruntime - se->vruntime;
> > + delta -= wakeup_latency_gran(curr, se);
> >
> > if (delta < 0)
> > return;
>
> What is derived from the latency nice you added is the runtime granulaity
> which has a role in preempting the current task.
>
> Given the same defination of latency nice as the nice, the runtime granularity
> can be computed without introducing the latency nice.
>
> Only for thoughts now.
>
> Hillf
>
> +++ b/kernel/sched/fair.c
> @@ -4569,7 +4569,7 @@ dequeue_entity(struct cfs_rq *cfs_rq, st
> static void
> check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
> {
> - unsigned long ideal_runtime, delta_exec;
> + unsigned long ideal_runtime, delta_exec, granu;
> struct sched_entity *se;
> s64 delta;
>
> @@ -4594,6 +4594,14 @@ check_preempt_tick(struct cfs_rq *cfs_rq
> return;
>
> se = __pick_first_entity(cfs_rq);
> +
> + granu = sysctl_sched_min_granularity +
> + (ideal_runtime - sysctl_sched_min_granularity) *
> + (se->latency_nice + 20) / LATENCY_NICE_WIDTH;
There is no latency_nice field in se but a latency_offset instead
Also at this step, we are sure that curr has run at least
sysctl_sched_min_granularity and we want now to compare curr vruntime
with first se's one. We take the latency offset into account to make
sure we will not preempt curr too early
> +
> + if (delta_exec < granu)
> + return;
> +
> delta = curr->vruntime - se->vruntime;
>
> if (delta < 0)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 5/8] sched/fair: Take into account latency priority at wakeup
2022-09-16 13:36 ` Vincent Guittot
@ 2022-09-17 22:58 ` Hillf Danton
2022-09-18 10:46 ` Vincent Guittot
0 siblings, 1 reply; 6+ messages in thread
From: Hillf Danton @ 2022-09-17 22:58 UTC (permalink / raw)
To: Vincent Guittot
Cc: peterz, mgorman, valentin.schneider, parth, linux-mm, linux-kernel
On 16 Sep 2022 15:36:53 +0200 Vincent Guittot <vincent.guittot@linaro.org> wrote:
>
> Hi Hillf,
>
> On Fri, 16 Sept 2022 at 14:03, Hillf Danton <hdanton@sina.com> wrote:
> >
> > Hello Vincent
> >
> > On 16 Sep 2022 10:03:02 +0200 Vincent Guittot <vincent.guittot@linaro.org> wrote:
> > >
> > > @@ -4606,6 +4608,7 @@ check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
> > >
> > > se = __pick_first_entity(cfs_rq);
> > > delta = curr->vruntime - se->vruntime;
> > > + delta -= wakeup_latency_gran(curr, se);
> > >
> > > if (delta < 0)
> > > return;
> >
> > What is derived from the latency nice you added is the runtime granulaity
> > which has a role in preempting the current task.
> >
> > Given the same defination of latency nice as the nice, the runtime granularity
> > can be computed without introducing the latency nice.
> >
> > Only for thoughts now.
> >
> > Hillf
> >
> > +++ b/kernel/sched/fair.c
> > @@ -4569,7 +4569,7 @@ dequeue_entity(struct cfs_rq *cfs_rq, st
> > static void
> > check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
> > {
> > - unsigned long ideal_runtime, delta_exec;
> > + unsigned long ideal_runtime, delta_exec, granu;
> > struct sched_entity *se;
> > s64 delta;
> >
> > @@ -4594,6 +4594,14 @@ check_preempt_tick(struct cfs_rq *cfs_rq
> > return;
> >
> > se = __pick_first_entity(cfs_rq);
> > +
> > + granu = sysctl_sched_min_granularity +
> > + (ideal_runtime - sysctl_sched_min_granularity) *
> > + (se->latency_nice + 20) / LATENCY_NICE_WIDTH;
>
> There is no latency_nice field in se but a latency_offset instead
>
> Also at this step, we are sure that curr has run at least
> sysctl_sched_min_granularity and we want now to compare curr vruntime
> with first se's one. We take the latency offset into account to make
> sure we will not preempt curr too early
>
> > +
> > + if (delta_exec < granu)
> > + return;
> > +
> > delta = curr->vruntime - se->vruntime;
> >
> > if (delta < 0)
return;
if (delta > ideal_runtime)
resched_curr(rq_of(cfs_rq));
After another look, curr is not preempted without the gap in vruntime
between curr and the first entity growing more than ideal runtime, while
with latency_offset, since the gap becomes larger, preempt happens later
than ideal runtime thoughts IMO.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 5/8] sched/fair: Take into account latency priority at wakeup
2022-09-17 22:58 ` Hillf Danton
@ 2022-09-18 10:46 ` Vincent Guittot
2022-09-20 11:32 ` Hillf Danton
0 siblings, 1 reply; 6+ messages in thread
From: Vincent Guittot @ 2022-09-18 10:46 UTC (permalink / raw)
To: Hillf Danton
Cc: peterz, mgorman, valentin.schneider, parth, linux-mm, linux-kernel
On Sun, 18 Sept 2022 at 00:58, Hillf Danton <hdanton@sina.com> wrote:
>
> On 16 Sep 2022 15:36:53 +0200 Vincent Guittot <vincent.guittot@linaro.org> wrote:
> >
> > Hi Hillf,
> >
> > On Fri, 16 Sept 2022 at 14:03, Hillf Danton <hdanton@sina.com> wrote:
> > >
> > > Hello Vincent
> > >
> > > On 16 Sep 2022 10:03:02 +0200 Vincent Guittot <vincent.guittot@linaro.org> wrote:
> > > >
> > > > @@ -4606,6 +4608,7 @@ check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
> > > >
> > > > se = __pick_first_entity(cfs_rq);
> > > > delta = curr->vruntime - se->vruntime;
> > > > + delta -= wakeup_latency_gran(curr, se);
> > > >
> > > > if (delta < 0)
> > > > return;
> > >
> > > What is derived from the latency nice you added is the runtime granulaity
> > > which has a role in preempting the current task.
> > >
> > > Given the same defination of latency nice as the nice, the runtime granularity
> > > can be computed without introducing the latency nice.
> > >
> > > Only for thoughts now.
> > >
> > > Hillf
> > >
> > > +++ b/kernel/sched/fair.c
> > > @@ -4569,7 +4569,7 @@ dequeue_entity(struct cfs_rq *cfs_rq, st
> > > static void
> > > check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
> > > {
> > > - unsigned long ideal_runtime, delta_exec;
> > > + unsigned long ideal_runtime, delta_exec, granu;
> > > struct sched_entity *se;
> > > s64 delta;
> > >
> > > @@ -4594,6 +4594,14 @@ check_preempt_tick(struct cfs_rq *cfs_rq
> > > return;
> > >
> > > se = __pick_first_entity(cfs_rq);
> > > +
> > > + granu = sysctl_sched_min_granularity +
> > > + (ideal_runtime - sysctl_sched_min_granularity) *
> > > + (se->latency_nice + 20) / LATENCY_NICE_WIDTH;
> >
> > There is no latency_nice field in se but a latency_offset instead
> >
> > Also at this step, we are sure that curr has run at least
> > sysctl_sched_min_granularity and we want now to compare curr vruntime
> > with first se's one. We take the latency offset into account to make
> > sure we will not preempt curr too early
> >
> > > +
> > > + if (delta_exec < granu)
> > > + return;
> > > +
> > > delta = curr->vruntime - se->vruntime;
> > >
> > > if (delta < 0)
> return;
>
> if (delta > ideal_runtime)
> resched_curr(rq_of(cfs_rq));
>
> After another look, curr is not preempted without the gap in vruntime
> between curr and the first entity growing more than ideal runtime, while
Curr can be preempted as it has run more than the ideal time (1st
test). This one is to make sure that the diff does not become too
large. Here we reuse the same comparison as wakeup to make sure that a
newly curr will get a chance to run its ideal time after having
preempted at wakeup the previous current
> with latency_offset, since the gap becomes larger, preempt happens later
> than ideal runtime thoughts IMO.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 5/8] sched/fair: Take into account latency priority at wakeup
2022-09-18 10:46 ` Vincent Guittot
@ 2022-09-20 11:32 ` Hillf Danton
2022-09-20 15:17 ` Vincent Guittot
0 siblings, 1 reply; 6+ messages in thread
From: Hillf Danton @ 2022-09-20 11:32 UTC (permalink / raw)
To: Vincent Guittot
Cc: peterz, mgorman, valentin.schneider, parth, linux-mm, linux-kernel
On 18 Sep 2022 12:46:00 +0200 Vincent Guittot <vincent.guittot@linaro.org> wrote:
> On Sun, 18 Sept 2022 at 00:58, Hillf Danton <hdanton@sina.com> wrote:
> >
> > On 16 Sep 2022 15:36:53 +0200 Vincent Guittot <vincent.guittot@linaro.org> wrote:
> > >
> > > Hi Hillf,
> > >
> > > On Fri, 16 Sept 2022 at 14:03, Hillf Danton <hdanton@sina.com> wrote:
> > > >
> > > > Hello Vincent
> > > >
> > > > On 16 Sep 2022 10:03:02 +0200 Vincent Guittot <vincent.guittot@linaro.org> wrote:
> > > > >
> > > > > @@ -4606,6 +4608,7 @@ check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
> > > > >
> > > > > se = __pick_first_entity(cfs_rq);
> > > > > delta = curr->vruntime - se->vruntime;
> > > > > + delta -= wakeup_latency_gran(curr, se);
> > > > >
> > > > > if (delta < 0)
> > > > > return;
> > > >
> > > > What is derived from the latency nice you added is the runtime granulaity
> > > > which has a role in preempting the current task.
> > > >
> > > > Given the same defination of latency nice as the nice, the runtime granularity
> > > > can be computed without introducing the latency nice.
> > > >
> > > > Only for thoughts now.
> > > >
> > > > Hillf
> > > >
> > > > +++ b/kernel/sched/fair.c
> > > > @@ -4569,7 +4569,7 @@ dequeue_entity(struct cfs_rq *cfs_rq, st
> > > > static void
> > > > check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
> > > > {
> > > > - unsigned long ideal_runtime, delta_exec;
> > > > + unsigned long ideal_runtime, delta_exec, granu;
> > > > struct sched_entity *se;
> > > > s64 delta;
> > > >
> > > > @@ -4594,6 +4594,14 @@ check_preempt_tick(struct cfs_rq *cfs_rq
> > > > return;
> > > >
> > > > se = __pick_first_entity(cfs_rq);
> > > > +
> > > > + granu = sysctl_sched_min_granularity +
> > > > + (ideal_runtime - sysctl_sched_min_granularity) *
> > > > + (se->latency_nice + 20) / LATENCY_NICE_WIDTH;
> > >
> > > There is no latency_nice field in se but a latency_offset instead
> > >
> > > Also at this step, we are sure that curr has run at least
> > > sysctl_sched_min_granularity and we want now to compare curr vruntime
> > > with first se's one. We take the latency offset into account to make
> > > sure we will not preempt curr too early
> > >
> > > > +
> > > > + if (delta_exec < granu)
> > > > + return;
> > > > +
> > > > delta = curr->vruntime - se->vruntime;
> > > >
> > > > if (delta < 0)
> > return;
> >
> > if (delta > ideal_runtime)
> > resched_curr(rq_of(cfs_rq));
> >
> > After another look, curr is not preempted without the gap in vruntime
> > between curr and the first entity growing more than ideal runtime, while
>
> Curr can be preempted as it has run more than the ideal time (1st
> test). This one is to make sure that the diff does not become too
> large. Here we reuse the same comparison as wakeup to make sure that a
> newly curr will get a chance to run its ideal time after having
> preempted at wakeup the previous current
IIUC it would take two checks to preempt correctly - diff in vruntime
is checked first to avoid preempting too early, then it is checked again
with latency_offset taken into account to avoid preempting too late.
+++ b/kernel/sched/fair.c
@@ -4571,7 +4571,7 @@ check_preempt_tick(struct cfs_rq *cfs_rq
{
unsigned long ideal_runtime, delta_exec;
struct sched_entity *se;
- s64 delta;
+ s64 delta, d2;
ideal_runtime = sched_slice(cfs_rq, curr);
delta_exec = curr->sum_exec_runtime - curr->prev_sum_exec_runtime;
@@ -4595,11 +4595,9 @@ check_preempt_tick(struct cfs_rq *cfs_rq
se = __pick_first_entity(cfs_rq);
delta = curr->vruntime - se->vruntime;
+ d2 = delta - wakeup_latency_gran(curr, se);
- if (delta < 0)
- return;
-
- if (delta > ideal_runtime)
+ if (delta > ideal_runtime || d2 > ideal_runtime)
resched_curr(rq_of(cfs_rq));
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 5/8] sched/fair: Take into account latency priority at wakeup
2022-09-20 11:32 ` Hillf Danton
@ 2022-09-20 15:17 ` Vincent Guittot
0 siblings, 0 replies; 6+ messages in thread
From: Vincent Guittot @ 2022-09-20 15:17 UTC (permalink / raw)
To: Hillf Danton
Cc: peterz, mgorman, valentin.schneider, parth, linux-mm, linux-kernel
On Tue, 20 Sept 2022 at 13:32, Hillf Danton <hdanton@sina.com> wrote:
>
> On 18 Sep 2022 12:46:00 +0200 Vincent Guittot <vincent.guittot@linaro.org> wrote:
> > On Sun, 18 Sept 2022 at 00:58, Hillf Danton <hdanton@sina.com> wrote:
> > >
> > > On 16 Sep 2022 15:36:53 +0200 Vincent Guittot <vincent.guittot@linaro.org> wrote:
> > > >
> > > > Hi Hillf,
> > > >
> > > > On Fri, 16 Sept 2022 at 14:03, Hillf Danton <hdanton@sina.com> wrote:
> > > > >
> > > > > Hello Vincent
> > > > >
> > > > > On 16 Sep 2022 10:03:02 +0200 Vincent Guittot <vincent.guittot@linaro.org> wrote:
> > > > > >
> > > > > > @@ -4606,6 +4608,7 @@ check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
> > > > > >
> > > > > > se = __pick_first_entity(cfs_rq);
> > > > > > delta = curr->vruntime - se->vruntime;
> > > > > > + delta -= wakeup_latency_gran(curr, se);
> > > > > >
> > > > > > if (delta < 0)
> > > > > > return;
> > > > >
> > > > > What is derived from the latency nice you added is the runtime granulaity
> > > > > which has a role in preempting the current task.
> > > > >
> > > > > Given the same defination of latency nice as the nice, the runtime granularity
> > > > > can be computed without introducing the latency nice.
> > > > >
> > > > > Only for thoughts now.
> > > > >
> > > > > Hillf
> > > > >
> > > > > +++ b/kernel/sched/fair.c
> > > > > @@ -4569,7 +4569,7 @@ dequeue_entity(struct cfs_rq *cfs_rq, st
> > > > > static void
> > > > > check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
> > > > > {
> > > > > - unsigned long ideal_runtime, delta_exec;
> > > > > + unsigned long ideal_runtime, delta_exec, granu;
> > > > > struct sched_entity *se;
> > > > > s64 delta;
> > > > >
> > > > > @@ -4594,6 +4594,14 @@ check_preempt_tick(struct cfs_rq *cfs_rq
> > > > > return;
> > > > >
> > > > > se = __pick_first_entity(cfs_rq);
> > > > > +
> > > > > + granu = sysctl_sched_min_granularity +
> > > > > + (ideal_runtime - sysctl_sched_min_granularity) *
> > > > > + (se->latency_nice + 20) / LATENCY_NICE_WIDTH;
> > > >
> > > > There is no latency_nice field in se but a latency_offset instead
> > > >
> > > > Also at this step, we are sure that curr has run at least
> > > > sysctl_sched_min_granularity and we want now to compare curr vruntime
> > > > with first se's one. We take the latency offset into account to make
> > > > sure we will not preempt curr too early
> > > >
> > > > > +
> > > > > + if (delta_exec < granu)
> > > > > + return;
> > > > > +
> > > > > delta = curr->vruntime - se->vruntime;
> > > > >
> > > > > if (delta < 0)
> > > return;
> > >
> > > if (delta > ideal_runtime)
> > > resched_curr(rq_of(cfs_rq));
> > >
> > > After another look, curr is not preempted without the gap in vruntime
> > > between curr and the first entity growing more than ideal runtime, while
> >
> > Curr can be preempted as it has run more than the ideal time (1st
> > test). This one is to make sure that the diff does not become too
> > large. Here we reuse the same comparison as wakeup to make sure that a
> > newly curr will get a chance to run its ideal time after having
> > preempted at wakeup the previous current
>
> IIUC it would take two checks to preempt correctly - diff in vruntime
> is checked first to avoid preempting too early, then it is checked again
> with latency_offset taken into account to avoid preempting too late.
The 1st test in check_preempt_tick() : if (delta_exec > ideal_runtime)
ensures that a resched happens after the current run is slice
The 2nd test : if (delta_exec < sysctl_sched_min_granularity)
ensures that current will run at least 3ms
The 3rd one : if (delta > ideal_runtime)
is there to make sure that there is not too much diff between the
vruntime. But we are comparing virtual runtime with execution time and
as Peter mentioned in another thread that's weird. [1] will fix it in
addition to ensure max runtime.
Then, current might have preempted first_entity few ms before thanks
to its latency_offset. If the tick happens quickly after the
preemption, delta might be above ideal_runtime whereas current has run
its ideal time yet
[1] https://lore.kernel.org/all/20220916131538.24706-1-vincent.guittot@linaro.org/
>
> +++ b/kernel/sched/fair.c
> @@ -4571,7 +4571,7 @@ check_preempt_tick(struct cfs_rq *cfs_rq
> {
> unsigned long ideal_runtime, delta_exec;
> struct sched_entity *se;
> - s64 delta;
> + s64 delta, d2;
>
> ideal_runtime = sched_slice(cfs_rq, curr);
> delta_exec = curr->sum_exec_runtime - curr->prev_sum_exec_runtime;
> @@ -4595,11 +4595,9 @@ check_preempt_tick(struct cfs_rq *cfs_rq
>
> se = __pick_first_entity(cfs_rq);
> delta = curr->vruntime - se->vruntime;
> + d2 = delta - wakeup_latency_gran(curr, se);
>
> - if (delta < 0)
> - return;
> -
> - if (delta > ideal_runtime)
> + if (delta > ideal_runtime || d2 > ideal_runtime)
> resched_curr(rq_of(cfs_rq));
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-20 15:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20220916080305.29574-6-vincent.guittot@linaro.org>
2022-09-16 12:02 ` [PATCH v4 5/8] sched/fair: Take into account latency priority at wakeup Hillf Danton
2022-09-16 13:36 ` Vincent Guittot
2022-09-17 22:58 ` Hillf Danton
2022-09-18 10:46 ` Vincent Guittot
2022-09-20 11:32 ` Hillf Danton
2022-09-20 15:17 ` Vincent Guittot
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).