linux-kernel - Re: [PATCH 07/18] perf hists: Exchange order of comparing items when collapsing hists

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: <87pq2ryksr.fsf@sejong.aot.lge.com>
Date:	Mon, 03 Dec 2012 10:41:08 +0900
From:	Namhyung Kim <namhyung@...nel.org>
To:	Arnaldo Carvalho de Melo <acme@...stprotocols.net>
Cc:	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Paul Mackerras <paulus@...ba.org>,
	Ingo Molnar <mingo@...nel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Jiri Olsa <jolsa@...hat.com>,
	Stephane Eranian <eranian@...gle.com>,
	Andi Kleen <andi@...stfloor.org>,
	Namhyung Kim <namhyung.kim@....com>
Subject: Re: [PATCH 07/18] perf hists: Exchange order of comparing items when collapsing hists

On Thu, 29 Nov 2012 15:52:57 -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Nov 29, 2012 at 03:38:35PM +0900, Namhyung Kim escreveu:
>> From: Namhyung Kim <namhyung.kim@....com>
>> 
>> When comparing entries for collapsing put the given entry first, and
>> then the iterated entry.  This is for the sake of consistency and will
>
> consistency with what? and, see below:
>
>> be required by the event group report.
>> 
>> Cc: Jiri Olsa <jolsa@...hat.com>
>> Cc: Stephane Eranian <eranian@...gle.com>
>> Signed-off-by: Namhyung Kim <namhyung@...nel.org>
>> ---
>>  tools/perf/util/hist.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
>> index 82df1b26f0d4..161c35e7ed0e 100644
>> --- a/tools/perf/util/hist.c
>> +++ b/tools/perf/util/hist.c
>> @@ -433,7 +433,7 @@ static bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
>>  		parent = *p;
>>  		iter = rb_entry(parent, struct hist_entry, rb_node_in);
>>  
>> -		cmp = hist_entry__collapse(iter, he);
>> +		cmp = hist_entry__collapse(he, iter);
>>  
>>  		if (!cmp) {
>>  			he_stat__add_stat(&iter->stat, &he->stat);
>
> doesn't this now gets inconsistent with the hist_entry__collapse() call?
> I.e. iter first, he after, also there is the case for callchains, below,
> care to elaborate here?

I meant it by consistent with hist_entry__cmp() and didn't consider
he_stat__add_stat and callchain_merge things - thought that they're
other kind of operation.

I needed this change because I introduced hists__{match,link}_collapsed
function in the patch 8 and I found that hist_entry__cmp and
hist_entry__collapse received same kind of arguments in different
order.  Sorry about missing this in the changelog.

However on the second thought, I feel like I don't need those _collapsed
functions at all and perf diff can be converted to use collapsed rb tree
directly instead.  IIUC perf diff use those functions to match entries
by sort keys and do additional resort output rb tree by sort keys (IMHO
the function names - _name_resort and _insert_by_name - are misnomers)
to do the match.

Since output resorting (by period) is only needed for the baseline,
other data files doesn't need to do this additional step.  So I can get
rid of those hists__{match,link}_collapsed functions and change plain
hists__{match,link} functions to use collapsed (or input) rb tree
directly.

Jiri, what do you think?  What am I missing? :)

Thanks,
Namhyung
--
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/