Re: [PATCH 3/3] perf record/report: Process events in order

[Thomas Gleixner]

Ian,

On Tue, 7 Dec 2010, Ian Munsie wrote:
> >      case PERF_RECORD_HEADER_ATTR:
> > -        return ops->attr(event, session);
> > +        /* This updates session->sample_id_all */
> > +        ret = ops->attr(event, session);
> > +        if (ops->ordering_requires_timestamps &&
> > +            ops->ordered_samples && !session->sample_id_all) {
> > +            session->ordered_samples.next_flush = ULLONG_MAX;
> > +            flush_sample_queue(session, ops);
> > +            ops->ordered_samples = false;
> > +        }
> 
> Makes sense. I did something similar in the report layer that I was
> about to send when I saw this email, but this way we have a generic
> solution for other parts of perf that might want this.
> The problem here is that we only get the PERF_RECORD_HEADER_ATTR if perf
> record has been piped somewhere, so running perf record <load>; perf
> report on an unpatched kernel results in the COMM, MMAP, etc events
> being processed last (timestamp -1ULL) and no userspace samples are
> attributed at all:

Ok. We need to treat timestamp ~0ULL the same as timestamp 0ULL then.
 
> > +    event__parse_sample(event, session, &sample);
> > +    if (dump_trace)
> > +        perf_session__print_tstamp(session, event, &sample);
> 
> Moving this here after the dump_printf("%#Lx [%#x]: PERF_RECORD_%s"...
> changes the output of perf report -D in a bad way. Changing the spacing
> in dump_printf can make up for it, or juggle the code around some more.

Crap. I wanted to restrict the sample parsing to the real events w/o
having this magic comparison in place as we filter out the synth stuff
in the switch case already. 

> How do you want to proceed? At this point either version of the patches
> are pretty functionally equivelant. Mine does the perf report -D

Hmm. Arnaldo merged my version already.

> reordering as well, but that isn't really necessary to solve the bug.
> Either way we only have a few minor fixups left.

Having time ordered output of -D needs more than fixing the time stamp
issue. The dump_printf/dump_trace stuff is scattered all over the
place. So that needs more code churn, as you want to output the non
synth events when the ordered queue is drained.

Will send an update with that solved soon.

Thanks,

	tglx

--
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/