linux-kernel - Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

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: <20160121125552.GA13893@danjae.kornet>
Date:	Thu, 21 Jan 2016 21:55:52 +0900
From:	Namhyung Kim <namhyung@...nel.org>
To:	Jiri Olsa <jolsa@...hat.com>
Cc:	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Ingo Molnar <mingo@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Jiri Olsa <jolsa@...nel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	David Ahern <dsahern@...il.com>,
	Stephane Eranian <eranian@...gle.com>,
	Andi Kleen <andi@...stfloor.org>,
	Wang Nan <wangnan0@...wei.com>
Subject: Re: [PATCH 01/17] perf hists: Basic support of hierarchical report
 view

Hi Jiri,

On Thu, Jan 21, 2016 at 11:43:30AM +0100, Jiri Olsa wrote:
> On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> > +}
> > +
> > +static bool hists__hierarchy_insert_entry(struct hists *hists,
> > +					  struct rb_root *root,
> > +					  struct hist_entry *he)
> > +{
> > +	struct perf_hpp_fmt *fmt;
> > +	struct hist_entry *new = NULL;
> > +	struct hist_entry *parent = NULL;
> > +	int depth = 0;
> > +
> > +	perf_hpp__for_each_sort_list(fmt) {
> > +		if (!perf_hpp__is_sort_entry(fmt) &&
> > +		    !perf_hpp__is_dynamic_entry(fmt))
> > +			continue;
> > +
> > +		/* insert copy of 'he' for each fmt into the hierarchy */
> > +		new = hierarchy_insert_entry(hists, root, he, fmt);
> > +		if (new == NULL)
> > +			break;
> 
> so hierarchy_insert_entry can fail because of memory allocation
> but the resort path does not cover any error path because it only
> shuffles entries from in-tree into sorted tree

Yes, memory allocation can fail anywhere.  If it happens, there's not
much thing we can do IMHO - just print warning and bail out.
Currently it silently ignores the allocation error and try to proceed.
But I guess it'll fail soon at other place anyway.

AFAICS current code also can fail in callchain_merge()..

Maybe we can change the return type of this function to int and treat
-1 as an error to detect such cases.


> 
> would it make more sense to do this in 'in-tree addition' path?
> and keep the resort functions to do only resort stuff

I don't follow.  There're 3 path to handle hist entries - let's say
them as 'addition', 'collapsing', and 'resort'.  This function does
the 'collapsing' part - it was originally intended to merge sharable
entries (namely for same 'comm' among different threads).  But I used
it to build a hierarchy since I found it useful as follows:

  1. it requires smaller change than doing it in the 'addition' path
  2. it can reuse current callback-based 'addition' paths so mem- and
     branch-mode can be supported easily (but it needs test..).
  3. the 'addition' path can be parallelized so it'll increase memory
     footprint if it build temporary local hierarchies during the path.

The 'resort' path always do sorting only..

Thanks,
Namhyung