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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
On Wed, Jun 1, 2011 at 10:03 PM, Hillf Danton <[email protected]> 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 <[email protected]>
> ---
> 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
On Thu, Jun 2, 2011 at 4:04 PM, Yong Zhang <[email protected]> wrote:
> On Wed, Jun 1, 2011 at 10:03 PM, Hillf Danton <[email protected]> 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
On Thu, 2011-06-02 at 16:04 +0800, Yong Zhang wrote:
> On Wed, Jun 1, 2011 at 10:03 PM, Hillf Danton <[email protected]> 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
On Thu, Jun 9, 2011 at 8:34 PM, Steven Rostedt <[email protected]> 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
On Fri, 2011-06-10 at 10:38 +0800, Yong Zhang wrote:
> On Thu, Jun 9, 2011 at 8:34 PM, Steven Rostedt <[email protected]> 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.
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 <[email protected]> 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.
On Thu, Jun 9, 2011 at 8:34 PM, Steven Rostedt <[email protected]> wrote:
> On Thu, 2011-06-02 at 16:04 +0800, Yong Zhang wrote:
>> On Wed, Jun 1, 2011 at 10:03 PM, Hillf Danton <[email protected]> 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;
On Fri, 2011-06-10 at 22:48 +0800, Hillf Danton wrote:
> On Thu, Jun 9, 2011 at 8:34 PM, Steven Rostedt <[email protected]> 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
On Fri, Jun 10, 2011 at 10:58 PM, Steven Rostedt <[email protected]> wrote:
> On Fri, 2011-06-10 at 22:48 +0800, Hillf Danton wrote:
>> On Thu, Jun 9, 2011 at 8:34 PM, Steven Rostedt <[email protected]> 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