Messages in this thread | | | Date | Tue, 12 May 2009 09:14:36 -0400 | From | Mathieu Desnoyers <> | Subject | Re: [PATCH v3] ftrace: add a tracepoint for __raise_softirq_irqoff() |
| |
* Xiao Guangrong (xiaoguangrong@cn.fujitsu.com) wrote: > > > Mathieu Desnoyers wrote: > > * Steven Rostedt (rostedt@goodmis.org) wrote: > >> > >> > >> On Mon, 11 May 2009, Mathieu Desnoyers wrote: > >> > >>> * Steven Rostedt (rostedt@goodmis.org) wrote: > >>>> On Mon, 11 May 2009, Mathieu Desnoyers wrote: > >>>>> 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. > > > Yes, but this would solve most of the include dependency problems at > > once. We would only have dependency on preempt.h left, which alone is > > easier to deal with. And if headers don't need to include any annoying > > headers, they don't need to use the ifdef BUILDING_EVENTS. But my point > > is that when it's needed (and we already see two cases where it's > > needed, with pvops instrumentation and softirq instrumentation, and I > > guess we'll see much more), it's good to have this infrastructure in > > place, so people don't end up trying to do hacks to the kernel code > > changing inlines for function calls to try to deal with the circular > > include issue. > > > > I think it's > > > > a) needed > > b) it does not hurt anyone who does not need it. > > > > So I would definitely recommend adding such define. > > > > Sorry for my poor English. > Problem is not only in TP_printk, but also in TP_PROTO if we add a > tracer in header file. > So, it can't be solved only by "get rid of including ftrace parts" > > Take v2 patch for example: > (it is at: http://marc.info/?l=linux-kernel&m=124081169727739&w=2) > > In order to trace __raise_softirq_irqoff(), we should add > a trace function in __raise_softirq_irqoff() and include > <include/trace/irq.h> in interrupte.h. > If we put <include/trace/irq.h> on top of linux/irq.h, > we will see a warning of "struct softirq_action declared > inside parameter list" in compile. It is because struct irqaction's > definition is bypasswd before TP_PROTO. >
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.
Only the ftrace-specific parts will need the #include <linux/irq.h>, which we should put in a #ifdef :
sched.c: #include linux/interrupt.h | |-->include <trace/irq.h> #ifdef BUILDING_EVENTS /* * Includes needed by TP_fast_assign, TP_printk, and * TP_STRUCT__entry : */ # include <linux/interrupt.h> #else /* Forward declarations for TP_PROTO */ struct softirq_action; #endif
TRACE_EVENT(......); |-->TP_PROTO(int irq, struct irqaction *action), (but struct softirq_action is not declared before, it raise a compile warning:struct softirq_action declared inside parameter list. |-> ......
Mathieu
> > I can't see the solvent for this in your discuss or I understand wrong? > > Xiao > > > Mathieu > > > >> -- Steve > >> > >>> I think it should work, but it looks a bit too simple, so I may have > >>> missed something... ? > >>> > >>> Mathieu > >>> > >>> > >>> -- > >>> Mathieu Desnoyers > >>> OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 > >>> > >
-- Mathieu Desnoyers OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
| |