[PATCH 0/3] perf top/tui: Fixes for percentage filter behavior

[PATCH 0/3] perf top/tui: Fixes for percentage filter behavior

[Namhyung Kim]

Hi,

This is a small fixes for a bug in perf top I found during some tests.
It gets segfault if symbol filter found no entries - accessing NULL
pointer in that case.

The patches are also available at perf/percentage-v9 branch on my tree
for your convenience.  I'm sorry for causing this bug..

Thanks,
Namhyung


Namhyung Kim (3):
  perf hists: Add missing updates on nr_non_filtered_entries
  perf ui/tui: Fix off-by-one in hist_browser__update_nr_entries()
  perf top/tui: Update nr_entries properly after filter is applied

 tools/perf/ui/browsers/hists.c | 34 +++++++++++++++++++---------------
 tools/perf/util/hist.c         |  8 +++++++-
 tools/perf/util/hist.h         |  6 ++++++
 3 files changed, 32 insertions(+), 16 deletions(-)

-- 
1.9.2

[PATCH 1/3] perf hists: Add missing updates on nr_non_filtered_entries

[Namhyung Kim]

When a filter is used for perf top, its hists->nr_non_filtered_entries
was not updated after applying the filter.  But it needs to be updated
as new samples are captured.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 5a892477aa50..2edbcd717f0e 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -259,8 +259,11 @@ void hists__decay_entries(struct hists *hists, bool zap_user, bool zap_kernel)
 			if (sort__need_collapse)
 				rb_erase(&n->rb_node_in, &hists->entries_collapsed);
 
-			hist_entry__free(n);
 			--hists->nr_entries;
+			if (!n->filtered)
+				--hists->nr_non_filtered_entries;
+
+			hist_entry__free(n);
 		}
 	}
 }
@@ -394,6 +397,9 @@ static struct hist_entry *add_hist_entry(struct hists *hists,
 		return NULL;
 
 	hists->nr_entries++;
+	if (!he->filtered)
+		hists->nr_non_filtered_entries++;
+
 	rb_link_node(&he->rb_node_in, parent, p);
 	rb_insert_color(&he->rb_node_in, hists->entries_in);
 out:
-- 
1.9.2

Re: [PATCH 1/3] perf hists: Add missing updates on nr_non_filtered_entries

[Jiri Olsa]

On Thu, Apr 17, 2014 at 04:53:24PM +0900, Namhyung Kim wrote:
> When a filter is used for perf top, its hists->nr_non_filtered_entries
> was not updated after applying the filter.  But it needs to be updated
> as new samples are captured.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/hist.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 5a892477aa50..2edbcd717f0e 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -259,8 +259,11 @@ void hists__decay_entries(struct hists *hists, bool zap_user, bool zap_kernel)
>  			if (sort__need_collapse)
>  				rb_erase(&n->rb_node_in, &hists->entries_collapsed);
>  
> -			hist_entry__free(n);
>  			--hists->nr_entries;
> +			if (!n->filtered)
> +				--hists->nr_non_filtered_entries;
> +
> +			hist_entry__free(n);
>  		}
>  	}
>  }
> @@ -394,6 +397,9 @@ static struct hist_entry *add_hist_entry(struct hists *hists,
>  		return NULL;
>  
>  	hists->nr_entries++;
> +	if (!he->filtered)
> +		hists->nr_non_filtered_entries++;
> +

I'm probably missing something, but what is hists->nr_non_filtered_entries
used for? I can only see it being inc/dec-ed, but not used..

jirka

>  	rb_link_node(&he->rb_node_in, parent, p);
>  	rb_insert_color(&he->rb_node_in, hists->entries_in);
>  out:
> -- 
> 1.9.2
> 

Re: [PATCH 1/3] perf hists: Add missing updates on nr_non_filtered_entries

[Namhyung Kim]

2014-04-17 (목), 15:10 +0200, Jiri Olsa:
> I'm probably missing something, but what is hists->nr_non_filtered_entries
> used for? I can only see it being inc/dec-ed, but not used..

Well, that's true.  Firstly, I thought that it'd be used for
browser->nr_entries but it couldn't due to --percent-limit.  So I ended
up keeping browser->nr_filtered_entries (looks like to be renamed as
->nr_non_filtered_entries) for that and update it regularly.

Maybe we can just use hists->nr_non_filtered_entries as
browser->nr_entries if --percent-limit is not used.

Thanks,
Namhyung

[PATCH 2/3] perf ui/tui: Fix off-by-one in hist_browser__update_nr_entries()

[Namhyung Kim]

The nr_entries variable is increased inside the loop in the function
but it also counts when hists__filter_entries() returns NULL.  Thus
the end result will have actual count + 1.

It'd become a problem especially there's no entry at all - it'd get a
segfault during referencing a NULL pointer.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 4d416984c59d..e86b95cc55db 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1349,9 +1349,10 @@ static void hist_browser__update_pcnt_entries(struct hist_browser *hb)
 	struct rb_node *nd = rb_first(&hb->hists->entries);
 
 	while (nd) {
-		nr_entries++;
 		nd = hists__filter_entries(rb_next(nd), hb->hists,
 					   hb->min_pcnt);
+		if (nd)
+			nr_entries++;
 	}
 
 	hb->nr_pcnt_entries = nr_entries;
-- 
1.9.2

[PATCH 3/3] perf top/tui: Update nr_entries properly after filter is applied

[Namhyung Kim]

The hist_browser__reset() is only called right after a filter is
applied so it needs to update browser->nr_entries properly.  We cannot
use hists->nr_non_filtered_entries directly since it's possible that
such entries are also filtered out by minimum percentage.

In addition when a filter is used for perf top, hist browser's
nr_entries field was not updated after applying the filter.  But it
needs to be updated as new samples are coming.

Rename ->nr_pcnt_entries and hist_browser__update_pcnt_entries() to
->nr_filtered_entries and hist_browser__update_nr_entries() since it's
now used for filtered entries as well.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 31 +++++++++++++++++--------------
 tools/perf/util/hist.h         |  6 ++++++
 2 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index e86b95cc55db..d98901a7e04f 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -26,13 +26,14 @@ struct hist_browser {
 	int		     print_seq;
 	bool		     show_dso;
 	float		     min_pcnt;
-	u64		     nr_pcnt_entries;
+	u64		     nr_filtered_entries;
 };
 
 extern void hist_browser__init_hpp(void);
 
 static int hists__browser_title(struct hists *hists, char *bf, size_t size,
 				const char *ev_name);
+static void hist_browser__update_nr_entries(struct hist_browser *hb);
 
 static void hist_browser__refresh_dimensions(struct hist_browser *browser)
 {
@@ -43,7 +44,8 @@ static void hist_browser__refresh_dimensions(struct hist_browser *browser)
 
 static void hist_browser__reset(struct hist_browser *browser)
 {
-	browser->b.nr_entries = browser->hists->nr_entries;
+	hist_browser__update_nr_entries(browser);
+	browser->b.nr_entries = browser->nr_filtered_entries;
 	hist_browser__refresh_dimensions(browser);
 	ui_browser__reset_index(&browser->b);
 }
@@ -310,8 +312,6 @@ static void ui_browser__warn_lost_events(struct ui_browser *browser)
 		"Or reduce the sampling frequency.");
 }
 
-static void hist_browser__update_pcnt_entries(struct hist_browser *hb);
-
 static int hist_browser__run(struct hist_browser *browser, const char *ev_name,
 			     struct hist_browser_timer *hbt)
 {
@@ -320,9 +320,11 @@ static int hist_browser__run(struct hist_browser *browser, const char *ev_name,
 	int delay_secs = hbt ? hbt->refresh : 0;
 
 	browser->b.entries = &browser->hists->entries;
-	browser->b.nr_entries = browser->hists->nr_entries;
-	if (browser->min_pcnt)
-		browser->b.nr_entries = browser->nr_pcnt_entries;
+
+	if (hists__has_filter(browser->hists) || browser->min_pcnt)
+		browser->b.nr_entries = browser->nr_filtered_entries;
+	else
+		browser->b.nr_entries = browser->hists->nr_entries;
 
 	hist_browser__refresh_dimensions(browser);
 	hists__browser_title(browser->hists, title, sizeof(title), ev_name);
@@ -339,9 +341,10 @@ static int hist_browser__run(struct hist_browser *browser, const char *ev_name,
 			u64 nr_entries;
 			hbt->timer(hbt->arg);
 
-			if (browser->min_pcnt) {
-				hist_browser__update_pcnt_entries(browser);
-				nr_entries = browser->nr_pcnt_entries;
+			if (hists__has_filter(browser->hists) ||
+			    browser->min_pcnt) {
+				hist_browser__update_nr_entries(browser);
+				nr_entries = browser->nr_filtered_entries;
 			} else {
 				nr_entries = browser->hists->nr_entries;
 			}
@@ -1343,7 +1346,7 @@ close_file_and_continue:
 	return ret;
 }
 
-static void hist_browser__update_pcnt_entries(struct hist_browser *hb)
+static void hist_browser__update_nr_entries(struct hist_browser *hb)
 {
 	u64 nr_entries = 0;
 	struct rb_node *nd = rb_first(&hb->hists->entries);
@@ -1355,7 +1358,7 @@ static void hist_browser__update_pcnt_entries(struct hist_browser *hb)
 			nr_entries++;
 	}
 
-	hb->nr_pcnt_entries = nr_entries;
+	hb->nr_filtered_entries = nr_entries;
 }
 
 static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
@@ -1410,9 +1413,9 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 	if (browser == NULL)
 		return -1;
 
-	if (min_pcnt) {
+	if (hists__has_filter(hists) || min_pcnt) {
 		browser->min_pcnt = min_pcnt;
-		hist_browser__update_pcnt_entries(browser);
+		hist_browser__update_nr_entries(browser);
 	}
 
 	fstack = pstack__new(2);
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 5a0343eb22e2..831faf086eb9 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -128,6 +128,12 @@ void hists__filter_by_dso(struct hists *hists);
 void hists__filter_by_thread(struct hists *hists);
 void hists__filter_by_symbol(struct hists *hists);
 
+static inline bool hists__has_filter(struct hists *hists)
+{
+	return hists->thread_filter || hists->dso_filter ||
+		hists->symbol_filter_str;
+}
+
 u16 hists__col_len(struct hists *hists, enum hist_column col);
 void hists__set_col_len(struct hists *hists, enum hist_column col, u16 len);
 bool hists__new_col_len(struct hists *hists, enum hist_column col, u16 len);
-- 
1.9.2

Re: [PATCH 3/3] perf top/tui: Update nr_entries properly after filter is applied

[Jiri Olsa]

On Thu, Apr 17, 2014 at 04:53:26PM +0900, Namhyung Kim wrote:

SNIP

> -static void hist_browser__update_pcnt_entries(struct hist_browser *hb)
> +static void hist_browser__update_nr_entries(struct hist_browser *hb)
>  {
>  	u64 nr_entries = 0;
>  	struct rb_node *nd = rb_first(&hb->hists->entries);
> @@ -1355,7 +1358,7 @@ static void hist_browser__update_pcnt_entries(struct hist_browser *hb)
>  			nr_entries++;
>  	}
>  
> -	hb->nr_pcnt_entries = nr_entries;
> +	hb->nr_filtered_entries = nr_entries;
>  }
>  
>  static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
> @@ -1410,9 +1413,9 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
>  	if (browser == NULL)
>  		return -1;
>  
> -	if (min_pcnt) {
> +	if (hists__has_filter(hists) || min_pcnt) {

seems like this could be coupled to browser__has_filter function

jirka

Re: [PATCH 3/3] perf top/tui: Update nr_entries properly after filter is applied

[Jiri Olsa]

On Thu, Apr 17, 2014 at 04:53:26PM +0900, Namhyung Kim wrote:
> The hist_browser__reset() is only called right after a filter is
> applied so it needs to update browser->nr_entries properly.  We cannot
> use hists->nr_non_filtered_entries directly since it's possible that
> such entries are also filtered out by minimum percentage.
> 
> In addition when a filter is used for perf top, hist browser's
> nr_entries field was not updated after applying the filter.  But it
> needs to be updated as new samples are coming.
> 
> Rename ->nr_pcnt_entries and hist_browser__update_pcnt_entries() to
> ->nr_filtered_entries and hist_browser__update_nr_entries() since it's
> now used for filtered entries as well.

Could you please split out that rename into separate patch?

thanks,
jirka

Re: [PATCH 3/3] perf top/tui: Update nr_entries properly after filter is applied

[Namhyung Kim]

2014-04-17 (목), 15:12 +0200, Jiri Olsa:
> On Thu, Apr 17, 2014 at 04:53:26PM +0900, Namhyung Kim wrote:
> > The hist_browser__reset() is only called right after a filter is
> > applied so it needs to update browser->nr_entries properly.  We cannot
> > use hists->nr_non_filtered_entries directly since it's possible that
> > such entries are also filtered out by minimum percentage.
> > 
> > In addition when a filter is used for perf top, hist browser's
> > nr_entries field was not updated after applying the filter.  But it
> > needs to be updated as new samples are coming.
> > 
> > Rename ->nr_pcnt_entries and hist_browser__update_pcnt_entries() to
> > ->nr_filtered_entries and hist_browser__update_nr_entries() since it's
> > now used for filtered entries as well.
> 
> Could you please split out that rename into separate patch?

Will do (with browser__has_filter() change).

Thanks,
Namhyung

Re: [PATCH 0/3] perf top/tui: Fixes for percentage filter behavior

[Jiri Olsa]

On Thu, Apr 17, 2014 at 04:53:23PM +0900, Namhyung Kim wrote:
> Hi,
> 
> This is a small fixes for a bug in perf top I found during some tests.
> It gets segfault if symbol filter found no entries - accessing NULL
> pointer in that case.

yep, I hit that yesterday as well, but I ended up just with
the patch below, your changes seems more comprehensive ;-)
going to review..

thanks,
jirka


> 
> The patches are also available at perf/percentage-v9 branch on my tree
> for your convenience.  I'm sorry for causing this bug..
> 
> Thanks,
> Namhyung
> 
> 
> Namhyung Kim (3):
>   perf hists: Add missing updates on nr_non_filtered_entries
>   perf ui/tui: Fix off-by-one in hist_browser__update_nr_entries()
>   perf top/tui: Update nr_entries properly after filter is applied
> 
>  tools/perf/ui/browsers/hists.c | 34 +++++++++++++++++++---------------
>  tools/perf/util/hist.c         |  8 +++++++-
>  tools/perf/util/hist.h         |  6 ++++++
>  3 files changed, 32 insertions(+), 16 deletions(-)
> 
> -- 
> 1.9.2
> 

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 4d41698..89c3ba0 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -866,6 +866,10 @@ static void ui_browser__hists_seek(struct ui_browser *browser,
 		return;
 	}
 
+	/* Nowhere to start from. */
+	if (!nd)
+		return;
+
 	/*
 	 * Moves not relative to the first visible entry invalidates its
 	 * row_offset:

Re: [PATCH 0/3] perf top/tui: Fixes for percentage filter behavior

[Namhyung Kim]

Hi Jiri,

2014-04-17 (목), 14:00 +0200, Jiri Olsa:
> On Thu, Apr 17, 2014 at 04:53:23PM +0900, Namhyung Kim wrote:
> > Hi,
> > 
> > This is a small fixes for a bug in perf top I found during some tests.
> > It gets segfault if symbol filter found no entries - accessing NULL
> > pointer in that case.
> 
> yep, I hit that yesterday as well, but I ended up just with
> the patch below, your changes seems more comprehensive ;-)
> going to review..

Well, the end result will be same, but I think it should have 0
nr_entries when there's no entry to show.

Thanks,
Namhyung

Re: [PATCH 0/3] perf top/tui: Fixes for percentage filter behavior

[Jiri Olsa]

On Thu, Apr 17, 2014 at 10:38:19PM +0900, Namhyung Kim wrote:
> Hi Jiri,
> 
> 2014-04-17 (목), 14:00 +0200, Jiri Olsa:
> > On Thu, Apr 17, 2014 at 04:53:23PM +0900, Namhyung Kim wrote:
> > > Hi,
> > > 
> > > This is a small fixes for a bug in perf top I found during some tests.
> > > It gets segfault if symbol filter found no entries - accessing NULL
> > > pointer in that case.
> > 
> > yep, I hit that yesterday as well, but I ended up just with
> > the patch below, your changes seems more comprehensive ;-)
> > going to review..
> 
> Well, the end result will be same, but I think it should have 0
> nr_entries when there's no entry to show.

also I hit another issue ;-)

- running report on data with callchains, and the screen is populated to the middle
- press 'END' key and the cursor goes to the bottom of the screen (I can tell based
  on the scrollbar), but there are no entries

I could make the cursor appear by pressing UP arrow, I bisected to:
  f214833 perf report: Add --percentage option

not sure where's the problem

thanks,
jirka

Re: [PATCH 0/3] perf top/tui: Fixes for percentage filter behavior

[Namhyung Kim]

Hi Jiri,

On Fri, Apr 18, 2014 at 3:52 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Thu, Apr 17, 2014 at 10:38:19PM +0900, Namhyung Kim wrote:
>> Hi Jiri,
>>
>> 2014-04-17 (목), 14:00 +0200, Jiri Olsa:
>> > On Thu, Apr 17, 2014 at 04:53:23PM +0900, Namhyung Kim wrote:
>> > > Hi,
>> > >
>> > > This is a small fixes for a bug in perf top I found during some tests.
>> > > It gets segfault if symbol filter found no entries - accessing NULL
>> > > pointer in that case.
>> >
>> > yep, I hit that yesterday as well, but I ended up just with
>> > the patch below, your changes seems more comprehensive ;-)
>> > going to review..
>>
>> Well, the end result will be same, but I think it should have 0
>> nr_entries when there's no entry to show.
>
> also I hit another issue ;-)
>
> - running report on data with callchains, and the screen is populated to the middle
> - press 'END' key and the cursor goes to the bottom of the screen (I can tell based
>   on the scrollbar), but there are no entries
>
> I could make the cursor appear by pressing UP arrow, I bisected to:
>   f214833 perf report: Add --percentage option
>
> not sure where's the problem

Hmm.. did you find it when running perf report?  AFAIK there's a
subtle problem of counting number of entries in perf report TUI with
callchains.  I wished fixing it but I was afraid of diving into the
tui code. ;-p  But I think it's a different one to the bug you
reported.

Anyway I found that there's an inconsistency when handling
hists->nr_entries.  We're increasing hists->nr_entries when new sample
comes in (ang not merged into an existing entry), but accesses the
nr_entries before adding it to the hist browser (i.e. output tree).  I
suspect the original perf top code has similar problem already but it
seems my change made it to be triggered easily.

Also when it moves hist entries to the output tree, it resets
nr_entries and increases nr_entries again as they're moved.  I think
it's a problem on perf top since it'll loose the number of entries
already existing on the output tree.  I'm not sure how it has been
working.. maybe I'm missing something.

I'm out of office now, I'll investigate it more when I'm back to
office next week.

Thanks,
Namhyung