Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1463852 > unrolled thread

RFC: callchain showing same entry as hist_entry

Back to article view | Back to linux.kernel

Contents

  RFC: callchain showing same entry as hist_entry Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-08-16 16:40 +0200
    Re: RFC: callchain showing same entry as hist_entry Jiri Olsa <jolsa@redhat.com> - 2016-08-16 17:00 +0200
    Re: RFC: callchain showing same entry as hist_entry Namhyung Kim <namhyung@kernel.org> - 2016-08-16 17:00 +0200
      Re: RFC: callchain showing same entry as hist_entry Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-08-16 17:20 +0200
        Re: RFC: callchain showing same entry as hist_entry Namhyung Kim <namhyung@kernel.org> - 2016-08-16 17:30 +0200
          Re: RFC: callchain showing same entry as hist_entry Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-08-16 17:40 +0200
            Re: RFC: callchain showing same entry as hist_entry Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-08-16 17:40 +0200
              Re: RFC: callchain showing same entry as hist_entry Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-08-16 17:50 +0200
                Re: RFC: callchain showing same entry as hist_entry Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-08-16 21:30 +0200
                  Re: RFC: callchain showing same entry as hist_entry Namhyung Kim <namhyung@kernel.org> - 2016-08-19 04:30 +0200
              Re: RFC: callchain showing same entry as hist_entry Namhyung Kim <namhyung@kernel.org> - 2016-08-16 17:50 +0200

#1463852 — RFC: callchain showing same entry as hist_entry

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.

Now looking at doing the same for the TUI...

- Arnaldo

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);

[toc] | [next] | [standalone]

#1463871

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.
> 
> Now looking at doing the same for the TUI...

yep, seems ok.. no sort_order -> no first entry

jirka

> 
> - Arnaldo
> 
> 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);

[toc] | [prev] | [next] | [standalone]

#1463872

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?

Thanks,
Namhyung


> 
> Now looking at doing the same for the TUI...
> 
> - Arnaldo
> 
> 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);

[toc] | [prev] | [next] | [standalone]

#1463883

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 <acme@redhat.com>
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 <adrian.hunter@intel.com>
    Cc: David Ahern <dsahern@gmail.com>
    Cc: Jiri Olsa <jolsa@kernel.org>
    Cc: Linus Torvalds <torvalds@linux-foundation.org>
    Cc: Namhyung Kim <namhyung@kernel.org>
    Cc: Wang Nan <wangnan0@huawei.com>
    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 <acme@redhat.com>

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);

[toc] | [prev] | [next] | [standalone]

#1463894

On Tue, Aug 16, 2016 at 12:08:03PM -0300, Arnaldo Carvalho de Melo wrote:
> 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 <acme@redhat.com>
> 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.

Did you run 'perf report -s sym --stdio'?  It seems that current code
already works as sane..  Do you want to make it work for the default sort
key too?

Thanks,
Namhyung


>     
>     Cc: Adrian Hunter <adrian.hunter@intel.com>
>     Cc: David Ahern <dsahern@gmail.com>
>     Cc: Jiri Olsa <jolsa@kernel.org>
>     Cc: Linus Torvalds <torvalds@linux-foundation.org>
>     Cc: Namhyung Kim <namhyung@kernel.org>
>     Cc: Wang Nan <wangnan0@huawei.com>
>     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 <acme@redhat.com>
> 
> 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);

[toc] | [prev] | [next] | [standalone]

#1463895

Em Wed, Aug 17, 2016 at 12:23:55AM +0900, Namhyung Kim escreveu:
> On Tue, Aug 16, 2016 at 12:08:03PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Aug 16, 2016 at 11:55:11PM +0900, Namhyung Kim escreveu:
> > > 
> > > On Tue, Aug 16, 2016 at 11:36:28AM -0300, Arnaldo Carvalho de Melo wrote:
> > > > 	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 <acme@redhat.com>
> > 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.
> 
> Did you run 'perf report -s sym --stdio'?  It seems that current code
> already works as sane..  Do you want to make it work for the default sort
>> key too?

What I want is not to repeat the hist_entry line as the first entry in
the callchain, which is confusing.

I.e. repeat the duplication that is happening with the default sort
entry, that contains "sym".

- Arnaldo

[toc] | [prev] | [next] | [standalone]

#1463896

Em Tue, Aug 16, 2016 at 12:31:43PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Aug 17, 2016 at 12:23:55AM +0900, Namhyung Kim escreveu:
> > Did you run 'perf report -s sym --stdio'?  It seems that current code
> > already works as sane..  Do you want to make it work for the default sort
> >> key too?
> 
> What I want is not to repeat the hist_entry line as the first entry in
> the callchain, which is confusing.
> 
> I.e. repeat the duplication that is happening with the default sort

s/repeat/remove/g

> entry, that contains "sym".
> 
> - Arnaldo

[toc] | [prev] | [next] | [standalone]

#1463902

Em Wed, Aug 17, 2016 at 12:41:18AM +0900, Namhyung Kim escreveu:
> On Tue, Aug 16, 2016 at 12:32:44PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Aug 16, 2016 at 12:31:43PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Wed, Aug 17, 2016 at 12:23:55AM +0900, Namhyung Kim escreveu:
> > > > Did you run 'perf report -s sym --stdio'?  It seems that current code
> > > > already works as sane..  Do you want to make it work for the default sort
> > > >> key too?
> > > 
> > > What I want is not to repeat the hist_entry line as the first entry in
> > > the callchain, which is confusing.
> > > 
> > > I.e. repeat the duplication that is happening with the default sort
> > 
> > s/repeat/remove/g
> > 
> > > entry, that contains "sym".
> 
> Hmm.. if so, wouldn't it be better skipping the first callchain entry
> when the user-given sort key contains "sym" too (not only when it
> starts with 'sym')?

Probably, I think, whatever causes the mentioned duplication. And do
that at all the UIs, hence the suggestion for a callchain__first_node()
or more suitably named routine where such logic would live, to be
used for all the callchain rendering interfaces.

- Arnaldo

[toc] | [prev] | [next] | [standalone]

#1464025

Em Tue, Aug 16, 2016 at 12:46:57PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Aug 17, 2016 at 12:41:18AM +0900, Namhyung Kim escreveu:
> > Hmm.. if so, wouldn't it be better skipping the first callchain entry
> > when the user-given sort key contains "sym" too (not only when it
> > starts with 'sym')?
> 
> Probably, I think, whatever causes the mentioned duplication. And do
> that at all the UIs, hence the suggestion for a callchain__first_node()
> or more suitably named routine where such logic would live, to be
> used for all the callchain rendering interfaces.

So, it is not possible to have that callchain__first_node() one, since
we're dealing with one of the "list" entries, that are in each "node",
etc, we need to do that test when printing the first entry, how about
this one instead?

commit 83b98f4da0068a4aefe49554351f33944dfd03d1
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
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, elide that first entry if "sym" is in
    the sort order, as in the current default sort order:
    
         2.62%  [k] __d_lookup_rcu
                |
                 --2.52%--lookup_fast
                           |
                            --2.50%--walk_component
    
    This is for --stdio, TUI being investigated.
    
    Cc: Adrian Hunter <adrian.hunter@intel.com>
    Cc: David Ahern <dsahern@gmail.com>
    Cc: Jiri Olsa <jolsa@kernel.org>
    Cc: Linus Torvalds <torvalds@linux-foundation.org>
    Cc: Namhyung Kim <namhyung@kernel.org>
    Cc: Wang Nan <wangnan0@huawei.com>
    Link: http://lkml.kernel.org/r/20160816143628.GG20972@kernel.org
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index 9b65f4a6b35a..a894e35bb4d4 100644
--- a/tools/perf/ui/stdio/hist.c
+++ b/tools/perf/ui/stdio/hist.c
@@ -206,8 +206,7 @@ static size_t callchain__fprintf_graph(FILE *fp, struct rb_root *root,
 			 * the symbol. No need to print it otherwise it appears as
 			 * displayed twice.
 			 */
-			if (!i++ && field_order == NULL &&
-			    sort_order && !prefixcmp(sort_order, "sym"))
+			if (!i++ && field_order == NULL && perf_hpp_list.sym)
 				continue;
 			if (!printed) {
 				ret += callchain__fprintf_left_margin(fp, left_margin);

[toc] | [prev] | [next] | [standalone]

#1465913

Hi Arnaldo,

On Tue, Aug 16, 2016 at 04:21:31PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Aug 16, 2016 at 12:46:57PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Wed, Aug 17, 2016 at 12:41:18AM +0900, Namhyung Kim escreveu:
> > > Hmm.. if so, wouldn't it be better skipping the first callchain entry
> > > when the user-given sort key contains "sym" too (not only when it
> > > starts with 'sym')?
> > 
> > Probably, I think, whatever causes the mentioned duplication. And do
> > that at all the UIs, hence the suggestion for a callchain__first_node()
> > or more suitably named routine where such logic would live, to be
> > used for all the callchain rendering interfaces.
> 
> So, it is not possible to have that callchain__first_node() one, since
> we're dealing with one of the "list" entries, that are in each "node",
> etc, we need to do that test when printing the first entry, how about
> this one instead?

Sorry for delay.  If we don't care about the order of 'symbol' sort
key, we need to consider output fields (-F option) too since they'll
be used as sort keys eventually.  Once it sets the perf_hpp_list
fields correctly like below, the condition will be more simplified:

Thanks,
Namhyung


---------------8<-----------------
diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
index 9b65f4a6b35a..0ec907433dc0 100644
--- a/tools/perf/ui/stdio/hist.c
+++ b/tools/perf/ui/stdio/hist.c
@@ -206,8 +206,7 @@ static size_t callchain__fprintf_graph(FILE *fp, struct rb_root *root,
 			 * the symbol. No need to print it otherwise it appears as
 			 * displayed twice.
 			 */
-			if (!i++ && field_order == NULL &&
-			    sort_order && !prefixcmp(sort_order, "sym"))
+			if (!i++ && perf_hpp_list.sym)
 				continue;
 			if (!printed) {
 				ret += callchain__fprintf_left_margin(fp, left_margin);
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 1884d7f9b9d2..073e108d83f9 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1720,6 +1720,19 @@ static int __sort_dimension__add_hpp_output(struct sort_dimension *sd,
 	if (hse == NULL)
 		return -1;
 
+	if (sd->entry == &sort_parent)
+		list->parent = 1;
+	else if (sd->entry == &sort_sym)
+		list->sym = 1;
+	else if (sd->entry == &sort_dso)
+		list->dso = 1;
+	else if (sd->entry == &sort_socket)
+		list->socket = 1;
+	else if (sd->entry == &sort_thread)
+		list->thread = 1;
+	else if (sd->entry == &sort_comm)
+		list->comm = 1;
+
 	perf_hpp_list__column_register(list, &hse->hpp);
 	return 0;
 }

[toc] | [prev] | [next] | [standalone]

#1463903

On Tue, Aug 16, 2016 at 12:32:44PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Aug 16, 2016 at 12:31:43PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Wed, Aug 17, 2016 at 12:23:55AM +0900, Namhyung Kim escreveu:
> > > Did you run 'perf report -s sym --stdio'?  It seems that current code
> > > already works as sane..  Do you want to make it work for the default sort
> > >> key too?
> > 
> > What I want is not to repeat the hist_entry line as the first entry in
> > the callchain, which is confusing.
> > 
> > I.e. repeat the duplication that is happening with the default sort
> 
> s/repeat/remove/g
> 
> > entry, that contains "sym".

Hmm.. if so, wouldn't it be better skipping the first callchain entry
when the user-given sort key contains "sym" too (not only when it
starts with 'sym')?

Thanks,
Namhyung

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web