Re: [PATCH v1 1/4] perf annotate: create a new hists to manage multiple events samples

From: Arnaldo Carvalho de Melo
Date: Thu Oct 05 2017 - 09:22:03 EST

Next message: Daniel Vetter: "[PATCH] drm/i915: Preallocate mmu notifier to unbreak cpu hotplug deadlock"
Previous message: tip-bot for Kees Cook: "[tip:timers/core] timer: Remove last user of TIMER_INITIALIZER"
Next in thread: Jin, Yao: "Re: [PATCH v1 1/4] perf annotate: create a new hists to manage multiple events samples"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Em Wed, Aug 16, 2017 at 06:18:33PM +0800, Jin Yao escreveu:
> An issue is found during using perf annotate.
>
> perf record -e cycles,branches ...
> perf annotate main --stdio
>
> The result only shows cycles. It should show both cycles and
> branches on the left side. It works with "--group", but need
> this to work even without groups.

Right, for --group we know that we'll be reading all the counters at
each sample, so it all works and we can use the current design.

When we're not using groups tho, each sample has just one of the events
and we end up with separate views.

Note tho that since the annotation buckets are kept per 'struct symbol'
instance, this problem should be present only in the hist_entry based
views, i.e. 'perf report' and 'perf top', right?

I.e. all struct hist_entry->ms.sym instances point to the same stuct
symbol and thus will use the same annotation histogram buckets, i.e.
symbol__annotation(hist_entry->ms.sym) point to the same 'struct
annotation' instance, and then its a matter of passing this pointer to
annotation__histogram(notes, IDX) where this IDX is perf_evsel->idx.

I wonder if we can't just add a new rb_node in struct hist_entry and
have it in two rb_trees, i.e. in two 'struct hists' instances, one per
evsel and one per evlist.

To be continued...

> In current design, the hists is per event. So we need a new
> hists to manage the samples for multiple events and use a new
> hist_event data structure to save the map/symbol information
> for per event.
>
> Signed-off-by: Jin Yao <yao.jin@xxxxxxxxxxxxxxx>
> ---
> tools/perf/builtin-annotate.c | 60 +++++++++++++++++++++++++++++++------------
> tools/perf/util/annotate.c | 8 ++++++
> tools/perf/util/annotate.h | 4 +++
> tools/perf/util/sort.c | 21 +++++++++++++++
> tools/perf/util/sort.h | 13 ++++++++++
> 5 files changed, 89 insertions(+), 17 deletions(-)
>
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 658c920..833866c 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -45,6 +45,7 @@ struct perf_annotate {
> bool skip_missing;
> const char *sym_hist_filter;
> const char *cpu_list;
> + struct hists events_hists;
> DECLARE_BITMAP(cpu_bitmap, MAX_NR_CPUS);
> };
>
> @@ -153,6 +154,7 @@ static int perf_evsel__add_sample(struct perf_evsel *evsel,
> struct hists *hists = evsel__hists(evsel);
> struct hist_entry *he;
> int ret;
> + struct hist_event *hevt;
>
> if (ann->sym_hist_filter != NULL &&
> (al->sym == NULL ||
> @@ -177,12 +179,30 @@ static int perf_evsel__add_sample(struct perf_evsel *evsel,
> */
> process_branch_stack(sample->branch_stack, al, sample);
>
> - he = hists__add_entry(hists, al, NULL, NULL, NULL, sample, true);
> - if (he == NULL)
> - return -ENOMEM;
> + if (symbol_conf.nr_events == 1) {
> + he = hists__add_entry(hists, al, NULL, NULL, NULL,
> + sample, true);
> + if (he == NULL)
> + return -ENOMEM;
> +
> + ret = hist_entry__inc_addr_samples(he, sample, evsel->idx,
> + al->addr);
> + hists__inc_nr_samples(hists, true);
> + } else {
> + he = hists__add_entry(&ann->events_hists, al, NULL,
> + NULL, NULL, sample, true);
> + if (he == NULL)
> + return -ENOMEM;
> +
> + hevt = hist_entry__event_add(he, evsel);
> + if (hevt == NULL)
> + return -ENOMEM;
> +
> + ret = hist_event__inc_addr_samples(hevt, sample, hevt->idx,
> + al->addr);
> + hists__inc_nr_samples(&ann->events_hists, true);
> + }
>
> - ret = hist_entry__inc_addr_samples(he, sample, evsel->idx, al->addr);
> - hists__inc_nr_samples(hists, true);
> return ret;
> }
>
> @@ -304,7 +324,8 @@ static int __cmd_annotate(struct perf_annotate *ann)
> int ret;
> struct perf_session *session = ann->session;
> struct perf_evsel *pos;
> - u64 total_nr_samples;
> + u64 total_nr_samples = 0;
> + struct hists *hists;
>
> if (ann->cpu_list) {
> ret = perf_session__cpu_bitmap(session, ann->cpu_list,
> @@ -335,23 +356,26 @@ static int __cmd_annotate(struct perf_annotate *ann)
> if (verbose > 2)
> perf_session__fprintf_dsos(session, stdout);
>
> - total_nr_samples = 0;
> - evlist__for_each_entry(session->evlist, pos) {
> - struct hists *hists = evsel__hists(pos);
> - u32 nr_samples = hists->stats.nr_events[PERF_RECORD_SAMPLE];
> + if (symbol_conf.nr_events > 1) {
> + hists = &ann->events_hists;
> + total_nr_samples +=
> + hists->stats.nr_events[PERF_RECORD_SAMPLE];
> +
> + hists__collapse_resort(hists, NULL);
> + hists__output_resort(hists, NULL);
> + hists__find_annotations(hists, NULL, ann);
> + } else {
> + evlist__for_each_entry(session->evlist, pos) {
> + hists = evsel__hists(pos);
> + total_nr_samples +=
> + hists->stats.nr_events[PERF_RECORD_SAMPLE];
>
> - if (nr_samples > 0) {
> - total_nr_samples += nr_samples;
> hists__collapse_resort(hists, NULL);
> /* Don't sort callchain */
> perf_evsel__reset_sample_bit(pos, CALLCHAIN);
> perf_evsel__output_resort(pos, NULL);
> -
> - if (symbol_conf.event_group &&
> - !perf_evsel__is_group_leader(pos))
> - continue;
> -
> hists__find_annotations(hists, pos, ann);
> + break;
> }
> }
>
> @@ -486,6 +510,8 @@ int cmd_annotate(int argc, const char **argv)
> if (ret < 0)
> goto out_delete;
>
> + __hists__init(&annotate.events_hists, &perf_hpp_list);
> +
> if (setup_sorting(NULL) < 0)
> usage_with_options(annotate_usage, options);
>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 2dab0e5..16ec881 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -828,6 +828,14 @@ int hist_entry__inc_addr_samples(struct hist_entry *he, struct perf_sample *samp
> return symbol__inc_addr_samples(he->ms.sym, he->ms.map, evidx, ip, sample);
> }
>
> +int hist_event__inc_addr_samples(struct hist_event *hevt,
> + struct perf_sample *sample,
> + int idx, u64 ip)
> +{
> + return symbol__inc_addr_samples(hevt->ms.sym, hevt->ms.map,
> + idx, ip, sample);
> +}
> +
> static void disasm_line__init_ins(struct disasm_line *dl, struct arch *arch, struct map *map)
> {
> dl->ins.ops = ins__find(arch, dl->ins.name);
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 9ce575c..0d44cfe 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -165,6 +165,10 @@ int addr_map_symbol__account_cycles(struct addr_map_symbol *ams,
> int hist_entry__inc_addr_samples(struct hist_entry *he, struct perf_sample *sample,
> int evidx, u64 addr);
>
> +int hist_event__inc_addr_samples(struct hist_event *hevt,
> + struct perf_sample *sample,
> + int idx, u64 addr);
> +
> int symbol__alloc_hist(struct symbol *sym);
> void symbol__annotate_zero_histograms(struct symbol *sym);
>
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 12359bd..1500a8e 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -1490,6 +1490,27 @@ struct sort_entry sort_sym_size = {
> .se_width_idx = HISTC_SYM_SIZE,
> };
>
> +struct hist_event *hist_entry__event_add(struct hist_entry *he,
> + struct perf_evsel *evsel)
> +{
> + int i;
> +
> + for (i = 0; i < he->event_nr; i++) {
> + if (he->events[i].evsel == evsel)
> + return &he->events[i];
> + }
> +
> + if (i < HIST_ENTRY_EVENTS) {
> + memset(&he->events[i], 0, sizeof(struct hist_event));
> + memcpy(&he->events[i].ms, &he->ms, sizeof(struct map_symbol));
> + he->events[i].evsel = evsel;
> + he->events[i].idx = i;
> + he->event_nr++;
> + return &he->events[i];
> + }
> +
> + return NULL;
> +}
>
> struct sort_dimension {
> const char *name;
> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> index b7c7559..446a2a3 100644
> --- a/tools/perf/util/sort.h
> +++ b/tools/perf/util/sort.h
> @@ -79,6 +79,14 @@ struct hist_entry_ops {
> void (*free)(void *ptr);
> };
>
> +#define HIST_ENTRY_EVENTS 8
> +
> +struct hist_event {
> + struct map_symbol ms;
> + struct perf_evsel *evsel;
> + int idx;
> +};
> +
> /**
> * struct hist_entry - histogram entry
> *
> @@ -95,6 +103,8 @@ struct hist_entry {
> struct he_stat stat;
> struct he_stat *stat_acc;
> struct map_symbol ms;
> + struct hist_event events[HIST_ENTRY_EVENTS];
> + int event_nr;
> struct thread *thread;
> struct comm *comm;
> struct namespace_id cgroup_id;
> @@ -291,4 +301,7 @@ sort__daddr_cmp(struct hist_entry *left, struct hist_entry *right);
> int64_t
> sort__dcacheline_cmp(struct hist_entry *left, struct hist_entry *right);
> char *hist_entry__get_srcline(struct hist_entry *he);
> +struct hist_event *hist_entry__event_add(struct hist_entry *he,
> + struct perf_evsel *evsel);
> +
> #endif /* __PERF_SORT_H */
> --
> 2.7.4

Next message: Daniel Vetter: "[PATCH] drm/i915: Preallocate mmu notifier to unbreak cpu hotplug deadlock"
Previous message: tip-bot for Kees Cook: "[tip:timers/core] timer: Remove last user of TIMER_INITIALIZER"
Next in thread: Jin, Yao: "Re: [PATCH v1 1/4] perf annotate: create a new hists to manage multiple events samples"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]