Messages in this thread | | | Date | Sat, 27 Sep 2008 20:39:12 +0200 | From | Ingo Molnar <> | Subject | Re: [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
| |