Hello Harvey,
Thank you for your great works!
Harvey Harrison wrote:
> Make the control flow of kprobe_handler more obvious.
>
> Collapse the separate if blocks/gotos with if/else blocks
> this unifies the duplication of the check for a breakpoint
> instruction race with another cpu.
I agree it is good to unify the duplications of
breakpoint checking and get_kprobe() calling.
>
> Create two jump targets:
> preempt_out: re-enables preemption before returning ret
> out: only returns ret
However, I'm not sure we should change "no_kprobe".
That label is commonly used in arch/*/kernel/kprobes.c.
And also, I prefer "return 1" to "{ret = 1; goto out;}"
for simplicity.
Or, how about initializing "ret" as 1 instead of 0?
Ananth, Jim,
I'd like to hear your comments on it.
> Signed-off-by: Harvey Harrison <[EMAIL PROTECTED]>
> ---
> Masami, noticed a small bug in the previous version in the !p
> case when the breakpoint was the kernel's. Please review this
> version.
>
> arch/x86/kernel/kprobes.c | 60
> +++++++++++++++++++++------------------------
> 1 files changed, 28 insertions(+), 32 deletions(-)
>
> diff --git a/arch/x86/kernel/kprobes.c b/arch/x86/kernel/kprobes.c
> index 4e33329..f8c7ac1 100644
> --- a/arch/x86/kernel/kprobes.c
> +++ b/arch/x86/kernel/kprobes.c
> @@ -480,32 +480,28 @@ static int __kprobes kprobe_handler(struct pt_regs
> *regs)
> preempt_disable();
> kcb = get_kprobe_ctlblk();
>
> - /* Check we're not actually recursing */
> - if (kprobe_running()) {
> - p = get_kprobe(addr);
> - if (p) {
> + p = get_kprobe(addr);
> + if (p) {
> + /* Check we're not actually recursing */
> + if (kprobe_running()) {
> ret = reenter_kprobe(p, regs, kcb);
> if (kcb->kprobe_status == KPROBE_REENTER)
> - return 1;
> + {
> + ret = 1;
> + goto out;
I think "return 1" is better.
> + }
> + goto preempt_out;
> } else {
> - if (*addr != BREAKPOINT_INSTRUCTION) {
> - /* The breakpoint instruction was removed by
> - * another cpu right after we hit, no further
> - * handling of this interrupt is appropriate
> - */
> - regs->ip = (unsigned long)addr;
> + set_current_kprobe(p, regs, kcb);
> + kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> + if (p->pre_handler && p->pre_handler(p, regs))
> + {
> + /* handler set things up, skip ss setup */
> ret = 1;
> - goto no_kprobe;
> + goto out;
Ditto.
> }
> - p = __get_cpu_var(current_kprobe);
> - if (p->break_handler && p->break_handler(p, regs))
> - goto ss_probe;
> }
> - goto no_kprobe;
> - }
> -
> - p = get_kprobe(addr);
> - if (!p) {
> + } else {
I think you'd better move "!p" block forward, because
this block means "relatively rare" cases. (sure, I know jprobe uses this block.)
> if (*addr != BREAKPOINT_INSTRUCTION) {
> /*
> * The breakpoint instruction was removed right
> @@ -518,34 +514,34 @@ static int __kprobes kprobe_handler(struct pt_regs
> *regs)
> */
> regs->ip = (unsigned long)addr;
> ret = 1;
> + goto preempt_out;
> + }
> + if (kprobe_running()) {
> + p = __get_cpu_var(current_kprobe);
> + if (p->break_handler && p->break_handler(p, regs))
> + goto ss_probe;
> }
> /* Not one of ours: let kernel handle it */
> - goto no_kprobe;
> + goto preempt_out;
> }
>
> - set_current_kprobe(p, regs, kcb);
> - kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> -
> - if (p->pre_handler && p->pre_handler(p, regs))
> - /* handler has already set things up, so skip ss setup */
> - return 1;
> -
> ss_probe:
> + ret = 1;
> #if !defined(CONFIG_PREEMPT) || defined(CONFIG_PM)
> if (p->ainsn.boostable == 1 && !p->post_handler) {
> /* Boost up -- we can execute copied instructions directly */
> reset_current_kprobe();
> regs->ip = (unsigned long)p->ainsn.insn;
> - preempt_enable_no_resched();
> - return 1;
> + goto preempt_out;
> }
> #endif
> prepare_singlestep(p, regs);
> kcb->kprobe_status = KPROBE_HIT_SS;
> - return 1;
> + goto out;
I think "return 1" is better.
>
> -no_kprobe:
> +preempt_out:
> preempt_enable_no_resched();
> +out:
> return ret;
> }
>
--
Masami Hiramatsu
Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division
e-mail: [EMAIL PROTECTED], [EMAIL PROTECTED]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
