Ingo,
This is the long awaited TRACE_EVENT for modules patch series.
Not only does it allow for modules to use the TRACE_EVENT infrastructure,
but it also cleans up the way TRACE_EVENTS are used in core kernel code.
Some of the clean ups are:
Removal of the two headers per trace system. No need to have
include/trace/sched.h and include/linux/sched_event_types.h
All the changes go into include/trace/sched.h. But note that how that
file is made is important. One could look at the sched.h file, or
skb.h, lockdep.h and kmem.h as an example.
Another clean up is that I got rid of the need to add these files
into include/trace/trace_events.h and include/trace/trace_event_types.h.
Those files have been deleted.
Another clean up is that we do not need to do the DEFINE_TRACE(name)
for every TRACE_EVENT (or TRACE_FORMAT and DECLARE_TRACE) in the
include/trace/ header. One only needs to define a CREATE_TRACE_POINTS
in one C file to do the work for them:
#define CREATE_TRACE_POINTS
#inlude <trace/sched.h>
That will do the DEFINE_TRACE for every defined trace item in sched.h.
I also removed the trace_events_stage_X.h files and combined them
into a include/trace/ftrace.h file.
I made sure that each stage did not break the code. Well, I tested
kmem at each level. kmem seems to be the most complex of the trace events.
At the end I made a module and tested it out as well. I even removed
the module as the trace was running. Note, if you remove the file
and view the trace, you will get something like:
<...>-4197 [002] 205.524992: Unknown type 47
<...>-4197 [002] 206.523325: Unknown type 47
<...>-4197 [002] 207.521665: Unknown type 47
<...>-4197 [002] 208.520118: Unknown type 47
The unknown type will appear. This is because the format to print the
string is in the module itself. When the code is removed, ftrace has
no way of knowing how to print that string out. But if you had a user
space tool that read the format file first, it could still parse the binary
data just fine.
Please pull the latest tip/tracing/core tree, which can be found at:
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
tip/tracing/core
Steven Rostedt (8):
tracing: consolidate trace and trace_event headers
tracing: create automated trace defines
tracing: make trace_seq operations available for core kernel
tracing/events: move declarations from trace directory to core include
tracing/events: move the ftrace event tracing code to core
tracing/events: convert event call sites to use a link list
tracing/events: add export symbols for trace events in modules
tracing/events: add support for modules to TRACE_EVENT
----
include/linux/ftrace_event.h | 150 +++++++++++
include/linux/module.h | 4 +
include/linux/trace_seq.h | 91 +++++++
include/linux/tracepoint.h | 9 +-
include/trace/define_trace.h | 79 ++++++
include/trace/ftrace.h | 493 +++++++++++++++++++++++++++++++++++
include/trace/irq.h | 56 ++++-
include/trace/irq_event_types.h | 55 ----
include/trace/kmem.h | 189 +++++++++++++-
include/trace/lockdep.h | 55 ++++-
include/trace/lockdep_event_types.h | 57 ----
include/trace/sched.h | 336 +++++++++++++++++++++++-
include/trace/sched_event_types.h | 337 ------------------------
include/trace/skb.h | 39 +++-
include/trace/skb_event_types.h | 38 ---
include/trace/trace_event_types.h | 7 -
include/trace/trace_events.h | 7 -
kernel/exit.c | 4 -
kernel/fork.c | 2 -
kernel/irq/handle.c | 7 +-
kernel/kthread.c | 3 -
kernel/lockdep.c | 12 +-
kernel/module.c | 7 +
kernel/sched.c | 10 +-
kernel/signal.c | 2 -
kernel/softirq.c | 3 -
kernel/trace/Makefile | 1 -
kernel/trace/events.c | 14 -
kernel/trace/trace.c | 3 +
kernel/trace/trace.h | 148 +----------
kernel/trace/trace_event_profile.c | 4 +-
kernel/trace/trace_events.c | 170 +++++++++----
kernel/trace/trace_events_filter.c | 10 +-
kernel/trace/trace_events_stage_1.h | 39 ---
kernel/trace/trace_events_stage_2.h | 170 ------------
kernel/trace/trace_events_stage_3.h | 279 --------------------
kernel/trace/trace_output.c | 3 +
kernel/trace/trace_output.h | 30 +--
mm/util.c | 11 +-
net/core/net-traces.c | 4 +-
40 files changed, 1648 insertions(+), 1290 deletions(-)
--
* Steven Rostedt <[email protected]> wrote:
> Ingo,
>
> This is the long awaited TRACE_EVENT for modules patch series.
>
> Not only does it allow for modules to use the TRACE_EVENT
> infrastructure, but it also cleans up the way TRACE_EVENTS are
> used in core kernel code.
Really impressive!
I've got a small testing hickup with it:
In file included from include/trace/lockdep.h:60,
from kernel/lockdep.c:51:
include/trace/define_trace.h:57:43: error: trace/lock.h: No such file or directory
The patch below fixes it.
Ingo
Index: linux/include/trace/lockdep.h
===================================================================
--- linux.orig/include/trace/lockdep.h
+++ linux/include/trace/lockdep.h
@@ -5,7 +5,7 @@
#include <linux/tracepoint.h>
#undef TRACE_SYSTEM
-#define TRACE_SYSTEM lock
+#define TRACE_SYSTEM lockdep
#ifdef CONFIG_LOCKDEP
* Steven Rostedt <[email protected]> wrote:
> include/trace/define_trace.h | 79 ++++++
> include/trace/ftrace.h | 493 +++++++++++++++++++++++++++++++++++
> include/trace/irq.h | 56 ++++-
> include/trace/irq_event_types.h | 55 ----
> include/trace/kmem.h | 189 +++++++++++++-
> include/trace/lockdep.h | 55 ++++-
> include/trace/lockdep_event_types.h | 57 ----
> include/trace/sched.h | 336 +++++++++++++++++++++++-
> include/trace/sched_event_types.h | 337 ------------------------
> include/trace/skb.h | 39 +++-
> include/trace/skb_event_types.h | 38 ---
> include/trace/trace_event_types.h | 7 -
> include/trace/trace_events.h | 7 -
Detail: we still have include/trace/kmem_event_types.h around - is
that intentional? It isnt actually used by anything so we can git-rm
it.
Also, we mix tracepoint definition headers with other misc headers
such as syscall.h or boot.h.
I think it would be cleaner and better sructured to have these
centrally enumerated tracepoint definitions separated in
include/trace/events/.
That sub-directory would _only_ include the TRACE_EVENT()
definitions - nothing else. Hence it would also be a nice in-situ
template collection for anyone adding new tracepoints. The existence
of other headers really distracts from that.
What do you think?
Ingo
there's another build failure as well:
kernel/trace/trace_events.c: In function ‘trace_module_add_events’:
kernel/trace/trace_events.c:805: error: dereferencing pointer to incomplete type
kernel/trace/trace_events.c:806: error: dereferencing pointer to incomplete type
kernel/trace/trace_events.c:806: error: dereferencing pointer to incomplete type
kernel/trace/trace_events.c: In function ‘trace_module_notify’:
kernel/trace/trace_events.c:850: error: ‘MODULE_STATE_COMING’ undeclared (first use in this function)
kernel/trace/trace_events.c:850: error: (Each undeclared identifier is reported only once
kernel/trace/trace_events.c:850: error: for each function it appears in.)
kernel/trace/trace_events.c:853: error: ‘MODULE_STATE_GOING’ undeclared (first use in this function)
config attached. The key bit is:
# CONFIG_MODULES is not set
I suspect you mainly concentrated on getting modular kernels right.
Fixing it for non-modular kernels should be no big deal.
Ingo
* Steven Rostedt <[email protected]> wrote:
>
> On Tue, 14 Apr 2009, Ingo Molnar wrote:
>
> >
> > * Steven Rostedt <[email protected]> wrote:
> >
> > > include/trace/define_trace.h | 79 ++++++
> > > include/trace/ftrace.h | 493 +++++++++++++++++++++++++++++++++++
> > > include/trace/irq.h | 56 ++++-
> > > include/trace/irq_event_types.h | 55 ----
> > > include/trace/kmem.h | 189 +++++++++++++-
> > > include/trace/lockdep.h | 55 ++++-
> > > include/trace/lockdep_event_types.h | 57 ----
> > > include/trace/sched.h | 336 +++++++++++++++++++++++-
> > > include/trace/sched_event_types.h | 337 ------------------------
> > > include/trace/skb.h | 39 +++-
> > > include/trace/skb_event_types.h | 38 ---
> > > include/trace/trace_event_types.h | 7 -
> > > include/trace/trace_events.h | 7 -
> >
> > Detail: we still have include/trace/kmem_event_types.h around - is
> > that intentional? It isnt actually used by anything so we can git-rm
> > it.
>
> I rebased it several times, doing minor tweaks and such. I think I may
> have missed a git-rm. That file is suppose to be deleted.
>
> >
> > Also, we mix tracepoint definition headers with other misc headers
> > such as syscall.h or boot.h.
> >
> > I think it would be cleaner and better sructured to have these
> > centrally enumerated tracepoint definitions separated in
> > include/trace/events/.
> >
> > That sub-directory would _only_ include the TRACE_EVENT()
> > definitions - nothing else. Hence it would also be a nice in-situ
> > template collection for anyone adding new tracepoints. The existence
> > of other headers really distracts from that.
> >
> > What do you think?
>
> I'm fine with that. As long as people are fine with doing:
>
> #include <trace/events/sched.h>
>
> and such.
Yes, i think it's very clear that way: trace/events/*.h bits are for
tracepoint definitions.
Ingo
On Tue, 14 Apr 2009, Ingo Molnar wrote:
>
> * Steven Rostedt <[email protected]> wrote:
>
> > include/trace/define_trace.h | 79 ++++++
> > include/trace/ftrace.h | 493 +++++++++++++++++++++++++++++++++++
> > include/trace/irq.h | 56 ++++-
> > include/trace/irq_event_types.h | 55 ----
> > include/trace/kmem.h | 189 +++++++++++++-
> > include/trace/lockdep.h | 55 ++++-
> > include/trace/lockdep_event_types.h | 57 ----
> > include/trace/sched.h | 336 +++++++++++++++++++++++-
> > include/trace/sched_event_types.h | 337 ------------------------
> > include/trace/skb.h | 39 +++-
> > include/trace/skb_event_types.h | 38 ---
> > include/trace/trace_event_types.h | 7 -
> > include/trace/trace_events.h | 7 -
>
> Detail: we still have include/trace/kmem_event_types.h around - is
> that intentional? It isnt actually used by anything so we can git-rm
> it.
I rebased it several times, doing minor tweaks and such. I think I may
have missed a git-rm. That file is suppose to be deleted.
>
> Also, we mix tracepoint definition headers with other misc headers
> such as syscall.h or boot.h.
>
> I think it would be cleaner and better sructured to have these
> centrally enumerated tracepoint definitions separated in
> include/trace/events/.
>
> That sub-directory would _only_ include the TRACE_EVENT()
> definitions - nothing else. Hence it would also be a nice in-situ
> template collection for anyone adding new tracepoints. The existence
> of other headers really distracts from that.
>
> What do you think?
I'm fine with that. As long as people are fine with doing:
#include <trace/events/sched.h>
and such.
-- Steve
On Tue, Apr 14, 2009 at 01:23:37PM -0400, Steven Rostedt wrote:
>
> Removal of the two headers per trace system. No need to have
> include/trace/sched.h and include/linux/sched_event_types.h
> All the changes go into include/trace/sched.h. But note that how that
> file is made is important. One could look at the sched.h file, or
> skb.h, lockdep.h and kmem.h as an example.
Hi Steven,
One thing which I would really like is to avoid needing to drop the
header file in include/trace/<subsystem.h>.
The problem that I have with this is that for ext4, we need to access
private data structures which are defined in header files in
fs/ext4/*.h --- which we moved into fs/ext4 a long time ago at the
request of those who felt include/linux/* was getting rather
cluttered, and if a subsystem had header files which were only needed
by files for that particular subsystems, they should be moved out of
include/linux.
I supported the above-mentioned cleanup, but it's causing problems
given that include/trace/ext4_events_types.h (or include/trace/ext4.h
in the new world order) needs access to various structure definitions
in fs/ext4/*.h. I could move the required header files into
include/linux/ext4_tracing_types.h --- which has the downside that it
is a very random collection of data structures --- or I could entirely
revert the cleanup we did long ago and move all of the ext4 header
files back into include/linux. But better yet would be if there was
some way we could tell the tracing subsystem that tracing header file
for ext4 could be found in fs/ext4/ext4_trace.h.
Any chance you could support something like this?
Thanks, regards,
- Ted
On Tue, 14 Apr 2009, Theodore Tso wrote:
> On Tue, Apr 14, 2009 at 01:23:37PM -0400, Steven Rostedt wrote:
> >
> > Removal of the two headers per trace system. No need to have
> > include/trace/sched.h and include/linux/sched_event_types.h
> > All the changes go into include/trace/sched.h. But note that how that
> > file is made is important. One could look at the sched.h file, or
> > skb.h, lockdep.h and kmem.h as an example.
>
> Hi Steven,
>
> One thing which I would really like is to avoid needing to drop the
> header file in include/trace/<subsystem.h>.
>
> The problem that I have with this is that for ext4, we need to access
> private data structures which are defined in header files in
> fs/ext4/*.h --- which we moved into fs/ext4 a long time ago at the
> request of those who felt include/linux/* was getting rather
> cluttered, and if a subsystem had header files which were only needed
> by files for that particular subsystems, they should be moved out of
> include/linux.
>
> I supported the above-mentioned cleanup, but it's causing problems
> given that include/trace/ext4_events_types.h (or include/trace/ext4.h
> in the new world order) needs access to various structure definitions
> in fs/ext4/*.h. I could move the required header files into
> include/linux/ext4_tracing_types.h --- which has the downside that it
> is a very random collection of data structures --- or I could entirely
> revert the cleanup we did long ago and move all of the ext4 header
> files back into include/linux. But better yet would be if there was
> some way we could tell the tracing subsystem that tracing header file
> for ext4 could be found in fs/ext4/ext4_trace.h.
>
> Any chance you could support something like this?
Yes, at the bottom of your header file, before including define_trace.h,
you can simply add:
#undef TRACE_INCLUDE_PATH
#define TRACE_INCLUDE_PATH ../../fs/ext4
#include <trace/define_trace.h>
the TRACE_INCLUDE_PATH is a reference from where trace/define_trace.h is
located. It may even be cleaner to do:
#undef TRACE_INCLUDE_PATH
#define TRACE_INCLUDE_PATH EXT4_PATH
and in the Makefile have:
CFLAGS_ext4_jbd2.o := -DEXT4_PATH=$(PWD)
That's something like what I did for my external module test.
-- Steve
Hi -
On Tue, Apr 14, 2009 at 05:04:45PM -0400, Theodore Tso wrote:
> [...]
> One thing which I would really like is to avoid needing to drop the
> header file in include/trace/<subsystem.h>.
>
> The problem that I have with this is that for ext4, we need to access
> private data structures which are defined in header files in
> fs/ext4/*.h --- which we moved into fs/ext4 a long time ago [...]
If the tracepoints/events expand to code that exposes those private
structures, then those structures are perhaps not quite so private any
more. That argues for moving those headers back under include/..., or
at least those type decls made reachable from the tracepoints.
- FChE
Theodore Tso wrote:
> Any chance you could support something like this?
>
>
I think that's already there. I'm defining
arch/x86/include/asm/paravirt-trace.h with:
#ifndef _ASM_X86_PARAVIRT_TRACE_H
#define _ASM_X86_PARAVIRT_TRACE_H
#include <linux/tracepoint.h>
#include <asm/paravirt_types.h>
#undef TRACE_SYSTEM
#define TRACE_SYSTEM pvops
#define TRACE_INCLUDE_FILE paravirt-trace
#define TRACE_INCLUDE_PATH asm
[...]
Which ends up including <asm/paravirt-trace.h>
J
On Tue, 14 Apr 2009, Jeremy Fitzhardinge wrote:
> Theodore Tso wrote:
> > Any chance you could support something like this?
> >
> >
>
> I think that's already there. I'm defining
> arch/x86/include/asm/paravirt-trace.h with:
>
> #ifndef _ASM_X86_PARAVIRT_TRACE_H
> #define _ASM_X86_PARAVIRT_TRACE_H
>
> #include <linux/tracepoint.h>
> #include <asm/paravirt_types.h>
>
> #undef TRACE_SYSTEM
> #define TRACE_SYSTEM pvops
>
> #define TRACE_INCLUDE_FILE paravirt-trace
> #define TRACE_INCLUDE_PATH asm
> [...]
>
>
> Which ends up including <asm/paravirt-trace.h>
Not quite. It ends up looking like
#include "asm/paravirt-trace.h"
As long as there is no "asm" directory in the include/trace directory, I
think that is fine.
-- Steve
On Tue, 14 Apr 2009, Steven Rostedt wrote:
>
> Yes, at the bottom of your header file, before including define_trace.h,
> you can simply add:
>
> #undef TRACE_INCLUDE_PATH
> #define TRACE_INCLUDE_PATH ../../fs/ext4
>
> #include <trace/define_trace.h>
>
> the TRACE_INCLUDE_PATH is a reference from where trace/define_trace.h is
> located. It may even be cleaner to do:
>
>
> #undef TRACE_INCLUDE_PATH
> #define TRACE_INCLUDE_PATH EXT4_PATH
>
> and in the Makefile have:
>
> CFLAGS_ext4_jbd2.o := -DEXT4_PATH=$(PWD)
>
> That's something like what I did for my external module test.
Note, you only need to add the CFLAGS for the object that performs the:
#define CREATE_TRACE_POINTS
-- Steve
On Tue, 14 Apr 2009, Frank Ch. Eigler wrote:
> Hi -
>
> On Tue, Apr 14, 2009 at 05:04:45PM -0400, Theodore Tso wrote:
> > [...]
> > One thing which I would really like is to avoid needing to drop the
> > header file in include/trace/<subsystem.h>.
> >
> > The problem that I have with this is that for ext4, we need to access
> > private data structures which are defined in header files in
> > fs/ext4/*.h --- which we moved into fs/ext4 a long time ago [...]
>
> If the tracepoints/events expand to code that exposes those private
> structures, then those structures are perhaps not quite so private any
> more. That argues for moving those headers back under include/..., or
> at least those type decls made reachable from the tracepoints.
I think those headers are fine. This must also work when ext4 is a module.
-- Steve
Steven Rostedt wrote:
> On Tue, 14 Apr 2009, Jeremy Fitzhardinge wrote:
>
>
>> Theodore Tso wrote:
>>
>>> Any chance you could support something like this?
>>>
>>>
>>>
>> I think that's already there. I'm defining
>> arch/x86/include/asm/paravirt-trace.h with:
>>
>> #ifndef _ASM_X86_PARAVIRT_TRACE_H
>> #define _ASM_X86_PARAVIRT_TRACE_H
>>
>> #include <linux/tracepoint.h>
>> #include <asm/paravirt_types.h>
>>
>> #undef TRACE_SYSTEM
>> #define TRACE_SYSTEM pvops
>>
>> #define TRACE_INCLUDE_FILE paravirt-trace
>> #define TRACE_INCLUDE_PATH asm
>> [...]
>>
>>
>> Which ends up including <asm/paravirt-trace.h>
>>
>
> Not quite. It ends up looking like
>
> #include "asm/paravirt-trace.h"
>
> As long as there is no "asm" directory in the include/trace directory, I
> think that is fine.
OK.
I'm having a bit of trouble with paravirt-trace.h when I actually
include it in paravirt.h. asm/paravirt.h is itself included in lots of
places, and so its fairly easy to end up with cyclic include
dependencies which are fairly painful. In this case I'm seeing
asm/paravirt.h -> linux/tracepoint.h -> linux/rcupate.h -> {lots of
stuff}, which gives errors like:
CC arch/x86/kernel/asm-offsets.s
In file included from /home/jeremy/git/linux/include/linux/thread_info.h:55,
from /home/jeremy/git/linux/include/linux/preempt.h:9,
from /home/jeremy/git/linux/include/linux/spinlock.h:50,
from /home/jeremy/git/linux/include/linux/rcupdate.h:37,
from /home/jeremy/git/linux/include/linux/tracepoint.h:18,
from /home/jeremy/git/linux/arch/x86/include/asm/paravirt-trace.h:4,
from /home/jeremy/git/linux/arch/x86/include/asm/paravirt.h:18,
from /home/jeremy/git/linux/arch/x86/include/asm/irqflags.h:55,
from /home/jeremy/git/linux/include/linux/irqflags.h:57,
from /home/jeremy/git/linux/arch/x86/include/asm/system.h:11,
from /home/jeremy/git/linux/arch/x86/include/asm/processor.h:17,
from /home/jeremy/git/linux/include/linux/prefetch.h:14,
from /home/jeremy/git/linux/include/linux/list.h:6,
from /home/jeremy/git/linux/include/linux/module.h:9,
from /home/jeremy/git/linux/include/linux/crypto.h:21,
from /home/jeremy/git/linux/arch/x86/kernel/asm-offsets_64.c:7,
from /home/jeremy/git/linux/arch/x86/kernel/asm-offsets.c:4:
/home/jeremy/git/linux/arch/x86/include/asm/thread_info.h:34: error: expected specifier-qualifier-list before 'mm_segment_t'
I'm wondering if there's much downside in making the code __DO_TRACE()
out of line so that we can make tracepoint.h have absolutely minimal
include dependencies?
J
* Jeremy Fitzhardinge <[email protected]> wrote:
> Steven Rostedt wrote:
>> On Tue, 14 Apr 2009, Jeremy Fitzhardinge wrote:
>>
>>
>>> Theodore Tso wrote:
>>>
>>>> Any chance you could support something like this?
>>>>
>>>>
>>> I think that's already there. I'm defining
>>> arch/x86/include/asm/paravirt-trace.h with:
>>>
>>> #ifndef _ASM_X86_PARAVIRT_TRACE_H
>>> #define _ASM_X86_PARAVIRT_TRACE_H
>>>
>>> #include <linux/tracepoint.h>
>>> #include <asm/paravirt_types.h>
>>>
>>> #undef TRACE_SYSTEM
>>> #define TRACE_SYSTEM pvops
>>>
>>> #define TRACE_INCLUDE_FILE paravirt-trace
>>> #define TRACE_INCLUDE_PATH asm
>>> [...]
>>>
>>>
>>> Which ends up including <asm/paravirt-trace.h>
>>>
>>
>> Not quite. It ends up looking like
>>
>> #include "asm/paravirt-trace.h"
>>
>> As long as there is no "asm" directory in the include/trace directory,
>> I think that is fine.
>
> OK.
>
> I'm having a bit of trouble with paravirt-trace.h when I actually
> include it in paravirt.h. asm/paravirt.h is itself included in
> lots of places, and so its fairly easy to end up with cyclic
> include dependencies which are fairly painful. In this case I'm
> seeing asm/paravirt.h -> linux/tracepoint.h -> linux/rcupate.h ->
> {lots of stuff}, which gives errors like:
tracepoint.h should not include any complex headers like rcupdate.h.
> I'm wondering if there's much downside in making the code
> __DO_TRACE() out of line so that we can make tracepoint.h have
> absolutely minimal include dependencies?
yeah.
And besides, the rcu_read_lock_sched_notrace() there should probably
be a preempt_disable_notrace() / preempt_enable_notrace() variant.
(it's sligtly faster that way and we better be atomic and
self-sufficient in tracepoints anyway)
Ingo
* Ingo Molnar ([email protected]) wrote:
>
> * Jeremy Fitzhardinge <[email protected]> wrote:
>
> > Steven Rostedt wrote:
> >> On Tue, 14 Apr 2009, Jeremy Fitzhardinge wrote:
> >>
> >>
> >>> Theodore Tso wrote:
> >>>
> >>>> Any chance you could support something like this?
> >>>>
> >>>>
> >>> I think that's already there. I'm defining
> >>> arch/x86/include/asm/paravirt-trace.h with:
> >>>
> >>> #ifndef _ASM_X86_PARAVIRT_TRACE_H
> >>> #define _ASM_X86_PARAVIRT_TRACE_H
> >>>
> >>> #include <linux/tracepoint.h>
> >>> #include <asm/paravirt_types.h>
> >>>
> >>> #undef TRACE_SYSTEM
> >>> #define TRACE_SYSTEM pvops
> >>>
> >>> #define TRACE_INCLUDE_FILE paravirt-trace
> >>> #define TRACE_INCLUDE_PATH asm
> >>> [...]
> >>>
> >>>
> >>> Which ends up including <asm/paravirt-trace.h>
> >>>
> >>
> >> Not quite. It ends up looking like
> >>
> >> #include "asm/paravirt-trace.h"
> >>
> >> As long as there is no "asm" directory in the include/trace directory,
> >> I think that is fine.
> >
> > OK.
> >
> > I'm having a bit of trouble with paravirt-trace.h when I actually
> > include it in paravirt.h. asm/paravirt.h is itself included in
> > lots of places, and so its fairly easy to end up with cyclic
> > include dependencies which are fairly painful. In this case I'm
> > seeing asm/paravirt.h -> linux/tracepoint.h -> linux/rcupate.h ->
> > {lots of stuff}, which gives errors like:
>
> tracepoint.h should not include any complex headers like rcupdate.h.
>
> > I'm wondering if there's much downside in making the code
> > __DO_TRACE() out of line so that we can make tracepoint.h have
> > absolutely minimal include dependencies?
>
> yeah.
>
> And besides, the rcu_read_lock_sched_notrace() there should probably
> be a preempt_disable_notrace() / preempt_enable_notrace() variant.
> (it's sligtly faster that way and we better be atomic and
> self-sufficient in tracepoints anyway)
>
Yes, rcu_read_(un)lock_sched_notrace maps directly to
preempt_(en/dis)able_notrace. But for RCU verifiability's sake, I made
sure to create rcu_read_lock versions of these primitives instead of
simply using preempt_disable. Maybe we should simply take those
low-level primitives out of rcupdate.h and put them in a simpler header?
Mathieu
> Ingo
>
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
On Tue, Apr 14, 2009 at 05:04:45PM -0400, Theodore Tso wrote:
> One thing which I would really like is to avoid needing to drop the
> header file in include/trace/<subsystem.h>.
>
> The problem that I have with this is that for ext4, we need to access
> private data structures which are defined in header files in
> fs/ext4/*.h --- which we moved into fs/ext4 a long time ago at the
> request of those who felt include/linux/* was getting rather
> cluttered, and if a subsystem had header files which were only needed
> by files for that particular subsystems, they should be moved out of
> include/linux.
Same is true for XFS, or just about any other self-contained in a module
subsystem I can think of. My current XFS trace events stuff includes
tons of headers in the xfs header in include/trace and adds -Ifs/xfs to
kernel/trace/Makefile. The latter should be gone by the new
implementation compiling the tracing stubs into the module, but it's
still not pretty.
On Tue, Apr 14, 2009 at 05:29:17PM -0400, Frank Ch. Eigler wrote:
> Hi -
>
> On Tue, Apr 14, 2009 at 05:04:45PM -0400, Theodore Tso wrote:
> > [...]
> > One thing which I would really like is to avoid needing to drop the
> > header file in include/trace/<subsystem.h>.
> >
> > The problem that I have with this is that for ext4, we need to access
> > private data structures which are defined in header files in
> > fs/ext4/*.h --- which we moved into fs/ext4 a long time ago [...]
>
> If the tracepoints/events expand to code that exposes those private
> structures, then those structures are perhaps not quite so private any
> more. That argues for moving those headers back under include/..., or
> at least those type decls made reachable from the tracepoints.
They expose them to the TRACE_EVENT output format, which then
pretty-prints them. They are still as private to the module as it gets.