Patchwork [v2,2/2] booke/kprobe: remove unnecessary preempt_enable_no_resched

login
register
mail settings
Submitter Tiejun Chen
Date July 11, 2011, 2:39 a.m.
Message ID <1310351976-24078-2-git-send-email-tiejun.chen@windriver.com>
Download mbox | patch
Permalink /patch/104150/
State Superseded
Headers show

Comments

Tiejun Chen - July 11, 2011, 2:39 a.m.
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(-)
Tiejun Chen - July 11, 2011, 2:39 a.m.
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
Ananth N Mavinakayanahalli - July 11, 2011, 5:55 a.m.
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
Tiejun Chen - July 11, 2011, 8:34 a.m.
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 - July 11, 2011, 11:28 a.m.
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
>

Patch

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;
 }