Skip to contentSkip to conversation

sched: Update task->on_rq when tasks are moving between runqueues

PATCH In review Olav Haugan 3 participants 5 messages Oct 24, 2015 – Nov 2, 2015

Subsystems (2)
Arm Qualcomm mailing list Scheduler
lore
Share & tools
lore

Olav Haugan

author
#0 lore raw

PATCH 1/1 sched: Update task->on_rq when tasks are moving between runqueues

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 <[email protected]>
---
 kernel/sched/core.c     | 2 ++
 kernel/sched/deadline.c | 4 ++++
 kernel/sched/rt.c       | 4 ++++
 3 files changed, 10 insertions(+)
kernel/sched/core.c +2 -0
index 10a8faa..5c7b614 100644
@@ -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 {
kernel/sched/deadline.c +4 -0
index fc8f010..21297d7 100644
@@ -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;
kernel/sched/rt.c +4 -0
index d2ea593..0735040 100644
@@ -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
Signature · 1 line
2.6.1
maintainer
#1 lore raw
Quoted 8 lines · Olav Haugan

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..

author
#2 lore raw
Quoted 11 lines · Peter Zijlstra
Quoted 8 lines · Olav Haugan

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,

Signature · 4 lines
.Olav

The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
maintainer
#3 lore raw
Quoted 13 lines · Olav Haugan
Quoted 11 lines · Peter Zijlstra
Quoted 8 lines · Olav Haugan

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..

Quoted 7 lines

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.

#4 lore raw
Quoted 22 lines · Peter Zijlstra
Quoted 13 lines · Olav Haugan
Quoted 11 lines · Peter Zijlstra
Quoted 8 lines · Olav Haugan

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..

Quoted 7 lines

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.

Quoted 3 lines

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.`

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/