perf hists browser: Fix UI bug after fold/unfold

PATCH Superseded He Kuang 4 participants 14 messages Mar 6, 2015 – Mar 14, 2015 perf events

Share & tools

He Kuang

author

2015-03-06 12:55 UTC #0 lore raw

PATCH 1/1 perf hists browser: Fix UI bug after fold/unfold

In perf hists browser, the fold/unfold stat of each hist entry is recorded but hb->nr_callchain_rows loses its value after zoom out and zoom in back. This causes a wrong row cursor range that restrict user to move down anymore.

Before:

  $ perf record -g -e syscalls:* ls
  $ perf report

(select an event and unfold, then zoom out and back, row cursor blocked)

Children Self Command Shared Object Symbol
================================================================
- 100.00% 100.00% ls libuClibc-0.9.33.2.so [.] lstat64
- lstat64

       16.67% 0x6469702e64
       16.67% 0x692d6f732e6f7364
       8.33% 0x442d00746f6f725f
       8.33% 0x746f006469
       8.33% 0x646c6f2e617461
       8.33% 0x646970
       8.33% 0x617461 <= [cursor may stop here, can't move down anymore]
       8.33% 0x7365
       8.33% 0x6469
       8.33% 0x65
  -   16.67%     0.00%  ls       [unknown]              [.]0x6469702e64
     0x6469702e64

When zoom into DSO or thread, nr_rows is not cleared which causes a similar problem.

This patch recalculates hb->nr_callchain_rows and clears nr_rows to fix the bug.

Signed-off-by He Kuang <[email protected]>

---
 tools/perf/ui/browsers/hists.c | 20 ++++++++++++++++++++
 tools/perf/util/hist.c         |  1 +
 2 files changed, 21 insertions(+)

tools/perf/ui/browsers/hists.c +20 -0

index 1106bb8..bef9ab0 100644
@@ -42,6 +42,7 @@ static void hist_browser__update_nr_entries(struct hist_browser *hb);
 
 static struct rb_node *hists__filter_entries(struct rb_node *nd,
 					     float min_pcnt);
+static int __hist_browser__get_folding(struct hist_browser *browser);
 
 static bool hist_browser__has_filter(struct hist_browser *hb)
 {

@@ -57,6 +58,7 @@ static u32 hist_browser__nr_entries(struct hist_browser *hb)
 	else
 		nr_entries = hb->hists->nr_entries;
 
+	hb->nr_callchain_rows = __hist_browser__get_folding(hb);
 	return nr_entries + hb->nr_callchain_rows;
 }

@@ -353,6 +355,24 @@ static void hist_entry__set_folding(struct hist_entry *he, bool unfold)
 		he->nr_rows = 0;
 }
 
+static int
+__hist_browser__get_folding(struct hist_browser *browser)
+{
+	struct rb_node *nd;
+	struct hists *hists = browser->hists;
+	int unfolded_rows = 0;
+
+	for (nd = rb_first(&hists->entries);
+	     (nd = hists__filter_entries(nd, browser->min_pcnt)) != NULL;
+	     nd = rb_next(nd)) {
+		struct hist_entry *he = rb_entry(nd, struct hist_entry, rb_node);
+
+		if (he->ms.unfolded)
+			unfolded_rows += he->nr_rows;
+	}
+	return unfolded_rows;
+}
+
 static void
 __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
 {

tools/perf/util/hist.c +1 -0

index 70b48a6..24aff7d 100644
@@ -1169,6 +1169,7 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
 	/* force fold unfiltered entry for simplicity */
 	h->ms.unfolded = false;
 	h->row_offset = 0;
+	h->nr_rows = 0;
 
 	hists->stats.nr_non_filtered_samples += h->stat.nr_events;

Signature · 1 line

2.2.0.33.gc18b867

Namhyung Kim

maintainer

2015-03-10 06:34 UTC #1 lore raw

Quoted 28 lines · He Kuang

In perf hists browser, the fold/unfold stat of each hist entry is recorded but hb->nr_callchain_rows loses its value after zoom out and zoom in back. This causes a wrong row cursor range that restrict user to move down anymore.
Before:
  $ perf record -g -e syscalls:* ls
  $ perf report
(select an event and unfold, then zoom out and back, row cursor blocked)
Children Self Command Shared Object Symbol
================================================================
- 100.00% 100.00% ls libuClibc-0.9.33.2.so [.] lstat64
- lstat64
       16.67% 0x6469702e64
       16.67% 0x692d6f732e6f7364
       8.33% 0x442d00746f6f725f
       8.33% 0x746f006469
       8.33% 0x646c6f2e617461
       8.33% 0x646970
       8.33% 0x617461 <= [cursor may stop here, can't move down anymore]
       8.33% 0x7365
       8.33% 0x6469
       8.33% 0x65
  -   16.67%     0.00%  ls       [unknown]              [.]0x6469702e64
     0x6469702e64

Quoted 5 lines

When zoom into DSO or thread, nr_rows is not cleared which causes a similar problem.
This patch recalculates hb->nr_callchain_rows and clears nr_rows to fix the bug.

The intention was that it should reset the folding state when a filter is applied. So I guess just resetting he->nr_rows to 0 in hists__ remove_entry_filter() would solve the problem.

Thanks,
Namhyung

Quoted 72 lines

Signed-off-by He Kuang <[email protected]>

---
 tools/perf/ui/browsers/hists.c | 20 ++++++++++++++++++++
 tools/perf/util/hist.c         |  1 +
 2 files changed, 21 insertions(+)

tools/perf/ui/browsers/hists.c +20 -0

index 1106bb8..bef9ab0 100644
@@ -42,6 +42,7 @@ static void hist_browser__update_nr_entries(struct hist_browser *hb);
 
 static struct rb_node *hists__filter_entries(struct rb_node *nd,
 					     float min_pcnt);
+static int __hist_browser__get_folding(struct hist_browser *browser);
 
 static bool hist_browser__has_filter(struct hist_browser *hb)
 {

@@ -57,6 +58,7 @@ static u32 hist_browser__nr_entries(struct hist_browser *hb)
 	else
 		nr_entries = hb->hists->nr_entries;
 
+	hb->nr_callchain_rows = __hist_browser__get_folding(hb);
 	return nr_entries + hb->nr_callchain_rows;
 }

@@ -353,6 +355,24 @@ static void hist_entry__set_folding(struct hist_entry *he, bool unfold)
 		he->nr_rows = 0;
 }
 
+static int
+__hist_browser__get_folding(struct hist_browser *browser)
+{
+	struct rb_node *nd;
+	struct hists *hists = browser->hists;
+	int unfolded_rows = 0;
+
+	for (nd = rb_first(&hists->entries);
+	     (nd = hists__filter_entries(nd, browser->min_pcnt)) != NULL;
+	     nd = rb_next(nd)) {
+		struct hist_entry *he = rb_entry(nd, struct hist_entry, rb_node);
+
+		if (he->ms.unfolded)
+			unfolded_rows += he->nr_rows;
+	}
+	return unfolded_rows;
+}
+
 static void
 __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
 {

tools/perf/util/hist.c +1 -0

index 70b48a6..24aff7d 100644
@@ -1169,6 +1169,7 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
 	/* force fold unfiltered entry for simplicity */
 	h->ms.unfolded = false;
 	h->row_offset = 0;
+	h->nr_rows = 0;
 
 	hists->stats.nr_non_filtered_samples += h->stat.nr_events;

Signature · 7 lines

2.2.0.33.gc18b867

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

He Kuang

author

2015-03-10 07:27 UTC #2 lore raw

hi,

Quoted 32 lines · Namhyung Kim

Quoted 31 lines · He Kuang
In perf hists browser, the fold/unfold stat of each hist entry is recorded but hb->nr_callchain_rows loses its value after zoom out and zoom in back. This causes a wrong row cursor range that restrict user to move down anymore.
Before:
   $ perf record -g -e syscalls:* ls
   $ perf report
(select an event and unfold, then zoom out and back, row cursor blocked)
Children Self Command Shared Object Symbol
================================================================
- 100.00% 100.00% ls libuClibc-0.9.33.2.so [.] lstat64
- lstat64
        16.67% 0x6469702e64
        16.67% 0x692d6f732e6f7364
        8.33% 0x442d00746f6f725f
        8.33% 0x746f006469
        8.33% 0x646c6f2e617461
        8.33% 0x646970
        8.33% 0x617461 <= [cursor may stop here, can't move down anymore]
        8.33% 0x7365
        8.33% 0x6469
        8.33% 0x65
   -   16.67%     0.00%  ls       [unknown]              [.]0x6469702e64
      0x6469702e64
When zoom into DSO or thread, nr_rows is not cleared which causes a similar problem.

Quoted 8 lines

Quoted 2 lines
This patch recalculates hb->nr_callchain_rows and clears nr_rows to fix the bug.
The intention was that it should reset the folding state when a filter is applied. So I guess just resetting he->nr_rows to 0 in hists__ remove_entry_filter() would solve the problem.
Thanks,
Namhyung

Actually, this patch fixes two bugs. The first one is caused by fold/unfold stat not matched with hb->nr_callchain_rows, which is not related to hists__remove_entry_filter. The second one is caused by he->nr_rows not cleared when zoom into DSO or thread.

These two bugs both have a same problem as I described in the chat, so I thought it can be resloved in one patch. What do you think?

Quoted 73 lines

Quoted 72 lines

Signed-off-by He Kuang <[email protected]>

---
  tools/perf/ui/browsers/hists.c | 20 ++++++++++++++++++++
  tools/perf/util/hist.c         |  1 +
  2 files changed, 21 insertions(+)

tools/perf/ui/browsers/hists.c +20 -0

index 1106bb8..bef9ab0 100644
@@ -42,6 +42,7 @@ static void hist_browser__update_nr_entries(struct hist_browser *hb);
 
  static struct rb_node *hists__filter_entries(struct rb_node *nd,
  					     float min_pcnt);
+static int __hist_browser__get_folding(struct hist_browser *browser);
 
  static bool hist_browser__has_filter(struct hist_browser *hb)
  {

@@ -57,6 +58,7 @@ static u32 hist_browser__nr_entries(struct hist_browser *hb)
  	else
  		nr_entries = hb->hists->nr_entries;
 
+	hb->nr_callchain_rows = __hist_browser__get_folding(hb);
  	return nr_entries + hb->nr_callchain_rows;
  }

@@ -353,6 +355,24 @@ static void hist_entry__set_folding(struct hist_entry *he, bool unfold)
  		he->nr_rows = 0;
  }
 
+static int
+__hist_browser__get_folding(struct hist_browser *browser)
+{
+	struct rb_node *nd;
+	struct hists *hists = browser->hists;
+	int unfolded_rows = 0;
+
+	for (nd = rb_first(&hists->entries);
+	     (nd = hists__filter_entries(nd, browser->min_pcnt)) != NULL;
+	     nd = rb_next(nd)) {
+		struct hist_entry *he = rb_entry(nd, struct hist_entry, rb_node);
+
+		if (he->ms.unfolded)
+			unfolded_rows += he->nr_rows;
+	}
+	return unfolded_rows;
+}
+
  static void
  __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
  {

tools/perf/util/hist.c +1 -0

index 70b48a6..24aff7d 100644
@@ -1169,6 +1169,7 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
  	/* force fold unfiltered entry for simplicity */
  	h->ms.unfolded = false;
  	h->row_offset = 0;
+	h->nr_rows = 0;
 
  	hists->stats.nr_non_filtered_samples += h->stat.nr_events;

Signature · 7 lines

2.2.0.33.gc18b867

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Namhyung Kim

maintainer

2015-03-11 00:32 UTC #3 lore raw

Hi,

Quoted 40 lines · He Kuang

hi,
Quoted 38 lines · Namhyung Kim
Quoted 28 lines · He Kuang
In perf hists browser, the fold/unfold stat of each hist entry is recorded but hb->nr_callchain_rows loses its value after zoom out and zoom in back. This causes a wrong row cursor range that restrict user to move down anymore.
Before:
  $ perf record -g -e syscalls:* ls
  $ perf report
(select an event and unfold, then zoom out and back, row cursor blocked)
Children Self Command Shared Object Symbol
================================================================
- 100.00% 100.00% ls libuClibc-0.9.33.2.so [.] lstat64
- lstat64
       16.67% 0x6469702e64
       16.67% 0x692d6f732e6f7364
       8.33% 0x442d00746f6f725f
       8.33% 0x746f006469
       8.33% 0x646c6f2e617461
       8.33% 0x646970
       8.33% 0x617461 <= [cursor may stop here, can't move down anymore]
       8.33% 0x7365
       8.33% 0x6469
       8.33% 0x65
  -   16.67%     0.00%  ls       [unknown]              [.]0x6469702e64
     0x6469702e64
Quoted 5 lines
When zoom into DSO or thread, nr_rows is not cleared which causes a similar problem.
This patch recalculates hb->nr_callchain_rows and clears nr_rows to fix the bug.
The intention was that it should reset the folding state when a filter is applied. So I guess just resetting he->nr_rows to 0 in hists__ remove_entry_filter() would solve the problem.

Quoted 6 lines

Quoted 2 lines
Thanks,
Namhyung
Actually, this patch fixes two bugs. The first one is caused by fold/unfold stat not matched with hb->nr_callchain_rows, which is not related to hists__remove_entry_filter.

So this is not related to zooming, right? Could you please give me a reproduce step then?

Quoted 2 lines

The second one is
caused by he->nr_rows not cleared when zoom into DSO or thread.

This will be solved by resetting he->nr_rows, I guess.

Quoted 4 lines

These two bugs both have a same problem as I described in the chat, so I thought it can be resloved in one patch. What do you think?

I think you'd better to split the fix into two patches with more detailed description and/or reproduce steps.

Thanks,
Namhyung

Quoted 73 lines

Quoted 60 lines

Signed-off-by He Kuang <[email protected]>

---
 tools/perf/ui/browsers/hists.c | 20 ++++++++++++++++++++
 tools/perf/util/hist.c         |  1 +
 2 files changed, 21 insertions(+)

tools/perf/ui/browsers/hists.c +20 -0

index 1106bb8..bef9ab0 100644
@@ -42,6 +42,7 @@ static void hist_browser__update_nr_entries(struct hist_browser *hb);
 static struct rb_node *hists__filter_entries(struct rb_node *nd,
 					     float min_pcnt);
+static int __hist_browser__get_folding(struct hist_browser *browser);
 static bool hist_browser__has_filter(struct hist_browser *hb)
 {

@@ -57,6 +58,7 @@ static u32 hist_browser__nr_entries(struct hist_browser *hb)
 	else
 		nr_entries = hb->hists->nr_entries;
+	hb->nr_callchain_rows = __hist_browser__get_folding(hb);
 	return nr_entries + hb->nr_callchain_rows;
 }

@@ -353,6 +355,24 @@ static void hist_entry__set_folding(struct hist_entry *he, bool unfold)
 		he->nr_rows = 0;
 }
+static int
+__hist_browser__get_folding(struct hist_browser *browser)
+{
+	struct rb_node *nd;
+	struct hists *hists = browser->hists;
+	int unfolded_rows = 0;
+
+	for (nd = rb_first(&hists->entries);
+	     (nd = hists__filter_entries(nd, browser->min_pcnt)) != NULL;
+	     nd = rb_next(nd)) {
+		struct hist_entry *he = rb_entry(nd, struct hist_entry, rb_node);
+
+		if (he->ms.unfolded)
+			unfolded_rows += he->nr_rows;
+	}
+	return unfolded_rows;
+}
+
 static void
 __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
 {

tools/perf/util/hist.c +1 -0

index 70b48a6..24aff7d 100644
@@ -1169,6 +1169,7 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
 	/* force fold unfiltered entry for simplicity */
 	h->ms.unfolded = false;
 	h->row_offset = 0;
+	h->nr_rows = 0;
 	hists->stats.nr_non_filtered_samples += h->stat.nr_events;

Signature · 1 line

2.2.0.33.gc18b867

Quoted 6 lines

Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

He Kuang

author

2015-03-11 12:40 UTC #4 lore raw

PATCH v2 1/2 perf hists browser: Fix UI bug after fold/unfold

This bug can be reproduced as follows:

  $ perf record -g -e syscalls:* ls
  $ perf report

Available samples
================================================================
2 syscalls:sys_enter_mprotect <= [enter one of the entries] 2 syscalls:sys_exit_mprotect
13 syscalls:sys_enter_brk
...

In the hists brower, unfold some of the items, now the cursor can reach to any rows:

Children Self Command Shared Object Symbol
================================================================
- 100.00% 100.00% ls libuClibc-0.9.33.2.so [.] lstat64
- lstat64

       16.67% 0x6469702e64
       8.33% 0x646970
       8.33% 0x617461
       8.33% 0x65
  -   16.67%     0.00%  ls       [unknown]              [.]0x6469702e64
     0x6469702e64 <= [cursor can reach to bottom line, everything is ok]

Now, zoom back to "Available samples" and enter again:

Children Self Command Shared Object Symbol
================================================================
- 100.00% 100.00% ls libuClibc-0.9.33.2.so [.] lstat64
- lstat64

       16.67% 0x6469702e64
       8.33% 0x646970
       8.33% 0x617461 <= [cursor may stop here, can't move down anymore]
       8.33% 0x65
  -   16.67%     0.00%  ls       [unknown]              [.]0x6469702e64
     0x6469702e64

This patch recalculates hb->nr_callchain_rows to fix the bug.

Signed-off-by He Kuang <[email protected]>

---
 tools/perf/ui/browsers/hists.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

tools/perf/ui/browsers/hists.c +20 -0

index 1106bb8..bef9ab0 100644
@@ -42,6 +42,7 @@ static void hist_browser__update_nr_entries(struct hist_browser *hb);
 
 static struct rb_node *hists__filter_entries(struct rb_node *nd,
 					     float min_pcnt);
+static int __hist_browser__get_folding(struct hist_browser *browser);
 
 static bool hist_browser__has_filter(struct hist_browser *hb)
 {

@@ -57,6 +58,7 @@ static u32 hist_browser__nr_entries(struct hist_browser *hb)
 	else
 		nr_entries = hb->hists->nr_entries;
 
+	hb->nr_callchain_rows = __hist_browser__get_folding(hb);
 	return nr_entries + hb->nr_callchain_rows;
 }

@@ -353,6 +355,24 @@ static void hist_entry__set_folding(struct hist_entry *he, bool unfold)
 		he->nr_rows = 0;
 }
 
+static int
+__hist_browser__get_folding(struct hist_browser *browser)
+{
+	struct rb_node *nd;
+	struct hists *hists = browser->hists;
+	int unfolded_rows = 0;
+
+	for (nd = rb_first(&hists->entries);
+	     (nd = hists__filter_entries(nd, browser->min_pcnt)) != NULL;
+	     nd = rb_next(nd)) {
+		struct hist_entry *he = rb_entry(nd, struct hist_entry, rb_node);
+
+		if (he->ms.unfolded)
+			unfolded_rows += he->nr_rows;
+	}
+	return unfolded_rows;
+}
+
 static void
 __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
 {

Signature · 1 line

2.2.0.33.gc18b867

He Kuang

author

2015-03-11 12:40 UTC #5 lore raw

PATCH v2 2/2 perf hists browser: Fix UI bug after zoom into thread/dso/symbol

When zoom into thread/dso/symbol, the fold/unfold stat is cleared in hists__filter_by_thread/dso/symbol(), but h->nr_rows is not cleared. So if we toggle fold stat on the unfold entires, nr_entries got a wrong value.

This bug can be reproduced as follows:

$ perf record -g -e syscalls:sys_enter_open ls
$ perf report

Children Self Command Shared Object Symbol
================================================================

  +   50.00%     0.00%  ls       ld64.so  [.]  _dl_get_ready_to_run
  -   50.00%     0.00%  ls       ld64.so  [.]  _dl_load_shared_library

_dl_load_shared_library <= [Zoom into thread/dso] _dl_get_ready_to_run
_start
...

In the new thread hists, all entries reset to fold, if we unfold the same entry as we previously unfolded, nr_entries got wrong value, and we can't move down cursor to bottom row.

Thread: ls
Children Self Command Shared Object Symbol
================================================================

  +   50.00%     0.00%  ls       ld64.so  [.]  _dl_get_ready_to_run
  -   50.00%     0.00%  ls       ld64.so  [.]  _dl_load_shared_library

_dl_load_shared_library
_dl_get_ready_to_run <= [cursor may stop here, can't move down] _start
...

This patch clear h->nr_rows to fix this bug.

Signed-off-by He Kuang <[email protected]>

---
 tools/perf/util/hist.c | 1 +
 1 file changed, 1 insertion(+)

tools/perf/util/hist.c +1 -0

index 70b48a6..24aff7d 100644
@@ -1169,6 +1169,7 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
 	/* force fold unfiltered entry for simplicity */
 	h->ms.unfolded = false;
 	h->row_offset = 0;
+	h->nr_rows = 0;
 
 	hists->stats.nr_non_filtered_samples += h->stat.nr_events;

Signature · 1 line

2.2.0.33.gc18b867

Arnaldo Carvalho de Melo

maintainer

2015-03-11 14:14 UTC #6 lore raw

Re: [PATCHv2 2/2] perf hists browser: Fix UI bug after zoom into thread/dso/symbol

Em Wed, Mar 11, 2015 at 08:36:03PM +0800, He Kuang escreveu:

Quoted 9 lines

When zoom into thread/dso/symbol, the fold/unfold stat is cleared in hists__filter_by_thread/dso/symbol(), but h->nr_rows is not cleared. So if we toggle fold stat on the unfold entires, nr_entries got a wrong value.
This bug can be reproduced as follows:
$ perf record -g -e syscalls:sys_enter_open ls
$ perf report

Thanks, bug reproduced and seems fixed after the patch, applying,

- Arnaldo

Quoted 44 lines

Children Self Command Shared Object Symbol
================================================================
  +   50.00%     0.00%  ls       ld64.so  [.]  _dl_get_ready_to_run
  -   50.00%     0.00%  ls       ld64.so  [.]  _dl_load_shared_library
_dl_load_shared_library <= [Zoom into thread/dso] _dl_get_ready_to_run
_start
...
In the new thread hists, all entries reset to fold, if we unfold the same entry as we previously unfolded, nr_entries got wrong value, and we can't move down cursor to bottom row.
Thread: ls
Children Self Command Shared Object Symbol
================================================================
  +   50.00%     0.00%  ls       ld64.so  [.]  _dl_get_ready_to_run
  -   50.00%     0.00%  ls       ld64.so  [.]  _dl_load_shared_library
_dl_load_shared_library
_dl_get_ready_to_run <= [cursor may stop here, can't move down] _start
...
This patch clear h->nr_rows to fix this bug.
Signed-off-by He Kuang <[email protected]>
---
 tools/perf/util/hist.c | 1 +
 1 file changed, 1 insertion(+)
tools/perf/util/hist.c +1 -0
index 70b48a6..24aff7d 100644
@@ -1169,6 +1169,7 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
 	/* force fold unfiltered entry for simplicity */
 	h->ms.unfolded = false;
 	h->row_offset = 0;
+	h->nr_rows = 0;
 
 	hists->stats.nr_non_filtered_samples += h->stat.nr_events;
 
Signature · 1 line
2.2.0.33.gc18b867

Namhyung Kim

maintainer Acked-by on v2 2/2

2015-03-12 08:11 UTC #7 lore raw

Re: [PATCHv2 2/2] perf hists browser: Fix UI bug after zoom into thread/dso/symbol

In reply to He Kuang

Quoted 28 lines · He Kuang

When zoom into thread/dso/symbol, the fold/unfold stat is cleared in hists__filter_by_thread/dso/symbol(), but h->nr_rows is not cleared. So if we toggle fold stat on the unfold entires, nr_entries got a wrong value.
This bug can be reproduced as follows:
$ perf record -g -e syscalls:sys_enter_open ls
$ perf report
Children Self Command Shared Object Symbol
================================================================
  +   50.00%     0.00%  ls       ld64.so  [.]  _dl_get_ready_to_run
  -   50.00%     0.00%  ls       ld64.so  [.]  _dl_load_shared_library
_dl_load_shared_library <= [Zoom into thread/dso] _dl_get_ready_to_run
_start
...
In the new thread hists, all entries reset to fold, if we unfold the same entry as we previously unfolded, nr_entries got wrong value, and we can't move down cursor to bottom row.
Thread: ls
Children Self Command Shared Object Symbol
================================================================
  +   50.00%     0.00%  ls       ld64.so  [.]  _dl_get_ready_to_run
  -   50.00%     0.00%  ls       ld64.so  [.]  _dl_load_shared_library

Quoted 8 lines

_dl_load_shared_library
_dl_get_ready_to_run <= [cursor may stop here, can't move down] _start
...
This patch clear h->nr_rows to fix this bug.
Signed-off-by He Kuang <[email protected]>

Acked-by Namhyung Kim <[email protected]>

Thanks,
Namhyung

Quoted 24 lines

---
 tools/perf/util/hist.c | 1 +
 1 file changed, 1 insertion(+)

tools/perf/util/hist.c +1 -0

index 70b48a6..24aff7d 100644
@@ -1169,6 +1169,7 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
 	/* force fold unfiltered entry for simplicity */
 	h->ms.unfolded = false;
 	h->row_offset = 0;
+	h->nr_rows = 0;
 
 	hists->stats.nr_non_filtered_samples += h->stat.nr_events;

Signature · 7 lines

2.2.0.33.gc18b867

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

tip-bot for He Kuang

merge bot

2015-03-14 07:05 UTC #8 lore raw

[tip:perf/core] perf hists browser: Fix UI bug after zoom into thread/dso/symbol

In reply to He Kuang

perf hists browser: Fix UI bug after zoom into thread/dso/symbol

Commit-ID:  a8cd1f4393032cd87e98803346865cdbceb15ad3
Gitweb:     http://git.kernel.org/tip/a8cd1f4393032cd87e98803346865cdbceb15ad3
Author:     He Kuang <[email protected]>
AuthorDate: Wed, 11 Mar 2015 20:36:03 +0800
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 12 Mar 2015 12:39:59 -0300

perf hists browser: Fix UI bug after zoom into thread/dso/symbol

This bug can be reproduced as follows:

$ perf record -g -e syscalls:sys_enter_open ls
$ perf report

Children Self Command Shared Object Symbol
================================================================

  +   50.00%     0.00%  ls       ld64.so  [.]  _dl_get_ready_to_run
  -   50.00%     0.00%  ls       ld64.so  [.]  _dl_load_shared_library

_dl_load_shared_library <= [Zoom into thread/dso] _dl_get_ready_to_run
_start
...

In the new thread hists, all entries reset to fold, if we unfold the same entry as we previously unfolded, nr_entries got wrong value, and we can't move down cursor to bottom row.

Thread: ls
Children Self Command Shared Object Symbol
================================================================

  +   50.00%     0.00%  ls       ld64.so  [.]  _dl_get_ready_to_run
  -   50.00%     0.00%  ls       ld64.so  [.]  _dl_load_shared_library

_dl_load_shared_library
_dl_get_ready_to_run <= [cursor may stop here, can't move down] _start
...

This patch clear h->nr_rows to fix this bug.

Signed-off-by He Kuang <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Wang Nan <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/util/hist.c | 1 +
 1 file changed, 1 insertion(+)

tools/perf/util/hist.c +1 -0

index 95f5ab7..d9a6d35 100644
@@ -1171,6 +1171,7 @@ static void hists__remove_entry_filter(struct hists *hists, struct hist_entry *h
 	/* force fold unfiltered entry for simplicity */
 	h->ms.unfolded = false;
 	h->row_offset = 0;
+	h->nr_rows = 0;
 
 	hists->stats.nr_non_filtered_samples += h->stat.nr_events;

Arnaldo Carvalho de Melo

maintainer

2015-03-11 13:47 UTC #9 lore raw

In reply to He Kuang

Em Wed, Mar 11, 2015 at 08:36:02PM +0800, He Kuang escreveu:

Quoted 37 lines

In perf hists browser, the fold/unfold stat of each hist entry is recorded but hb->nr_callchain_rows loses its value after zoom out and zoom in back. This causes a wrong row cursor range that restrict user to move down anymore.
This bug can be reproduced as follows:
  $ perf record -g -e syscalls:* ls
  $ perf report
Available samples
================================================================
2 syscalls:sys_enter_mprotect <= [enter one of the entries] 2 syscalls:sys_exit_mprotect
13 syscalls:sys_enter_brk
...
In the hists brower, unfold some of the items, now the cursor can reach to any rows:
Children Self Command Shared Object Symbol
================================================================
- 100.00% 100.00% ls libuClibc-0.9.33.2.so [.] lstat64
- lstat64
       16.67% 0x6469702e64
       8.33% 0x646970
       8.33% 0x617461
       8.33% 0x65
  -   16.67%     0.00%  ls       [unknown]              [.]0x6469702e64
     0x6469702e64 <= [cursor can reach to bottom line, everything is ok]
Now, zoom back to "Available samples" and enter again:
Children Self Command Shared Object Symbol
================================================================
- 100.00% 100.00% ls libuClibc-0.9.33.2.so [.] lstat64
- lstat64

Quoted 8 lines

       16.67% 0x6469702e64
       8.33% 0x646970
       8.33% 0x617461 <= [cursor may stop here, can't move down anymore]
       8.33% 0x65
  -   16.67%     0.00%  ls       [unknown]              [.]0x6469702e64
     0x6469702e64

This patch recalculates hb->nr_callchain_rows to fix the bug.

Two nits below:

Quoted 10 lines

Signed-off-by He Kuang <[email protected]>
---
 tools/perf/ui/browsers/hists.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
tools/perf/ui/browsers/hists.c +0 -0
index 1106bb8..bef9ab0 100644

tools/perf/ui/browsers/hists.c static void hist_browser__update_nr_entries(struct hist_browser *hb); · line 45

Quoted 4 lines

tools/perf/ui/browsers/hists.c +1 -0

@@ -42,6 +42,7 @@ static void hist_browser__update_nr_entries(struct hist_browser *hb);
 
 static struct rb_node *hists__filter_entries(struct rb_node *nd,
 					     float min_pcnt);
+static int __hist_browser__get_folding(struct hist_browser *browser);

Why not move the whole function body to...

tools/perf/ui/browsers/hists.c static void hist_browser__update_nr_entries(struct hist_browser *hb); · line 48

Quoted 3 lines

tools/perf/ui/browsers/hists.c +0 -0

 static bool hist_browser__has_filter(struct hist_browser *hb)
 {

Just before the only caller, i.e. here and...

Quoted 9 lines

tools/perf/ui/browsers/hists.c +1 -0

@@ -57,6 +58,7 @@ static u32 hist_browser__nr_entries(struct hist_browser *hb)
 	else
 		nr_entries = hb->hists->nr_entries;
 
+	hb->nr_callchain_rows = __hist_browser__get_folding(hb);
 	return nr_entries + hb->nr_callchain_rows;
 }

tools/perf/ui/browsers/hists.c static void hist_entry__set_folding(struct hist_entry *he, bool unfold) · line 359

Quoted 5 lines

tools/perf/ui/browsers/hists.c +2 -0

@@ -353,6 +355,24 @@ static void hist_entry__set_folding(struct hist_entry *he, bool unfold)
 		he->nr_rows = 0;
 }
 
+static int
+__hist_browser__get_folding(struct hist_browser *browser)

Why use the __ prefix since there is no hist_browser__get_folding that calls it and does something more?

Summary: please rename it to hist_browser__get_folding() and move it to just before its only caller.

Also, besides these nits, are you Ok with this now Namhyung?

Thanks a lot for working on this!

- Arnaldo

Quoted 21 lines

tools/perf/ui/browsers/hists.c +16 -0

+{
+	struct rb_node *nd;
+	struct hists *hists = browser->hists;
+	int unfolded_rows = 0;
+
+	for (nd = rb_first(&hists->entries);
+	     (nd = hists__filter_entries(nd, browser->min_pcnt)) != NULL;
+	     nd = rb_next(nd)) {
+		struct hist_entry *he = rb_entry(nd, struct hist_entry, rb_node);
+
+		if (he->ms.unfolded)
+			unfolded_rows += he->nr_rows;
+	}
+	return unfolded_rows;
+}
+
 static void
 __hist_browser__set_folding(struct hist_browser *browser, bool unfold)
 {

Signature · 1 line

2.2.0.33.gc18b867

He Kuang

author

2015-03-12 07:25 UTC #10 lore raw

PATCH v3 1/1 perf hists browser: Fix UI bug after fold/unfold

This bug can be reproduced as follows:

  $ perf record -g -e syscalls:* ls
  $ perf report

In the hists brower, unfold some of the items, now the cursor can reach to any rows:

Children Self Command Shared Object Symbol
================================================================
- 100.00% 100.00% ls libuClibc-0.9.33.2.so [.] lstat64
- lstat64

       16.67% 0x6469702e64
       8.33% 0x646970
       8.33% 0x617461
       8.33% 0x65
  -   16.67%     0.00%  ls       [unknown]              [.]0x6469702e64
     0x6469702e64 <= [cursor can reach to bottom line, everything is ok]

Now, zoom back to "Available samples" and enter again:

Children Self Command Shared Object Symbol
================================================================
- 100.00% 100.00% ls libuClibc-0.9.33.2.so [.] lstat64
- lstat64

       16.67% 0x6469702e64
       8.33% 0x646970
       8.33% 0x617461 <= [cursor may stop here, can't move down anymore]
       8.33% 0x65
  -   16.67%     0.00%  ls       [unknown]              [.]0x6469702e64
     0x6469702e64

This patch recalculates hb->nr_callchain_rows to fix the bug.

Signed-off-by He Kuang <[email protected]>

---
 tools/perf/ui/browsers/hists.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

tools/perf/ui/browsers/hists.c +19 -0

index 1106bb8..0856209 100644
@@ -48,6 +48,24 @@ static bool hist_browser__has_filter(struct hist_browser *hb)
 	return hists__has_filter(hb->hists) || hb->min_pcnt;
 }
 
+static int hist_browser__get_folding(struct hist_browser *browser)
+{
+	struct rb_node *nd;
+	struct hists *hists = browser->hists;
+	int unfolded_rows = 0;
+
+	for (nd = rb_first(&hists->entries);
+	     (nd = hists__filter_entries(nd, browser->min_pcnt)) != NULL;
+	     nd = rb_next(nd)) {
+		struct hist_entry *he =
+			rb_entry(nd, struct hist_entry, rb_node);
+
+		if (he->ms.unfolded)
+			unfolded_rows += he->nr_rows;
+	}
+	return unfolded_rows;
+}
+
 static u32 hist_browser__nr_entries(struct hist_browser *hb)
 {
 	u32 nr_entries;

@@ -57,6 +75,7 @@ static u32 hist_browser__nr_entries(struct hist_browser *hb)
 	else
 		nr_entries = hb->hists->nr_entries;
 
+	hb->nr_callchain_rows = hist_browser__get_folding(hb);
 	return nr_entries + hb->nr_callchain_rows;
 }

Signature · 1 line

2.2.0.33.gc18b867

Namhyung Kim

maintainer Acked-by on v3 1/1

2015-03-12 07:56 UTC #11 lore raw

Quoted 44 lines · He Kuang

In perf hists browser, the fold/unfold stat of each hist entry is recorded but hb->nr_callchain_rows loses its value after zoom out and zoom in back. This causes a wrong row cursor range that restrict user to move down anymore.
This bug can be reproduced as follows:
  $ perf record -g -e syscalls:* ls
  $ perf report
Available samples
================================================================
2 syscalls:sys_enter_mprotect <= [enter one of the entries] 2 syscalls:sys_exit_mprotect
13 syscalls:sys_enter_brk
...
In the hists brower, unfold some of the items, now the cursor can reach to any rows:
Children Self Command Shared Object Symbol
================================================================
- 100.00% 100.00% ls libuClibc-0.9.33.2.so [.] lstat64
- lstat64
       16.67% 0x6469702e64
       8.33% 0x646970
       8.33% 0x617461
       8.33% 0x65
  -   16.67%     0.00%  ls       [unknown]              [.]0x6469702e64
     0x6469702e64 <= [cursor can reach to bottom line, everything is ok]
Now, zoom back to "Available samples" and enter again:
Children Self Command Shared Object Symbol
================================================================
- 100.00% 100.00% ls libuClibc-0.9.33.2.so [.] lstat64
- lstat64
       16.67% 0x6469702e64
       8.33% 0x646970
       8.33% 0x617461 <= [cursor may stop here, can't move down anymore]
       8.33% 0x65
  -   16.67%     0.00%  ls       [unknown]              [.]0x6469702e64
     0x6469702e64

Quoted 3 lines

This patch recalculates hb->nr_callchain_rows to fix the bug.
Signed-off-by He Kuang <[email protected]>

Acked-by Namhyung Kim <[email protected]>

Thanks,
Namhyung

Quoted 49 lines

---
 tools/perf/ui/browsers/hists.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

tools/perf/ui/browsers/hists.c +19 -0

index 1106bb8..0856209 100644
@@ -48,6 +48,24 @@ static bool hist_browser__has_filter(struct hist_browser *hb)
 	return hists__has_filter(hb->hists) || hb->min_pcnt;
 }
 
+static int hist_browser__get_folding(struct hist_browser *browser)
+{
+	struct rb_node *nd;
+	struct hists *hists = browser->hists;
+	int unfolded_rows = 0;
+
+	for (nd = rb_first(&hists->entries);
+	     (nd = hists__filter_entries(nd, browser->min_pcnt)) != NULL;
+	     nd = rb_next(nd)) {
+		struct hist_entry *he =
+			rb_entry(nd, struct hist_entry, rb_node);
+
+		if (he->ms.unfolded)
+			unfolded_rows += he->nr_rows;
+	}
+	return unfolded_rows;
+}
+
 static u32 hist_browser__nr_entries(struct hist_browser *hb)
 {
 	u32 nr_entries;

@@ -57,6 +75,7 @@ static u32 hist_browser__nr_entries(struct hist_browser *hb)
 	else
 		nr_entries = hb->hists->nr_entries;
 
+	hb->nr_callchain_rows = hist_browser__get_folding(hb);
 	return nr_entries + hb->nr_callchain_rows;
 }

Signature · 7 lines

2.2.0.33.gc18b867

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Arnaldo Carvalho de Melo

maintainer Applied

2015-03-12 16:19 UTC #12 lore raw

Em Thu, Mar 12, 2015 at 04:51:10PM +0900, Namhyung Kim escreveu:

Quoted 6 lines · He Kuang

Quoted 3 lines
This patch recalculates hb->nr_callchain_rows to fix the bug.
Signed-off-by He Kuang <[email protected]>
Acked-by Namhyung Kim <[email protected]>

Thanks, applied.

- Arnaldo

tip-bot for He Kuang

merge bot

2015-03-14 07:05 UTC #13 lore raw

In reply to He Kuang

perf hists browser: Fix UI bug after fold/unfold

Commit-ID:  4fabf3d19cec11d9faa567f8cf0290298c5a93ea
Gitweb:     http://git.kernel.org/tip/4fabf3d19cec11d9faa567f8cf0290298c5a93ea
Author:     He Kuang <[email protected]>
AuthorDate: Thu, 12 Mar 2015 15:21:49 +0800
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 12 Mar 2015 13:18:38 -0300

perf hists browser: Fix UI bug after fold/unfold

This bug can be reproduced as follows:

  $ perf record -g -e syscalls:* ls
  $ perf report

In the hists brower, unfold some of the items, now the cursor can reach to any rows:

Children Self Command Shared Object Symbol
================================================================
- 100.00% 100.00% ls libuClibc-0.9.33.2.so [.] lstat64
- lstat64

       16.67% 0x6469702e64
       8.33% 0x646970
       8.33% 0x617461
       8.33% 0x65
  -   16.67%     0.00%  ls       [unknown]              [.]0x6469702e64
     0x6469702e64 <= [cursor can reach to bottom line, everything is ok]

Now, zoom back to "Available samples" and enter again:

Children Self Command Shared Object Symbol
================================================================
- 100.00% 100.00% ls libuClibc-0.9.33.2.so [.] lstat64
- lstat64

       16.67% 0x6469702e64
       8.33% 0x646970
       8.33% 0x617461 <= [cursor may stop here, can't move down anymore]
       8.33% 0x65
  -   16.67%     0.00%  ls       [unknown]              [.]0x6469702e64
     0x6469702e64

This patch recalculates hb->nr_callchain_rows to fix the bug.

Signed-off-by He Kuang <[email protected]>

Acked-by Namhyung Kim <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Wang Nan <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/ui/browsers/hists.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

tools/perf/ui/browsers/hists.c +19 -0

index ad312d9..49eddeb 100644
@@ -48,6 +48,24 @@ static bool hist_browser__has_filter(struct hist_browser *hb)
 	return hists__has_filter(hb->hists) || hb->min_pcnt;
 }
 
+static int hist_browser__get_folding(struct hist_browser *browser)
+{
+	struct rb_node *nd;
+	struct hists *hists = browser->hists;
+	int unfolded_rows = 0;
+
+	for (nd = rb_first(&hists->entries);
+	     (nd = hists__filter_entries(nd, browser->min_pcnt)) != NULL;
+	     nd = rb_next(nd)) {
+		struct hist_entry *he =
+			rb_entry(nd, struct hist_entry, rb_node);
+
+		if (he->ms.unfolded)
+			unfolded_rows += he->nr_rows;
+	}
+	return unfolded_rows;
+}
+
 static u32 hist_browser__nr_entries(struct hist_browser *hb)
 {
 	u32 nr_entries;

@@ -57,6 +75,7 @@ static u32 hist_browser__nr_entries(struct hist_browser *hb)
 	else
 		nr_entries = hb->hists->nr_entries;
 
+	hb->nr_callchain_rows = hist_browser__get_folding(hb);
 	return nr_entries + hb->nr_callchain_rows;
 }