linux-mips
[Top] [All Lists]

Re: [PATCH 5/5] tracing, MIPS: Fix set_graph_function of function graph

To: Sergei Shtylyov <sshtylyov@mvista.com>
Subject: Re: [PATCH 5/5] tracing, MIPS: Fix set_graph_function of function graph tracer
From: Steven Rostedt <srostedt@redhat.com>
Date: Thu, 20 Jan 2011 09:04:38 -0500
Cc: Wu Zhangjin <wuzhangjin@gmail.com>, Ralf Baechle <ralf@linux-mips.org>, linux-mips@linux-mips.org
In-reply-to: <4D381677.3000502@mvista.com>
Organization: Red Hat
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <cover.1295464855.git.wuzhangjin@gmail.com> <9967898043e58db7b311d35695e9422e67cef5f6.1295464855.git.wuzhangjin@gmail.com> <4D381677.3000502@mvista.com>
Sender: linux-mips-bounce@linux-mips.org
On Thu, 2011-01-20 at 14:03 +0300, Sergei Shtylyov wrote:

> > diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
> > index bc91e4a..62775d7 100644
> > --- a/arch/mips/kernel/ftrace.c
> > +++ b/arch/mips/kernel/ftrace.c
> [...]
> > @@ -304,7 +304,14 @@ void prepare_ftrace_return(unsigned long 
> > *parent_ra_addr, unsigned long self_ra,
> >             return;
> >     }
> >
> > -   trace.func = self_ra;
> > +   /*
> > +    * Get the recorded ip of the current mcount calling site in the
> > +    * __mcount_loc section, which will be used to filter the function
> > +    * entries configured through the tracing/set_graph_function interface.
> > +    */
> > +
> > +   insns = (in_kernel_space(self_ra)) ? 2 : (MCOUNT_OFFSET_INSNS + 1);
> 
>     Unneeded parens.

agreed

> 
> > +   trace.func = self_ra - (MCOUNT_INSN_SIZE * insns);
> 
>     Here too.

I like the parens here. Yes, it is basic math precedence, but it stands
out to a reviewer who has their brain more focused on correct code than
thinking about which op has precedence.

Reviewing code that has:

        trace.func = self_ra - MCOUNT_INSN_SIZE * insns;

And as I my mother language reads left to right, my thought process
goes: subtract MCOUNT_INSN_SIZE from self_ra and then times insns... oh
wait, times goes first; stop reset, restart... subtract MCOUNT_INSN_SIZE
times insns from self_ra.  That stop reset and restart of the thought
process breaks the train of thought and could waste a lot more time than
just the moment it happened. All this is avoided by the parenthesis that
automatically trigger the brain to think those go first.

I say, leave these in.

-- Steve



<Prev in Thread] Current Thread [Next in Thread>