linux-kernel - Re: [PATCH 2/5] perf script: extend db-export api to include callchains for samples

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: <5719D914.2000109@intel.com>
Date:	Fri, 22 Apr 2016 10:56:04 +0300
From:	Adrian Hunter <adrian.hunter@...el.com>
To:	Chris Phlipot <cphlipot0@...il.com>, acme@...nel.org,
	mingo@...hat.com, peterz@...radead.org
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/5] perf script: extend db-export api to include
 callchains for samples

On 19/04/16 11:56, Chris Phlipot wrote:
> The current implementation of the python database export API only
> includes call path information when using some form of call/return
> tracing, but is unable to do so when sampling.
> 
> The following API extensions allow exporting of data collected by
> perf record when using --call-graph.
> 
> The additions to the python api include the following:
> - add call_path_id to sample_table to allow association of samples
>   with call paths
> 
> - add dso_id to call_path_table to more closely align the data with that
>   of a callchain_node

The call_paths table already has symbol_id which belongs uniquely to a DSO,
so why do we need dso_id as well?

> 
> db-export and trace-script-python were both extended to accomidate the API

accomidate -> accommodate

> changes listed above.
> 
> Thread-stack's functionality was expanded to allow storage and exporting
> of callchains that result from individual samples.
> 
> - Introduced a new static function (thread_stack__process_callchain) to
>   resolve call paths using the existing callchain resolution provided by
>   thread__resolve_callchain
> 
> - The existing call_path tree in call_return_processor is used for storing
>   the data from the resolved callchain.
> 
> - Call_return_processor was also extended to allow the ability to export
>   full call paths in addtion to the existing individual call/return pairs,
>   since call/return pairs are not available when doing sampling.

Why do you need a callback?  Seems like the only thing you need from
thread-stack.c is the call path tree.  You could move that to its own .c/.h
files and then process the call chain in db-export.c

Also a list of changes like the one above heavily implies you are not
obeying the one patch == one change rule.  Please try to make patches that
only do one thing and also run checkpatch.

If you don't mind, I'll let you respond to my comments before I comment on
any other patches.