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

    small nitpicking review, nothing structural yet:

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

    > +#define RB_ALIGNMENT_SHIFT 2
    > +#define RB_ALIGNMENT (1 << RB_ALIGNMENT_SHIFT)
    > +#define RB_MAX_SMALL_DATA (28)

    no need to put numeric literals into parenthesis.

    > +static inline unsigned
    > +ring_buffer_event_length(struct ring_buffer_event *event)
    > +{
    > + unsigned length;
    > +
    > + switch (event->type) {
    > + case RB_TYPE_PADDING:
    > + /* undefined */
    > + return -1;
    > +
    > + case RB_TYPE_TIME_EXTENT:
    > + return RB_LEN_TIME_EXTENT;
    > +
    > + case RB_TYPE_TIME_STAMP:
    > + return RB_LEN_TIME_STAMP;
    > +
    > + case RB_TYPE_DATA:
    > + if (event->len)
    > + length = event->len << RB_ALIGNMENT_SHIFT;
    > + else
    > + length = event->array[0];
    > + return length + RB_EVNT_HDR_SIZE;
    > + default:
    > + BUG();
    > + }
    > + /* not hit */
    > + return 0;

    too large, please uninline.

    > +static inline void *
    > +ring_buffer_event_data(struct ring_buffer_event *event)
    > +{
    > + BUG_ON(event->type != RB_TYPE_DATA);
    > + /* If length is in len field, then array[0] has the data */
    > + if (event->len)
    > + return (void *)&event->array[0];
    > + /* Otherwise length is in array[0] and array[1] has the data */
    > + return (void *)&event->array[1];
    > +}

    ditto.

    > +/* FIXME!!! */
    > +u64 ring_buffer_time_stamp(int cpu)
    > +{
    > + /* shift to debug/test normalization and TIME_EXTENTS */
    > + return sched_clock() << DEBUG_SHIFT;

    [ duly noted ;-) ]

    > +}
    > +void ring_buffer_normalize_time_stamp(int cpu, u64 *ts)

    needs extra newline above.

    > +/*
    > + * head_page == tail_page && head == tail then buffer is empty.
    > + */
    > +struct ring_buffer_per_cpu {
    > + int cpu;
    > + struct ring_buffer *buffer;
    > + raw_spinlock_t lock;

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

    > +struct ring_buffer {
    > + unsigned long size;
    > + unsigned pages;
    > + unsigned flags;
    > + int cpus;
    > + cpumask_t cpumask;
    > + atomic_t record_disabled;
    > +
    > + struct mutex mutex;
    > +
    > + struct ring_buffer_per_cpu **buffers;
    > +};
    > +
    > +struct ring_buffer_iter {
    > + struct ring_buffer_per_cpu *cpu_buffer;
    > + unsigned long head;
    > + struct buffer_page *head_page;
    > + u64 read_stamp;

    please use consistent vertical whitespaces. Above, in the struct
    ring_buffer definition, you can add another tab to most of the vars -
    that will also make the '**buffers' line look nice.

    same for all structs across this file. In my experience, a 50% vertical
    break works best - the one you used here in 'struct ring_buffer_iter'.

    > +};
    > +
    > +#define CHECK_COND(buffer, cond) \
    > + if (unlikely(cond)) { \
    > + atomic_inc(&buffer->record_disabled); \
    > + WARN_ON(1); \
    > + return -1; \
    > + }

    please name it RINGBUFFER_BUG_ON() / RINGBUFFER_WARN_ON(), so that we
    dont have to memorize another set of debug names. [ See
    DEBUG_LOCKS_WARN_ON() in include/linux/debug_locks.h ]

    you can change it to:

    > +static int
    > +rb_allocate_pages(struct ring_buffer_per_cpu *cpu_buffer, unsigned nr_pages)
    > +{
    > + struct list_head *head = &cpu_buffer->pages;
    > + LIST_HEAD(pages);
    > + struct buffer_page *page, *tmp;
    > + unsigned long addr;
    > + unsigned i;

    please apply ftrace's standard reverse christmas tree style and move the
    'pages' line down two lines.

    > +int ring_buffer_resize(struct ring_buffer *buffer, unsigned long size)
    > +{
    > + struct ring_buffer_per_cpu *cpu_buffer;
    > + unsigned long buffer_size;
    > + LIST_HEAD(pages);
    > + unsigned long addr;
    > + unsigned nr_pages, rm_pages, new_pages;
    > + struct buffer_page *page, *tmp;
    > + int i, cpu;

    ditto.

    > +static inline void *rb_page_index(struct buffer_page *page, unsigned index)
    > +{
    > + void *addr;
    > +
    > + addr = page_address(&page->page);

    'addr' initialization can move to the definition line - you save two
    lines.

    > + return addr + index;
    > +}
    > +
    > +static inline struct ring_buffer_event *
    > +rb_head_event(struct ring_buffer_per_cpu *cpu_buffer)
    > +{
    > + return rb_page_index(cpu_buffer->head_page,
    > + cpu_buffer->head);

    can all move to the same return line.

    > +}
    > +
    > +static inline struct ring_buffer_event *
    > +rb_iter_head_event(struct ring_buffer_iter *iter)
    > +{
    > + return rb_page_index(iter->head_page,
    > + iter->head);

    ditto.

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

    > +static struct ring_buffer_event *
    > +rb_reserve_next_event(struct ring_buffer_per_cpu *cpu_buffer,
    > + unsigned type, unsigned long length)
    > +{
    > + u64 ts, delta;
    > + struct ring_buffer_event *event;
    > + static int once;
    > +
    > + ts = ring_buffer_time_stamp(cpu_buffer->cpu);
    > +
    > + if (cpu_buffer->tail) {
    > + delta = ts - cpu_buffer->write_stamp;
    > +
    > + if (test_time_stamp(delta)) {
    > + if (unlikely(delta > (1ULL << 59) && !once++)) {
    > + printk(KERN_WARNING "Delta way too big! %llu"
    > + " ts=%llu write stamp = %llu\n",
    > + delta, ts, cpu_buffer->write_stamp);
    > + WARN_ON(1);
    > + }
    > + /*
    > + * The delta is too big, we to add a
    > + * new timestamp.
    > + */
    > + event = __rb_reserve_next(cpu_buffer,
    > + RB_TYPE_TIME_EXTENT,
    > + RB_LEN_TIME_EXTENT,
    > + &ts);
    > + if (!event)
    > + return NULL;
    > +
    > + /* check to see if we went to the next page */
    > + if (cpu_buffer->tail) {
    > + /* Still on same page, update timestamp */
    > + event->time_delta = delta & TS_MASK;
    > + event->array[0] = delta >> TS_SHIFT;
    > + /* commit the time event */
    > + cpu_buffer->tail +=
    > + ring_buffer_event_length(event);
    > + cpu_buffer->write_stamp = ts;
    > + delta = 0;
    > + }
    > + }
    > + } else {
    > + rb_add_stamp(cpu_buffer, &ts);
    > + delta = 0;
    > + }
    > +
    > + event = __rb_reserve_next(cpu_buffer, type, length, &ts);
    > + if (!event)
    > + return NULL;
    > +
    > + /* If the reserve went to the next page, our delta is zero */
    > + if (!cpu_buffer->tail)
    > + delta = 0;
    > +
    > + event->time_delta = delta;
    > +
    > + return event;
    > +}

    this function is too long, please split it up. The first condition's
    body could go into a separate function i guess.

    > + RB_TYPE_TIME_EXTENT, /* Extent the time delta
    > + * array[0] = time delta (28 .. 59)
    > + * size = 8 bytes
    > + */

    please use standard comment style:

    /*
    * Comment
    */

    Ingo


    \
     
     \ /
      Last update: 2008-09-27 20:43    [W:0.040 / U:90.928 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site