Re: [PATCH v4 4/4] perf tools: Support "branch-misses:pp" on arm64

[Mark Rutland <mark.rutland@arm.com>]

Hi James,

On Mon, Feb 24, 2020 at 05:08:26PM +0000, James Clark wrote:
> On 2/17/20 11:42 AM, Adrian Hunter wrote:
> > On 11/02/20 4:04 pm, James Clark wrote:
> >> From: Tan Xiaojun <tanxiaojun@huawei.com>
> >>
> >> At the suggestion of James Clark, use spe to support the precise
> >> ip of some events. Currently its support event is:
> >> branch-misses.
> >>
> >> Example usage:
> >>
> >> $ ./perf record -e branch-misses:pp dd if=/dev/zero of=/dev/null count=10000
> >> (:p/pp/ppp is same for this case.)
> >>
> >> $ ./perf report --stdio
> >> ("--stdio is not necessary")
> >>
> >> --------------------------------------------------------------------
> >> ...
> >>  # Samples: 14  of event 'branch-misses:pp'
> >>  # Event count (approx.): 14
> >>  #
> >>  # Children      Self  Command  Shared Object      Symbol
> >>  # ........  ........  .......  .................  ..........................
> >>  #
> >>     14.29%    14.29%  dd       [kernel.kallsyms]  [k] __arch_copy_from_user
> >>     14.29%    14.29%  dd       libc-2.28.so       [.] _dl_addr
> >>      7.14%     7.14%  dd       [kernel.kallsyms]  [k] __free_pages
> >>      7.14%     7.14%  dd       [kernel.kallsyms]  [k] __pi_memcpy
> >>      7.14%     7.14%  dd       [kernel.kallsyms]  [k] pagecache_get_page
> >>      7.14%     7.14%  dd       [kernel.kallsyms]  [k] unmap_single_vma
> >>      7.14%     7.14%  dd       dd                 [.] 0x00000000000025ec
> >>      7.14%     7.14%  dd       ld-2.28.so         [.] _dl_lookup_symbol_x
> >>      7.14%     7.14%  dd       ld-2.28.so         [.] check_match
> >>      7.14%     7.14%  dd       libc-2.28.so       [.] __mpn_rshift
> >>      7.14%     7.14%  dd       libc-2.28.so       [.] _nl_intern_locale_data
> >>      7.14%     7.14%  dd       libc-2.28.so       [.] read_alias_file
> >> ...
> >> --------------------------------------------------------------------
> >>
> >> Signed-off-by: Tan Xiaojun <tanxiaojun@huawei.com>
> >> Suggested-by: James Clark <James.Clark@arm.com>
> >> Tested-by: Qi Liu <liuqi115@hisilicon.com>
> >> Signed-off-by: James Clark <james.clark@arm.com>
> >> Cc: Will Deacon <will@kernel.org>
> >> Cc: Mark Rutland <mark.rutland@arm.com>
> >> Cc: Peter Zijlstra <peterz@infradead.org>
> >> Cc: Ingo Molnar <mingo@redhat.com>
> >> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> >> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> >> Cc: Jiri Olsa <jolsa@redhat.com>
> >> Cc: Tan Xiaojun <tanxiaojun@huawei.com>
> >> Cc: Al Grant <al.grant@arm.com>
> >> Cc: Namhyung Kim <namhyung@kernel.org>
> >> ---
> >>  tools/perf/arch/arm/util/auxtrace.c | 38 +++++++++++++++++++++++++++++
> >>  tools/perf/builtin-record.c         |  5 ++++
> >>  tools/perf/util/arm-spe.c           |  9 +++++++
> >>  tools/perf/util/arm-spe.h           |  3 +++
> >>  tools/perf/util/auxtrace.h          |  6 +++++
> >>  5 files changed, 61 insertions(+)
> >>
> >> diff --git a/tools/perf/arch/arm/util/auxtrace.c b/tools/perf/arch/arm/util/auxtrace.c
> >> index 0a6e75b8777a..18f0ea7556e7 100644
> >> --- a/tools/perf/arch/arm/util/auxtrace.c
> >> +++ b/tools/perf/arch/arm/util/auxtrace.c
> >> @@ -10,11 +10,25 @@
> >>  
> >>  #include "../../util/auxtrace.h"
> >>  #include "../../util/debug.h"
> >> +#include "../../util/env.h"
> >>  #include "../../util/evlist.h"
> >>  #include "../../util/pmu.h"
> >>  #include "cs-etm.h"
> >>  #include "arm-spe.h"
> >>  
> >> +#define SPE_ATTR_TS_ENABLE		BIT(0)
> >> +#define SPE_ATTR_PA_ENABLE		BIT(1)
> >> +#define SPE_ATTR_PCT_ENABLE		BIT(2)
> >> +#define SPE_ATTR_JITTER			BIT(16)
> >> +#define SPE_ATTR_BRANCH_FILTER		BIT(32)
> >> +#define SPE_ATTR_LOAD_FILTER		BIT(33)
> >> +#define SPE_ATTR_STORE_FILTER		BIT(34)
> >> +
> >> +#define SPE_ATTR_EV_RETIRED		BIT(1)
> >> +#define SPE_ATTR_EV_CACHE		BIT(3)
> >> +#define SPE_ATTR_EV_TLB			BIT(5)
> >> +#define SPE_ATTR_EV_BRANCH		BIT(7)
> >> +
> >>  static struct perf_pmu **find_all_arm_spe_pmus(int *nr_spes, int *err)
> >>  {
> >>  	struct perf_pmu **arm_spe_pmus = NULL;
> >> @@ -108,3 +122,27 @@ struct auxtrace_record
> >>  	*err = 0;
> >>  	return NULL;
> >>  }
> >> +
> >> +void auxtrace__preprocess_evlist(struct evlist *evlist)
> >> +{
> >> +	struct evsel *evsel;
> >> +	struct perf_pmu *pmu;
> >> +
> >> +	evlist__for_each_entry(evlist, evsel) {
> >> +		/* Currently only supports precise_ip for branch-misses on arm64 */
> >> +		if (!strcmp(perf_env__arch(evlist->env), "arm64")
> > 
> > Isn't config ambiguous unless you also check type i.e.
> > 
> > 			&& evsel->core.attr.type == PERF_TYPE_HARDWARE
> > 
> 
> Yes you're right I will add this.
> 
> >> +			&& evsel->core.attr.config == PERF_COUNT_HW_BRANCH_MISSES
> >> +			&& evsel->core.attr.precise_ip)
> >> +		{
> >> +			pmu = perf_pmu__find("arm_spe_0");
> >> +			if (pmu) {
> > 
> > Changing the event seems a bit weird.
> > 
> 
> This is because there is no support in the kernel for the precise_ip attribute on Arm.
> SPE can give you precise ip data for the same event, but changing the event is required.

I don't think this is the right level to override the event.

It's true that contemporary CPU PMUs can't generate synchronous events,
and hence the kernel doesn't have a precise IP, but that's not
necessarily going to be the case in future. I'd rather we didn't
silently override the event requested by the user, as I think that is
going to cause more problems for us in future.

When the SPE buffer overflows, events will be silently dropped, which I
don't believe is in keeping with the usual semantics of precise events.
Additionally, hard-coding the "arm_spe_0" name means that this will not
work reliably on big.LITTLE systems.

Instead, can we have the user explicitly request to use the SPE PMU in
this way? If the perf tool could be smart with the "_%d" suffix, and
collate PMUs differing only by that, the user would only need to do
something like:

  perf record -e arm_spe/branch-misses/pp

... which doesn't seem to onerous.

Is something like that possible?

Thanks,
Mark.