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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LFD.2.00.1012070942070.2653@localhost6.localdomain6>
Date:	Tue, 7 Dec 2010 11:47:35 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Ian Munsie <imunsie@....ibm.com>
cc:	linux-kernel <linux-kernel@...r.kernel.org>,
	Arnaldo Carvalho de Melo <acme@...stprotocols.net>,
	Ingo Molnar <mingo@...e.hu>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Mike Galbraith <efault@....de>,
	Paul Mackerras <paulus@...ba.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH 3/3] perf record/report: Process events in order

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