perf/core: Set event shadow time for inactive events too

While commit f79256532682 ("perf/core: fix userpage->time_enabled of inactive events") fixed this problem for user rdpmc usage, bperf (perf stat with BPF) still has the same problem that accessing inactive perf events from BPF using bpf_perf_event_read_value().

You can reproduce this problem easily. As this is about a small window with multiplexing, we need a large number of events and short duration like below:

 # perf stat -a -v --bpf-counters -e instructions,branches,branch-misses \
   -e cache-references,cache-misses,bus-cycles,ref-cycles,cycles sleep 0.1

Control descriptor is not initialized
instructions: 19616489 431324015 360374366
branches: 3685346 417640114 344175443
branch-misses: 75714 404089360 336145421
cache-references: 438667 390474289 327444074
cache-misses: 49279 349333164 272835067
bus-cycles: 631887 283423953 165164214
ref-cycles: 2578771111104847872 18446744069443110306 182116355
cycles: 1785221016051271680 18446744071682768912 115821694

Performance counter stats for 'system wide':

         19,616,489      instructions              #    0.00  insn per cycle           ( 83.55%)
          3,685,346      branches                                                      ( 82.41%)
             75,714      branch-misses             #    2.05% of all branches          ( 83.19%)
            438,667      cache-references                                              ( 83.86%)
             49,279      cache-misses              #   11.234 % of all cache refs      ( 78.10%)
            631,887      bus-cycles                                                    ( 58.27%)
 2,578,771,111,104,847,872      ref-cycles                                                     (0.00%)
 1,785,221,016,051,271,680      cycles                                                         (0.00%)

      0.010824702 seconds time elapsed

As you can see, it shows invalid values for the last two events. The -v option shows that the enabled time is way bigger than the running time. So it scaled the counter values using the ratio between the two and resulted in that. This problem can get worse if users want no-aggregation or cgroup aggregation with a small interval.

Actually 18446744069443110306 is 0xffffffff01b345a2 so it seems to have a negative enabled time. In fact, bperf keeps values returned by bpf_perf_event_read_value() which calls perf_event_read_local(), and accumulates delta between two calls. When event->shadow_ctx_time is not set, it'd return invalid enabled time which is bigger than normal. Later, the shadow time is set and the function starts to return a valid time. At the moment, the recent value is smaller than before so the delta in the bperf can be negative.

I think we need to set the shadow time even the events are inactive so that BPF programs (or other potential users) can see valid time values anytime.

Cc Song Liu <[email protected]>

Signed-off-by Namhyung Kim <[email protected]>

While commit f79256532682 ("perf/core: fix userpage->time_enabled of inactive events") fixed this problem for user rdpmc usage,

Quoted 2 lines · Namhyung Kim

While commit f79256532682 ("perf/core: fix userpage->time_enabled of inactive events") fixed this problem for user rdpmc usage,

You're referring to 'this problem' before actually describing a problem :-(

Also, you now have me looking at that commit again, and I'm still hating it. Also, I'm again struggling to make sense of it; all except the very last hunk that is.

So the whole, full-fat, mmap self-monitor thing looks like:

        u32 seq, time_mult, time_shift, index, width = 64;
        u64 count, enabled, running;
        u64 cyc, time_offset, time_cycles = 0, time_mask = ~0ULL;
        u64 quot, rem, delta;
        s64 pmc = 0;

        do {
                seq = pc->lock;
                barrier();

                enabled = pc->time_enabled;
                running = pc->time_running;

                if (pc->cap_user_time && enabled != running) {
                        cyc = rdtsc();
                        time_offset = pc->time_offset;
                        time_mult   = pc->time_mult;
                        time_shift  = pc->time_shift;
                }

                if (pc->cap_user_time_short) {
                        time_cycles = pc->time_cycles;
                        time_mask   = pc->time_mask;
                }

                index = pc->index;
                count = pc->offset;
                if (pc->cap_user_rdpmc && index) {
                        width = pc->pmc_width;
                        pmc = rdpmc(index - 1);
                }

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

        if (width < 64) {
                pmc <<= 64 - width;
                pmc >>= 64 - width;
        }
        count += pmc;

        cyc = time_cycles + ((cyc - time_cycles) & time_mask);

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

        enabled += delta;
        if (index)
                running += delta;

        quot = count / running;
        rem  = count % running;
        count = quot * enabled + (rem * enabled) / running;

Now, the thing that sticks out to me is that 'enabled' is unconditionally advanced. It *always* runs.

So how can not updating ->time_enabled when the counter is INACTIVE due to rotation (which causes ->index == 0), cause enabled to not be up-to-date?

Can we please figure that out so I can go revert all but the last hunk of that patch?

Hi Peter,

Quoted 6 lines · Peter Zijlstra

Quoted 2 lines · Namhyung Kim

While commit f79256532682 ("perf/core: fix userpage->time_enabled of inactive events") fixed this problem for user rdpmc usage,

You're referring to 'this problem' before actually describing a problem :-(

Well, it's a problem of reporting incorrect 'enabled' time. I'm sorry if it was not clear.

Quoted 68 lines

Also, you now have me looking at that commit again, and I'm still hating it. Also, I'm again struggling to make sense of it; all except the very last hunk that is.

So the whole, full-fat, mmap self-monitor thing looks like:

        u32 seq, time_mult, time_shift, index, width = 64;
        u64 count, enabled, running;
        u64 cyc, time_offset, time_cycles = 0, time_mask = ~0ULL;
        u64 quot, rem, delta;
        s64 pmc = 0;

        do {
                seq = pc->lock;
                barrier();

                enabled = pc->time_enabled;
                running = pc->time_running;

                if (pc->cap_user_time && enabled != running) {
                        cyc = rdtsc();
                        time_offset = pc->time_offset;
                        time_mult   = pc->time_mult;
                        time_shift  = pc->time_shift;
                }

                if (pc->cap_user_time_short) {
                        time_cycles = pc->time_cycles;
                        time_mask   = pc->time_mask;
                }

                index = pc->index;
                count = pc->offset;
                if (pc->cap_user_rdpmc && index) {
                        width = pc->pmc_width;
                        pmc = rdpmc(index - 1);
                }

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

        if (width < 64) {
                pmc <<= 64 - width;
                pmc >>= 64 - width;
        }
        count += pmc;

        cyc = time_cycles + ((cyc - time_cycles) & time_mask);

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

        enabled += delta;
        if (index)
                running += delta;

        quot = count / running;
        rem  = count % running;
        count = quot * enabled + (rem * enabled) / running;

Now, the thing that sticks out to me is that 'enabled' is unconditionally advanced. It *always* runs.

Quoted 3 lines

So how can not updating ->time_enabled when the counter is INACTIVE due to rotation (which causes ->index == 0), cause enabled to not be up-to-date?

Hmm.. I don't get it. In my understanding, that's the whole point of the enabled time - tracking time it was not active due to the multiplexing (rotation). So that users might want to scale the count based on the ratio of running vs enabled.

Do I miss something?

Actually 18446744069443110306 is 0xffffffff01b345a2 so it seems to have a negative enabled time. In fact, bperf keeps values returned by bpf_perf_event_read_value() which calls perf_event_read_local(), and accumulates delta between two calls. When event->shadow_ctx_time is not set, it'd return invalid enabled time which is bigger than normal.

Quoted 5 lines · Namhyung Kim

Actually 18446744069443110306 is 0xffffffff01b345a2 so it seems to have a negative enabled time. In fact, bperf keeps values returned by bpf_perf_event_read_value() which calls perf_event_read_local(), and accumulates delta between two calls. When event->shadow_ctx_time is not set, it'd return invalid enabled time which is bigger than normal.

*that*, how does it happen that shadow_time isn't set? It should be last set when the event switches to INACTIVE, no? At which point the logic in perf_event_read_local() should make @enabled move forward while @running stays put.

Let me go rummage around a bit... either I'm missing something obvious or something's smelly.

Quoted 13 lines · Peter Zijlstra

Quoted 5 lines · Namhyung Kim

Actually 18446744069443110306 is 0xffffffff01b345a2 so it seems to have a negative enabled time. In fact, bperf keeps values returned by bpf_perf_event_read_value() which calls perf_event_read_local(), and accumulates delta between two calls. When event->shadow_ctx_time is not set, it'd return invalid enabled time which is bigger than normal.

*that*, how does it happen that shadow_time isn't set? It should be last set when the event switches to INACTIVE, no? At which point the logic in perf_event_read_local() should make @enabled move forward while @running stays put.

Quoted 2 lines

Let me go rummage around a bit... either I'm missing something obvious or something's smelly.

How's this then?

include/linux/perf_event.h +1 -3

index 7b7525e9155f..82baa8bdfc86 100644
@@ -686,9 +686,7 @@ struct perf_event {
        /*
         * timestamp shadows the actual context timing but it can
         * be safely used in NMI interrupt context. It reflects the
-        * context time as it was when the event was last scheduled in,
-        * or when ctx_sched_in failed to schedule the event because we
-        * run out of PMC.
+        * context time as it was when the event was last scheduled in.
         *
         * ctx_time already accounts for ctx->timestamp. Therefore to
         * compute ctx_time for a sample, simply add perf_clock().

kernel/events/core.c +3 -0

index 28aaeacdaea1..20637b7f420c 100644
@@ -640,6 +640,9 @@ __perf_update_times(struct perf_event *event, u64 now, u64 *enabled, u64 *runnin
                *running += delta;
 }
 
+static void perf_set_shadow_time(struct perf_event *event,
+                                struct perf_event_context *ctx);
+
 static void perf_event_update_time(struct perf_event *event)
 {
        u64 now = perf_event_time(event);

kernel/events/core.c +0 -26

 }
 
 static void perf_event_update_sibling_time(struct perf_event *leader)

@@ -2552,8 +2556,6 @@ event_sched_in(struct perf_event *event,
 
        perf_pmu_disable(event->pmu);
 
-       perf_set_shadow_time(event, ctx);
-
        perf_log_itrace_start(event);
 
        if (event->pmu->add(event, PERF_EF_START)) {

@@ -3707,29 +3709,6 @@ static noinline int visit_groups_merge(struct perf_cpu_context *cpuctx,
        return 0;
 }
 
-static inline bool event_update_userpage(struct perf_event *event)
-{
-       if (likely(!atomic_read(&event->mmap_count)))
-               return false;
-
-       perf_event_update_time(event);
-       perf_set_shadow_time(event, event->ctx);
-       perf_event_update_userpage(event);
-
-       return true;
-}
-
-static inline void group_update_userpage(struct perf_event *group_event)
-{
-       struct perf_event *event;
-
-       if (!event_update_userpage(group_event))
-               return;
-
-       for_each_sibling_event(event, group_event)
-               event_update_userpage(event);
-}
-
 static int merge_sched_in(struct perf_event *event, void *data)
 {
        struct perf_event_context *ctx = event->ctx;

@@ -3748,15 +3727,14 @@ static int merge_sched_in(struct perf_event *event, void *data)
        }
 
        if (event->state == PERF_EVENT_STATE_INACTIVE) {
-               *can_add_hw = 0;
                if (event->attr.pinned) {
                        perf_cgroup_event_disable(event, ctx);

kernel/events/core.c +0 -1

        }
 
        return 0;

@@ -6349,7 +6327,6 @@ static int perf_mmap(struct file *file, struct vm_area_struct *vma)
                ring_buffer_attach(event, rb);
 
                perf_event_update_time(event);
-               perf_set_shadow_time(event, event->ctx);
                perf_event_init_userpage(event);
                perf_event_update_userpage(event);
        } else {

Quoted 15 lines · Peter Zijlstra

Quoted 13 lines · Peter Zijlstra

Quoted 5 lines · Namhyung Kim

Actually 18446744069443110306 is 0xffffffff01b345a2 so it seems to have a negative enabled time. In fact, bperf keeps values returned by bpf_perf_event_read_value() which calls perf_event_read_local(), and accumulates delta between two calls. When event->shadow_ctx_time is not set, it'd return invalid enabled time which is bigger than normal.

*that*, how does it happen that shadow_time isn't set? It should be last set when the event switches to INACTIVE, no? At which point the logic in perf_event_read_local() should make @enabled move forward while @running stays put.

Quoted 4 lines

Quoted 2 lines

Let me go rummage around a bit... either I'm missing something obvious or something's smelly.

How's this then?

Still the same :(

Maybe because the event is enabled from the beginning. Then it might miss set_state/update_time at all.

Quoted 8 lines

kernel/events/core.c +0 -0

index 28aaeacdaea1..20637b7f420c 100644
@@ -640,6 +640,9 @@ __perf_update_times(struct perf_event *event, u64 now, u64 *enabled, u64 *runnin
                *running += delta;
 }
 

kernel/events/core.c static void perf_event_update_time(struct perf_event *event) · line 653

Quoted 11 lines

kernel/events/core.c +4 -0

+static void perf_set_shadow_time(struct perf_event *event,
+                                struct perf_event_context *ctx);
+
 static void perf_event_update_time(struct perf_event *event)
 {
        u64 now = perf_event_time(event);

@@ -647,6 +650,7 @@ static void perf_event_update_time(struct perf_event *event)
        __perf_update_times(event, now, &event->total_time_enabled,
                                        &event->total_time_running);
        event->tstamp = now;
+       perf_set_shadow_time(event, event->ctx);

I like this.

Quoted 12 lines

kernel/events/core.c +0 -5

@@ -3748,15 +3727,14 @@ static int merge_sched_in(struct perf_event *event, void *data)
        }
 
        if (event->state == PERF_EVENT_STATE_INACTIVE) {
-               *can_add_hw = 0;
                if (event->attr.pinned) {
                        perf_cgroup_event_disable(event, ctx);
                        perf_event_set_state(event, PERF_EVENT_STATE_ERROR);
-               } else {
-                       ctx->rotate_necessary = 1;
-                       perf_mux_hrtimer_restart(cpuctx);
-                       group_update_userpage(event);

kernel/events/core.c static int merge_sched_in(struct perf_event *event, void *data) · line 3737

Quoted 5 lines

kernel/events/core.c +4 -0

                }
+
+               *can_add_hw = 0;
+               ctx->rotate_necessary = 1;
+               perf_mux_hrtimer_restart(cpuctx);

Not sure about this. We might not want to rotate them if a pinned event failed...?

Quoted 19 lines · Namhyung Kim

Quoted 18 lines · Peter Zijlstra

Quoted 15 lines · Peter Zijlstra

Quoted 5 lines · Namhyung Kim

Actually 18446744069443110306 is 0xffffffff01b345a2 so it seems to have a negative enabled time. In fact, bperf keeps values returned by bpf_perf_event_read_value() which calls perf_event_read_local(), and accumulates delta between two calls. When event->shadow_ctx_time is not set, it'd return invalid enabled time which is bigger than normal.

*that*, how does it happen that shadow_time isn't set? It should be last set when the event switches to INACTIVE, no? At which point the logic in perf_event_read_local() should make @enabled move forward while @running stays put.

Let me go rummage around a bit... either I'm missing something obvious or something's smelly.

Quoted 3 lines

Quoted 1 line

How's this then?

Still the same :(

You're doing that bpf-cgroup crud, right? Where exactly do you hook into to do the counter reads?

Quoted 3 lines

Maybe because the event is enabled from the beginning. Then it might miss set_state/update_time at all.

Even then, it's set to INACTIVE and any state change thereafter needs to go through perf_event_set_state() and update the relevant timestamps.

Quoted 2 lines

You're doing that bpf-cgroup crud, right? Where exactly do you hook into to do the counter reads?

That's true but it doesn't use cgroup events actually. They are plain cpu events and BPF is called from a separate 'cgroup-switches' event to read out the counters.

Quoted 5 lines

Quoted 2 lines

Maybe because the event is enabled from the beginning. Then it might miss set_state/update_time at all.

Even then, it's set to INACTIVE and any state change thereafter needs to go through perf_event_set_state() and update the relevant timestamps.

Right, but the problem happens when you read the event *before* any state change.

Quoted 5 lines · Namhyung Kim

Actually 18446744069443110306 is 0xffffffff01b345a2 so it seems to have a negative enabled time. In fact, bperf keeps values returned by bpf_perf_event_read_value() which calls perf_event_read_local(), and accumulates delta between two calls. When event->shadow_ctx_time is not set, it'd return invalid enabled time which is bigger than normal.

*that*, how does it happen that shadow_time isn't set? It should be last set when the event switches to INACTIVE, no?

At which point the logic in
perf_event_read_local() should make @enabled move forward while @running stays put.

Let me go rummage around a bit... either I'm missing something obvious or something's smelly.

Quoted 11 lines · Peter Zijlstra

Quoted 5 lines · Namhyung Kim

Actually 18446744069443110306 is 0xffffffff01b345a2 so it seems to have a negative enabled time. In fact, bperf keeps values returned by bpf_perf_event_read_value() which calls perf_event_read_local(), and accumulates delta between two calls. When event->shadow_ctx_time is not set, it'd return invalid enabled time which is bigger than normal.

*that*, how does it happen that shadow_time isn't set? It should be last set when the event switches to INACTIVE, no?

As you can see, perf_event_set_state() doesn't set the shadow time. It's called from event_sched_in() which might result in ACTIVE or INACTIVE. But the problem is that there's a case that event_sched_in was not called at all - when group_can_go_on() returns false.

Quoted 3 lines

At which point the logic in
perf_event_read_local() should make @enabled move forward while @running stays put.

It's not about updating event->total_time_enabled, it only afftects the returned value of @enabled.

I'd say the time calculation is broken so it'd break @running as well. But this case can only happen on INACTIVE - otherwise it'd call event_sched_in() and update the shadow time properly, so no issue there. And then we can see the broken value of enabled time only.

Quoted 22 lines · Namhyung Kim

Quoted 11 lines · Peter Zijlstra

Quoted 5 lines · Namhyung Kim

Actually 18446744069443110306 is 0xffffffff01b345a2 so it seems to have a negative enabled time. In fact, bperf keeps values returned by bpf_perf_event_read_value() which calls perf_event_read_local(), and accumulates delta between two calls. When event->shadow_ctx_time is not set, it'd return invalid enabled time which is bigger than normal.

*that*, how does it happen that shadow_time isn't set? It should be last set when the event switches to INACTIVE, no?

As you can see, perf_event_set_state() doesn't set the shadow time. It's called from event_sched_in() which might result in ACTIVE or INACTIVE. But the problem is that there's a case that event_sched_in was not called at all - when group_can_go_on() returns false.

Quoted 3 lines

At which point the logic in
perf_event_read_local() should make @enabled move forward while @running stays put.

Quoted 8 lines

It's not about updating event->total_time_enabled, it only afftects the returned value of @enabled.

I'd say the time calculation is broken so it'd break @running as well. But this case can only happen on INACTIVE - otherwise it'd call event_sched_in() and update the shadow time properly, so no issue there. And then we can see the broken value of enabled time only.

I'm thinking this is a cgroup specific thing. Normally the shadow_time thing is simply a relative displacement between event-time and the global clock. That displacement never changes, except when you do IOC_DISABLE/IOC_ENABLE.

However, for cgroup things are different, since the cgroup events aren't unconditionally runnable, that is, the enabled time should only count when the cgroup is active, right?

So perhaps perf_event_read_local() should use a cgroup clock instead of perf_clock() for cgroup events.

Let me think about that some more...

I'm thinking this is a cgroup specific thing. Normally the shadow_time thing is simply a relative displacement between event-time and the global clock. That displacement never changes, except when you do IOC_DISABLE/IOC_ENABLE.

However, for cgroup things are different, since the cgroup events aren't unconditionally runnable, that is, the enabled time should only count when the cgroup is active, right?

So perhaps perf_event_read_local() should use a cgroup clock instead of perf_clock() for cgroup events.

Let me think about that some more...

Quoted 12 lines · Peter Zijlstra

I'm thinking this is a cgroup specific thing. Normally the shadow_time thing is simply a relative displacement between event-time and the global clock. That displacement never changes, except when you do IOC_DISABLE/IOC_ENABLE.

However, for cgroup things are different, since the cgroup events aren't unconditionally runnable, that is, the enabled time should only count when the cgroup is active, right?

So perhaps perf_event_read_local() should use a cgroup clock instead of perf_clock() for cgroup events.

Quoted 1 line

Let me think about that some more...

How's this then? Song, could you also please test and or better explain the problem f79256532682 pretends to cure? Because the below is reverting that, I *really* hate having to touch the events we're not scheduling.

Quoted 20 lines · Peter Zijlstra

Quoted 13 lines · Peter Zijlstra

I'm thinking this is a cgroup specific thing. Normally the shadow_time thing is simply a relative displacement between event-time and the global clock. That displacement never changes, except when you do IOC_DISABLE/IOC_ENABLE.

However, for cgroup things are different, since the cgroup events aren't unconditionally runnable, that is, the enabled time should only count when the cgroup is active, right?

So perhaps perf_event_read_local() should use a cgroup clock instead of perf_clock() for cgroup events.

Let me think about that some more...

Quoted 4 lines

How's this then? Song, could you also please test and or better explain the problem f79256532682 pretends to cure? Because the below is reverting that, I *really* hate having to touch the events we're not scheduling.

Unfortunately, this change bring the bug back. For time_enabled in rdpmc case to work properly, we have to touch all the enabled but not running events, right?

Quoted 3 lines · Song Liu

Unfortunately, this change bring the bug back. For time_enabled in rdpmc case to work properly, we have to touch all the enabled but not running events, right?

Ohh.. argh. I think I see why, it looses the context time enable edge, and because this is all strictly per-event in the uapi (there is no ctx representation) it can't be cured by improving ctx time handling :/

Bah, I so hate this.

Ohh.. argh. I think I see why, it looses the context time enable edge, and because this is all strictly per-event in the uapi (there is no ctx representation) it can't be cured by improving ctx time handling :/

Bah, I so hate this.

How's this then?

---
Subject: perf: Fix perf_event_read_local() time
From: Peter Zijlstra <[email protected]>
Date: Mon Dec 20 09:59:47 CET 2021

Time readers that cannot take locks (due to NMI etc..) currently make use of perf_event::shadow_ctx_time, which, for that event gives:

time' = now + (time - timestamp)

or, alternatively arranged:

time' = time + (now - timestamp)

IOW, the progression of time since the last time the shadow_ctx_time was updated.

There's problems with this:

A) the shadow_ctx_time is per-event, even though the ctx_time it reflects is obviously per context. The direct concequence of this is that the context needs to iterate all events all the time to keep the shadow_ctx_time in sync.

B) even with the prior point, the context itself might not be active meaning its time should not advance to begin with.

C) shadow_ctx_time isn't consistently updated when ctx_time is

There are 3 users of this stuff, that suffer differently from this:

- calc_timer_values()
- perf_output_read()
- perf_event_update_userpage() /* A */

- perf_event_read_local() /* A,B */

In particular, perf_output_read() doesn't suffer at all, because it's sample driven and hence only relevant when the event is actually running.

This same was supposed to be true for perf_event_update_userpage(), after all self-monitoring implies the context is active *HOWEVER*, as per commit f79256532682 ("perf/core: fix userpage->time_enabled of inactive events") this goes wrong when combined with counter overcommit, in that case those events that do not get scheduled when the context becomes active (task events typically) miss out on the EVENT_TIME update and ENABLED time is inflated (for a little while) with the time the context was inactive. Once the event gets rotated in, this gets corrected, leading to a non-monotonic timeflow.

perf_event_read_local() made things even worse, it can request time at any point, suffering all the problems perf_event_update_userpage() does and more. Because while perf_event_update_userpage() is limited by the context being active, perf_event_read_local() users have no such constraint.

Therefore, completely overhaul things and do away with perf_event::shadow_ctx_time. Instead have regular context time updates keep track of this offset directly and provide perf_event_time_now() to complement perf_event_time().

perf_event_time_now() will, in adition to being context wide, also take into account if the context is active. For inactive context, it will not advance time.

This latter property means the cgroup perf_cgroup_info context needs to grow addition state to track this.

Additionally, since all this is strictly per-cpu, we can use barrier() to order context activity vs context time.

Fixes 7d9285e82db5 ("perf/bpf: Extend the perf_event_read_local() interface, a.k.a. "bpf: perf event change needed for subsequent bpf helpers"")

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 include/linux/perf_event.h |   15 ---
 kernel/events/core.c       |  224 +++++++++++++++++++++++++++------------------
 2 files changed, 138 insertions(+), 101 deletions(-)

include/linux/perf_event.h +3 -12

@@ -683,18 +683,6 @@ struct perf_event {
        u64                             total_time_running;
        u64                             tstamp;
 
-       /*
-        * timestamp shadows the actual context timing but it can
-        * be safely used in NMI interrupt context. It reflects the
-        * context time as it was when the event was last scheduled in,
-        * or when ctx_sched_in failed to schedule the event because we
-        * run out of PMC.
-        *
-        * ctx_time already accounts for ctx->timestamp. Therefore to
-        * compute ctx_time for a sample, simply add perf_clock().
-        */
-       u64                             shadow_ctx_time;
-
        struct perf_event_attr          attr;
        u16                             header_size;
        u16                             id_header_size;

@@ -841,6 +829,7 @@ struct perf_event_context {
         */
        u64                             time;
        u64                             timestamp;
+       u64                             timeoffset;
 
        /*
         * These fields let us detect when two contexts have both

@@ -923,6 +912,8 @@ struct bpf_perf_event_data_kern {
 struct perf_cgroup_info {
        u64                             time;
        u64                             timestamp;
+       u64                             timeoffset;
+       int                             active;
 };
 
 struct perf_cgroup {

kernel/events/core.c +135 -89

@@ -674,6 +674,23 @@ perf_event_set_state(struct perf_event *
        WRITE_ONCE(event->state, state);
 }
 
+/*
+ * UP store-release, load-acquire
+ */
+
+#define __store_release(ptr, val)                                      \
+do {                                                                   \
+       barrier();                                                      \
+       WRITE_ONCE(*(ptr), (val));                                      \
+} while (0)
+
+#define __load_acquire(ptr)                                            \
+({                                                                     \
+       __unqual_scalar_typeof(*(ptr)) ___p = READ_ONCE(*(ptr));        \
+       barrier();                                                      \
+       ___p;                                                           \
+})
+
 #ifdef CONFIG_CGROUP_PERF
 
 static inline bool

@@ -719,34 +736,51 @@ static inline u64 perf_cgroup_event_time
        return t->time;
 }
 
-static inline void __update_cgrp_time(struct perf_cgroup *cgrp)
+static inline u64 perf_cgroup_event_time_now(struct perf_event *event, u64 now)
 {
-       struct perf_cgroup_info *info;
-       u64 now;
-
-       now = perf_clock();
+       struct perf_cgroup_info *t;
 
-       info = this_cpu_ptr(cgrp->info);
+       t = per_cpu_ptr(event->cgrp->info, event->cpu);
+       if (!__load_acquire(&t->active))
+               return t->time;
+       now += READ_ONCE(t->timeoffset);
+       return now;
+}
 
-       info->time += now - info->timestamp;
+static inline void __update_cgrp_time(struct perf_cgroup_info *info, u64 now, bool adv)
+{
+       if (adv)
+               info->time += now - info->timestamp;
        info->timestamp = now;
+       /*
+        * see update_context_time()
+        */
+       WRITE_ONCE(info->timeoffset, info->time - info->timestamp);
 }
 
-static inline void update_cgrp_time_from_cpuctx(struct perf_cpu_context *cpuctx)
+static inline void update_cgrp_time_from_cpuctx(struct perf_cpu_context *cpuctx, bool final)
 {
        struct perf_cgroup *cgrp = cpuctx->cgrp;
        struct cgroup_subsys_state *css;
+       struct perf_cgroup_info *info;
 
        if (cgrp) {
+               u64 now = perf_clock();
+
                for (css = &cgrp->css; css; css = css->parent) {
                        cgrp = container_of(css, struct perf_cgroup, css);
-                       __update_cgrp_time(cgrp);
+                       info = this_cpu_ptr(cgrp->info);
+
+                       __update_cgrp_time(info, now, true);
+                       if (final)
+                               __store_release(&info->active, 0);
                }
        }
 }
 
 static inline void update_cgrp_time_from_event(struct perf_event *event)
 {
+       struct perf_cgroup_info *info;
        struct perf_cgroup *cgrp;
 
        /*

@@ -760,8 +794,10 @@ static inline void update_cgrp_time_from
        /*
         * Do not update time when cgroup is not active
         */
-       if (cgroup_is_descendant(cgrp->css.cgroup, event->cgrp->css.cgroup))
-               __update_cgrp_time(event->cgrp);
+       if (cgroup_is_descendant(cgrp->css.cgroup, event->cgrp->css.cgroup)) {
+               info = this_cpu_ptr(event->cgrp->info);
+               __update_cgrp_time(info, perf_clock(), true);
+       }
 }
 
 static inline void

@@ -785,7 +821,8 @@ perf_cgroup_set_timestamp(struct task_st
        for (css = &cgrp->css; css; css = css->parent) {
                cgrp = container_of(css, struct perf_cgroup, css);
                info = this_cpu_ptr(cgrp->info);
-               info->timestamp = ctx->timestamp;
+               __update_cgrp_time(info, ctx->timestamp, false);
+               __store_release(&info->active, 1);
        }
 }
 

@@ -982,14 +1019,6 @@ static inline int perf_cgroup_connect(in
 }
 
 static inline void
-perf_cgroup_set_shadow_time(struct perf_event *event, u64 now)
-{
-       struct perf_cgroup_info *t;
-       t = per_cpu_ptr(event->cgrp->info, event->cpu);
-       event->shadow_ctx_time = now - t->timestamp;
-}
-
-static inline void
 perf_cgroup_event_enable(struct perf_event *event, struct perf_event_context *ctx)
 {
        struct perf_cpu_context *cpuctx;

@@ -1066,7 +1095,8 @@ static inline void update_cgrp_time_from
 {
 }
 
-static inline void update_cgrp_time_from_cpuctx(struct perf_cpu_context *cpuctx)
+static inline void update_cgrp_time_from_cpuctx(struct perf_cpu_context *cpuctx,
+                                               bool final)
 {
 }
 

@@ -1098,12 +1128,12 @@ perf_cgroup_switch(struct task_struct *t
 {
 }
 
-static inline void
-perf_cgroup_set_shadow_time(struct perf_event *event, u64 now)
+static inline u64 perf_cgroup_event_time(struct perf_event *event)
 {
+       return 0;
 }
 
-static inline u64 perf_cgroup_event_time(struct perf_event *event)
+static inline u64 perf_cgroup_event_time_now(struct perf_event *event, u64 now)
 {
        return 0;
 }

@@ -1525,22 +1555,59 @@ static void perf_unpin_context(struct pe
 /*
  * Update the record of the current time in a context.
  */
-static void update_context_time(struct perf_event_context *ctx)
+static void __update_context_time(struct perf_event_context *ctx, bool adv)
 {
        u64 now = perf_clock();
 
-       ctx->time += now - ctx->timestamp;
+       if (adv)
+               ctx->time += now - ctx->timestamp;
        ctx->timestamp = now;
+
+       /*
+        * The above: time' = time + (now - timestamp), can be re-arranged
+        * into: time` = now + (time - timestamp), which gives a single value
+        * offset to compute future time without locks on.
+        *
+        * See perf_event_time_now(), which can be used from NMI context where
+        * it's (obviously) not possible to acquire ctx->lock in order to read
+        * both the above values in a consistent manner.
+        */
+       WRITE_ONCE(ctx->timeoffset, ctx->time - ctx->timestamp);
+}
+
+static void update_context_time(struct perf_event_context *ctx)
+{
+       __update_context_time(ctx, true);
 }
 
 static u64 perf_event_time(struct perf_event *event)
 {
        struct perf_event_context *ctx = event->ctx;
 
+       if (unlikely(!ctx))
+               return 0;
+
        if (is_cgroup_event(event))
                return perf_cgroup_event_time(event);
 
-       return ctx ? ctx->time : 0;
+       return ctx->time;
+}
+
+static u64 perf_event_time_now(struct perf_event *event, u64 now)
+{
+       struct perf_event_context *ctx = event->ctx;
+
+       if (unlikely(!ctx))
+               return 0;
+
+       if (is_cgroup_event(event))
+               return perf_cgroup_event_time_now(event, now);
+
+       if (!(__load_acquire(&ctx->is_active) & EVENT_TIME))
+               return ctx->time;
+
+       now += READ_ONCE(ctx->timeoffset);
+       return now;
 }
 
 static enum event_type_t get_event_type(struct perf_event *event)

@@ -2346,7 +2413,7 @@ __perf_remove_from_context(struct perf_e
 
        if (ctx->is_active & EVENT_TIME) {
                update_context_time(ctx);
-               update_cgrp_time_from_cpuctx(cpuctx);
+               update_cgrp_time_from_cpuctx(cpuctx, false);
        }
 
        event_sched_out(event, cpuctx, ctx);

@@ -2357,6 +2424,9 @@ __perf_remove_from_context(struct perf_e
        list_del_event(event, ctx);
 
        if (!ctx->nr_events && ctx->is_active) {
+               if (ctx == &cpuctx->ctx)
+                       update_cgrp_time_from_cpuctx(cpuctx, true);
+
                ctx->is_active = 0;
                ctx->rotate_necessary = 0;
                if (ctx->task) {

@@ -2478,40 +2548,6 @@ void perf_event_disable_inatomic(struct
        irq_work_queue(&event->pending);
 }
 
-static void perf_set_shadow_time(struct perf_event *event,
-                                struct perf_event_context *ctx)
-{
-       /*
-        * use the correct time source for the time snapshot
-        *
-        * We could get by without this by leveraging the
-        * fact that to get to this function, the caller
-        * has most likely already called update_context_time()
-        * and update_cgrp_time_xx() and thus both timestamp
-        * are identical (or very close). Given that tstamp is,
-        * already adjusted for cgroup, we could say that:
-        *    tstamp - ctx->timestamp
-        * is equivalent to
-        *    tstamp - cgrp->timestamp.
-        *
-        * Then, in perf_output_read(), the calculation would
-        * work with no changes because:
-        * - event is guaranteed scheduled in
-        * - no scheduled out in between
-        * - thus the timestamp would be the same
-        *
-        * But this is a bit hairy.
-        *
-        * So instead, we have an explicit cgroup call to remain
-        * within the time source all along. We believe it
-        * is cleaner and simpler to understand.
-        */
-       if (is_cgroup_event(event))
-               perf_cgroup_set_shadow_time(event, event->tstamp);
-       else
-               event->shadow_ctx_time = event->tstamp - ctx->timestamp;
-}
-
 #define MAX_INTERRUPTS (~0ULL)
 
 static void perf_log_throttle(struct perf_event *event, int enable);

@@ -2552,8 +2588,6 @@ event_sched_in(struct perf_event *event,
 
        perf_pmu_disable(event->pmu);
 
-       perf_set_shadow_time(event, ctx);
-
        perf_log_itrace_start(event);
 
        if (event->pmu->add(event, PERF_EF_START)) {

@@ -3247,16 +3281,6 @@ static void ctx_sched_out(struct perf_ev
                return;
        }
 
-       ctx->is_active &= ~event_type;
-       if (!(ctx->is_active & EVENT_ALL))
-               ctx->is_active = 0;
-
-       if (ctx->task) {
-               WARN_ON_ONCE(cpuctx->task_ctx != ctx);
-               if (!ctx->is_active)
-                       cpuctx->task_ctx = NULL;
-       }
-
        /*
         * Always update time if it was set; not only when it changes.
         * Otherwise we can 'forget' to update time for any but the last

@@ -3270,7 +3294,22 @@ static void ctx_sched_out(struct perf_ev
        if (is_active & EVENT_TIME) {
                /* update (and stop) ctx time */
                update_context_time(ctx);
-               update_cgrp_time_from_cpuctx(cpuctx);
+               update_cgrp_time_from_cpuctx(cpuctx, ctx == &cpuctx->ctx);
+               /*
+                * CPU-release for the below ->is_active store,
+                * see __load_acquire() in perf_event_time_now()
+                */
+               barrier();
+       }
+
+       ctx->is_active &= ~event_type;
+       if (!(ctx->is_active & EVENT_ALL))
+               ctx->is_active = 0;
+
+       if (ctx->task) {
+               WARN_ON_ONCE(cpuctx->task_ctx != ctx);
+               if (!ctx->is_active)
+                       cpuctx->task_ctx = NULL;
        }
 
        is_active ^= ctx->is_active; /* changed bits */

@@ -3707,13 +3746,19 @@ static noinline int visit_groups_merge(s
        return 0;
 }
 
+/*
+ * Because the userpage is strictly per-event (there is no concept of context,
+ * so there cannot be a context indirection), every userpage must be updated
+ * when context time starts :-(
+ *
+ * IOW, we must not miss EVENT_TIME edges.
+ */
 static inline bool event_update_userpage(struct perf_event *event)
 {
        if (likely(!atomic_read(&event->mmap_count)))
                return false;
 
        perf_event_update_time(event);
-       perf_set_shadow_time(event, event->ctx);
        perf_event_update_userpage(event);
 
        return true;

@@ -3797,13 +3842,23 @@ ctx_sched_in(struct perf_event_context *
             struct task_struct *task)
 {
        int is_active = ctx->is_active;
-       u64 now;
 
        lockdep_assert_held(&ctx->lock);
 
        if (likely(!ctx->nr_events))
                return;
 
+       if (is_active ^ EVENT_TIME) {
+               /* start ctx time */
+               __update_context_time(ctx, false);
+               perf_cgroup_set_timestamp(task, ctx);
+               /*
+                * CPU-release for the below ->is_active store,
+                * see __load_acquire() in perf_event_time_now()
+                */
+               barrier();
+       }
+
        ctx->is_active |= (event_type | EVENT_TIME);
        if (ctx->task) {
                if (!is_active)

@@ -3814,13 +3869,6 @@ ctx_sched_in(struct perf_event_context *
 
        is_active ^= ctx->is_active; /* changed bits */
 
-       if (is_active & EVENT_TIME) {
-               /* start ctx time */
-               now = perf_clock();
-               ctx->timestamp = now;
-               perf_cgroup_set_timestamp(task, ctx);
-       }
-
        /*
         * First go through the list and put on any pinned groups
         * in order to give them the best chance of going on.

@@ -4473,10 +4521,9 @@ int perf_event_read_local(struct perf_ev
 
        *value = local64_read(&event->count);
        if (enabled || running) {
-               u64 now = event->shadow_ctx_time + perf_clock();
-               u64 __enabled, __running;
+               u64 __enabled, __running, __now;;
 
-               __perf_update_times(event, now, &__enabled, &__running);
+               calc_timer_values(event, &__now, &__enabled, &__running);
                if (enabled)
                        *enabled = __enabled;
                if (running)

@@ -5806,7 +5853,7 @@ static void calc_timer_values(struct per
        u64 ctx_time;
 
        *now = perf_clock();
-       ctx_time = event->shadow_ctx_time + *now;
+       ctx_time = perf_event_time_now(event, *now);
        __perf_update_times(event, ctx_time, enabled, running);
 }
 

@@ -6349,7 +6396,6 @@ static int perf_mmap(struct file *file,
                ring_buffer_attach(event, rb);
 
                perf_event_update_time(event);
-               perf_set_shadow_time(event, event->ctx);
                perf_event_init_userpage(event);
                perf_event_update_userpage(event);
        } else {

How's this then?

---
Subject: perf: Fix perf_event_read_local() time
From: Peter Zijlstra <[email protected]>
Date: Mon Dec 20 09:59:47 CET 2021

Time readers that cannot take locks (due to NMI etc..) currently make use of perf_event::shadow_ctx_time, which, for that event gives:

time' = now + (time - timestamp)

or, alternatively arranged:

time' = time + (now - timestamp)

IOW, the progression of time since the last time the shadow_ctx_time was updated.

There's problems with this:

A) the shadow_ctx_time is per-event, even though the ctx_time it reflects is obviously per context. The direct concequence of this is that the context needs to iterate all events all the time to keep the shadow_ctx_time in sync.

B) even with the prior point, the context itself might not be active meaning its time should not advance to begin with.

C) shadow_ctx_time isn't consistently updated when ctx_time is

There are 3 users of this stuff, that suffer differently from this:

- calc_timer_values()
- perf_output_read()
- perf_event_update_userpage() /* A */

- perf_event_read_local() /* A,B */

In particular, perf_output_read() doesn't suffer at all, because it's sample driven and hence only relevant when the event is actually running.

This same was supposed to be true for perf_event_update_userpage(), after all self-monitoring implies the context is active *HOWEVER*, as per commit f79256532682 ("perf/core: fix userpage->time_enabled of inactive events") this goes wrong when combined with counter overcommit, in that case those events that do not get scheduled when the context becomes active (task events typically) miss out on the EVENT_TIME update and ENABLED time is inflated (for a little while) with the time the context was inactive. Once the event gets rotated in, this gets corrected, leading to a non-monotonic timeflow.

perf_event_read_local() made things even worse, it can request time at any point, suffering all the problems perf_event_update_userpage() does and more. Because while perf_event_update_userpage() is limited by the context being active, perf_event_read_local() users have no such constraint.

Therefore, completely overhaul things and do away with perf_event::shadow_ctx_time. Instead have regular context time updates keep track of this offset directly and provide perf_event_time_now() to complement perf_event_time().

perf_event_time_now() will, in adition to being context wide, also take into account if the context is active. For inactive context, it will not advance time.

This latter property means the cgroup perf_cgroup_info context needs to grow addition state to track this.

Additionally, since all this is strictly per-cpu, we can use barrier() to order context activity vs context time.

Fixes 7d9285e82db5 ("perf/bpf: Extend the perf_event_read_local() interface, a.k.a. "bpf: perf event change needed for subsequent bpf helpers"")

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
include/linux/perf_event.h | 15 ---
kernel/events/core.c | 224 +++++++++++++++++++++++++++------------------

2 files changed, 138 insertions(+), 101 deletions(-)

include/linux/perf_event.h +3 -12

@@ -683,18 +683,6 @@ struct perf_event {
 	u64				total_time_running;
 	u64				tstamp;
 
-	/*
-	 * timestamp shadows the actual context timing but it can
-	 * be safely used in NMI interrupt context. It reflects the
-	 * context time as it was when the event was last scheduled in,
-	 * or when ctx_sched_in failed to schedule the event because we
-	 * run out of PMC.
-	 *
-	 * ctx_time already accounts for ctx->timestamp. Therefore to
-	 * compute ctx_time for a sample, simply add perf_clock().
-	 */
-	u64				shadow_ctx_time;
-
 	struct perf_event_attr		attr;
 	u16				header_size;
 	u16				id_header_size;

@@ -841,6 +829,7 @@ struct perf_event_context {
 	 */
 	u64				time;
 	u64				timestamp;
+	u64				timeoffset;
 
 	/*
 	 * These fields let us detect when two contexts have both

@@ -923,6 +912,8 @@ struct bpf_perf_event_data_kern {
 struct perf_cgroup_info {
 	u64				time;
 	u64				timestamp;
+	u64				timeoffset;
+	int				active;
 };
 
 struct perf_cgroup {

kernel/events/core.c +17 -0

@@ -674,6 +674,23 @@ perf_event_set_state(struct perf_event *
 	WRITE_ONCE(event->state, state);
 }
 
+/*
+ * UP store-release, load-acquire
+ */
+
+#define __store_release(ptr, val)					\
+do {									\
+	barrier();							\
+	WRITE_ONCE(*(ptr), (val));					\
+} while (0)
+
+#define __load_acquire(ptr)						\
+({									\
+	__unqual_scalar_typeof(*(ptr)) ___p = READ_ONCE(*(ptr));	\
+	barrier();							\
+	___p;								\
+})
+
 #ifdef CONFIG_CGROUP_PERF
 

static inline bool

kernel/events/core.c +30 -11

@@ -719,34 +736,51 @@ static inline u64 perf_cgroup_event_time
 	return t->time;
 }
 
-static inline void __update_cgrp_time(struct perf_cgroup *cgrp)
+static inline u64 perf_cgroup_event_time_now(struct perf_event *event, u64 now)
 {
-	struct perf_cgroup_info *info;
-	u64 now;
-
-	now = perf_clock();
+	struct perf_cgroup_info *t;
 
-	info = this_cpu_ptr(cgrp->info);
+	t = per_cpu_ptr(event->cgrp->info, event->cpu);
+	if (!__load_acquire(&t->active))
+		return t->time;
+	now += READ_ONCE(t->timeoffset);
+	return now;
+}
 
-	info->time += now - info->timestamp;
+static inline void __update_cgrp_time(struct perf_cgroup_info *info, u64 now, bool adv)
+{
+	if (adv)
+		info->time += now - info->timestamp;
 	info->timestamp = now;
+	/*
+	 * see update_context_time()
+	 */
+	WRITE_ONCE(info->timeoffset, info->time - info->timestamp);
 }
 
-static inline void update_cgrp_time_from_cpuctx(struct perf_cpu_context *cpuctx)
+static inline void update_cgrp_time_from_cpuctx(struct perf_cpu_context *cpuctx, bool final)
 {
 	struct perf_cgroup *cgrp = cpuctx->cgrp;
 	struct cgroup_subsys_state *css;
+	struct perf_cgroup_info *info;
 
 	if (cgrp) {
+		u64 now = perf_clock();
+
 		for (css = &cgrp->css; css; css = css->parent) {
 			cgrp = container_of(css, struct perf_cgroup, css);
-			__update_cgrp_time(cgrp);
+			info = this_cpu_ptr(cgrp->info);
+
+			__update_cgrp_time(info, now, true);
+			if (final)
+				__store_release(&info->active, 0);
 		}
 	}
 }
 
 static inline void update_cgrp_time_from_event(struct perf_event *event)
 {
+	struct perf_cgroup_info *info;
 	struct perf_cgroup *cgrp;
 
 	/*

@@ -760,8 +794,10 @@ static inline void update_cgrp_time_from
 	/*
 	 * Do not update time when cgroup is not active
 	 */
-	if (cgroup_is_descendant(cgrp->css.cgroup, event->cgrp->css.cgroup))
-		__update_cgrp_time(event->cgrp);
+	if (cgroup_is_descendant(cgrp->css.cgroup, event->cgrp->css.cgroup)) {
+		info = this_cpu_ptr(event->cgrp->info);
+		__update_cgrp_time(info, perf_clock(), true);
+	}
 }
 

static inline void

kernel/events/core.c +2 -1

@@ -785,7 +821,8 @@ perf_cgroup_set_timestamp(struct task_st
 	for (css = &cgrp->css; css; css = css->parent) {
 		cgrp = container_of(css, struct perf_cgroup, css);
 		info = this_cpu_ptr(cgrp->info);
-		info->timestamp = ctx->timestamp;
+		__update_cgrp_time(info, ctx->timestamp, false);
+		__store_release(&info->active, 1);
 	}
 }
 

@@ -982,14 +1019,6 @@ static inline int perf_cgroup_connect(in
 }
 

static inline void

kernel/events/core.c +0 -8

-perf_cgroup_set_shadow_time(struct perf_event *event, u64 now)
-{
-	struct perf_cgroup_info *t;
-	t = per_cpu_ptr(event->cgrp->info, event->cpu);
-	event->shadow_ctx_time = now - t->timestamp;
-}
-
-static inline void

perf_cgroup_event_enable(struct perf_event *event, struct perf_event_context *ctx)

{
	struct perf_cpu_context *cpuctx;

kernel/events/core.c +86 -69

@@ -1066,7 +1095,8 @@ static inline void update_cgrp_time_from
 {
 }
 
-static inline void update_cgrp_time_from_cpuctx(struct perf_cpu_context *cpuctx)
+static inline void update_cgrp_time_from_cpuctx(struct perf_cpu_context *cpuctx,
+						bool final)
 {
 }
 

@@ -1098,12 +1128,12 @@ perf_cgroup_switch(struct task_struct *t
 {
 }
 
-static inline void
-perf_cgroup_set_shadow_time(struct perf_event *event, u64 now)
+static inline u64 perf_cgroup_event_time(struct perf_event *event)
 {
+	return 0;
 }
 
-static inline u64 perf_cgroup_event_time(struct perf_event *event)
+static inline u64 perf_cgroup_event_time_now(struct perf_event *event, u64 now)
 {
 	return 0;
 }

@@ -1525,22 +1555,59 @@ static void perf_unpin_context(struct pe
 /*
 * Update the record of the current time in a context.
 */
-static void update_context_time(struct perf_event_context *ctx)
+static void __update_context_time(struct perf_event_context *ctx, bool adv)
 {
 	u64 now = perf_clock();
 
-	ctx->time += now - ctx->timestamp;
+	if (adv)
+		ctx->time += now - ctx->timestamp;
 	ctx->timestamp = now;
+
+	/*
+	 * The above: time' = time + (now - timestamp), can be re-arranged
+	 * into: time` = now + (time - timestamp), which gives a single value
+	 * offset to compute future time without locks on.
+	 *
+	 * See perf_event_time_now(), which can be used from NMI context where
+	 * it's (obviously) not possible to acquire ctx->lock in order to read
+	 * both the above values in a consistent manner.
+	 */
+	WRITE_ONCE(ctx->timeoffset, ctx->time - ctx->timestamp);
+}
+
+static void update_context_time(struct perf_event_context *ctx)
+{
+	__update_context_time(ctx, true);
 }
 
 static u64 perf_event_time(struct perf_event *event)
 {
 	struct perf_event_context *ctx = event->ctx;
 
+	if (unlikely(!ctx))
+		return 0;
+
 	if (is_cgroup_event(event))
 		return perf_cgroup_event_time(event);
 
-	return ctx ? ctx->time : 0;
+	return ctx->time;
+}
+
+static u64 perf_event_time_now(struct perf_event *event, u64 now)
+{
+	struct perf_event_context *ctx = event->ctx;
+
+	if (unlikely(!ctx))
+		return 0;
+
+	if (is_cgroup_event(event))
+		return perf_cgroup_event_time_now(event, now);
+
+	if (!(__load_acquire(&ctx->is_active) & EVENT_TIME))
+		return ctx->time;
+
+	now += READ_ONCE(ctx->timeoffset);
+	return now;
 }
 
 static enum event_type_t get_event_type(struct perf_event *event)

@@ -2346,7 +2413,7 @@ __perf_remove_from_context(struct perf_e
 
 	if (ctx->is_active & EVENT_TIME) {
 		update_context_time(ctx);
-		update_cgrp_time_from_cpuctx(cpuctx);
+		update_cgrp_time_from_cpuctx(cpuctx, false);
 	}
 
 	event_sched_out(event, cpuctx, ctx);

@@ -2357,6 +2424,9 @@ __perf_remove_from_context(struct perf_e
 	list_del_event(event, ctx);
 
 	if (!ctx->nr_events && ctx->is_active) {
+		if (ctx == &cpuctx->ctx)
+			update_cgrp_time_from_cpuctx(cpuctx, true);
+
 		ctx->is_active = 0;
 		ctx->rotate_necessary = 0;
 		if (ctx->task) {

@@ -2478,40 +2548,6 @@ void perf_event_disable_inatomic(struct
 	irq_work_queue(&event->pending);
 }
 
-static void perf_set_shadow_time(struct perf_event *event,
-				 struct perf_event_context *ctx)
-{
-	/*
-	 * use the correct time source for the time snapshot
-	 *
-	 * We could get by without this by leveraging the
-	 * fact that to get to this function, the caller
-	 * has most likely already called update_context_time()
-	 * and update_cgrp_time_xx() and thus both timestamp
-	 * are identical (or very close). Given that tstamp is,
-	 * already adjusted for cgroup, we could say that:
-	 *    tstamp - ctx->timestamp
-	 * is equivalent to
-	 *    tstamp - cgrp->timestamp.
-	 *
-	 * Then, in perf_output_read(), the calculation would
-	 * work with no changes because:
-	 * - event is guaranteed scheduled in
-	 * - no scheduled out in between
-	 * - thus the timestamp would be the same
-	 *
-	 * But this is a bit hairy.
-	 *
-	 * So instead, we have an explicit cgroup call to remain
-	 * within the time source all along. We believe it
-	 * is cleaner and simpler to understand.
-	 */
-	if (is_cgroup_event(event))
-		perf_cgroup_set_shadow_time(event, event->tstamp);
-	else
-		event->shadow_ctx_time = event->tstamp - ctx->timestamp;
-}
-
 #define MAX_INTERRUPTS (~0ULL)
 
 static void perf_log_throttle(struct perf_event *event, int enable);

@@ -2552,8 +2588,6 @@ event_sched_in(struct perf_event *event,
 
 	perf_pmu_disable(event->pmu);
 
-	perf_set_shadow_time(event, ctx);
-
 	perf_log_itrace_start(event);
 
 	if (event->pmu->add(event, PERF_EF_START)) {

@@ -3247,16 +3281,6 @@ static void ctx_sched_out(struct perf_ev
 		return;
 	}
 
-	ctx->is_active &= ~event_type;
-	if (!(ctx->is_active & EVENT_ALL))
-		ctx->is_active = 0;
-
-	if (ctx->task) {
-		WARN_ON_ONCE(cpuctx->task_ctx != ctx);
-		if (!ctx->is_active)
-			cpuctx->task_ctx = NULL;
-	}
-
 	/*
 	 * Always update time if it was set; not only when it changes.
 	 * Otherwise we can 'forget' to update time for any but the last

@@ -3270,7 +3294,22 @@ static void ctx_sched_out(struct perf_ev
 	if (is_active & EVENT_TIME) {
 		/* update (and stop) ctx time */
 		update_context_time(ctx);
-		update_cgrp_time_from_cpuctx(cpuctx);
+		update_cgrp_time_from_cpuctx(cpuctx, ctx == &cpuctx->ctx);
+		/*
+		 * CPU-release for the below ->is_active store,
+		 * see __load_acquire() in perf_event_time_now()
+		 */
+		barrier();
+	}
+
+	ctx->is_active &= ~event_type;
+	if (!(ctx->is_active & EVENT_ALL))
+		ctx->is_active = 0;
+
+	if (ctx->task) {
+		WARN_ON_ONCE(cpuctx->task_ctx != ctx);
+		if (!ctx->is_active)
+			cpuctx->task_ctx = NULL;
 	}
 
 	is_active ^= ctx->is_active; /* changed bits */

@@ -3707,13 +3746,19 @@ static noinline int visit_groups_merge(s
 	return 0;
 }
 
+/*
+ * Because the userpage is strictly per-event (there is no concept of context,
+ * so there cannot be a context indirection), every userpage must be updated
+ * when context time starts :-(
+ *
+ * IOW, we must not miss EVENT_TIME edges.
+ */
 static inline bool event_update_userpage(struct perf_event *event)
 {
 	if (likely(!atomic_read(&event->mmap_count)))
 		return false;
 
 	perf_event_update_time(event);
-	perf_set_shadow_time(event, event->ctx);
 	perf_event_update_userpage(event);
 
 	return true;

@@ -3797,13 +3842,23 @@ ctx_sched_in(struct perf_event_context *
 	     struct task_struct *task)
 {
 	int is_active = ctx->is_active;
-	u64 now;
 
 	lockdep_assert_held(&ctx->lock);
 
 	if (likely(!ctx->nr_events))
 		return;
 
+	if (is_active ^ EVENT_TIME) {
+		/* start ctx time */
+		__update_context_time(ctx, false);
+		perf_cgroup_set_timestamp(task, ctx);
+		/*
+		 * CPU-release for the below ->is_active store,
+		 * see __load_acquire() in perf_event_time_now()
+		 */
+		barrier();
+	}
+
 	ctx->is_active |= (event_type | EVENT_TIME);
 	if (ctx->task) {
 		if (!is_active)

@@ -3814,13 +3869,6 @@ ctx_sched_in(struct perf_event_context *
 
 	is_active ^= ctx->is_active; /* changed bits */
 
-	if (is_active & EVENT_TIME) {
-		/* start ctx time */
-		now = perf_clock();
-		ctx->timestamp = now;
-		perf_cgroup_set_timestamp(task, ctx);
-	}
-
 	/*
 	 * First go through the list and put on any pinned groups
 	 * in order to give them the best chance of going on.

@@ -4473,10 +4521,9 @@ int perf_event_read_local(struct perf_ev
 
 	*value = local64_read(&event->count);
 	if (enabled || running) {
-		u64 now = event->shadow_ctx_time + perf_clock();
-		u64 __enabled, __running;
+		u64 __enabled, __running, __now;;
 
-		__perf_update_times(event, now, &__enabled, &__running);
+		calc_timer_values(event, &__now, &__enabled, &__running);
 		if (enabled)
 			*enabled = __enabled;
 		if (running)

@@ -5806,7 +5853,7 @@ static void calc_timer_values(struct per
 	u64 ctx_time;
 
 	*now = perf_clock();
-	ctx_time = event->shadow_ctx_time + *now;
+	ctx_time = perf_event_time_now(event, *now);
 	__perf_update_times(event, ctx_time, enabled, running);
 }
 

@@ -6349,7 +6396,6 @@ static int perf_mmap(struct file *file,
 		ring_buffer_attach(event, rb);
 
 		perf_event_update_time(event);
-		perf_set_shadow_time(event, event->ctx);
 		perf_event_init_userpage(event);
 		perf_event_update_userpage(event);
 	} else {

I think we need to forward the declaration of calc_timer_values?

With that fixed, this passes the test for enabled time.

Tested-by Song Liu <[email protected]>