lkml.org 
[lkml]   [2008]   [Sep]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v9] Unified trace buffer

* Steven Rostedt <rostedt@goodmis.org> wrote:

> > > Index: linux-trace.git/include/linux/ring_buffer.h
> > > +enum {
> > > + RB_TYPE_PADDING, /* Left over page padding
> >
> > RB_ clashes with red-black tree namespace. (on the thought level)
>
> Yeah, Linus pointed this out with the rb_ static function names. But since
> the functions are static I kept them as is. But here we have global names.
>
> Would RNGBF_ be OK, or do you have any other ideas?

that's even worse i think :-/ And this isnt bikeshed-painting really,
the RNGBF_ name hurts my eyes and RB_ is definitely confusing to read.
(as the rbtree constants are in capitals as well and similarly named)

RING_TYPE_PADDING

or:

RINGBUF_TYPE_PADDING

yes, it's longer, but still, saner.

> > too large, please uninline.
>
> I calculated this on x86_64 to add 78 bytes. Is that still too big?

yes, way too big. Sometimes we make savings from a 10 bytes function
already. (but it's always case dependent - if a function has a lot of
parameters then uninlining can hurt)

the only exception would be if there's normally only a single
instantiation per tracer, and if it's in the absolute tracing hotpath.

> > hm, should not be raw, at least initially. I am 95% sure we'll see
> > lockups, we always did when we iterated ftrace's buffer
> > implementation ;-)
>
> It was to prevent lockdep from checking the locks from inside. We had
> issues with ftroce and lockdep in the past, because ftrace would trace
> the internals of lockdep, and lockdep would then recurse back into
> itself to trace. If lockdep itself can get away with not using
> raw_spinlocks, then this will be OK to make back to spinlock.

would be nice to make sure that ftrace's recursion checks work as
intended - and the same goes for lockdep's recursion checks. Yes, we had
problems in this area, and it would be nice to make sure it all works
fine. (or fix it if it doesnt)


> > > + for (head = 0; head < rb_head_size(cpu_buffer);
> > > + head += ring_buffer_event_length(event)) {
> > > + event = rb_page_index(cpu_buffer->head_page, head);
> > > + BUG_ON(rb_null_event(event));
> >
> > ( optional:when there's a multi-line loop then i generally try to insert
> > an extra newline when starting the body - to make sure the iterator
> > and the body stands apart visually. Matter of taste. )
>
> Will fix, I have no preference.

clarification: multi-line loop _condition_. It's pretty rare (this is
such a case) but sometimes unavoidable - and then the newline helps
visually.

> > > + RB_TYPE_TIME_EXTENT, /* Extent the time delta
> > > + * array[0] = time delta (28 .. 59)
> > > + * size = 8 bytes
> > > + */
> >
> > please use standard comment style:
> >
> > /*
> > * Comment
> > */
>
> Hmm, this is interesting. I kind of like this because it is not really a
> standard comment. It is a comment about the definitions of the enum. I
> believe if they are above:
>
> /*
> * Comment
> */
> RB_ENUM_TYPE,
>
> It is not as readable. But if we do:

no, it is not readable. My point was that you should do:
>
> RB_ENUM_TYPE, /*
> * Comment
> */
>
> The comment is not at the same line as the enum, which also looks
> unpleasing.

but you did:

> RB_ENUM_TYPE, /* Comment
> */

So i suggested to fix it to:

+ RB_TYPE_TIME_EXTENT, /*
+ * Extent the time delta
+ * array[0] = time delta (28 .. 59)
+ * size = 8 bytes
+ */
ok? I.e. "comment" should have the same visual properties as other
comments.

I fully agree with moving it next to the enum, i sometimes use that
style too, it's a nice touch and more readable in this case than
comment-ahead. (which we use for statements)

Ingo


\
 
 \ /
  Last update: 2008-09-27 21:45    [from the cache]
©2003-2011 Jasper Spaans. Advertise on this site