[RT] possible bug in trace_start_sched_wakeup

[RT] possible bug in trace_start_sched_wakeup

[Steven Rostedt]

Ingo,

I've been agonizing over the latency code, and have a question about
__trace_start_sched_wakup.

Here we have:

void __trace_start_sched_wakeup(struct task_struct *p)
{
	struct cpu_trace *tr;
	int cpu;

	if (trace_user_triggered || !wakeup_timing)
		return;

	spin_lock(&sch.trace_lock);
	if (sch.task && (sch.task->prio >= p->prio))
		goto out_unlock;


I don't get the sch.task->prio >= p->prio.  Here the lower the number
the greater the priority.  So this if statement is saying:

If sch.task is either NULL or if p is greater in priority than or equal
to the priority of sch.task then quit.

Then again later in  trace_stop_sched_switched:

		if (sch.task && (sch.task->prio >= p->prio))
			sch.task = NULL;

Again, if p is a higher priority than sch.task we set sch.task to NULL??


What am I missing here?

Thanks,

-- Steve

Re: [RT] possible bug in trace_start_sched_wakeup

[Steven Rostedt]

Ingo,

I'm assuming that this was a bug, so I'm patching it.  I made it now
test to see if the new process is realtime and higher priority than the
current tracing process to continue.

I'll analyze it more tomorrow (after a few hours of sleep) to see if
this makes sense, as well as hope that you have a comment or two saying
it does, or if it doesn't, to enlighten me to why.

Thanks,

-- Steve


Index: linux-2.6.15-rt15/kernel/latency.c
===================================================================
--- linux-2.6.15-rt15.orig/kernel/latency.c	2006-01-25 17:02:26.000000000 -0500
+++ linux-2.6.15-rt15/kernel/latency.c	2006-01-26 21:03:30.000000000 -0500
@@ -1908,7 +1908,7 @@
 		return;
 
 	spin_lock(&sch.trace_lock);
-	if (sch.task && (sch.task->prio >= p->prio))
+	if (sch.task && ((sch.task->prio <= p->prio) || !rt_task(p)))
 		goto out_unlock;
 
 	/*
@@ -1974,7 +1974,7 @@
 	} else {
 		if (sch.task)
 			trace_special_pid(sch.task->pid, sch.task->prio, p->prio);
-		if (sch.task && (sch.task->prio >= p->prio))
+		if (sch.task && ((sch.task->prio <= p->prio) || !rt_task(p)))
 			sch.task = NULL;
 		spin_unlock(&sch.trace_lock);
 	}

Re: [RT] possible bug in trace_start_sched_wakeup

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

>  	spin_lock(&sch.trace_lock);
> -	if (sch.task && (sch.task->prio >= p->prio))
> +	if (sch.task && ((sch.task->prio <= p->prio) || !rt_task(p)))
>  		goto out_unlock;

good catch - but i'd not do the !rt_task(p) condition, because e.g. PI 
related priority boosting works _without_ changing p->policy. So it is 
p->prio that controls. I.e. a simple "sch.task->prio <= p->prio" should 
be enough.

	Ingo

Re: [RT] possible bug in trace_start_sched_wakeup

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

>  			trace_special_pid(sch.task->pid, sch.task->prio, p->prio);
> -		if (sch.task && (sch.task->prio >= p->prio))
> +		if (sch.task && ((sch.task->prio <= p->prio) || !rt_task(p)))
>  			sch.task = NULL;

this second condition i'd not change: it just expresses the rare case 
where a higher-prio task hits the CPU that we somehow did not start to 
trace. In that case we just zap the current trace.

	Ingo

Re: [RT] possible bug in trace_start_sched_wakeup

[Steven Rostedt]

On Fri, 2006-01-27 at 10:46 +0100, Ingo Molnar wrote:
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> >  	spin_lock(&sch.trace_lock);
> > -	if (sch.task && (sch.task->prio >= p->prio))
> > +	if (sch.task && ((sch.task->prio <= p->prio) || !rt_task(p)))
> >  		goto out_unlock;
> 
> good catch - but i'd not do the !rt_task(p) condition, because e.g. PI 
> related priority boosting works _without_ changing p->policy. So it is 
> p->prio that controls. I.e. a simple "sch.task->prio <= p->prio" should 
> be enough.

Ah, I don't know what I was thinking about the rt_task part (I was
working on very little sleep).  You're right.  Nuke it!

Thanks,

-- Steve

Re: [RT] possible bug in trace_start_sched_wakeup

[Steven Rostedt]

On Fri, 2006-01-27 at 10:54 +0100, Ingo Molnar wrote:
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> >  			trace_special_pid(sch.task->pid, sch.task->prio, p->prio);
> > -		if (sch.task && (sch.task->prio >= p->prio))
> > +		if (sch.task && ((sch.task->prio <= p->prio) || !rt_task(p)))
> >  			sch.task = NULL;
> 
> this second condition i'd not change: it just expresses the rare case 
> where a higher-prio task hits the CPU that we somehow did not start to 
> trace. In that case we just zap the current trace.
> 

OK, I think I understand that part now too.  There wasn't any comments
about what it was doing so I wasn't sure if that was the right move.
But looking at it further, I believe you are right.

Thanks,

-- Steve