libperf and arm64 userspace counter access support

From: Raphael Gault <[email protected]>

Keep track of event opened with direct access to the hardware counters and modify permissions while they are open.

The strategy used here is the same which x86 uses: everytime an event is mapped, the permissions are set if required. The atomic field added in the mm_context helps keep track of the different event opened and de-activate the permissions when all are unmapped.
We also need to update the permissions in the context switch code so that tasks keep the right permissions.

Signed-off-by Raphael Gault <[email protected]>

Signed-off-by Rob Herring <[email protected]>

---
v2:
- Move mapped/unmapped into arm64 code. Fixes arm32.
- Rebase on cap_user_time_short changes

Changes from Raphael's v4:
- Drop homogeneous check
- Disable access for chained counters
- Set pmc_width in user page

---
 arch/arm64/include/asm/mmu.h         |  5 ++++
 arch/arm64/include/asm/mmu_context.h |  2 ++
 arch/arm64/include/asm/perf_event.h  | 14 ++++++++++
 arch/arm64/kernel/perf_event.c       | 41 ++++++++++++++++++++++++++++
 4 files changed, 62 insertions(+)

arch/arm64/include/asm/mmu.h +5 -0

index a7a5ecaa2e83..52cfdb676f06 100644
@@ -19,6 +19,11 @@
 
 typedef struct {
 	atomic64_t	id;
+	/*
+	 * non-zero if userspace have access to hardware
+	 * counters directly.
+	 */
+	atomic_t	pmu_direct_access;
 #ifdef CONFIG_COMPAT
 	void		*sigpage;
 #endif

arch/arm64/include/asm/mmu_context.h +2 -0

index f2d7537d6f83..d24589ecb07a 100644
@@ -21,6 +21,7 @@
 #include <asm/proc-fns.h>
 #include <asm-generic/mm_hooks.h>
 #include <asm/cputype.h>
+#include <asm/perf_event.h>
 #include <asm/sysreg.h>
 #include <asm/tlbflush.h>
 

@@ -224,6 +225,7 @@ static inline void __switch_mm(struct mm_struct *next)
 	}
 
 	check_and_switch_context(next);
+	perf_switch_user_access(next);
 }
 
 static inline void

arch/arm64/include/asm/perf_event.h +3 -0

index 2c2d7dbe8a02..a025d9595d51 100644
@@ -8,6 +8,7 @@
 
 #include <asm/stack_pointer.h>
 #include <asm/ptrace.h>
+#include <linux/mm_types.h>
 
 #define	ARMV8_PMU_MAX_COUNTERS	32
 #define	ARMV8_PMU_COUNTER_MASK	(ARMV8_PMU_MAX_COUNTERS - 1)

@@ -251,4 +252,17 @@ extern unsigned long perf_misc_flags(struct pt_regs *regs);
 	(regs)->pstate = PSR_MODE_EL1h;	\
 }
 
+static inline void perf_switch_user_access(struct mm_struct *mm)
+{

arch/arm64/include/asm/perf_event.h +1 -0

+
 #endif

arch/arm64/kernel/perf_event.c +24 -0

index e14f360a7883..8344dc030823 100644
@@ -834,6 +834,41 @@ static int armv8pmu_access_event_idx(struct perf_event *event)
 	return event->hw.idx;
 }
 
+static void refresh_pmuserenr(void *mm)
+{
+	perf_switch_user_access(mm);
+}
+
+static void armv8pmu_event_mapped(struct perf_event *event, struct mm_struct *mm)
+{
+	if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR))
+		return;
+
+	/*
+	 * This function relies on not being called concurrently in two
+	 * tasks in the same mm.  Otherwise one task could observe
+	 * pmu_direct_access > 1 and return all the way back to
+	 * userspace with user access disabled while another task is still
+	 * doing on_each_cpu_mask() to enable user access.
+	 *
+	 * For now, this can't happen because all callers hold mmap_lock
+	 * for write.  If this changes, we'll need a different solution.
+	 */
+	lockdep_assert_held_write(&mm->mmap_lock);
+
+	if (atomic_inc_return(&mm->context.pmu_direct_access) == 1)
+		on_each_cpu(refresh_pmuserenr, mm, 1);

arch/arm64/kernel/perf_event.c +7 -0

+
 /*
  * Add an event filter to a given event.
  */

@@ -1058,6 +1093,8 @@ static int armv8_pmu_init(struct arm_pmu *cpu_pmu, char *name,
 	cpu_pmu->filter_match		= armv8pmu_filter_match;
 
 	cpu_pmu->pmu.event_idx		= armv8pmu_access_event_idx;
+	cpu_pmu->pmu.event_mapped	= armv8pmu_event_mapped;
+	cpu_pmu->pmu.event_unmapped	= armv8pmu_event_unmapped;
 
 	cpu_pmu->name			= name;
 	cpu_pmu->map_event		= map_event;

@@ -1218,6 +1255,10 @@ void arch_perf_update_userpage(struct perf_event *event,
 	userpg->cap_user_time = 0;
 	userpg->cap_user_time_zero = 0;
 	userpg->cap_user_time_short = 0;
+	userpg->cap_user_rdpmc = !!(event->hw.flags & ARMPMU_EL0_RD_CNTR);
+
+	if (userpg->cap_user_rdpmc)
+		userpg->pmc_width = armv8pmu_event_is_64bit(event) ? 64 : 32;
 
 	do {
 		rd = sched_clock_read_begin(&seq);

Signature · 1 line

2.25.1

Hi Rob,

Thanks for this, and sorry for the long delay since this was last reviewed. Overall this is looking pretty good, but I have a couple of remaining concerns.

Will, I have a query for you below.

Quoted 93 lines · Rob Herring

From: Raphael Gault <[email protected]>

Keep track of event opened with direct access to the hardware counters and modify permissions while they are open.

The strategy used here is the same which x86 uses: everytime an event is mapped, the permissions are set if required. The atomic field added in the mm_context helps keep track of the different event opened and de-activate the permissions when all are unmapped.
We also need to update the permissions in the context switch code so that tasks keep the right permissions.

Signed-off-by Raphael Gault <[email protected]>

Signed-off-by Rob Herring <[email protected]>

---
v2:
- Move mapped/unmapped into arm64 code. Fixes arm32.
- Rebase on cap_user_time_short changes

Changes from Raphael's v4:
- Drop homogeneous check
- Disable access for chained counters
- Set pmc_width in user page

---
 arch/arm64/include/asm/mmu.h         |  5 ++++
 arch/arm64/include/asm/mmu_context.h |  2 ++
 arch/arm64/include/asm/perf_event.h  | 14 ++++++++++
 arch/arm64/kernel/perf_event.c       | 41 ++++++++++++++++++++++++++++
 4 files changed, 62 insertions(+)

arch/arm64/include/asm/mmu.h +5 -0

index a7a5ecaa2e83..52cfdb676f06 100644
@@ -19,6 +19,11 @@
 
 typedef struct {
      atomic64_t      id;
+     /*
+      * non-zero if userspace have access to hardware
+      * counters directly.
+      */
+     atomic_t        pmu_direct_access;
 #ifdef CONFIG_COMPAT
      void            *sigpage;
 #endif

arch/arm64/include/asm/mmu_context.h +2 -0

index f2d7537d6f83..d24589ecb07a 100644
@@ -21,6 +21,7 @@
 #include <asm/proc-fns.h>
 #include <asm-generic/mm_hooks.h>
 #include <asm/cputype.h>
+#include <asm/perf_event.h>
 #include <asm/sysreg.h>
 #include <asm/tlbflush.h>
 

@@ -224,6 +225,7 @@ static inline void __switch_mm(struct mm_struct *next)
      }
 
      check_and_switch_context(next);
+     perf_switch_user_access(next);
 }
 
 static inline void

arch/arm64/include/asm/perf_event.h +12 -0

index 2c2d7dbe8a02..a025d9595d51 100644
@@ -8,6 +8,7 @@
 
 #include <asm/stack_pointer.h>
 #include <asm/ptrace.h>
+#include <linux/mm_types.h>
 
 #define      ARMV8_PMU_MAX_COUNTERS  32
 #define      ARMV8_PMU_COUNTER_MASK  (ARMV8_PMU_MAX_COUNTERS - 1)

@@ -251,4 +252,17 @@ extern unsigned long perf_misc_flags(struct pt_regs *regs);
      (regs)->pstate = PSR_MODE_EL1h; \
 }
 
+static inline void perf_switch_user_access(struct mm_struct *mm)
+{
+     if (!IS_ENABLED(CONFIG_PERF_EVENTS))
+             return;
+
+     if (atomic_read(&mm->context.pmu_direct_access)) {
+             write_sysreg(ARMV8_PMU_USERENR_ER|ARMV8_PMU_USERENR_CR,
+                          pmuserenr_el0);
+     } else {
+             write_sysreg(0, pmuserenr_el0);
+     }

arch/arm64/include/asm/perf_event.h extern unsigned long perf_misc_flags(struct pt_regs *regs); · line 266

Quoted 1 line

arch/arm64/include/asm/perf_event.h +1 -0

+}

PMUSERENR.ER gives RW access to PMSELR_EL0. While we no longer use PMSELR_EL0 in the kernel, we can preempt and migrate userspace tasks between homogeneous CPUs, and presumably need to context-switch this with the task (like we do for TPIDR_EL0 and friends), or clear the register on context-switch to prevent it becoming an unintended covert channel.

These bits also enable AArch32 access. Is there any way an AArch32 task can enable this? If so we should probably block that given we do not support this interface on 32-bit arm.

arch/arm64/kernel/perf_event.c static int armv8pmu_access_event_idx(struct perf_event *event) · line 870

Quoted 8 lines · Rob Herring

arch/arm64/kernel/perf_event.c +8 -0

+static void armv8pmu_event_unmapped(struct perf_event *event, struct mm_struct *mm)
+{
+	if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR))
+		return;
+
+	if (atomic_dec_and_test(&mm->context.pmu_direct_access))
+		on_each_cpu_mask(mm_cpumask(mm), refresh_pmuserenr, NULL, 1);
+}

I didn't think we kept our mm_cpumask() up-to-date in all cases on arm64, so I'm not sure we can use it like this.

Will, can you confirm either way?

Quoted 13 lines · Mark Rutland

arch/arm64/kernel/perf_event.c static int armv8pmu_access_event_idx(struct perf_event *event) · line 870

Quoted 8 lines · Rob Herring

arch/arm64/kernel/perf_event.c +8 -0

+static void armv8pmu_event_unmapped(struct perf_event *event, struct mm_struct *mm)
+{
+	if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR))
+		return;
+
+	if (atomic_dec_and_test(&mm->context.pmu_direct_access))
+		on_each_cpu_mask(mm_cpumask(mm), refresh_pmuserenr, NULL, 1);
+}

I didn't think we kept our mm_cpumask() up-to-date in all cases on arm64, so I'm not sure we can use it like this.

Quoted 1 line

Will, can you confirm either way?

We don't update mm_cpumask() as the cost of the atomic showed up in some benchmarks I did years ago and we've never had any need for the thing anyway because out TLB invalidation is one or all.

Quoted 14 lines · Will Deacon

Quoted 13 lines · Mark Rutland

arch/arm64/kernel/perf_event.c static int armv8pmu_access_event_idx(struct perf_event *event) · line 870

Quoted 8 lines · Rob Herring

arch/arm64/kernel/perf_event.c +8 -0

+static void armv8pmu_event_unmapped(struct perf_event *event, struct mm_struct *mm)
+{
+	if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR))
+		return;
+
+	if (atomic_dec_and_test(&mm->context.pmu_direct_access))
+		on_each_cpu_mask(mm_cpumask(mm), refresh_pmuserenr, NULL, 1);
+}

I didn't think we kept our mm_cpumask() up-to-date in all cases on arm64, so I'm not sure we can use it like this.

Quoted 5 lines

Quoted 1 line

Will, can you confirm either way?

We don't update mm_cpumask() as the cost of the atomic showed up in some benchmarks I did years ago and we've never had any need for the thing anyway because out TLB invalidation is one or all.

That's good because we're also passing NULL instead of mm which would crash. So it must be more than it's not up to date, but it's always 0. It looks like event_mapped on x86 uses mm_cpumask(mm) which I guess was dropped when copying this code as it didn't work... For reference, the x86 version of this originated in commit 7911d3f7af14a6.

I'm not clear on why we need to update pmuserenr_el0 here anyways. To get here userspace has to mmap the event and then unmmap it. If we did nothing, then counter accesses would not fault until the next context switch.

If you all have any ideas, I'm all ears. I'm not a scheduler nor perf hacker. ;)

Quoted 14 lines · Will Deacon

Quoted 13 lines · Mark Rutland

arch/arm64/kernel/perf_event.c static int armv8pmu_access_event_idx(struct perf_event *event) · line 870

Quoted 8 lines · Rob Herring

arch/arm64/kernel/perf_event.c +8 -0

+static void armv8pmu_event_unmapped(struct perf_event *event, struct mm_struct *mm)
+{
+	if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR))
+		return;
+
+	if (atomic_dec_and_test(&mm->context.pmu_direct_access))
+		on_each_cpu_mask(mm_cpumask(mm), refresh_pmuserenr, NULL, 1);
+}

I didn't think we kept our mm_cpumask() up-to-date in all cases on arm64, so I'm not sure we can use it like this.

Quoted 5 lines

Quoted 1 line

Will, can you confirm either way?

We don't update mm_cpumask() as the cost of the atomic showed up in some benchmarks I did years ago and we've never had any need for the thing anyway because out TLB invalidation is one or all.

That's good because we're also passing NULL instead of mm which would crash. So it must be more than it's not up to date, but it's always 0. It looks like event_mapped on x86 uses mm_cpumask(mm) which I guess was dropped when copying this code as it didn't work... For reference, the x86 version of this originated in commit 7911d3f7af14a6.

I'm not clear on why we need to update pmuserenr_el0 here anyways. To get here userspace has to mmap the event and then unmmap it. If we did nothing, then counter accesses would not fault until the next context switch.

If you all have any ideas, I'm all ears. I'm not a scheduler nor perf hacker. ;)

Quoted 3 lines · Rob Herring

Quoted 2 lines · Will Deacon

Quoted 1 line · Mark Rutland

On Thu, Oct 01, 2020 at 09:01:09AM -0500, Rob Herring wrote:

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

arch/arm64/kernel/perf_event.c static int armv8pmu_access_event_idx(struct perf_event *event) · line 870

Quoted 8 lines

arch/arm64/kernel/perf_event.c +8 -0

+static void armv8pmu_event_unmapped(struct perf_event *event, struct mm_struct *mm)
+{
+	if (!(event->hw.flags & ARMPMU_EL0_RD_CNTR))
+		return;
+
+	if (atomic_dec_and_test(&mm->context.pmu_direct_access))
+		on_each_cpu_mask(mm_cpumask(mm), refresh_pmuserenr, NULL, 1);
+}

Quoted 16 lines

Quoted 9 lines

Quoted 5 lines

I didn't think we kept our mm_cpumask() up-to-date in all cases on arm64, so I'm not sure we can use it like this.

Will, can you confirm either way?

We don't update mm_cpumask() as the cost of the atomic showed up in some benchmarks I did years ago and we've never had any need for the thing anyway because out TLB invalidation is one or all.

That's good because we're also passing NULL instead of mm which would crash. So it must be more than it's not up to date, but it's always 0. It looks like event_mapped on x86 uses mm_cpumask(mm) which I guess was dropped when copying this code as it didn't work... For reference, the x86 version of this originated in commit 7911d3f7af14a6.

Quoted 4 lines

I'm not clear on why we need to update pmuserenr_el0 here anyways. To get here userspace has to mmap the event and then unmmap it. If we did nothing, then counter accesses would not fault until the next context switch.

Quoted 3 lines

If you all have any ideas, I'm all ears. I'm not a scheduler nor perf hacker. ;)

Mark, Will, any thoughts on this?

tools/lib/perf/evsel.c +10 -0

+void *perf_evsel__mmap(struct perf_evsel *evsel, int pages)
+{
+	int ret;
+	struct perf_mmap *map;
+	struct perf_mmap_param mp = {
+		.prot = PROT_READ | PROT_WRITE,
+	};
+
+	if (FD(evsel, 0, 0) < 0)
+		return NULL;

On Thu, Oct 01, 2020 at 09:01:11AM -0500, Rob Herring wrote:

SNIP

Quoted 11 lines

tools/lib/perf/evsel.c +10 -0

+void *perf_evsel__mmap(struct perf_evsel *evsel, int pages)
+{
+     int ret;
+     struct perf_mmap *map;
+     struct perf_mmap_param mp = {
+             .prot = PROT_READ | PROT_WRITE,
+     };
+
+     if (FD(evsel, 0, 0) < 0)
+             return NULL;

tools/lib/perf/evsel.c void perf_evsel__close_cpu(struct perf_evsel *evsel, int cpu) · line 180

Quoted 10 lines

tools/lib/perf/evsel.c +10 -0

+
+     mp.mask = (pages * page_size) - 1;
+
+     map = zalloc(sizeof(*map));
+     if (!map)
+             return NULL;
+
+     perf_mmap__init(map, NULL, false, NULL);
+
+     ret = perf_mmap__mmap(map, &mp, FD(evsel, 0, 0), 0);

hum, so you map event for FD(0,0) but later in perf_evsel__read you allow to read any cpu/thread combination ending up reading data from FD(0,0) map:

        int perf_evsel__read(struct perf_evsel *evsel, int cpu, int thread,
                             struct perf_counts_values *count)
        {
                size_t size = perf_evsel__read_size(evsel);

                memset(count, 0, sizeof(*count));

                if (FD(evsel, cpu, thread) < 0)
                        return -EINVAL;

                if (evsel->mmap && !perf_mmap__read_self(evsel->mmap, count))
                        return 0;

I think we should either check cpu == 0, thread == 0, or make it general and store perf_evsel::mmap in xyarray as we do for fds

Quoted 45 lines · Jiri Olsa

On Thu, Oct 01, 2020 at 09:01:11AM -0500, Rob Herring wrote:

SNIP

Quoted 11 lines

tools/lib/perf/evsel.c +10 -0

+void *perf_evsel__mmap(struct perf_evsel *evsel, int pages)
+{
+     int ret;
+     struct perf_mmap *map;
+     struct perf_mmap_param mp = {
+             .prot = PROT_READ | PROT_WRITE,
+     };
+
+     if (FD(evsel, 0, 0) < 0)
+             return NULL;

tools/lib/perf/evsel.c void perf_evsel__close_cpu(struct perf_evsel *evsel, int cpu) · line 180

Quoted 10 lines

tools/lib/perf/evsel.c +10 -0

+
+     mp.mask = (pages * page_size) - 1;
+
+     map = zalloc(sizeof(*map));
+     if (!map)
+             return NULL;
+
+     perf_mmap__init(map, NULL, false, NULL);
+
+     ret = perf_mmap__mmap(map, &mp, FD(evsel, 0, 0), 0);

hum, so you map event for FD(0,0) but later in perf_evsel__read you allow to read any cpu/thread combination ending up reading data from FD(0,0) map:

        int perf_evsel__read(struct perf_evsel *evsel, int cpu, int thread,
                             struct perf_counts_values *count)
        {
                size_t size = perf_evsel__read_size(evsel);

                memset(count, 0, sizeof(*count));

                if (FD(evsel, cpu, thread) < 0)
                        return -EINVAL;

                if (evsel->mmap && !perf_mmap__read_self(evsel->mmap, count))
                        return 0;

Quoted 2 lines

I think we should either check cpu == 0, thread == 0, or make it general and store perf_evsel::mmap in xyarray as we do for fds

The mmapped read will actually fail and then we fallback here. My main concern though is adding more overhead on a feature that's meant to be low overhead (granted, it's not much). Maybe we could add checks on the mmap that we've opened the event with pid == 0 and cpu == -1 (so only 1 FD)?

Quoted 46 lines · Rob Herring

Quoted 45 lines · Jiri Olsa

On Thu, Oct 01, 2020 at 09:01:11AM -0500, Rob Herring wrote:

SNIP

Quoted 11 lines

tools/lib/perf/evsel.c +10 -0

+void *perf_evsel__mmap(struct perf_evsel *evsel, int pages)
+{
+     int ret;
+     struct perf_mmap *map;
+     struct perf_mmap_param mp = {
+             .prot = PROT_READ | PROT_WRITE,
+     };
+
+     if (FD(evsel, 0, 0) < 0)
+             return NULL;

tools/lib/perf/evsel.c void perf_evsel__close_cpu(struct perf_evsel *evsel, int cpu) · line 180

Quoted 10 lines

tools/lib/perf/evsel.c +10 -0

+
+     mp.mask = (pages * page_size) - 1;
+
+     map = zalloc(sizeof(*map));
+     if (!map)
+             return NULL;
+
+     perf_mmap__init(map, NULL, false, NULL);
+
+     ret = perf_mmap__mmap(map, &mp, FD(evsel, 0, 0), 0);

hum, so you map event for FD(0,0) but later in perf_evsel__read you allow to read any cpu/thread combination ending up reading data from FD(0,0) map:

        int perf_evsel__read(struct perf_evsel *evsel, int cpu, int thread,
                             struct perf_counts_values *count)
        {
                size_t size = perf_evsel__read_size(evsel);

                memset(count, 0, sizeof(*count));

                if (FD(evsel, cpu, thread) < 0)
                        return -EINVAL;

                if (evsel->mmap && !perf_mmap__read_self(evsel->mmap, count))
                        return 0;

Quoted 8 lines

Quoted 2 lines

I think we should either check cpu == 0, thread == 0, or make it general and store perf_evsel::mmap in xyarray as we do for fds

The mmapped read will actually fail and then we fallback here. My main concern though is adding more overhead on a feature that's meant to be low overhead (granted, it's not much). Maybe we could add checks on the mmap that we've opened the event with pid == 0 and cpu == -1 (so only 1 FD)?

but then you limit this just for single fd.. having mmap as xyarray would not be that bad and perf_evsel__mmap will call perf_mmap__mmap for each defined cpu/thread .. so it depends on user how fast this will be - how many maps needs to be created/mmaped

Quoted 57 lines · Jiri Olsa

Quoted 54 lines · Rob Herring

Quoted 45 lines · Jiri Olsa

On Thu, Oct 01, 2020 at 09:01:11AM -0500, Rob Herring wrote:

SNIP

Quoted 11 lines

tools/lib/perf/evsel.c +10 -0

+void *perf_evsel__mmap(struct perf_evsel *evsel, int pages)
+{
+     int ret;
+     struct perf_mmap *map;
+     struct perf_mmap_param mp = {
+             .prot = PROT_READ | PROT_WRITE,
+     };
+
+     if (FD(evsel, 0, 0) < 0)
+             return NULL;

tools/lib/perf/evsel.c void perf_evsel__close_cpu(struct perf_evsel *evsel, int cpu) · line 180

Quoted 10 lines

tools/lib/perf/evsel.c +10 -0

+
+     mp.mask = (pages * page_size) - 1;
+
+     map = zalloc(sizeof(*map));
+     if (!map)
+             return NULL;
+
+     perf_mmap__init(map, NULL, false, NULL);
+
+     ret = perf_mmap__mmap(map, &mp, FD(evsel, 0, 0), 0);

hum, so you map event for FD(0,0) but later in perf_evsel__read you allow to read any cpu/thread combination ending up reading data from FD(0,0) map:

        int perf_evsel__read(struct perf_evsel *evsel, int cpu, int thread,
                             struct perf_counts_values *count)
        {
                size_t size = perf_evsel__read_size(evsel);

                memset(count, 0, sizeof(*count));

                if (FD(evsel, cpu, thread) < 0)
                        return -EINVAL;

                if (evsel->mmap && !perf_mmap__read_self(evsel->mmap, count))
                        return 0;

Quoted 2 lines

I think we should either check cpu == 0, thread == 0, or make it general and store perf_evsel::mmap in xyarray as we do for fds

The mmapped read will actually fail and then we fallback here. My main concern though is adding more overhead on a feature that's meant to be low overhead (granted, it's not much). Maybe we could add checks on the mmap that we've opened the event with pid == 0 and cpu == -1 (so only 1 FD)?

Quoted 4 lines

but then you limit this just for single fd.. having mmap as xyarray would not be that bad and perf_evsel__mmap will call perf_mmap__mmap for each defined cpu/thread .. so it depends on user how fast this will be - how many maps needs to be created/mmaped

Given userspace access fails for anything other than the calling thread and all cpus, how would more than 1 mmap be useful here?

If we did want multiple mmaps, wouldn't we just use the evlist API in that case? It already does all that.

Quoted 63 lines · Rob Herring

Quoted 61 lines · Jiri Olsa

Quoted 46 lines · Rob Herring

Quoted 45 lines · Jiri Olsa

On Thu, Oct 01, 2020 at 09:01:11AM -0500, Rob Herring wrote:

SNIP

Quoted 11 lines

(quoted diff) +10 -0

+void *perf_evsel__mmap(struct perf_evsel *evsel, int pages)
+{
+     int ret;
+     struct perf_mmap *map;
+     struct perf_mmap_param mp = {
+             .prot = PROT_READ | PROT_WRITE,
+     };
+
+     if (FD(evsel, 0, 0) < 0)
+             return NULL;

Review

Quoted 10 lines

(quoted diff) +10 -0

+
+     mp.mask = (pages * page_size) - 1;
+
+     map = zalloc(sizeof(*map));
+     if (!map)
+             return NULL;
+
+     perf_mmap__init(map, NULL, false, NULL);
+
+     ret = perf_mmap__mmap(map, &mp, FD(evsel, 0, 0), 0);

hum, so you map event for FD(0,0) but later in perf_evsel__read you allow to read any cpu/thread combination ending up reading data from FD(0,0) map:

        int perf_evsel__read(struct perf_evsel *evsel, int cpu, int thread,
                             struct perf_counts_values *count)
        {
                size_t size = perf_evsel__read_size(evsel);

                memset(count, 0, sizeof(*count));

                if (FD(evsel, cpu, thread) < 0)
                        return -EINVAL;

                if (evsel->mmap && !perf_mmap__read_self(evsel->mmap, count))
                        return 0;

Quoted 8 lines

Quoted 2 lines

I think we should either check cpu == 0, thread == 0, or make it general and store perf_evsel::mmap in xyarray as we do for fds

The mmapped read will actually fail and then we fallback here. My main concern though is adding more overhead on a feature that's meant to be low overhead (granted, it's not much). Maybe we could add checks on the mmap that we've opened the event with pid == 0 and cpu == -1 (so only 1 FD)?

but then you limit this just for single fd.. having mmap as xyarray would not be that bad and perf_evsel__mmap will call perf_mmap__mmap for each defined cpu/thread .. so it depends on user how fast this will be - how many maps needs to be created/mmaped

Quoted 2 lines

Given userspace access fails for anything other than the calling thread and all cpus, how would more than 1 mmap be useful here?

I'm not sure what you mean by fail in here.. you need mmap for each event fd you want to read from

in the example below we read stats from all cpus via perf_evsel__read, if we insert this call after perf_evsel__open:

  perf_evsel__mmap(cpus, NULL);

that maps page for each event, then perf_evsel__read could go through the fast code, no?

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

Quoted 6 lines

The mmapped read will actually fail and then we fallback here. My main concern though is adding more overhead on a feature that's meant to be low overhead (granted, it's not much). Maybe we could add checks on the mmap that we've opened the event with pid == 0 and cpu == -1 (so only 1 FD)?

but then you limit this just for single fd.. having mmap as xyarray would not be that bad and perf_evsel__mmap will call perf_mmap__mmap for each defined cpu/thread .. so it depends on user how fast this will be - how many maps needs to be created/mmaped

Quoted 5 lines

Quoted 2 lines

Given userspace access fails for anything other than the calling thread and all cpus, how would more than 1 mmap be useful here?

I'm not sure what you mean by fail in here.. you need mmap for each event fd you want to read from

Yes, but that's one mmap per event (evsel) which is different than per cpu/thread.

Quoted 8 lines

in the example below we read stats from all cpus via perf_evsel__read, if we insert this call after perf_evsel__open:

  perf_evsel__mmap(cpus, NULL);

that maps page for each event, then perf_evsel__read could go through the fast code, no?

No, because we're not self-monitoring (pid == 0 and cpu == -1). With the following change:

tools/lib/perf/tests/test-evsel.c +1 -0

index eeca8203d73d..1fca9c121f7c 100644
@@ -17,6 +17,7 @@ static int test_stat_cpu(void)
 {
        struct perf_cpu_map *cpus;
        struct perf_evsel *evsel;
+       struct perf_event_mmap_page *pc;
        struct perf_event_attr attr = {
                .type   = PERF_TYPE_SOFTWARE,
                .config = PERF_COUNT_SW_CPU_CLOCK,

@@ -32,6 +33,15 @@ static int test_stat_cpu(void)
        err = perf_evsel__open(evsel, cpus, NULL);

tools/lib/perf/tests/test-evsel.c +1 -0

+
        perf_cpu_map__for_each_cpu(cpu, tmp, cpus) {
                struct perf_counts_values counts = { .val = 0 };
 

I get:

- running test-evsel.c...FAILED test-evsel.c:40 userspace counter access not supported

If I set it to pid==0, userspace counter access is also disabled.

Maybe there is some use for mmap beyond fast path read for self-monitoring or what evlist mmap does, but I don't know what that would be.

Note that we could get rid of the mmap API and just do the mmap behind the scenes whenever we get the magic setup that works. The main downside with that is you can't check if the fast path is enabled or not (though we could have a perf_evsel__is_fast_read(evsel, cpu, thread) instead).

Rob

On Tue, Oct 20, 2020 at 12:11:47PM -0500, Rob Herring wrote:

SNIP

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

Quoted 6 lines

The mmapped read will actually fail and then we fallback here. My main concern though is adding more overhead on a feature that's meant to be low overhead (granted, it's not much). Maybe we could add checks on the mmap that we've opened the event with pid == 0 and cpu == -1 (so only 1 FD)?

but then you limit this just for single fd.. having mmap as xyarray would not be that bad and perf_evsel__mmap will call perf_mmap__mmap for each defined cpu/thread .. so it depends on user how fast this will be - how many maps needs to be created/mmaped

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Given userspace access fails for anything other than the calling thread and all cpus, how would more than 1 mmap be useful here?

I'm not sure what you mean by fail in here.. you need mmap for each event fd you want to read from

Yes, but that's one mmap per event (evsel) which is different than per cpu/thread.

right, and you need mmap per fd IIUC

Quoted 29 lines

Quoted 8 lines

in the example below we read stats from all cpus via perf_evsel__read, if we insert this call after perf_evsel__open:

  perf_evsel__mmap(cpus, NULL);

that maps page for each event, then perf_evsel__read could go through the fast code, no?

No, because we're not self-monitoring (pid == 0 and cpu == -1). With the following change:

tools/lib/perf/tests/test-evsel.c +1 -0

index eeca8203d73d..1fca9c121f7c 100644
@@ -17,6 +17,7 @@ static int test_stat_cpu(void)
 {
        struct perf_cpu_map *cpus;
        struct perf_evsel *evsel;
+       struct perf_event_mmap_page *pc;
        struct perf_event_attr attr = {
                .type   = PERF_TYPE_SOFTWARE,
                .config = PERF_COUNT_SW_CPU_CLOCK,

@@ -32,6 +33,15 @@ static int test_stat_cpu(void)
        err = perf_evsel__open(evsel, cpus, NULL);
        __T("failed to open evsel", err == 0);
 

tools/lib/perf/tests/test-evsel.c static int test_stat_cpu(void) · line 43

Quoted 8 lines

tools/lib/perf/tests/test-evsel.c +8 -0

+       pc = perf_evsel__mmap(evsel, 0);
+       __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

I'll need to check, I'm surprised this would depend on the way you open the event

Quoted 24 lines

Quoted 24 lines

Quoted 3 lines

that maps page for each event, then perf_evsel__read could go through the fast code, no?

No, because we're not self-monitoring (pid == 0 and cpu == -1). With the following change:

tools/lib/perf/tests/test-evsel.c +1 -0

index eeca8203d73d..1fca9c121f7c 100644
@@ -17,6 +17,7 @@ static int test_stat_cpu(void)
 {
        struct perf_cpu_map *cpus;
        struct perf_evsel *evsel;
+       struct perf_event_mmap_page *pc;
        struct perf_event_attr attr = {
                .type   = PERF_TYPE_SOFTWARE,
                .config = PERF_COUNT_SW_CPU_CLOCK,

@@ -32,6 +33,15 @@ static int test_stat_cpu(void)
        err = perf_evsel__open(evsel, cpus, NULL);
        __T("failed to open evsel", err == 0);
 

Quoted 11 lines

tools/lib/perf/tests/test-evsel.c static int test_stat_cpu(void) · line 43

Quoted 8 lines

tools/lib/perf/tests/test-evsel.c +8 -0

+       pc = perf_evsel__mmap(evsel, 0);
+       __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

I'll need to check, I'm surprised this would depend on the way you open the event

Any more thoughts on this?

On Thu, Nov 05, 2020 at 10:19:24AM -0600, Rob Herring wrote:

SNIP

Quoted 36 lines

Quoted 35 lines

Quoted 22 lines

Quoted 3 lines

that maps page for each event, then perf_evsel__read could go through the fast code, no?

No, because we're not self-monitoring (pid == 0 and cpu == -1). With the following change:

tools/lib/perf/tests/test-evsel.c +1 -0

index eeca8203d73d..1fca9c121f7c 100644
@@ -17,6 +17,7 @@ static int test_stat_cpu(void)
 {
        struct perf_cpu_map *cpus;
        struct perf_evsel *evsel;
+       struct perf_event_mmap_page *pc;
        struct perf_event_attr attr = {
                .type   = PERF_TYPE_SOFTWARE,
                .config = PERF_COUNT_SW_CPU_CLOCK,

@@ -32,6 +33,15 @@ static int test_stat_cpu(void)
        err = perf_evsel__open(evsel, cpus, NULL);

tools/lib/perf/tests/test-evsel.c static int test_stat_cpu(void) · line 43

Quoted 10 lines

tools/lib/perf/tests/test-evsel.c +8 -0

        __T("failed to open evsel", err == 0);
 
+       pc = perf_evsel__mmap(evsel, 0);
+       __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

I'll need to check, I'm surprised this would depend on the way you open the event

Quoted 1 line

Any more thoughts on this?

sry I got stuck with other stuff.. I tried your change and pc->cap_user_rdpmc is 0 because the test creates software event, which does not support that

when I change that to:

.type = PERF_TYPE_HARDWARE,
.config = PERF_COUNT_HW_CPU_CYCLES,

I don't see any of those warning you added

Quoted 41 lines · Jiri Olsa

On Thu, Nov 05, 2020 at 10:19:24AM -0600, Rob Herring wrote:

SNIP

Quoted 36 lines

Quoted 35 lines

Quoted 22 lines

Quoted 3 lines

that maps page for each event, then perf_evsel__read could go through the fast code, no?

No, because we're not self-monitoring (pid == 0 and cpu == -1). With the following change:

tools/lib/perf/tests/test-evsel.c +1 -0

index eeca8203d73d..1fca9c121f7c 100644
@@ -17,6 +17,7 @@ static int test_stat_cpu(void)
 {
        struct perf_cpu_map *cpus;
        struct perf_evsel *evsel;
+       struct perf_event_mmap_page *pc;
        struct perf_event_attr attr = {
                .type   = PERF_TYPE_SOFTWARE,
                .config = PERF_COUNT_SW_CPU_CLOCK,

@@ -32,6 +33,15 @@ static int test_stat_cpu(void)
        err = perf_evsel__open(evsel, cpus, NULL);

tools/lib/perf/tests/test-evsel.c static int test_stat_cpu(void) · line 43

Quoted 10 lines

tools/lib/perf/tests/test-evsel.c +8 -0

        __T("failed to open evsel", err == 0);
 
+       pc = perf_evsel__mmap(evsel, 0);
+       __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

I'll need to check, I'm surprised this would depend on the way you open the event

Quoted 5 lines

Quoted 1 line

Any more thoughts on this?

sry I got stuck with other stuff.. I tried your change and pc->cap_user_rdpmc is 0 because the test creates software event, which does not support that

Sigh, yes, of course.

Quoted 6 lines

when I change that to:

.type = PERF_TYPE_HARDWARE,
.config = PERF_COUNT_HW_CPU_CYCLES,

I don't see any of those warning you added

So I've now implemented the per fd mmap. It seems to run and get some data, but for the above case the counts don't look right.

cpu0: count = 0x10883, ena = 0xbf42, run = 0xbf42
cpu1: count = 0x1bc65, ena = 0xa278, run = 0xa278
cpu2: count = 0x1fab2, ena = 0x91ea, run = 0x91ea
cpu3: count = 0x23d61, ena = 0x81ac, run = 0x81ac
cpu4: count = 0x2936a, ena = 0x7149, run = 0x7149
cpu5: count = 0x2cd4e, ena = 0x634f, run = 0x634f
cpu6: count = 0x3139f, ena = 0x53e7, run = 0x53e7
cpu7: count = 0x35350, ena = 0x4690, run = 0x4690

For comparison, this is what I get using the slow path read():
cpu0: count = 0x1c40, ena = 0x188b5, run = 0x188b5
cpu1: count = 0x18e0, ena = 0x1b8f4, run = 0x1b8f4
cpu2: count = 0x745e, ena = 0x1ab9e, run = 0x1ab9e
cpu3: count = 0x2416, ena = 0x1a280, run = 0x1a280
cpu4: count = 0x19c7, ena = 0x19b00, run = 0x19b00
cpu5: count = 0x1737, ena = 0x19262, run = 0x19262
cpu6: count = 0x11d0e, ena = 0x18944, run = 0x18944
cpu7: count = 0x20dbe, ena = 0x181f4, run = 0x181f4

The difference is we get a sequentially increasing count rather than 1 random CPU (the one running the test) with a much higher count. That seems to me we're just reading the count for the calling process, not each CPU.

For this to work correctly, cap_user_rdpmc would have to be set only for the CPU's mmap that matches the calling process's CPU. I'm not sure whether that can be done. Even if it can, is it really worth doing so? You're accelerating reading an event on 1 out of N CPUs. And what do we do on every kernel up til now this won't work on? Another cap bit?

Rob

P.S. I did find one bug with all this. The shifts by pmc_width in the read seq need to be a signed count. This test happens to have raw counter values starting at 2^47.

Quoted 69 lines · Rob Herring

Quoted 41 lines · Jiri Olsa

On Thu, Nov 05, 2020 at 10:19:24AM -0600, Rob Herring wrote:

SNIP

Quoted 36 lines

Quoted 35 lines

Quoted 22 lines

Quoted 3 lines

that maps page for each event, then perf_evsel__read could go through the fast code, no?

No, because we're not self-monitoring (pid == 0 and cpu == -1). With the following change:

tools/lib/perf/tests/test-evsel.c +1 -0

index eeca8203d73d..1fca9c121f7c 100644
@@ -17,6 +17,7 @@ static int test_stat_cpu(void)
 {
        struct perf_cpu_map *cpus;
        struct perf_evsel *evsel;
+       struct perf_event_mmap_page *pc;
        struct perf_event_attr attr = {
                .type   = PERF_TYPE_SOFTWARE,
                .config = PERF_COUNT_SW_CPU_CLOCK,

@@ -32,6 +33,15 @@ static int test_stat_cpu(void)
        err = perf_evsel__open(evsel, cpus, NULL);

tools/lib/perf/tests/test-evsel.c static int test_stat_cpu(void) · line 43

Quoted 10 lines

tools/lib/perf/tests/test-evsel.c +8 -0

        __T("failed to open evsel", err == 0);
 
+       pc = perf_evsel__mmap(evsel, 0);
+       __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

I'll need to check, I'm surprised this would depend on the way you open the event

Quoted 5 lines

Quoted 1 line

Any more thoughts on this?

sry I got stuck with other stuff.. I tried your change and pc->cap_user_rdpmc is 0 because the test creates software event, which does not support that

Sigh, yes, of course.

Quoted 6 lines

when I change that to:

.type = PERF_TYPE_HARDWARE,
.config = PERF_COUNT_HW_CPU_CYCLES,

I don't see any of those warning you added

So I've now implemented the per fd mmap. It seems to run and get some data, but for the above case the counts don't look right.

cpu0: count = 0x10883, ena = 0xbf42, run = 0xbf42
cpu1: count = 0x1bc65, ena = 0xa278, run = 0xa278
cpu2: count = 0x1fab2, ena = 0x91ea, run = 0x91ea
cpu3: count = 0x23d61, ena = 0x81ac, run = 0x81ac
cpu4: count = 0x2936a, ena = 0x7149, run = 0x7149
cpu5: count = 0x2cd4e, ena = 0x634f, run = 0x634f
cpu6: count = 0x3139f, ena = 0x53e7, run = 0x53e7
cpu7: count = 0x35350, ena = 0x4690, run = 0x4690

Quoted 9 lines

For comparison, this is what I get using the slow path read():
cpu0: count = 0x1c40, ena = 0x188b5, run = 0x188b5
cpu1: count = 0x18e0, ena = 0x1b8f4, run = 0x1b8f4
cpu2: count = 0x745e, ena = 0x1ab9e, run = 0x1ab9e
cpu3: count = 0x2416, ena = 0x1a280, run = 0x1a280
cpu4: count = 0x19c7, ena = 0x19b00, run = 0x19b00
cpu5: count = 0x1737, ena = 0x19262, run = 0x19262
cpu6: count = 0x11d0e, ena = 0x18944, run = 0x18944
cpu7: count = 0x20dbe, ena = 0x181f4, run = 0x181f4

hum, could you please send/push changes with that test? I can try it and check