* [RT] possible bug in trace_start_sched_wakeup
@ 2006-01-27 1:57 Steven Rostedt
2006-01-27 4:38 ` Steven Rostedt
0 siblings, 1 reply; 6+ messages in thread
From: Steven Rostedt @ 2006-01-27 1:57 UTC (permalink / raw)
To: Ingo Molnar; +Cc: LKML
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RT] possible bug in trace_start_sched_wakeup
2006-01-27 1:57 [RT] possible bug in trace_start_sched_wakeup Steven Rostedt
@ 2006-01-27 4:38 ` Steven Rostedt
2006-01-27 9:46 ` Ingo Molnar
2006-01-27 9:54 ` Ingo Molnar
0 siblings, 2 replies; 6+ messages in thread
From: Steven Rostedt @ 2006-01-27 4:38 UTC (permalink / raw)
To: Ingo Molnar; +Cc: LKML
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);
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RT] possible bug in trace_start_sched_wakeup
2006-01-27 4:38 ` Steven Rostedt
@ 2006-01-27 9:46 ` Ingo Molnar
2006-01-27 12:46 ` Steven Rostedt
2006-01-27 9:54 ` Ingo Molnar
1 sibling, 1 reply; 6+ messages in thread
From: Ingo Molnar @ 2006-01-27 9:46 UTC (permalink / raw)
To: Steven Rostedt; +Cc: LKML
* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RT] possible bug in trace_start_sched_wakeup
2006-01-27 9:46 ` Ingo Molnar
@ 2006-01-27 12:46 ` Steven Rostedt
0 siblings, 0 replies; 6+ messages in thread
From: Steven Rostedt @ 2006-01-27 12:46 UTC (permalink / raw)
To: Ingo Molnar; +Cc: LKML
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RT] possible bug in trace_start_sched_wakeup
2006-01-27 4:38 ` Steven Rostedt
2006-01-27 9:46 ` Ingo Molnar
@ 2006-01-27 9:54 ` Ingo Molnar
2006-01-27 12:47 ` Steven Rostedt
1 sibling, 1 reply; 6+ messages in thread
From: Ingo Molnar @ 2006-01-27 9:54 UTC (permalink / raw)
To: Steven Rostedt; +Cc: LKML
* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RT] possible bug in trace_start_sched_wakeup
2006-01-27 9:54 ` Ingo Molnar
@ 2006-01-27 12:47 ` Steven Rostedt
0 siblings, 0 replies; 6+ messages in thread
From: Steven Rostedt @ 2006-01-27 12:47 UTC (permalink / raw)
To: Ingo Molnar; +Cc: LKML
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-01-27 12:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-01-27 1:57 [RT] possible bug in trace_start_sched_wakeup Steven Rostedt
2006-01-27 4:38 ` Steven Rostedt
2006-01-27 9:46 ` Ingo Molnar
2006-01-27 12:46 ` Steven Rostedt
2006-01-27 9:54 ` Ingo Molnar
2006-01-27 12:47 ` Steven Rostedt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).