[PATCH v3 0/3] Tidy user rdpmc documentation and testing

[PATCH v3 0/3] Tidy user rdpmc documentation and testing

[Ian Rogers]

libperf's perf_mmap__read_self and the addition of arm64 support mean
that the perf_event.h and the rdpmc perf test have become
stale. Refresh the documentation in perf_event.h, remove the x86 rdpmc
test and port the libperf test as a non-architecture specific test.

A comment is added to perf_event.h to avoid a divide by zero when
scaling counts if the running time is 0. This was previously discussed
in this thread:
https://lore.kernel.org/lkml/CAP-5=fVRdqvswtyQMg5cB+ntTGda+SAYskjTQednEH-AeZo13g@mail.gmail.com/

v3. Incorporates fixes from Rob Herring <robh@kernel.org> to make the
    perf_event.h comments better. It drops the already merged sanitizer fix.
v2. Alters the skip in test_stat_user_read for open to always be a
    skip as perf_event_open may fail with EACCES (permissions), ENOSYS
    (not supported) and ENOENT (hypervisor). Adds Rob Herring's
    acked-by on patch 3.

Ian Rogers (3):
  perf: Align user space counter reading with code
  perf test: Remove x86 rdpmc test
  perf test: Add user space counter reading tests

 include/uapi/linux/perf_event.h        |  35 +++--
 tools/include/uapi/linux/perf_event.h  |  35 +++--
 tools/perf/arch/x86/tests/Build        |   1 -
 tools/perf/arch/x86/tests/arch-tests.c |   2 -
 tools/perf/arch/x86/tests/rdpmc.c      | 182 -------------------------
 tools/perf/tests/mmap-basic.c          | 127 ++++++++++++++++-
 6 files changed, 170 insertions(+), 212 deletions(-)
 delete mode 100644 tools/perf/arch/x86/tests/rdpmc.c

-- 
2.37.0.170.g444d1eabd0-goog

[PATCH v3 1/3] perf: Align user space counter reading with code

[Ian Rogers]

Align the user space counter reading documentation with the code in
perf_mmap__read_self. Previously the documentation was based on the perf
rdpmc test, but now general purpose code is provided by libperf.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 include/uapi/linux/perf_event.h       | 35 +++++++++++++++++----------
 tools/include/uapi/linux/perf_event.h | 35 +++++++++++++++++----------
 2 files changed, 44 insertions(+), 26 deletions(-)

diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index d37629dbad72..6826dabb7e03 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -538,9 +538,13 @@ struct perf_event_mmap_page {
 	 *
 	 *     if (pc->cap_usr_time && enabled != running) {
 	 *       cyc = rdtsc();
-	 *       time_offset = pc->time_offset;
 	 *       time_mult   = pc->time_mult;
 	 *       time_shift  = pc->time_shift;
+	 *       time_offset = pc->time_offset;
+	 *       if (pc->cap_user_time_short) {
+	 *         time_cycles = pc->time_cycles;
+	 *         time_mask = pc->time_mask;
+	 *       }
 	 *     }
 	 *
 	 *     index = pc->index;
@@ -548,6 +552,9 @@ struct perf_event_mmap_page {
 	 *     if (pc->cap_user_rdpmc && index) {
 	 *       width = pc->pmc_width;
 	 *       pmc = rdpmc(index - 1);
+	 *       pmc <<= 64 - width;
+	 *       pmc >>= 64 - width;
+	 *       count += pmc;
 	 *     }
 	 *
 	 *     barrier();
@@ -590,25 +597,27 @@ struct perf_event_mmap_page {
 	 * If cap_usr_time the below fields can be used to compute the time
 	 * delta since time_enabled (in ns) using rdtsc or similar.
 	 *
-	 *   u64 quot, rem;
-	 *   u64 delta;
-	 *
-	 *   quot = (cyc >> time_shift);
-	 *   rem = cyc & (((u64)1 << time_shift) - 1);
-	 *   delta = time_offset + quot * time_mult +
-	 *              ((rem * time_mult) >> time_shift);
+	 *   cyc = time_cycles + ((cyc - time_cycles) & time_mask);
+	 *   delta = time_offset + mul_u64_u32_shr(cyc, time_mult, time_shift);
 	 *
 	 * Where time_offset,time_mult,time_shift and cyc are read in the
-	 * seqcount loop described above. This delta can then be added to
-	 * enabled and possible running (if index), improving the scaling:
+	 * seqcount loop described above. mul_u64_u32_shr will compute:
+	 *
+	 *   (u64)(((unsigned __int128)cyc * time_mult) >> time_shift)
+	 *
+	 * This delta can then be added to enabled and possible running (if
+	 * index) to improve the scaling. Due to event multiplexing, running
+	 * may be zero and so care is needed to avoid division by zero.
 	 *
 	 *   enabled += delta;
 	 *   if (index)
 	 *     running += delta;
 	 *
-	 *   quot = count / running;
-	 *   rem  = count % running;
-	 *   count = quot * enabled + (rem * enabled) / running;
+	 *   if (running != 0) {
+	 *     quot = count / running;
+	 *     rem  = count % running;
+	 *     count = quot * enabled + (rem * enabled) / running;
+	 *   }
 	 */
 	__u16	time_shift;
 	__u32	time_mult;
diff --git a/tools/include/uapi/linux/perf_event.h b/tools/include/uapi/linux/perf_event.h
index d37629dbad72..6826dabb7e03 100644
--- a/tools/include/uapi/linux/perf_event.h
+++ b/tools/include/uapi/linux/perf_event.h
@@ -538,9 +538,13 @@ struct perf_event_mmap_page {
 	 *
 	 *     if (pc->cap_usr_time && enabled != running) {
 	 *       cyc = rdtsc();
-	 *       time_offset = pc->time_offset;
 	 *       time_mult   = pc->time_mult;
 	 *       time_shift  = pc->time_shift;
+	 *       time_offset = pc->time_offset;
+	 *       if (pc->cap_user_time_short) {
+	 *         time_cycles = pc->time_cycles;
+	 *         time_mask = pc->time_mask;
+	 *       }
 	 *     }
 	 *
 	 *     index = pc->index;
@@ -548,6 +552,9 @@ struct perf_event_mmap_page {
 	 *     if (pc->cap_user_rdpmc && index) {
 	 *       width = pc->pmc_width;
 	 *       pmc = rdpmc(index - 1);
+	 *       pmc <<= 64 - width;
+	 *       pmc >>= 64 - width;
+	 *       count += pmc;
 	 *     }
 	 *
 	 *     barrier();
@@ -590,25 +597,27 @@ struct perf_event_mmap_page {
 	 * If cap_usr_time the below fields can be used to compute the time
 	 * delta since time_enabled (in ns) using rdtsc or similar.
 	 *
-	 *   u64 quot, rem;
-	 *   u64 delta;
-	 *
-	 *   quot = (cyc >> time_shift);
-	 *   rem = cyc & (((u64)1 << time_shift) - 1);
-	 *   delta = time_offset + quot * time_mult +
-	 *              ((rem * time_mult) >> time_shift);
+	 *   cyc = time_cycles + ((cyc - time_cycles) & time_mask);
+	 *   delta = time_offset + mul_u64_u32_shr(cyc, time_mult, time_shift);
 	 *
 	 * Where time_offset,time_mult,time_shift and cyc are read in the
-	 * seqcount loop described above. This delta can then be added to
-	 * enabled and possible running (if index), improving the scaling:
+	 * seqcount loop described above. mul_u64_u32_shr will compute:
+	 *
+	 *   (u64)(((unsigned __int128)cyc * time_mult) >> time_shift)
+	 *
+	 * This delta can then be added to enabled and possible running (if
+	 * index) to improve the scaling. Due to event multiplexing, running
+	 * may be zero and so care is needed to avoid division by zero.
 	 *
 	 *   enabled += delta;
 	 *   if (index)
 	 *     running += delta;
 	 *
-	 *   quot = count / running;
-	 *   rem  = count % running;
-	 *   count = quot * enabled + (rem * enabled) / running;
+	 *   if (running != 0) {
+	 *     quot = count / running;
+	 *     rem  = count % running;
+	 *     count = quot * enabled + (rem * enabled) / running;
+	 *   }
 	 */
 	__u16	time_shift;
 	__u32	time_mult;
-- 
2.37.0.170.g444d1eabd0-goog

[PATCH v3 2/3] perf test: Remove x86 rdpmc test

[Ian Rogers]

This test has been superseded by test_stat_user_read in:
tools/lib/perf/tests/test-evsel.c
The updated test doesn't divide-by-0 when running time of a counter is
0. It also supports ARM64.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/arch/x86/tests/Build        |   1 -
 tools/perf/arch/x86/tests/arch-tests.c |   2 -
 tools/perf/arch/x86/tests/rdpmc.c      | 182 -------------------------
 3 files changed, 185 deletions(-)
 delete mode 100644 tools/perf/arch/x86/tests/rdpmc.c

diff --git a/tools/perf/arch/x86/tests/Build b/tools/perf/arch/x86/tests/Build
index 28d793390198..70b5bcbc15df 100644
--- a/tools/perf/arch/x86/tests/Build
+++ b/tools/perf/arch/x86/tests/Build
@@ -2,7 +2,6 @@ perf-$(CONFIG_DWARF_UNWIND) += regs_load.o
 perf-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o
 
 perf-y += arch-tests.o
-perf-y += rdpmc.o
 perf-y += sample-parsing.o
 perf-$(CONFIG_AUXTRACE) += insn-x86.o intel-pt-pkt-decoder-test.o
 perf-$(CONFIG_X86_64) += bp-modify.o
diff --git a/tools/perf/arch/x86/tests/arch-tests.c b/tools/perf/arch/x86/tests/arch-tests.c
index 64fb73d14d2f..04018b8aa85b 100644
--- a/tools/perf/arch/x86/tests/arch-tests.c
+++ b/tools/perf/arch/x86/tests/arch-tests.c
@@ -3,7 +3,6 @@
 #include "tests/tests.h"
 #include "arch-tests.h"
 
-DEFINE_SUITE("x86 rdpmc", rdpmc);
 #ifdef HAVE_AUXTRACE_SUPPORT
 DEFINE_SUITE("x86 instruction decoder - new instructions", insn_x86);
 DEFINE_SUITE("Intel PT packet decoder", intel_pt_pkt_decoder);
@@ -14,7 +13,6 @@ DEFINE_SUITE("x86 bp modify", bp_modify);
 DEFINE_SUITE("x86 Sample parsing", x86_sample_parsing);
 
 struct test_suite *arch_tests[] = {
-	&suite__rdpmc,
 #ifdef HAVE_DWARF_UNWIND_SUPPORT
 	&suite__dwarf_unwind,
 #endif
diff --git a/tools/perf/arch/x86/tests/rdpmc.c b/tools/perf/arch/x86/tests/rdpmc.c
deleted file mode 100644
index 498413ad9c97..000000000000
--- a/tools/perf/arch/x86/tests/rdpmc.c
+++ /dev/null
@@ -1,182 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-#include <errno.h>
-#include <unistd.h>
-#include <stdlib.h>
-#include <signal.h>
-#include <sys/mman.h>
-#include <sys/types.h>
-#include <sys/wait.h>
-#include <linux/string.h>
-#include <linux/types.h>
-#include "perf-sys.h"
-#include "debug.h"
-#include "tests/tests.h"
-#include "cloexec.h"
-#include "event.h"
-#include <internal/lib.h> // page_size
-#include "arch-tests.h"
-
-static u64 rdpmc(unsigned int counter)
-{
-	unsigned int low, high;
-
-	asm volatile("rdpmc" : "=a" (low), "=d" (high) : "c" (counter));
-
-	return low | ((u64)high) << 32;
-}
-
-static u64 rdtsc(void)
-{
-	unsigned int low, high;
-
-	asm volatile("rdtsc" : "=a" (low), "=d" (high));
-
-	return low | ((u64)high) << 32;
-}
-
-static u64 mmap_read_self(void *addr)
-{
-	struct perf_event_mmap_page *pc = addr;
-	u32 seq, idx, time_mult = 0, time_shift = 0;
-	u64 count, cyc = 0, time_offset = 0, enabled, running, delta;
-
-	do {
-		seq = pc->lock;
-		barrier();
-
-		enabled = pc->time_enabled;
-		running = pc->time_running;
-
-		if (enabled != running) {
-			cyc = rdtsc();
-			time_mult = pc->time_mult;
-			time_shift = pc->time_shift;
-			time_offset = pc->time_offset;
-		}
-
-		idx = pc->index;
-		count = pc->offset;
-		if (idx)
-			count += rdpmc(idx - 1);
-
-		barrier();
-	} while (pc->lock != seq);
-
-	if (enabled != running) {
-		u64 quot, rem;
-
-		quot = (cyc >> time_shift);
-		rem = cyc & (((u64)1 << time_shift) - 1);
-		delta = time_offset + quot * time_mult +
-			((rem * time_mult) >> time_shift);
-
-		enabled += delta;
-		if (idx)
-			running += delta;
-
-		quot = count / running;
-		rem = count % running;
-		count = quot * enabled + (rem * enabled) / running;
-	}
-
-	return count;
-}
-
-/*
- * If the RDPMC instruction faults then signal this back to the test parent task:
- */
-static void segfault_handler(int sig __maybe_unused,
-			     siginfo_t *info __maybe_unused,
-			     void *uc __maybe_unused)
-{
-	exit(-1);
-}
-
-static int __test__rdpmc(void)
-{
-	volatile int tmp = 0;
-	u64 i, loops = 1000;
-	int n;
-	int fd;
-	void *addr;
-	struct perf_event_attr attr = {
-		.type = PERF_TYPE_HARDWARE,
-		.config = PERF_COUNT_HW_INSTRUCTIONS,
-		.exclude_kernel = 1,
-	};
-	u64 delta_sum = 0;
-        struct sigaction sa;
-	char sbuf[STRERR_BUFSIZE];
-
-	sigfillset(&sa.sa_mask);
-	sa.sa_sigaction = segfault_handler;
-	sa.sa_flags = 0;
-	sigaction(SIGSEGV, &sa, NULL);
-
-	fd = sys_perf_event_open(&attr, 0, -1, -1,
-				 perf_event_open_cloexec_flag());
-	if (fd < 0) {
-		pr_err("Error: sys_perf_event_open() syscall returned "
-		       "with %d (%s)\n", fd,
-		       str_error_r(errno, sbuf, sizeof(sbuf)));
-		return -1;
-	}
-
-	addr = mmap(NULL, page_size, PROT_READ, MAP_SHARED, fd, 0);
-	if (addr == (void *)(-1)) {
-		pr_err("Error: mmap() syscall returned with (%s)\n",
-		       str_error_r(errno, sbuf, sizeof(sbuf)));
-		goto out_close;
-	}
-
-	for (n = 0; n < 6; n++) {
-		u64 stamp, now, delta;
-
-		stamp = mmap_read_self(addr);
-
-		for (i = 0; i < loops; i++)
-			tmp++;
-
-		now = mmap_read_self(addr);
-		loops *= 10;
-
-		delta = now - stamp;
-		pr_debug("%14d: %14Lu\n", n, (long long)delta);
-
-		delta_sum += delta;
-	}
-
-	munmap(addr, page_size);
-	pr_debug("   ");
-out_close:
-	close(fd);
-
-	if (!delta_sum)
-		return -1;
-
-	return 0;
-}
-
-int test__rdpmc(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
-{
-	int status = 0;
-	int wret = 0;
-	int ret;
-	int pid;
-
-	pid = fork();
-	if (pid < 0)
-		return -1;
-
-	if (!pid) {
-		ret = __test__rdpmc();
-
-		exit(ret);
-	}
-
-	wret = waitpid(pid, &status, 0);
-	if (wret < 0 || status)
-		return -1;
-
-	return 0;
-}
-- 
2.37.0.170.g444d1eabd0-goog

[PATCH v3 3/3] perf test: Add user space counter reading tests

[Ian Rogers]

These tests are based on test_stat_user_read in
tools/lib/perf/tests/test-evsel.c. The tests are modified to skip if
perf_event_open fails or rdpmc isn't supported.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/mmap-basic.c | 127 +++++++++++++++++++++++++++++++++-
 1 file changed, 126 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/mmap-basic.c b/tools/perf/tests/mmap-basic.c
index 30bbe144648a..dfb6173b2a82 100644
--- a/tools/perf/tests/mmap-basic.c
+++ b/tools/perf/tests/mmap-basic.c
@@ -170,14 +170,139 @@ static int test__basic_mmap(struct test_suite *test __maybe_unused, int subtest
 	return err;
 }
 
+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,
+#ifdef __aarch64__
+		.config1 = 0x2,		/* Request user access */
+#endif
+	};
+	int err, i, ret = TEST_FAIL;
+	bool opened = false, mapped = false;
+
+	threads = perf_thread_map__new_dummy();
+	TEST_ASSERT_VAL("failed to create threads", threads);
+
+	perf_thread_map__set_pid(threads, 0, 0);
+
+	evsel = perf_evsel__new(&attr);
+	TEST_ASSERT_VAL("failed to create evsel", evsel);
+
+	err = perf_evsel__open(evsel, NULL, threads);
+	if (err) {
+		pr_err("failed to open evsel: %s\n", strerror(-err));
+		ret = TEST_SKIP;
+		goto out;
+	}
+	opened = true;
+
+	err = perf_evsel__mmap(evsel, 0);
+	if (err) {
+		pr_err("failed to mmap evsel: %s\n", strerror(-err));
+		goto out;
+	}
+	mapped = true;
+
+	pc = perf_evsel__mmap_base(evsel, 0, 0);
+	if (!pc) {
+		pr_err("failed to get mmapped address\n");
+		goto out;
+	}
+
+	if (!pc->cap_user_rdpmc || !pc->index) {
+		pr_err("userspace counter access not %s\n",
+			!pc->cap_user_rdpmc ? "supported" : "enabled");
+		ret = TEST_SKIP;
+		goto out;
+	}
+	if (pc->pmc_width < 32) {
+		pr_err("userspace counter width not set (%d)\n", pc->pmc_width);
+		goto out;
+	}
+
+	perf_evsel__read(evsel, 0, 0, &counts);
+	if (counts.val == 0) {
+		pr_err("failed to read value for evsel\n");
+		goto out;
+	}
+
+	for (i = 0; i < 5; i++) {
+		volatile int count = 0x10000 << i;
+		__u64 start, end, last = 0;
+
+		pr_debug("\tloop = %u, ", count);
+
+		perf_evsel__read(evsel, 0, 0, &counts);
+		start = counts.val;
+
+		while (count--) ;
+
+		perf_evsel__read(evsel, 0, 0, &counts);
+		end = counts.val;
+
+		if ((end - start) < last) {
+			pr_err("invalid counter data: end=%llu start=%llu last= %llu\n",
+				end, start, last);
+			goto out;
+		}
+		last = end - start;
+		pr_debug("count = %llu\n", end - start);
+	}
+	ret = TEST_OK;
+
+out:
+	if (mapped)
+		perf_evsel__munmap(evsel);
+	if (opened)
+		perf_evsel__close(evsel);
+	perf_evsel__delete(evsel);
+
+	perf_thread_map__put(threads);
+	return ret;
+}
+
+static int test__mmap_user_read_instr(struct test_suite *test __maybe_unused,
+				      int subtest __maybe_unused)
+{
+	return test_stat_user_read(PERF_COUNT_HW_INSTRUCTIONS);
+}
+
+static int test__mmap_user_read_cycles(struct test_suite *test __maybe_unused,
+				       int subtest __maybe_unused)
+{
+	return test_stat_user_read(PERF_COUNT_HW_CPU_CYCLES);
+}
+
 static struct test_case tests__basic_mmap[] = {
 	TEST_CASE_REASON("Read samples using the mmap interface",
 			 basic_mmap,
 			 "permissions"),
+	TEST_CASE_REASON("User space counter reading of instructions",
+			 mmap_user_read_instr,
+#if defined(__i386__) || defined(__x86_64__) || defined(__aarch64__)
+			 "permissions"
+#else
+			 "unsupported"
+#endif
+		),
+	TEST_CASE_REASON("User space counter reading of cycles",
+			 mmap_user_read_cycles,
+#if defined(__i386__) || defined(__x86_64__) || defined(__aarch64__)
+			 "permissions"
+#else
+			 "unsupported"
+#endif
+		),
 	{	.name = NULL, }
 };
 
 struct test_suite suite__basic_mmap = {
-	.desc = "Read samples using the mmap interface",
+	.desc = "mmap interface tests",
 	.test_cases = tests__basic_mmap,
 };
-- 
2.37.0.170.g444d1eabd0-goog

Re: [PATCH v3 1/3] perf: Align user space counter reading with code

[Rob Herring]

On Tue, Jul 19, 2022 at 4:39 PM Ian Rogers <irogers@google.com> wrote:
>
> Align the user space counter reading documentation with the code in
> perf_mmap__read_self. Previously the documentation was based on the perf
> rdpmc test, but now general purpose code is provided by libperf.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  include/uapi/linux/perf_event.h       | 35 +++++++++++++++++----------
>  tools/include/uapi/linux/perf_event.h | 35 +++++++++++++++++----------
>  2 files changed, 44 insertions(+), 26 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 3/3] perf test: Add user space counter reading tests

[Vince Weaver]

On Tue, 19 Jul 2022, Ian Rogers wrote:

> These tests are based on test_stat_user_read in
> tools/lib/perf/tests/test-evsel.c. The tests are modified to skip if
> perf_event_open fails or rdpmc isn't supported.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>

> +		.type	= PERF_TYPE_HARDWARE,
> +		.config	= event,
> +#ifdef __aarch64__
> +		.config1 = 0x2,		/* Request user access */
> +#endif

should this value have a name rather than being a "magic constant"?

Vince

Re: [PATCH v3 1/3] perf: Align user space counter reading with code

[Vince Weaver]

On Tue, 19 Jul 2022, Ian Rogers wrote:

> Align the user space counter reading documentation with the code in
> perf_mmap__read_self. Previously the documentation was based on the perf
> rdpmc test, but now general purpose code is provided by libperf.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  include/uapi/linux/perf_event.h       | 35 +++++++++++++++++----------
>  tools/include/uapi/linux/perf_event.h | 35 +++++++++++++++++----------
>  2 files changed, 44 insertions(+), 26 deletions(-)
> 
> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index d37629dbad72..6826dabb7e03 100644
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -538,9 +538,13 @@ struct perf_event_mmap_page {
>  	 *
>  	 *     if (pc->cap_usr_time && enabled != running) {
>  	 *       cyc = rdtsc();
> -	 *       time_offset = pc->time_offset;
>  	 *       time_mult   = pc->time_mult;
>  	 *       time_shift  = pc->time_shift;
> +	 *       time_offset = pc->time_offset;
> +	 *       if (pc->cap_user_time_short) {
> +	 *         time_cycles = pc->time_cycles;
> +	 *         time_mask = pc->time_mask;
> +	 *       }

From what I've been told, and from what perf_mmap__read_self() actually 
does, many of these MMAP fields need to be accessed by READ_ONCE()
(a GPLv2 only interface) to be correct.

Should we update perf_event.h to reflect this?  Otherwise it's confusing 
when the actual code and the documentation in the header don't match like 
this.  As an example, see the actual code snippets from
perf_mmap__read_self()

		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);

...


Vince

Re: [PATCH v3 3/3] perf test: Add user space counter reading tests

[Ian Rogers]

On Wed, Jul 20, 2022 at 7:57 AM Vince Weaver <vincent.weaver@maine.edu> wrote:
>
> On Tue, 19 Jul 2022, Ian Rogers wrote:
>
> > These tests are based on test_stat_user_read in
> > tools/lib/perf/tests/test-evsel.c. The tests are modified to skip if
> > perf_event_open fails or rdpmc isn't supported.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
>
> > +             .type   = PERF_TYPE_HARDWARE,
> > +             .config = event,
> > +#ifdef __aarch64__
> > +             .config1 = 0x2,         /* Request user access */
> > +#endif
>
> should this value have a name rather than being a "magic constant"?

Thanks! Firstly, this is just moving code around and so not a
regression introduced by this code:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/lib/perf/tests/test-evsel.c?h=perf/core#n134
I don't believe there is an existing constant for this purpose. The
kernel isn't using one either:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/perf_event.c#n309

So I agree with you and hopefully this is something ARM will clean up.

Thanks,
Ian

> Vince

Re: [PATCH v3 1/3] perf: Align user space counter reading with code

[Ian Rogers]

On Wed, Jul 20, 2022 at 8:06 AM Vince Weaver <vincent.weaver@maine.edu> wrote:
>
> On Tue, 19 Jul 2022, Ian Rogers wrote:
>
> > Align the user space counter reading documentation with the code in
> > perf_mmap__read_self. Previously the documentation was based on the perf
> > rdpmc test, but now general purpose code is provided by libperf.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  include/uapi/linux/perf_event.h       | 35 +++++++++++++++++----------
> >  tools/include/uapi/linux/perf_event.h | 35 +++++++++++++++++----------
> >  2 files changed, 44 insertions(+), 26 deletions(-)
> >
> > diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> > index d37629dbad72..6826dabb7e03 100644
> > --- a/include/uapi/linux/perf_event.h
> > +++ b/include/uapi/linux/perf_event.h
> > @@ -538,9 +538,13 @@ struct perf_event_mmap_page {
> >        *
> >        *     if (pc->cap_usr_time && enabled != running) {
> >        *       cyc = rdtsc();
> > -      *       time_offset = pc->time_offset;
> >        *       time_mult   = pc->time_mult;
> >        *       time_shift  = pc->time_shift;
> > +      *       time_offset = pc->time_offset;
> > +      *       if (pc->cap_user_time_short) {
> > +      *         time_cycles = pc->time_cycles;
> > +      *         time_mask = pc->time_mask;
> > +      *       }
>
> From what I've been told, and from what perf_mmap__read_self() actually
> does, many of these MMAP fields need to be accessed by READ_ONCE()
> (a GPLv2 only interface) to be correct.
>
> Should we update perf_event.h to reflect this?  Otherwise it's confusing
> when the actual code and the documentation in the header don't match like
> this.  As an example, see the actual code snippets from
> perf_mmap__read_self()
>
>                 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);
>
> ...

Thanks! So I think what this patch proposes is an improvement,
specifically it aligns it better with the code and deals with the
divide by zero. I think what you're asking for makes sense but as you
point out READ_ONCE probably isn't the correct API for something
outside the kernel. Given the kernel is now C11:
https://lwn.net/Articles/885941/
This opens the possibility of using stdatomic.h, so perhaps we can
move these variables to more correct atomic types. So, I think we can
land this and worry about the atomic API in a follow up.

Thanks,
Ian

> Vince

Re: [PATCH v3 1/3] perf: Align user space counter reading with code

[Arnaldo Carvalho de Melo]

Em Tue, Jul 19, 2022 at 03:39:44PM -0700, Ian Rogers escreveu:
> Align the user space counter reading documentation with the code in
> perf_mmap__read_self. Previously the documentation was based on the perf
> rdpmc test, but now general purpose code is provided by libperf.

Peter, can you merge this so as not to make Linus raise eyebrows with me
processing things outside tools/perf/ when asking him to pull perf
userspace?

- Arnaldo
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  include/uapi/linux/perf_event.h       | 35 +++++++++++++++++----------
>  tools/include/uapi/linux/perf_event.h | 35 +++++++++++++++++----------
>  2 files changed, 44 insertions(+), 26 deletions(-)
> 
> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index d37629dbad72..6826dabb7e03 100644
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -538,9 +538,13 @@ struct perf_event_mmap_page {
>  	 *
>  	 *     if (pc->cap_usr_time && enabled != running) {
>  	 *       cyc = rdtsc();
> -	 *       time_offset = pc->time_offset;
>  	 *       time_mult   = pc->time_mult;
>  	 *       time_shift  = pc->time_shift;
> +	 *       time_offset = pc->time_offset;
> +	 *       if (pc->cap_user_time_short) {
> +	 *         time_cycles = pc->time_cycles;
> +	 *         time_mask = pc->time_mask;
> +	 *       }
>  	 *     }
>  	 *
>  	 *     index = pc->index;
> @@ -548,6 +552,9 @@ struct perf_event_mmap_page {
>  	 *     if (pc->cap_user_rdpmc && index) {
>  	 *       width = pc->pmc_width;
>  	 *       pmc = rdpmc(index - 1);
> +	 *       pmc <<= 64 - width;
> +	 *       pmc >>= 64 - width;
> +	 *       count += pmc;
>  	 *     }
>  	 *
>  	 *     barrier();
> @@ -590,25 +597,27 @@ struct perf_event_mmap_page {
>  	 * If cap_usr_time the below fields can be used to compute the time
>  	 * delta since time_enabled (in ns) using rdtsc or similar.
>  	 *
> -	 *   u64 quot, rem;
> -	 *   u64 delta;
> -	 *
> -	 *   quot = (cyc >> time_shift);
> -	 *   rem = cyc & (((u64)1 << time_shift) - 1);
> -	 *   delta = time_offset + quot * time_mult +
> -	 *              ((rem * time_mult) >> time_shift);
> +	 *   cyc = time_cycles + ((cyc - time_cycles) & time_mask);
> +	 *   delta = time_offset + mul_u64_u32_shr(cyc, time_mult, time_shift);
>  	 *
>  	 * Where time_offset,time_mult,time_shift and cyc are read in the
> -	 * seqcount loop described above. This delta can then be added to
> -	 * enabled and possible running (if index), improving the scaling:
> +	 * seqcount loop described above. mul_u64_u32_shr will compute:
> +	 *
> +	 *   (u64)(((unsigned __int128)cyc * time_mult) >> time_shift)
> +	 *
> +	 * This delta can then be added to enabled and possible running (if
> +	 * index) to improve the scaling. Due to event multiplexing, running
> +	 * may be zero and so care is needed to avoid division by zero.
>  	 *
>  	 *   enabled += delta;
>  	 *   if (index)
>  	 *     running += delta;
>  	 *
> -	 *   quot = count / running;
> -	 *   rem  = count % running;
> -	 *   count = quot * enabled + (rem * enabled) / running;
> +	 *   if (running != 0) {
> +	 *     quot = count / running;
> +	 *     rem  = count % running;
> +	 *     count = quot * enabled + (rem * enabled) / running;
> +	 *   }
>  	 */
>  	__u16	time_shift;
>  	__u32	time_mult;
> diff --git a/tools/include/uapi/linux/perf_event.h b/tools/include/uapi/linux/perf_event.h
> index d37629dbad72..6826dabb7e03 100644
> --- a/tools/include/uapi/linux/perf_event.h
> +++ b/tools/include/uapi/linux/perf_event.h
> @@ -538,9 +538,13 @@ struct perf_event_mmap_page {
>  	 *
>  	 *     if (pc->cap_usr_time && enabled != running) {
>  	 *       cyc = rdtsc();
> -	 *       time_offset = pc->time_offset;
>  	 *       time_mult   = pc->time_mult;
>  	 *       time_shift  = pc->time_shift;
> +	 *       time_offset = pc->time_offset;
> +	 *       if (pc->cap_user_time_short) {
> +	 *         time_cycles = pc->time_cycles;
> +	 *         time_mask = pc->time_mask;
> +	 *       }
>  	 *     }
>  	 *
>  	 *     index = pc->index;
> @@ -548,6 +552,9 @@ struct perf_event_mmap_page {
>  	 *     if (pc->cap_user_rdpmc && index) {
>  	 *       width = pc->pmc_width;
>  	 *       pmc = rdpmc(index - 1);
> +	 *       pmc <<= 64 - width;
> +	 *       pmc >>= 64 - width;
> +	 *       count += pmc;
>  	 *     }
>  	 *
>  	 *     barrier();
> @@ -590,25 +597,27 @@ struct perf_event_mmap_page {
>  	 * If cap_usr_time the below fields can be used to compute the time
>  	 * delta since time_enabled (in ns) using rdtsc or similar.
>  	 *
> -	 *   u64 quot, rem;
> -	 *   u64 delta;
> -	 *
> -	 *   quot = (cyc >> time_shift);
> -	 *   rem = cyc & (((u64)1 << time_shift) - 1);
> -	 *   delta = time_offset + quot * time_mult +
> -	 *              ((rem * time_mult) >> time_shift);
> +	 *   cyc = time_cycles + ((cyc - time_cycles) & time_mask);
> +	 *   delta = time_offset + mul_u64_u32_shr(cyc, time_mult, time_shift);
>  	 *
>  	 * Where time_offset,time_mult,time_shift and cyc are read in the
> -	 * seqcount loop described above. This delta can then be added to
> -	 * enabled and possible running (if index), improving the scaling:
> +	 * seqcount loop described above. mul_u64_u32_shr will compute:
> +	 *
> +	 *   (u64)(((unsigned __int128)cyc * time_mult) >> time_shift)
> +	 *
> +	 * This delta can then be added to enabled and possible running (if
> +	 * index) to improve the scaling. Due to event multiplexing, running
> +	 * may be zero and so care is needed to avoid division by zero.
>  	 *
>  	 *   enabled += delta;
>  	 *   if (index)
>  	 *     running += delta;
>  	 *
> -	 *   quot = count / running;
> -	 *   rem  = count % running;
> -	 *   count = quot * enabled + (rem * enabled) / running;
> +	 *   if (running != 0) {
> +	 *     quot = count / running;
> +	 *     rem  = count % running;
> +	 *     count = quot * enabled + (rem * enabled) / running;
> +	 *   }
>  	 */
>  	__u16	time_shift;
>  	__u32	time_mult;
> -- 
> 2.37.0.170.g444d1eabd0-goog

-- 

- Arnaldo

Re: [PATCH v3 2/3] perf test: Remove x86 rdpmc test

[Arnaldo Carvalho de Melo]

Em Tue, Jul 19, 2022 at 03:39:45PM -0700, Ian Rogers escreveu:
> This test has been superseded by test_stat_user_read in:
> tools/lib/perf/tests/test-evsel.c
> The updated test doesn't divide-by-0 when running time of a counter is
> 0. It also supports ARM64.

Thanks, applied.

- Arnaldo

 
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/arch/x86/tests/Build        |   1 -
>  tools/perf/arch/x86/tests/arch-tests.c |   2 -
>  tools/perf/arch/x86/tests/rdpmc.c      | 182 -------------------------
>  3 files changed, 185 deletions(-)
>  delete mode 100644 tools/perf/arch/x86/tests/rdpmc.c
> 
> diff --git a/tools/perf/arch/x86/tests/Build b/tools/perf/arch/x86/tests/Build
> index 28d793390198..70b5bcbc15df 100644
> --- a/tools/perf/arch/x86/tests/Build
> +++ b/tools/perf/arch/x86/tests/Build
> @@ -2,7 +2,6 @@ perf-$(CONFIG_DWARF_UNWIND) += regs_load.o
>  perf-$(CONFIG_DWARF_UNWIND) += dwarf-unwind.o
>  
>  perf-y += arch-tests.o
> -perf-y += rdpmc.o
>  perf-y += sample-parsing.o
>  perf-$(CONFIG_AUXTRACE) += insn-x86.o intel-pt-pkt-decoder-test.o
>  perf-$(CONFIG_X86_64) += bp-modify.o
> diff --git a/tools/perf/arch/x86/tests/arch-tests.c b/tools/perf/arch/x86/tests/arch-tests.c
> index 64fb73d14d2f..04018b8aa85b 100644
> --- a/tools/perf/arch/x86/tests/arch-tests.c
> +++ b/tools/perf/arch/x86/tests/arch-tests.c
> @@ -3,7 +3,6 @@
>  #include "tests/tests.h"
>  #include "arch-tests.h"
>  
> -DEFINE_SUITE("x86 rdpmc", rdpmc);
>  #ifdef HAVE_AUXTRACE_SUPPORT
>  DEFINE_SUITE("x86 instruction decoder - new instructions", insn_x86);
>  DEFINE_SUITE("Intel PT packet decoder", intel_pt_pkt_decoder);
> @@ -14,7 +13,6 @@ DEFINE_SUITE("x86 bp modify", bp_modify);
>  DEFINE_SUITE("x86 Sample parsing", x86_sample_parsing);
>  
>  struct test_suite *arch_tests[] = {
> -	&suite__rdpmc,
>  #ifdef HAVE_DWARF_UNWIND_SUPPORT
>  	&suite__dwarf_unwind,
>  #endif
> diff --git a/tools/perf/arch/x86/tests/rdpmc.c b/tools/perf/arch/x86/tests/rdpmc.c
> deleted file mode 100644
> index 498413ad9c97..000000000000
> --- a/tools/perf/arch/x86/tests/rdpmc.c
> +++ /dev/null
> @@ -1,182 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -#include <errno.h>
> -#include <unistd.h>
> -#include <stdlib.h>
> -#include <signal.h>
> -#include <sys/mman.h>
> -#include <sys/types.h>
> -#include <sys/wait.h>
> -#include <linux/string.h>
> -#include <linux/types.h>
> -#include "perf-sys.h"
> -#include "debug.h"
> -#include "tests/tests.h"
> -#include "cloexec.h"
> -#include "event.h"
> -#include <internal/lib.h> // page_size
> -#include "arch-tests.h"
> -
> -static u64 rdpmc(unsigned int counter)
> -{
> -	unsigned int low, high;
> -
> -	asm volatile("rdpmc" : "=a" (low), "=d" (high) : "c" (counter));
> -
> -	return low | ((u64)high) << 32;
> -}
> -
> -static u64 rdtsc(void)
> -{
> -	unsigned int low, high;
> -
> -	asm volatile("rdtsc" : "=a" (low), "=d" (high));
> -
> -	return low | ((u64)high) << 32;
> -}
> -
> -static u64 mmap_read_self(void *addr)
> -{
> -	struct perf_event_mmap_page *pc = addr;
> -	u32 seq, idx, time_mult = 0, time_shift = 0;
> -	u64 count, cyc = 0, time_offset = 0, enabled, running, delta;
> -
> -	do {
> -		seq = pc->lock;
> -		barrier();
> -
> -		enabled = pc->time_enabled;
> -		running = pc->time_running;
> -
> -		if (enabled != running) {
> -			cyc = rdtsc();
> -			time_mult = pc->time_mult;
> -			time_shift = pc->time_shift;
> -			time_offset = pc->time_offset;
> -		}
> -
> -		idx = pc->index;
> -		count = pc->offset;
> -		if (idx)
> -			count += rdpmc(idx - 1);
> -
> -		barrier();
> -	} while (pc->lock != seq);
> -
> -	if (enabled != running) {
> -		u64 quot, rem;
> -
> -		quot = (cyc >> time_shift);
> -		rem = cyc & (((u64)1 << time_shift) - 1);
> -		delta = time_offset + quot * time_mult +
> -			((rem * time_mult) >> time_shift);
> -
> -		enabled += delta;
> -		if (idx)
> -			running += delta;
> -
> -		quot = count / running;
> -		rem = count % running;
> -		count = quot * enabled + (rem * enabled) / running;
> -	}
> -
> -	return count;
> -}
> -
> -/*
> - * If the RDPMC instruction faults then signal this back to the test parent task:
> - */
> -static void segfault_handler(int sig __maybe_unused,
> -			     siginfo_t *info __maybe_unused,
> -			     void *uc __maybe_unused)
> -{
> -	exit(-1);
> -}
> -
> -static int __test__rdpmc(void)
> -{
> -	volatile int tmp = 0;
> -	u64 i, loops = 1000;
> -	int n;
> -	int fd;
> -	void *addr;
> -	struct perf_event_attr attr = {
> -		.type = PERF_TYPE_HARDWARE,
> -		.config = PERF_COUNT_HW_INSTRUCTIONS,
> -		.exclude_kernel = 1,
> -	};
> -	u64 delta_sum = 0;
> -        struct sigaction sa;
> -	char sbuf[STRERR_BUFSIZE];
> -
> -	sigfillset(&sa.sa_mask);
> -	sa.sa_sigaction = segfault_handler;
> -	sa.sa_flags = 0;
> -	sigaction(SIGSEGV, &sa, NULL);
> -
> -	fd = sys_perf_event_open(&attr, 0, -1, -1,
> -				 perf_event_open_cloexec_flag());
> -	if (fd < 0) {
> -		pr_err("Error: sys_perf_event_open() syscall returned "
> -		       "with %d (%s)\n", fd,
> -		       str_error_r(errno, sbuf, sizeof(sbuf)));
> -		return -1;
> -	}
> -
> -	addr = mmap(NULL, page_size, PROT_READ, MAP_SHARED, fd, 0);
> -	if (addr == (void *)(-1)) {
> -		pr_err("Error: mmap() syscall returned with (%s)\n",
> -		       str_error_r(errno, sbuf, sizeof(sbuf)));
> -		goto out_close;
> -	}
> -
> -	for (n = 0; n < 6; n++) {
> -		u64 stamp, now, delta;
> -
> -		stamp = mmap_read_self(addr);
> -
> -		for (i = 0; i < loops; i++)
> -			tmp++;
> -
> -		now = mmap_read_self(addr);
> -		loops *= 10;
> -
> -		delta = now - stamp;
> -		pr_debug("%14d: %14Lu\n", n, (long long)delta);
> -
> -		delta_sum += delta;
> -	}
> -
> -	munmap(addr, page_size);
> -	pr_debug("   ");
> -out_close:
> -	close(fd);
> -
> -	if (!delta_sum)
> -		return -1;
> -
> -	return 0;
> -}
> -
> -int test__rdpmc(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
> -{
> -	int status = 0;
> -	int wret = 0;
> -	int ret;
> -	int pid;
> -
> -	pid = fork();
> -	if (pid < 0)
> -		return -1;
> -
> -	if (!pid) {
> -		ret = __test__rdpmc();
> -
> -		exit(ret);
> -	}
> -
> -	wret = waitpid(pid, &status, 0);
> -	if (wret < 0 || status)
> -		return -1;
> -
> -	return 0;
> -}
> -- 
> 2.37.0.170.g444d1eabd0-goog

-- 

- Arnaldo

Re: [PATCH v3 3/3] perf test: Add user space counter reading tests

[Arnaldo Carvalho de Melo]

Em Tue, Jul 19, 2022 at 03:39:46PM -0700, Ian Rogers escreveu:
> These tests are based on test_stat_user_read in
> tools/lib/perf/tests/test-evsel.c. The tests are modified to skip if
> perf_event_open fails or rdpmc isn't supported.

Thanks, tested and applied.

- Arnaldo

 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/tests/mmap-basic.c | 127 +++++++++++++++++++++++++++++++++-
>  1 file changed, 126 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/tests/mmap-basic.c b/tools/perf/tests/mmap-basic.c
> index 30bbe144648a..dfb6173b2a82 100644
> --- a/tools/perf/tests/mmap-basic.c
> +++ b/tools/perf/tests/mmap-basic.c
> @@ -170,14 +170,139 @@ static int test__basic_mmap(struct test_suite *test __maybe_unused, int subtest
>  	return err;
>  }
>  
> +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,
> +#ifdef __aarch64__
> +		.config1 = 0x2,		/* Request user access */
> +#endif
> +	};
> +	int err, i, ret = TEST_FAIL;
> +	bool opened = false, mapped = false;
> +
> +	threads = perf_thread_map__new_dummy();
> +	TEST_ASSERT_VAL("failed to create threads", threads);
> +
> +	perf_thread_map__set_pid(threads, 0, 0);
> +
> +	evsel = perf_evsel__new(&attr);
> +	TEST_ASSERT_VAL("failed to create evsel", evsel);
> +
> +	err = perf_evsel__open(evsel, NULL, threads);
> +	if (err) {
> +		pr_err("failed to open evsel: %s\n", strerror(-err));
> +		ret = TEST_SKIP;
> +		goto out;
> +	}
> +	opened = true;
> +
> +	err = perf_evsel__mmap(evsel, 0);
> +	if (err) {
> +		pr_err("failed to mmap evsel: %s\n", strerror(-err));
> +		goto out;
> +	}
> +	mapped = true;
> +
> +	pc = perf_evsel__mmap_base(evsel, 0, 0);
> +	if (!pc) {
> +		pr_err("failed to get mmapped address\n");
> +		goto out;
> +	}
> +
> +	if (!pc->cap_user_rdpmc || !pc->index) {
> +		pr_err("userspace counter access not %s\n",
> +			!pc->cap_user_rdpmc ? "supported" : "enabled");
> +		ret = TEST_SKIP;
> +		goto out;
> +	}
> +	if (pc->pmc_width < 32) {
> +		pr_err("userspace counter width not set (%d)\n", pc->pmc_width);
> +		goto out;
> +	}
> +
> +	perf_evsel__read(evsel, 0, 0, &counts);
> +	if (counts.val == 0) {
> +		pr_err("failed to read value for evsel\n");
> +		goto out;
> +	}
> +
> +	for (i = 0; i < 5; i++) {
> +		volatile int count = 0x10000 << i;
> +		__u64 start, end, last = 0;
> +
> +		pr_debug("\tloop = %u, ", count);
> +
> +		perf_evsel__read(evsel, 0, 0, &counts);
> +		start = counts.val;
> +
> +		while (count--) ;
> +
> +		perf_evsel__read(evsel, 0, 0, &counts);
> +		end = counts.val;
> +
> +		if ((end - start) < last) {
> +			pr_err("invalid counter data: end=%llu start=%llu last= %llu\n",
> +				end, start, last);
> +			goto out;
> +		}
> +		last = end - start;
> +		pr_debug("count = %llu\n", end - start);
> +	}
> +	ret = TEST_OK;
> +
> +out:
> +	if (mapped)
> +		perf_evsel__munmap(evsel);
> +	if (opened)
> +		perf_evsel__close(evsel);
> +	perf_evsel__delete(evsel);
> +
> +	perf_thread_map__put(threads);
> +	return ret;
> +}
> +
> +static int test__mmap_user_read_instr(struct test_suite *test __maybe_unused,
> +				      int subtest __maybe_unused)
> +{
> +	return test_stat_user_read(PERF_COUNT_HW_INSTRUCTIONS);
> +}
> +
> +static int test__mmap_user_read_cycles(struct test_suite *test __maybe_unused,
> +				       int subtest __maybe_unused)
> +{
> +	return test_stat_user_read(PERF_COUNT_HW_CPU_CYCLES);
> +}
> +
>  static struct test_case tests__basic_mmap[] = {
>  	TEST_CASE_REASON("Read samples using the mmap interface",
>  			 basic_mmap,
>  			 "permissions"),
> +	TEST_CASE_REASON("User space counter reading of instructions",
> +			 mmap_user_read_instr,
> +#if defined(__i386__) || defined(__x86_64__) || defined(__aarch64__)
> +			 "permissions"
> +#else
> +			 "unsupported"
> +#endif
> +		),
> +	TEST_CASE_REASON("User space counter reading of cycles",
> +			 mmap_user_read_cycles,
> +#if defined(__i386__) || defined(__x86_64__) || defined(__aarch64__)
> +			 "permissions"
> +#else
> +			 "unsupported"
> +#endif
> +		),
>  	{	.name = NULL, }
>  };
>  
>  struct test_suite suite__basic_mmap = {
> -	.desc = "Read samples using the mmap interface",
> +	.desc = "mmap interface tests",
>  	.test_cases = tests__basic_mmap,
>  };
> -- 
> 2.37.0.170.g444d1eabd0-goog

-- 

- Arnaldo

Re: [PATCH v3 1/3] perf: Align user space counter reading with code

[Ingo Molnar]

* Vince Weaver <vincent.weaver@maine.edu> wrote:

> On Tue, 19 Jul 2022, Ian Rogers wrote:
> 
> > Align the user space counter reading documentation with the code in
> > perf_mmap__read_self. Previously the documentation was based on the perf
> > rdpmc test, but now general purpose code is provided by libperf.
> > 
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  include/uapi/linux/perf_event.h       | 35 +++++++++++++++++----------
> >  tools/include/uapi/linux/perf_event.h | 35 +++++++++++++++++----------
> >  2 files changed, 44 insertions(+), 26 deletions(-)
> > 
> > diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> > index d37629dbad72..6826dabb7e03 100644
> > --- a/include/uapi/linux/perf_event.h
> > +++ b/include/uapi/linux/perf_event.h
> > @@ -538,9 +538,13 @@ struct perf_event_mmap_page {
> >  	 *
> >  	 *     if (pc->cap_usr_time && enabled != running) {
> >  	 *       cyc = rdtsc();
> > -	 *       time_offset = pc->time_offset;
> >  	 *       time_mult   = pc->time_mult;
> >  	 *       time_shift  = pc->time_shift;
> > +	 *       time_offset = pc->time_offset;
> > +	 *       if (pc->cap_user_time_short) {
> > +	 *         time_cycles = pc->time_cycles;
> > +	 *         time_mask = pc->time_mask;
> > +	 *       }
> 
> From what I've been told, and from what perf_mmap__read_self() actually 
> does, many of these MMAP fields need to be accessed by READ_ONCE()
> (a GPLv2 only interface) to be correct.

BTW., for this purpose I guess we could add a READ_ONCE() variant to perf 
tooling that reimplements it with more relaxed licensing, so that the 
headers & sample code can be included in GPL-incompatible projects?

READ_ONCE() isn't a super complicated primitive, and we've always been 
permissive with simple primitives.

Thanks,

	Ingo