Re: [patch] Real-Time Preemption, deactivate() scheduling issue

[Eugeny S. Mints]

Esben Nielsen wrote:
As I read the code the driver task (A) should _not_ be removed from the
runqueue. It has to be waken up to call schedule_timeout() such it gets
back on the runqueue after 10 ms. If it is taken out of the runqueue at
line 76 it will stay off the runqueue forever in the TASK_UNINTERRUBTIBLE
state!
Exactly. This is definilty the bug in the driver code - a developer just
didn;t care about proper utilization of set_current_state(). The driver 
works
just because as you have described - his fortune
that scheduler doesn't remove task in not TASK_RUNNING state from a run 
queue.
And my main question was - does everybody think it's ok have task in not 
TASK_RUNNING state in run queue. My current feeling is that this should 
not be allowed.
As I read the use PREEMPT_ACTIVE, it is there to test on wether this
rescheduling is volentery or forced (a preemption). If it is forced the
task shall ofcourse not go off the runqueue but stay there to run again
when it gets the highest priority. That is why PREEMPT_ACTIVE is set in
preempt_schedule() and preempt_schedule_irq(). On the other hand if the
task itself has called schedule() or schedule_timeout() it has to go out
of the runqueue and wait for some event to wake it up.
You right - it works perfectly - but not for  my test case - I believe 
task in not TASK_RUNNING state should be removed from a run queue by the 
first (any - volontery or forced) execution of the schedule() which 
detects the task state is not TASK_RUNNIG.
Yes there will be tasks in state other that TASK_RUNNING on the runqueue.
The "bug" as I see it is in the scheduler interface: There is no way to
set the task state and call schedule() or schedule_timeout() atomicly.
Therefore you can be preempted while the state is not TASK_RUNNING.
Exactly. IMO this interface is weird and needs rework. I don;t undestand 
what the reason to set task state before schedule_timeout() call but not 
inside, right before the schedule(). The actual task state may be passed 
as a parameter.

As to tasks in not TASK_RUNNING state into a run queue - I always 
believe the definition of a run queue is - queue of tasks ready to run, 
i.e. in TASK_RUNNING state.
	
	Eugeny
Esben
On Thu, 3 Mar 2005, Eugeny S. Mints wrote:

please consider the following scenario for full RT kernel.
Task A is running then an irq is occured which in turn wakes up irq 
related thread (B) of a higher priority than A.

my current understanding that actual context switch between A and B will 
occure at preempt_schedule_irq() on the "return form irq " path.

in this case the following "if" statement in __schedule() always returns 
false since  preempt_schedule_irq() always sets up  PREEMPT_ACTIVE 
before __schedule() call.

        if ((prev->state & ~TASK_RUNNING_MUTEX) &&
                        !(preempt_count() & PREEMPT_ACTIVE)) {
as result the deactivate() is never called for preempted task A in this 
scenario. BUt if the task A is preempted while not in TASK_RUNNING state 
such behaviour seems incorrect since we get a task in not TASK_RUNNING 
state linked into a run queue.

An example:
drivers/net/irda/sir_dev.c: 76 (2.6.10 kernel)
        spin_lock_irqsave(&dev->tx_lock, flags); /* serialize th other 
tx operations */
        while (dev->tx_buff.len > 0) {    /* wait until tx idle */
                spin_unlock_irqrestore(&dev->tx_lock, flags);
76:             set_current_state(TASK_UNINTERRUPTIBLE);
                schedule_timeout(msecs_to_jiffies(10));
                spin_lock_irqsave(&dev->tx_lock, flags);
        }

At  line 76 irqs are enabled, preemption is enabled.
Let assume the task A executes this code and gets preempted right after 
line 76. Task state is TASK_UNINTERRUPTIBLE but it will not be 
deactevated. Of cource this is the bug in set_current_state() 
utilization in this particular driver but schedule stuff should be 
robust to such bugs I believe. There are a lot such bugs in the kernel I 
believe.

Not sure what the actual reason for !(preempt_count() & PREEMPT_ACTIVE)) 
  condition is but if it's just a sort of optimization (not remove a 
task from run queue if it was preemped in TASK_RUNNING state) then 
probably it should be removed in order to save correctness. Patch attached.

        Eugeny




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