Date: Thu, 16 Feb 2006 13:48:24 +0100 From: Marius Strobl <marius@alchemy.franken.de> To: Antoine Brodin <antoine.brodin@laposte.net>, jhb@freebsd.org Cc: freebsd-sparc64@freebsd.org Subject: Re: sparc64/93226: DEBUG_LOCKS (really stack_save()) causes panics on sparc64 Message-ID: <20060216134823.S53619@newtrinity.zeist.de> In-Reply-To: <20060214205432.38121641.antoine.brodin@laposte.net>; from antoine.brodin@laposte.net on Tue, Feb 14, 2006 at 08:54:32PM %2B0100 References: <200602131150.k1DBo6S1074438@freefall.freebsd.org> <200602131223.51561.jhb@freebsd.org> <20060213193613.547d1b8f.antoine.brodin@laposte.net> <200602131430.11228.jhb@freebsd.org> <20060213213719.7767921e.antoine.brodin@laposte.net> <20060214094744.A81690@newtrinity.zeist.de> <20060214205432.38121641.antoine.brodin@laposte.net>
next in thread | previous in thread | raw e-mail | index | archive | help
--uZ3hkaAS1mZxFaxD
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
On Tue, Feb 14, 2006 at 08:54:32PM +0100, Antoine Brodin wrote:
> Marius Strobl <marius@alchemy.franken.de> wrote:
> > On Mon, Feb 13, 2006 at 09:37:19PM +0100, Antoine Brodin wrote:
> > > John Baldwin <jhb@freebsd.org> wrote:
> > > > On Monday 13 February 2006 13:36, Antoine Brodin wrote:
> > > > > John Baldwin <jhb@freebsd.org> wrote:
> > > > > > If there are kernel functions before the assembly ones (dependent on link
> > > > > > order) then this would wrongly bail when it hit those. I think you need
> > > > > > to do what the ddb stack tracing code does which is to lookup the symbol
> > > > > > name and do a bcmp() on the first 4 chars to recognize trapframes.
> > > > >
> > > > > I ran objdump -d on a sparc64 kernel and it looks like tl0_* and tl1_*
> > > > > are always at the beginning of the code, there is some kind of magic.
> > > >
> > > > magic aside, it would be best to use the same algorithm in both places IMO.
> > > > It would also be a lot more intuitive to other folks later on.
> > >
> > > There are 2 reasons why I didn't use db_search_symbol() and
> > > db_symbol_values() :
> > >
> > > - first they aren't reentrant, they use a global variable
> > > db_last_symtab and they can panic if a thread sets db_last_symtab to 0
> > > while another one is using it. I found this in my mail archive :
> > > %%%
> > > Stopped at X_db_symbol_values+0x18: cmpl $0,0xc(%eax)
> > > db> trace
> > > Tracing pid 34983 tid 100093 td 0xc2e9c640
> > > X_db_symbol_values(0,c0738214,e84859f4,e84859c4,7c) at X_db_symbol_values+0x18
> > > db_symbol_values(c0738214,e84859f4,0,c89d19c8,0) at db_symbol_values+0x40
> > > %%%
> > > It can be fixed easily but I don't have the fix anymore.
> > > You can use linker_ddb_search_symbol() and linker_ddb_symbol_values()
> > > too that are safer.
> > >
> > > - the second reason is performance. if you replace
> > > CTRSTACK(KTR_LOCK, &stack, 0, 1);
> > > by
> > > CTRSTACK(KTR_LOCK, &stack, 0, 0);
> > > in kern_lock.c, i.e. if you print the symbol name in the ktr traces, you
> > > will notice that your box is extremely slow. (you type ls, you wait 4 or 5
> > > seconds and you have the result)
> > >
> >
> > Can't we just use what's done in support.S and add two dummy symbols
> > to mark the begin and end of the asm functions in question?
>
> There's already a tl0_base symbol. You can add a tl0_end (or tl1_end ?)
> symbol between tl1_intr and fork_trampoline.
>
Ok, how about the attached patch? It uses two pairs of dummy symbols
in exception.S to determine in stack_save() whether it was one of the
tl0_*() or tl1_*() asm functions; one pair for those in the .trap
section that is "magically" placed at the beginning of the .text
section via the linker script and the other pair for those in the
regular .text section. That way we don't rely on the location of
these functions in the kernel and don't have the performance penalty
of *search_symbol()/*symbol_values(). For consistency db_backtrace()
is changed to also use the new markers instead of bcmp()'ing with
the symbol names.
Marius
--
This mail was scanned by AntiVir Milter.
This product is licensed for non-commercial use.
See www.antivir.de for details.
--uZ3hkaAS1mZxFaxD
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="db_tl.diff"
Index: exception.S
===================================================================
RCS file: /mnt/futile/usr/data/bsd/cvs/fbsd/src/sys/sparc64/sparc64/exception.S,v
retrieving revision 1.71
diff -u -r1.71 exception.S
--- exception.S 16 Jan 2006 01:35:25 -0000 1.71
+++ exception.S 16 Feb 2006 11:59:50 -0000
@@ -166,6 +166,10 @@
ldx [ASP_REG + 0], %g1 ; \
inc 16, ASP_REG
+ .globl tl_text_begin
+tl_text_begin:
+ nop
+
ENTRY(tl1_kstack_fault)
rdpr %tl, %g1
1: cmp %g1, 2
@@ -1818,6 +1822,10 @@
.endm
.sect .trap
+ .globl tl_trap_begin
+tl_trap_begin:
+ nop
+
.align 0x8000
.globl tl0_base
@@ -2015,6 +2023,10 @@
tl1_gen T_RSTRWP_VIRT ! 0x303
tl1_reserved 252 ! 0x304-0x3ff
+ .globl tl_trap_end
+tl_trap_end:
+ nop
+
/*
* User trap entry point.
*
@@ -2901,6 +2913,10 @@
retry
END(tl1_intr)
+ .globl tl_text_end
+tl_text_end:
+ nop
+
/*
* Freshly forked processes come here when switched to for the first time.
* The arguments to fork_exit() have been setup in the locals, we must move
Index: db_trace.c
===================================================================
RCS file: /mnt/futile/usr/data/bsd/cvs/fbsd/src/sys/sparc64/sparc64/db_trace.c,v
retrieving revision 1.24
diff -u -r1.24 db_trace.c
--- db_trace.c 3 Aug 2005 04:27:39 -0000 1.24
+++ db_trace.c 16 Feb 2006 11:51:27 -0000
@@ -49,6 +49,11 @@
#include <ddb/db_variables.h>
#include <ddb/db_watch.h>
+extern char tl_trap_begin[];
+extern char tl_trap_end[];
+extern char tl_text_begin[];
+extern char tl_text_end[];
+
#define INKERNEL(va) \
((va) >= VM_MIN_KERNEL_ADDRESS && (va) <= VM_MAX_KERNEL_ADDRESS)
@@ -259,8 +264,10 @@
name = "(null)";
fp = (struct frame *)(db_get_value((db_addr_t)&fp->fr_fp,
sizeof(fp->fr_fp), FALSE) + SPOFF);
- if (bcmp(name, "tl0_", 4) == 0 ||
- bcmp(name, "tl1_", 4) == 0) {
+ if ((value > (u_long)tl_trap_begin &&
+ value < (u_long)tl_trap_end) ||
+ (value > (u_long)tl_text_begin &&
+ value < (u_long)tl_text_end)) {
tf = (struct trapframe *)(fp + 1);
npc = db_get_value((db_addr_t)&tf->tf_tpc,
sizeof(tf->tf_tpc), FALSE);
@@ -307,6 +314,12 @@
callpc = fp->fr_pc;
if (!INKERNEL(callpc))
break;
+ /* Don't bother traversing trap frames. */
+ if ((callpc > (u_long)tl_trap_begin &&
+ callpc < (u_long)tl_trap_end) ||
+ (callpc > (u_long)tl_text_begin &&
+ callpc < (u_long)tl_text_end))
+ break;
if (stack_put(st, callpc) == -1)
break;
fp = (struct frame *)(fp->fr_fp + SPOFF);
--uZ3hkaAS1mZxFaxD--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20060216134823.S53619>