* thread_saved_pc() on x86 returns (thread->esp)[3]. Bogus, since the
third word from the stack top has absolutely nothing to return address of
any kind. Correct value: (thread->esp)[0][1] - ebp is on top of the stack
and the rest is obvious. Current code gives completely bogus addresses -
try to say Alt-SysRq-T and watch the show.
* kernel/sched::show_state() used the wrong check for process being
on some CPU - p==current is nice for UP, but on SMP it's obviously wrong.
Moroever, nothing prevents schedule() another CPU while we are going through
the process state. Fix: show_state() grabs runqueue_lock, show_task() checks
p->has_cpu instead of p == current.
Please, apply.
Cheers,
Al
diff -urN rc11-2/include/asm-i386/processor.h rc11-2-show_task/include/asm-i386/processor.h
--- rc11-2/include/asm-i386/processor.h Fri Nov 10 09:14:04 2000
+++ rc11-2-show_task/include/asm-i386/processor.h Fri Nov 10 16:08:15 2000
@@ -412,7 +412,7 @@
*/
extern inline unsigned long thread_saved_pc(struct thread_struct *t)
{
- return ((unsigned long *)t->esp)[3];
+ return ((unsigned long **)t->esp)[0][1];
}
unsigned long get_wchan(struct task_struct *p);
diff -urN rc11-2/kernel/sched.c rc11-2-show_task/kernel/sched.c
--- rc11-2/kernel/sched.c Fri Nov 10 09:18:43 2000
+++ rc11-2-show_task/kernel/sched.c Fri Nov 10 16:11:39 2000
@@ -1121,12 +1121,12 @@
else
printk(" ");
#if (BITS_PER_LONG == 32)
- if (p == current)
+ if (p->has_cpu)
printk(" current ");
else
printk(" %08lX ", thread_saved_pc(&p->thread));
#else
- if (p == current)
+ if (p->has_cpu)
printk(" current task ");
else
printk(" %016lx ", thread_saved_pc(&p->thread));
@@ -1186,6 +1186,7 @@
void show_state(void)
{
struct task_struct *p;
+ unsigned long flags;
#if (BITS_PER_LONG == 32)
printk("\n"
@@ -1196,10 +1197,12 @@
" free sibling\n");
printk(" task PC stack pid father child younger older\n");
#endif
+ spin_lock_irqsave(&runqueue_lock, flags);
read_lock(&tasklist_lock);
for_each_task(p)
show_task(p);
read_unlock(&tasklist_lock);
+ spin_unlock_irqrestore(&runqueue_lock, flags);
}
/*
On Fri, Nov 10, 2000 at 04:26:32PM -0500, Alexander Viro wrote:
> * thread_saved_pc() on x86 returns (thread->esp)[3]. Bogus, since the
> third word from the stack top has absolutely nothing to return address of
> any kind. Correct value: (thread->esp)[0][1] - ebp is on top of the stack
> and the rest is obvious. Current code gives completely bogus addresses -
> try to say Alt-SysRq-T and watch the show.
Reminds me that the Alpha implementation of get_wchan() looks to me like
it doesn't handle all cases of schedule() being called from another
scheduler function correctly. Some Alpha guru may want to take a look at
it.
I recently had to fix the mips / mips64 versions of get_wchan() - for the
dozenth time. I'd really like to see a wchan field in task_struct to avoid
get_wchan breaking every once in a while. Current implementation more than
qualifies as a crazy hack ...
Ralf
On Fri, 10 Nov 2000, Alexander Viro wrote:
> diff -urN rc11-2/include/asm-i386/processor.h rc11-2-show_task/include/asm-i386/processor.h
> --- rc11-2/include/asm-i386/processor.h Fri Nov 10 09:14:04 2000
> +++ rc11-2-show_task/include/asm-i386/processor.h Fri Nov 10 16:08:15 2000
> @@ -412,7 +412,7 @@
> */
> extern inline unsigned long thread_saved_pc(struct thread_struct *t)
> {
> - return ((unsigned long *)t->esp)[3];
> + return ((unsigned long **)t->esp)[0][1];
> }
The above needs to get verified: it should be something like
unsigned long *ebp = *((unsigned long **)t->esp);
if ((void *) ebp < (void *) t)
return 0;
if ((void *) ebp >= (void *) t + 2*PAGE_SIZE)
return 0;
if (3 & (unsigned long)ebp)
return 0;
return *ebp;
because otherwise I guarantee that we'll eventually have a bug with a
invalid pointer reference in the debugging code and that would be bad.
Linus
On Sat, 11 Nov 2000, Linus Torvalds wrote:
> On Fri, 10 Nov 2000, Alexander Viro wrote:
> > diff -urN rc11-2/include/asm-i386/processor.h rc11-2-show_task/include/asm-i386/processor.h
> > --- rc11-2/include/asm-i386/processor.h Fri Nov 10 09:14:04 2000
> > +++ rc11-2-show_task/include/asm-i386/processor.h Fri Nov 10 16:08:15 2000
> > @@ -412,7 +412,7 @@
> > */
> > extern inline unsigned long thread_saved_pc(struct thread_struct *t)
> > {
> > - return ((unsigned long *)t->esp)[3];
> > + return ((unsigned long **)t->esp)[0][1];
> > }
>
> The above needs to get verified: it should be something like
>
> unsigned long *ebp = *((unsigned long **)t->esp);
>
> if ((void *) ebp < (void *) t)
> return 0;
> if ((void *) ebp >= (void *) t + 2*PAGE_SIZE)
> return 0;
> if (3 & (unsigned long)ebp)
> return 0;
> return *ebp;
>
> because otherwise I guarantee that we'll eventually have a bug with a
> invalid pointer reference in the debugging code and that would be bad.
I would probably turn it into
unsigned long *ebp = *((unsigned long **)t->esp);
/* Bits 0,1 and 13..31 must be shared with the stack base */
if (((unsigned long)ebp ^ (unsigned long)t) & ~(2*PAGE_SIZE-4))
return 0;
return *ebp;
Comments? Alternative variant: just let schedule() store its return address
in the task_struct. Yeah, it's couple of tacts per schedule(). And much saner
code, without second-guessing the compiler. OTOH, the value is used only
by Alt-SysRq-T, so... Hell knows.
Cheers,
Al
On Sat, 11 Nov 2000, Alexander Viro wrote:
> I would probably turn it into
> unsigned long *ebp = *((unsigned long **)t->esp);
ebp++; /* We want return address, not the previous frame pointer */
> /* Bits 0,1 and 13..31 must be shared with the stack base */
> if (((unsigned long)ebp ^ (unsigned long)t) & ~(2*PAGE_SIZE-4))
> return 0;
>
> return *ebp;
Sorry.
Cheers,
Al
On Sat, Nov 11, 2000 at 02:06:34PM +0100, Ralf Baechle wrote:
> Reminds me that the Alpha implementation of get_wchan() looks to me like
> it doesn't handle all cases of schedule() being called from another
> scheduler function correctly.
Certainly not -- it's impossible.
> I'd really like to see a wchan field in task_struct to avoid
> get_wchan breaking every once in a while.
Indeed.
r~
On Sat, Nov 11, 2000 at 10:18:46PM -0500, Alexander Viro wrote:
> Alternative variant: just let schedule() store its return address
> in the task_struct.
Please please. I can't reliably unwind the stack on Alpha.
> OTOH, the value is used only by Alt-SysRq-T, so... Hell knows.
No, it's also used by 'ps -l'. See wchan.
r~
Richard Henderson <[email protected]> writes:
> > OTOH, the value is used only by Alt-SysRq-T, so... Hell knows.
>
> No, it's also used by 'ps -l'. See wchan.
ps -l uses get_wchan() (an architecture specific function from
arch/*/kernel/process.c) to get the return address from
schedule(). And now thread_saved_pc() seems to do the same (at least
on x86). Is there any reason to have two architecture specific
functions doing the same or do I miss something?
Jean
PS: Architectures other then x86 use thread_saved_pc() to implement
get_wchan(). If the debug output of Alt-SysRq-T is supposed to show
the waiting channel we should use get_wchan() instead of thread_saved_pc().
--
I get up each morning, gather my wits.
Pick up the paper, read the obits.
if I'm not there I know I'm not dead.
So I eat a good breakfast and go back to bed. Peete Seeger
On Tue, Nov 14, 2000 at 10:19:32AM +0100, Jean Wolter wrote:
> > > OTOH, the value is used only by Alt-SysRq-T, so... Hell knows.
> >
> > No, it's also used by 'ps -l'. See wchan.
>
> ps -l uses get_wchan() (an architecture specific function from
> arch/*/kernel/process.c) to get the return address from
> schedule(). And now thread_saved_pc() seems to do the same (at least
> on x86). Is there any reason to have two architecture specific
> functions doing the same or do I miss something?
>
> Jean
>
> PS: Architectures other then x86 use thread_saved_pc() to implement
> get_wchan(). If the debug output of Alt-SysRq-T is supposed to show
> the waiting channel we should use get_wchan() instead of thread_saved_pc().
Probably historic reasons, it's been that way as long as I can think back.
Yet the use of thread_saved_pc() in kernel/sched.c should imho be considered
a buglet and be replaced by get_wchan to get more meaningful debugging
information.
Ralf