Add cap_user_time aarch64

PATCH New Michael O'Farrell 3 participants 4 messages Jul 13, 2018 – Jul 24, 2018

Subsystems (3)

perf events arm64 Arm PMU profiling and debugging

lore

Share & tools

lore

Michael O'Farrell

author

2018-07-13 19:24 UTC #0 lore raw

PATCH 1/1 Add cap_user_time aarch64

It is useful to get the running time of a thread. Doing so in an efficient manner can be important for performance of user applications. Avoiding system calls in clock_gettime when handling CLOCK_THREAD_CPUTIME_ID is important. Other clocks are handled in the VDSO, but CLOCK_THREAD_CPUTIME_ID falls back on the system call.

CLOCK_THREAD_CPUTIME_ID is not handled in the VDSO since it would have costs associated with maintaining updated user space accessible time offsets. These offsets have to be updated everytime the a thread is scheduled/descheduled. However, for programs regularly checking the running time of a thread, this is a performance improvement.

This patch takes a middle ground, and adds support for cap_user_time an optional feature of the perf_event API. This way costs are only incurred when the perf_event api is enabled. This is done the same way as it is in x86. This patch also enables the cap_user_time facility of perf_event since it comes free with the stable clock.

Ultimately this allows calculating the thread running time in userspace on aarch64 as follows (adapted from perf_event_open manpage):

u32 seq, time_mult, time_shift;
u64 running, count, time_offset, quot, rem, delta;
struct perf_event_mmap_page *pc;
pc = buf; // buf is the perf event mmaped page as documented in the API.

if (pc->cap_usr_time) {
    do {
        seq = pc->lock;
        barrier();
        running = pc->time_running;

        count = readCNTVCT_EL0();  // Read ARM hardware clock.
        time_offset = pc->time_offset;
        time_mult   = pc->time_mult;
        time_shift  = pc->time_shift;

        barrier();
    } while (pc->lock != seq);

    quot = (count >> time_shift);
    rem = count & (((u64)1 << time_shift) - 1);
    delta = time_offset + quot * time_mult +
            ((rem * time_mult) >> time_shift);

    running += delta;
    // running now has the current nanosecond level thread time.
}

Summary of changes in the patch:

For aarch64 systems with a stable scheduler clock, make arch_perf_update_userpage update the timing information stored in the perf_event page. Requiring the following calculations:
- Calculate the appropriate time_mult, and time_shift factors to convert ticks to nano seconds for the current clock frequency.
- Adjust the mult and shift factors to avoid shift factors of 32 bits. (possibly unnecessary)
- The time_offset userspace should apply when doing calculations:
negative the current sched time (now), because time_running and time_enabled fields of the perf_event page have just been updated.
- The time_zero field is zero since we have are using the stable sched_clock, and it started at zero.
Toggle bits to appropriate values:
- Enable cap_user_time
- Enable cap_user_time_zero. Since the sched clock is stable, and it started at zero this is able to be true

Signed-off-by Michael O'Farrell <[email protected]>

---
 arch/arm64/kernel/perf_event.c | 39 ++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

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

index 33147aacdafd..c63f31caf7ac 100644
@@ -25,6 +25,7 @@
 #include <asm/virt.h>
 
 #include <linux/acpi.h>
+#include <linux/clocksource.h>
 #include <linux/of.h>
 #include <linux/perf/arm_pmu.h>
 #include <linux/platform_device.h>

@@ -1127,3 +1128,41 @@ static int __init armv8_pmu_driver_init(void)
 		return arm_pmu_acpi_probe(armv8_pmuv3_init);
 }
 device_initcall(armv8_pmu_driver_init)
+
+#ifndef CONFIG_HAVE_UNSTABLE_SCHED_CLOCK
+void arch_perf_update_userpage(struct perf_event *event,
+			       struct perf_event_mmap_page *userpg, u64 now)
+{
+	u32 freq;
+	u32 shift;
+
+	/*
+	 * Internal timekeeping for enabled/running/stopped times
+	 * is always computed with the sched_clock.
+	 */
+	freq = arch_timer_get_rate();
+	userpg->cap_user_time = 1;
+
+	clocks_calc_mult_shift(&userpg->time_mult, &shift, freq,
+			NSEC_PER_SEC, 0);
+	/*
+	 * time_shift is not expected to be greater than 31 due to
+	 * the original published conversion algorithm shifting a
+	 * 32-bit value (now specifies a 64-bit value) - refer
+	 * perf_event_mmap_page documentation in perf_event.h.
+	 */
+	if (shift == 32) {
+		shift = 31;
+		userpg->time_mult >>= 1;
+	}
+	userpg->time_shift = (u16)shift;
+	userpg->time_offset = -now;
+
+	/*
+	 * We are always using the sched_clock as the base, so
+	 * cap_user_time_zero makes sense.
+	 */
+	userpg->cap_user_time_zero = 1;
+	userpg->time_zero = 0;
+}
+#endif /* !CONFIG_HAVE_UNSTABLE_SCHED_CLOCK */

Signature · 1 line

2.17.1

Will Deacon

2018-07-24 15:14 UTC #1 lore raw

PATCH 1/1 Add cap_user_time aarch64

Hi Michael,

[adding Peter]

Thanks for the patch. I think the general idea is sound, but I have a few questions about the implementation.

Quoted 26 lines · Michael O'Farrell

It is useful to get the running time of a thread. Doing so in an efficient manner can be important for performance of user applications. Avoiding system calls in clock_gettime when handling CLOCK_THREAD_CPUTIME_ID is important. Other clocks are handled in the VDSO, but CLOCK_THREAD_CPUTIME_ID falls back on the system call.
CLOCK_THREAD_CPUTIME_ID is not handled in the VDSO since it would have costs associated with maintaining updated user space accessible time offsets. These offsets have to be updated everytime the a thread is scheduled/descheduled. However, for programs regularly checking the running time of a thread, this is a performance improvement.
This patch takes a middle ground, and adds support for cap_user_time an optional feature of the perf_event API. This way costs are only incurred when the perf_event api is enabled. This is done the same way as it is in x86. This patch also enables the cap_user_time facility of perf_event since it comes free with the stable clock.
Ultimately this allows calculating the thread running time in userspace on aarch64 as follows (adapted from perf_event_open manpage):
u32 seq, time_mult, time_shift;
u64 running, count, time_offset, quot, rem, delta;
struct perf_event_mmap_page *pc;
pc = buf; // buf is the perf event mmaped page as documented in the API.

Quoted 13 lines

if (pc->cap_usr_time) {
    do {
        seq = pc->lock;
        barrier();
        running = pc->time_running;

        count = readCNTVCT_EL0();  // Read ARM hardware clock.
        time_offset = pc->time_offset;
        time_mult   = pc->time_mult;
        time_shift  = pc->time_shift;

        barrier();
    } while (pc->lock != seq);

Using only compiler barriers here raises my eyebrows a little, but as long as this is all in the context of the reader thread, then it should be ok. I think it means that the comment in events/core.c is bogus:

	/*
	 * Disable preemption so as to not let the corresponding user-space
	 * spin too long if we get preempted.
	 */
	preempt_disable();
	++userpg->lock;
	barrier();

because user-space won't get stuck spinning unless its running concurrently, at which point it's hosed anyway. Hmmm.

Quoted 13 lines

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

index 33147aacdafd..c63f31caf7ac 100644
@@ -25,6 +25,7 @@
 #include <asm/virt.h>
 
 #include <linux/acpi.h>
+#include <linux/clocksource.h>
 #include <linux/of.h>
 #include <linux/perf/arm_pmu.h>
 #include <linux/platform_device.h>

arch/arm64/kernel/perf_event.c static int __init armv8_pmu_driver_init(void) · line 1132

Quoted 5 lines

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

@@ -1127,3 +1128,41 @@ static int __init armv8_pmu_driver_init(void)
 		return arm_pmu_acpi_probe(armv8_pmuv3_init);
 }
 device_initcall(armv8_pmu_driver_init)
+
+#ifndef CONFIG_HAVE_UNSTABLE_SCHED_CLOCK

We never select CONFIG_HAVE_UNSTABLE_SCHED_CLOCK, so you can remove the guards.

Quoted 15 lines

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

+void arch_perf_update_userpage(struct perf_event *event,
+			       struct perf_event_mmap_page *userpg, u64 now)
+{
+	u32 freq;
+	u32 shift;
+
+	/*
+	 * Internal timekeeping for enabled/running/stopped times
+	 * is always computed with the sched_clock.
+	 */
+	freq = arch_timer_get_rate();
+	userpg->cap_user_time = 1;
+
+	clocks_calc_mult_shift(&userpg->time_mult, &shift, freq,
+			NSEC_PER_SEC, 0);

arch/arm64/kernel/perf_event.c static int __init armv8_pmu_driver_init(void) · line 1157

Quoted 10 lines

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

+	/*
+	 * time_shift is not expected to be greater than 31 due to
+	 * the original published conversion algorithm shifting a
+	 * 32-bit value (now specifies a 64-bit value) - refer
+	 * perf_event_mmap_page documentation in perf_event.h.
+	 */
+	if (shift == 32) {
+		shift = 31;
+		userpg->time_mult >>= 1;
+	}

Can you explain this a bit more, please? Given that we've never enabled this on arm64, why do we have a problem with a legacy algorithm?

arch/arm64/kernel/perf_event.c static int __init armv8_pmu_driver_init(void) · line 1164

Quoted 7 lines

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

+	userpg->time_shift = (u16)shift;
+	userpg->time_offset = -now;
+
+	/*
+	 * We are always using the sched_clock as the base, so
+	 * cap_user_time_zero makes sense.
+	 */

How are you enforcing that we're always using sched_clock?

arch/arm64/kernel/perf_event.c static int __init armv8_pmu_driver_init(void) · line 1166

Quoted 2 lines

arch/arm64/kernel/perf_event.c +2 -0
+	userpg->cap_user_time_zero = 1;
+	userpg->time_zero = 0;

I'm not sure we can rely on the architected timer being zero initialised -- our booting documentation just require it to be consistent between CPUs.

Will

Peter Zijlstra

maintainer

2018-07-24 15:45 UTC #2 lore raw

PATCH 1/1 Add cap_user_time aarch64

Quoted 12 lines · Will Deacon

Using only compiler barriers here raises my eyebrows a little, but as long as this is all in the context of the reader thread, then it should be ok. I think it means that the comment in events/core.c is bogus:
	/*
	 * Disable preemption so as to not let the corresponding user-space
	 * spin too long if we get preempted.
	 */
	preempt_disable();
	++userpg->lock;
	barrier();

Quoted 2 lines

because user-space won't get stuck spinning unless its running concurrently, at which point it's hosed anyway. Hmmm.

Right, we should fix that. The only reason to keep that preempt_disable() there is so that the various timestamps retain temporal locality.

Quoted 20 lines

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

+void arch_perf_update_userpage(struct perf_event *event,
+			       struct perf_event_mmap_page *userpg, u64 now)
+{
+	u32 freq;
+	u32 shift;
+
+	/*
+	 * Internal timekeeping for enabled/running/stopped times
+	 * is always computed with the sched_clock.
+	 */
+	freq = arch_timer_get_rate();
+	userpg->cap_user_time = 1;
+
+	clocks_calc_mult_shift(&userpg->time_mult, &shift, freq,
+			NSEC_PER_SEC, 0);
+	/*
+	 * time_shift is not expected to be greater than 31 due to
+	 * the original published conversion algorithm shifting a
+	 * 32-bit value (now specifies a 64-bit value) - refer
+	 * perf_event_mmap_page documentation in perf_event.h.

Quoted 8 lines

arch/arm64/kernel/perf_event.c static int __init armv8_pmu_driver_init(void) · line 1157
Quoted 5 lines
arch/arm64/kernel/perf_event.c +5 -0
+	 */
+	if (shift == 32) {
+		shift = 31;
+		userpg->time_mult >>= 1;
+	}
Can you explain this a bit more, please? Given that we've never enabled this on arm64, why do we have a problem with a legacy algorithm?

The worry is, I suppose, that someone still uses the old algorithm and then ports it to arm64 without checking.

Quoted 10 lines

arch/arm64/kernel/perf_event.c static int __init armv8_pmu_driver_init(void) · line 1164
Quoted 7 lines
arch/arm64/kernel/perf_event.c +7 -0
+	userpg->time_shift = (u16)shift;
+	userpg->time_offset = -now;
+
+	/*
+	 * We are always using the sched_clock as the base, so
+	 * cap_user_time_zero makes sense.
+	 */
How are you enforcing that we're always using sched_clock?

Quoted 5 lines

arch/arm64/kernel/perf_event.c static int __init armv8_pmu_driver_init(void) · line 1166
Quoted 2 lines
arch/arm64/kernel/perf_event.c +2 -0
+	userpg->cap_user_time_zero = 1;
+	userpg->time_zero = 0;
I'm not sure we can rely on the architected timer being zero initialised -- our booting documentation just require it to be consistent between CPUs.

On x86 the above is:

	/*
	 * cap_user_time_zero doesn't make sense when we're using a different
	 * time base for the records.
	 */
	if (!event->attr.use_clockid) {
		userpg->cap_user_time_zero = 1;
		userpg->time_zero = offset;
	}

Michael O'Farrell

author

2018-07-24 22:41 UTC #3 lore raw

PATCH 1/1 Add cap_user_time aarch64

Thanks Will and Peter.

So:
- Remove the ifndef for CONFIG_HAVE_UNSTABLE_SCHED_CLOCK
- Update the comment in events/core.c to read "Disable preemption to guarantee consistent time stamps are stored to the user page."

Quoted 50 lines · Peter Zijlstra

Quoted 12 lines · Will Deacon
Using only compiler barriers here raises my eyebrows a little, but as long as this is all in the context of the reader thread, then it should be ok. I think it means that the comment in events/core.c is bogus:
      /*
       * Disable preemption so as to not let the corresponding user-space
       * spin too long if we get preempted.
       */
      preempt_disable();
      ++userpg->lock;
      barrier();
Quoted 2 lines
because user-space won't get stuck spinning unless its running concurrently, at which point it's hosed anyway. Hmmm.
Right, we should fix that. The only reason to keep that preempt_disable() there is so that the various timestamps retain temporal locality.
Quoted 20 lines
Quoted 20 lines
arch/arm64/kernel/perf_event.c +20 -0
+void arch_perf_update_userpage(struct perf_event *event,
+                          struct perf_event_mmap_page *userpg, u64 now)
+{
+   u32 freq;
+   u32 shift;
+
+   /*
+    * Internal timekeeping for enabled/running/stopped times
+    * is always computed with the sched_clock.
+    */
+   freq = arch_timer_get_rate();
+   userpg->cap_user_time = 1;
+
+   clocks_calc_mult_shift(&userpg->time_mult, &shift, freq,
+                   NSEC_PER_SEC, 0);
+   /*
+    * time_shift is not expected to be greater than 31 due to
+    * the original published conversion algorithm shifting a
+    * 32-bit value (now specifies a 64-bit value) - refer
+    * perf_event_mmap_page documentation in perf_event.h.
Quoted 8 lines
arch/arm64/kernel/perf_event.c static int __init armv8_pmu_driver_init(void) · line 1157
Quoted 5 lines
arch/arm64/kernel/perf_event.c +5 -0
+    */
+   if (shift == 32) {
+           shift = 31;
+           userpg->time_mult >>= 1;
+   }
Can you explain this a bit more, please? Given that we've never enabled this on arm64, why do we have a problem with a legacy algorithm?

Quoted 2 lines

The worry is, I suppose, that someone still uses the old algorithm and then ports it to arm64 without checking.

Thats exactly what I was thinking. If we flip this bit, existing user code that is
cross platform may change behavior and break if it relies on the old API.

Quoted 20 lines

Quoted 10 lines
arch/arm64/kernel/perf_event.c static int __init armv8_pmu_driver_init(void) · line 1164
Quoted 9 lines
arch/arm64/kernel/perf_event.c +7 -0
+   userpg->time_shift = (u16)shift;
+   userpg->time_offset = -now;
+
+   /*
+    * We are always using the sched_clock as the base, so
+    * cap_user_time_zero makes sense.
+    */
Quoted 7 lines
How are you enforcing that we're always using sched_clock?
arch/arm64/kernel/perf_event.c static int __init armv8_pmu_driver_init(void) · line 1166
Quoted 2 lines
arch/arm64/kernel/perf_event.c +2 -0
+   userpg->cap_user_time_zero = 1;
+   userpg->time_zero = 0;
I'm not sure we can rely on the architected timer being zero initialised -- our booting documentation just require it to be consistent between CPUs.
On x86 the above is:

Quoted 8 lines

        /*
         * cap_user_time_zero doesn't make sense when we're using a different
         * time base for the records.
         */
        if (!event->attr.use_clockid) {
                userpg->cap_user_time_zero = 1;
                userpg->time_zero = offset;
        }

I was under the impression that the updating of the user page was always done using perf_clock() which uses the local_clock which is the sched_clock. I saw that the use_clockid allows setting the clock to use in other parts of the perf event system, but I didn't think it affected the user page update. This comment seems to support my claim:
/*
* Internal timekeeping for enabled/running/stopped times
* is always in the local_clock domain.
*/
Why does having a different time base for the records make cap_user_time_zero not make sense?

I could rework the time_zero offset to be computed using ktime_get_boot_ns, or remove cap_user_time_zero from this patch since it is a separate concept from cap_user_time.