linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Namhyung Kim <namhyung@kernel.org>
To: Arnaldo Carvalho de Melo <acme@kernel.org>, Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>,
	Ingo Molnar <mingo@kernel.org>, Paul Mackerras <paulus@samba.org>,
	Namhyung Kim <namhyung.kim@lge.com>,
	Namhyung Kim <namhyung@kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	David Ahern <dsahern@gmail.com>, Andi Kleen <andi@firstfloor.org>
Subject: [PATCHSET 0/7] perf tools: Fixup for the --percentage change (v2)
Date: Wed, 23 Apr 2014 16:00:01 +0900	[thread overview]
Message-ID: <1398236408-8856-1-git-send-email-namhyung@kernel.org> (raw)

Hello,

This patchset tries to fix bugs in percentage handling which is
recently changed.  The perf top with symbol filter could cause a
segfault (NULL pointer dereference) if the filter found no entry.

In this patchset, I moved accounting of various histogram stats to be
calculated at the time it actually shown to users.  Although I tested
it on my system for a while, it needs more testing since it'll affect
behaviors of many commands/usages.

There're three main fields in the hists->stats to affect the output:
number of samples, number of hist entries and total periods.

Also there're three stages to process samples: at first, samples are
converted to a hist entry and added to the input tree, and then they are
moved to the collapsed tree if needed, and finally they're moved to the
output tree to be shown to user.

The (part of) stats are accounted when samples are added to the input
tree and then reset before moving to the output tree, and re-counted
during insertion to the output tree.

I can see some reason to do it this way but it's basically not necessary
and could make a problem in multi-threaded programs like perf top.

The perf report does all these passes sequentially in a single thread so
it seems no problem.  But perf top uses two threads - one for gathering
samples (in the input tree) and another for (collapsing and) moving them
to the output tree.  Thus accounting stat in parallel can result in an
inaccurate stats and the output.

So I'd like to get rid of the accounting on the input stage as you can
see it just gets dropped before doing output resort.  I originally make
the all three stats are accounted when doing output resort but changed
mind to account number of samples in the input stage and others in the
output stage.  Because it'd make more sense accounting number of events
(sample event) in the input stage (as all other events are also
accounted in the input stage) and it'd make less changes in code.

So yes, it has a same problem of inaccurate number of samples, but its
impact should be smaller than other stats - seeing increasing sample
count (could be slightly inaccurate) without new entries in the browser.


You can get this on the 'perf/percentage-v11' branch (yes, v11 - as
it's continued from my previous series) in my tree

  git://git.kenrel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git


Any comments, review and testing are welcomed.

Thanks,
Namhyung


Namhyung Kim (7):
  perf report: Count number of entries separately
  perf tools: Account entry stats when it's added to the output tree
  perf hists: Add missing update on filtered stats in
    hists__decay_entries()
  perf ui/tui: Fix off-by-one in hist_browser__update_nr_entries()
  perf ui/tui: Rename hist_browser__update_nr_entries()
  perf top/tui: Update nr_entries properly after a filter is applied
  perf hists/tui: Count callchain rows separately

 tools/perf/builtin-annotate.c  |  3 +-
 tools/perf/builtin-diff.c      | 23 ++++++-----
 tools/perf/builtin-report.c    | 58 ++++++++++++---------------
 tools/perf/ui/browsers/hists.c | 86 ++++++++++++++++++++++++++--------------
 tools/perf/util/hist.c         | 89 +++++++++++++++++++++++++++---------------
 tools/perf/util/hist.h         |  9 ++++-
 6 files changed, 162 insertions(+), 106 deletions(-)

-- 
1.9.2


             reply	other threads:[~2014-04-23  7:00 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-04-23  7:00 Namhyung Kim [this message]
2014-04-23  7:00 ` [PATCH 1/7] perf report: Count number of entries separately Namhyung Kim
2014-04-23  7:00 ` [PATCH 2/7] perf tools: Account entry stats when it's added to the output tree Namhyung Kim
2014-04-23 13:35   ` Jiri Olsa
2014-04-23 14:43     ` Namhyung Kim
2014-04-23  7:00 ` [PATCH 3/7] perf hists: Add missing update on filtered stats in hists__decay_entries() Namhyung Kim
2014-04-23  7:00 ` [PATCH 4/7] perf ui/tui: Fix off-by-one in hist_browser__update_nr_entries() Namhyung Kim
2014-04-23  7:00 ` [PATCH 5/7] perf ui/tui: Rename hist_browser__update_nr_entries() Namhyung Kim
2014-04-23  7:00 ` [PATCH 6/7] perf top/tui: Update nr_entries properly after a filter is applied Namhyung Kim
2014-04-23  7:00 ` [PATCH 7/7] perf hists/tui: Count callchain rows separately Namhyung Kim
2014-04-23 14:29   ` Jiri Olsa
2014-04-23 14:57     ` Namhyung Kim
2014-04-23 15:07       ` Jiri Olsa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1398236408-8856-1-git-send-email-namhyung@kernel.org \
    --to=namhyung@kernel.org \
    --cc=a.p.zijlstra@chello.nl \
    --cc=acme@kernel.org \
    --cc=andi@firstfloor.org \
    --cc=dsahern@gmail.com \
    --cc=jolsa@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=namhyung.kim@lge.com \
    --cc=paulus@samba.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).