Re: [PATCH v9] Unified trace buffer

[Ingo Molnar]

small nitpicking review, nothing structural yet:

* Steven Rostedt <rostedt@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/