From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759492AbcAUNgH (ORCPT ); Thu, 21 Jan 2016 08:36:07 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47721 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759002AbcAUNgD (ORCPT ); Thu, 21 Jan 2016 08:36:03 -0500 Date: Thu, 21 Jan 2016 14:35:58 +0100 From: Jiri Olsa To: Namhyung Kim 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: <20160121133558.GC11238@krava.brq.redhat.com> 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> <20160121125552.GA13893@danjae.kornet> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160121125552.GA13893@danjae.kornet> 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 On Thu, Jan 21, 2016 at 09:55:52PM +0900, Namhyung Kim wrote: SNIP > > > + /* 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. I thought the 'policy' is to handle all allocation failures > > 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.. well, you are adding/duplicating entries now in resort path and that is not just 'sorting only' you allow only sort and tracepoint entries to be added in hierrarych view, so there's no resort needed, but still it could be added in future? not sure it still makes more sense to me to do this in 'addition' path, because you basically add new entries but have no other grounds for this also I might be missing something ;-) thanks, jirka