2.5 TASK_INTERRUPTIBLE preemption race

Hi Andrew, Robert, Alan, Everyone,
The following 2.5 code fragment seems unsafe in a preempt
environment. A preemption could occur between the time when TASK_UNINTERRUPTIBLE is set and the `spin_lock_irqsave'. This would cause the task to switch out and never come back, as it would have been switched away while in TASK_UNINTERRUPTIBLE without yet having put itself onto the semaphore wait queue, where it could be found later by a wakeup service.

    void __down(struct semaphore * sem)
    {
	struct task_struct *tsk = current;
	DECLARE_WAITQUEUE(wait, tsk);
	unsigned long flags;

	tsk->state = TASK_UNINTERRUPTIBLE;
	spin_lock_irqsave(&sem->wait.lock, flags);
	add_wait_queue_exclusive_locked(&sem->wait, &wait);
	....
    }

Is this analysis correct? If it is, perhaps there is an alternative to fixing these cases individually: make the TASK_INTERRUPTIBLE/ TASK_UNINTERRUPTIBLE states block preemption. In which case the 'set_current_state(TASK_RUNNING)' macro would need to include the same preemption check as 'preemption_enable'.

I suspect there is already some mechanism in place to prevent this problem, as I have never seen this lockup happen on any of my 2.4-preempt systems.

Joe

PS: here is an example where the preemption race appears harmless. If a preemption happens between the 'set_current_state' and 'schedule', it only causes the 'schedule' to NOP: the preemption, on return, would have changed the state from TASK_UNINTERRUPTIBLE back to TASK_RUNNING.

void __wait_on_inode(struct inode *inode)

{
	DECLARE_WAITQUEUE(wait, current);
	wait_queue_head_t *wq = i_waitq_head(inode);

	add_wait_queue(wq, &wait);
repeat:
	set_current_state(TASK_UNINTERRUPTIBLE);
	if (inode->i_state & I_LOCK) {
		schedule();
		goto repeat;
	}
	remove_wait_queue(wq, &wait);
	__set_current_state(TASK_RUNNING);
}

PPS: the above may need a 'mb()' between the 'add_wait_queue' and 'set_current_state' regardless.

Thankfully you are wrong or we would have some serious problems :)

See kernel/sched.c :: preempt_schedule() where we set p->preempt_count to PREEMPT_ACTIVE.

Then see kernel/sched.c :: schedule() where we short-circuit the remove-from-runqueue code if PREEMPT_ACTIVE is set.

Thus, it is safe to preempt regardless of the task's state. It will eventually reschedule.

Robert Love

I see. It is because the 'goto pick_next_task' skips the 'deactivate_task' call. Therefore the previous task remains on the run queue in spite of its TASK_UNINTERRUPTIBLE state. Clever!

Joe

Yep :)

We actually did away with it for a bit in mid 2.5... it turned out to not be worth it. Its a little odd to use the flag like that, but its quite simply and the results are good.

Means we can safely schedule anytime, no matter the state. There are tons of other places where we would not want to sleep if it were not for PREEMPT_ACTIVE.

Robert Love