| Message ID | 1310351976-24078-2-git-send-email-tiejun.chen@windriver.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
v1 -> v2: when allocate pgirq_ctx, use 'hw_cpu' to identify cpu ID in exc_lvl_early_init(). BTW, I already validated these patches on fsl mpc8536 by kprobe do_fork() and show_interrupts(). Note this bug I fixed is from another email thread, "[BUG?]3.0-rc4+ftrace+kprobe: set kprobe at instruction 'stwu' lead to system crash/freeze", Yong Zhang, <yong.zhang0@gmail.com> previously reported. Tiejun
On Mon, Jul 11, 2011 at 10:39:35AM +0800, Tiejun Chen wrote: > When enable CONFIG_PREEMPT we will trigger the following call trace: > > BUG: scheduling while atomic: swapper/1/0x10000000 > ... > > krpobe always goes through the following path: > > program_check_exception() > | > + notify_die(DIE_BPT, "breakpoint",...) > | > + kprobe_handler() > | > + preempt_disable(); > + break_handler() <- preempt_enable_no_resched() > + emulate_step() > + preempt_enable_no_resched() > ... > exit > > We should remove unnecessary preempt_enable_no_resched() inside of break_handler() > since looks longjmp_break_handler() always go the above path. The current code is correct. Reasoning follows... setjmp_pre_handler() and longjmp_break_handler() are used only for jprobes. In the case of a jprobe, the code flow would be: bp hit -> kprobe_handler() -> preempt_disable() -> setjmp_pre_handler() (not that since this routine returns 1, we skip sstep here) -> jp->entry() -> jprobe_return() -> bp hit -> kprobe_handler() -> preempt_disable() again -> longjmp_break_handler() -> preempt_enable() -> sstep -> preempt_enable() (for the second kprobe_handler() entry). You could verify this with a preempt_count() printk with a CONFIG_PREEMPT=y kernel. > Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com> Nack, sorry :-) Ananth
Ananth N Mavinakayanahalli wrote: > On Mon, Jul 11, 2011 at 10:39:35AM +0800, Tiejun Chen wrote: >> When enable CONFIG_PREEMPT we will trigger the following call trace: >> >> BUG: scheduling while atomic: swapper/1/0x10000000 >> ... >> >> krpobe always goes through the following path: >> >> program_check_exception() >> | >> + notify_die(DIE_BPT, "breakpoint",...) >> | >> + kprobe_handler() >> | >> + preempt_disable(); >> + break_handler() <- preempt_enable_no_resched() >> + emulate_step() >> + preempt_enable_no_resched() >> ... >> exit >> >> We should remove unnecessary preempt_enable_no_resched() inside of break_handler() >> since looks longjmp_break_handler() always go the above path. > > The current code is correct. Reasoning follows... > > setjmp_pre_handler() and longjmp_break_handler() are used only for > jprobes. In the case of a jprobe, the code flow would be: > > bp hit -> kprobe_handler() -> preempt_disable() -> setjmp_pre_handler() > (not that since this routine returns 1, we skip sstep here) -> jp->entry() > -> jprobe_return() -> bp hit -> kprobe_handler() -> preempt_disable() again > -> longjmp_break_handler() -> preempt_enable() -> sstep -> preempt_enable() > (for the second kprobe_handler() entry). > > You could verify this with a preempt_count() printk with a > CONFIG_PREEMPT=y kernel. > >> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com> > > Nack, sorry :-) You're right. When use EXC_LEVEL_EXCEPTION_PROLOG for Critical/Machine check, if the exception came from kernel mode, we copy thread_info flags, *preempt*, and task pointer from the process thread_info. So here I steal EXC_LEVEL_EXCEPTION_PROLOG for Program Exception, preempt count would be corrupted incorrectly. Thanks Tiejun > > Ananth >
tiejun.chen wrote: > Ananth N Mavinakayanahalli wrote: >> On Mon, Jul 11, 2011 at 10:39:35AM +0800, Tiejun Chen wrote: >>> When enable CONFIG_PREEMPT we will trigger the following call trace: >>> >>> BUG: scheduling while atomic: swapper/1/0x10000000 >>> ... >>> >>> krpobe always goes through the following path: >>> >>> program_check_exception() >>> | >>> + notify_die(DIE_BPT, "breakpoint",...) >>> | >>> + kprobe_handler() >>> | >>> + preempt_disable(); >>> + break_handler() <- preempt_enable_no_resched() >>> + emulate_step() >>> + preempt_enable_no_resched() >>> ... >>> exit >>> >>> We should remove unnecessary preempt_enable_no_resched() inside of break_handler() >>> since looks longjmp_break_handler() always go the above path. >> The current code is correct. Reasoning follows... >> >> setjmp_pre_handler() and longjmp_break_handler() are used only for >> jprobes. In the case of a jprobe, the code flow would be: >> >> bp hit -> kprobe_handler() -> preempt_disable() -> setjmp_pre_handler() >> (not that since this routine returns 1, we skip sstep here) -> jp->entry() >> -> jprobe_return() -> bp hit -> kprobe_handler() -> preempt_disable() again >> -> longjmp_break_handler() -> preempt_enable() -> sstep -> preempt_enable() >> (for the second kprobe_handler() entry). >> >> You could verify this with a preempt_count() printk with a >> CONFIG_PREEMPT=y kernel. >> >>> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com> >> Nack, sorry :-) > > You're right. > > When use EXC_LEVEL_EXCEPTION_PROLOG for Critical/Machine check, if the exception > came from kernel mode, we copy thread_info flags, *preempt*, and task pointer > from the process thread_info. So here I steal EXC_LEVEL_EXCEPTION_PROLOG for > Program Exception, preempt count would be corrupted incorrectly. Looks I miss the specific return-from-program-exc to restore those necessary thread information like we did for debug exception with ret_from_debug_exc when use EXC_LEVEL_EXCEPTION_PROLOG for debug exception. Will update this on v3. Tiejun > > Thanks > Tiejun > >> Ananth >> > > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev >
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c index bc47352..a8a2a4d 100644 --- a/arch/powerpc/kernel/kprobes.c +++ b/arch/powerpc/kernel/kprobes.c @@ -552,7 +552,6 @@ int __kprobes longjmp_break_handler(struct kprobe *p, struct pt_regs *regs) * saved regs... */ memcpy(regs, &kcb->jprobe_saved_regs, sizeof(struct pt_regs)); - preempt_enable_no_resched(); return 1; }
When enable CONFIG_PREEMPT we will trigger the following call trace: BUG: scheduling while atomic: swapper/1/0x10000000 ... krpobe always goes through the following path: program_check_exception() | + notify_die(DIE_BPT, "breakpoint",...) | + kprobe_handler() | + preempt_disable(); + break_handler() <- preempt_enable_no_resched() + emulate_step() + preempt_enable_no_resched() ... exit We should remove unnecessary preempt_enable_no_resched() inside of break_handler() since looks longjmp_break_handler() always go the above path. Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com> --- arch/powerpc/kernel/kprobes.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)