Re: [PATCH 3/8] Add yaffs2 file system: guts code

[Charles Manning <manningc2@actrix.gen.nz>]

On Tuesday 07 December 2010 13:47:43 Steven Rostedt wrote:
> On Tue, 2010-12-07 at 00:03 +0100, Arnd Bergmann wrote:
> > > > > yaffs_trace(YAFFS_TRACE_BUFFERS,
> > > > > 	"Out of temp buffers at line %d, other held by lines:",line_no);
> > > > > for (i = 0; i < YAFFS_N_TEMP_BUFFERS; i++)
> > > > > 	yaffs_trace(YAFFS_TRACE_BUFFERS," %d ", dev->temp_buffer[i].line);
> > > > > yaffs_trace(YAFFS_TRACE_BUFFERS, "\n");
> > > > >
> > > > > Would that be OK?
> > > > >
> > > > > I am loath to have to pull out useful code then plug it back in
> > > > > again.
> > > >
> > > > I don't think the yaffs_trace() function would be much better than
> > > > the T() macro, I was talking more about the fact that you have your
> > > > own nonstandard tracing infrastructure than the ugliness of the
> > > > interface.
> > > >
> > > > The point of pulling it out now would be force you to rethink the
> > > > tracing. If you think that you'd arrive at the same conclusion, just
> > > > save the diff between the code with and without tracing so you can
> > > > submit that patch again later.
> > > >
> > > > Having some sort of tracing is clearly useful, but it's also not
> > > > essential for the basic yaffs2 operation. We want to keep a
> > > > consistent way of presenting trace points across the kernel, so as
> > > > long as you do it differently, your code is going to be viewed with
> > > > some suspicion.
> > > >
> > > > Please have a look at how ext4, gfs2 and xfs do tracing.
> > >
> > > Looking in Linus' tree, all of those contain custom tracing of the form
> > > I propose.
> >
> > Hmm, yes I guess that's right...
> >
> > I was specifically talking about the include/trace/* based trace events
> > as something to look at, not the random printk based tracing stuff.
> > Maybe Steven or Frederic can give some more insight on that.
>
> What are all those T() functions? Some look like they should be replaced
> with printk(KERN_* "") functions, some others for tracing (I guess the
> ones with YAFFS_TRACE_TRACING).

Yes those are very ugly. That is why I proposed changing them to 

yaffs_trace(bit, "format", args).

That gives printk tracing which I can select on the fly by enabling the 
selected bits in the bitmask. eg. If I want to see the OS calls and the mtd 
accesses then I enable YAFFS_TRACE_MTD and YAFFS_TRACE_OS and only those 
grace groups get spat out.

People find this very handy, especially during system integration, so I am 
loath to lose it. It is simple and it works.

Will it not be acceptable to just leave in the printk-style messages and 
perhaps addTRACE_EVENT later?

>
> ext4, gfs and xfs all have converted to the TRACE_EVENT() methods. When
> you have this, you get tracing for free. The work with both ftrace and
> perf. You can look at the samples/trace_events/ code for examples.
>
> Note, if you use TRACE_EVENT() and you want to debug even more, you can
> simply add trace_printk() and that will also appear in your tracing
> output.

>From what I see, ext4 uses both trace_event and wrapped printk tracing, some 
right alongside eachother so it is a duplication - not a replacement.

YAFFS has approx 500 trace lines in it. Some of those would make sense to 
attach to TRACE_EVENT() , but most not. trace/events/ext4.h has 1172 lines 
for around 28 events (== 40-odd lines per event).

Still reading everything I can find on this  (inc, your LWN articles) to get 
an understanding of what capabilities these give me and what heuristic should 
be used to define trace points vs printks.

-- Charles