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.024 / U:239.340 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site