Re: [PATCH v9] Unified trace buffer

[Ingo Molnar <mingo@elte.hu>]

* 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