Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753091AbdLEJYg (ORCPT ); Tue, 5 Dec 2017 04:24:36 -0500 Received: from mail-pf0-f193.google.com ([209.85.192.193]:40596 "EHLO mail-pf0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753064AbdLEJYb (ORCPT ); Tue, 5 Dec 2017 04:24:31 -0500 X-Google-Smtp-Source: AGs4zMZvFtLyyVaIEYMvOOmmyA9dTGMsPBa03txqD84cB/DxWAW4kSVsNzAv0r+0SEc030wWUVOPXw== Date: Tue, 5 Dec 2017 17:24:25 +0800 From: Alan Kao To: Steven Rostedt Cc: Palmer Dabbelt , Albert Ou , patches@groups.riscv.org, Ingo Molnar , linux-kernel@vger.kernel.org, Alan Kao , Greentime Hu Subject: Re: [PATCH] riscv/ftrace: Add basic support Message-ID: <20171205092420.GA30557@d0b76efcf64c> References: <20171130085335.7949-1-alankao@andestech.com> <20171130152421.773fe6ec@gandalf.local.home> <20171204055228.GC30057@d0b76efcf64c> <20171204030509.1369baeb@vmware.local.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171204030509.1369baeb@vmware.local.home> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5736 Lines: 156 On Mon, Dec 04, 2017 at 03:05:09AM -0500, Steven Rostedt wrote: > On Mon, 4 Dec 2017 13:52:30 +0800 > Alan Kao 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