linux-kernel - Re: [PATCH v9] Unified trace buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 27 Sep 2008 21:41:05 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Peter Zijlstra <peterz@...radead.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	prasad@...ux.vnet.ibm.com,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Mathieu Desnoyers <compudj@...stal.dyndns.org>,
	"Frank Ch. Eigler" <fche@...hat.com>,
	David Wilder <dwilder@...ibm.com>, hch@....de,
	Martin Bligh <mbligh@...gle.com>,
	Christoph Hellwig <hch@...radead.org>,
	Masami Hiramatsu <mhiramat@...hat.com>,
	Steven Rostedt <srostedt@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...stprotocols.net>
Subject: Re: [PATCH v9] Unified trace buffer


* Steven Rostedt <rostedt@...dmis.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/