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

[Lai Jiangshan <laijs@cn.fujitsu.com>]

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>

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

Lai.