| 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
| ||
|
Date: Sat, 27 Sep 2008 20:39:12 +0200
From: Ingo Molnar <mingo@...e.hu>
To: Steven Rostedt <rostedt@...dmis.org>
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
small nitpicking review, nothing structural yet:
* Steven Rostedt <rostedt@...dmis.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
--
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/
Powered by blists - more mailing lists