From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755392AbcBZWoB (ORCPT ); Fri, 26 Feb 2016 17:44:01 -0500 Received: from mail.kernel.org ([198.145.29.136]:54621 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755193AbcBZWoA (ORCPT ); Fri, 26 Feb 2016 17:44:00 -0500 Date: Fri, 26 Feb 2016 19:43:54 -0300 From: Arnaldo Carvalho de Melo To: Namhyung Kim Cc: Ingo Molnar , Peter Zijlstra , Jiri Olsa , LKML , David Ahern , Andi Kleen , Stephane Eranian , Wang Nan Subject: Re: [PATCH 5/5] perf report: Update column width of dynamic entries Message-ID: <20160226224354.GC8720@kernel.org> References: <1456512767-1164-1-git-send-email-namhyung@kernel.org> <1456512767-1164-5-git-send-email-namhyung@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1456512767-1164-5-git-send-email-namhyung@kernel.org> X-Url: http://acmel.wordpress.com 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 Em Sat, Feb 27, 2016 at 03:52:47AM +0900, Namhyung Kim escreveu: > The column width of dynamic entries is updated when comparing hist > entries. However some unique entries can miss the chance to update. So > move the update to output resort stage to make sure every entry will get > called before display. > > To do that, abuse ->sort callback to update the width when the third > argument is NULL. When resorting entries in normal path, it never be > NULL so it should be fine IMHO. > > Before: > > # Overhead ptr / bytes_req / gfp_flags > # .............. .......................................... > # > 37.50% 0xffff8803f7669400 > 37.50% 448 > 37.50% GFP_ATOMIC|GFP_NOWARN|GFP_NOMEMALLOC > 10.42% 0xffff8803f766be00 > 8.33% 96 > 8.33% GFP_ATOMIC|GFP_NOWARN|GFP_NOMEMALLOC > 2.08% 512 > 2.08% GFP_KERNEL|GFP_NOWARN|GFP_REPEAT|GFP <-- here So, I tested this with: perf report ... > /tmp/before apply the patch, and then do > /tmp/after, do the diff, reproduced what you described, slightly different output: [root@felicio ~]# diff -u /tmp/before /tmp/after | grep ^[+-] --- /tmp/before 2016-02-26 19:38:31.321497576 -0300 +++ /tmp/after 2016-02-26 19:39:24.043614934 -0300 -# .............. .................................... +# .............. ........................................ - 0.35% GFP_ATOMIC|GFP_ZERO|GFP_NOMEMA + 0.35% GFP_ATOMIC|GFP_ZERO|GFP_NOMEMALLOC - 0.35% GFP_ATOMIC|GFP_ZERO|GFP_NOMEMA + 0.35% GFP_ATOMIC|GFP_ZERO|GFP_NOMEMALLOC - 0.35% GFP_ATOMIC|GFP_ZERO|GFP_NOMEMA + 0.35% GFP_ATOMIC|GFP_ZERO|GFP_NOMEMALLOC - 0.35% GFP_ATOMIC|GFP_ZERO|GFP_NOMEMA + 0.35% GFP_ATOMIC|GFP_ZERO|GFP_NOMEMALLOC - 0.35% GFP_ATOMIC|GFP_ZERO|GFP_NOMEMA + 0.35% GFP_ATOMIC|GFP_ZERO|GFP_NOMEMALLOC - 0.35% GFP_ATOMIC|GFP_ZERO|GFP_NOMEMA + 0.35% GFP_ATOMIC|GFP_ZERO|GFP_NOMEMALLOC - 0.35% GFP_ATOMIC|GFP_ZERO|GFP_NOMEMA + 0.35% GFP_ATOMIC|GFP_ZERO|GFP_NOMEMALLOC [root@felicio ~]# diff -u /tmp/before /tmp/after | grep ^[+-] This is something else to add to the regression tests, output diff for as many command line combos outputs as we can... Anyway, good work, all applied with Jiri's acks. - Arnaldo > > After: > > # Overhead ptr / bytes_req / gfp_flags > # .............. ..................................................... > # > 37.50% 0xffff8803f7669400 > 37.50% 448 > 37.50% GFP_ATOMIC|GFP_NOWARN|GFP_NOMEMALLOC > 10.42% 0xffff8803f766be00 > 8.33% 96 > 8.33% GFP_ATOMIC|GFP_NOWARN|GFP_NOMEMALLOC > 2.08% 512 > 2.08% GFP_KERNEL|GFP_NOWARN|GFP_REPEAT|GFP_NOMEMALLOC > > Signed-off-by: Namhyung Kim > --- > tools/perf/util/hist.c | 11 +++++++++++ > tools/perf/util/sort.c | 8 +++++--- > 2 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c > index 9b3f582867d6..4b8b67bc0cd8 100644 > --- a/tools/perf/util/hist.c > +++ b/tools/perf/util/hist.c > @@ -1371,6 +1371,10 @@ static void hierarchy_insert_output_entry(struct rb_root *root, > > rb_link_node(&he->rb_node, parent, p); > rb_insert_color(&he->rb_node, root); > + > + /* update column width of dynamic entry */ > + if (perf_hpp__is_dynamic_entry(he->fmt)) > + he->fmt->sort(he->fmt, he, NULL); > } > > static void hists__hierarchy_output_resort(struct hists *hists, > @@ -1440,6 +1444,7 @@ static void __hists__insert_output_entry(struct rb_root *entries, > struct rb_node **p = &entries->rb_node; > struct rb_node *parent = NULL; > struct hist_entry *iter; > + struct perf_hpp_fmt *fmt; > > if (use_callchain) { > if (callchain_param.mode == CHAIN_GRAPH_REL) { > @@ -1466,6 +1471,12 @@ static void __hists__insert_output_entry(struct rb_root *entries, > > rb_link_node(&he->rb_node, parent, p); > rb_insert_color(&he->rb_node, entries); > + > + perf_hpp_list__for_each_sort_list(&perf_hpp_list, fmt) { > + if (perf_hpp__is_dynamic_entry(fmt) && > + perf_hpp__defined_dynamic_entry(fmt, he->hists)) > + fmt->sort(fmt, he, NULL); /* update column width */ > + } > } > > static void output_resort(struct hists *hists, struct ui_progress *prog, > diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c > index 3b1b4018f111..1457c9d975a0 100644 > --- a/tools/perf/util/sort.c > +++ b/tools/perf/util/sort.c > @@ -1819,6 +1819,11 @@ static int64_t __sort__hde_cmp(struct perf_hpp_fmt *fmt, > > hde = container_of(fmt, struct hpp_dynamic_entry, hpp); > > + if (b == NULL) { > + update_dynamic_len(hde, a); > + return 0; > + } > + > field = hde->field; > if (field->flags & FIELD_IS_DYNAMIC) { > unsigned long long dyn; > @@ -1833,9 +1838,6 @@ static int64_t __sort__hde_cmp(struct perf_hpp_fmt *fmt, > } else { > offset = field->offset; > size = field->size; > - > - update_dynamic_len(hde, a); > - update_dynamic_len(hde, b); > } > > return memcmp(a->raw_data + offset, b->raw_data + offset, size); > -- > 2.7.1