Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1333203 > unrolled thread

[PATCH] sched/deadline: Always calculate end of period on sched_yield()

Back to article view | Back to linux.kernel

Contents

  [PATCH] sched/deadline: Always calculate end of period on  sched_yield() Steven Rostedt <rostedt@goodmis.org> - 2016-02-13 00:20 +0100
    Re: [PATCH] sched/deadline: Always calculate end of period on  sched_yield() Juri Lelli <juri.lelli@arm.com> - 2016-02-15 11:20 +0100
      Re: [PATCH] sched/deadline: Always calculate end of period on  sched_yield() Daniel Bristot de Oliveira <bristot@redhat.com> - 2016-02-15 13:40 +0100
      Re: [PATCH] sched/deadline: Always calculate end of period on  sched_yield() Steven Rostedt <rostedt@goodmis.org> - 2016-02-15 17:30 +0100

#1333203 — [PATCH] sched/deadline: Always calculate end of period on sched_yield()

I'm writing a test case for SCHED_DEADLINE, and notice a strange
anomaly. Every so often, a deadline is missed and when I looked into
it, it happened because the sched_yield() had no effect (it didn't end
the previous period and let the start of the next runtime happen on the
end of the old period).

deadline-2228    7...1   116.778420: sys_enter_sched_yield: 
deadline-2228    7d..3   116.778421: hrtimer_cancel:       hrtimer=0xffff88011ebd79a0
deadline-2228    7d..2   116.778422: rcu_utilization:      Start context switch
deadline-2228    7d..2   116.778423: rcu_utilization:      End context switch
deadline-2228    7d..4   116.778423: hrtimer_start:        hrtimer=0xffff88011ebd79a0 function=hrtick/0x0 expires=116124420428 softexpires=116124420428
deadline-2228    7...1   116.778425: sys_exit_sched_yield: 0x0


Schedule was never called. A added some trace_printks() and discovered
that this happens when sched_yield() is called right after a tick that
updates its current bandwidth.

When the schedule tick happens that updates the current bandwidth,
update_curr_dl() is called, where it updates curr->se.exec_start to
rq_clock_task(rq).

The rq_clock_task(rq) gets updated by update_rq_clock_task() that gets
update by various points in the scheduler.

Now, if the user task calls sched_yield() just after a bandwidth update
synced curr->se.exec_start to rq_clock_task(rq), when sched_yield()
calls into update_curr_dl() we have:

	delta_exec = rq_clock_task(rq) - curr->se.exec_start;
	if (unlikely((s64)delta_exec <= 0))
		return;

Coming in here from a sched_yield() will have delta_exec == 0 if the
sched_yield() was called after a DL tick and before another
update_rq_clock_task() is called.

This means that the task will not release its remaining runtime, and
the will start off in the current period when it expected to be in the
next period.

The fix that appears to work for me is to add a test in
update_curr_dl() to not exit if delta_exec is zero and
dl_se->dl_yielded is true.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index cd64c979d0e1..1dd180cda574 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -735,7 +735,7 @@ static void update_curr_dl(struct rq *rq)
 	 * approach need further study.
 	 */
 	delta_exec = rq_clock_task(rq) - curr->se.exec_start;
-	if (unlikely((s64)delta_exec <= 0))
+	if (unlikely((s64)delta_exec <= 0 && !dl_se->dl_yielded))
 		return;
 
 	schedstat_set(curr->se.statistics.exec_max,

[toc] | [next] | [standalone]

#1334343

Hi,

On 12/02/16 18:10, Steven Rostedt wrote:
> I'm writing a test case for SCHED_DEADLINE, and notice a strange
> anomaly. Every so often, a deadline is missed and when I looked into
> it, it happened because the sched_yield() had no effect (it didn't end
> the previous period and let the start of the next runtime happen on the
> end of the old period).
> 
> deadline-2228    7...1   116.778420: sys_enter_sched_yield: 
> deadline-2228    7d..3   116.778421: hrtimer_cancel:       hrtimer=0xffff88011ebd79a0
> deadline-2228    7d..2   116.778422: rcu_utilization:      Start context switch
> deadline-2228    7d..2   116.778423: rcu_utilization:      End context switch
> deadline-2228    7d..4   116.778423: hrtimer_start:        hrtimer=0xffff88011ebd79a0 function=hrtick/0x0 expires=116124420428 softexpires=116124420428
> deadline-2228    7...1   116.778425: sys_exit_sched_yield: 0x0
> 
> 
> Schedule was never called. A added some trace_printks() and discovered
> that this happens when sched_yield() is called right after a tick that
> updates its current bandwidth.
> 
> When the schedule tick happens that updates the current bandwidth,
> update_curr_dl() is called, where it updates curr->se.exec_start to
> rq_clock_task(rq).
> 
> The rq_clock_task(rq) gets updated by update_rq_clock_task() that gets
> update by various points in the scheduler.
> 
> Now, if the user task calls sched_yield() just after a bandwidth update
> synced curr->se.exec_start to rq_clock_task(rq), when sched_yield()
> calls into update_curr_dl() we have:
> 
> 	delta_exec = rq_clock_task(rq) - curr->se.exec_start;
> 	if (unlikely((s64)delta_exec <= 0))
> 		return;
> 
> Coming in here from a sched_yield() will have delta_exec == 0 if the
> sched_yield() was called after a DL tick and before another
> update_rq_clock_task() is called.
> 
> This means that the task will not release its remaining runtime, and
> the will start off in the current period when it expected to be in the
> next period.
> 
> The fix that appears to work for me is to add a test in
> update_curr_dl() to not exit if delta_exec is zero and
> dl_se->dl_yielded is true.
> 
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index cd64c979d0e1..1dd180cda574 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -735,7 +735,7 @@ static void update_curr_dl(struct rq *rq)
>  	 * approach need further study.
>  	 */
>  	delta_exec = rq_clock_task(rq) - curr->se.exec_start;
> -	if (unlikely((s64)delta_exec <= 0))
> +	if (unlikely((s64)delta_exec <= 0 && !dl_se->dl_yielded))
>  		return;
>

This looks good to me. Do you think we could also skip some of the
following updates/accounting in this case? Not sure we win anything by
doing that, though.

Thanks,

- Juri

[toc] | [prev] | [next] | [standalone]

#1334439

On 02/15/2016 08:18 AM, Juri Lelli wrote:
> Do you think we could also skip some of the
> following updates/accounting in this case? Not sure we win anything by
> doing that, though.

I reviewed rostedt's patch and the following updates/accounting
operations. I agree with rostedt's patch, and also agree that
if (delta_exec == 0) it is a good idea to skip some += 0 and
function calls of the next updates/accounting operations,
before the if (dl_runtime_exeeded...).

[toc] | [prev] | [next] | [standalone]

#1334576

On Mon, 15 Feb 2016 10:18:24 +0000
Juri Lelli <juri.lelli@arm.com> wrote:


> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > ---
> > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > index cd64c979d0e1..1dd180cda574 100644
> > --- a/kernel/sched/deadline.c
> > +++ b/kernel/sched/deadline.c
> > @@ -735,7 +735,7 @@ static void update_curr_dl(struct rq *rq)
> >  	 * approach need further study.
> >  	 */
> >  	delta_exec = rq_clock_task(rq) - curr->se.exec_start;
> > -	if (unlikely((s64)delta_exec <= 0))
> > +	if (unlikely((s64)delta_exec <= 0 && !dl_se->dl_yielded))
> >  		return;
> >  
> 
> This looks good to me. Do you think we could also skip some of the
> following updates/accounting in this case? Not sure we win anything by
> doing that, though.
>

Well, I would say we get this patch in first and think about other
updates second. This fixes one bug, might as well pull it in.

I'm now looking into a second bug. I'm getting:

 RT throttling activated

and

 DL replenish lagged to much

messages, back to back, when I'm only using 50% of the band width.
Looks to be a leak of how much is being used. The big issue here is
that these messages kill the test due to the latency caused to perform
the printk(). After the messages are splatted out (they only print once
per boot), the tests run fine again. IOW, there seems to be no real
issue of something doing too much bandwidth.

I get this with or without this current patch.

-- Steve

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web