Re: [v2 PATCH 2/2] booke/kprobe: remove unnecessary preempt_enable_no_resched

[Ananth N Mavinakayanahalli <ananth@in.ibm.com>]

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