From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935156AbcAUM4z (ORCPT ); Thu, 21 Jan 2016 07:56:55 -0500 Received: from mail-pf0-f194.google.com ([209.85.192.194]:35409 "EHLO mail-pf0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934699AbcAUM4v (ORCPT ); Thu, 21 Jan 2016 07:56:51 -0500 Date: Thu, 21 Jan 2016 21:55:52 +0900 From: Namhyung Kim To: Jiri Olsa Cc: Arnaldo Carvalho de Melo , Ingo Molnar , Peter Zijlstra , Jiri Olsa , LKML , David Ahern , Stephane Eranian , Andi Kleen , Wang Nan Subject: Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view Message-ID: <20160121125552.GA13893@danjae.kornet> References: <1452960197-5323-1-git-send-email-namhyung@kernel.org> <1452960197-5323-2-git-send-email-namhyung@kernel.org> <20160121104330.GD13547@krava.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160121104330.GD13547@krava.brq.redhat.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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