* [PATCH 1/2] powerpc/kprobes: Remove redundant code
@ 2020-02-14 13:44 Christophe Leroy
2020-02-14 13:44 ` [PATCH 2/2] powerpc/kprobes: Reduce depth of a test Christophe Leroy
2020-02-18 14:39 ` [PATCH 1/2] powerpc/kprobes: Remove redundant code Naveen N. Rao
0 siblings, 2 replies; 5+ messages in thread
From: Christophe Leroy @ 2020-02-14 13:44 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
At the time being we have something like
if (something) {
p = get();
if (p) {
if (something_wrong)
goto out;
...
return;
} else if (a != b) {
if (some_error)
goto out;
...
}
goto out;
}
p = get();
if (!p) {
if (a != b) {
if (some_error)
goto out;
...
}
goto out;
}
This is similar to
p = get();
if (something) {
if (p) {
if (something_wrong)
goto out;
...
return;
}
}
if (!p) {
if (a != b) {
if (some_error)
goto out;
...
}
goto out;
}
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/kernel/kprobes.c | 15 +--------------
1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index f8b848aa65bd..7a925eb76ec0 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -276,8 +276,8 @@ int kprobe_handler(struct pt_regs *regs)
kcb = get_kprobe_ctlblk();
/* Check we're not actually recursing */
+ p = get_kprobe(addr);
if (kprobe_running()) {
- p = get_kprobe(addr);
if (p) {
kprobe_opcode_t insn = *p->ainsn.insn;
if (kcb->kprobe_status == KPROBE_HIT_SS &&
@@ -308,22 +308,9 @@ int kprobe_handler(struct pt_regs *regs)
}
prepare_singlestep(p, regs);
return 1;
- } else if (*addr != BREAKPOINT_INSTRUCTION) {
- /* If trap variant, then it belongs not to us */
- kprobe_opcode_t cur_insn = *addr;
-
- if (is_trap(cur_insn))
- goto no_kprobe;
- /* The breakpoint instruction was removed by
- * another cpu right after we hit, no further
- * handling of this interrupt is appropriate
- */
- ret = 1;
}
- goto no_kprobe;
}
- p = get_kprobe(addr);
if (!p) {
if (*addr != BREAKPOINT_INSTRUCTION) {
/*
--
2.25.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] powerpc/kprobes: Reduce depth of a test
2020-02-14 13:44 [PATCH 1/2] powerpc/kprobes: Remove redundant code Christophe Leroy
@ 2020-02-14 13:44 ` Christophe Leroy
2020-02-18 14:40 ` Naveen N. Rao
2020-02-18 14:39 ` [PATCH 1/2] powerpc/kprobes: Remove redundant code Naveen N. Rao
1 sibling, 1 reply; 5+ messages in thread
From: Christophe Leroy @ 2020-02-14 13:44 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
if (a) {
if (b)
do_something();
}
Is equivalent to
if (a & b)
do_something();
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/kernel/kprobes.c | 58 +++++++++++++++++------------------
1 file changed, 28 insertions(+), 30 deletions(-)
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 7a925eb76ec0..d7c80a078c1e 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -277,38 +277,36 @@ int kprobe_handler(struct pt_regs *regs)
/* Check we're not actually recursing */
p = get_kprobe(addr);
- if (kprobe_running()) {
- if (p) {
- kprobe_opcode_t insn = *p->ainsn.insn;
- if (kcb->kprobe_status == KPROBE_HIT_SS &&
- is_trap(insn)) {
- /* Turn off 'trace' bits */
- regs->msr &= ~MSR_SINGLESTEP;
- regs->msr |= kcb->kprobe_saved_msr;
- goto no_kprobe;
- }
- /* We have reentered the kprobe_handler(), since
- * another probe was hit while within the handler.
- * We here save the original kprobes variables and
- * just single step on the instruction of the new probe
- * without calling any user handlers.
- */
- save_previous_kprobe(kcb);
- set_current_kprobe(p, regs, kcb);
- kprobes_inc_nmissed_count(p);
- kcb->kprobe_status = KPROBE_REENTER;
- if (p->ainsn.boostable >= 0) {
- ret = try_to_emulate(p, regs);
-
- if (ret > 0) {
- restore_previous_kprobe(kcb);
- preempt_enable_no_resched();
- return 1;
- }
+ if (kprobe_running() && p) {
+ kprobe_opcode_t insn = *p->ainsn.insn;
+
+ if (kcb->kprobe_status == KPROBE_HIT_SS && is_trap(insn)) {
+ /* Turn off 'trace' bits */
+ regs->msr &= ~MSR_SINGLESTEP;
+ regs->msr |= kcb->kprobe_saved_msr;
+ goto no_kprobe;
+ }
+ /* We have reentered the kprobe_handler(), since
+ * another probe was hit while within the handler.
+ * We here save the original kprobes variables and
+ * just single step on the instruction of the new probe
+ * without calling any user handlers.
+ */
+ save_previous_kprobe(kcb);
+ set_current_kprobe(p, regs, kcb);
+ kprobes_inc_nmissed_count(p);
+ kcb->kprobe_status = KPROBE_REENTER;
+ if (p->ainsn.boostable >= 0) {
+ ret = try_to_emulate(p, regs);
+
+ if (ret > 0) {
+ restore_previous_kprobe(kcb);
+ preempt_enable_no_resched();
+ return 1;
}
- prepare_singlestep(p, regs);
- return 1;
}
+ prepare_singlestep(p, regs);
+ return 1;
}
if (!p) {
--
2.25.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] powerpc/kprobes: Remove redundant code
2020-02-14 13:44 [PATCH 1/2] powerpc/kprobes: Remove redundant code Christophe Leroy
2020-02-14 13:44 ` [PATCH 2/2] powerpc/kprobes: Reduce depth of a test Christophe Leroy
@ 2020-02-18 14:39 ` Naveen N. Rao
2020-02-19 7:48 ` Christophe Leroy
1 sibling, 1 reply; 5+ messages in thread
From: Naveen N. Rao @ 2020-02-18 14:39 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
Christophe Leroy wrote:
> At the time being we have something like
>
> if (something) {
> p = get();
> if (p) {
> if (something_wrong)
> goto out;
> ...
> return;
> } else if (a != b) {
> if (some_error)
> goto out;
> ...
> }
> goto out;
> }
> p = get();
> if (!p) {
> if (a != b) {
> if (some_error)
> goto out;
> ...
> }
> goto out;
> }
>
> This is similar to
>
> p = get();
> if (something) {
> if (p) {
> if (something_wrong)
> goto out;
> ...
> return;
> }
> }
> if (!p) {
> if (a != b) {
> if (some_error)
> goto out;
> ...
> }
> goto out;
> }
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/kernel/kprobes.c | 15 +--------------
> 1 file changed, 1 insertion(+), 14 deletions(-)
Good cleanup, thanks.
>
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index f8b848aa65bd..7a925eb76ec0 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -276,8 +276,8 @@ int kprobe_handler(struct pt_regs *regs)
> kcb = get_kprobe_ctlblk();
>
> /* Check we're not actually recursing */
> + p = get_kprobe(addr);
> if (kprobe_running()) {
> - p = get_kprobe(addr);
> if (p) {
> kprobe_opcode_t insn = *p->ainsn.insn;
> if (kcb->kprobe_status == KPROBE_HIT_SS &&
> @@ -308,22 +308,9 @@ int kprobe_handler(struct pt_regs *regs)
> }
> prepare_singlestep(p, regs);
> return 1;
> - } else if (*addr != BREAKPOINT_INSTRUCTION) {
> - /* If trap variant, then it belongs not to us */
> - kprobe_opcode_t cur_insn = *addr;
> -
> - if (is_trap(cur_insn))
> - goto no_kprobe;
> - /* The breakpoint instruction was removed by
> - * another cpu right after we hit, no further
> - * handling of this interrupt is appropriate
> - */
> - ret = 1;
> }
> - goto no_kprobe;
A minot nit -- removing the above goto makes a slight change to the
logic. But, see my comments for the next patch.
- Naveen
> }
>
> - p = get_kprobe(addr);
> if (!p) {
> if (*addr != BREAKPOINT_INSTRUCTION) {
> /*
> --
> 2.25.0
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] powerpc/kprobes: Reduce depth of a test
2020-02-14 13:44 ` [PATCH 2/2] powerpc/kprobes: Reduce depth of a test Christophe Leroy
@ 2020-02-18 14:40 ` Naveen N. Rao
0 siblings, 0 replies; 5+ messages in thread
From: Naveen N. Rao @ 2020-02-18 14:40 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
Christophe Leroy wrote:
> if (a) {
> if (b)
> do_something();
> }
>
> Is equivalent to
>
> if (a & b)
> do_something();
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/kernel/kprobes.c | 58 +++++++++++++++++------------------
> 1 file changed, 28 insertions(+), 30 deletions(-)
>
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 7a925eb76ec0..d7c80a078c1e 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -277,38 +277,36 @@ int kprobe_handler(struct pt_regs *regs)
>
> /* Check we're not actually recursing */
> p = get_kprobe(addr);
> - if (kprobe_running()) {
> - if (p) {
> - kprobe_opcode_t insn = *p->ainsn.insn;
> - if (kcb->kprobe_status == KPROBE_HIT_SS &&
> - is_trap(insn)) {
> - /* Turn off 'trace' bits */
> - regs->msr &= ~MSR_SINGLESTEP;
> - regs->msr |= kcb->kprobe_saved_msr;
> - goto no_kprobe;
> - }
> - /* We have reentered the kprobe_handler(), since
> - * another probe was hit while within the handler.
> - * We here save the original kprobes variables and
> - * just single step on the instruction of the new probe
> - * without calling any user handlers.
> - */
> - save_previous_kprobe(kcb);
> - set_current_kprobe(p, regs, kcb);
> - kprobes_inc_nmissed_count(p);
> - kcb->kprobe_status = KPROBE_REENTER;
> - if (p->ainsn.boostable >= 0) {
> - ret = try_to_emulate(p, regs);
> -
> - if (ret > 0) {
> - restore_previous_kprobe(kcb);
> - preempt_enable_no_resched();
> - return 1;
> - }
> + if (kprobe_running() && p) {
> + kprobe_opcode_t insn = *p->ainsn.insn;
> +
> + if (kcb->kprobe_status == KPROBE_HIT_SS && is_trap(insn)) {
> + /* Turn off 'trace' bits */
> + regs->msr &= ~MSR_SINGLESTEP;
> + regs->msr |= kcb->kprobe_saved_msr;
> + goto no_kprobe;
> + }
> + /* We have reentered the kprobe_handler(), since
> + * another probe was hit while within the handler.
> + * We here save the original kprobes variables and
> + * just single step on the instruction of the new probe
> + * without calling any user handlers.
> + */
> + save_previous_kprobe(kcb);
> + set_current_kprobe(p, regs, kcb);
> + kprobes_inc_nmissed_count(p);
> + kcb->kprobe_status = KPROBE_REENTER;
> + if (p->ainsn.boostable >= 0) {
> + ret = try_to_emulate(p, regs);
> +
> + if (ret > 0) {
> + restore_previous_kprobe(kcb);
> + preempt_enable_no_resched();
> + return 1;
> }
> - prepare_singlestep(p, regs);
> - return 1;
> }
> + prepare_singlestep(p, regs);
> + return 1;
> }
>
If we move the below !p case before the check for kprobe_running() right
after get_kprobe(), we won't need to check for (p) above and we won't
have any change in logic from Patch 1.
> if (!p) {
- Naveen
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] powerpc/kprobes: Remove redundant code
2020-02-18 14:39 ` [PATCH 1/2] powerpc/kprobes: Remove redundant code Naveen N. Rao
@ 2020-02-19 7:48 ` Christophe Leroy
0 siblings, 0 replies; 5+ messages in thread
From: Christophe Leroy @ 2020-02-19 7:48 UTC (permalink / raw)
To: Naveen N. Rao, Benjamin Herrenschmidt, Michael Ellerman, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
Le 18/02/2020 à 15:39, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>> At the time being we have something like
>>
>> if (something) {
>> p = get();
>> if (p) {
>> if (something_wrong)
>> goto out;
>> ...
>> return;
>> } else if (a != b) {
>> if (some_error)
>> goto out;
>> ...
>> }
>> goto out;
>> }
>> p = get();
>> if (!p) {
>> if (a != b) {
>> if (some_error)
>> goto out;
>> ...
>> }
>> goto out;
>> }
>>
>> This is similar to
>>
>> p = get();
>> if (something) {
>> if (p) {
>> if (something_wrong)
>> goto out;
>> ...
>> return;
>> }
>> }
>> if (!p) {
>> if (a != b) {
>> if (some_error)
>> goto out;
>> ...
>> }
>> goto out;
>> }
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>> arch/powerpc/kernel/kprobes.c | 15 +--------------
>> 1 file changed, 1 insertion(+), 14 deletions(-)
>
> Good cleanup, thanks.
>
>>
>> diff --git a/arch/powerpc/kernel/kprobes.c
>> b/arch/powerpc/kernel/kprobes.c
>> index f8b848aa65bd..7a925eb76ec0 100644
>> --- a/arch/powerpc/kernel/kprobes.c
>> +++ b/arch/powerpc/kernel/kprobes.c
>> @@ -276,8 +276,8 @@ int kprobe_handler(struct pt_regs *regs)
>> kcb = get_kprobe_ctlblk();
>>
>> /* Check we're not actually recursing */
>> + p = get_kprobe(addr);
>> if (kprobe_running()) {
>> - p = get_kprobe(addr);
>> if (p) {
>> kprobe_opcode_t insn = *p->ainsn.insn;
>> if (kcb->kprobe_status == KPROBE_HIT_SS &&
>> @@ -308,22 +308,9 @@ int kprobe_handler(struct pt_regs *regs)
>> }
>> prepare_singlestep(p, regs);
>> return 1;
>> - } else if (*addr != BREAKPOINT_INSTRUCTION) {
>> - /* If trap variant, then it belongs not to us */
>> - kprobe_opcode_t cur_insn = *addr;
>> -
>> - if (is_trap(cur_insn))
>> - goto no_kprobe;
>> - /* The breakpoint instruction was removed by
>> - * another cpu right after we hit, no further
>> - * handling of this interrupt is appropriate
>> - */
>> - ret = 1;
>> }
>> - goto no_kprobe;
>
> A minot nit -- removing the above goto makes a slight change to the
> logic. But, see my comments for the next patch.
All legs of the (p) case are have either a return or a goto, so that
goto no_kprobe is limited to the !p case, we have to fall_through now.
Christophe
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-02-19 7:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-14 13:44 [PATCH 1/2] powerpc/kprobes: Remove redundant code Christophe Leroy
2020-02-14 13:44 ` [PATCH 2/2] powerpc/kprobes: Reduce depth of a test Christophe Leroy
2020-02-18 14:40 ` Naveen N. Rao
2020-02-18 14:39 ` [PATCH 1/2] powerpc/kprobes: Remove redundant code Naveen N. Rao
2020-02-19 7:48 ` Christophe Leroy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).