From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757181AbYIYREW (ORCPT ); Thu, 25 Sep 2008 13:04:22 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1754717AbYIYREN (ORCPT ); Thu, 25 Sep 2008 13:04:13 -0400 Received: from smtp1.linux-foundation.org ([140.211.169.13]:53123 "EHLO smtp1.linux-foundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753551AbYIYREL (ORCPT ); Thu, 25 Sep 2008 13:04:11 -0400 Date: Thu, 25 Sep 2008 10:02:02 -0700 (PDT) From: Linus Torvalds To: Steven Rostedt cc: linux-kernel@vger.kernel.org, Ingo Molnar , Thomas Gleixner , Peter Zijlstra , Andrew Morton , prasad@linux.vnet.ibm.com, Mathieu Desnoyers , "Frank Ch. Eigler" , David Wilder , hch@lst.de, Martin Bligh , Christoph Hellwig , Steven Rostedt Subject: Re: [RFC PATCH 1/2 v2] Unified trace buffer In-Reply-To: <20080925160048.641420646@goodmis.org> Message-ID: References: <20080925155807.158539649@goodmis.org> <20080925160048.641420646@goodmis.org> User-Agent: Alpine 1.10 (LFD 962 2008-03-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 25 Sep 2008, Steven Rostedt wrote: > + > +/** > + * ring_buffer_event_length - return the length of the event > + * @event: the event to get the length of > + * > + * Note, if the event is bigger than 256 bytes, the length > + * can not be held in the shifted 5 bits. The length is then > + * added as a short (unshifted) in the body. The comment seems stale ;) > + > +/** > + * ring_buffer_peek - peek at the next event to be read > + * @iter: The ring buffer iterator > + * @iter_next_cpu: The CPU that the next event belongs on > + * > + * This will return the event that will be read next, but does > + * not increment the iterator. > + */ > +struct ring_buffer_event * > +ring_buffer_peek(struct ring_buffer *buffer, int cpu, u64 *ts) > +{ > + struct ring_buffer_per_cpu *cpu_buffer; > + struct ring_buffer_event *event; > + u64 delta; > + > + cpu_buffer = buffer->buffers[cpu]; > + > + again: > + if (ring_buffer_per_cpu_empty(cpu_buffer)) > + return NULL; > + > + event = ring_buffer_head_event(cpu_buffer); > + > + switch (event->type) { > + case RB_TYPE_PADDING: > + ring_buffer_inc_page(buffer, &cpu_buffer->head_page); > + rb_reset_read_page(cpu_buffer); > + goto again; > + > + case RB_TYPE_TIME_EXTENT: > + delta = event->data; > + delta <<= TS_SHIFT; > + delta += event->time_delta; > + cpu_buffer->read_stamp += delta; > + goto again; > + > + case RB_TYPE_TIME_STAMP: > + /* FIXME: not implemented */ > + goto again; > + > + case RB_TYPE_SMALL_DATA: > + case RB_TYPE_LARGE_DATA: > + case RB_TYPE_STRING: > + if (ts) > + *ts = cpu_buffer->read_stamp + event->time_delta; > + return event; Your timestamp handling seems odd. You do it per-event, but I think it should happen for all events, ie just do *ts += event->time_delta; _outside_ the case statement, and then in RB_TYPE_TIME_EXTENT you'd do either - relative: *ts += event->data << TS_SHIFT; - absolute timestamp events: *ts = (event->data << TS_SHIFT) + event->time_delta; but the bigger issue is that I think the timestamp should be relative to the _previous_ event, not relative to the page start. IOW, you really should accumulate them. IOW, the base timestamp cannot be in the cpu_buffer, it needs to be in the iterator data structure, since it updates as you walk over it. Otherwise the extended TSC format will be _horrible_. You don't want to add it in front of every event in the page just because you had a pause at the beginning of the page. You want to have a running update, so that you only need to add it after there was a pause. Linus