Re: [PATCH] riscv/ftrace: Add basic support

[Alan Kao]

On Mon, Dec 04, 2017 at 03:05:09AM -0500, Steven Rostedt wrote:
> On Mon, 4 Dec 2017 13:52:30 +0800
> Alan Kao <nonerkao@xxxxxxxxx> wrote:
> 
> > > > Note that the functions in both ftrace.c and setup.c should not be
> > > > hooked with the compiler's -pg option: to prevent infinite self-
> > > > referencing for the former, and to ignore early setup stuff for the latter.  
> > > 
> > > I'm curious to what is in setup.c that ftrace uses.  
> > 
> > In the scenario for some embedded systems, the __init prefix does not give 
> > us the notrace feature without the MODULE config.  Therefore, all functions 
> > would have been hooked with the _mcount trampoline if the -pg flag was not 
> > specifically disabled.
> 
> But is there functions you may want to trace. There's an effort going
> on to allow function tracing to start in early boot up.
> 

Fair enough, but no. Those (in setup.c) are very early stage functions. 
Unless Palmer has different opinion on this, making all of them notrace
should be ok.

> > 
> > And a terrible result would have happened after function setup_vm called
> > _mcount.  As _mcount compared the value of ftrace_trace_function and 
> > the position of ftrace_stub, it crashed the kernel because one of them 
> > was a physical address while the other was a virtual address but
> > actually they pointed to the same.
> > 
> > Adding notrace to setup_vm can solve the described issue, but it might be 
> > redundant once the MODULE config becomes stable and default on most 
> > platforms. To be honest, nobody really needs those init procedures to be
> > ftrace-able.
> 
> Um no, because MODULE init code can now be traced. It use to be that we
> didn't trace any __init, but I worked on having both inits be traced.
> The module code was a little bit trickier because it can be loaded
> multiple times and we needed to figure out the best way to handle init
> functions in the buffer that went stale and is replaced by other module
> init functions.
> 

Thanks for the explanation. But sorry for being unclear, I didn't mean
all the init procedures, but only those in setup.c.

>  
> > > > +config TRACE_IRQFLAGS_SUPPORT
> > > > +	def_bool y
> > > > +
> > > > +config LOCKDEP_SUPPORT
> > > > +	def_bool y  
> > > 
> > > Hmm, not sure the above is needed for function tracing.
> > >  
> > 
> > FTRACE depends on TRACING_SUPPORT, and TRACING_SUPPORT depends on
> > TRACE_IRQFLAGS_SUPPORT. But LOCKDEP_SUPPORT is not actually needed
> > for any of the ftrace features implemented in this patch.
> 
> Hmm, I think that's stale. Thanks for bringing that to my attention,
> and don't believe that dependency still exists.
> 
> > 
> > The LOCKDEP_SUPPORT will be removed in the next version.
> >
> 
> I should have also asked, is lockdep really supported on this arch, and
> is IRQSFLAGS really supported too? I vaguely remember making ftrace
> depend on IRQFLAGS because we wanted archs to support TRACE_IRQFLAGS
> before they supported ftrace. Maybe I'll keep that dependency.

We do have the implementation of IRQFLAGS in
arch/riscv/include/asm/irqflags.  But I'm not sure about LOCKDEP.
 
> > > > +ENTRY(_mcount)
> > > > +	la	t4, ftrace_stub
> > > > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > > > +	la	t0, ftrace_graph_return
> > > > +	ld	t1, 0(t0)
> > > > +	bne	t1, t4, do_ftrace_graph_caller  
> > > 
> > > If function graph is enabled, you jump straight to the graph tracer,
> > > but never return back to here?
> > >  
> > 
> > Because prepare_ftrace_return function can return to the caller of
> > _mcount directly without messing up the stack.
> 
> Yes, is that required?
>

I don't get your point here. Are you suggesting that a call is better than 
a jump here, for future extension towards dynamic tracing support? 

> > 
> > > > +
> > > > +	la	t3, ftrace_graph_entry
> > > > +	ld	t2, 0(t3)
> > > > +	la	t6, ftrace_graph_entry_stub
> > > > +	bne	t2, t6, do_ftrace_graph_caller
> > > > +#endif
> > > > +	la	t3, ftrace_trace_function
> > > > +	ld	t5, 0(t3)
> > > > +	bne	t5, t4, do_trace
> > > > +	ret
> > > > +
> > > > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > > > +/*
> > > > + * A pseudo representation for the function graph tracer:
> > > > + * prepare_to_return(&ra_to_caller_of_caller, ra_to_caller)
> > > > + */
> > > > +do_ftrace_graph_caller:
> > > > +	addi	a0, s0, -8
> > > > +	mv	a1, ra
> > > > +#ifdef HAVE_FUNCTION_GRAPH_FP_TEST
> > > > +	ld	a2, -16(s0)
> > > > +#endif
> > > > +	SAVE_ABI_STATE
> > > > +	la	t0, prepare_ftrace_return
> > > > +	jalr	t0
> > > > +	STORE_ABI_STATE  
> > > 
> > > I'm guessing you don't support function tracer and function graph
> > > tracer running at the same time?
> > > 
> > > -- Steve
> > >   
> > 
> > This code section implements similar logic as those for arm, arm64,
> > blackfin, and others.  Also, according to Documentation/trace/ftrace.txt,
> > the current_tracer is introduced as singular.
> > 
> > Is it necessary to support simultaneous tracers?
> 
> Well, you can do things like have multiple buffers today (different
> tracers recording in different buffers). We can have function tracing
> happening at the same time as the graph tracer.
> 
> Is this a requirement? No. Just letting you know.
> 
> While you only support static ftrace, and not dynamic (code modifying)
> ftrace, this isn't yet an issue. I'm just trying to let you know of
> some of the current features that are supported in other archs, in case
> you extend this.
> 
> -- Steve
> 
Thanks for the information. I am now working on the dynamic part.
Knowing that the current documents are outdated, I will reference the
implementations of other architectures much more carefully.

If no other comments on this patch, the v2 of this will be ready soon.

Many thanks,
Alan