From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753617AbcHPPJ4 (ORCPT ); Tue, 16 Aug 2016 11:09:56 -0400 Received: from mail.kernel.org ([198.145.29.136]:47772 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752021AbcHPPJw (ORCPT ); Tue, 16 Aug 2016 11:09:52 -0400 Date: Tue, 16 Aug 2016 12:08:03 -0300 From: Arnaldo Carvalho de Melo To: Namhyung Kim Cc: Jiri Olsa , Linux Kernel Mailing List Subject: Re: RFC: callchain showing same entry as hist_entry Message-ID: <20160816150803.GI20972@kernel.org> References: <20160816143628.GG20972@kernel.org> <20160816145511.GA11382@danjae.aot.lge.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160816145511.GA11382@danjae.aot.lge.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.6.2 (2016-07-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Em Tue, Aug 16, 2016 at 11:55:11PM +0900, Namhyung Kim escreveu: > Hi Arnaldo, > > On Tue, Aug 16, 2016 at 11:36:28AM -0300, Arnaldo Carvalho de Melo wrote: > > Hi Namhyung, Jiri, > > > > please take a look at the patch below, and Ack if possible, it > > is a problem introduced in: > > > > cfaa154b2335 ("perf tools: Get rid of obsolete hist_entry__sort_list") > > > > That is not equivalent to the code that was there and results in having > > the same entry as the first entry for the callchain as in the > > hist_entry, which is annoying and doesn't match the original intent of > > that 'continue' branch, as described in the comment right above it. > > AFAIK the intent was to skip first callchain entry iff the first sort > key is 'symbol'. The sort_order being NULL means it'd use the default > sort key which is 'comm,dso,sym' so it should not skip the first > callchain entry. > > The original code (before cfaa154b2335) was like below: > > - if (!i++ && sort__first_dimension == SORT_SYM) > + if (!i++ && field_order == NULL && > + sort_order && !prefixcmp(sort_order, "sym")) > > I think the current code works as intended, no? Well, see below the cset comment (local one, will change as we discuss), may clarify further, but yeah, we can't take for granted that the default sort order has "sym" in it :-\ So the original intent description was ambiguous, what we want to avoid is what is in the cset comment below, so the logic for that should be a bit different, tho. For starters we need to have a callchain_something__first() that will skip the first one using this logic and then use it in all places in --stdio, --tui, --gtk where we need this. commit c066eaa8d06b87c77b82d55bbc59be308aca4684 Author: Arnaldo Carvalho de Melo Date: Tue Aug 16 11:36:50 2016 -0300 perf callchain: Do not repeat the hist_entry symbol as the 1st callchain entry We were getting: 2.62% [k] __d_lookup_rcu | ---__d_lookup_rcu | --2.52%--lookup_fast | --2.50%--walk_component Noticed the __d_lookup_rcu dup, when the original coder intent, and the saner output is: 2.62% [k] __d_lookup_rcu | --2.52%--lookup_fast | --2.50%--walk_component This is for --stdio, TUI being investigated. Cc: Adrian Hunter Cc: David Ahern Cc: Jiri Olsa Cc: Linus Torvalds Cc: Namhyung Kim Cc: Wang Nan Fixes: cfaa154b2335 ("perf tools: Get rid of obsolete hist_entry__sort_list") Link: http://lkml.kernel.org/r/20160816143628.GG20972@kernel.org Signed-off-by: Arnaldo Carvalho de Melo diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c index 9b65f4a6b35a..46a083e59ce9 100644 --- a/tools/perf/ui/stdio/hist.c +++ b/tools/perf/ui/stdio/hist.c @@ -207,7 +207,7 @@ static size_t callchain__fprintf_graph(FILE *fp, struct rb_root *root, * displayed twice. */ if (!i++ && field_order == NULL && - sort_order && !prefixcmp(sort_order, "sym")) + (sort_order == NULL || !prefixcmp(sort_order, "sym"))) continue; if (!printed) { ret += callchain__fprintf_left_margin(fp, left_margin);