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

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



\
 
 \ /
  Last update: 2008-09-27 21:57    [W:0.169 / U:0.288 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site