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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <alpine.DEB.1.10.0809271549160.16572@gandalf.stny.rr.com>
Date:	Sat, 27 Sep 2008 15:54:07 -0400 (EDT)
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Ingo Molnar <mingo@...e.hu>
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


On Sat, 27 Sep 2008, Ingo Molnar wrote:
> 
> * Steven Rostedt <rostedt@...dmis.org> wrote:
> 
> 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.

I don't mind the extra typing, it is just a bit more difficult to keep in
the 80 character line limit.

> 
> > > 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. 

It is a hot path in the internals. Perhaps I'll make an inline function
in the interal code "rb_event_length" and have the other users call.

unsigned ring_buffer_event(struct ring_buffer_event *event)
{
	return rb_event_length(event);
}

> 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)

But then we have:

        RB_TYPE_PADDING,        /*
				 * Left over page padding
                                 * array is ignored
                                 * size is variable depending on
                                 * how much padding is needed
                                 */
        RB_TYPE_TIME_EXTENT,    /*
				 * Extent the time delta
                                 * array[0] = time delta (28 .. 59)
                                 * size = 8 bytes
                                 */

Where it is not as easy to see which comment is with which enum. 
Especially when you have many enums. That's why I like the method I used 
with:

        RB_TYPE_PADDING,        /* Left over page padding
                                 * array is ignored
                                 * size is variable depending on
                                 * how much padding is needed
                                 */
        RB_TYPE_TIME_EXTENT,    /* Extent the time delta
                                 * array[0] = time delta (28 .. 59)
                                 * size = 8 bytes
                                 */

Where it is very easy to notice which comment goes with which enum.

-- Steve

--
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/