Re: [PATCH 1/2] powerpc/kprobes: Remove redundant code

[Christophe Leroy]

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@xxxxxx>
> > ---
> > Â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