* [PATCH] sched: remove resetting exec_start in put_prev_task_rt() @ 2011-06-01 14:03 Hillf Danton 2011-06-01 14:22 ` Steven Rostedt ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Hillf Danton @ 2011-06-01 14:03 UTC (permalink / raw) To: LKML Cc: Steven Rostedt, Mike Galbraith, Yong Zhang, Peter Zijlstra, Ingo Molnar 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] sched: remove resetting exec_start in put_prev_task_rt() 2011-06-01 14:03 [PATCH] sched: remove resetting exec_start in put_prev_task_rt() Hillf Danton @ 2011-06-01 14:22 ` Steven Rostedt 2011-06-02 8:04 ` Yong Zhang 2011-08-14 16:03 ` [tip:sched/core] sched: Remove " tip-bot for Hillf Danton 2 siblings, 0 replies; 12+ messages in thread From: Steven Rostedt @ 2011-06-01 14:22 UTC (permalink / raw) To: Hillf Danton Cc: LKML, Mike Galbraith, Yong Zhang, Peter Zijlstra, Ingo Molnar 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] sched: remove resetting exec_start in put_prev_task_rt() 2011-06-01 14:03 [PATCH] sched: remove resetting exec_start in put_prev_task_rt() Hillf Danton 2011-06-01 14:22 ` Steven Rostedt @ 2011-06-02 8:04 ` Yong Zhang 2011-06-04 4:26 ` Hillf Danton 2011-06-09 12:34 ` Steven Rostedt 2011-08-14 16:03 ` [tip:sched/core] sched: Remove " tip-bot for Hillf Danton 2 siblings, 2 replies; 12+ messages in thread From: Yong Zhang @ 2011-06-02 8:04 UTC (permalink / raw) To: Hillf Danton Cc: LKML, Steven Rostedt, Mike Galbraith, Peter Zijlstra, Ingo Molnar 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] sched: remove resetting exec_start in put_prev_task_rt() 2011-06-02 8:04 ` Yong Zhang @ 2011-06-04 4:26 ` Hillf Danton 2011-06-09 12:34 ` Steven Rostedt 1 sibling, 0 replies; 12+ messages in thread From: Hillf Danton @ 2011-06-04 4:26 UTC (permalink / raw) To: Yong Zhang Cc: LKML, Steven Rostedt, Mike Galbraith, Peter Zijlstra, Ingo Molnar 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] sched: remove resetting exec_start in put_prev_task_rt() 2011-06-02 8:04 ` Yong Zhang 2011-06-04 4:26 ` Hillf Danton @ 2011-06-09 12:34 ` Steven Rostedt 2011-06-10 2:38 ` Yong Zhang 2011-06-10 14:48 ` Hillf Danton 1 sibling, 2 replies; 12+ messages in thread From: Steven Rostedt @ 2011-06-09 12:34 UTC (permalink / raw) To: Yong Zhang Cc: Hillf Danton, LKML, Mike Galbraith, Peter Zijlstra, Ingo Molnar 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] sched: remove resetting exec_start in put_prev_task_rt() 2011-06-09 12:34 ` Steven Rostedt @ 2011-06-10 2:38 ` Yong Zhang 2011-06-10 2:41 ` Steven Rostedt 2011-06-10 14:48 ` Hillf Danton 1 sibling, 1 reply; 12+ messages in thread From: Yong Zhang @ 2011-06-10 2:38 UTC (permalink / raw) To: Steven Rostedt Cc: Hillf Danton, LKML, Mike Galbraith, Peter Zijlstra, Ingo Molnar 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] sched: remove resetting exec_start in put_prev_task_rt() 2011-06-10 2:38 ` Yong Zhang @ 2011-06-10 2:41 ` Steven Rostedt 2011-06-10 10:19 ` Peter Zijlstra 0 siblings, 1 reply; 12+ messages in thread From: Steven Rostedt @ 2011-06-10 2:41 UTC (permalink / raw) To: Yong Zhang Cc: Hillf Danton, LKML, Mike Galbraith, Peter Zijlstra, Ingo Molnar 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] sched: remove resetting exec_start in put_prev_task_rt() 2011-06-10 2:41 ` Steven Rostedt @ 2011-06-10 10:19 ` Peter Zijlstra 0 siblings, 0 replies; 12+ messages in thread From: Peter Zijlstra @ 2011-06-10 10:19 UTC (permalink / raw) To: Steven Rostedt Cc: Yong Zhang, Hillf Danton, LKML, Mike Galbraith, Ingo Molnar 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] sched: remove resetting exec_start in put_prev_task_rt() 2011-06-09 12:34 ` Steven Rostedt 2011-06-10 2:38 ` Yong Zhang @ 2011-06-10 14:48 ` Hillf Danton 2011-06-10 14:58 ` Steven Rostedt 1 sibling, 1 reply; 12+ messages in thread From: Hillf Danton @ 2011-06-10 14:48 UTC (permalink / raw) To: Steven Rostedt Cc: Yong Zhang, LKML, Mike Galbraith, Peter Zijlstra, Ingo Molnar 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; ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] sched: remove resetting exec_start in put_prev_task_rt() 2011-06-10 14:48 ` Hillf Danton @ 2011-06-10 14:58 ` Steven Rostedt 2011-06-10 15:12 ` Hillf Danton 0 siblings, 1 reply; 12+ messages in thread From: Steven Rostedt @ 2011-06-10 14:58 UTC (permalink / raw) To: Hillf Danton Cc: Yong Zhang, LKML, Mike Galbraith, Peter Zijlstra, Ingo Molnar 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] sched: remove resetting exec_start in put_prev_task_rt() 2011-06-10 14:58 ` Steven Rostedt @ 2011-06-10 15:12 ` Hillf Danton 0 siblings, 0 replies; 12+ messages in thread From: Hillf Danton @ 2011-06-10 15:12 UTC (permalink / raw) To: Steven Rostedt Cc: Yong Zhang, LKML, Mike Galbraith, Peter Zijlstra, Ingo Molnar 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [tip:sched/core] sched: Remove resetting exec_start in put_prev_task_rt() 2011-06-01 14:03 [PATCH] sched: remove resetting exec_start in put_prev_task_rt() Hillf Danton 2011-06-01 14:22 ` Steven Rostedt 2011-06-02 8:04 ` Yong Zhang @ 2011-08-14 16:03 ` tip-bot for Hillf Danton 2 siblings, 0 replies; 12+ messages in thread From: tip-bot for Hillf Danton @ 2011-08-14 16:03 UTC (permalink / raw) To: linux-tip-commits Cc: linux-kernel, hpa, mingo, a.p.zijlstra, efault, dhillf, rostedt, tglx, yong.zhang0, mingo 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-08-14 16:04 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-06-01 14:03 [PATCH] sched: remove resetting exec_start in put_prev_task_rt() Hillf Danton 2011-06-01 14:22 ` Steven Rostedt 2011-06-02 8:04 ` Yong Zhang 2011-06-04 4:26 ` Hillf Danton 2011-06-09 12:34 ` Steven Rostedt 2011-06-10 2:38 ` Yong Zhang 2011-06-10 2:41 ` Steven Rostedt 2011-06-10 10:19 ` Peter Zijlstra 2011-06-10 14:48 ` Hillf Danton 2011-06-10 14:58 ` Steven Rostedt 2011-06-10 15:12 ` Hillf Danton 2011-08-14 16:03 ` [tip:sched/core] sched: Remove " tip-bot for Hillf Danton
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).