O(1) scheduler seems to lock up on sched_FIFO and sched_RR ta sks

Hi All

I have another test case that is showing this behavior. It is very similar to George's, however, it is a simplification of another one using threads that a co-worker, Adam Li found a few days ago.

Parent (FIFO 5) forks child that sets itself to FIFO 4 and busy loops, then it sleeps five seconds and kills the child.

Doing SysRq + T after a while shows the parent'd call trace to be at sys_rt_sigaction+0xd1, that is just inside the final copy_to_user() in signal.c:sys_rt_sigaction().

Reprioritizing events/0 to FIFO 5+ fixes the inversion.

If I call nanosleep directly (with system() instead of glibc's sleep(), so I avoid all the rt_sig calls), I get the parent process always stuck in work_resched+0x5, in entry.S:work_resched, just after the call to the scheduler - however, I cannot trace what is happening inside the scheduler.

My point here is: I am trying to trace where this program is making use of workqueues inside of the kernel, and I can find none. The only place where I need to look some more is inside the timer code, but in a quick glance, it seems it is not being used, so why is it affected by the reprioritization of the events/0 thread? George, can you help me here?

kernel is 2.5.67, SMP and PREEMPT with maxcpus=1; tomorrow I will try .72 ...

Iñaky Pérez-González -- Not speaking for Intel -- all opinions are my own (and my fault)

well, printk (console input/output) can already make use of keventd.

Ingo

Hm! I wonder. Robert is working on a fix for schedsetschedule() where it fails to actually tell the scheduler to switch to a process that it just made higher priority or away from one it just lowered.

The net result is that the caller keeps running (FIFO for all in this case) when, in fact it should have been switched out. Next time schedule() actually switches, it is all sorted out again. Could the elavation of the events/0 thread cause this needed switch?

-g

George Anzinger   [email protected]
High-res-timers:  http://sourceforge.net/projects/high-res-timers/
Preemption patch: http://www.kernel.org/pub/linux/kernel/people/rml

I posted a fix for this a month ago that was ignored. Which is a good thing, since now that I look at it again, I don't care for the approach I took nor does it appear to be complete.

Joe

----------- original posting

setscheduler is not forcing a reschedule when needed like set_user_nice does. It should.

Joe

@@ -1716,6 +1716,7 @@
 	unsigned long flags;
 	runqueue_t *rq;
 	task_t *p;
+	int oldprio;
 
 	if (!param || pid < 0)
 		goto out_nounlock;

@@ -1778,12 +1779,20 @@
 	retval = 0;
 	p->policy = policy;
 	p->rt_priority = lp.sched_priority;
+	oldprio = p->prio;
 	if (policy != SCHED_NORMAL)
 		p->prio = MAX_USER_RT_PRIO-1 - p->rt_priority;
 	else
 		p->prio = p->static_prio;
-	if (array)
+	if (array) {
 		__activate_task(p, task_rq(p));
+		/*
+		 * Reschedule if on a CPU and the priority dropped, or not on
+		 * a CPU and the priority rose above the currently running task.
+		 */
+		if ((rq->curr == p) ? (p->prio > oldprio) : (p->prio < rq->curr->prio))
+			resched_task(rq->curr);
+	}
 
 out_unlock:
 	task_rq_unlock(rq, &flags);

Ah, sorry for missing it. Other than that tertiary statement inside an if ;) my patch is about the same.

Why do you think it is incomplete? It looks correct to me.

Robert Love

It may be better to add it to __activate_task() rather than after the single activate_task() use. At the time I wrote the patch I did not think to look at the five __activate_task() calls to see if they each needed the test. By me not looking, my patch is automatically incorrect, even if it turns out to be correct.

Joe

Here is my patch. It is the same idea as Joe's. Is there a better fix?

Basically, the problem is that setscheduler() does not set need_resched when needed. There are two basic cases where this is needed:

- the task is running, but now it is no longer the highest priority task on the rq
- the task is not running, but now it is the highest priority task on the rq

In either case, we need to set need_resched to invoke the scheduler.

Patch is against 2.5.72. Comments?

Robert Love

setschedule() needs to force a reschedule in some situations.

 kernel/sched.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletion(-)

diff -urN linux-2.5.72/kernel/sched.c linux/kernel/sched.c
@@ -1691,6 +1691,7 @@
 {
 	struct sched_param lp;
 	int retval = -EINVAL;
+	int oldprio;
 	prio_array_t *array;
 	unsigned long flags;
 	runqueue_t *rq;

@@ -1757,12 +1758,24 @@
 	retval = 0;
 	p->policy = policy;
 	p->rt_priority = lp.sched_priority;
+	oldprio = p->prio;
 	if (policy != SCHED_NORMAL)
 		p->prio = MAX_USER_RT_PRIO-1 - p->rt_priority;
 	else
 		p->prio = p->static_prio;
-	if (array)
+	if (array) {
 		__activate_task(p, task_rq(p));
+		/*
+		 * Reschedule if we are currently running on this runqueue and
+		 * our priority decreased, or if we are not currently running on
+		 * this runqueue and our priority is higher than the current's
+		 */
+		if (rq->curr == p) {
+			if (p->prio > oldprio)
+				resched_task(rq->curr);
+		} else if (p->prio < rq->curr->prio)
+			resched_task(rq->curr);
+	}
 
 out_unlock:
 	task_rq_unlock(rq, &flags);

Looks good to me.

migration_thread and try_to_wake_up already have a simplier version of your test that seems to be correct for that environment, so no change is needed there.

wake_up_forked_process in principle might need your patch, but as it appears to be called only from boot code it is unimportant that it have the lowest possible latency, so no change is needed there either.

Joe

Good.

Agreed.

This is worse than just a latency issue, by the way. Imagine if a FIFO/50 thread promotes a FIFO/40 thread to FIFO/60. The thread should run immediately (because, at priority 60, it is the highest), but it may not until the FIFO/50 thread completes.

Robert Love