| Message ID | 1445709662-17232-1-git-send-email-ohaugan@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On Sat, Oct 24, 2015 at 11:01:02AM -0700, Olav Haugan wrote: > Task->on_rq has three states: > 0 - Task is not on runqueue (rq) > 1 (TASK_ON_RQ_QUEUED) - Task is on rq > 2 (TASK_ON_RQ_MIGRATING) - Task is on rq but in the process of being > migrated to another rq > > When a task is moving between rqs task->on_rq state should be > TASK_ON_RQ_MIGRATING Only when not holding both rq locks.. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15-10-25 11:09:24, Peter Zijlstra wrote: > On Sat, Oct 24, 2015 at 11:01:02AM -0700, Olav Haugan wrote: > > Task->on_rq has three states: > > 0 - Task is not on runqueue (rq) > > 1 (TASK_ON_RQ_QUEUED) - Task is on rq > > 2 (TASK_ON_RQ_MIGRATING) - Task is on rq but in the process of being > > migrated to another rq > > > > When a task is moving between rqs task->on_rq state should be > > TASK_ON_RQ_MIGRATING > > Only when not holding both rq locks.. IMHO I think we should keep the state of p->on_rq updated with the correct state all the time unless I am incorrect in what p->on_rq represent. The task is moving between rq's and is on the rq so the state should be TASK_ON_RQ_MIGRATING right? I do realize that the code is currently not broken. However, in the future someone might come along and change set_task_cpu() and the code change might rely on an accurate p->on_rq value. It would be good design to keep this value correct. Thanks,
On Wed, Oct 28, 2015 at 05:57:10PM -0700, Olav Haugan wrote: > On 15-10-25 11:09:24, Peter Zijlstra wrote: > > On Sat, Oct 24, 2015 at 11:01:02AM -0700, Olav Haugan wrote: > > > Task->on_rq has three states: > > > 0 - Task is not on runqueue (rq) > > > 1 (TASK_ON_RQ_QUEUED) - Task is on rq > > > 2 (TASK_ON_RQ_MIGRATING) - Task is on rq but in the process of being > > > migrated to another rq > > > > > > When a task is moving between rqs task->on_rq state should be > > > TASK_ON_RQ_MIGRATING > > > > Only when not holding both rq locks.. > > IMHO I think we should keep the state of p->on_rq updated with the correct state > all the time unless I am incorrect in what p->on_rq represent. The task > is moving between rq's and is on the rq so the state should be > TASK_ON_RQ_MIGRATING right? I do realize that the code is currently not > broken. However, in the future someone might come along and change > set_task_cpu() and the code change might rely on an accurate p->on_rq value. It > would be good design to keep this value correct. At the same time; we should also provide lean and fast code. Is it better to add assertions about required state than to add superfluous code for just in case scenarios. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 28, 2015 at 6:58 PM, Peter Zijlstra <peterz@infradead.org> wrote: > On Wed, Oct 28, 2015 at 05:57:10PM -0700, Olav Haugan wrote: >> On 15-10-25 11:09:24, Peter Zijlstra wrote: >> > On Sat, Oct 24, 2015 at 11:01:02AM -0700, Olav Haugan wrote: >> > > Task->on_rq has three states: >> > > 0 - Task is not on runqueue (rq) >> > > 1 (TASK_ON_RQ_QUEUED) - Task is on rq >> > > 2 (TASK_ON_RQ_MIGRATING) - Task is on rq but in the process of being >> > > migrated to another rq >> > > >> > > When a task is moving between rqs task->on_rq state should be >> > > TASK_ON_RQ_MIGRATING >> > >> > Only when not holding both rq locks.. >> >> IMHO I think we should keep the state of p->on_rq updated with the correct state >> all the time unless I am incorrect in what p->on_rq represent. The task >> is moving between rq's and is on the rq so the state should be >> TASK_ON_RQ_MIGRATING right? I do realize that the code is currently not >> broken. However, in the future someone might come along and change >> set_task_cpu() and the code change might rely on an accurate p->on_rq value. It >> would be good design to keep this value correct. > > At the same time; we should also provide lean and fast code. Is it > better to add assertions about required state than to add superfluous > code for just in case scenarios. The state is only worth publishing if it's exceptional. I think Peter's new documentation helps to make this more clear. The intent of this change may be better captured by pointing out in a comment somewhere that detach_task() is *also* updating the task_cpu pointer which then lets us lean on holding that lock to make the state non-interesting.` > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 10a8faa..5c7b614 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -1309,7 +1309,9 @@ static void __migrate_swap_task(struct task_struct *p, int cpu) dst_rq = cpu_rq(cpu); deactivate_task(src_rq, p, 0); + p->on_rq = TASK_ON_RQ_MIGRATING; set_task_cpu(p, cpu); + p->on_rq = TASK_ON_RQ_QUEUED; activate_task(dst_rq, p, 0); check_preempt_curr(dst_rq, p, 0); } else { diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c index fc8f010..21297d7 100644 --- a/kernel/sched/deadline.c +++ b/kernel/sched/deadline.c @@ -1547,7 +1547,9 @@ retry: } deactivate_task(rq, next_task, 0); + next_task->on_rq = TASK_ON_RQ_MIGRATING; set_task_cpu(next_task, later_rq->cpu); + next_task->on_rq = TASK_ON_RQ_QUEUED; activate_task(later_rq, next_task, 0); ret = 1; @@ -1635,7 +1637,9 @@ static void pull_dl_task(struct rq *this_rq) resched = true; deactivate_task(src_rq, p, 0); + p->on_rq = TASK_ON_RQ_MIGRATING; set_task_cpu(p, this_cpu); + p->on_rq = TASK_ON_RQ_QUEUED; activate_task(this_rq, p, 0); dmin = p->dl.deadline; diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c index d2ea593..0735040 100644 --- a/kernel/sched/rt.c +++ b/kernel/sched/rt.c @@ -1790,7 +1790,9 @@ retry: } deactivate_task(rq, next_task, 0); + next_task->on_rq = TASK_ON_RQ_MIGRATING; set_task_cpu(next_task, lowest_rq->cpu); + next_task->on_rq = TASK_ON_RQ_QUEUED; activate_task(lowest_rq, next_task, 0); ret = 1; @@ -2044,7 +2046,9 @@ static void pull_rt_task(struct rq *this_rq) resched = true; deactivate_task(src_rq, p, 0); + p->on_rq = TASK_ON_RQ_MIGRATING; set_task_cpu(p, this_cpu); + p->on_rq = TASK_ON_RQ_QUEUED; activate_task(this_rq, p, 0); /* * We continue with the search, just in
Task->on_rq has three states: 0 - Task is not on runqueue (rq) 1 (TASK_ON_RQ_QUEUED) - Task is on rq 2 (TASK_ON_RQ_MIGRATING) - Task is on rq but in the process of being migrated to another rq When a task is moving between rqs task->on_rq state should be TASK_ON_RQ_MIGRATING Signed-off-by: Olav Haugan <ohaugan@codeaurora.org> --- kernel/sched/core.c | 2 ++ kernel/sched/deadline.c | 4 ++++ kernel/sched/rt.c | 4 ++++ 3 files changed, 10 insertions(+)