2004-11-20 05:14:16

by maximilian attems

Subject: [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(-)

diff -puN kernel/sched.c~fix-test-kernel_sched kernel/sched.c
--- linux-2.6.10-rc2-bk4/kernel/sched.c~fix-test-kernel_sched 2004-11-20 03:04:46.000000000 +0100
+++ linux-2.6.10-rc2-bk4-max/kernel/sched.c 2004-11-20 03:04:46.000000000 +0100
@@ -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))))
_

2004-11-20 11:53:55

by Ingo Molnar

Subject: Re: [patch 3/8] kernel/sched.c: fix subtle TASK_RUNNING compare

* [email protected] <[email protected]> wrote:

> 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

2004-11-20 12:12:51

by Domen Puncer

Subject: Re: kernel/sched.c: fix subtle TASK_RUNNING compare

On 20/11/04 13:53 +0100, Ingo Molnar wrote:
>
> * [email protected] <[email protected]> wrote:
>
> > 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

2004-11-20 15:37:54

by Ingo Molnar

Subject: Re: kernel/sched.c: fix subtle TASK_RUNNING compare

* Domen Puncer <[email protected]> wrote:

> On 20/11/04 13:53 +0100, Ingo Molnar wrote:
> >
> > * [email protected] <[email protected]> wrote:
> >
> > > 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)

hm ... ok.

Ingo