[PATCH] sched: remove resetting exec_start in put_prev_task_rt()

[PATCH] sched: remove resetting exec_start in put_prev_task_rt()

[Hillf Danton]

Resetting exec_start, after updated in update_curr_rt(), could open window for
messing up the subsequent computations of delta_exec of the given task.

Signed-off-by: Hillf Danton <dhillf@gmail.com>
---
 kernel/sched_rt.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
index 88725c9..0f0cfce 100644
--- a/kernel/sched_rt.c
+++ b/kernel/sched_rt.c
@@ -1166,7 +1166,6 @@ static struct task_struct
*pick_next_task_rt(struct rq *rq)
 static void put_prev_task_rt(struct rq *rq, struct task_struct *p)
 {
 	update_curr_rt(rq);
-	p->se.exec_start = 0;

 	/*
 	 * The previous task needs to be made eligible for pushing

Re: [PATCH] sched: remove resetting exec_start in put_prev_task_rt()

[Steven Rostedt]

On Wed, 2011-06-01 at 22:03 +0800, Hillf Danton wrote:
> Resetting exec_start, after updated in update_curr_rt(), could open window for
> messing up the subsequent computations of delta_exec of the given task.

The domain scheduling was mostly done by Peter, but when you make a
change like this, you need to explain it better than this. I really have
no idea what you mean in this change log.

Please give an example of how this "open window" can mess up the
subsequent computations of delta_exec. That is, have write a theoretical
timeline of events that shows in detail how this can mess things up.

This makes understanding your changes much easier, otherwise you will
have to wait till we have the time to look and figure things out
ourselves. As we are currently working on other areas of the kernel so
you will have to wait much longer than if you showed us exactly what
happened.

Thanks!

-- Steve

> 
> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> ---
>  kernel/sched_rt.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
> index 88725c9..0f0cfce 100644
> --- a/kernel/sched_rt.c
> +++ b/kernel/sched_rt.c
> @@ -1166,7 +1166,6 @@ static struct task_struct
> *pick_next_task_rt(struct rq *rq)
>  static void put_prev_task_rt(struct rq *rq, struct task_struct *p)
>  {
>  	update_curr_rt(rq);
> -	p->se.exec_start = 0;
> 
>  	/*
>  	 * The previous task needs to be made eligible for pushing

Re: [PATCH] sched: remove resetting exec_start in put_prev_task_rt()

[Yong Zhang]

On Wed, Jun 1, 2011 at 10:03 PM, Hillf Danton <dhillf@gmail.com> wrote:
> Resetting exec_start, after updated in update_curr_rt(), could open window for
> messing up the subsequent computations of delta_exec of the given task.

I can't see how could this happen. what kind of 'subsequent computations'
do you mean?

But because exec_start will be reset by _pick_next_task_rt()/set_curr_task_rt(),
you patch is ok. IMHO it is not critical, it's just cleanup instead.

Thanks,
Yong

>
> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> ---
>  kernel/sched_rt.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
> index 88725c9..0f0cfce 100644
> --- a/kernel/sched_rt.c
> +++ b/kernel/sched_rt.c
> @@ -1166,7 +1166,6 @@ static struct task_struct
> *pick_next_task_rt(struct rq *rq)
>  static void put_prev_task_rt(struct rq *rq, struct task_struct *p)
>  {
>        update_curr_rt(rq);
> -       p->se.exec_start = 0;
>
>        /*
>         * The previous task needs to be made eligible for pushing
>



-- 
Only stand for myself

Re: [PATCH] sched: remove resetting exec_start in put_prev_task_rt()

[Hillf Danton]

On Thu, Jun 2, 2011 at 4:04 PM, Yong Zhang <yong.zhang0@gmail.com> wrote:
> On Wed, Jun 1, 2011 at 10:03 PM, Hillf Danton <dhillf@gmail.com> wrote:
>> Resetting exec_start, after updated in update_curr_rt(), could open window for
>> messing up the subsequent computations of delta_exec of the given task.
>
> I can't see how could this happen. what kind of 'subsequent computations'
> do you mean?
>
> But because exec_start will be reset by _pick_next_task_rt()/set_curr_task_rt(),
> you patch is ok. IMHO it is not critical, it's just cleanup instead.
>

H i Yong

The window is closed in the two cases, as you tip fingered, would you please
say a few more words about the window and tick timer?

thanks
           Hillf

Re: [PATCH] sched: remove resetting exec_start in put_prev_task_rt()

[Steven Rostedt]

On Thu, 2011-06-02 at 16:04 +0800, Yong Zhang wrote:
> On Wed, Jun 1, 2011 at 10:03 PM, Hillf Danton <dhillf@gmail.com> wrote:
> > Resetting exec_start, after updated in update_curr_rt(), could open window for
> > messing up the subsequent computations of delta_exec of the given task.
> 
> I can't see how could this happen. what kind of 'subsequent computations'
> do you mean?

I still don't see a race.

Hilf, if you still believe there's a race here, can you explain it in
detail. Do something like:

	CPU0			CPU1
	----			----
   do_something;
				does_something_not_expected;
   continue_something;

Obviously changing what those "somethings" are. That way we can visually
see what you are trying to say.

> 
> But because exec_start will be reset by _pick_next_task_rt()/set_curr_task_rt(),
> you patch is ok. IMHO it is not critical, it's just cleanup instead.

I disagree. Yes the exec_start is reset there, but I like the fact that
it's 0 when not running.

-- Steve

Re: [PATCH] sched: remove resetting exec_start in put_prev_task_rt()

[Yong Zhang]

On Thu, Jun 9, 2011 at 8:34 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> I disagree. Yes the exec_start is reset there, but I like the fact that
> it's 0 when not running.

Actually this depends on how we look at the code:
if we set exec_start to 0 explicitly, as you said the code is more direct and
readable.
if we don't set exec_start to 0, we can save one instruction though
it's minor.

I have no strong opinion on either of them :)

BTW, put_prev_task_fair() doesn't set exec_start to 0.

Thanks,
Yong


-- 
Only stand for myself

Re: [PATCH] sched: remove resetting exec_start in put_prev_task_rt()

[Steven Rostedt]

On Fri, 2011-06-10 at 10:38 +0800, Yong Zhang wrote:
> On Thu, Jun 9, 2011 at 8:34 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > I disagree. Yes the exec_start is reset there, but I like the fact that
> > it's 0 when not running.
> 
> Actually this depends on how we look at the code:
> if we set exec_start to 0 explicitly, as you said the code is more direct and
> readable.
> if we don't set exec_start to 0, we can save one instruction though
> it's minor.
> 
> I have no strong opinion on either of them :)

I don't have any real strong opinion on this either, so I'll just let
Peter decide :)

-- Steve

> 
> BTW, put_prev_task_fair() doesn't set exec_start to 0.

Re: [PATCH] sched: remove resetting exec_start in put_prev_task_rt()

[Peter Zijlstra]

On Thu, 2011-06-09 at 22:41 -0400, Steven Rostedt wrote:
> On Fri, 2011-06-10 at 10:38 +0800, Yong Zhang wrote:
> > On Thu, Jun 9, 2011 at 8:34 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > > I disagree. Yes the exec_start is reset there, but I like the fact that
> > > it's 0 when not running.
> > 
> > Actually this depends on how we look at the code:
> > if we set exec_start to 0 explicitly, as you said the code is more direct and
> > readable.
> > if we don't set exec_start to 0, we can save one instruction though
> > it's minor.
> > 
> > I have no strong opinion on either of them :)
> 
> I don't have any real strong opinion on this either, so I'll just let
> Peter decide :)

Yay! So IFF its correct (I didn't check) then sure it saves a whole
store :-), I don't think sched_fair clears exec_start on de-schedule
either.

Re: [PATCH] sched: remove resetting exec_start in put_prev_task_rt()

[Hillf Danton]

On Thu, Jun 9, 2011 at 8:34 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 2011-06-02 at 16:04 +0800, Yong Zhang wrote:
>> On Wed, Jun 1, 2011 at 10:03 PM, Hillf Danton <dhillf@gmail.com> wrote:
>> > Resetting exec_start, after updated in update_curr_rt(), could open window for
>> > messing up the subsequent computations of delta_exec of the given task.
>>
>> I can't see how could this happen. what kind of 'subsequent computations'
>> do you mean?
>
> I still don't see a race.
>
> Hilf, if you still believe there's a race here, can you explain it in
> detail. Do something like:
>
>        CPU0                    CPU1
>        ----                    ----
>   do_something;
>                                does_something_not_expected;
>   continue_something;
>
> Obviously changing what those "somethings" are. That way we can visually
> see what you are trying to say.
>
>>
>> But because exec_start will be reset by _pick_next_task_rt()/set_curr_task_rt(),
>> you patch is ok. IMHO it is not critical, it's just cleanup instead.
>
> I disagree. Yes the exec_start is reset there, but I like the fact that
> it's 0 when not running.
>

Hi Steve

Thank you a lot, and Peter as well, for your lessons on mutex_spin_on_owner:)

Resetting exec_start to zero has no negative functionality in RT scheduling,
as shown by Yong.

After put_prev_task() is called in schedule(),

	put_prev_task(rq, prev);
	next = pick_next_task(rq);
	clear_tsk_need_resched(prev);

next is picked. Lets assume that next is not prev, and prev is still on RQ,
then prev's sched_class is changed to CFS while it is waiting on RQ.
After sched_class switch, prev is under CFS charge, and the exec_start field
could be taken into other games.

In task_hot(), called when migrating task, zeroed exec_start is trapped as
the following.

btw, I could not locate where proc_sched_show_task() is called.

Again thanks all a lot, /Hillf

---
 kernel/sched.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index fd18f39..195bd4a 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -2184,6 +2184,7 @@ task_hot(struct task_struct *p, u64 now, struct
sched_domain *sd)
 	if (sysctl_sched_migration_cost == 0)
 		return 0;

+	BUG_ON(p->se.exec_start == 0);
 	delta = now - p->se.exec_start;

 	return delta < (s64)sysctl_sched_migration_cost;

Re: [PATCH] sched: remove resetting exec_start in put_prev_task_rt()

[Steven Rostedt]

On Fri, 2011-06-10 at 22:48 +0800, Hillf Danton wrote:
> On Thu, Jun 9, 2011 at 8:34 PM, Steven Rostedt <rostedt@goodmis.org> wrote:

> Resetting exec_start to zero has no negative functionality in RT scheduling,
> as shown by Yong.
> 
> After put_prev_task() is called in schedule(),
> 
> 	put_prev_task(rq, prev);
> 	next = pick_next_task(rq);
> 	clear_tsk_need_resched(prev);
> 
> next is picked. Lets assume that next is not prev, and prev is still on RQ,
> then prev's sched_class is changed to CFS while it is waiting on RQ.
> After sched_class switch, prev is under CFS charge, and the exec_start field
> could be taken into other games.
> 
> In task_hot(), called when migrating task, zeroed exec_start is trapped as
> the following.
> 

How could any of that happen? This is all done under the rq->lock.
prev's sched class can not change at this time. Everything you stated is
protected by the rq->lock. I don't see any race conditions here.

-- Steve

Re: [PATCH] sched: remove resetting exec_start in put_prev_task_rt()

[Hillf Danton]

On Fri, Jun 10, 2011 at 10:58 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 2011-06-10 at 22:48 +0800, Hillf Danton wrote:
>> On Thu, Jun 9, 2011 at 8:34 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
>> Resetting exec_start to zero has no negative functionality in RT scheduling,
>> as shown by Yong.
>>
>> After put_prev_task() is called in schedule(),
>>
>>       put_prev_task(rq, prev);
>>       next = pick_next_task(rq);
>>       clear_tsk_need_resched(prev);
>>
>> next is picked. Lets assume that next is not prev, and prev is still on RQ,
>> then prev's sched_class is changed to CFS while it is waiting on RQ.
>> After sched_class switch, prev is under CFS charge, and the exec_start field
>> could be taken into other games.
>>
>> In task_hot(), called when migrating task, zeroed exec_start is trapped as
>> the following.
>>
>
> How could any of that happen? This is all done under the rq->lock.
> prev's sched class can not change at this time. Everything you stated is
> protected by the rq->lock. I don't see any race conditions here.
>

Hi Steve

Yeah you are right, the snippet in schedule() is under RQ lock, but next could
be a RT task, and if it is FIFO and willing to hog CPU ie. 10 minutes, then
prev has to wait on its RQ. While waiting, however, its sched_class could be
changed at anytime. Here I show it is tiny possible that zeroed exec_start
could rush out of our control.

thanks
           Hillf

[tip:sched/core] sched: Remove resetting exec_start in put_prev_task_rt()

[tip-bot for Hillf Danton]

Commit-ID:  1812a643ccbfeb61a00a7f0d7219606e63d8815b
Gitweb:     http://git.kernel.org/tip/1812a643ccbfeb61a00a7f0d7219606e63d8815b
Author:     Hillf Danton <dhillf@gmail.com>
AuthorDate: Thu, 16 Jun 2011 21:55:21 -0400
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sun, 14 Aug 2011 12:00:50 +0200

sched: Remove resetting exec_start in put_prev_task_rt()

There's no reason to clean the exec_start in put_prev_task_rt() as it is reset
when the task gets back to the run queue. This saves us doing a store() in the
fast path.

Signed-off-by: Hillf Danton <dhillf@gmail.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Yong Zhang <yong.zhang0@gmail.com>
Link: http://lkml.kernel.org/r/BANLkTimqWD=q6YnSDi-v9y=LMWecgEzEWg@mail.gmail.com
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/sched_rt.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
index 8e18945..70107a3 100644
--- a/kernel/sched_rt.c
+++ b/kernel/sched_rt.c
@@ -1178,7 +1178,6 @@ static struct task_struct *pick_next_task_rt(struct rq *rq)
 static void put_prev_task_rt(struct rq *rq, struct task_struct *p)
 {
 	update_curr_rt(rq);
-	p->se.exec_start = 0;
 
 	/*
 	 * The previous task needs to be made eligible for pushing