Re: [PATCH 1/2] MIPS: ftrace.h: Fix the MCOUNT_INSN_SIZE definition

[Steven Rostedt]

On Mon, 22 Sep 2014 09:55:09 -0700
David Daney <ddaney.cavm@xxxxxxxxx> wrote:

> On 09/22/2014 06:32 AM, Markos Chandras wrote:
> > The MCOUNT_INSN_SIZE is meant to be used to denote the overall
> > size of the mcount() call. Since a jal instruction is used to
> > call mcount() the delay slot should be taken into consideration
> > as well.
> > This also replaces the MCOUNT_INSN_SIZE usage with the real size
> > of a single MIPS instruction since, as described above, the
> > MCOUNT_INSN_SIZE is used to denote the total overhead of the
> > mcount() call.
> 
> Are you seeing errors with the existing code?  If so please state what 
> they are.
> 
> By changing this, we can no longer atomically replace the instruction. 
> So I think shouldn't be changing this stuff unless there is a real bug 
> we are fixing.

Actually, it looks like the code still works the same, as it uses the
old size of 4 (FTRACE_MIPS_INSN_SIZE) to do the update.

> 
> In conclusion: NAK unless the patch fixes a bug, in which case the 
> change log *must* state what the bug is, and how the patch addresses the 
> problem.
> 

I agree that the change log needs to explicitly state what is being
fixed. The only thing I can think of is if MIPS has kprobes, and
kprobes does different logic or may reject completely changes to ftrace
mcount call locations. This change will have kprobes flag the delay
slot as owned by ftrace.

It may also fix the stack tracer, as it searches for the ip saved in
the return address to find where the true stack is (skipping the stack
part that calls the strack tracer itself). If the link register holds
the location after the delay slot, then this would require
MCOUNT_INSN_SIZE to include the delay slot as well. Or I could add
another macro called MCOUNT_DELAY_SLOT_SIZE that can be defined by an
arch (and keep it zero for all other archs). That wouldn't be too much
of an issue to implement.

But again, the change log needs to express what is being fixed, which I
don't see.

I'm willing to update the generic code to express different ways of
implementation to keep the archs from doing hacks like this.

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/