| Message ID | 642c8b4ca59e658be38d8dde00f994e183790a6a.1581687838.git.christophe.leroy@c-s.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] powerpc/kprobes: Remove redundant code | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (a5bc6e124219546a81ce334dc9b16483d55e9abf) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 31 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
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 > >
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
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) { /*
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(-)