Re: [PATCH/RFC 3/3] ath5k: trace resets

[Bob Copeland <me@bobcopeland.com>]

On Tue, Jul 20, 2010 at 02:20:49PM +0900, Bruno Randolf wrote:
> > @@ -931,6 +930,7 @@ ath5k_debug_printrxbuf(struct ath5k_buf *bf, int done,
> >  void
> >  ath5k_debug_printrxbuffs(struct ath5k_softc *sc, struct ath5k_hw *ah)
> >  {
> > +#if 0

(The above, by the way, is a mistake I'll fix; I forgot to remove it after
doing patch 1/3.)

> again, here my same concerns: printing the reasons for resets is something 
> which is useful on embedded boards and production setups which can't have 
> tracing enabled. which is why i want to object against this change!

What Johannes said wrt performance: during tracing, only the binary
representations of the data are written to the trace ring buffer, so
unlike the current debug code, we aren't doing printk formatting until
the trace buffer is read.  You can save the raw binary data from the
trace and do formatting on another machine.

Another advantage is better granularity: if you only care about watching
tx on the cab queue, you can dynamically filter based on the tracepoint
arguments, something like:

  # echo "qnum == 6" > /debug/tracing/events/ath5k/ath5k_tx/filter

With the debug printks, you have to hack the driver or grep and hope
the printk buffer didn't overflow and spill what you were looking for.

One thing I do need to look into is reducing the size, right now
text size went up by about 4k when adding these tracepoints.

> also adding a reason argument to the reset function just for tracing seems to 
> be... umm... not so nice... couldn't you add the tracepoints before? 

Yeah, it's kind of hacky, but not without precedent; ieee80211_wake_queues
does something similar.  But I'm not tied to it, adding another tracepoint
for when and why the reset was scheduled would be OK, or maybe we just
drop the reason and plan on using ftrace to figure that out.  It's still
worth keeping tracepoints when reset actually runs and finishes since that
is the most useful information for tracking down race conditions.

> and: didn't we want to split channel change out of reset anyhow?

Of course.  When we do so we probably won't need the frequency argument,
but I think that's otherwise orthogonal to this...

-- 
Bob Copeland %% www.bobcopeland.com