Re: [PATCH v3] ftrace: add a tracepoint for __raise_softirq_irqoff()

[Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>]

* Lai Jiangshan (laijs@cn.fujitsu.com) wrote:
> Mathieu Desnoyers wrote:
> > * Lai Jiangshan (laijs@cn.fujitsu.com) wrote:
> >> Mathieu Desnoyers wrote:
> >>> I partially agree with you :
> >>>
> >>> Yes, we should try to fix TRACE_EVENT, but we should fix it _before_ we
> >>> start using it widely. Circular header dependencies is a real problem
> >>> with TRACE_EVENT right now.
> >>>
> >>> Until we fix this, I will be tempted to stay with a known-good solution,
> >>> which is DECLARE/DEFINE_TRACE.
> >>>
> >>>
> >> I partially agree with you:
> >>
> >> Yes, Circular header dependencies is a real problem with TRACE_EVENT
> >> right now. It is also a problem with DECLARE_TRACE. It's a stubborn
> >> disease with C-Language (for complex headers). Can we fix C-Language?
> >>
> >> o Macros in header (!CREATE_TRACE_POINTS)
> >>
> >> When CREATE_TRACE_POINTS is not defined, TRACE_EVENT is definitely
> >> the same as DECLARE_TRACE. Actually, TRACE_EVENT is:
> >>
> >> #define TRACE_EVENT(name, proto, args, struct, assign, print)	\
> >> 	DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
> >>
> >> So TRACE_EVENT and DECLARE_TRACE are the same in header files.
> >> And so TRACE_EVENT and DECLARE_TRACE have the same advantages and
> >> disadvantages. More TRACE_EVENT equals to a known-good solution.
> >>
> >> o Macros in c-file
> >>
> >> tracepoint uses DEFINE_TRACE only.
> >>
> >> ftrace uses CREATE_TRACE_POINTS + TRACE_EVENT:
> >> 	#define CREATE_TRACE_POINTS
> >> 	#include <trace/events/sched.h> (which uses TRACE_EVENT)
> >>
> >> ftrace generates more code which uses the tracepoints.
> >>
> >>> Then add a forward declaration of
> >>>
> >>> struct softirqaction;
> >>>
> >>> At the top of trace/irq.h. I did it in quite a few places in the LTTng
> >>> tree. TP_PROTO just needs a forward declaration, not the full structure
> >>> declaration.
> >>>
> >> Thank you for your valuable suggestions.
> >>
> >> You are the father of tracepoint and LTTng, your experience in
> >> LTTng is very useful for ftrace.
> >>
> >> I'm glad for your suggestions.
> >>
> >>
> >> Xiao Guangrong, could you add forward declarations of
> >>
> >> struct irqaction;
> >> struct softirq_action;
> >>
> >> at the top of trace/irq.h as Mathieu's suggestions.
> >> (and remove "#include <linux/interrupt.h>")
> >>
> > 
> > You will probably still need something like :
> > 
> > #ifdef CREATE_TRACE_POINTS
> > #include <linux/interrupt.h>
> > #else
> > struct irqaction;
> > struct softirq_action;
> > #endif
> > 
> 
> It's not needed for trace/events/irq.h
> 
> Yes, it's a solution.
> 
> But I don't think we have to do this, CREATE_TRACE_POINTS is
> needed only _once_ for every <trace/events/xxxx.h>
> 
> The .c file which defines CREATE_TRACE_POINTS can provide
> (had provided likely) things like "#include <linux/interrupt.h>"
> 
> See kernel/softirq.c:
> #include <linux/interrupt.h>
> ......
> ......
> #define CREATE_TRACE_POINTS
> #include <trace/events/irq.h>
> 

Isn't it all included under kernel/trace/events.c ? e.g. :

#include <trace/trace_events.h>

#include "trace_output.h"

#include "trace_events_stage_1.h"
#include "trace_events_stage_2.h"
#include "trace_events_stage_3.h"

Therefore, adding a #include <linux/interrupt.h> would be just weird
here. We would have to do that for every tracepoint headers, and we
would just lose the ability to keep track of which tracepoint header has
which include dependency.

I thought the goal of TRACE_EVENT() was exactly the opposite : to
declare everything in one location....

Therefore, if you agree that it's good to have TRACE_EVENT() declaring
everything in one location, then I don't see why you would argue to move
the include dependencies in the very remote kernel/trace/events.c file ?

Mathieu

> I don't think it's a problem, CREATE_TRACE_POINTS is defined only
> once for a <trace/events/xxxx.h>.
> 
> Lai.
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68