From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753168AbcAVKpZ (ORCPT <rfc822;w@1wt.eu>);
	Fri, 22 Jan 2016 05:45:25 -0500
Received: from mail-pa0-f65.google.com ([209.85.220.65]:33985 "EHLO
	mail-pa0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752632AbcAVKpR (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 22 Jan 2016 05:45:17 -0500
Date: Fri, 22 Jan 2016 19:44:17 +0900
From: Namhyung Kim <namhyung@kernel.org>
To: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>, Ingo Molnar <mingo@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>, Jiri Olsa <jolsa@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>, David Ahern <dsahern@gmail.com>,
        Stephane Eranian <eranian@google.com>,
        Andi Kleen <andi@firstfloor.org>, Wang Nan <wangnan0@huawei.com>
Subject: Re: [PATCH 01/17] perf hists: Basic support of hierarchical report
 view
Message-ID: <20160122104417.GB8942@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>
 <20160121125552.GA13893@danjae.kornet>
 <20160121133558.GC11238@krava.brq.redhat.com>
 <20160121140209.GA2312@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20160121140209.GA2312@kernel.org>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Arnaldo,

On Thu, Jan 21, 2016 at 11:02:09AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jan 21, 2016 at 02:35:58PM +0100, Jiri Olsa escreveu:
> > 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;
> 
> Also, can we rename 'new' to new_he? In the past I used 'self' and
> Thomas rightly told me that 'self' didn't convey any info, likewise for
> 'new' (that is even a keyword in C++ and may confuse some syntax
> highligting, etc).

OK


> 
> > > > 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
> 
> yup, if some place doesn't, we need to fix it, silently trowing away
> stuff is not good. At the very least count the number of failures and
> inform the user somewhere on the screen.

OK, will change.


>  
> > > AFAICS current code also can fail in callchain_merge()..
> 
> That needs to be fixed too, then.

OK

Thanks,
Namhyung