Comments

I wrote sputrace before generic tracing infrastrucure was available.
Now that we have the generic event tracer we can convert it over and
remove a lot of code:
8 files changed, 45 insertions(+), 285 deletions(-)
To use it make sure CONFIG_EVENT_TRACING is enabled and then enable
the spufs trace channel by
echo 1 > /sys/kernel/debug/tracing/events/spufs/spufs_context
and then read the trace records using e.g.
cat /sys/kernel/debug/tracing/trace
Note that the patch is ontop of the current tracing tree
git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git#tracing/ftrace
as there have been some TRACE_EVENT changes in there that aren't in
mainline yet.
I don't have any cell hardware anymore so this is only compile tested.
Signed-off-by: Christoph Hellwig <hch@lst.de>

On Wed, May 06, 2009 at 12:57:48PM +0200, Ingo Molnar wrote:
> Nice! Needs also an Ack from PowerPC folks before we can do this. > The cross section to other powerpc code seems to be rather low.
cbe-oss-dev is the Cell list.
> > -config SPU_TRACE> > - tristate "SPU event tracing support"> > - depends on SPU_FS && MARKERS> > - help> > - This option allows reading a trace of spu-related events through> > - the sputrace file in procfs.> > I think we should keep this option around.
Why? trace_events that aren't enabled are extremly low overhead.
And most in the current tree are non-optional.
> > +# magic for the trace events> > +CFLAGS_sched.o := -I$(src)> > Steve, i'm wondering whether this type of Makefile hackery (caused > by modular tracepoints) could be eliminated ...
We would just have to include the header file with "" instead of <>.
But I remember Steve not liking this when we talked about it.

* Christoph Hellwig <hch@lst.de> wrote:
> On Wed, May 06, 2009 at 12:57:48PM +0200, Ingo Molnar wrote:> > Nice! Needs also an Ack from PowerPC folks before we can do this. > > The cross section to other powerpc code seems to be rather low.> > cbe-oss-dev is the Cell list.
( ... and linuxppc-dev. And since it's all a sub-architecture of
PowerPC and Cell changes go upstream via the PowerPC tree, it's
nice to have the attention (and acks) of generic-arch maintainers
as well (if they are interested). And even if it's OK, Ben and
Arnd obviously calls the shots when it comes to workflow details:
in which tree and how to queue it up. )
> > > -config SPU_TRACE> > > - tristate "SPU event tracing support"> > > - depends on SPU_FS && MARKERS> > > - help> > > - This option allows reading a trace of spu-related events through> > > - the sputrace file in procfs.> > > > I think we should keep this option around.> > Why? trace_events that aren't enabled are extremly low overhead.> And most in the current tree are non-optional.
It is general curtesy to maintain the old Kconfig structure and ease
migration to a new facility.
Part of the "being nice" excercise ;-)
> > > +# magic for the trace events> > > +CFLAGS_sched.o := -I$(src)> > > > Steve, i'm wondering whether this type of Makefile hackery (caused > > by modular tracepoints) could be eliminated ...> > We would just have to include the header file with "" instead of > <>. But I remember Steve not liking this when we talked about it.
Yeah. But changing Makefiles isnt particularly clean either ...
And adding -I$(src) can have side-effects: we often have a local
foo.h while an include/linux/foo.h as well.
Ingo

On Wed, 2009-05-06 at 13:23 +0200, Ingo Molnar wrote:
> > > > +# magic for the trace events> > > > +CFLAGS_sched.o := -I$(src)> > > > > > Steve, i'm wondering whether this type of Makefile hackery (caused > > > by modular tracepoints) could be eliminated ...> > > > We would just have to include the header file with "" instead of > > <>. But I remember Steve not liking this when we talked about it.> > Yeah. But changing Makefiles isnt particularly clean either ...> > And adding -I$(src) can have side-effects: we often have a local > foo.h while an include/linux/foo.h as well.
That still would not conflict, because
#include "foo.h"
will not include "linux/foo.h" and
#include <linux/foo.h>
will not include a local foo.h, unless there's also a local "linux"
directory with a foo.h in it.
The Makefile hack has to do with being able to have the "foo.h" file
with the TRACE_EVENTs someplace other than include/trace.
If the "foo.h" is in include/trace.h we do not need to include this
hack. But because the include/trace/define_trace.h needs to include the
"foo.h" file recursively, it must be able to find it. If we do not add a
search path, include/trace/define_trace.h will not look in the other
locations.
Note, as Christoph did, we only need to add the include path to the file
that defines "CREATE_TRACE_POINTS". Which is only one file.
CFLAGS_sched.o := -I$(src)
Only touches the sched.c file in that directory (Note, for those reading
this thread out of context, this is not the same file as kernel/sched.c)
-- Steve

On Wed, May 06, 2009 at 12:57:48PM +0200, Ingo Molnar wrote:
> > (Note: please Cc all tracing related patches to lkml.)> > * Christoph Hellwig <hch@lst.de> wrote:> > > I wrote sputrace before generic tracing infrastrucure was > > available. Now that we have the generic event tracer we can > > convert it over and remove a lot of code:> > > > 8 files changed, 45 insertions(+), 285 deletions(-)> > Nice! Needs also an Ack from PowerPC folks before we can do this. > The cross section to other powerpc code seems to be rather low.> > A few comments:> > > To use it make sure CONFIG_EVENT_TRACING is enabled and then enable> > the spufs trace channel by> > > > echo 1 > /sys/kernel/debug/tracing/events/spufs/spufs_context> > > > and then read the trace records using e.g.> > > > cat /sys/kernel/debug/tracing/trace> > > > > > Note that the patch is ontop of the current tracing tree> > > > git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git#tracing/ftrace> > > > as there have been some TRACE_EVENT changes in there that aren't in> > mainline yet.> > > > I don't have any cell hardware anymore so this is only compile tested.> > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de>> > > > Index: linux-2.6-tip/arch/powerpc/platforms/cell/spufs/sputrace.h> > ===================================================================> > --- /dev/null 1970-01-01 00:00:00.000000000 +0000> > +++ linux-2.6-tip/arch/powerpc/platforms/cell/spufs/sputrace.h 2009-05-06 10:17:20.000000000 +0000> > @@ -0,0 +1,39 @@> > +#if !defined(_TRACE_SPUFS_H) || defined(TRACE_HEADER_MULTI_READ)> > +#define _TRACE_SPUFS_H> > +> > +#include <linux/tracepoint.h>> > +> > +#undef TRACE_SYSTEM> > +#define TRACE_SYSTEM spufs> > +> > +TRACE_EVENT(spufs_context,> > + TP_PROTO(struct spu_context *ctx, struct spu *spu, const char *name),> > + TP_ARGS(ctx, spu, name),> > +> > + TP_STRUCT__entry(> > + __field(int, owner_tid)> > + __field(const char *, name)
Christoph,
I don't know much the code you are tracing. But it is rare that
a const char * is safe on tracing. Still it could be, you just have to
ensure the string cannot be freed in any way because this pointer
will be stored in the ring buffer and it can be read and dereferenced later
in a random time, could be several years :-)
So if this pointer references built-in data, no problem with that.
But if it can freed (comes from a module, __initdata, ...), then
you should use the __string() field which does an strcpy on the
ring buffer.
If you think this is safe, then it's the best choice because
storing a pointer is of course less costly than an strcpy.
If so I will add the support for char * in the filters (trivial).
Thanks,
Frederic.

On Wed, May 06, 2009 at 06:53:37PM +0200, Frederic Weisbecker wrote:
> I don't know much the code you are tracing. But it is rare that> a const char * is safe on tracing. Still it could be, you just have to> ensure the string cannot be freed in any way because this pointer> will be stored in the ring buffer and it can be read and dereferenced later> in a random time, could be several years :-)> > So if this pointer references built-in data, no problem with that.> But if it can freed (comes from a module, __initdata, ...), then> you should use the __string() field which does an strcpy on the> ring buffer.> > If you think this is safe, then it's the best choice because> storing a pointer is of course less costly than an strcpy.> If so I will add the support for char * in the filters (trivial).
The pointer here only ever references string constants, it's always
a string literal in the callers.

On Wed, 2009-05-06 at 19:13 +0200, Christoph Hellwig wrote:
> On Wed, May 06, 2009 at 06:53:37PM +0200, Frederic Weisbecker wrote:> > I don't know much the code you are tracing. But it is rare that> > a const char * is safe on tracing. Still it could be, you just have to> > ensure the string cannot be freed in any way because this pointer> > will be stored in the ring buffer and it can be read and dereferenced later> > in a random time, could be several years :-)> > > > So if this pointer references built-in data, no problem with that.> > But if it can freed (comes from a module, __initdata, ...), then> > you should use the __string() field which does an strcpy on the> > ring buffer.> > > > If you think this is safe, then it's the best choice because> > storing a pointer is of course less costly than an strcpy.> > If so I will add the support for char * in the filters (trivial).> > The pointer here only ever references string constants, it's always> a string literal in the callers.
The worry is if this is used by modules. A constant string may not be
around when the buffer is read.
This should not be a problem because the formatting of the string is not
around either, and we just output 'unknown type'. But I may be adding
code when a module is unloaded to reset the ring buffer if the module
registered any events. That's because we have other races to worry
about.
-- Steve

Hi Christoph,
Nice work!
We'd just need to keep in sync with the performance folks who use the
sputrace data. I don't imagine that the changes would be complex on
their end though.
I'm happy to merge into the spufs tree, pending the ftrace dependencies.
Or would this be better going in with those?
Cheers,
Jeremy

* Jeremy Kerr <jk@ozlabs.org> wrote:
> Hi Christoph,> > Nice work!> > We'd just need to keep in sync with the performance folks who use > the sputrace data. I don't imagine that the changes would be > complex on their end though.> > I'm happy to merge into the spufs tree, pending the ftrace > dependencies. Or would this be better going in with those?
If you think there's not going to be big ongoing conflicts in this
specific code in the next month leading up to the .31 merge window,
and if you ack all the changes, i'd be glad to merge this patch into
the tracing tree.
I do daily builds of powerpc defconfig, which has these spufs
settings:
#
# Cell Broadband Engine options
#
CONFIG_SPU_FS=m
CONFIG_SPU_FS_64K_LS=y
# CONFIG_SPU_TRACE is not set
so spufs is build-tested, but spu-trace is not - but with
Christoph's patch the tracing facility would be available
unconditionally, so that would get (build) tested as well,
and propagated to linux-next.
Ingo

On Wed, May 06, 2009 at 03:05:39PM -0400, Steven Rostedt wrote:
> The worry is if this is used by modules. A constant string may not be> around when the buffer is read.> > This should not be a problem because the formatting of the string is not> around either, and we just output 'unknown type'. But I may be adding> code when a module is unloaded to reset the ring buffer if the module> registered any events. That's because we have other races to worry> about.
I think having some constant string description for trace events is
pretty common and used in just above any ad-hoc tracer I've seen,
and just storing the pointers makes this a lot more efficient.
So either we try to make sure this works, or we need some big waivers
in the documentation that people need to use the slower __string
variant.

On Thu, May 07, 2009 at 11:34:10AM +1000, Jeremy Kerr wrote:
> Hi Christoph,> > Nice work!> > We'd just need to keep in sync with the performance folks who use the > sputrace data. I don't imagine that the changes would be complex on > their end though.> > I'm happy to merge into the spufs tree, pending the ftrace dependencies. > Or would this be better going in with those?
For me life would be simpler if goes in via the ftrace tree, so if you
don't have any changes conflicting with it pending (and I kept the
wrapper macros for the tracepoints to keep that chance low) I'd go down
the route.
Btw, do you want to keep the config option or are you fine with removing
it?

On Thu, May 07, 2009 at 11:34:10AM +1000, Jeremy Kerr wrote:
> Hi Christoph,> > Nice work!> > We'd just need to keep in sync with the performance folks who use the > sputrace data. I don't imagine that the changes would be complex on > their end though.> > I'm happy to merge into the spufs tree, pending the ftrace dependencies. > Or would this be better going in with those?
As we're getting to the end of the subsystem merge window for 2.6.31
what's the feedback from the performance folks?
If everyone is fine with the patch I'll respin it with the field
re-ordering suggested by Ingo and then we can put it in the tracing
tree.

* Christoph Hellwig <hch@lst.de> wrote:
> On Thu, May 07, 2009 at 11:34:10AM +1000, Jeremy Kerr wrote:> > Hi Christoph,> > > > Nice work!> > > > We'd just need to keep in sync with the performance folks who use the > > sputrace data. I don't imagine that the changes would be complex on > > their end though.> > > > I'm happy to merge into the spufs tree, pending the ftrace dependencies. > > Or would this be better going in with those?> > As we're getting to the end of the subsystem merge window for > 2.6.31 what's the feedback from the performance folks?> > If everyone is fine with the patch I'll respin it with the field > re-ordering suggested by Ingo and then we can put it in the > tracing tree.
either solution is fine with me. (It could also be pushed out of
order in the merge window, after the powerpc and the tracing tree.)
Ingo

On Thu, Jun 11, 2009 at 10:18:06AM +1000, Jeremy Kerr wrote:
> Hi Christoph,> > > As we're getting to the end of the subsystem merge window for 2.6.31> > what's the feedback from the performance folks?> > Still no answer, I say merge it.
Now that all pre-requisites are in mainline can you just pull in through
the spufs tree? Would be great to still get it into 2.6.31 so we can
kill markers entirely this merge window.