Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754111AbcC3TDE (ORCPT ); Wed, 30 Mar 2016 15:03:04 -0400 Received: from mail.anarazel.de ([217.115.131.40]:37888 "EHLO mail.anarazel.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752392AbcC3TDC (ORCPT ); Wed, 30 Mar 2016 15:03:02 -0400 Date: Wed, 30 Mar 2016 21:02:45 +0200 From: Andres Freund To: Namhyung Kim Cc: Arnaldo Carvalho de Melo , Peter Zijlstra , Ingo Molnar , linux-kernel@vger.kernel.org Subject: Re: "perf hists browser: Support flat callchains" appears to have broken parent reporting Message-ID: <20160330190245.GB13305@awork2.anarazel.de> References: <20160330123418.GC12336@awork2.anarazel.de> <20160330134634.GA3420@kernel.org> <20160330141926.GA5242@awork2.anarazel.de> <20160330160010.GA1557@danjae> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160330160010.GA1557@danjae> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3897 Lines: 101 Hi! On 2016-03-31 01:00:10 +0900, Namhyung Kim wrote: > On Wed, Mar 30, 2016 at 04:19:26PM +0200, Andres Freund wrote: > > On 2016-03-30 10:46:34 -0300, Arnaldo Carvalho de Melo wrote: > > > Em Wed, Mar 30, 2016 at 02:34:18PM +0200, Andres Freund escreveu: > > > > Hi, > > > > > > > > 4b3a3212233a - "perf hists browser: Support flat callchains" seems to > > > > have broken callchain display in tui mode when using !flat mode, or at > > > > least changed it in an unintended manner. > > > > > > humm, at first I thought this would be related to --percent-limit... > > > > I'm not using --percent-limit. Just to be sure, I did explicitly set it > > to various values, and it looks unrelated. > > > > > What tree/branch are you using? Can you try pressing 'L' to play with > > > the percent limit? > > > > I'm primarily using linus' tree, and bisected the behavioural down to > > that individual commit. > > Thanks for reporting and finding this! No problem. I'm somewhat surprised to be the first to report this, the behavioural change confused me quite a bit. Maybe it took others about as long as me to figure out it's actually a perf report problem ;) > > I don't know the perf code at all, but skimming through the commit, the > > following hunk looks suspicious: > > > > @@ -263,7 +295,7 @@ static void callchain_node__init_have_children(struct callchain_node *node, > > chain = list_entry(node->val.next, struct callchain_list, list); > > chain->has_children = has_sibling; > > > > - if (!list_empty(&node->val)) { > > + if (node->val.next != node->val.prev) { > > chain = list_entry(node->val.prev, struct callchain_list, list); > > chain->has_children = !RB_EMPTY_ROOT(&node->rb_root); > > } > > > > Reverting that individual change fixes things. I'm not actually sure > > what the post 4b3a3212233a version actually tests for? > > Yeah, this is it. It's my fault that I thought if the first chain > (node->val.next) was set by has_sibling, no need to go to the body > of the "if" statement when next == prev case. But it's not... > Do you mind resending the fix as a formal patch with my ack ? Done. I couldn't really explain the problem, so I handwaved the actual problem away: I've not really grasped how the rbtree, callchain_lists and callchain_node are intertwined. My symptom description probably is inaccurate as well. >From 017d80e3c98c7237183ac20bb1cfc81bd5e9a474 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Wed, 30 Mar 2016 19:39:28 +0200 Subject: [PATCH] perf hists: Fix determination of a callchain node's childlessness. 4b3a3212233a ("perf hists browser: Support flat callchains") over-aggressively tried to optimize callchain_node__init_have_children(). That lead to --tui mode not allowing to expand call chain elements if a call chain element had only one parent. That's why --inverted callgraphs looked halfway sane, but plain ones didn't. Revert that individual optimization, it wasn't really related to the rest of the commit. Fixes: 4b3a3212233a042f48b7b8fedc64933e1ccd8643 Acked-by: Namhyung Kim Signed-off-by: Andres Freund --- tools/perf/ui/browsers/hists.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c index 4b98165..2a83414 100644 --- a/tools/perf/ui/browsers/hists.c +++ b/tools/perf/ui/browsers/hists.c @@ -337,7 +337,7 @@ static void callchain_node__init_have_children(struct callchain_node *node, chain = list_entry(node->val.next, struct callchain_list, list); chain->has_children = has_sibling; - if (node->val.next != node->val.prev) { + if (!list_empty(&node->val)) { chain = list_entry(node->val.prev, struct callchain_list, list); chain->has_children = !RB_EMPTY_ROOT(&node->rb_root); } -- 2.7.0.229.g701fa7f.dirty