Re: [PATCH v2 5/9] libperf: Add support for user space counter access

From: Jiri Olsa <jolsa@redhat.com>
To: Rob Herring <robh@kernel.org>
Cc: Will Deacon <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Namhyung Kim <namhyung@kernel.org>,
	Raphael Gault <raphael.gault@arm.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Ian Rogers <irogers@google.com>,
	honnappa.nagarahalli@arm.com
Subject: Re: [PATCH v2 5/9] libperf: Add support for user space counter access
Date: Mon, 31 Aug 2020 11:11:23 +0200	[thread overview]
Message-ID: <20200831091123.GB406859@krava> (raw)
In-Reply-To: <20200828205614.3391252-6-robh@kernel.org>

On Fri, Aug 28, 2020 at 02:56:10PM -0600, Rob Herring wrote:

SNIP

>  #endif /* __LIBPERF_INTERNAL_MMAP_H */
> diff --git a/tools/lib/perf/include/perf/evsel.h b/tools/lib/perf/include/perf/evsel.h
> index c82ec39a4ad0..6d0da962870c 100644
> --- a/tools/lib/perf/include/perf/evsel.h
> +++ b/tools/lib/perf/include/perf/evsel.h
> @@ -27,6 +27,7 @@ LIBPERF_API int perf_evsel__open(struct perf_evsel *evsel, struct perf_cpu_map *
>  				 struct perf_thread_map *threads);
>  LIBPERF_API void perf_evsel__close(struct perf_evsel *evsel);
>  LIBPERF_API void perf_evsel__close_cpu(struct perf_evsel *evsel, int cpu);
> +LIBPERF_API void *perf_evsel__mmap(struct perf_evsel *evsel);
>  LIBPERF_API int perf_evsel__read(struct perf_evsel *evsel, int cpu, int thread,
>  				 struct perf_counts_values *count);
>  LIBPERF_API int perf_evsel__enable(struct perf_evsel *evsel);
> diff --git a/tools/lib/perf/libperf.map b/tools/lib/perf/libperf.map
> index 7be1af8a546c..733a0647be8b 100644
> --- a/tools/lib/perf/libperf.map
> +++ b/tools/lib/perf/libperf.map
> @@ -23,6 +23,7 @@ LIBPERF_0.0.1 {
>  		perf_evsel__disable;
>  		perf_evsel__open;
>  		perf_evsel__close;
> +		perf_evsel__mmap;
>  		perf_evsel__read;
>  		perf_evsel__cpus;
>  		perf_evsel__threads;

please put perf_evsel__mmap changes into separate patch

SNIP

> +int perf_mmap__read_self(struct perf_mmap *map, struct perf_counts_values *count)
> +{
> +	struct perf_event_mmap_page *pc = map->base;
> +	u32 seq, idx, time_mult = 0, time_shift = 0;
> +	u64 cnt, cyc = 0, time_offset = 0, time_cycles = 0, time_mask = ~0ULL;
> +
> +	BUG_ON(!pc);
> +
> +	if (!pc->cap_user_rdpmc)
> +		return -1;
> +
> +	do {
> +		seq = READ_ONCE(pc->lock);
> +		barrier();
> +
> +		count->ena = READ_ONCE(pc->time_enabled);
> +		count->run = READ_ONCE(pc->time_running);
> +
> +		if (pc->cap_user_time && count->ena != count->run) {
> +			cyc = read_timestamp();
> +			time_mult = READ_ONCE(pc->time_mult);
> +			time_shift = READ_ONCE(pc->time_shift);
> +			time_offset = READ_ONCE(pc->time_offset);
> +
> +			if (pc->cap_user_time_short) {
> +				time_cycles = READ_ONCE(pc->time_cycles);
> +				time_mask = READ_ONCE(pc->time_mask);
> +			}
> +		}
> +
> +		idx = READ_ONCE(pc->index);
> +		cnt = READ_ONCE(pc->offset);
> +		if (pc->cap_user_rdpmc && idx) {

no need to check pc->cap_user_rdpmc again

> +static int test_stat_user_read(int event)
> +{
> +	struct perf_counts_values counts = { .val = 0 };
> +	struct perf_thread_map *threads;
> +	struct perf_evsel *evsel;
> +	struct perf_event_mmap_page *pc;
> +	struct perf_event_attr attr = {
> +		.type	= PERF_TYPE_HARDWARE,
> +		.config	= event,
> +	};
> +	int err, i;
> +
> +	threads = perf_thread_map__new_dummy();
> +	__T("failed to create threads", threads);
> +
> +	perf_thread_map__set_pid(threads, 0, 0);
> +
> +	evsel = perf_evsel__new(&attr);
> +	__T("failed to create evsel", evsel);
> +
> +	err = perf_evsel__open(evsel, NULL, threads);
> +	__T("failed to open evsel", err == 0);
> +
> +	pc = perf_evsel__mmap(evsel);
> +	__T("failed to mmap evsel", pc);
> +
> +#if defined(__i386__) || defined(__x86_64__) || defined(__aarch64__)
> +	__T("userspace counter access not supported", pc->cap_user_rdpmc);
> +	__T("userspace counter access not enabled", pc->index);
> +	__T("userspace counter width not set", pc->pmc_width >= 32);
> +#endif
> +
> +	perf_evsel__read(evsel, 0, 0, &counts);
> +	__T("failed to read value for evsel", counts.val != 0);
> +
> +	fputs("\n", stderr);
> +	for (i = 0; i < 5; i++) {
> +		volatile int count = 0x10000 << i;
> +		__u64 start, end, last = 0;
> +
> +		fprintf(stderr, "\tloop = %u, ", count);

we should add support to display verbose output for tests,
because right now this breaks the output:

- running test-cpumap.c...OK
- running test-threadmap.c...OK
- running test-evlist.c...OK
- running test-evsel.c...
        loop = 65536, count = 328035
        loop = 131072, count = 655715
        loop = 262144, count = 1311075
        loop = 524288, count = 2627060
        loop = 1048576, count = 5253540

        loop = 65536, count = 327594
        loop = 131072, count = 659930
        loop = 262144, count = 1378892
        loop = 524288, count = 2664341
        loop = 1048576, count = 5365682
OK

but we can do it in separate change later

thanks,
jirka