kernel/sched.c: fix subtle TASK_RUNNING compare

PATCH In review [email protected] 3 participants 4 messages Nov 20, 2004

lore

Share & tools

lore

[email protected]

author

2004-11-20 05:14 UTC #0 lore raw

PATCH 3/8 kernel/sched.c: fix subtle TASK_RUNNING compare

Hi.

This test was depending on TASK_RUNNING being defined as 0.

Signed-off-by Domen Puncer <[email protected]>

Signed-off-by Maximilian Attems <[email protected]>

---

 linux-2.6.10-rc2-bk4-max/kernel/sched.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

kernel/sched.c renamed from kernel/sched.c~fix-test-kernel_sched +2 -1

diff -puN kernel/sched.c~fix-test-kernel_sched kernel/sched.c
@@ -2576,7 +2576,8 @@ need_resched_nonpreemptible:
 	 * to picking the next task.
 	 */
 	switch_count = &prev->nivcsw;
-	if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) {
+	if (prev->state != TASK_RUNNING &&
+			!(preempt_count() & PREEMPT_ACTIVE)) {
 		switch_count = &prev->nvcsw;
 		if (unlikely((prev->state & TASK_INTERRUPTIBLE) &&
 				unlikely(signal_pending(prev))))

Ingo Molnar

2004-11-20 11:53 UTC #1 lore raw

kernel/sched.c need_resched_nonpreemptible: · line 2581

Quoted 5 lines · * [email protected] <[email protected]> wrote:

kernel/sched.c renamed from kernel/sched.c~fix-test-kernel_sched +2 -1

 	switch_count = &prev->nivcsw;
-	if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) {
+	if (prev->state != TASK_RUNNING &&
+			!(preempt_count() & PREEMPT_ACTIVE)) {
 		switch_count = &prev->nvcsw;

nack. We inherently rely on the process state mask being a bitmask and TASK_RUNNING thus being zero.

Ingo

Domen Puncer

2004-11-20 12:12 UTC #2 lore raw

Quoted 11 lines · Ingo Molnar

kernel/sched.c need_resched_nonpreemptible: · line 2581
Quoted 5 lines · * [email protected] <[email protected]> wrote:
kernel/sched.c renamed from kernel/sched.c~fix-test-kernel_sched +2 -1
 	switch_count = &prev->nivcsw;
-	if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) {
+	if (prev->state != TASK_RUNNING &&
+			!(preempt_count() & PREEMPT_ACTIVE)) {
 		switch_count = &prev->nvcsw;
nack. We inherently rely on the process state mask being a bitmask and TASK_RUNNING thus being zero.

Hmm... but other compares in sched.c are ok? ;-)

1211:   BUG_ON(p->state != TASK_RUNNING);
2550:   if (unlikely(current == rq->idle) && current->state != TASK_RUNNING) {
3609:   if (state == TASK_RUNNING)
3640:   if (state != TASK_RUNNING)

Well, it just looks more readable to me. But i don't have too strong feelings about this. :-)

Domen

Ingo Molnar

2004-11-20 15:37 UTC #3 lore raw

Quoted 13 lines · Domen Puncer

Quoted 11 lines · Ingo Molnar
kernel/sched.c need_resched_nonpreemptible: · line 2581
Quoted 5 lines · * [email protected] <[email protected]> wrote:
kernel/sched.c renamed from kernel/sched.c~fix-test-kernel_sched +2 -1
 	switch_count = &prev->nivcsw;
-	if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) {
+	if (prev->state != TASK_RUNNING &&
+			!(preempt_count() & PREEMPT_ACTIVE)) {
 		switch_count = &prev->nvcsw;
nack. We inherently rely on the process state mask being a bitmask and TASK_RUNNING thus being zero.

Quoted 5 lines

Hmm... but other compares in sched.c are ok? ;-)

1211:   BUG_ON(p->state != TASK_RUNNING);
2550:   if (unlikely(current == rq->idle) && current->state != TASK_RUNNING) {
3609:   if (state == TASK_RUNNING)
3640:   if (state != TASK_RUNNING)

hm ... ok.

Ingo