On 09/22/2014 07:25 PM, Steven Rostedt wrote: > 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. Indeed I haven't seen any functional change when it comes to replacing the instruction. > [...] > > 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. Yes, this is the only case I spotted as well. Perhaps I should put that in the changelog. 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. If you want to fix that in the generic code then I am fine with it. -- markos
