linux-kernel - Re: [PATCH 1/3] perf pt: Mark PT return events as "return"

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: <574480AB.2000701@intel.com>
Date:	Tue, 24 May 2016 19:26:19 +0300
From:	Adrian Hunter <adrian.hunter@...el.com>
To:	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Andi Kleen <andi@...stfloor.org>
Cc:	jolsa@...nel.org, linux-kernel@...r.kernel.org,
	Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH 1/3] perf pt: Mark PT return events as "return"

On 24/05/2016 5:57 p.m., Arnaldo Carvalho de Melo wrote:
> Em Mon, May 23, 2016 at 05:52:23PM -0700, Andi Kleen escreveu:
>> From: Andi Kleen <ak@...ux.intel.com>
>>
>> With perf script --itrace=cr we can synthesize calls and returns out of
>> a PT log. However both calls and returns are marked with the same event,
>> called branches. This makes it difficult to read and post process,
>> because calls and returns are somewhat diffferent.
>>
>> Create a separate return event and mark the returns as return.
>
> Adrian, you voiced some concerns about this patch, are those settled?

What about beautifying the sample flags i.e. instead of displaying the
letters interpret them into something more human readable e.g.

bc	call
br	return
bo	conditional jump
b	jump
bci	software interrupt
bri	return from interrupt
bcs	system call
brs	return from system call
by	asynchronous branch
bcyi	hardware interrupt
bA	transaction abort
bB	trace begin
bE	trace end

In Tx (x) can turn up on a number of those too.


> Can I have your Acked-by, please?
>
> - Arnaldo
>
>> Cc: adrian.hunter@...el.com
>> Signed-off-by: Andi Kleen <ak@...ux.intel.com>
>> ---
>>   tools/perf/util/intel-pt.c | 53 +++++++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 48 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
>> index 137196990012..4f0b1d7adf50 100644
>> --- a/tools/perf/util/intel-pt.c
>> +++ b/tools/perf/util/intel-pt.c
>> @@ -82,9 +82,12 @@ struct intel_pt {
>>   	u64 instructions_id;
>>
>>   	bool sample_branches;
>> +	bool sample_returns;
>>   	u32 branches_filter;
>>   	u64 branches_sample_type;
>> +	u64 returns_sample_type;
>>   	u64 branches_id;
>> +	u64 returns_id;
>>
>>   	bool sample_transactions;
>>   	u64 transactions_sample_type;
>> @@ -960,7 +963,8 @@ static int intel_pt_inject_event(union perf_event *event,
>>   	return perf_event__synthesize_sample(event, type, 0, sample, swapped);
>>   }
>>
>> -static int intel_pt_synth_branch_sample(struct intel_pt_queue *ptq)
>> +static int intel_pt_synth_branch_sample(struct intel_pt_queue *ptq,
>> +					bool is_return)
>>   {
>>   	int ret;
>>   	struct intel_pt *pt = ptq->pt;
>> @@ -990,8 +994,13 @@ static int intel_pt_synth_branch_sample(struct intel_pt_queue *ptq)
>>   	sample.pid = ptq->pid;
>>   	sample.tid = ptq->tid;
>>   	sample.addr = ptq->state->to_ip;
>> -	sample.id = ptq->pt->branches_id;
>> -	sample.stream_id = ptq->pt->branches_id;
>> +	if (is_return) {
>> +		sample.id = ptq->pt->returns_id;
>> +		sample.stream_id = ptq->pt->returns_id;
>> +	} else {
>> +		sample.id = ptq->pt->branches_id;
>> +		sample.stream_id = ptq->pt->branches_id;
>> +	}
>>   	sample.period = 1;
>>   	sample.cpu = ptq->cpu;
>>   	sample.flags = ptq->flags;
>> @@ -1014,6 +1023,8 @@ static int intel_pt_synth_branch_sample(struct intel_pt_queue *ptq)
>>
>>   	if (pt->synth_opts.inject) {
>>   		ret = intel_pt_inject_event(event, &sample,
>> +					    is_return ?
>> +					    pt->returns_sample_type :
>>   					    pt->branches_sample_type,
>>   					    pt->synth_needs_swap);
>>   		if (ret)
>> @@ -1241,7 +1252,13 @@ static int intel_pt_sample(struct intel_pt_queue *ptq)
>>   		thread_stack__set_trace_nr(ptq->thread, state->trace_nr);
>>
>>   	if (pt->sample_branches) {
>> -		err = intel_pt_synth_branch_sample(ptq);
>> +		err = intel_pt_synth_branch_sample(ptq, false);
>> +		if (err)
>> +			return err;
>> +	}
>> +
>> +	if (pt->sample_returns) {
>> +		err = intel_pt_synth_branch_sample(ptq, true);
>>   		if (err)
>>   			return err;
>>   	}
>> @@ -1956,7 +1973,33 @@ static int intel_pt_synth_events(struct intel_pt *pt,
>>   		}
>>   		pt->sample_branches = true;
>>   		pt->branches_sample_type = attr.sample_type;
>> -		pt->branches_id = id;
>> +		pt->branches_id = id++;
>> +	}
>> +	if (pt->synth_opts.returns) {
>> +		attr.config = PERF_COUNT_HW_BRANCH_INSTRUCTIONS;
>> +		attr.sample_period = 1;
>> +		attr.sample_type |= PERF_SAMPLE_ADDR;
>> +		attr.sample_type &= ~(u64)PERF_SAMPLE_CALLCHAIN;
>> +		attr.sample_type &= ~(u64)PERF_SAMPLE_BRANCH_STACK;
>> +		pr_debug("Synthesizing 'return' event with id %" PRIu64 " sample type %#" PRIx64 "\n",
>> +			 id, (u64)attr.sample_type);
>> +		err = intel_pt_synth_event(session, &attr, id);
>> +		if (err) {
>> +			pr_err("%s: failed to synthesize 'return' event type\n",
>> +			       __func__);
>> +			return err;
>> +		}
>> +		pt->sample_returns = true;
>> +		pt->returns_sample_type = attr.sample_type;
>> +		pt->returns_id = id;
>> +		evlist__for_each(evlist, evsel) {
>> +			if (evsel->id && evsel->id[0] == pt->returns_id) {
>> +				if (evsel->name)
>> +					zfree(&evsel->name);
>> +				evsel->name = strdup("return");
>> +				break;
>> +			}
>> +		}
>>   	}
>>
>>   	pt->synth_needs_swap = evsel->needs_swap;
>> --
>> 2.5.5