Re: [PATCH 6/6] perf_counter: kerneltop: output event support

[Peter Zijlstra <a.p.zijlstra@chello.nl>]

On Fri, 2009-04-03 at 17:21 -0700, Corey Ashford wrote:
> As I was stealing code from kerneltop today to use in the PAPI profiling 
> implementation, I ran across the code below:
> 
> Peter Zijlstra wrote:
> > Teach kerneltop about the new output ABI.
> > 
> > XXX: anybody fancy integrating the PID/TID data into the output?
> > 
> > Bump the mmap_data pages a little because we bloated the output and
> > have to be more careful about overruns with structured data.
> > 
> > Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > ---
> [snip]
> 
> > 
> > @@ -1147,28 +1152,75 @@ static void mmap_read(struct mmap_data *
> >  	unsigned int head = mmap_read_head(md);
> >  	unsigned int old = md->prev;
> >  	unsigned char *data = md->base + page_size;
> > +	int diff;
> > 
> >  	gettimeofday(&this_read, NULL);
> > 
> > -	if (head - old > md->mask) {
> > +	/*
> > +	 * If we're further behind than half the buffer, there's a chance
> > +	 * the writer will bite our tail and screw up the events under us.
> > +	 *
> > +	 * If we somehow ended up ahead of the head, we got messed up.
> > +	 *
> > +	 * In either case, truncate and restart at head.
> > +	 */
> > +	diff = head - old;
> > +	if (diff > md->mask / 2 || diff < 0) {
> >  		struct timeval iv;
> >  		unsigned long msecs;
> > 
> >  		timersub(&this_read, &last_read, &iv);
> >  		msecs = iv.tv_sec*1000 + iv.tv_usec/1000;
> > 
> > -		fprintf(stderr, "WARNING: failed to keep up with mmap data.  Last read %lu msecs ago.\n", msecs);
> > +		fprintf(stderr, "WARNING: failed to keep up with mmap data."
> > +				"  Last read %lu msecs ago.\n", msecs);
> > 
> [snip]
> 
> The test for diff < 0 looks incorrect to me.  This shouldn't be an 
> error, because it will frequently be the case that the head has wrapped 
> around back to the beginning of the mmap'd pages, while old is near the end.
> 
> What it needs to find out, I think, is if the modulo distance between 
> old and head is greater than 1/2 of the span of the mmap'd pages. 
> Here's a suggested change:
> 
> -	diff = head - old;
> +	diff = (head - old) & md->mask;
> -	if (diff > md->mask / 2 || diff < 0) {
> +	if (diff > md->mask / 2) {
> 
> 
> What do you think?

head and old are both u32 and are monotonically incremented, that means
that (s32)(head - old) < 0 will only be true if old is ahead (or more
than 2^31 behind) of head.

Since mask will be smaller than 2^31 this seemed like a reasonable
integrity test.