[PATCH 1/2] perf top: Decay all events in the evlist

[PATCH 1/2] perf top: Decay all events in the evlist

[Namhyung Kim]

Currently perf top only decays entries in a selected evsel.  I don't
know whether it's intended (maybe due to performance reason?) but
anyway it might show incorrect output when event group is used since
users will see leader event is decayed but others are not.

This patch moves the decay code into evlist__resort_hists() so that
stdio and tui code shared the logic.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-top.c | 38 +++++++++++++-------------------------
 1 file changed, 13 insertions(+), 25 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 5970723cd55a..9d3059d2029d 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -264,13 +264,23 @@ static void perf_top__show_details(struct perf_top *top)
 	pthread_mutex_unlock(&notes->lock);
 }
 
-static void evlist__resort_hists(struct evlist *evlist)
+static void evlist__resort_hists(struct perf_top *t)
 {
+	struct evlist *evlist = t->evlist;
 	struct evsel *pos;
 
 	evlist__for_each_entry(evlist, pos) {
 		struct hists *hists = evsel__hists(pos);
 
+		if (evlist->enabled) {
+			if (t->zero) {
+				hists__delete_entries(hists);
+			} else {
+				hists__decay_entries(hists, t->hide_user_symbols,
+						     t->hide_kernel_symbols);
+			}
+		}
+
 		hists__collapse_resort(hists, NULL);
 
 		/* Non-group events are considered as leader */
@@ -319,16 +329,7 @@ static void perf_top__print_sym_table(struct perf_top *top)
 		return;
 	}
 
-	if (top->evlist->enabled) {
-		if (top->zero) {
-			hists__delete_entries(hists);
-		} else {
-			hists__decay_entries(hists, top->hide_user_symbols,
-					     top->hide_kernel_symbols);
-		}
-	}
-
-	evlist__resort_hists(top->evlist);
+	evlist__resort_hists(top);
 
 	hists__output_recalc_col_len(hists, top->print_entries - printed);
 	putchar('\n');
@@ -576,24 +577,11 @@ static bool perf_top__handle_keypress(struct perf_top *top, int c)
 static void perf_top__sort_new_samples(void *arg)
 {
 	struct perf_top *t = arg;
-	struct evsel *evsel = t->sym_evsel;
-	struct hists *hists;
 
 	if (t->evlist->selected != NULL)
 		t->sym_evsel = t->evlist->selected;
 
-	hists = evsel__hists(evsel);
-
-	if (t->evlist->enabled) {
-		if (t->zero) {
-			hists__delete_entries(hists);
-		} else {
-			hists__decay_entries(hists, t->hide_user_symbols,
-					     t->hide_kernel_symbols);
-		}
-	}
-
-	evlist__resort_hists(t->evlist);
+	evlist__resort_hists(t);
 
 	if (t->lost || t->drop)
 		pr_warning("Too slow to read ring buffer (change period (-c/-F) or limit CPUs (-C)\n");
-- 
2.23.0.187.g17f5b7556c-goog

[PATCH 2/2] perf top: Fix event group with more than two events

[Namhyung Kim]

The event group feature links relevant hist entries among events so
that they can be displayed together.  During the link process, each
hist entry in non-leader events is connected to a hist entry in the
leader event.  This is done in order of events specified in the
command line so it assumes that events are linked in the order.

But perf top can break the assumption since it does the link process
multiple times.  For example, a hist entry can be in the third event
only at first so it's linked after the leader.  Some time later,
second event has a hist entry for it and it'll be linked after the
entry of the third event.

This makes the code compilicated to deal with such unordered entries.
This patch simply unlink all the entries after it's printed so that
they can assume the correct order after the repeated link process.
Also it'd be easy to deal with decaying old entries IMHO.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-top.c |  6 ++++++
 tools/perf/util/hist.c   | 39 +++++++++++++++++++++------------------
 tools/perf/util/hist.h   |  1 +
 3 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 9d3059d2029d..b871dd72e4bd 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -272,6 +272,12 @@ static void evlist__resort_hists(struct perf_top *t)
 	evlist__for_each_entry(evlist, pos) {
 		struct hists *hists = evsel__hists(pos);
 
+		/*
+		 * unlink existing entries so that they can be linked
+		 * in a correct order in hists__match() below.
+		 */
+		hists__unlink(hists);
+
 		if (evlist->enabled) {
 			if (t->zero) {
 				hists__delete_entries(hists);
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 8efbf58dc3d0..47401210e087 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -2436,7 +2436,7 @@ void hists__match(struct hists *leader, struct hists *other)
 {
 	struct rb_root_cached *root;
 	struct rb_node *nd;
-	struct hist_entry *pos, *pair, *pos_pair, *tmp_pair;
+	struct hist_entry *pos, *pair;
 
 	if (symbol_conf.report_hierarchy) {
 		/* hierarchy report always collapses entries */
@@ -2453,24 +2453,8 @@ void hists__match(struct hists *leader, struct hists *other)
 		pos  = rb_entry(nd, struct hist_entry, rb_node_in);
 		pair = hists__find_entry(other, pos);
 
-		if (pair && list_empty(&pair->pairs.node)) {
-			list_for_each_entry_safe(pos_pair, tmp_pair, &pos->pairs.head, pairs.node) {
-				if (pos_pair->hists == other) {
-					/*
-					 * XXX maybe decayed entries can appear
-					 * here?  but then we would have use
-					 * after free, as decayed entries are
-					 * freed see hists__delete_entry
-					 */
-					BUG_ON(!pos_pair->dummy);
-					list_del_init(&pos_pair->pairs.node);
-					hist_entry__delete(pos_pair);
-					break;
-				}
-			}
-
+		if (pair)
 			hist_entry__add_pair(pair, pos);
-		}
 	}
 }
 
@@ -2555,6 +2539,25 @@ int hists__link(struct hists *leader, struct hists *other)
 	return 0;
 }
 
+int hists__unlink(struct hists *hists)
+{
+	struct rb_root_cached *root;
+	struct rb_node *nd;
+	struct hist_entry *pos;
+
+	if (hists__has(hists, need_collapse))
+		root = &hists->entries_collapsed;
+	else
+		root = hists->entries_in;
+
+	for (nd = rb_first_cached(root); nd; nd = rb_next(nd)) {
+		pos = rb_entry(nd, struct hist_entry, rb_node_in);
+		list_del_init(&pos->pairs.node);
+	}
+
+	return 0;
+}
+
 void hist__account_cycles(struct branch_stack *bs, struct addr_location *al,
 			  struct perf_sample *sample, bool nonany_branch_mode)
 {
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 83d5fc15429c..7b9267ebebeb 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -217,6 +217,7 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *he);
 
 void hists__match(struct hists *leader, struct hists *other);
 int hists__link(struct hists *leader, struct hists *other);
+int hists__unlink(struct hists *hists);
 
 struct hists_evsel {
 	struct evsel evsel;
-- 
2.23.0.187.g17f5b7556c-goog

Re: [PATCH 1/2] perf top: Decay all events in the evlist

[Arnaldo Carvalho de Melo]

Em Wed, Aug 28, 2019 at 08:15:54AM +0900, Namhyung Kim escreveu:
> Currently perf top only decays entries in a selected evsel.  I don't
> know whether it's intended (maybe due to performance reason?) but
> anyway it might show incorrect output when event group is used since
> users will see leader event is decayed but others are not.
> 
> This patch moves the decay code into evlist__resort_hists() so that
> stdio and tui code shared the logic.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-top.c | 38 +++++++++++++-------------------------
>  1 file changed, 13 insertions(+), 25 deletions(-)
> 
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 5970723cd55a..9d3059d2029d 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -264,13 +264,23 @@ static void perf_top__show_details(struct perf_top *top)
>  	pthread_mutex_unlock(&notes->lock);
>  }
>  
> -static void evlist__resort_hists(struct evlist *evlist)
> +static void evlist__resort_hists(struct perf_top *t)

Since this now operates on the perf_top struct, I'll rename it to
perf_top__resort_hists(), ok? No need to send an updated patch.

- Arnaldo

>  {
> +	struct evlist *evlist = t->evlist;
>  	struct evsel *pos;
>  
>  	evlist__for_each_entry(evlist, pos) {
>  		struct hists *hists = evsel__hists(pos);
>  
> +		if (evlist->enabled) {
> +			if (t->zero) {
> +				hists__delete_entries(hists);
> +			} else {
> +				hists__decay_entries(hists, t->hide_user_symbols,
> +						     t->hide_kernel_symbols);
> +			}
> +		}
> +
>  		hists__collapse_resort(hists, NULL);
>  
>  		/* Non-group events are considered as leader */
> @@ -319,16 +329,7 @@ static void perf_top__print_sym_table(struct perf_top *top)
>  		return;
>  	}
>  
> -	if (top->evlist->enabled) {
> -		if (top->zero) {
> -			hists__delete_entries(hists);
> -		} else {
> -			hists__decay_entries(hists, top->hide_user_symbols,
> -					     top->hide_kernel_symbols);
> -		}
> -	}
> -
> -	evlist__resort_hists(top->evlist);
> +	evlist__resort_hists(top);
>  
>  	hists__output_recalc_col_len(hists, top->print_entries - printed);
>  	putchar('\n');
> @@ -576,24 +577,11 @@ static bool perf_top__handle_keypress(struct perf_top *top, int c)
>  static void perf_top__sort_new_samples(void *arg)
>  {
>  	struct perf_top *t = arg;
> -	struct evsel *evsel = t->sym_evsel;
> -	struct hists *hists;
>  
>  	if (t->evlist->selected != NULL)
>  		t->sym_evsel = t->evlist->selected;
>  
> -	hists = evsel__hists(evsel);
> -
> -	if (t->evlist->enabled) {
> -		if (t->zero) {
> -			hists__delete_entries(hists);
> -		} else {
> -			hists__decay_entries(hists, t->hide_user_symbols,
> -					     t->hide_kernel_symbols);
> -		}
> -	}
> -
> -	evlist__resort_hists(t->evlist);
> +	evlist__resort_hists(t);
>  
>  	if (t->lost || t->drop)
>  		pr_warning("Too slow to read ring buffer (change period (-c/-F) or limit CPUs (-C)\n");
> -- 
> 2.23.0.187.g17f5b7556c-goog

-- 

- Arnaldo

Re: [PATCH 2/2] perf top: Fix event group with more than two events

[Arnaldo Carvalho de Melo]

Em Wed, Aug 28, 2019 at 08:15:55AM +0900, Namhyung Kim escreveu:
> The event group feature links relevant hist entries among events so
> that they can be displayed together.  During the link process, each
> hist entry in non-leader events is connected to a hist entry in the
> leader event.  This is done in order of events specified in the
> command line so it assumes that events are linked in the order.
> 
> But perf top can break the assumption since it does the link process
> multiple times.  For example, a hist entry can be in the third event
> only at first so it's linked after the leader.  Some time later,
> second event has a hist entry for it and it'll be linked after the
> entry of the third event.
> 
> This makes the code compilicated to deal with such unordered entries.
> This patch simply unlink all the entries after it's printed so that
> they can assume the correct order after the repeated link process.
> Also it'd be easy to deal with decaying old entries IMHO.

Excellent, I just tested it with:

# perf top --show-total-period -e '{cycles,instructions,cache-references,cache-misses}'

And the total period goes down for all evsels when that symbol is
quiescent between screen refreshes, and no crash, all seems to be
working as expected, thanks for fixing this!

Also when pressing 'A' the annotation also seems to be working.

All applied.

- Arnaldo
 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-top.c |  6 ++++++
>  tools/perf/util/hist.c   | 39 +++++++++++++++++++++------------------
>  tools/perf/util/hist.h   |  1 +
>  3 files changed, 28 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 9d3059d2029d..b871dd72e4bd 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -272,6 +272,12 @@ static void evlist__resort_hists(struct perf_top *t)
>  	evlist__for_each_entry(evlist, pos) {
>  		struct hists *hists = evsel__hists(pos);
>  
> +		/*
> +		 * unlink existing entries so that they can be linked
> +		 * in a correct order in hists__match() below.
> +		 */
> +		hists__unlink(hists);
> +
>  		if (evlist->enabled) {
>  			if (t->zero) {
>  				hists__delete_entries(hists);
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 8efbf58dc3d0..47401210e087 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -2436,7 +2436,7 @@ void hists__match(struct hists *leader, struct hists *other)
>  {
>  	struct rb_root_cached *root;
>  	struct rb_node *nd;
> -	struct hist_entry *pos, *pair, *pos_pair, *tmp_pair;
> +	struct hist_entry *pos, *pair;
>  
>  	if (symbol_conf.report_hierarchy) {
>  		/* hierarchy report always collapses entries */
> @@ -2453,24 +2453,8 @@ void hists__match(struct hists *leader, struct hists *other)
>  		pos  = rb_entry(nd, struct hist_entry, rb_node_in);
>  		pair = hists__find_entry(other, pos);
>  
> -		if (pair && list_empty(&pair->pairs.node)) {
> -			list_for_each_entry_safe(pos_pair, tmp_pair, &pos->pairs.head, pairs.node) {
> -				if (pos_pair->hists == other) {
> -					/*
> -					 * XXX maybe decayed entries can appear
> -					 * here?  but then we would have use
> -					 * after free, as decayed entries are
> -					 * freed see hists__delete_entry
> -					 */
> -					BUG_ON(!pos_pair->dummy);
> -					list_del_init(&pos_pair->pairs.node);
> -					hist_entry__delete(pos_pair);
> -					break;
> -				}
> -			}
> -
> +		if (pair)
>  			hist_entry__add_pair(pair, pos);
> -		}
>  	}
>  }
>  
> @@ -2555,6 +2539,25 @@ int hists__link(struct hists *leader, struct hists *other)
>  	return 0;
>  }
>  
> +int hists__unlink(struct hists *hists)
> +{
> +	struct rb_root_cached *root;
> +	struct rb_node *nd;
> +	struct hist_entry *pos;
> +
> +	if (hists__has(hists, need_collapse))
> +		root = &hists->entries_collapsed;
> +	else
> +		root = hists->entries_in;
> +
> +	for (nd = rb_first_cached(root); nd; nd = rb_next(nd)) {
> +		pos = rb_entry(nd, struct hist_entry, rb_node_in);
> +		list_del_init(&pos->pairs.node);
> +	}
> +
> +	return 0;
> +}
> +
>  void hist__account_cycles(struct branch_stack *bs, struct addr_location *al,
>  			  struct perf_sample *sample, bool nonany_branch_mode)
>  {
> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> index 83d5fc15429c..7b9267ebebeb 100644
> --- a/tools/perf/util/hist.h
> +++ b/tools/perf/util/hist.h
> @@ -217,6 +217,7 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *he);
>  
>  void hists__match(struct hists *leader, struct hists *other);
>  int hists__link(struct hists *leader, struct hists *other);
> +int hists__unlink(struct hists *hists);
>  
>  struct hists_evsel {
>  	struct evsel evsel;
> -- 
> 2.23.0.187.g17f5b7556c-goog

-- 

- Arnaldo

[tip: perf/core] perf top: Decay all events in the evlist

[tip-bot2 for Namhyung Kim]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     ea4385f804eadce3f4fd8698d4ffd9e85fb6d5e0
Gitweb:        https://git.kernel.org/tip/ea4385f804eadce3f4fd8698d4ffd9e85fb6d5e0
Author:        Namhyung Kim <namhyung@kernel.org>
AuthorDate:    Wed, 28 Aug 2019 08:15:54 +09:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Wed, 28 Aug 2019 18:15:03 -03:00

perf top: Decay all events in the evlist

Currently perf top only decays entries in a selected evsel.  I don't
know whether it's intended (maybe due to performance reason?) but anyway
it might show incorrect output when event group is used since users will
see leader event is decayed but others are not.

This patch moves the decay code into perf_top__resort_hists() so that
stdio and TUI code shared the logic.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Link: http://lkml.kernel.org/r/20190827231555.121411-1-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-top.c | 38 +++++++++++++-------------------------
 1 file changed, 13 insertions(+), 25 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 42ba733..104dbb1 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -265,13 +265,23 @@ static void perf_top__show_details(struct perf_top *top)
 	pthread_mutex_unlock(&notes->lock);
 }
 
-static void evlist__resort_hists(struct evlist *evlist)
+static void perf_top__resort_hists(struct perf_top *t)
 {
+	struct evlist *evlist = t->evlist;
 	struct evsel *pos;
 
 	evlist__for_each_entry(evlist, pos) {
 		struct hists *hists = evsel__hists(pos);
 
+		if (evlist->enabled) {
+			if (t->zero) {
+				hists__delete_entries(hists);
+			} else {
+				hists__decay_entries(hists, t->hide_user_symbols,
+						     t->hide_kernel_symbols);
+			}
+		}
+
 		hists__collapse_resort(hists, NULL);
 
 		/* Non-group events are considered as leader */
@@ -320,16 +330,7 @@ static void perf_top__print_sym_table(struct perf_top *top)
 		return;
 	}
 
-	if (top->evlist->enabled) {
-		if (top->zero) {
-			hists__delete_entries(hists);
-		} else {
-			hists__decay_entries(hists, top->hide_user_symbols,
-					     top->hide_kernel_symbols);
-		}
-	}
-
-	evlist__resort_hists(top->evlist);
+	perf_top__resort_hists(top);
 
 	hists__output_recalc_col_len(hists, top->print_entries - printed);
 	putchar('\n');
@@ -577,24 +578,11 @@ static bool perf_top__handle_keypress(struct perf_top *top, int c)
 static void perf_top__sort_new_samples(void *arg)
 {
 	struct perf_top *t = arg;
-	struct evsel *evsel = t->sym_evsel;
-	struct hists *hists;
 
 	if (t->evlist->selected != NULL)
 		t->sym_evsel = t->evlist->selected;
 
-	hists = evsel__hists(evsel);
-
-	if (t->evlist->enabled) {
-		if (t->zero) {
-			hists__delete_entries(hists);
-		} else {
-			hists__decay_entries(hists, t->hide_user_symbols,
-					     t->hide_kernel_symbols);
-		}
-	}
-
-	evlist__resort_hists(t->evlist);
+	perf_top__resort_hists(t);
 
 	if (t->lost || t->drop)
 		pr_warning("Too slow to read ring buffer (change period (-c/-F) or limit CPUs (-C)\n");

[tip: perf/core] perf top: Fix event group with more than two events

[tip-bot2 for Namhyung Kim]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     be5863b7d9281bbb932542d16b7d758357fde267
Gitweb:        https://git.kernel.org/tip/be5863b7d9281bbb932542d16b7d758357fde267
Author:        Namhyung Kim <namhyung@kernel.org>
AuthorDate:    Wed, 28 Aug 2019 08:15:55 +09:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Wed, 28 Aug 2019 18:15:03 -03:00

perf top: Fix event group with more than two events

The event group feature links relevant hist entries among events so that
they can be displayed together.  During the link process, each hist
entry in non-leader events is connected to a hist entry in the leader
event.  This is done in order of events specified in the command line so
it assumes that events are linked in the order.

But 'perf top' can break the assumption since it does the link process
multiple times.  For example, a hist entry can be in the third event
only at first so it's linked after the leader.  Some time later, second
event has a hist entry for it and it'll be linked after the entry of the
third event.

This makes the code compilicated to deal with such unordered entries.
This patch simply unlink all the entries after it's printed so that they
can assume the correct order after the repeated link process.  Also it'd
be easy to deal with decaying old entries IMHO.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Link: http://lkml.kernel.org/r/20190827231555.121411-2-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-top.c |  6 ++++++-
 tools/perf/util/hist.c   | 39 +++++++++++++++++++++------------------
 tools/perf/util/hist.h   |  1 +-
 3 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 104dbb1..c3f9544 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -273,6 +273,12 @@ static void perf_top__resort_hists(struct perf_top *t)
 	evlist__for_each_entry(evlist, pos) {
 		struct hists *hists = evsel__hists(pos);
 
+		/*
+		 * unlink existing entries so that they can be linked
+		 * in a correct order in hists__match() below.
+		 */
+		hists__unlink(hists);
+
 		if (evlist->enabled) {
 			if (t->zero) {
 				hists__delete_entries(hists);
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 3370267..e0b1496 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -2439,7 +2439,7 @@ void hists__match(struct hists *leader, struct hists *other)
 {
 	struct rb_root_cached *root;
 	struct rb_node *nd;
-	struct hist_entry *pos, *pair, *pos_pair, *tmp_pair;
+	struct hist_entry *pos, *pair;
 
 	if (symbol_conf.report_hierarchy) {
 		/* hierarchy report always collapses entries */
@@ -2456,24 +2456,8 @@ void hists__match(struct hists *leader, struct hists *other)
 		pos  = rb_entry(nd, struct hist_entry, rb_node_in);
 		pair = hists__find_entry(other, pos);
 
-		if (pair && list_empty(&pair->pairs.node)) {
-			list_for_each_entry_safe(pos_pair, tmp_pair, &pos->pairs.head, pairs.node) {
-				if (pos_pair->hists == other) {
-					/*
-					 * XXX maybe decayed entries can appear
-					 * here?  but then we would have use
-					 * after free, as decayed entries are
-					 * freed see hists__delete_entry
-					 */
-					BUG_ON(!pos_pair->dummy);
-					list_del_init(&pos_pair->pairs.node);
-					hist_entry__delete(pos_pair);
-					break;
-				}
-			}
-
+		if (pair)
 			hist_entry__add_pair(pair, pos);
-		}
 	}
 }
 
@@ -2558,6 +2542,25 @@ int hists__link(struct hists *leader, struct hists *other)
 	return 0;
 }
 
+int hists__unlink(struct hists *hists)
+{
+	struct rb_root_cached *root;
+	struct rb_node *nd;
+	struct hist_entry *pos;
+
+	if (hists__has(hists, need_collapse))
+		root = &hists->entries_collapsed;
+	else
+		root = hists->entries_in;
+
+	for (nd = rb_first_cached(root); nd; nd = rb_next(nd)) {
+		pos = rb_entry(nd, struct hist_entry, rb_node_in);
+		list_del_init(&pos->pairs.node);
+	}
+
+	return 0;
+}
+
 void hist__account_cycles(struct branch_stack *bs, struct addr_location *al,
 			  struct perf_sample *sample, bool nonany_branch_mode)
 {
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 83d5fc1..7b9267e 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -217,6 +217,7 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *he);
 
 void hists__match(struct hists *leader, struct hists *other);
 int hists__link(struct hists *leader, struct hists *other);
+int hists__unlink(struct hists *hists);
 
 struct hists_evsel {
 	struct evsel evsel;

Re: [PATCH 1/2] perf top: Decay all events in the evlist

[Namhyung Kim]

Hi Arnaldo,

On Wed, Aug 28, 2019 at 9:49 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Wed, Aug 28, 2019 at 08:15:54AM +0900, Namhyung Kim escreveu:
> > Currently perf top only decays entries in a selected evsel.  I don't
> > know whether it's intended (maybe due to performance reason?) but
> > anyway it might show incorrect output when event group is used since
> > users will see leader event is decayed but others are not.
> >
> > This patch moves the decay code into evlist__resort_hists() so that
> > stdio and tui code shared the logic.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/builtin-top.c | 38 +++++++++++++-------------------------
> >  1 file changed, 13 insertions(+), 25 deletions(-)
> >
> > diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> > index 5970723cd55a..9d3059d2029d 100644
> > --- a/tools/perf/builtin-top.c
> > +++ b/tools/perf/builtin-top.c
> > @@ -264,13 +264,23 @@ static void perf_top__show_details(struct perf_top *top)
> >       pthread_mutex_unlock(&notes->lock);
> >  }
> >
> > -static void evlist__resort_hists(struct evlist *evlist)
> > +static void evlist__resort_hists(struct perf_top *t)
>
> Since this now operates on the perf_top struct, I'll rename it to
> perf_top__resort_hists(), ok? No need to send an updated patch.

Right.  Thanks for doing this!

Namhyung