Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1724437 > unrolled thread

[RFC][PATCH] perf: Rewrite enabled/running timekeeping

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC][PATCH] perf: Rewrite enabled/running timekeeping Peter Zijlstra <peterz@infradead.org> - 2017-08-31 19:20 +0200
    Re: [RFC][PATCH] perf: Rewrite enabled/running timekeeping Alexey Budankov <alexey.budankov@linux.intel.com> - 2017-09-01 12:50 +0200
      Re: [RFC][PATCH] perf: Rewrite enabled/running timekeeping Peter Zijlstra <peterz@infradead.org> - 2017-09-01 14:40 +0200
    Re: [RFC][PATCH] perf: Rewrite enabled/running timekeeping Alexey Budankov <alexey.budankov@linux.intel.com> - 2017-09-01 13:20 +0200
      Re: [RFC][PATCH] perf: Rewrite enabled/running timekeeping Peter Zijlstra <peterz@infradead.org> - 2017-09-01 14:50 +0200
    Re: [RFC][PATCH] perf: Rewrite enabled/running timekeeping Vince Weaver <vince@deater.net> - 2017-09-01 23:30 +0200
    Re: [RFC][PATCH] perf: Rewrite enabled/running timekeeping Alexey Budankov <alexey.budankov@linux.intel.com> - 2017-09-04 12:50 +0200
      Re: [RFC][PATCH] perf: Rewrite enabled/running timekeeping Peter Zijlstra <peterz@infradead.org> - 2017-09-04 14:10 +0200
        Re: [RFC][PATCH] perf: Rewrite enabled/running timekeeping Alexey Budankov <alexey.budankov@linux.intel.com> - 2017-09-04 17:00 +0200
          Re: [RFC][PATCH] perf: Rewrite enabled/running timekeeping Peter Zijlstra <peterz@infradead.org> - 2017-09-04 17:50 +0200
            Re: [RFC][PATCH] perf: Rewrite enabled/running timekeeping Peter Zijlstra <peterz@infradead.org> - 2017-09-04 18:00 +0200
            Re: [RFC][PATCH] perf: Rewrite enabled/running timekeeping Peter Zijlstra <peterz@infradead.org> - 2017-09-05 13:30 +0200
            Re: [RFC][PATCH] perf: Rewrite enabled/running timekeeping Alexey Budankov <alexey.budankov@linux.intel.com> - 2017-09-05 14:10 +0200
              Re: [RFC][PATCH] perf: Rewrite enabled/running timekeeping Peter Zijlstra <peterz@infradead.org> - 2017-09-05 15:00 +0200
              Re: [RFC][PATCH] perf: Rewrite enabled/running timekeeping Peter Zijlstra <peterz@infradead.org> - 2017-09-05 18:10 +0200
                Re: [RFC][PATCH] perf: Rewrite enabled/running timekeeping Alexey Budankov <alexey.budankov@linux.intel.com> - 2017-09-06 15:50 +0200
                Re: [RFC][PATCH] perf: Rewrite enabled/running timekeeping Alexey Budankov <alexey.budankov@linux.intel.com> - 2017-09-08 10:50 +0200
    Re: [RFC][PATCH] perf: Rewrite enabled/running timekeeping Peter Zijlstra <peterz@infradead.org> - 2017-09-05 11:50 +0200

#1724437 — [RFC][PATCH] perf: Rewrite enabled/running timekeeping

On Wed, Aug 23, 2017 at 11:54:15AM +0300, Alexey Budankov wrote:
> On 22.08.2017 23:47, Peter Zijlstra wrote:
> > On Thu, Aug 10, 2017 at 06:57:43PM +0300, Alexey Budankov wrote:
> >> The key thing in the patch is explicit updating of tstamp fields for
> >> INACTIVE events in update_event_times().
> > 
> >> @@ -1405,6 +1426,9 @@ static void update_event_times(struct perf_event *event)
> >>  	    event->group_leader->state < PERF_EVENT_STATE_INACTIVE)
> >>  		return;
> >>  
> >> +	if (event->state == PERF_EVENT_STATE_INACTIVE)
> >> +		perf_event_tstamp_update(event);
> >> +
> >>  	/*
> >>  	 * in cgroup mode, time_enabled represents
> >>  	 * the time the event was enabled AND active
> > 
> > But why!? I thought the whole point was to not need to do this.
> 
> update_event_times() is not called from timer interrupt handler 
> thus it is not on the critical path which is optimized in this patch set.
> 
> But update_event_times() is called in the context of read() syscall so
> this is the place where we may update event times for INACTIVE events 
> instead of timer interrupt.
> 
> Also update_event_times() is called on thread context switch out so
> we get event times also updated when the thread migrates to other CPU.
> 
> > 
> > The thing I outlined earlier would only need to update timestamps when
> > events change state and at no other point in time.
> 
> But we still may request times while event is in INACTIVE state 
> thru read() syscall and event timings need to be up-to-date. 

Sure, read() also updates.

So the below completely rewrites timekeeping (and probably breaks
world) but does away with the need to touch events that don't get
scheduled.

Esp the cgroup stuff is entirely untested since I simply don't know how
to operate that. I did run Vince's tests on it, and I think it doesn't
regress, but I'm near a migraine so I can't really see straight atm.

Vince, Stephane, could you guys have a peek?

(There's a few other bits in, I'll break up into patches and write
comments and Changelogs later, I think its can be split in some 5
patches).

The basic idea is really simple, we have a single timestamp and
depending on the state we update enabled/running. This obviously only
requires updates when we change state and when we need up-to-date
timestamps (read).

No more weird and wonderful mind bending interaction between 3 different
timestamps with arcane update rules.

---
 include/linux/perf_event.h |  25 +-
 kernel/events/core.c       | 551 ++++++++++++++++-----------------------------
 2 files changed, 192 insertions(+), 384 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 8e22f24ded6a..2a6ae48a1a96 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -485,9 +485,9 @@ struct perf_addr_filters_head {
 };
 
 /**
- * enum perf_event_active_state - the states of a event
+ * enum perf_event_state - the states of a event
  */
-enum perf_event_active_state {
+enum perf_event_state {
 	PERF_EVENT_STATE_DEAD		= -4,
 	PERF_EVENT_STATE_EXIT		= -3,
 	PERF_EVENT_STATE_ERROR		= -2,
@@ -578,7 +578,7 @@ struct perf_event {
 	struct pmu			*pmu;
 	void				*pmu_private;
 
-	enum perf_event_active_state	state;
+	enum perf_event_state		state;
 	unsigned int			attach_state;
 	local64_t			count;
 	atomic64_t			child_count;
@@ -588,26 +588,10 @@ struct perf_event {
 	 * has been enabled (i.e. eligible to run, and the task has
 	 * been scheduled in, if this is a per-task event)
 	 * and running (scheduled onto the CPU), respectively.
-	 *
-	 * They are computed from tstamp_enabled, tstamp_running and
-	 * tstamp_stopped when the event is in INACTIVE or ACTIVE state.
 	 */
 	u64				total_time_enabled;
 	u64				total_time_running;
-
-	/*
-	 * These are timestamps used for computing total_time_enabled
-	 * and total_time_running when the event is in INACTIVE or
-	 * ACTIVE state, measured in nanoseconds from an arbitrary point
-	 * in time.
-	 * tstamp_enabled: the notional time when the event was enabled
-	 * tstamp_running: the notional time when the event was scheduled on
-	 * tstamp_stopped: in INACTIVE state, the notional time when the
-	 *	event was scheduled off.
-	 */
-	u64				tstamp_enabled;
-	u64				tstamp_running;
-	u64				tstamp_stopped;
+	u64				tstamp;
 
 	/*
 	 * timestamp shadows the actual context timing but it can
@@ -699,7 +683,6 @@ struct perf_event {
 
 #ifdef CONFIG_CGROUP_PERF
 	struct perf_cgroup		*cgrp; /* cgroup event is attach to */
-	int				cgrp_defer_enabled;
 #endif
 
 	struct list_head		sb_list;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 294f1927f944..e968b3eab9c7 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -582,6 +582,70 @@ static inline u64 perf_event_clock(struct perf_event *event)
 	return event->clock();
 }
 
+/*
+ * XXX comment about timekeeping goes here
+ */
+
+static __always_inline enum perf_event_state
+__perf_effective_state(struct perf_event *event)
+{
+	struct perf_event *leader = event->group_leader;
+
+	if (leader->state <= PERF_EVENT_STATE_OFF)
+		return leader->state;
+
+	return event->state;
+}
+
+static __always_inline void
+__perf_update_times(struct perf_event *event, u64 now, u64 *enabled, u64 *running)
+{
+	enum perf_event_state state = __perf_effective_state(event);
+	u64 delta = now - event->tstamp;
+
+	*enabled = event->total_time_enabled;
+	if (state >= PERF_EVENT_STATE_INACTIVE)
+		*enabled += delta;
+
+	*running = event->total_time_running;
+	if (state >= PERF_EVENT_STATE_ACTIVE)
+		*running += delta;
+}
+
+static void perf_event_update_time(struct perf_event *event)
+{
+	u64 now = perf_event_time(event);
+
+	__perf_update_times(event, now, &event->total_time_enabled,
+					&event->total_time_running);
+	event->tstamp = now;
+}
+
+static void perf_event_update_sibling_time(struct perf_event *leader)
+{
+	struct perf_event *sibling;
+
+	list_for_each_entry(sibling, &leader->sibling_list, group_entry)
+		perf_event_update_time(sibling);
+}
+
+static void
+perf_event_set_state(struct perf_event *event, enum perf_event_state state)
+{
+	if (event->state == state)
+		return;
+
+	perf_event_update_time(event);
+	/*
+	 * If a group leader gets enabled/disabled all its siblings
+	 * are affected too.
+	 */
+	if ((event->state < 0) ^ (state < 0))
+		perf_event_update_sibling_time(event);
+
+	WRITE_ONCE(event->state, state);
+}
+
 #ifdef CONFIG_CGROUP_PERF
 
 static inline bool
@@ -841,40 +905,6 @@ perf_cgroup_set_shadow_time(struct perf_event *event, u64 now)
 	event->shadow_ctx_time = now - t->timestamp;
 }
 
-static inline void
-perf_cgroup_defer_enabled(struct perf_event *event)
-{
-	/*
-	 * when the current task's perf cgroup does not match
-	 * the event's, we need to remember to call the
-	 * perf_mark_enable() function the first time a task with
-	 * a matching perf cgroup is scheduled in.
-	 */
-	if (is_cgroup_event(event) && !perf_cgroup_match(event))
-		event->cgrp_defer_enabled = 1;
-}
-
-static inline void
-perf_cgroup_mark_enabled(struct perf_event *event,
-			 struct perf_event_context *ctx)
-{
-	struct perf_event *sub;
-	u64 tstamp = perf_event_time(event);
-
-	if (!event->cgrp_defer_enabled)
-		return;
-
-	event->cgrp_defer_enabled = 0;
-
-	event->tstamp_enabled = tstamp - event->total_time_enabled;
-	list_for_each_entry(sub, &event->sibling_list, group_entry) {
-		if (sub->state >= PERF_EVENT_STATE_INACTIVE) {
-			sub->tstamp_enabled = tstamp - sub->total_time_enabled;
-			sub->cgrp_defer_enabled = 0;
-		}
-	}
-}
-
 /*
  * Update cpuctx->cgrp so that it is set when first cgroup event is added and
  * cleared when last cgroup event is removed.
@@ -973,17 +1003,6 @@ static inline u64 perf_cgroup_event_time(struct perf_event *event)
 }
 
 static inline void
-perf_cgroup_defer_enabled(struct perf_event *event)
-{
-}
-
-static inline void
-perf_cgroup_mark_enabled(struct perf_event *event,
-			 struct perf_event_context *ctx)
-{
-}
-
-static inline void
 list_update_cgroup_event(struct perf_event *event,
 			 struct perf_event_context *ctx, bool add)
 {
@@ -1396,60 +1415,6 @@ static u64 perf_event_time(struct perf_event *event)
 	return ctx ? ctx->time : 0;
 }
 
-/*
- * Update the total_time_enabled and total_time_running fields for a event.
- */
-static void update_event_times(struct perf_event *event)
-{
-	struct perf_event_context *ctx = event->ctx;
-	u64 run_end;
-
-	lockdep_assert_held(&ctx->lock);
-
-	if (event->state < PERF_EVENT_STATE_INACTIVE ||
-	    event->group_leader->state < PERF_EVENT_STATE_INACTIVE)
-		return;
-
-	/*
-	 * in cgroup mode, time_enabled represents
-	 * the time the event was enabled AND active
-	 * tasks were in the monitored cgroup. This is
-	 * independent of the activity of the context as
-	 * there may be a mix of cgroup and non-cgroup events.
-	 *
-	 * That is why we treat cgroup events differently
-	 * here.
-	 */
-	if (is_cgroup_event(event))
-		run_end = perf_cgroup_event_time(event);
-	else if (ctx->is_active)
-		run_end = ctx->time;
-	else
-		run_end = event->tstamp_stopped;
-
-	event->total_time_enabled = run_end - event->tstamp_enabled;
-
-	if (event->state == PERF_EVENT_STATE_INACTIVE)
-		run_end = event->tstamp_stopped;
-	else
-		run_end = perf_event_time(event);
-
-	event->total_time_running = run_end - event->tstamp_running;
-
-}
-
-/*
- * Update total_time_enabled and total_time_running for all events in a group.
- */
-static void update_group_times(struct perf_event *leader)
-{
-	struct perf_event *event;
-
-	update_event_times(leader);
-	list_for_each_entry(event, &leader->sibling_list, group_entry)
-		update_event_times(event);
-}
-
 static enum event_type_t get_event_type(struct perf_event *event)
 {
 	struct perf_event_context *ctx = event->ctx;
@@ -1492,6 +1457,8 @@ list_add_event(struct perf_event *event, struct perf_event_context *ctx)
 	WARN_ON_ONCE(event->attach_state & PERF_ATTACH_CONTEXT);
 	event->attach_state |= PERF_ATTACH_CONTEXT;
 
+	event->tstamp = perf_event_time(event);
+
 	/*
 	 * If we're a stand alone event or group leader, we go to the context
 	 * list, group events are kept attached to the group so that
@@ -1699,8 +1666,6 @@ list_del_event(struct perf_event *event, struct perf_event_context *ctx)
 	if (event->group_leader == event)
 		list_del_init(&event->group_entry);
 
-	update_group_times(event);
-
 	/*
 	 * If event was in error state, then keep it
 	 * that way, otherwise bogus counts will be
@@ -1709,7 +1674,7 @@ list_del_event(struct perf_event *event, struct perf_event_context *ctx)
 	 * of the event
 	 */
 	if (event->state > PERF_EVENT_STATE_OFF)
-		event->state = PERF_EVENT_STATE_OFF;
+		perf_event_set_state(event, PERF_EVENT_STATE_OFF);
 
 	ctx->generation++;
 }
@@ -1808,38 +1773,24 @@ event_sched_out(struct perf_event *event,
 		  struct perf_cpu_context *cpuctx,
 		  struct perf_event_context *ctx)
 {
-	u64 tstamp = perf_event_time(event);
-	u64 delta;
+	enum perf_event_state state = PERF_EVENT_STATE_INACTIVE;
 
 	WARN_ON_ONCE(event->ctx != ctx);
 	lockdep_assert_held(&ctx->lock);
 
-	/*
-	 * An event which could not be activated because of
-	 * filter mismatch still needs to have its timings
-	 * maintained, otherwise bogus information is return
-	 * via read() for time_enabled, time_running:
-	 */
-	if (event->state == PERF_EVENT_STATE_INACTIVE &&
-	    !event_filter_match(event)) {
-		delta = tstamp - event->tstamp_stopped;
-		event->tstamp_running += delta;
-		event->tstamp_stopped = tstamp;
-	}
-
 	if (event->state != PERF_EVENT_STATE_ACTIVE)
 		return;
 
 	perf_pmu_disable(event->pmu);
 
-	event->tstamp_stopped = tstamp;
 	event->pmu->del(event, 0);
 	event->oncpu = -1;
-	event->state = PERF_EVENT_STATE_INACTIVE;
+
 	if (event->pending_disable) {
 		event->pending_disable = 0;
-		event->state = PERF_EVENT_STATE_OFF;
+		state = PERF_EVENT_STATE_OFF;
 	}
+	perf_event_set_state(event, state);
 
 	if (!is_software_event(event))
 		cpuctx->active_oncpu--;
@@ -1859,7 +1810,9 @@ group_sched_out(struct perf_event *group_event,
 		struct perf_event_context *ctx)
 {
 	struct perf_event *event;
-	int state = group_event->state;
+
+	if (group_event->state != PERF_EVENT_STATE_ACTIVE)
+		return;
 
 	perf_pmu_disable(ctx->pmu);
 
@@ -1873,7 +1826,7 @@ group_sched_out(struct perf_event *group_event,
 
 	perf_pmu_enable(ctx->pmu);
 
-	if (state == PERF_EVENT_STATE_ACTIVE && group_event->attr.exclusive)
+	if (group_event->attr.exclusive)
 		cpuctx->exclusive = 0;
 }
 
@@ -1893,6 +1846,11 @@ __perf_remove_from_context(struct perf_event *event,
 {
 	unsigned long flags = (unsigned long)info;
 
+	if (ctx->is_active & EVENT_TIME) {
+		update_context_time(ctx);
+		update_cgrp_time_from_cpuctx(cpuctx);
+	}
+
 	event_sched_out(event, cpuctx, ctx);
 	if (flags & DETACH_GROUP)
 		perf_group_detach(event);
@@ -1955,14 +1913,17 @@ static void __perf_event_disable(struct perf_event *event,
 	if (event->state < PERF_EVENT_STATE_INACTIVE)
 		return;
 
-	update_context_time(ctx);
-	update_cgrp_time_from_event(event);
-	update_group_times(event);
+	if (ctx->is_active & EVENT_TIME) {
+		update_context_time(ctx);
+		update_cgrp_time_from_cpuctx(cpuctx);
+	}
+
 	if (event == event->group_leader)
 		group_sched_out(event, cpuctx, ctx);
 	else
 		event_sched_out(event, cpuctx, ctx);
-	event->state = PERF_EVENT_STATE_OFF;
+
+	perf_event_set_state(event, PERF_EVENT_STATE_OFF);
 }
 
 /*
@@ -2019,8 +1980,7 @@ void perf_event_disable_inatomic(struct perf_event *event)
 }
 
 static void perf_set_shadow_time(struct perf_event *event,
-				 struct perf_event_context *ctx,
-				 u64 tstamp)
+				 struct perf_event_context *ctx)
 {
 	/*
 	 * use the correct time source for the time snapshot
@@ -2048,9 +2008,9 @@ static void perf_set_shadow_time(struct perf_event *event,
 	 * is cleaner and simpler to understand.
 	 */
 	if (is_cgroup_event(event))
-		perf_cgroup_set_shadow_time(event, tstamp);
+		perf_cgroup_set_shadow_time(event, event->tstamp);
 	else
-		event->shadow_ctx_time = tstamp - ctx->timestamp;
+		event->shadow_ctx_time = event->tstamp - ctx->timestamp;
 }
 
 #define MAX_INTERRUPTS (~0ULL)
@@ -2063,7 +2023,6 @@ event_sched_in(struct perf_event *event,
 		 struct perf_cpu_context *cpuctx,
 		 struct perf_event_context *ctx)
 {
-	u64 tstamp = perf_event_time(event);
 	int ret = 0;
 
 	lockdep_assert_held(&ctx->lock);
@@ -2077,7 +2036,7 @@ event_sched_in(struct perf_event *event,
 	 * is visible.
 	 */
 	smp_wmb();
-	WRITE_ONCE(event->state, PERF_EVENT_STATE_ACTIVE);
+	perf_event_set_state(event, PERF_EVENT_STATE_ACTIVE);
 
 	/*
 	 * Unthrottle events, since we scheduled we might have missed several
@@ -2089,26 +2048,19 @@ event_sched_in(struct perf_event *event,
 		event->hw.interrupts = 0;
 	}
 
-	/*
-	 * The new state must be visible before we turn it on in the hardware:
-	 */
-	smp_wmb();
-
 	perf_pmu_disable(event->pmu);
 
-	perf_set_shadow_time(event, ctx, tstamp);
+	perf_set_shadow_time(event, ctx);
 
 	perf_log_itrace_start(event);
 
 	if (event->pmu->add(event, PERF_EF_START)) {
-		event->state = PERF_EVENT_STATE_INACTIVE;
+		perf_event_set_state(event, PERF_EVENT_STATE_INACTIVE);
 		event->oncpu = -1;
 		ret = -EAGAIN;
 		goto out;
 	}
 
-	event->tstamp_running += tstamp - event->tstamp_stopped;
-
 	if (!is_software_event(event))
 		cpuctx->active_oncpu++;
 	if (!ctx->nr_active++)
@@ -2132,8 +2084,6 @@ group_sched_in(struct perf_event *group_event,
 {
 	struct perf_event *event, *partial_group = NULL;
 	struct pmu *pmu = ctx->pmu;
-	u64 now = ctx->time;
-	bool simulate = false;
 
 	if (group_event->state == PERF_EVENT_STATE_OFF)
 		return 0;
@@ -2163,27 +2113,13 @@ group_sched_in(struct perf_event *group_event,
 	/*
 	 * Groups can be scheduled in as one unit only, so undo any
 	 * partial group before returning:
-	 * The events up to the failed event are scheduled out normally,
-	 * tstamp_stopped will be updated.
-	 *
-	 * The failed events and the remaining siblings need to have
-	 * their timings updated as if they had gone thru event_sched_in()
-	 * and event_sched_out(). This is required to get consistent timings
-	 * across the group. This also takes care of the case where the group
-	 * could never be scheduled by ensuring tstamp_stopped is set to mark
-	 * the time the event was actually stopped, such that time delta
-	 * calculation in update_event_times() is correct.
+	 * The events up to the failed event are scheduled out normally.
 	 */
 	list_for_each_entry(event, &group_event->sibling_list, group_entry) {
 		if (event == partial_group)
-			simulate = true;
+			break;
 
-		if (simulate) {
-			event->tstamp_running += now - event->tstamp_stopped;
-			event->tstamp_stopped = now;
-		} else {
-			event_sched_out(event, cpuctx, ctx);
-		}
+		event_sched_out(event, cpuctx, ctx);
 	}
 	event_sched_out(group_event, cpuctx, ctx);
 
@@ -2225,46 +2161,11 @@ static int group_can_go_on(struct perf_event *event,
 	return can_add_hw;
 }
 
-/*
- * Complement to update_event_times(). This computes the tstamp_* values to
- * continue 'enabled' state from @now, and effectively discards the time
- * between the prior tstamp_stopped and now (as we were in the OFF state, or
- * just switched (context) time base).
- *
- * This further assumes '@event->state == INACTIVE' (we just came from OFF) and
- * cannot have been scheduled in yet. And going into INACTIVE state means
- * '@event->tstamp_stopped = @now'.
- *
- * Thus given the rules of update_event_times():
- *
- *   total_time_enabled = tstamp_stopped - tstamp_enabled
- *   total_time_running = tstamp_stopped - tstamp_running
- *
- * We can insert 'tstamp_stopped == now' and reverse them to compute new
- * tstamp_* values.
- */
-static void __perf_event_enable_time(struct perf_event *event, u64 now)
-{
-	WARN_ON_ONCE(event->state != PERF_EVENT_STATE_INACTIVE);
-
-	event->tstamp_stopped = now;
-	event->tstamp_enabled = now - event->total_time_enabled;
-	event->tstamp_running = now - event->total_time_running;
-}
-
 static void add_event_to_ctx(struct perf_event *event,
 			       struct perf_event_context *ctx)
 {
-	u64 tstamp = perf_event_time(event);
-
 	list_add_event(event, ctx);
 	perf_group_attach(event);
-	/*
-	 * We can be called with event->state == STATE_OFF when we create with
-	 * .disabled = 1. In that case the IOC_ENABLE will call this function.
-	 */
-	if (event->state == PERF_EVENT_STATE_INACTIVE)
-		__perf_event_enable_time(event, tstamp);
 }
 
 static void ctx_sched_out(struct perf_event_context *ctx,
@@ -2496,28 +2397,6 @@ perf_install_in_context(struct perf_event_context *ctx,
 }
 
 /*
- * Put a event into inactive state and update time fields.
- * Enabling the leader of a group effectively enables all
- * the group members that aren't explicitly disabled, so we
- * have to update their ->tstamp_enabled also.
- * Note: this works for group members as well as group leaders
- * since the non-leader members' sibling_lists will be empty.
- */
-static void __perf_event_mark_enabled(struct perf_event *event)
-{
-	struct perf_event *sub;
-	u64 tstamp = perf_event_time(event);
-
-	event->state = PERF_EVENT_STATE_INACTIVE;
-	__perf_event_enable_time(event, tstamp);
-	list_for_each_entry(sub, &event->sibling_list, group_entry) {
-		/* XXX should not be > INACTIVE if event isn't */
-		if (sub->state >= PERF_EVENT_STATE_INACTIVE)
-			__perf_event_enable_time(sub, tstamp);
-	}
-}
-
-/*
  * Cross CPU call to enable a performance event
  */
 static void __perf_event_enable(struct perf_event *event,
@@ -2535,14 +2414,12 @@ static void __perf_event_enable(struct perf_event *event,
 	if (ctx->is_active)
 		ctx_sched_out(ctx, cpuctx, EVENT_TIME);
 
-	__perf_event_mark_enabled(event);
+	perf_event_set_state(event, PERF_EVENT_STATE_INACTIVE);
 
 	if (!ctx->is_active)
 		return;
 
 	if (!event_filter_match(event)) {
-		if (is_cgroup_event(event))
-			perf_cgroup_defer_enabled(event);
 		ctx_sched_in(ctx, cpuctx, EVENT_TIME, current);
 		return;
 	}
@@ -2862,18 +2739,10 @@ static void __perf_event_sync_stat(struct perf_event *event,
 	 * we know the event must be on the current CPU, therefore we
 	 * don't need to use it.
 	 */
-	switch (event->state) {
-	case PERF_EVENT_STATE_ACTIVE:
+	if (event->state == PERF_EVENT_STATE_ACTIVE)
 		event->pmu->read(event);
-		/* fall-through */
 
-	case PERF_EVENT_STATE_INACTIVE:
-		update_event_times(event);
-		break;
-
-	default:
-		break;
-	}
+	perf_event_update_time(event);
 
 	/*
 	 * In order to keep per-task stats reliable we need to flip the event
@@ -3110,10 +2979,6 @@ ctx_pinned_sched_in(struct perf_event_context *ctx,
 		if (!event_filter_match(event))
 			continue;
 
-		/* may need to reset tstamp_enabled */
-		if (is_cgroup_event(event))
-			perf_cgroup_mark_enabled(event, ctx);
-
 		if (group_can_go_on(event, cpuctx, 1))
 			group_sched_in(event, cpuctx, ctx);
 
@@ -3121,10 +2986,8 @@ ctx_pinned_sched_in(struct perf_event_context *ctx,
 		 * If this pinned group hasn't been scheduled,
 		 * put it in error state.
 		 */
-		if (event->state == PERF_EVENT_STATE_INACTIVE) {
-			update_group_times(event);
-			event->state = PERF_EVENT_STATE_ERROR;
-		}
+		if (event->state == PERF_EVENT_STATE_INACTIVE)
+			perf_event_set_state(event, PERF_EVENT_STATE_ERROR);
 	}
 }
 
@@ -3146,10 +3009,6 @@ ctx_flexible_sched_in(struct perf_event_context *ctx,
 		if (!event_filter_match(event))
 			continue;
 
-		/* may need to reset tstamp_enabled */
-		if (is_cgroup_event(event))
-			perf_cgroup_mark_enabled(event, ctx);
-
 		if (group_can_go_on(event, cpuctx, can_add_hw)) {
 			if (group_sched_in(event, cpuctx, ctx))
 				can_add_hw = 0;
@@ -3541,7 +3400,7 @@ static int event_enable_on_exec(struct perf_event *event,
 	if (event->state >= PERF_EVENT_STATE_INACTIVE)
 		return 0;
 
-	__perf_event_mark_enabled(event);
+	perf_event_set_state(event, PERF_EVENT_STATE_INACTIVE);
 
 	return 1;
 }
@@ -3590,12 +3449,6 @@ static void perf_event_enable_on_exec(int ctxn)
 		put_ctx(clone_ctx);
 }
 
-struct perf_read_data {
-	struct perf_event *event;
-	bool group;
-	int ret;
-};
-
 static int __perf_event_read_cpu(struct perf_event *event, int event_cpu)
 {
 	u16 local_pkg, event_pkg;
@@ -3613,64 +3466,6 @@ static int __perf_event_read_cpu(struct perf_event *event, int event_cpu)
 	return event_cpu;
 }
 
-/*
- * Cross CPU call to read the hardware event
- */
-static void __perf_event_read(void *info)
-{
-	struct perf_read_data *data = info;
-	struct perf_event *sub, *event = data->event;
-	struct perf_event_context *ctx = event->ctx;
-	struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
-	struct pmu *pmu = event->pmu;
-
-	/*
-	 * If this is a task context, we need to check whether it is
-	 * the current task context of this cpu.  If not it has been
-	 * scheduled out before the smp call arrived.  In that case
-	 * event->count would have been updated to a recent sample
-	 * when the event was scheduled out.
-	 */
-	if (ctx->task && cpuctx->task_ctx != ctx)
-		return;
-
-	raw_spin_lock(&ctx->lock);
-	if (ctx->is_active) {
-		update_context_time(ctx);
-		update_cgrp_time_from_event(event);
-	}
-
-	update_event_times(event);
-	if (event->state != PERF_EVENT_STATE_ACTIVE)
-		goto unlock;
-
-	if (!data->group) {
-		pmu->read(event);
-		data->ret = 0;
-		goto unlock;
-	}
-
-	pmu->start_txn(pmu, PERF_PMU_TXN_READ);
-
-	pmu->read(event);
-
-	list_for_each_entry(sub, &event->sibling_list, group_entry) {
-		update_event_times(sub);
-		if (sub->state == PERF_EVENT_STATE_ACTIVE) {
-			/*
-			 * Use sibling's PMU rather than @event's since
-			 * sibling could be on different (eg: software) PMU.
-			 */
-			sub->pmu->read(sub);
-		}
-	}
-
-	data->ret = pmu->commit_txn(pmu);
-
-unlock:
-	raw_spin_unlock(&ctx->lock);
-}
-
 static inline u64 perf_event_count(struct perf_event *event)
 {
 	return local64_read(&event->count) + atomic64_read(&event->child_count);
@@ -3733,63 +3528,81 @@ int perf_event_read_local(struct perf_event *event, u64 *value)
 	return ret;
 }
 
-static int perf_event_read(struct perf_event *event, bool group)
+struct perf_read_data {
+	struct perf_event *event;
+	bool group;
+	int ret;
+};
+
+static void __perf_event_read(struct perf_event *event,
+			      struct perf_cpu_context *cpuctx,
+			      struct perf_event_context *ctx,
+			      void *data)
 {
-	int event_cpu, ret = 0;
+	struct perf_read_data *prd = data;
+	struct pmu *pmu = event->pmu;
+	struct perf_event *sibling;
 
-	/*
-	 * If event is enabled and currently active on a CPU, update the
-	 * value in the event structure:
-	 */
-	if (event->state == PERF_EVENT_STATE_ACTIVE) {
-		struct perf_read_data data = {
-			.event = event,
-			.group = group,
-			.ret = 0,
-		};
+	if (ctx->is_active & EVENT_TIME) {
+		update_context_time(ctx);
+		update_cgrp_time_from_cpuctx(cpuctx);
+	}
 
-		event_cpu = READ_ONCE(event->oncpu);
-		if ((unsigned)event_cpu >= nr_cpu_ids)
-			return 0;
+	perf_event_update_time(event);
+	if (prd->group)
+		perf_event_update_sibling_time(event);
 
-		preempt_disable();
-		event_cpu = __perf_event_read_cpu(event, event_cpu);
+	if (event->state != PERF_EVENT_STATE_ACTIVE)
+		return;
 
+	if (!prd->group) {
+		pmu->read(event);
+		prd->ret = 0;
+		return;
+	}
+
+	pmu->start_txn(pmu, PERF_PMU_TXN_READ);
+
+	pmu->read(event);
+	list_for_each_entry(sibling, &event->sibling_list, group_entry) {
+		if (sibling->state == PERF_EVENT_STATE_ACTIVE) {
+			/*
+			 * Use sibling's PMU rather than @event's since
+			 * sibling could be on different (eg: software) PMU.
+			 */
+			sibling->pmu->read(sibling);
+		}
+	}
+
+	prd->ret = pmu->commit_txn(pmu);
+}
+
+static int perf_event_read(struct perf_event *event, bool group)
+{
+	struct perf_read_data prd = {
+		.event = event,
+		.group = group,
+		.ret = 0,
+	};
+
+	if (event->ctx->task) {
+		event_function_call(event, __perf_event_read, &prd);
+	} else {
 		/*
-		 * Purposely ignore the smp_call_function_single() return
-		 * value.
-		 *
-		 * If event_cpu isn't a valid CPU it means the event got
-		 * scheduled out and that will have updated the event count.
-		 *
-		 * Therefore, either way, we'll have an up-to-date event count
-		 * after this.
-		 */
-		(void)smp_call_function_single(event_cpu, __perf_event_read, &data, 1);
-		preempt_enable();
-		ret = data.ret;
-	} else if (event->state == PERF_EVENT_STATE_INACTIVE) {
-		struct perf_event_context *ctx = event->ctx;
-		unsigned long flags;
-
-		raw_spin_lock_irqsave(&ctx->lock, flags);
-		/*
-		 * may read while context is not active
-		 * (e.g., thread is blocked), in that case
-		 * we cannot update context time
+		 * For uncore events (which are per definition per-cpu)
+		 * allow a different read CPU from event->cpu.
 		 */
-		if (ctx->is_active) {
-			update_context_time(ctx);
-			update_cgrp_time_from_event(event);
-		}
-		if (group)
-			update_group_times(event);
-		else
-			update_event_times(event);
-		raw_spin_unlock_irqrestore(&ctx->lock, flags);
+		struct event_function_struct efs = {
+			.event = event,
+			.func = __perf_event_read,
+			.data = &prd,
+		};
+		int cpu = __perf_event_read_cpu(event, event->cpu);
+
+		cpu_function_call(cpu, event_function, &efs);
 	}
 
-	return ret;
+	return prd.ret;
 }
 
 /*
@@ -4388,7 +4201,7 @@ static int perf_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
-u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running)
+static u64 __perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running)
 {
 	struct perf_event *child;
 	u64 total = 0;
@@ -4416,6 +4229,18 @@ u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running)
 
 	return total;
 }
+
+u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running)
+{
+	struct perf_event_context *ctx;
+	u64 count;
+
+	ctx = perf_event_ctx_lock(event);
+	count = __perf_event_read_value(event, enabled, running);
+	perf_event_ctx_unlock(event, ctx);
+
+	return count;
+}
 EXPORT_SYMBOL_GPL(perf_event_read_value);
 
 static int __perf_read_group_add(struct perf_event *leader,
@@ -4431,6 +4256,8 @@ static int __perf_read_group_add(struct perf_event *leader,
 	if (ret)
 		return ret;
 
+	raw_spin_lock_irqsave(&ctx->lock, flags);
+
 	/*
 	 * Since we co-schedule groups, {enabled,running} times of siblings
 	 * will be identical to those of the leader, so we only publish one
@@ -4453,8 +4280,6 @@ static int __perf_read_group_add(struct perf_event *leader,
 	if (read_format & PERF_FORMAT_ID)
 		values[n++] = primary_event_id(leader);
 
-	raw_spin_lock_irqsave(&ctx->lock, flags);
-
 	list_for_each_entry(sub, &leader->sibling_list, group_entry) {
 		values[n++] += perf_event_count(sub);
 		if (read_format & PERF_FORMAT_ID)
@@ -4518,7 +4343,7 @@ static int perf_read_one(struct perf_event *event,
 	u64 values[4];
 	int n = 0;
 
-	values[n++] = perf_event_read_value(event, &enabled, &running);
+	values[n++] = __perf_event_read_value(event, &enabled, &running);
 	if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
 		values[n++] = enabled;
 	if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)
@@ -4897,8 +4722,7 @@ static void calc_timer_values(struct perf_event *event,
 
 	*now = perf_clock();
 	ctx_time = event->shadow_ctx_time + *now;
-	*enabled = ctx_time - event->tstamp_enabled;
-	*running = ctx_time - event->tstamp_running;
+	__perf_update_times(event, ctx_time, enabled, running);
 }
 
 static void perf_event_init_userpage(struct perf_event *event)
@@ -10516,7 +10340,7 @@ perf_event_exit_event(struct perf_event *child_event,
 	if (parent_event)
 		perf_group_detach(child_event);
 	list_del_event(child_event, child_ctx);
-	child_event->state = PERF_EVENT_STATE_EXIT; /* is_event_hup() */
+	perf_event_set_state(child_event, PERF_EVENT_STATE_EXIT); /* is_event_hup() */
 	raw_spin_unlock_irq(&child_ctx->lock);
 
 	/*
@@ -10754,7 +10578,7 @@ inherit_event(struct perf_event *parent_event,
 	      struct perf_event *group_leader,
 	      struct perf_event_context *child_ctx)
 {
-	enum perf_event_active_state parent_state = parent_event->state;
+	enum perf_event_state parent_state = parent_event->state;
 	struct perf_event *child_event;
 	unsigned long flags;
 
@@ -11090,6 +10914,7 @@ static void __perf_event_exit_context(void *__info)
 	struct perf_event *event;
 
 	raw_spin_lock(&ctx->lock);
+	ctx_sched_out(ctx, cpuctx, EVENT_TIME);
 	list_for_each_entry(event, &ctx->event_list, event_entry)
 		__perf_remove_from_context(event, cpuctx, ctx, (void *)DETACH_GROUP);
 	raw_spin_unlock(&ctx->lock);

[toc] | [next] | [standalone]

#1724964

On 31.08.2017 20:18, Peter Zijlstra wrote:
> On Wed, Aug 23, 2017 at 11:54:15AM +0300, Alexey Budankov wrote:
>> On 22.08.2017 23:47, Peter Zijlstra wrote:
>>> On Thu, Aug 10, 2017 at 06:57:43PM +0300, Alexey Budankov wrote:
>>>> The key thing in the patch is explicit updating of tstamp fields for
>>>> INACTIVE events in update_event_times().
>>>
>>>> @@ -1405,6 +1426,9 @@ static void update_event_times(struct perf_event *event)
>>>>  	    event->group_leader->state < PERF_EVENT_STATE_INACTIVE)
>>>>  		return;
>>>>  
>>>> +	if (event->state == PERF_EVENT_STATE_INACTIVE)
>>>> +		perf_event_tstamp_update(event);
>>>> +
>>>>  	/*
>>>>  	 * in cgroup mode, time_enabled represents
>>>>  	 * the time the event was enabled AND active
>>>
>>> But why!? I thought the whole point was to not need to do this.
>>
>> update_event_times() is not called from timer interrupt handler 
>> thus it is not on the critical path which is optimized in this patch set.
>>
>> But update_event_times() is called in the context of read() syscall so
>> this is the place where we may update event times for INACTIVE events 
>> instead of timer interrupt.
>>
>> Also update_event_times() is called on thread context switch out so
>> we get event times also updated when the thread migrates to other CPU.
>>
>>>
>>> The thing I outlined earlier would only need to update timestamps when
>>> events change state and at no other point in time.
>>
>> But we still may request times while event is in INACTIVE state 
>> thru read() syscall and event timings need to be up-to-date. 
> 
> Sure, read() also updates.
> 
> So the below completely rewrites timekeeping (and probably breaks
> world) but does away with the need to touch events that don't get
> scheduled.
> 
> Esp the cgroup stuff is entirely untested since I simply don't know how
> to operate that. I did run Vince's tests on it, and I think it doesn't
> regress, but I'm near a migraine so I can't really see straight atm.
> 
> Vince, Stephane, could you guys have a peek?
> 
> (There's a few other bits in, I'll break up into patches and write
> comments and Changelogs later, I think its can be split in some 5
> patches).
> 
> The basic idea is really simple, we have a single timestamp and
> depending on the state we update enabled/running. This obviously only
> requires updates when we change state and when we need up-to-date
> timestamps (read).
> 
> No more weird and wonderful mind bending interaction between 3 different
> timestamps with arcane update rules.

Well, this looks like an "opposite" approach to event timekeeping in 
comparison to what we currently have. 

Do you want this rework before or after the current patch set?

> 
> ---
>  include/linux/perf_event.h |  25 +-
>  kernel/events/core.c       | 551 ++++++++++++++++-----------------------------
>  2 files changed, 192 insertions(+), 384 deletions(-)
> 
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 8e22f24ded6a..2a6ae48a1a96 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -485,9 +485,9 @@ struct perf_addr_filters_head {
>  };
>  
>  /**
> - * enum perf_event_active_state - the states of a event
> + * enum perf_event_state - the states of a event
>   */
> -enum perf_event_active_state {
> +enum perf_event_state {
>  	PERF_EVENT_STATE_DEAD		= -4,
>  	PERF_EVENT_STATE_EXIT		= -3,
>  	PERF_EVENT_STATE_ERROR		= -2,
> @@ -578,7 +578,7 @@ struct perf_event {
>  	struct pmu			*pmu;
>  	void				*pmu_private;
>  
> -	enum perf_event_active_state	state;
> +	enum perf_event_state		state;
>  	unsigned int			attach_state;
>  	local64_t			count;
>  	atomic64_t			child_count;
> @@ -588,26 +588,10 @@ struct perf_event {
>  	 * has been enabled (i.e. eligible to run, and the task has
>  	 * been scheduled in, if this is a per-task event)
>  	 * and running (scheduled onto the CPU), respectively.
> -	 *
> -	 * They are computed from tstamp_enabled, tstamp_running and
> -	 * tstamp_stopped when the event is in INACTIVE or ACTIVE state.
>  	 */
>  	u64				total_time_enabled;
>  	u64				total_time_running;
> -
> -	/*
> -	 * These are timestamps used for computing total_time_enabled
> -	 * and total_time_running when the event is in INACTIVE or
> -	 * ACTIVE state, measured in nanoseconds from an arbitrary point
> -	 * in time.
> -	 * tstamp_enabled: the notional time when the event was enabled
> -	 * tstamp_running: the notional time when the event was scheduled on
> -	 * tstamp_stopped: in INACTIVE state, the notional time when the
> -	 *	event was scheduled off.
> -	 */
> -	u64				tstamp_enabled;
> -	u64				tstamp_running;
> -	u64				tstamp_stopped;
> +	u64				tstamp;
>  
>  	/*
>  	 * timestamp shadows the actual context timing but it can
> @@ -699,7 +683,6 @@ struct perf_event {
>  
>  #ifdef CONFIG_CGROUP_PERF
>  	struct perf_cgroup		*cgrp; /* cgroup event is attach to */
> -	int				cgrp_defer_enabled;
>  #endif
>  
>  	struct list_head		sb_list;
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 294f1927f944..e968b3eab9c7 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -582,6 +582,70 @@ static inline u64 perf_event_clock(struct perf_event *event)
>  	return event->clock();
>  }
>  
> +/*
> + * XXX comment about timekeeping goes here
> + */
> +
> +static __always_inline enum perf_event_state
> +__perf_effective_state(struct perf_event *event)
> +{
> +	struct perf_event *leader = event->group_leader;
> +
> +	if (leader->state <= PERF_EVENT_STATE_OFF)
> +		return leader->state;
> +
> +	return event->state;
> +}
> +
> +static __always_inline void
> +__perf_update_times(struct perf_event *event, u64 now, u64 *enabled, u64 *running)
> +{
> +	enum perf_event_state state = __perf_effective_state(event);
> +	u64 delta = now - event->tstamp;
> +
> +	*enabled = event->total_time_enabled;
> +	if (state >= PERF_EVENT_STATE_INACTIVE)
> +		*enabled += delta;
> +
> +	*running = event->total_time_running;
> +	if (state >= PERF_EVENT_STATE_ACTIVE)
> +		*running += delta;
> +}
> +
> +static void perf_event_update_time(struct perf_event *event)
> +{
> +	u64 now = perf_event_time(event);
> +
> +	__perf_update_times(event, now, &event->total_time_enabled,
> +					&event->total_time_running);
> +	event->tstamp = now;
> +}
> +
> +static void perf_event_update_sibling_time(struct perf_event *leader)
> +{
> +	struct perf_event *sibling;
> +
> +	list_for_each_entry(sibling, &leader->sibling_list, group_entry)
> +		perf_event_update_time(sibling);
> +}
> +
> +static void
> +perf_event_set_state(struct perf_event *event, enum perf_event_state state)
> +{
> +	if (event->state == state)
> +		return;
> +
> +	perf_event_update_time(event);
> +	/*
> +	 * If a group leader gets enabled/disabled all its siblings
> +	 * are affected too.
> +	 */
> +	if ((event->state < 0) ^ (state < 0))
> +		perf_event_update_sibling_time(event);
> +
> +	WRITE_ONCE(event->state, state);
> +}
> +
>  #ifdef CONFIG_CGROUP_PERF
>  
>  static inline bool
> @@ -841,40 +905,6 @@ perf_cgroup_set_shadow_time(struct perf_event *event, u64 now)
>  	event->shadow_ctx_time = now - t->timestamp;
>  }
>  
> -static inline void
> -perf_cgroup_defer_enabled(struct perf_event *event)
> -{
> -	/*
> -	 * when the current task's perf cgroup does not match
> -	 * the event's, we need to remember to call the
> -	 * perf_mark_enable() function the first time a task with
> -	 * a matching perf cgroup is scheduled in.
> -	 */
> -	if (is_cgroup_event(event) && !perf_cgroup_match(event))
> -		event->cgrp_defer_enabled = 1;
> -}
> -
> -static inline void
> -perf_cgroup_mark_enabled(struct perf_event *event,
> -			 struct perf_event_context *ctx)
> -{
> -	struct perf_event *sub;
> -	u64 tstamp = perf_event_time(event);
> -
> -	if (!event->cgrp_defer_enabled)
> -		return;
> -
> -	event->cgrp_defer_enabled = 0;
> -
> -	event->tstamp_enabled = tstamp - event->total_time_enabled;
> -	list_for_each_entry(sub, &event->sibling_list, group_entry) {
> -		if (sub->state >= PERF_EVENT_STATE_INACTIVE) {
> -			sub->tstamp_enabled = tstamp - sub->total_time_enabled;
> -			sub->cgrp_defer_enabled = 0;
> -		}
> -	}
> -}
> -
>  /*
>   * Update cpuctx->cgrp so that it is set when first cgroup event is added and
>   * cleared when last cgroup event is removed.
> @@ -973,17 +1003,6 @@ static inline u64 perf_cgroup_event_time(struct perf_event *event)
>  }
>  
>  static inline void
> -perf_cgroup_defer_enabled(struct perf_event *event)
> -{
> -}
> -
> -static inline void
> -perf_cgroup_mark_enabled(struct perf_event *event,
> -			 struct perf_event_context *ctx)
> -{
> -}
> -
> -static inline void
>  list_update_cgroup_event(struct perf_event *event,
>  			 struct perf_event_context *ctx, bool add)
>  {
> @@ -1396,60 +1415,6 @@ static u64 perf_event_time(struct perf_event *event)
>  	return ctx ? ctx->time : 0;
>  }
>  
> -/*
> - * Update the total_time_enabled and total_time_running fields for a event.
> - */
> -static void update_event_times(struct perf_event *event)
> -{
> -	struct perf_event_context *ctx = event->ctx;
> -	u64 run_end;
> -
> -	lockdep_assert_held(&ctx->lock);
> -
> -	if (event->state < PERF_EVENT_STATE_INACTIVE ||
> -	    event->group_leader->state < PERF_EVENT_STATE_INACTIVE)
> -		return;
> -
> -	/*
> -	 * in cgroup mode, time_enabled represents
> -	 * the time the event was enabled AND active
> -	 * tasks were in the monitored cgroup. This is
> -	 * independent of the activity of the context as
> -	 * there may be a mix of cgroup and non-cgroup events.
> -	 *
> -	 * That is why we treat cgroup events differently
> -	 * here.
> -	 */
> -	if (is_cgroup_event(event))
> -		run_end = perf_cgroup_event_time(event);
> -	else if (ctx->is_active)
> -		run_end = ctx->time;
> -	else
> -		run_end = event->tstamp_stopped;
> -
> -	event->total_time_enabled = run_end - event->tstamp_enabled;
> -
> -	if (event->state == PERF_EVENT_STATE_INACTIVE)
> -		run_end = event->tstamp_stopped;
> -	else
> -		run_end = perf_event_time(event);
> -
> -	event->total_time_running = run_end - event->tstamp_running;
> -
> -}
> -
> -/*
> - * Update total_time_enabled and total_time_running for all events in a group.
> - */
> -static void update_group_times(struct perf_event *leader)
> -{
> -	struct perf_event *event;
> -
> -	update_event_times(leader);
> -	list_for_each_entry(event, &leader->sibling_list, group_entry)
> -		update_event_times(event);
> -}
> -
>  static enum event_type_t get_event_type(struct perf_event *event)
>  {
>  	struct perf_event_context *ctx = event->ctx;
> @@ -1492,6 +1457,8 @@ list_add_event(struct perf_event *event, struct perf_event_context *ctx)
>  	WARN_ON_ONCE(event->attach_state & PERF_ATTACH_CONTEXT);
>  	event->attach_state |= PERF_ATTACH_CONTEXT;
>  
> +	event->tstamp = perf_event_time(event);
> +
>  	/*
>  	 * If we're a stand alone event or group leader, we go to the context
>  	 * list, group events are kept attached to the group so that
> @@ -1699,8 +1666,6 @@ list_del_event(struct perf_event *event, struct perf_event_context *ctx)
>  	if (event->group_leader == event)
>  		list_del_init(&event->group_entry);
>  
> -	update_group_times(event);
> -
>  	/*
>  	 * If event was in error state, then keep it
>  	 * that way, otherwise bogus counts will be
> @@ -1709,7 +1674,7 @@ list_del_event(struct perf_event *event, struct perf_event_context *ctx)
>  	 * of the event
>  	 */
>  	if (event->state > PERF_EVENT_STATE_OFF)
> -		event->state = PERF_EVENT_STATE_OFF;
> +		perf_event_set_state(event, PERF_EVENT_STATE_OFF);
>  
>  	ctx->generation++;
>  }
> @@ -1808,38 +1773,24 @@ event_sched_out(struct perf_event *event,
>  		  struct perf_cpu_context *cpuctx,
>  		  struct perf_event_context *ctx)
>  {
> -	u64 tstamp = perf_event_time(event);
> -	u64 delta;
> +	enum perf_event_state state = PERF_EVENT_STATE_INACTIVE;
>  
>  	WARN_ON_ONCE(event->ctx != ctx);
>  	lockdep_assert_held(&ctx->lock);
>  
> -	/*
> -	 * An event which could not be activated because of
> -	 * filter mismatch still needs to have its timings
> -	 * maintained, otherwise bogus information is return
> -	 * via read() for time_enabled, time_running:
> -	 */
> -	if (event->state == PERF_EVENT_STATE_INACTIVE &&
> -	    !event_filter_match(event)) {
> -		delta = tstamp - event->tstamp_stopped;
> -		event->tstamp_running += delta;
> -		event->tstamp_stopped = tstamp;
> -	}
> -
>  	if (event->state != PERF_EVENT_STATE_ACTIVE)
>  		return;
>  
>  	perf_pmu_disable(event->pmu);
>  
> -	event->tstamp_stopped = tstamp;
>  	event->pmu->del(event, 0);
>  	event->oncpu = -1;
> -	event->state = PERF_EVENT_STATE_INACTIVE;
> +
>  	if (event->pending_disable) {
>  		event->pending_disable = 0;
> -		event->state = PERF_EVENT_STATE_OFF;
> +		state = PERF_EVENT_STATE_OFF;
>  	}
> +	perf_event_set_state(event, state);
>  
>  	if (!is_software_event(event))
>  		cpuctx->active_oncpu--;
> @@ -1859,7 +1810,9 @@ group_sched_out(struct perf_event *group_event,
>  		struct perf_event_context *ctx)
>  {
>  	struct perf_event *event;
> -	int state = group_event->state;
> +
> +	if (group_event->state != PERF_EVENT_STATE_ACTIVE)
> +		return;
>  
>  	perf_pmu_disable(ctx->pmu);
>  
> @@ -1873,7 +1826,7 @@ group_sched_out(struct perf_event *group_event,
>  
>  	perf_pmu_enable(ctx->pmu);
>  
> -	if (state == PERF_EVENT_STATE_ACTIVE && group_event->attr.exclusive)
> +	if (group_event->attr.exclusive)
>  		cpuctx->exclusive = 0;
>  }
>  
> @@ -1893,6 +1846,11 @@ __perf_remove_from_context(struct perf_event *event,
>  {
>  	unsigned long flags = (unsigned long)info;
>  
> +	if (ctx->is_active & EVENT_TIME) {
> +		update_context_time(ctx);
> +		update_cgrp_time_from_cpuctx(cpuctx);
> +	}
> +
>  	event_sched_out(event, cpuctx, ctx);
>  	if (flags & DETACH_GROUP)
>  		perf_group_detach(event);
> @@ -1955,14 +1913,17 @@ static void __perf_event_disable(struct perf_event *event,
>  	if (event->state < PERF_EVENT_STATE_INACTIVE)
>  		return;
>  
> -	update_context_time(ctx);
> -	update_cgrp_time_from_event(event);
> -	update_group_times(event);
> +	if (ctx->is_active & EVENT_TIME) {
> +		update_context_time(ctx);
> +		update_cgrp_time_from_cpuctx(cpuctx);
> +	}
> +
>  	if (event == event->group_leader)
>  		group_sched_out(event, cpuctx, ctx);
>  	else
>  		event_sched_out(event, cpuctx, ctx);
> -	event->state = PERF_EVENT_STATE_OFF;
> +
> +	perf_event_set_state(event, PERF_EVENT_STATE_OFF);
>  }
>  
>  /*
> @@ -2019,8 +1980,7 @@ void perf_event_disable_inatomic(struct perf_event *event)
>  }
>  
>  static void perf_set_shadow_time(struct perf_event *event,
> -				 struct perf_event_context *ctx,
> -				 u64 tstamp)
> +				 struct perf_event_context *ctx)
>  {
>  	/*
>  	 * use the correct time source for the time snapshot
> @@ -2048,9 +2008,9 @@ static void perf_set_shadow_time(struct perf_event *event,
>  	 * is cleaner and simpler to understand.
>  	 */
>  	if (is_cgroup_event(event))
> -		perf_cgroup_set_shadow_time(event, tstamp);
> +		perf_cgroup_set_shadow_time(event, event->tstamp);
>  	else
> -		event->shadow_ctx_time = tstamp - ctx->timestamp;
> +		event->shadow_ctx_time = event->tstamp - ctx->timestamp;
>  }
>  
>  #define MAX_INTERRUPTS (~0ULL)
> @@ -2063,7 +2023,6 @@ event_sched_in(struct perf_event *event,
>  		 struct perf_cpu_context *cpuctx,
>  		 struct perf_event_context *ctx)
>  {
> -	u64 tstamp = perf_event_time(event);
>  	int ret = 0;
>  
>  	lockdep_assert_held(&ctx->lock);
> @@ -2077,7 +2036,7 @@ event_sched_in(struct perf_event *event,
>  	 * is visible.
>  	 */
>  	smp_wmb();
> -	WRITE_ONCE(event->state, PERF_EVENT_STATE_ACTIVE);
> +	perf_event_set_state(event, PERF_EVENT_STATE_ACTIVE);
>  
>  	/*
>  	 * Unthrottle events, since we scheduled we might have missed several
> @@ -2089,26 +2048,19 @@ event_sched_in(struct perf_event *event,
>  		event->hw.interrupts = 0;
>  	}
>  
> -	/*
> -	 * The new state must be visible before we turn it on in the hardware:
> -	 */
> -	smp_wmb();
> -
>  	perf_pmu_disable(event->pmu);
>  
> -	perf_set_shadow_time(event, ctx, tstamp);
> +	perf_set_shadow_time(event, ctx);
>  
>  	perf_log_itrace_start(event);
>  
>  	if (event->pmu->add(event, PERF_EF_START)) {
> -		event->state = PERF_EVENT_STATE_INACTIVE;
> +		perf_event_set_state(event, PERF_EVENT_STATE_INACTIVE);
>  		event->oncpu = -1;
>  		ret = -EAGAIN;
>  		goto out;
>  	}
>  
> -	event->tstamp_running += tstamp - event->tstamp_stopped;
> -
>  	if (!is_software_event(event))
>  		cpuctx->active_oncpu++;
>  	if (!ctx->nr_active++)
> @@ -2132,8 +2084,6 @@ group_sched_in(struct perf_event *group_event,
>  {
>  	struct perf_event *event, *partial_group = NULL;
>  	struct pmu *pmu = ctx->pmu;
> -	u64 now = ctx->time;
> -	bool simulate = false;
>  
>  	if (group_event->state == PERF_EVENT_STATE_OFF)
>  		return 0;
> @@ -2163,27 +2113,13 @@ group_sched_in(struct perf_event *group_event,
>  	/*
>  	 * Groups can be scheduled in as one unit only, so undo any
>  	 * partial group before returning:
> -	 * The events up to the failed event are scheduled out normally,
> -	 * tstamp_stopped will be updated.
> -	 *
> -	 * The failed events and the remaining siblings need to have
> -	 * their timings updated as if they had gone thru event_sched_in()
> -	 * and event_sched_out(). This is required to get consistent timings
> -	 * across the group. This also takes care of the case where the group
> -	 * could never be scheduled by ensuring tstamp_stopped is set to mark
> -	 * the time the event was actually stopped, such that time delta
> -	 * calculation in update_event_times() is correct.
> +	 * The events up to the failed event are scheduled out normally.
>  	 */
>  	list_for_each_entry(event, &group_event->sibling_list, group_entry) {
>  		if (event == partial_group)
> -			simulate = true;
> +			break;
>  
> -		if (simulate) {
> -			event->tstamp_running += now - event->tstamp_stopped;
> -			event->tstamp_stopped = now;
> -		} else {
> -			event_sched_out(event, cpuctx, ctx);
> -		}
> +		event_sched_out(event, cpuctx, ctx);
>  	}
>  	event_sched_out(group_event, cpuctx, ctx);
>  
> @@ -2225,46 +2161,11 @@ static int group_can_go_on(struct perf_event *event,
>  	return can_add_hw;
>  }
>  
> -/*
> - * Complement to update_event_times(). This computes the tstamp_* values to
> - * continue 'enabled' state from @now, and effectively discards the time
> - * between the prior tstamp_stopped and now (as we were in the OFF state, or
> - * just switched (context) time base).
> - *
> - * This further assumes '@event->state == INACTIVE' (we just came from OFF) and
> - * cannot have been scheduled in yet. And going into INACTIVE state means
> - * '@event->tstamp_stopped = @now'.
> - *
> - * Thus given the rules of update_event_times():
> - *
> - *   total_time_enabled = tstamp_stopped - tstamp_enabled
> - *   total_time_running = tstamp_stopped - tstamp_running
> - *
> - * We can insert 'tstamp_stopped == now' and reverse them to compute new
> - * tstamp_* values.
> - */
> -static void __perf_event_enable_time(struct perf_event *event, u64 now)
> -{
> -	WARN_ON_ONCE(event->state != PERF_EVENT_STATE_INACTIVE);
> -
> -	event->tstamp_stopped = now;
> -	event->tstamp_enabled = now - event->total_time_enabled;
> -	event->tstamp_running = now - event->total_time_running;
> -}
> -
>  static void add_event_to_ctx(struct perf_event *event,
>  			       struct perf_event_context *ctx)
>  {
> -	u64 tstamp = perf_event_time(event);
> -
>  	list_add_event(event, ctx);
>  	perf_group_attach(event);
> -	/*
> -	 * We can be called with event->state == STATE_OFF when we create with
> -	 * .disabled = 1. In that case the IOC_ENABLE will call this function.
> -	 */
> -	if (event->state == PERF_EVENT_STATE_INACTIVE)
> -		__perf_event_enable_time(event, tstamp);
>  }
>  
>  static void ctx_sched_out(struct perf_event_context *ctx,
> @@ -2496,28 +2397,6 @@ perf_install_in_context(struct perf_event_context *ctx,
>  }
>  
>  /*
> - * Put a event into inactive state and update time fields.
> - * Enabling the leader of a group effectively enables all
> - * the group members that aren't explicitly disabled, so we
> - * have to update their ->tstamp_enabled also.
> - * Note: this works for group members as well as group leaders
> - * since the non-leader members' sibling_lists will be empty.
> - */
> -static void __perf_event_mark_enabled(struct perf_event *event)
> -{
> -	struct perf_event *sub;
> -	u64 tstamp = perf_event_time(event);
> -
> -	event->state = PERF_EVENT_STATE_INACTIVE;
> -	__perf_event_enable_time(event, tstamp);
> -	list_for_each_entry(sub, &event->sibling_list, group_entry) {
> -		/* XXX should not be > INACTIVE if event isn't */
> -		if (sub->state >= PERF_EVENT_STATE_INACTIVE)
> -			__perf_event_enable_time(sub, tstamp);
> -	}
> -}
> -
> -/*
>   * Cross CPU call to enable a performance event
>   */
>  static void __perf_event_enable(struct perf_event *event,
> @@ -2535,14 +2414,12 @@ static void __perf_event_enable(struct perf_event *event,
>  	if (ctx->is_active)
>  		ctx_sched_out(ctx, cpuctx, EVENT_TIME);
>  
> -	__perf_event_mark_enabled(event);
> +	perf_event_set_state(event, PERF_EVENT_STATE_INACTIVE);
>  
>  	if (!ctx->is_active)
>  		return;
>  
>  	if (!event_filter_match(event)) {
> -		if (is_cgroup_event(event))
> -			perf_cgroup_defer_enabled(event);
>  		ctx_sched_in(ctx, cpuctx, EVENT_TIME, current);
>  		return;
>  	}
> @@ -2862,18 +2739,10 @@ static void __perf_event_sync_stat(struct perf_event *event,
>  	 * we know the event must be on the current CPU, therefore we
>  	 * don't need to use it.
>  	 */
> -	switch (event->state) {
> -	case PERF_EVENT_STATE_ACTIVE:
> +	if (event->state == PERF_EVENT_STATE_ACTIVE)
>  		event->pmu->read(event);
> -		/* fall-through */
>  
> -	case PERF_EVENT_STATE_INACTIVE:
> -		update_event_times(event);
> -		break;
> -
> -	default:
> -		break;
> -	}
> +	perf_event_update_time(event);
>  
>  	/*
>  	 * In order to keep per-task stats reliable we need to flip the event
> @@ -3110,10 +2979,6 @@ ctx_pinned_sched_in(struct perf_event_context *ctx,
>  		if (!event_filter_match(event))
>  			continue;
>  
> -		/* may need to reset tstamp_enabled */
> -		if (is_cgroup_event(event))
> -			perf_cgroup_mark_enabled(event, ctx);
> -
>  		if (group_can_go_on(event, cpuctx, 1))
>  			group_sched_in(event, cpuctx, ctx);
>  
> @@ -3121,10 +2986,8 @@ ctx_pinned_sched_in(struct perf_event_context *ctx,
>  		 * If this pinned group hasn't been scheduled,
>  		 * put it in error state.
>  		 */
> -		if (event->state == PERF_EVENT_STATE_INACTIVE) {
> -			update_group_times(event);
> -			event->state = PERF_EVENT_STATE_ERROR;
> -		}
> +		if (event->state == PERF_EVENT_STATE_INACTIVE)
> +			perf_event_set_state(event, PERF_EVENT_STATE_ERROR);
>  	}
>  }
>  
> @@ -3146,10 +3009,6 @@ ctx_flexible_sched_in(struct perf_event_context *ctx,
>  		if (!event_filter_match(event))
>  			continue;
>  
> -		/* may need to reset tstamp_enabled */
> -		if (is_cgroup_event(event))
> -			perf_cgroup_mark_enabled(event, ctx);
> -
>  		if (group_can_go_on(event, cpuctx, can_add_hw)) {
>  			if (group_sched_in(event, cpuctx, ctx))
>  				can_add_hw = 0;
> @@ -3541,7 +3400,7 @@ static int event_enable_on_exec(struct perf_event *event,
>  	if (event->state >= PERF_EVENT_STATE_INACTIVE)
>  		return 0;
>  
> -	__perf_event_mark_enabled(event);
> +	perf_event_set_state(event, PERF_EVENT_STATE_INACTIVE);
>  
>  	return 1;
>  }
> @@ -3590,12 +3449,6 @@ static void perf_event_enable_on_exec(int ctxn)
>  		put_ctx(clone_ctx);
>  }
>  
> -struct perf_read_data {
> -	struct perf_event *event;
> -	bool group;
> -	int ret;
> -};
> -
>  static int __perf_event_read_cpu(struct perf_event *event, int event_cpu)
>  {
>  	u16 local_pkg, event_pkg;
> @@ -3613,64 +3466,6 @@ static int __perf_event_read_cpu(struct perf_event *event, int event_cpu)
>  	return event_cpu;
>  }
>  
> -/*
> - * Cross CPU call to read the hardware event
> - */
> -static void __perf_event_read(void *info)
> -{
> -	struct perf_read_data *data = info;
> -	struct perf_event *sub, *event = data->event;
> -	struct perf_event_context *ctx = event->ctx;
> -	struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
> -	struct pmu *pmu = event->pmu;
> -
> -	/*
> -	 * If this is a task context, we need to check whether it is
> -	 * the current task context of this cpu.  If not it has been
> -	 * scheduled out before the smp call arrived.  In that case
> -	 * event->count would have been updated to a recent sample
> -	 * when the event was scheduled out.
> -	 */
> -	if (ctx->task && cpuctx->task_ctx != ctx)
> -		return;
> -
> -	raw_spin_lock(&ctx->lock);
> -	if (ctx->is_active) {
> -		update_context_time(ctx);
> -		update_cgrp_time_from_event(event);
> -	}
> -
> -	update_event_times(event);
> -	if (event->state != PERF_EVENT_STATE_ACTIVE)
> -		goto unlock;
> -
> -	if (!data->group) {
> -		pmu->read(event);
> -		data->ret = 0;
> -		goto unlock;
> -	}
> -
> -	pmu->start_txn(pmu, PERF_PMU_TXN_READ);
> -
> -	pmu->read(event);
> -
> -	list_for_each_entry(sub, &event->sibling_list, group_entry) {
> -		update_event_times(sub);
> -		if (sub->state == PERF_EVENT_STATE_ACTIVE) {
> -			/*
> -			 * Use sibling's PMU rather than @event's since
> -			 * sibling could be on different (eg: software) PMU.
> -			 */
> -			sub->pmu->read(sub);
> -		}
> -	}
> -
> -	data->ret = pmu->commit_txn(pmu);
> -
> -unlock:
> -	raw_spin_unlock(&ctx->lock);
> -}
> -
>  static inline u64 perf_event_count(struct perf_event *event)
>  {
>  	return local64_read(&event->count) + atomic64_read(&event->child_count);
> @@ -3733,63 +3528,81 @@ int perf_event_read_local(struct perf_event *event, u64 *value)
>  	return ret;
>  }
>  
> -static int perf_event_read(struct perf_event *event, bool group)
> +struct perf_read_data {
> +	struct perf_event *event;
> +	bool group;
> +	int ret;
> +};
> +
> +static void __perf_event_read(struct perf_event *event,
> +			      struct perf_cpu_context *cpuctx,
> +			      struct perf_event_context *ctx,
> +			      void *data)
>  {
> -	int event_cpu, ret = 0;
> +	struct perf_read_data *prd = data;
> +	struct pmu *pmu = event->pmu;
> +	struct perf_event *sibling;
>  
> -	/*
> -	 * If event is enabled and currently active on a CPU, update the
> -	 * value in the event structure:
> -	 */
> -	if (event->state == PERF_EVENT_STATE_ACTIVE) {
> -		struct perf_read_data data = {
> -			.event = event,
> -			.group = group,
> -			.ret = 0,
> -		};
> +	if (ctx->is_active & EVENT_TIME) {
> +		update_context_time(ctx);
> +		update_cgrp_time_from_cpuctx(cpuctx);
> +	}
>  
> -		event_cpu = READ_ONCE(event->oncpu);
> -		if ((unsigned)event_cpu >= nr_cpu_ids)
> -			return 0;
> +	perf_event_update_time(event);
> +	if (prd->group)
> +		perf_event_update_sibling_time(event);
>  
> -		preempt_disable();
> -		event_cpu = __perf_event_read_cpu(event, event_cpu);
> +	if (event->state != PERF_EVENT_STATE_ACTIVE)
> +		return;
>  
> +	if (!prd->group) {
> +		pmu->read(event);
> +		prd->ret = 0;
> +		return;
> +	}
> +
> +	pmu->start_txn(pmu, PERF_PMU_TXN_READ);
> +
> +	pmu->read(event);
> +	list_for_each_entry(sibling, &event->sibling_list, group_entry) {
> +		if (sibling->state == PERF_EVENT_STATE_ACTIVE) {
> +			/*
> +			 * Use sibling's PMU rather than @event's since
> +			 * sibling could be on different (eg: software) PMU.
> +			 */
> +			sibling->pmu->read(sibling);
> +		}
> +	}
> +
> +	prd->ret = pmu->commit_txn(pmu);
> +}
> +
> +static int perf_event_read(struct perf_event *event, bool group)
> +{
> +	struct perf_read_data prd = {
> +		.event = event,
> +		.group = group,
> +		.ret = 0,
> +	};
> +
> +	if (event->ctx->task) {
> +		event_function_call(event, __perf_event_read, &prd);
> +	} else {
>  		/*
> -		 * Purposely ignore the smp_call_function_single() return
> -		 * value.
> -		 *
> -		 * If event_cpu isn't a valid CPU it means the event got
> -		 * scheduled out and that will have updated the event count.
> -		 *
> -		 * Therefore, either way, we'll have an up-to-date event count
> -		 * after this.
> -		 */
> -		(void)smp_call_function_single(event_cpu, __perf_event_read, &data, 1);
> -		preempt_enable();
> -		ret = data.ret;
> -	} else if (event->state == PERF_EVENT_STATE_INACTIVE) {
> -		struct perf_event_context *ctx = event->ctx;
> -		unsigned long flags;
> -
> -		raw_spin_lock_irqsave(&ctx->lock, flags);
> -		/*
> -		 * may read while context is not active
> -		 * (e.g., thread is blocked), in that case
> -		 * we cannot update context time
> +		 * For uncore events (which are per definition per-cpu)
> +		 * allow a different read CPU from event->cpu.
>  		 */
> -		if (ctx->is_active) {
> -			update_context_time(ctx);
> -			update_cgrp_time_from_event(event);
> -		}
> -		if (group)
> -			update_group_times(event);
> -		else
> -			update_event_times(event);
> -		raw_spin_unlock_irqrestore(&ctx->lock, flags);
> +		struct event_function_struct efs = {
> +			.event = event,
> +			.func = __perf_event_read,
> +			.data = &prd,
> +		};
> +		int cpu = __perf_event_read_cpu(event, event->cpu);
> +
> +		cpu_function_call(cpu, event_function, &efs);
>  	}
>  
> -	return ret;
> +	return prd.ret;
>  }
>  
>  /*
> @@ -4388,7 +4201,7 @@ static int perf_release(struct inode *inode, struct file *file)
>  	return 0;
>  }
>  
> -u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running)
> +static u64 __perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running)
>  {
>  	struct perf_event *child;
>  	u64 total = 0;
> @@ -4416,6 +4229,18 @@ u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running)
>  
>  	return total;
>  }
> +
> +u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running)
> +{
> +	struct perf_event_context *ctx;
> +	u64 count;
> +
> +	ctx = perf_event_ctx_lock(event);
> +	count = __perf_event_read_value(event, enabled, running);
> +	perf_event_ctx_unlock(event, ctx);
> +
> +	return count;
> +}
>  EXPORT_SYMBOL_GPL(perf_event_read_value);
>  
>  static int __perf_read_group_add(struct perf_event *leader,
> @@ -4431,6 +4256,8 @@ static int __perf_read_group_add(struct perf_event *leader,
>  	if (ret)
>  		return ret;
>  
> +	raw_spin_lock_irqsave(&ctx->lock, flags);
> +
>  	/*
>  	 * Since we co-schedule groups, {enabled,running} times of siblings
>  	 * will be identical to those of the leader, so we only publish one
> @@ -4453,8 +4280,6 @@ static int __perf_read_group_add(struct perf_event *leader,
>  	if (read_format & PERF_FORMAT_ID)
>  		values[n++] = primary_event_id(leader);
>  
> -	raw_spin_lock_irqsave(&ctx->lock, flags);
> -
>  	list_for_each_entry(sub, &leader->sibling_list, group_entry) {
>  		values[n++] += perf_event_count(sub);
>  		if (read_format & PERF_FORMAT_ID)
> @@ -4518,7 +4343,7 @@ static int perf_read_one(struct perf_event *event,
>  	u64 values[4];
>  	int n = 0;
>  
> -	values[n++] = perf_event_read_value(event, &enabled, &running);
> +	values[n++] = __perf_event_read_value(event, &enabled, &running);
>  	if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
>  		values[n++] = enabled;
>  	if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)
> @@ -4897,8 +4722,7 @@ static void calc_timer_values(struct perf_event *event,
>  
>  	*now = perf_clock();
>  	ctx_time = event->shadow_ctx_time + *now;
> -	*enabled = ctx_time - event->tstamp_enabled;
> -	*running = ctx_time - event->tstamp_running;
> +	__perf_update_times(event, ctx_time, enabled, running);
>  }
>  
>  static void perf_event_init_userpage(struct perf_event *event)
> @@ -10516,7 +10340,7 @@ perf_event_exit_event(struct perf_event *child_event,
>  	if (parent_event)
>  		perf_group_detach(child_event);
>  	list_del_event(child_event, child_ctx);
> -	child_event->state = PERF_EVENT_STATE_EXIT; /* is_event_hup() */
> +	perf_event_set_state(child_event, PERF_EVENT_STATE_EXIT); /* is_event_hup() */
>  	raw_spin_unlock_irq(&child_ctx->lock);
>  
>  	/*
> @@ -10754,7 +10578,7 @@ inherit_event(struct perf_event *parent_event,
>  	      struct perf_event *group_leader,
>  	      struct perf_event_context *child_ctx)
>  {
> -	enum perf_event_active_state parent_state = parent_event->state;
> +	enum perf_event_state parent_state = parent_event->state;
>  	struct perf_event *child_event;
>  	unsigned long flags;
>  
> @@ -11090,6 +10914,7 @@ static void __perf_event_exit_context(void *__info)
>  	struct perf_event *event;
>  
>  	raw_spin_lock(&ctx->lock);
> +	ctx_sched_out(ctx, cpuctx, EVENT_TIME);
>  	list_for_each_entry(event, &ctx->event_list, event_entry)
>  		__perf_remove_from_context(event, cpuctx, ctx, (void *)DETACH_GROUP);
>  	raw_spin_unlock(&ctx->lock);
> 

[toc] | [prev] | [next] | [standalone]

#1725019

On Fri, Sep 01, 2017 at 01:45:17PM +0300, Alexey Budankov wrote:
> Well, this looks like an "opposite" approach to event timekeeping in 
> comparison to what we currently have. 

I would say 'sane' approach. The current thing is horrible.

> Do you want this rework before or after the current patch set?

Before I would think, because the whole point of the rb-tree thing is to
not touch all events all the time. And you can only do that after you
fix that timekeeping.

[toc] | [prev] | [next] | [standalone]

#1724981

On 31.08.2017 20:18, Peter Zijlstra wrote:
> On Wed, Aug 23, 2017 at 11:54:15AM +0300, Alexey Budankov wrote:
>> On 22.08.2017 23:47, Peter Zijlstra wrote:
>>> On Thu, Aug 10, 2017 at 06:57:43PM +0300, Alexey Budankov wrote:
>>>> The key thing in the patch is explicit updating of tstamp fields for
>>>> INACTIVE events in update_event_times().
>>>
>>>> @@ -1405,6 +1426,9 @@ static void update_event_times(struct perf_event *event)
>>>>  	    event->group_leader->state < PERF_EVENT_STATE_INACTIVE)
>>>>  		return;
>>>>  
>>>> +	if (event->state == PERF_EVENT_STATE_INACTIVE)
>>>> +		perf_event_tstamp_update(event);
>>>> +
>>>>  	/*
>>>>  	 * in cgroup mode, time_enabled represents
>>>>  	 * the time the event was enabled AND active
>>>
>>> But why!? I thought the whole point was to not need to do this.
>>
>> update_event_times() is not called from timer interrupt handler 
>> thus it is not on the critical path which is optimized in this patch set.
>>
>> But update_event_times() is called in the context of read() syscall so
>> this is the place where we may update event times for INACTIVE events 
>> instead of timer interrupt.
>>
>> Also update_event_times() is called on thread context switch out so
>> we get event times also updated when the thread migrates to other CPU.
>>
>>>
>>> The thing I outlined earlier would only need to update timestamps when
>>> events change state and at no other point in time.
>>
>> But we still may request times while event is in INACTIVE state 
>> thru read() syscall and event timings need to be up-to-date. 
> 
> Sure, read() also updates.
> 
> So the below completely rewrites timekeeping (and probably breaks
> world) but does away with the need to touch events that don't get
> scheduled.
> 
> Esp the cgroup stuff is entirely untested since I simply don't know how
> to operate that. I did run Vince's tests on it, and I think it doesn't
> regress, but I'm near a migraine so I can't really see straight atm.
> 
> Vince, Stephane, could you guys have a peek?
> 
> (There's a few other bits in, I'll break up into patches and write
> comments and Changelogs later, I think its can be split in some 5
> patches).
> 
> The basic idea is really simple, we have a single timestamp and
> depending on the state we update enabled/running. This obviously only
> requires updates when we change state and when we need up-to-date
> timestamps (read).
> 
> No more weird and wonderful mind bending interaction between 3 different
> timestamps with arcane update rules.
> 
> ---
>  include/linux/perf_event.h |  25 +-
>  kernel/events/core.c       | 551 ++++++++++++++++-----------------------------
>  2 files changed, 192 insertions(+), 384 deletions(-)
> 

Tried to apply on top of this:

perf/core 1b2f76d77a277bb70d38ad0991ed7f16bbc115a9 [origin/perf/core] Merge tag 'perf-core-for-mingo-4.14-20170829' of git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux into perf/core

but failed:

Checking patch include/linux/perf_event.h...
Hunk #1 succeeded at 498 (offset 13 lines).
Hunk #2 succeeded at 591 (offset 13 lines).
Hunk #3 succeeded at 601 (offset 13 lines).
Hunk #4 succeeded at 696 (offset 13 lines).
Checking patch kernel/events/core.c...
error: while searching for:
	return event_cpu;
}

/*
 * Cross CPU call to read the hardware event
 */
static void __perf_event_read(void *info)
{
	struct perf_read_data *data = info;
	struct perf_event *sub, *event = data->event;
	struct perf_event_context *ctx = event->ctx;
	struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
	struct pmu *pmu = event->pmu;

	/*
	 * If this is a task context, we need to check whether it is
	 * the current task context of this cpu.  If not it has been
	 * scheduled out before the smp call arrived.  In that case
	 * event->count would have been updated to a recent sample
	 * when the event was scheduled out.
	 */
	if (ctx->task && cpuctx->task_ctx != ctx)
		return;

	raw_spin_lock(&ctx->lock);
	if (ctx->is_active) {
		update_context_time(ctx);
		update_cgrp_time_from_event(event);
	}

	update_event_times(event);
	if (event->state != PERF_EVENT_STATE_ACTIVE)
		goto unlock;

	if (!data->group) {
		pmu->read(event);
		data->ret = 0;
		goto unlock;
	}

	pmu->start_txn(pmu, PERF_PMU_TXN_READ);

	pmu->read(event);

	list_for_each_entry(sub, &event->sibling_list, group_entry) {
		update_event_times(sub);
		if (sub->state == PERF_EVENT_STATE_ACTIVE) {
			/*
			 * Use sibling's PMU rather than @event's since
			 * sibling could be on different (eg: software) PMU.
			 */
			sub->pmu->read(sub);
		}
	}

	data->ret = pmu->commit_txn(pmu);

unlock:
	raw_spin_unlock(&ctx->lock);
}

static inline u64 perf_event_count(struct perf_event *event)
{
	return local64_read(&event->count) + atomic64_read(&event->child_count);

error: patch failed: kernel/events/core.c:3613
error: kernel/events/core.c: patch does not apply

> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 8e22f24ded6a..2a6ae48a1a96 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -485,9 +485,9 @@ struct perf_addr_filters_head {
>  };
>  
>  /**
> - * enum perf_event_active_state - the states of a event
> + * enum perf_event_state - the states of a event
>   */
> -enum perf_event_active_state {
> +enum perf_event_state {
>  	PERF_EVENT_STATE_DEAD		= -4,
>  	PERF_EVENT_STATE_EXIT		= -3,
>  	PERF_EVENT_STATE_ERROR		= -2,
> @@ -578,7 +578,7 @@ struct perf_event {
>  	struct pmu			*pmu;
>  	void				*pmu_private;
>  
> -	enum perf_event_active_state	state;
> +	enum perf_event_state		state;
>  	unsigned int			attach_state;
>  	local64_t			count;
>  	atomic64_t			child_count;
> @@ -588,26 +588,10 @@ struct perf_event {
>  	 * has been enabled (i.e. eligible to run, and the task has
>  	 * been scheduled in, if this is a per-task event)
>  	 * and running (scheduled onto the CPU), respectively.
> -	 *
> -	 * They are computed from tstamp_enabled, tstamp_running and
> -	 * tstamp_stopped when the event is in INACTIVE or ACTIVE state.
>  	 */
>  	u64				total_time_enabled;
>  	u64				total_time_running;
> -
> -	/*
> -	 * These are timestamps used for computing total_time_enabled
> -	 * and total_time_running when the event is in INACTIVE or
> -	 * ACTIVE state, measured in nanoseconds from an arbitrary point
> -	 * in time.
> -	 * tstamp_enabled: the notional time when the event was enabled
> -	 * tstamp_running: the notional time when the event was scheduled on
> -	 * tstamp_stopped: in INACTIVE state, the notional time when the
> -	 *	event was scheduled off.
> -	 */
> -	u64				tstamp_enabled;
> -	u64				tstamp_running;
> -	u64				tstamp_stopped;
> +	u64				tstamp;
>  
>  	/*
>  	 * timestamp shadows the actual context timing but it can
> @@ -699,7 +683,6 @@ struct perf_event {
>  
>  #ifdef CONFIG_CGROUP_PERF
>  	struct perf_cgroup		*cgrp; /* cgroup event is attach to */
> -	int				cgrp_defer_enabled;
>  #endif
>  
>  	struct list_head		sb_list;
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 294f1927f944..e968b3eab9c7 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -582,6 +582,70 @@ static inline u64 perf_event_clock(struct perf_event *event)
>  	return event->clock();
>  }
>  
> +/*
> + * XXX comment about timekeeping goes here
> + */
> +
> +static __always_inline enum perf_event_state
> +__perf_effective_state(struct perf_event *event)
> +{
> +	struct perf_event *leader = event->group_leader;
> +
> +	if (leader->state <= PERF_EVENT_STATE_OFF)
> +		return leader->state;
> +
> +	return event->state;
> +}
> +
> +static __always_inline void
> +__perf_update_times(struct perf_event *event, u64 now, u64 *enabled, u64 *running)
> +{
> +	enum perf_event_state state = __perf_effective_state(event);
> +	u64 delta = now - event->tstamp;
> +
> +	*enabled = event->total_time_enabled;
> +	if (state >= PERF_EVENT_STATE_INACTIVE)
> +		*enabled += delta;
> +
> +	*running = event->total_time_running;
> +	if (state >= PERF_EVENT_STATE_ACTIVE)
> +		*running += delta;
> +}
> +
> +static void perf_event_update_time(struct perf_event *event)
> +{
> +	u64 now = perf_event_time(event);
> +
> +	__perf_update_times(event, now, &event->total_time_enabled,
> +					&event->total_time_running);
> +	event->tstamp = now;
> +}
> +
> +static void perf_event_update_sibling_time(struct perf_event *leader)
> +{
> +	struct perf_event *sibling;
> +
> +	list_for_each_entry(sibling, &leader->sibling_list, group_entry)
> +		perf_event_update_time(sibling);
> +}
> +
> +static void
> +perf_event_set_state(struct perf_event *event, enum perf_event_state state)
> +{
> +	if (event->state == state)
> +		return;
> +
> +	perf_event_update_time(event);
> +	/*
> +	 * If a group leader gets enabled/disabled all its siblings
> +	 * are affected too.
> +	 */
> +	if ((event->state < 0) ^ (state < 0))
> +		perf_event_update_sibling_time(event);
> +
> +	WRITE_ONCE(event->state, state);
> +}
> +
>  #ifdef CONFIG_CGROUP_PERF
>  
>  static inline bool
> @@ -841,40 +905,6 @@ perf_cgroup_set_shadow_time(struct perf_event *event, u64 now)
>  	event->shadow_ctx_time = now - t->timestamp;
>  }
>  
> -static inline void
> -perf_cgroup_defer_enabled(struct perf_event *event)
> -{
> -	/*
> -	 * when the current task's perf cgroup does not match
> -	 * the event's, we need to remember to call the
> -	 * perf_mark_enable() function the first time a task with
> -	 * a matching perf cgroup is scheduled in.
> -	 */
> -	if (is_cgroup_event(event) && !perf_cgroup_match(event))
> -		event->cgrp_defer_enabled = 1;
> -}
> -
> -static inline void
> -perf_cgroup_mark_enabled(struct perf_event *event,
> -			 struct perf_event_context *ctx)
> -{
> -	struct perf_event *sub;
> -	u64 tstamp = perf_event_time(event);
> -
> -	if (!event->cgrp_defer_enabled)
> -		return;
> -
> -	event->cgrp_defer_enabled = 0;
> -
> -	event->tstamp_enabled = tstamp - event->total_time_enabled;
> -	list_for_each_entry(sub, &event->sibling_list, group_entry) {
> -		if (sub->state >= PERF_EVENT_STATE_INACTIVE) {
> -			sub->tstamp_enabled = tstamp - sub->total_time_enabled;
> -			sub->cgrp_defer_enabled = 0;
> -		}
> -	}
> -}
> -
>  /*
>   * Update cpuctx->cgrp so that it is set when first cgroup event is added and
>   * cleared when last cgroup event is removed.
> @@ -973,17 +1003,6 @@ static inline u64 perf_cgroup_event_time(struct perf_event *event)
>  }
>  
>  static inline void
> -perf_cgroup_defer_enabled(struct perf_event *event)
> -{
> -}
> -
> -static inline void
> -perf_cgroup_mark_enabled(struct perf_event *event,
> -			 struct perf_event_context *ctx)
> -{
> -}
> -
> -static inline void
>  list_update_cgroup_event(struct perf_event *event,
>  			 struct perf_event_context *ctx, bool add)
>  {
> @@ -1396,60 +1415,6 @@ static u64 perf_event_time(struct perf_event *event)
>  	return ctx ? ctx->time : 0;
>  }
>  
> -/*
> - * Update the total_time_enabled and total_time_running fields for a event.
> - */
> -static void update_event_times(struct perf_event *event)
> -{
> -	struct perf_event_context *ctx = event->ctx;
> -	u64 run_end;
> -
> -	lockdep_assert_held(&ctx->lock);
> -
> -	if (event->state < PERF_EVENT_STATE_INACTIVE ||
> -	    event->group_leader->state < PERF_EVENT_STATE_INACTIVE)
> -		return;
> -
> -	/*
> -	 * in cgroup mode, time_enabled represents
> -	 * the time the event was enabled AND active
> -	 * tasks were in the monitored cgroup. This is
> -	 * independent of the activity of the context as
> -	 * there may be a mix of cgroup and non-cgroup events.
> -	 *
> -	 * That is why we treat cgroup events differently
> -	 * here.
> -	 */
> -	if (is_cgroup_event(event))
> -		run_end = perf_cgroup_event_time(event);
> -	else if (ctx->is_active)
> -		run_end = ctx->time;
> -	else
> -		run_end = event->tstamp_stopped;
> -
> -	event->total_time_enabled = run_end - event->tstamp_enabled;
> -
> -	if (event->state == PERF_EVENT_STATE_INACTIVE)
> -		run_end = event->tstamp_stopped;
> -	else
> -		run_end = perf_event_time(event);
> -
> -	event->total_time_running = run_end - event->tstamp_running;
> -
> -}
> -
> -/*
> - * Update total_time_enabled and total_time_running for all events in a group.
> - */
> -static void update_group_times(struct perf_event *leader)
> -{
> -	struct perf_event *event;
> -
> -	update_event_times(leader);
> -	list_for_each_entry(event, &leader->sibling_list, group_entry)
> -		update_event_times(event);
> -}
> -
>  static enum event_type_t get_event_type(struct perf_event *event)
>  {
>  	struct perf_event_context *ctx = event->ctx;
> @@ -1492,6 +1457,8 @@ list_add_event(struct perf_event *event, struct perf_event_context *ctx)
>  	WARN_ON_ONCE(event->attach_state & PERF_ATTACH_CONTEXT);
>  	event->attach_state |= PERF_ATTACH_CONTEXT;
>  
> +	event->tstamp = perf_event_time(event);
> +
>  	/*
>  	 * If we're a stand alone event or group leader, we go to the context
>  	 * list, group events are kept attached to the group so that
> @@ -1699,8 +1666,6 @@ list_del_event(struct perf_event *event, struct perf_event_context *ctx)
>  	if (event->group_leader == event)
>  		list_del_init(&event->group_entry);
>  
> -	update_group_times(event);
> -
>  	/*
>  	 * If event was in error state, then keep it
>  	 * that way, otherwise bogus counts will be
> @@ -1709,7 +1674,7 @@ list_del_event(struct perf_event *event, struct perf_event_context *ctx)
>  	 * of the event
>  	 */
>  	if (event->state > PERF_EVENT_STATE_OFF)
> -		event->state = PERF_EVENT_STATE_OFF;
> +		perf_event_set_state(event, PERF_EVENT_STATE_OFF);
>  
>  	ctx->generation++;
>  }
> @@ -1808,38 +1773,24 @@ event_sched_out(struct perf_event *event,
>  		  struct perf_cpu_context *cpuctx,
>  		  struct perf_event_context *ctx)
>  {
> -	u64 tstamp = perf_event_time(event);
> -	u64 delta;
> +	enum perf_event_state state = PERF_EVENT_STATE_INACTIVE;
>  
>  	WARN_ON_ONCE(event->ctx != ctx);
>  	lockdep_assert_held(&ctx->lock);
>  
> -	/*
> -	 * An event which could not be activated because of
> -	 * filter mismatch still needs to have its timings
> -	 * maintained, otherwise bogus information is return
> -	 * via read() for time_enabled, time_running:
> -	 */
> -	if (event->state == PERF_EVENT_STATE_INACTIVE &&
> -	    !event_filter_match(event)) {
> -		delta = tstamp - event->tstamp_stopped;
> -		event->tstamp_running += delta;
> -		event->tstamp_stopped = tstamp;
> -	}
> -
>  	if (event->state != PERF_EVENT_STATE_ACTIVE)
>  		return;
>  
>  	perf_pmu_disable(event->pmu);
>  
> -	event->tstamp_stopped = tstamp;
>  	event->pmu->del(event, 0);
>  	event->oncpu = -1;
> -	event->state = PERF_EVENT_STATE_INACTIVE;
> +
>  	if (event->pending_disable) {
>  		event->pending_disable = 0;
> -		event->state = PERF_EVENT_STATE_OFF;
> +		state = PERF_EVENT_STATE_OFF;
>  	}
> +	perf_event_set_state(event, state);
>  
>  	if (!is_software_event(event))
>  		cpuctx->active_oncpu--;
> @@ -1859,7 +1810,9 @@ group_sched_out(struct perf_event *group_event,
>  		struct perf_event_context *ctx)
>  {
>  	struct perf_event *event;
> -	int state = group_event->state;
> +
> +	if (group_event->state != PERF_EVENT_STATE_ACTIVE)
> +		return;
>  
>  	perf_pmu_disable(ctx->pmu);
>  
> @@ -1873,7 +1826,7 @@ group_sched_out(struct perf_event *group_event,
>  
>  	perf_pmu_enable(ctx->pmu);
>  
> -	if (state == PERF_EVENT_STATE_ACTIVE && group_event->attr.exclusive)
> +	if (group_event->attr.exclusive)
>  		cpuctx->exclusive = 0;
>  }
>  
> @@ -1893,6 +1846,11 @@ __perf_remove_from_context(struct perf_event *event,
>  {
>  	unsigned long flags = (unsigned long)info;
>  
> +	if (ctx->is_active & EVENT_TIME) {
> +		update_context_time(ctx);
> +		update_cgrp_time_from_cpuctx(cpuctx);
> +	}
> +
>  	event_sched_out(event, cpuctx, ctx);
>  	if (flags & DETACH_GROUP)
>  		perf_group_detach(event);
> @@ -1955,14 +1913,17 @@ static void __perf_event_disable(struct perf_event *event,
>  	if (event->state < PERF_EVENT_STATE_INACTIVE)
>  		return;
>  
> -	update_context_time(ctx);
> -	update_cgrp_time_from_event(event);
> -	update_group_times(event);
> +	if (ctx->is_active & EVENT_TIME) {
> +		update_context_time(ctx);
> +		update_cgrp_time_from_cpuctx(cpuctx);
> +	}
> +
>  	if (event == event->group_leader)
>  		group_sched_out(event, cpuctx, ctx);
>  	else
>  		event_sched_out(event, cpuctx, ctx);
> -	event->state = PERF_EVENT_STATE_OFF;
> +
> +	perf_event_set_state(event, PERF_EVENT_STATE_OFF);
>  }
>  
>  /*
> @@ -2019,8 +1980,7 @@ void perf_event_disable_inatomic(struct perf_event *event)
>  }
>  
>  static void perf_set_shadow_time(struct perf_event *event,
> -				 struct perf_event_context *ctx,
> -				 u64 tstamp)
> +				 struct perf_event_context *ctx)
>  {
>  	/*
>  	 * use the correct time source for the time snapshot
> @@ -2048,9 +2008,9 @@ static void perf_set_shadow_time(struct perf_event *event,
>  	 * is cleaner and simpler to understand.
>  	 */
>  	if (is_cgroup_event(event))
> -		perf_cgroup_set_shadow_time(event, tstamp);
> +		perf_cgroup_set_shadow_time(event, event->tstamp);
>  	else
> -		event->shadow_ctx_time = tstamp - ctx->timestamp;
> +		event->shadow_ctx_time = event->tstamp - ctx->timestamp;
>  }
>  
>  #define MAX_INTERRUPTS (~0ULL)
> @@ -2063,7 +2023,6 @@ event_sched_in(struct perf_event *event,
>  		 struct perf_cpu_context *cpuctx,
>  		 struct perf_event_context *ctx)
>  {
> -	u64 tstamp = perf_event_time(event);
>  	int ret = 0;
>  
>  	lockdep_assert_held(&ctx->lock);
> @@ -2077,7 +2036,7 @@ event_sched_in(struct perf_event *event,
>  	 * is visible.
>  	 */
>  	smp_wmb();
> -	WRITE_ONCE(event->state, PERF_EVENT_STATE_ACTIVE);
> +	perf_event_set_state(event, PERF_EVENT_STATE_ACTIVE);
>  
>  	/*
>  	 * Unthrottle events, since we scheduled we might have missed several
> @@ -2089,26 +2048,19 @@ event_sched_in(struct perf_event *event,
>  		event->hw.interrupts = 0;
>  	}
>  
> -	/*
> -	 * The new state must be visible before we turn it on in the hardware:
> -	 */
> -	smp_wmb();
> -
>  	perf_pmu_disable(event->pmu);
>  
> -	perf_set_shadow_time(event, ctx, tstamp);
> +	perf_set_shadow_time(event, ctx);
>  
>  	perf_log_itrace_start(event);
>  
>  	if (event->pmu->add(event, PERF_EF_START)) {
> -		event->state = PERF_EVENT_STATE_INACTIVE;
> +		perf_event_set_state(event, PERF_EVENT_STATE_INACTIVE);
>  		event->oncpu = -1;
>  		ret = -EAGAIN;
>  		goto out;
>  	}
>  
> -	event->tstamp_running += tstamp - event->tstamp_stopped;
> -
>  	if (!is_software_event(event))
>  		cpuctx->active_oncpu++;
>  	if (!ctx->nr_active++)
> @@ -2132,8 +2084,6 @@ group_sched_in(struct perf_event *group_event,
>  {
>  	struct perf_event *event, *partial_group = NULL;
>  	struct pmu *pmu = ctx->pmu;
> -	u64 now = ctx->time;
> -	bool simulate = false;
>  
>  	if (group_event->state == PERF_EVENT_STATE_OFF)
>  		return 0;
> @@ -2163,27 +2113,13 @@ group_sched_in(struct perf_event *group_event,
>  	/*
>  	 * Groups can be scheduled in as one unit only, so undo any
>  	 * partial group before returning:
> -	 * The events up to the failed event are scheduled out normally,
> -	 * tstamp_stopped will be updated.
> -	 *
> -	 * The failed events and the remaining siblings need to have
> -	 * their timings updated as if they had gone thru event_sched_in()
> -	 * and event_sched_out(). This is required to get consistent timings
> -	 * across the group. This also takes care of the case where the group
> -	 * could never be scheduled by ensuring tstamp_stopped is set to mark
> -	 * the time the event was actually stopped, such that time delta
> -	 * calculation in update_event_times() is correct.
> +	 * The events up to the failed event are scheduled out normally.
>  	 */
>  	list_for_each_entry(event, &group_event->sibling_list, group_entry) {
>  		if (event == partial_group)
> -			simulate = true;
> +			break;
>  
> -		if (simulate) {
> -			event->tstamp_running += now - event->tstamp_stopped;
> -			event->tstamp_stopped = now;
> -		} else {
> -			event_sched_out(event, cpuctx, ctx);
> -		}
> +		event_sched_out(event, cpuctx, ctx);
>  	}
>  	event_sched_out(group_event, cpuctx, ctx);
>  
> @@ -2225,46 +2161,11 @@ static int group_can_go_on(struct perf_event *event,
>  	return can_add_hw;
>  }
>  
> -/*
> - * Complement to update_event_times(). This computes the tstamp_* values to
> - * continue 'enabled' state from @now, and effectively discards the time
> - * between the prior tstamp_stopped and now (as we were in the OFF state, or
> - * just switched (context) time base).
> - *
> - * This further assumes '@event->state == INACTIVE' (we just came from OFF) and
> - * cannot have been scheduled in yet. And going into INACTIVE state means
> - * '@event->tstamp_stopped = @now'.
> - *
> - * Thus given the rules of update_event_times():
> - *
> - *   total_time_enabled = tstamp_stopped - tstamp_enabled
> - *   total_time_running = tstamp_stopped - tstamp_running
> - *
> - * We can insert 'tstamp_stopped == now' and reverse them to compute new
> - * tstamp_* values.
> - */
> -static void __perf_event_enable_time(struct perf_event *event, u64 now)
> -{
> -	WARN_ON_ONCE(event->state != PERF_EVENT_STATE_INACTIVE);
> -
> -	event->tstamp_stopped = now;
> -	event->tstamp_enabled = now - event->total_time_enabled;
> -	event->tstamp_running = now - event->total_time_running;
> -}
> -
>  static void add_event_to_ctx(struct perf_event *event,
>  			       struct perf_event_context *ctx)
>  {
> -	u64 tstamp = perf_event_time(event);
> -
>  	list_add_event(event, ctx);
>  	perf_group_attach(event);
> -	/*
> -	 * We can be called with event->state == STATE_OFF when we create with
> -	 * .disabled = 1. In that case the IOC_ENABLE will call this function.
> -	 */
> -	if (event->state == PERF_EVENT_STATE_INACTIVE)
> -		__perf_event_enable_time(event, tstamp);
>  }
>  
>  static void ctx_sched_out(struct perf_event_context *ctx,
> @@ -2496,28 +2397,6 @@ perf_install_in_context(struct perf_event_context *ctx,
>  }
>  
>  /*
> - * Put a event into inactive state and update time fields.
> - * Enabling the leader of a group effectively enables all
> - * the group members that aren't explicitly disabled, so we
> - * have to update their ->tstamp_enabled also.
> - * Note: this works for group members as well as group leaders
> - * since the non-leader members' sibling_lists will be empty.
> - */
> -static void __perf_event_mark_enabled(struct perf_event *event)
> -{
> -	struct perf_event *sub;
> -	u64 tstamp = perf_event_time(event);
> -
> -	event->state = PERF_EVENT_STATE_INACTIVE;
> -	__perf_event_enable_time(event, tstamp);
> -	list_for_each_entry(sub, &event->sibling_list, group_entry) {
> -		/* XXX should not be > INACTIVE if event isn't */
> -		if (sub->state >= PERF_EVENT_STATE_INACTIVE)
> -			__perf_event_enable_time(sub, tstamp);
> -	}
> -}
> -
> -/*
>   * Cross CPU call to enable a performance event
>   */
>  static void __perf_event_enable(struct perf_event *event,
> @@ -2535,14 +2414,12 @@ static void __perf_event_enable(struct perf_event *event,
>  	if (ctx->is_active)
>  		ctx_sched_out(ctx, cpuctx, EVENT_TIME);
>  
> -	__perf_event_mark_enabled(event);
> +	perf_event_set_state(event, PERF_EVENT_STATE_INACTIVE);
>  
>  	if (!ctx->is_active)
>  		return;
>  
>  	if (!event_filter_match(event)) {
> -		if (is_cgroup_event(event))
> -			perf_cgroup_defer_enabled(event);
>  		ctx_sched_in(ctx, cpuctx, EVENT_TIME, current);
>  		return;
>  	}
> @@ -2862,18 +2739,10 @@ static void __perf_event_sync_stat(struct perf_event *event,
>  	 * we know the event must be on the current CPU, therefore we
>  	 * don't need to use it.
>  	 */
> -	switch (event->state) {
> -	case PERF_EVENT_STATE_ACTIVE:
> +	if (event->state == PERF_EVENT_STATE_ACTIVE)
>  		event->pmu->read(event);
> -		/* fall-through */
>  
> -	case PERF_EVENT_STATE_INACTIVE:
> -		update_event_times(event);
> -		break;
> -
> -	default:
> -		break;
> -	}
> +	perf_event_update_time(event);
>  
>  	/*
>  	 * In order to keep per-task stats reliable we need to flip the event
> @@ -3110,10 +2979,6 @@ ctx_pinned_sched_in(struct perf_event_context *ctx,
>  		if (!event_filter_match(event))
>  			continue;
>  
> -		/* may need to reset tstamp_enabled */
> -		if (is_cgroup_event(event))
> -			perf_cgroup_mark_enabled(event, ctx);
> -
>  		if (group_can_go_on(event, cpuctx, 1))
>  			group_sched_in(event, cpuctx, ctx);
>  
> @@ -3121,10 +2986,8 @@ ctx_pinned_sched_in(struct perf_event_context *ctx,
>  		 * If this pinned group hasn't been scheduled,
>  		 * put it in error state.
>  		 */
> -		if (event->state == PERF_EVENT_STATE_INACTIVE) {
> -			update_group_times(event);
> -			event->state = PERF_EVENT_STATE_ERROR;
> -		}
> +		if (event->state == PERF_EVENT_STATE_INACTIVE)
> +			perf_event_set_state(event, PERF_EVENT_STATE_ERROR);
>  	}
>  }
>  
> @@ -3146,10 +3009,6 @@ ctx_flexible_sched_in(struct perf_event_context *ctx,
>  		if (!event_filter_match(event))
>  			continue;
>  
> -		/* may need to reset tstamp_enabled */
> -		if (is_cgroup_event(event))
> -			perf_cgroup_mark_enabled(event, ctx);
> -
>  		if (group_can_go_on(event, cpuctx, can_add_hw)) {
>  			if (group_sched_in(event, cpuctx, ctx))
>  				can_add_hw = 0;
> @@ -3541,7 +3400,7 @@ static int event_enable_on_exec(struct perf_event *event,
>  	if (event->state >= PERF_EVENT_STATE_INACTIVE)
>  		return 0;
>  
> -	__perf_event_mark_enabled(event);
> +	perf_event_set_state(event, PERF_EVENT_STATE_INACTIVE);
>  
>  	return 1;
>  }
> @@ -3590,12 +3449,6 @@ static void perf_event_enable_on_exec(int ctxn)
>  		put_ctx(clone_ctx);
>  }
>  
> -struct perf_read_data {
> -	struct perf_event *event;
> -	bool group;
> -	int ret;
> -};
> -
>  static int __perf_event_read_cpu(struct perf_event *event, int event_cpu)
>  {
>  	u16 local_pkg, event_pkg;
> @@ -3613,64 +3466,6 @@ static int __perf_event_read_cpu(struct perf_event *event, int event_cpu)
>  	return event_cpu;
>  }
>  
> -/*
> - * Cross CPU call to read the hardware event
> - */
> -static void __perf_event_read(void *info)
> -{
> -	struct perf_read_data *data = info;
> -	struct perf_event *sub, *event = data->event;
> -	struct perf_event_context *ctx = event->ctx;
> -	struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
> -	struct pmu *pmu = event->pmu;
> -
> -	/*
> -	 * If this is a task context, we need to check whether it is
> -	 * the current task context of this cpu.  If not it has been
> -	 * scheduled out before the smp call arrived.  In that case
> -	 * event->count would have been updated to a recent sample
> -	 * when the event was scheduled out.
> -	 */
> -	if (ctx->task && cpuctx->task_ctx != ctx)
> -		return;
> -
> -	raw_spin_lock(&ctx->lock);
> -	if (ctx->is_active) {
> -		update_context_time(ctx);
> -		update_cgrp_time_from_event(event);
> -	}
> -
> -	update_event_times(event);
> -	if (event->state != PERF_EVENT_STATE_ACTIVE)
> -		goto unlock;
> -
> -	if (!data->group) {
> -		pmu->read(event);
> -		data->ret = 0;
> -		goto unlock;
> -	}
> -
> -	pmu->start_txn(pmu, PERF_PMU_TXN_READ);
> -
> -	pmu->read(event);
> -
> -	list_for_each_entry(sub, &event->sibling_list, group_entry) {
> -		update_event_times(sub);
> -		if (sub->state == PERF_EVENT_STATE_ACTIVE) {
> -			/*
> -			 * Use sibling's PMU rather than @event's since
> -			 * sibling could be on different (eg: software) PMU.
> -			 */
> -			sub->pmu->read(sub);
> -		}
> -	}
> -
> -	data->ret = pmu->commit_txn(pmu);
> -
> -unlock:
> -	raw_spin_unlock(&ctx->lock);
> -}
> -
>  static inline u64 perf_event_count(struct perf_event *event)
>  {
>  	return local64_read(&event->count) + atomic64_read(&event->child_count);
> @@ -3733,63 +3528,81 @@ int perf_event_read_local(struct perf_event *event, u64 *value)
>  	return ret;
>  }
>  
> -static int perf_event_read(struct perf_event *event, bool group)
> +struct perf_read_data {
> +	struct perf_event *event;
> +	bool group;
> +	int ret;
> +};
> +
> +static void __perf_event_read(struct perf_event *event,
> +			      struct perf_cpu_context *cpuctx,
> +			      struct perf_event_context *ctx,
> +			      void *data)
>  {
> -	int event_cpu, ret = 0;
> +	struct perf_read_data *prd = data;
> +	struct pmu *pmu = event->pmu;
> +	struct perf_event *sibling;
>  
> -	/*
> -	 * If event is enabled and currently active on a CPU, update the
> -	 * value in the event structure:
> -	 */
> -	if (event->state == PERF_EVENT_STATE_ACTIVE) {
> -		struct perf_read_data data = {
> -			.event = event,
> -			.group = group,
> -			.ret = 0,
> -		};
> +	if (ctx->is_active & EVENT_TIME) {
> +		update_context_time(ctx);
> +		update_cgrp_time_from_cpuctx(cpuctx);
> +	}
>  
> -		event_cpu = READ_ONCE(event->oncpu);
> -		if ((unsigned)event_cpu >= nr_cpu_ids)
> -			return 0;
> +	perf_event_update_time(event);
> +	if (prd->group)
> +		perf_event_update_sibling_time(event);
>  
> -		preempt_disable();
> -		event_cpu = __perf_event_read_cpu(event, event_cpu);
> +	if (event->state != PERF_EVENT_STATE_ACTIVE)
> +		return;
>  
> +	if (!prd->group) {
> +		pmu->read(event);
> +		prd->ret = 0;
> +		return;
> +	}
> +
> +	pmu->start_txn(pmu, PERF_PMU_TXN_READ);
> +
> +	pmu->read(event);
> +	list_for_each_entry(sibling, &event->sibling_list, group_entry) {
> +		if (sibling->state == PERF_EVENT_STATE_ACTIVE) {
> +			/*
> +			 * Use sibling's PMU rather than @event's since
> +			 * sibling could be on different (eg: software) PMU.
> +			 */
> +			sibling->pmu->read(sibling);
> +		}
> +	}
> +
> +	prd->ret = pmu->commit_txn(pmu);
> +}
> +
> +static int perf_event_read(struct perf_event *event, bool group)
> +{
> +	struct perf_read_data prd = {
> +		.event = event,
> +		.group = group,
> +		.ret = 0,
> +	};
> +
> +	if (event->ctx->task) {
> +		event_function_call(event, __perf_event_read, &prd);
> +	} else {
>  		/*
> -		 * Purposely ignore the smp_call_function_single() return
> -		 * value.
> -		 *
> -		 * If event_cpu isn't a valid CPU it means the event got
> -		 * scheduled out and that will have updated the event count.
> -		 *
> -		 * Therefore, either way, we'll have an up-to-date event count
> -		 * after this.
> -		 */
> -		(void)smp_call_function_single(event_cpu, __perf_event_read, &data, 1);
> -		preempt_enable();
> -		ret = data.ret;
> -	} else if (event->state == PERF_EVENT_STATE_INACTIVE) {
> -		struct perf_event_context *ctx = event->ctx;
> -		unsigned long flags;
> -
> -		raw_spin_lock_irqsave(&ctx->lock, flags);
> -		/*
> -		 * may read while context is not active
> -		 * (e.g., thread is blocked), in that case
> -		 * we cannot update context time
> +		 * For uncore events (which are per definition per-cpu)
> +		 * allow a different read CPU from event->cpu.
>  		 */
> -		if (ctx->is_active) {
> -			update_context_time(ctx);
> -			update_cgrp_time_from_event(event);
> -		}
> -		if (group)
> -			update_group_times(event);
> -		else
> -			update_event_times(event);
> -		raw_spin_unlock_irqrestore(&ctx->lock, flags);
> +		struct event_function_struct efs = {
> +			.event = event,
> +			.func = __perf_event_read,
> +			.data = &prd,
> +		};
> +		int cpu = __perf_event_read_cpu(event, event->cpu);
> +
> +		cpu_function_call(cpu, event_function, &efs);
>  	}
>  
> -	return ret;
> +	return prd.ret;
>  }
>  
>  /*
> @@ -4388,7 +4201,7 @@ static int perf_release(struct inode *inode, struct file *file)
>  	return 0;
>  }
>  
> -u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running)
> +static u64 __perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running)
>  {
>  	struct perf_event *child;
>  	u64 total = 0;
> @@ -4416,6 +4229,18 @@ u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running)
>  
>  	return total;
>  }
> +
> +u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running)
> +{
> +	struct perf_event_context *ctx;
> +	u64 count;
> +
> +	ctx = perf_event_ctx_lock(event);
> +	count = __perf_event_read_value(event, enabled, running);
> +	perf_event_ctx_unlock(event, ctx);
> +
> +	return count;
> +}
>  EXPORT_SYMBOL_GPL(perf_event_read_value);
>  
>  static int __perf_read_group_add(struct perf_event *leader,
> @@ -4431,6 +4256,8 @@ static int __perf_read_group_add(struct perf_event *leader,
>  	if (ret)
>  		return ret;
>  
> +	raw_spin_lock_irqsave(&ctx->lock, flags);
> +
>  	/*
>  	 * Since we co-schedule groups, {enabled,running} times of siblings
>  	 * will be identical to those of the leader, so we only publish one
> @@ -4453,8 +4280,6 @@ static int __perf_read_group_add(struct perf_event *leader,
>  	if (read_format & PERF_FORMAT_ID)
>  		values[n++] = primary_event_id(leader);
>  
> -	raw_spin_lock_irqsave(&ctx->lock, flags);
> -
>  	list_for_each_entry(sub, &leader->sibling_list, group_entry) {
>  		values[n++] += perf_event_count(sub);
>  		if (read_format & PERF_FORMAT_ID)
> @@ -4518,7 +4343,7 @@ static int perf_read_one(struct perf_event *event,
>  	u64 values[4];
>  	int n = 0;
>  
> -	values[n++] = perf_event_read_value(event, &enabled, &running);
> +	values[n++] = __perf_event_read_value(event, &enabled, &running);
>  	if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
>  		values[n++] = enabled;
>  	if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)
> @@ -4897,8 +4722,7 @@ static void calc_timer_values(struct perf_event *event,
>  
>  	*now = perf_clock();
>  	ctx_time = event->shadow_ctx_time + *now;
> -	*enabled = ctx_time - event->tstamp_enabled;
> -	*running = ctx_time - event->tstamp_running;
> +	__perf_update_times(event, ctx_time, enabled, running);
>  }
>  
>  static void perf_event_init_userpage(struct perf_event *event)
> @@ -10516,7 +10340,7 @@ perf_event_exit_event(struct perf_event *child_event,
>  	if (parent_event)
>  		perf_group_detach(child_event);
>  	list_del_event(child_event, child_ctx);
> -	child_event->state = PERF_EVENT_STATE_EXIT; /* is_event_hup() */
> +	perf_event_set_state(child_event, PERF_EVENT_STATE_EXIT); /* is_event_hup() */
>  	raw_spin_unlock_irq(&child_ctx->lock);
>  
>  	/*
> @@ -10754,7 +10578,7 @@ inherit_event(struct perf_event *parent_event,
>  	      struct perf_event *group_leader,
>  	      struct perf_event_context *child_ctx)
>  {
> -	enum perf_event_active_state parent_state = parent_event->state;
> +	enum perf_event_state parent_state = parent_event->state;
>  	struct perf_event *child_event;
>  	unsigned long flags;
>  
> @@ -11090,6 +10914,7 @@ static void __perf_event_exit_context(void *__info)
>  	struct perf_event *event;
>  
>  	raw_spin_lock(&ctx->lock);
> +	ctx_sched_out(ctx, cpuctx, EVENT_TIME);
>  	list_for_each_entry(event, &ctx->event_list, event_entry)
>  		__perf_remove_from_context(event, cpuctx, ctx, (void *)DETACH_GROUP);
>  	raw_spin_unlock(&ctx->lock);
> 

[toc] | [prev] | [next] | [standalone]

#1725024

On Fri, Sep 01, 2017 at 02:17:17PM +0300, Alexey Budankov wrote:
> > No more weird and wonderful mind bending interaction between 3 different
> > timestamps with arcane update rules.
> > 
> > ---
> >  include/linux/perf_event.h |  25 +-
> >  kernel/events/core.c       | 551 ++++++++++++++++-----------------------------
> >  2 files changed, 192 insertions(+), 384 deletions(-)
> > 
> 
> Tried to apply on top of this:
> 
> perf/core 1b2f76d77a277bb70d38ad0991ed7f16bbc115a9 [origin/perf/core] Merge tag 'perf-core-for-mingo-4.14-20170829' of git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux into perf/core

Applies on top of tip/master without issue.

[toc] | [prev] | [next] | [standalone]

#1725349

On Thu, 31 Aug 2017, Peter Zijlstra wrote:

> So the below completely rewrites timekeeping (and probably breaks
> world) but does away with the need to touch events that don't get
> scheduled.
> 
> Esp the cgroup stuff is entirely untested since I simply don't know how
> to operate that. I did run Vince's tests on it, and I think it doesn't
> regress, but I'm near a migraine so I can't really see straight atm.
> 
> Vince, Stephane, could you guys have a peek?

I have to admit that I *always* got lost trying to figure out the old 
so I might not be the best person to review the changes.

I did try running the perf_event_tests on a few machines and they all pass.

I also ran the PAPI tests and a few of the multiplexing tests fail about 
10% of the time but I think they also fail 10% of the time with the old 
code too.  I need to figure out why that's happening but it's likely a 
PAPI issue not a kernel one.

Vince

[toc] | [prev] | [next] | [standalone]

#1725992

Hi,
On 31.08.2017 20:18, Peter Zijlstra wrote:
> On Wed, Aug 23, 2017 at 11:54:15AM +0300, Alexey Budankov wrote:
>> On 22.08.2017 23:47, Peter Zijlstra wrote:
>>> On Thu, Aug 10, 2017 at 06:57:43PM +0300, Alexey Budankov wrote:
>>>> The key thing in the patch is explicit updating of tstamp fields for
>>>> INACTIVE events in update_event_times().
>>>
>>>> @@ -1405,6 +1426,9 @@ static void update_event_times(struct perf_event *event)
>>>>  	    event->group_leader->state < PERF_EVENT_STATE_INACTIVE)
>>>>  		return;
>>>>  
>>>> +	if (event->state == PERF_EVENT_STATE_INACTIVE)
>>>> +		perf_event_tstamp_update(event);
>>>> +
>>>>  	/*
>>>>  	 * in cgroup mode, time_enabled represents
>>>>  	 * the time the event was enabled AND active
>>>
>>> But why!? I thought the whole point was to not need to do this.
>>
>> update_event_times() is not called from timer interrupt handler 
>> thus it is not on the critical path which is optimized in this patch set.
>>
>> But update_event_times() is called in the context of read() syscall so
>> this is the place where we may update event times for INACTIVE events 
>> instead of timer interrupt.
>>
>> Also update_event_times() is called on thread context switch out so
>> we get event times also updated when the thread migrates to other CPU.
>>
>>>
>>> The thing I outlined earlier would only need to update timestamps when
>>> events change state and at no other point in time.
>>
>> But we still may request times while event is in INACTIVE state 
>> thru read() syscall and event timings need to be up-to-date. 
> 
> Sure, read() also updates.
> 
> So the below completely rewrites timekeeping (and probably breaks
> world) but does away with the need to touch events that don't get
> scheduled.

We still need and do iterate thru all events at some points e.g. on context switches.

> 
> Esp the cgroup stuff is entirely untested since I simply don't know how
> to operate that. I did run Vince's tests on it, and I think it doesn't
> regress, but I'm near a migraine so I can't really see straight atm.
> 
> Vince, Stephane, could you guys have a peek?
> 
> (There's a few other bits in, I'll break up into patches and write
> comments and Changelogs later, I think its can be split in some 5
> patches).
> 
> The basic idea is really simple, we have a single timestamp and
> depending on the state we update enabled/running. This obviously only
> requires updates when we change state and when we need up-to-date
> timestamps (read).

I would prefer to have this rework in a FSM similar to that below, 
so state transition and the corresponding tstamp, total_time_enabled 
and total_time_running manipulation logic would be consolidated in 
one place and adjacent lines of code.

From the table below event->state FSM is not as simple as it may seem 
on the first sight so in order to avoid regressions after rework we 
better keep that in mind and explicitly implement allowed and disallowed
state transitions.

    A	  	I	    O	       E	   X	      D          U

A   Te+,Tr+     Te+,Tr+     Te+,Tr+    Te+,Tr+     Te+,Tr+    Te+,Tr+    ---
    ts 	        ts          ts         ts          ts         ts

I   Te+,ts      Te+,ts      Te+,ts     Te+,ts      Te+,ts     Te+,ts     ---

O   Te=0,Tr=0,  Te=0,Tr=0,  Te=0,Tr=0  Te=0,Tr=0   Te=0,Tr=0  Te=0,Tr=0  ---
    ts          ts          ts         ts          ts         ts

E   Te=0,Tr=0,  Te=0,Tr=0,  Te=0,Tr=0  Te=0,Tr=0   Te=0,Tr=0  Te=0,Tr=0  ---
    ts          ts          ts         ts          ts         ts

X   ---         ---         ---        ---         ---        ---        ---

D   ---         ---         ---        ---         ---        ---        ---

U   ---         Te=0,Tr=0   Te=0,Tr=0  ---         ---        ---        ---
                ts          ts          

LEGEND:

U - allocation, A - ACTIVE, I - INACTIVE, O - OFF, 
E - ERROR, X - EXIT, D - DEAD,

Te=0  - event->total_time_enabled  = 0
Te+   - event->total_time_enabled += delta

Tr=0  - event->total_time_running  = 0
Tr+   - event->total_time_running += delta

ts    - event->tstamp = perf_event_time(event)

static void
perf_event_change_state(struct perf_event *event, enum perf_event_state state)
{
        u64 delta = 0;
	u64 now = perf_event_time(event);

        delta = now - event->tstamp;
	event->tstamp = now;

	switch(event->state)
	{
	case A:
		switch(state)
		{
		case A:
                        ...
			break;
		case I:
			event->total_time_enabled += delta;
			event->total_time_running += delta;
			event->state = state;
			break;
		case O:
			...
			break;
		case E:
			...
	...
	case I:
		...
		break;
	...
	}
}

---

Regards,
Alexey

[toc] | [prev] | [next] | [standalone]

#1726023

On Mon, Sep 04, 2017 at 01:46:45PM +0300, Alexey Budankov wrote:
> > So the below completely rewrites timekeeping (and probably breaks
> > world) but does away with the need to touch events that don't get
> > scheduled.
> 
> We still need and do iterate thru all events at some points e.g. on context switches.

Why do we _need_ to? On ctx switch we should stop iteration for a PMU
once we fail to schedule an event, same as for rotation.

> > The basic idea is really simple, we have a single timestamp and
> > depending on the state we update enabled/running. This obviously only
> > requires updates when we change state and when we need up-to-date
> > timestamps (read).
> 
> I would prefer to have this rework in a FSM similar to that below, 
> so state transition and the corresponding tstamp, total_time_enabled 
> and total_time_running manipulation logic would be consolidated in 
> one place and adjacent lines of code.
> 
> From the table below event->state FSM is not as simple as it may seem 
> on the first sight so in order to avoid regressions after rework we 
> better keep that in mind and explicitly implement allowed and disallowed
> state transitions.

Maybe if we introduce something like CONFIG_PERF_DEBUG, but I fear that
for normal operation that's all fairly horrible overhead.

>     A	  	I	    O	       E	   X	      D          U
> 
> A   Te+,Tr+     Te+,Tr+     Te+,Tr+    Te+,Tr+     Te+,Tr+    Te+,Tr+    ---
>     ts 	        ts          ts         ts          ts         ts
> 
> I   Te+,ts      Te+,ts      Te+,ts     Te+,ts      Te+,ts     Te+,ts     ---
> 
> O   Te=0,Tr=0,  Te=0,Tr=0,  Te=0,Tr=0  Te=0,Tr=0   Te=0,Tr=0  Te=0,Tr=0  ---
>     ts          ts          ts         ts          ts         ts
> 
> E   Te=0,Tr=0,  Te=0,Tr=0,  Te=0,Tr=0  Te=0,Tr=0   Te=0,Tr=0  Te=0,Tr=0  ---
>     ts          ts          ts         ts          ts         ts
> 
> X   ---         ---         ---        ---         ---        ---        ---
> 
> D   ---         ---         ---        ---         ---        ---        ---
> 
> U   ---         Te=0,Tr=0   Te=0,Tr=0  ---         ---        ---        ---
>                 ts          ts          
> 
> LEGEND:
> 
> U - allocation, A - ACTIVE, I - INACTIVE, O - OFF, 
> E - ERROR, X - EXIT, D - DEAD,

Not sure we care about the different <0 values, they're all effectively
OFF.

[toc] | [prev] | [next] | [standalone]

#1726131

On 04.09.2017 15:08, Peter Zijlstra wrote:
> On Mon, Sep 04, 2017 at 01:46:45PM +0300, Alexey Budankov wrote:
>>> So the below completely rewrites timekeeping (and probably breaks
>>> world) but does away with the need to touch events that don't get
>>> scheduled.
>>
>> We still need and do iterate thru all events at some points e.g. on context switches.
> 
> Why do we _need_ to?

We do so in the current implementation with several tstamp_* fields.

> On ctx switch we should stop iteration for a PMU once we fail toschedule an event, same as for rotation> 
>>> The basic idea is really simple, we have a single timestamp and
>>> depending on the state we update enabled/running. This obviously only
>>> requires updates when we change state and when we need up-to-date
>>> timestamps (read).
>>
>> I would prefer to have this rework in a FSM similar to that below, 
>> so state transition and the corresponding tstamp, total_time_enabled 
>> and total_time_running manipulation logic would be consolidated in 
>> one place and adjacent lines of code.
>>
>> From the table below event->state FSM is not as simple as it may seem 
>> on the first sight so in order to avoid regressions after rework we 
>> better keep that in mind and explicitly implement allowed and disallowed
>> state transitions.
> 
> Maybe if we introduce something like CONFIG_PERF_DEBUG, but I fear that
> for normal operation that's all fairly horrible overhead.
> 
>>     A	  	I	    O	       E	   X	      D     U
>>
>> A   Te+,Tr+     Te+,Tr+     Te+,Tr+    Te+,Tr+     Te+,Tr+    Te+,Tr+    ---
>>     ts 	        ts          ts         ts          ts         ts
>>
>> I   Te+,ts      Te+,ts      Te+,ts     Te+,ts      Te+,ts     Te+,ts     ---
>>
>> O   Te=0,Tr=0,  Te=0,Tr=0,  Te=0,Tr=0  Te=0,Tr=0   Te=0,Tr=0  Te=0,Tr=0  ---
>>     ts          ts          ts         ts          ts         ts
>>
>> E   Te=0,Tr=0,  Te=0,Tr=0,  Te=0,Tr=0  Te=0,Tr=0   Te=0,Tr=0  Te=0,Tr=0  ---
>>     ts          ts          ts         ts          ts         ts
>>
>> X   ---         ---         ---        ---         ---        ---        ---
>>
>> D   ---         ---         ---        ---         ---        ---        ---
>>
>> U   ---         Te=0,Tr=0   Te=0,Tr=0  ---         ---        ---        ---
>>                 ts          ts          
>>
>> LEGEND:
>>
>> U - allocation, A - ACTIVE, I - INACTIVE, O - OFF, 
>> E - ERROR, X - EXIT, D - DEAD,
> 
> Not sure we care about the different <0 values, they're all effectively
> OFF.

We still need to care about proper initial state of timings when moving above >=0 state.

> 
> 
> 

[toc] | [prev] | [next] | [standalone]

#1726156

On Mon, Sep 04, 2017 at 05:56:06PM +0300, Alexey Budankov wrote:
> On 04.09.2017 15:08, Peter Zijlstra wrote:
> > On Mon, Sep 04, 2017 at 01:46:45PM +0300, Alexey Budankov wrote:
> >>> So the below completely rewrites timekeeping (and probably breaks
> >>> world) but does away with the need to touch events that don't get
> >>> scheduled.
> >>
> >> We still need and do iterate thru all events at some points e.g. on context switches.
> > 
> > Why do we _need_ to?
> 
> We do so in the current implementation with several tstamp_* fields.

Right, but we want to stop doing so asap :-)


> >> U - allocation, A - ACTIVE, I - INACTIVE, O - OFF, 
> >> E - ERROR, X - EXIT, D - DEAD,
> > 
> > Not sure we care about the different <0 values, they're all effectively
> > OFF.
> 
> We still need to care about proper initial state of timings when moving above >=0 state.

Very true. I'm not sure I fully covered that, let me see if there's
something sensible to do for that.

[toc] | [prev] | [next] | [standalone]

#1726160

On Mon, Sep 04, 2017 at 05:41:45PM +0200, Peter Zijlstra wrote:

> > >> U - allocation, A - ACTIVE, I - INACTIVE, O - OFF, 
> > >> E - ERROR, X - EXIT, D - DEAD,
> > > 
> > > Not sure we care about the different <0 values, they're all effectively
> > > OFF.
> > 
> > We still need to care about proper initial state of timings when moving above >=0 state.
> 
> Very true. I'm not sure I fully covered that, let me see if there's
> something sensible to do for that.


So given this:

static __always_inline enum perf_event_state
__perf_effective_state(struct perf_event *event)
{
	struct perf_event *leader = event->group_leader;

	if (leader->state <= PERF_EVENT_STATE_OFF)
		return leader->state;

	return event->state;
}

static __always_inline void
__perf_update_times(struct perf_event *event, u64 now, u64 *enabled, u64 *running)
{
	enum perf_event_state state = __perf_effective_state(event);
	u64 delta = now - event->tstamp;

	*enabled = event->total_time_enabled;
	if (state >= PERF_EVENT_STATE_INACTIVE)
		*enabled += delta;

	*running = event->total_time_running;
	if (state >= PERF_EVENT_STATE_ACTIVE)
		*running += delta;
}

static void perf_event_update_time(struct perf_event *event)
{
	u64 now = perf_event_time(event);

	__perf_update_times(event, now, &event->total_time_enabled,
					&event->total_time_running);
	event->tstamp = now;
}

static void perf_event_update_sibling_time(struct perf_event *leader)
{
	struct perf_event *sibling;

	list_for_each_entry(sibling, &leader->sibling_list, group_entry)
		perf_event_update_time(sibling);
}

static void
perf_event_set_state(struct perf_event *event, enum perf_event_state state)
{
	if (event->state == state)
		return;

	perf_event_update_time(event);
	/*
	 * If a group leader gets enabled/disabled all its siblings
	 * are affected too.
	 */
	if ((event->state < 0) ^ (state < 0))
		perf_event_update_sibling_time(event);

	WRITE_ONCE(event->state, state);
}


If event->state < 0, and we do perf_event_set_state(event, INACTIVE)
then perf_event_update_time() will not add to enabled, not add to
running, but set ->tstamp = now.

So I think it DTRT.

[toc] | [prev] | [next] | [standalone]

#1726637

On Tue, Sep 05, 2017 at 01:17:39PM +0300, Alexey Budankov wrote:
> However we can't completely get rid of whole tree iterations because of 
> inheritance code on forks in perf_event_init_context() here:

Right, fork() / inherit needs to iterate the full thing, nothing to be
done about that.

I'll go make proper patches for that timekeeping rewrite and then have a
look at your patches.

[toc] | [prev] | [next] | [standalone]

#1726651

Hi,
On 05.09.2017 13:17, Alexey Budankov wrote:
> On 04.09.2017 18:41, Peter Zijlstra wrote:
>> On Mon, Sep 04, 2017 at 05:56:06PM +0300, Alexey Budankov wrote:
>>> On 04.09.2017 15:08, Peter Zijlstra wrote:
>>>> On Mon, Sep 04, 2017 at 01:46:45PM +0300, Alexey Budankov wrote:
>>>>>> So the below completely rewrites timekeeping (and probably breaks
>>>>>> world) but does away with the need to touch events that don't get
>>>>>> scheduled.
>>>>>
>>>>> We still need and do iterate thru all events at some points e.g. on context switches.
>>>>
>>>> Why do we _need_ to?
>>>
>>> We do so in the current implementation with several tstamp_* fields.
>>
>> Right, but we want to stop doing so asap :-)
>>
> 
> Well, I see you point :). It turns out that with straightforward timekeeping 
> we can also avoid whole tree iteration on context switches additionally to RB 
> tree based iterations and rotations on hrtimer interrupt. That brings even more 
> performance and rotation switch can be avoided.
> 
> However we can't completely get rid of whole tree iterations because of 
> inheritance code on forks in perf_event_init_context() here:
> 
> perf_event_groups_for_each(event, &parent_ctx->pinned_groups, group_node) {
> 		ret = inherit_task_group(event, parent, parent_ctx,
> 					 child, ctxn, &inherited_all);
> 		if (ret)
> 			goto out_unlock;
> }
> 
> and here:
> 
> perf_event_groups_for_each(event, &parent_ctx->flexible_groups, group_node) {
> 		ret = inherit_task_group(event, parent, parent_ctx,
> 					 child, ctxn, &inherited_all);
> 		if (ret)
> 			goto out_unlock;
> }
> 
> Below is the patch set put on top of the timekeeping rework. 
> 
> It is for tip/master branch.
> 
> ---
>  include/linux/perf_event.h |  40 +--
>  kernel/events/core.c       | 839 +++++++++++++++++++++++----------------------
>  2 files changed, 448 insertions(+), 431 deletions(-)
> 

Got this under perf_fuzzer on Xeon Phi (KNL):

[ 6614.226280] ------------[ cut here ]------------
[ 6614.226305] WARNING: CPU: 45 PID: 43385 at kernel/events/core.c:239 event_function+0xb3/0xe0
[ 6614.226310] Modules linked in: btrfs xor raid6_pq ufs hfsplus hfs minix vfat msdos fat jfs xfs reiserfs binfmt_misc xt_CHECKSUM iptable_mangle fuse ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack libcrc32c tun bridge stp llc ebtable_filter ebtables ip6table_filter ip6_tables cmac arc4 md4 nls_utf8 nfsv3 cifs rpcsec_gss_krb5 nfsv4 nfs ccm dns_resolver fscache rpcrdma ib_isert iscsi_target_mod ib_iser libiscsi scsi_transport_iscsi ib_srpt target_core_mod ib_srp scsi_transport_srp ib_ipoib rdma_ucm ib_ucm ib_uverbs ib_umad rdma_cm ib_cm iw_cm intel_rapl hfi1 sb_edac nfsd x86_pkg_temp_thermal intel_powerclamp rdmavt ipmi_ssif coretemp iTCO_wdt joydev ib_core crct10dif_pclmul iTCO_vendor_support crc32_pclmul ipmi_si ghash_clmulni_intel
[ 6614.226444]  auth_rpcgss ipmi_devintf mei_me tpm_tis intel_cstate mei tpm_tis_core intel_uncore pcspkr intel_rapl_perf tpm ipmi_msghandler shpchp nfs_acl lpc_ich lockd i2c_i801 wmi grace acpi_power_meter acpi_pad sunrpc mgag200 drm_kms_helper ttm drm igb crc32c_intel ptp pps_core dca i2c_algo_bit
[ 6614.226501] CPU: 45 PID: 43385 Comm: perf_fuzzer Not tainted 4.13.0-v11.1.2+ #5
[ 6614.226506] Hardware name: Intel Corporation S7200AP/S7200AP, BIOS S72C610.86B.01.01.0190.080520162104 08/05/2016
[ 6614.226511] task: ffff8d0f5866c000 task.stack: ffffa6b05aae8000
[ 6614.226517] RIP: 0010:event_function+0xb3/0xe0
[ 6614.226522] RSP: 0018:ffffa6b05aaebc30 EFLAGS: 00010087
[ 6614.226528] RAX: 0000000000000000 RBX: ffffc6b03c545a90 RCX: ffffa6b05aaebd00
[ 6614.226532] RDX: 0000000000000000 RSI: ffffa6b05aaebca0 RDI: ffffc6b03c545a98
[ 6614.226536] RBP: ffffa6b05aaebc58 R08: 000000000001f8e0 R09: ffff8d0fa99fd120
[ 6614.226540] R10: ffff8d0fbb407900 R11: 0000000000000000 R12: ffffc6b03ba05a90
[ 6614.226544] R13: 0000000000000000 R14: ffffa6b05aaebd48 R15: ffff8d0f9c48a000
[ 6614.226550] FS:  00007f05cff85740(0000) GS:ffff8d0fbc540000(0000) knlGS:0000000000000000
[ 6614.226555] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 6614.226559] CR2: 00000000007211c0 CR3: 0000002f269fa000 CR4: 00000000001407e0
[ 6614.226563] Call Trace:
[ 6614.226577]  remote_function+0x3b/0x50
[ 6614.226585]  generic_exec_single+0x9a/0xd0
[ 6614.226592]  smp_call_function_single+0xc8/0x100
[ 6614.226599]  cpu_function_call+0x43/0x60
[ 6614.226606]  ? cpu_clock_event_read+0x10/0x10
[ 6614.226612]  perf_event_read+0xc7/0xe0
[ 6614.226619]  ? perf_install_in_context+0xf0/0xf0
[ 6614.226625]  __perf_read_group_add+0x25/0x180
[ 6614.226632]  perf_read+0xcb/0x2b0
[ 6614.226640]  __vfs_read+0x37/0x160
[ 6614.226648]  ? security_file_permission+0x9d/0xc0
[ 6614.226655]  vfs_read+0x8c/0x130
[ 6614.226661]  SyS_read+0x55/0xc0
[ 6614.226670]  do_syscall_64+0x67/0x180
[ 6614.226678]  entry_SYSCALL64_slow_path+0x25/0x25
[ 6614.226684] RIP: 0033:0x7f05cfaad980
[ 6614.226688] RSP: 002b:00007fff1b562b48 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
[ 6614.226694] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f05cfaad980
[ 6614.226699] RDX: 0000000000004fcf RSI: 0000000000735680 RDI: 0000000000000003
[ 6614.226703] RBP: 00007fff1b562b60 R08: 00007f05cfd800f4 R09: 00007f05cfd80140
[ 6614.226707] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000401980
[ 6614.226711] R13: 00007fff1b564f60 R14: 0000000000000000 R15: 0000000000000000
[ 6614.226716] Code: e2 48 89 de 4c 89 ff 41 ff 56 08 31 c0 4d 85 ed 74 05 41 c6 45 08 00 c6 43 08 00 5b 41 5c 41 5d 41 5e 41 5f 5d c3 49 39 dc 74 cf <0f> ff eb cb 0f ff 0f 1f 80 00 00 00 00 e9 78 ff ff ff 0f ff 66 
[ 6614.226812] ---[ end trace ff12704813059a28 ]---

static int event_function(void *info)
{
	struct event_function_struct *efs = info;
	struct perf_event *event = efs->event;
	struct perf_event_context *ctx = event->ctx;
	struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
	struct perf_event_context *task_ctx = cpuctx->task_ctx;
	int ret = 0;

	WARN_ON_ONCE(!irqs_disabled());

	perf_ctx_lock(cpuctx, task_ctx);
	/*
	 * Since we do the IPI call without holding ctx->lock things can have
	 * changed, double check we hit the task we set out to hit.
	 */
	if (ctx->task) {
		if (ctx->task != current) {
			ret = -ESRCH;
			goto unlock;
		}

		/*
		 * We only use event_function_call() on established contexts,
		 * and event_function() is only ever called when active (or
		 * rather, we'll have bailed in task_function_call() or the
		 * above ctx->task != current test), therefore we must have
		 * ctx->is_active here.
		 */
		WARN_ON_ONCE(!ctx->is_active);
		/*
		 * And since we have ctx->is_active, cpuctx->task_ctx must
		 * match.
		 */
		WARN_ON_ONCE(task_ctx != ctx);
	} else {
===>		WARN_ON_ONCE(&cpuctx->ctx != ctx);
	}

	efs->func(event, cpuctx, ctx, efs->data);
unlock:
	perf_ctx_unlock(cpuctx, task_ctx);

	return ret;
}

[toc] | [prev] | [next] | [standalone]

#1726702

On Tue, Sep 05, 2017 at 03:06:26PM +0300, Alexey Budankov wrote:
> [ 6614.226305] WARNING: CPU: 45 PID: 43385 at kernel/events/core.c:239 event_function+0xb3/0xe0

> [ 6614.226563] Call Trace:
> [ 6614.226577]  remote_function+0x3b/0x50
> [ 6614.226585]  generic_exec_single+0x9a/0xd0
> [ 6614.226592]  smp_call_function_single+0xc8/0x100
> [ 6614.226599]  cpu_function_call+0x43/0x60
> [ 6614.226606]  ? cpu_clock_event_read+0x10/0x10
> [ 6614.226612]  perf_event_read+0xc7/0xe0
> [ 6614.226619]  ? perf_install_in_context+0xf0/0xf0
> [ 6614.226625]  __perf_read_group_add+0x25/0x180
> [ 6614.226632]  perf_read+0xcb/0x2b0
> [ 6614.226640]  __vfs_read+0x37/0x160
> [ 6614.226648]  ? security_file_permission+0x9d/0xc0
> [ 6614.226655]  vfs_read+0x8c/0x130
> [ 6614.226661]  SyS_read+0x55/0xc0
> [ 6614.226670]  do_syscall_64+0x67/0x180
> [ 6614.226678]  entry_SYSCALL64_slow_path+0x25/0x25


Hmm.. must be the perf_event_read() rewrite, let me stare at that.

Thanks for testing.

[toc] | [prev] | [next] | [standalone]

#1726835

On Tue, Sep 05, 2017 at 03:06:26PM +0300, Alexey Budankov wrote:
> [ 6614.226305] WARNING: CPU: 45 PID: 43385 at kernel/events/core.c:239 event_function+0xb3/0xe0

I think I avoided that problem by not radically rewriting
perf_event_read() but fixing it instead:

  https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/commit/?h=perf/core&id=8ad650955ede95e4a6fd6afbda2a0b37d4af9c29

Full tree at:

  git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git perf/core


Very minimally tested so far, I'll continue tomorrow.

[toc] | [prev] | [next] | [standalone]

#1727486

On 05.09.2017 19:03, Peter Zijlstra wrote:
> On Tue, Sep 05, 2017 at 03:06:26PM +0300, Alexey Budankov wrote:
>> [ 6614.226305] WARNING: CPU: 45 PID: 43385 at kernel/events/core.c:239 event_function+0xb3/0xe0
> 
> I think I avoided that problem by not radically rewriting
> perf_event_read() but fixing it instead:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/commit/?h=perf/core&id=8ad650955ede95e4a6fd6afbda2a0b37d4af9c29
> 
> Full tree at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git perf/core
> 
> 
> Very minimally tested so far, I'll continue tomorrow.
> 
No access to:

git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git perf/core 

for some reason. Also tried:

https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git perf/core
https://kernel.googlesource.com/pub/scm/linux/kernel/git/peterz/queue.git perf/core

with no luck.

[toc] | [prev] | [next] | [standalone]

#1728697

On 05.09.2017 19:03, Peter Zijlstra wrote:
> On Tue, Sep 05, 2017 at 03:06:26PM +0300, Alexey Budankov wrote:
>> [ 6614.226305] WARNING: CPU: 45 PID: 43385 at kernel/events/core.c:239 event_function+0xb3/0xe0
> 
> I think I avoided that problem by not radically rewriting
> perf_event_read() but fixing it instead:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/commit/?h=perf/core&id=8ad650955ede95e4a6fd6afbda2a0b37d4af9c29
> 
> Full tree at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git perf/core
> 
> 
> Very minimally tested so far, I'll continue tomorrow.
> 

The patch set v9 on top of peterz/queue perf/core repository above:

---
 include/linux/perf_event.h |  16 ++-
 kernel/events/core.c       | 307 +++++++++++++++++++++++++++++++++++++--------
 2 files changed, 267 insertions(+), 56 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 2a6ae48..92cda40 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -557,7 +557,11 @@ struct perf_event {
 	 */
 	struct list_head		group_entry;
 	struct list_head		sibling_list;
-
+	/*
+	 * Node on the pinned or flexible tree located at the event context;
+	 */
+	struct rb_node			group_node;
+	u64				group_index;
 	/*
 	 * We need storage to track the entries in perf_pmu_migrate_context; we
 	 * cannot use the event_entry because of RCU and we want to keep the
@@ -689,6 +693,12 @@ struct perf_event {
 #endif /* CONFIG_PERF_EVENTS */
 };
 
+
+struct perf_event_groups {
+	struct rb_root	tree;
+	u64		index;
+};
+
 /**
  * struct perf_event_context - event context structure
  *
@@ -709,8 +719,8 @@ struct perf_event_context {
 	struct mutex			mutex;
 
 	struct list_head		active_ctx_list;
-	struct list_head		pinned_groups;
-	struct list_head		flexible_groups;
+	struct perf_event_groups	pinned_groups;
+	struct perf_event_groups	flexible_groups;
 	struct list_head		event_list;
 	int				nr_events;
 	int				nr_active;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 56e9214..8158f1d 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1454,8 +1454,21 @@ static enum event_type_t get_event_type(struct perf_event *event)
 	return event_type;
 }
 
-static struct list_head *
-ctx_group_list(struct perf_event *event, struct perf_event_context *ctx)
+/*
+ * Helper function to initialize group leader event;
+ */
+void init_event_group(struct perf_event *event)
+{
+	RB_CLEAR_NODE(&event->group_node);
+	event->group_index = 0;
+}
+
+/*
+ * Extract pinned or flexible groups from the context
+ * based on event attrs bits;
+ */
+static struct perf_event_groups *
+get_event_groups(struct perf_event *event, struct perf_event_context *ctx)
 {
 	if (event->attr.pinned)
 		return &ctx->pinned_groups;
@@ -1464,6 +1477,169 @@ static enum event_type_t get_event_type(struct perf_event *event)
 }
 
 /*
+ * Helper function to initializes perf event groups object;
+ */
+void perf_event_groups_init(struct perf_event_groups *groups)
+{
+	groups->tree = RB_ROOT;
+	groups->index = 0;
+}
+
+/*
+ * Compare function for event groups;
+ * Implements complex key that first sorts by CPU and then by
+ * virtual index which provides ordering when rotating
+ * groups for the same CPU;
+ */
+int perf_event_groups_less(struct perf_event *left, struct perf_event *right)
+{
+	if (left->cpu < right->cpu) {
+		return 1;
+	} else if (left->cpu > right->cpu) {
+		return 0;
+	} else {
+		if (left->group_index < right->group_index) {
+			return 1;
+		} else if(left->group_index > right->group_index) {
+			return 0;
+		} else {
+			return 0;
+		}
+	}
+}
+
+/*
+ * Insert a group into a tree using event->cpu as a key. If event->cpu node
+ * is already attached to the tree then the event is added to the attached
+ * group's group_list list.
+ */
+static void
+perf_event_groups_insert(struct perf_event_groups *groups,
+		struct perf_event *event)
+{
+	struct perf_event *node_event;
+	struct rb_node *parent;
+	struct rb_node **node;
+
+	event->group_index = ++groups->index;
+
+	node = &groups->tree.rb_node;
+	parent = *node;
+
+	while (*node) {
+		parent = *node;
+		node_event = container_of(*node,
+				struct perf_event, group_node);
+
+		if (perf_event_groups_less(event, node_event))
+			node = &parent->rb_left;
+		else
+			node = &parent->rb_right;
+	}
+
+	rb_link_node(&event->group_node, parent, node);
+	rb_insert_color(&event->group_node, &groups->tree);
+}
+
+/*
+ * Helper function to insert event into the pinned or
+ * flexible groups;
+ */
+static void
+add_event_to_groups(struct perf_event *event, struct perf_event_context *ctx)
+{
+	struct perf_event_groups *groups;
+
+	groups = get_event_groups(event, ctx);
+	perf_event_groups_insert(groups, event);
+}
+
+/*
+ * Delete a group from a tree. If the group is directly attached to the tree
+ * it also detaches all groups on the group's group_list list.
+ */
+static void
+perf_event_groups_delete(struct perf_event_groups *groups,
+		struct perf_event *event)
+{
+	if (!RB_EMPTY_NODE(&event->group_node) &&
+	    !RB_EMPTY_ROOT(&groups->tree))
+		rb_erase(&event->group_node, &groups->tree);
+
+	init_event_group(event);
+}
+
+/*
+ * Helper function to delete event from its groups;
+ */
+static void
+del_event_from_groups(struct perf_event *event, struct perf_event_context *ctx)
+{
+	struct perf_event_groups *groups;
+
+	groups = get_event_groups(event, ctx);
+	perf_event_groups_delete(groups, event);
+}
+
+/*
+ * Get a group by a cpu key from groups tree with the least group_index;
+ */
+static struct perf_event *
+perf_event_groups_first(struct perf_event_groups *groups, int cpu)
+{
+	struct perf_event *node_event = NULL, *match = NULL;
+	struct rb_node *node = groups->tree.rb_node;
+
+	while (node) {
+		node_event = container_of(node,
+				struct perf_event, group_node);
+
+		if (cpu < node_event->cpu) {
+			node = node->rb_left;
+		} else if (cpu > node_event->cpu) {
+			node = node->rb_right;
+		} else {
+			match = node_event;
+			node = node->rb_left;
+		}
+	}
+
+	return match;
+}
+
+/*
+ * Find group list by a cpu key and rotate it.
+ */
+static void
+perf_event_groups_rotate(struct perf_event_groups *groups, int cpu)
+{
+	struct perf_event *event =
+			perf_event_groups_first(groups, cpu);
+
+	if (event) {
+		perf_event_groups_delete(groups, event);
+		perf_event_groups_insert(groups, event);
+	}
+}
+
+/*
+ * Iterate event groups thru the whole tree.
+ */
+#define perf_event_groups_for_each(event, groups, node)		\
+	for (event = rb_entry_safe(rb_first(&((groups)->tree)),	\
+				typeof(*event), node); event;	\
+		event = rb_entry_safe(rb_next(&event->node),	\
+				typeof(*event), node))
+/*
+ * Iterate event groups with cpu == key.
+ */
+#define perf_event_groups_for_each_cpu(event, key, groups, node) \
+	for (event = perf_event_groups_first(groups, key);	 \
+		event && event->cpu == key;			 \
+		event = rb_entry_safe(rb_next(&event->node),	 \
+				typeof(*event), node))
+
+/*
  * Add a event from the lists for its context.
  * Must be called with ctx->mutex and ctx->lock held.
  */
@@ -1483,12 +1659,8 @@ static enum event_type_t get_event_type(struct perf_event *event)
 	 * perf_group_detach can, at all times, locate all siblings.
 	 */
 	if (event->group_leader == event) {
-		struct list_head *list;
-
 		event->group_caps = event->event_caps;
-
-		list = ctx_group_list(event, ctx);
-		list_add_tail(&event->group_entry, list);
+		add_event_to_groups(event, ctx);
 	}
 
 	list_update_cgroup_event(event, ctx, true);
@@ -1682,7 +1854,7 @@ static void perf_group_attach(struct perf_event *event)
 	list_del_rcu(&event->event_entry);
 
 	if (event->group_leader == event)
-		list_del_init(&event->group_entry);
+		del_event_from_groups(event, ctx);
 
 	/*
 	 * If event was in error state, then keep it
@@ -1700,7 +1872,6 @@ static void perf_group_attach(struct perf_event *event)
 static void perf_group_detach(struct perf_event *event)
 {
 	struct perf_event *sibling, *tmp;
-	struct list_head *list = NULL;
 
 	lockdep_assert_held(&event->ctx->lock);
 
@@ -1721,22 +1892,23 @@ static void perf_group_detach(struct perf_event *event)
 		goto out;
 	}
 
-	if (!list_empty(&event->group_entry))
-		list = &event->group_entry;
-
 	/*
 	 * If this was a group event with sibling events then
 	 * upgrade the siblings to singleton events by adding them
 	 * to whatever list we are on.
 	 */
 	list_for_each_entry_safe(sibling, tmp, &event->sibling_list, group_entry) {
-		if (list)
-			list_move_tail(&sibling->group_entry, list);
+
 		sibling->group_leader = sibling;
 
 		/* Inherit group flags from the previous leader */
 		sibling->group_caps = event->group_caps;
 
+		if (!RB_EMPTY_NODE(&event->group_node)) {
+			list_del_init(&sibling->group_entry);
+			add_event_to_groups(sibling, event->ctx);
+		}
+
 		WARN_ON_ONCE(sibling->ctx != event->ctx);
 	}
 
@@ -2180,6 +2352,22 @@ static int group_can_go_on(struct perf_event *event,
 	return can_add_hw;
 }
 
+static int
+flexible_group_sched_in(struct perf_event *event,
+			struct perf_event_context *ctx,
+		        struct perf_cpu_context *cpuctx,
+			int *can_add_hw)
+{
+	if (event->state <= PERF_EVENT_STATE_OFF || !event_filter_match(event))
+		return 0;
+
+	if (group_can_go_on(event, cpuctx, *can_add_hw))
+		if (group_sched_in(event, cpuctx, ctx))
+			*can_add_hw = 0;
+
+	return 1;
+}
+
 static void add_event_to_ctx(struct perf_event *event,
 			       struct perf_event_context *ctx)
 {
@@ -2646,6 +2834,7 @@ static void ctx_sched_out(struct perf_event_context *ctx,
 			  struct perf_cpu_context *cpuctx,
 			  enum event_type_t event_type)
 {
+	int sw = -1, cpu = smp_processor_id();
 	int is_active = ctx->is_active;
 	struct perf_event *event;
 
@@ -2694,12 +2883,20 @@ static void ctx_sched_out(struct perf_event_context *ctx,
 
 	perf_pmu_disable(ctx->pmu);
 	if (is_active & EVENT_PINNED) {
-		list_for_each_entry(event, &ctx->pinned_groups, group_entry)
+		perf_event_groups_for_each_cpu(event, cpu,
+				&ctx->pinned_groups, group_node)
+			group_sched_out(event, cpuctx, ctx);
+		perf_event_groups_for_each_cpu(event, sw,
+				&ctx->pinned_groups, group_node)
 			group_sched_out(event, cpuctx, ctx);
 	}
 
 	if (is_active & EVENT_FLEXIBLE) {
-		list_for_each_entry(event, &ctx->flexible_groups, group_entry)
+		perf_event_groups_for_each_cpu(event, cpu,
+				&ctx->flexible_groups, group_node)
+			group_sched_out(event, cpuctx, ctx);
+		perf_event_groups_for_each_cpu(event, sw,
+				&ctx->flexible_groups, group_node)
 			group_sched_out(event, cpuctx, ctx);
 	}
 	perf_pmu_enable(ctx->pmu);
@@ -2990,23 +3187,28 @@ static void cpu_ctx_sched_out(struct perf_cpu_context *cpuctx,
 ctx_pinned_sched_in(struct perf_event_context *ctx,
 		    struct perf_cpu_context *cpuctx)
 {
+	int sw = -1, cpu = smp_processor_id();
 	struct perf_event *event;
+	int can_add_hw;
+
+	perf_event_groups_for_each_cpu(event, sw,
+			&ctx->pinned_groups, group_node) {
+		can_add_hw = 1;
+		if (flexible_group_sched_in(event, ctx, cpuctx, &can_add_hw)) {
+			if (event->state == PERF_EVENT_STATE_INACTIVE)
+				perf_event_set_state(event,
+						PERF_EVENT_STATE_ERROR);
+		}
+	}
 
-	list_for_each_entry(event, &ctx->pinned_groups, group_entry) {
-		if (event->state <= PERF_EVENT_STATE_OFF)
-			continue;
-		if (!event_filter_match(event))
-			continue;
-
-		if (group_can_go_on(event, cpuctx, 1))
-			group_sched_in(event, cpuctx, ctx);
-
-		/*
-		 * If this pinned group hasn't been scheduled,
-		 * put it in error state.
-		 */
-		if (event->state == PERF_EVENT_STATE_INACTIVE)
-			perf_event_set_state(event, PERF_EVENT_STATE_ERROR);
+	perf_event_groups_for_each_cpu(event, cpu,
+			&ctx->pinned_groups, group_node) {
+		can_add_hw = 1;
+		if (flexible_group_sched_in(event, ctx, cpuctx, &can_add_hw)) {
+			if (event->state == PERF_EVENT_STATE_INACTIVE)
+				perf_event_set_state(event,
+						PERF_EVENT_STATE_ERROR);
+		}
 	}
 }
 
@@ -3014,25 +3216,19 @@ static void cpu_ctx_sched_out(struct perf_cpu_context *cpuctx,
 ctx_flexible_sched_in(struct perf_event_context *ctx,
 		      struct perf_cpu_context *cpuctx)
 {
+	int sw = -1, cpu = smp_processor_id();
 	struct perf_event *event;
 	int can_add_hw = 1;
 
-	list_for_each_entry(event, &ctx->flexible_groups, group_entry) {
-		/* Ignore events in OFF or ERROR state */
-		if (event->state <= PERF_EVENT_STATE_OFF)
-			continue;
-		/*
-		 * Listen to the 'cpu' scheduling filter constraint
-		 * of events:
-		 */
-		if (!event_filter_match(event))
-			continue;
+	perf_event_groups_for_each_cpu(event, sw,
+			&ctx->flexible_groups, group_node)
+		flexible_group_sched_in(event, ctx, cpuctx, &can_add_hw);
+
+	can_add_hw = 1;
+	perf_event_groups_for_each_cpu(event, cpu,
+			&ctx->flexible_groups, group_node)
+		flexible_group_sched_in(event, ctx, cpuctx, &can_add_hw);
 
-		if (group_can_go_on(event, cpuctx, can_add_hw)) {
-			if (group_sched_in(event, cpuctx, ctx))
-				can_add_hw = 0;
-		}
-	}
 }
 
 static void
@@ -3113,7 +3309,7 @@ static void perf_event_context_sched_in(struct perf_event_context *ctx,
 	 * However, if task's ctx is not carrying any pinned
 	 * events, no need to flip the cpuctx's events around.
 	 */
-	if (!list_empty(&ctx->pinned_groups))
+	if (!RB_EMPTY_ROOT(&ctx->pinned_groups.tree))
 		cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE);
 	perf_event_sched_in(cpuctx, ctx, task);
 	perf_pmu_enable(ctx->pmu);
@@ -3350,8 +3546,12 @@ static void rotate_ctx(struct perf_event_context *ctx)
 	 * Rotate the first entry last of non-pinned groups. Rotation might be
 	 * disabled by the inheritance code.
 	 */
-	if (!ctx->rotate_disable)
-		list_rotate_left(&ctx->flexible_groups);
+	if (!ctx->rotate_disable) {
+		int sw = -1, cpu = smp_processor_id();
+
+		perf_event_groups_rotate(&ctx->flexible_groups, sw);
+		perf_event_groups_rotate(&ctx->flexible_groups, cpu);
+	}
 }
 
 static int perf_rotate_context(struct perf_cpu_context *cpuctx)
@@ -3698,8 +3898,8 @@ static void __perf_event_init_context(struct perf_event_context *ctx)
 	raw_spin_lock_init(&ctx->lock);
 	mutex_init(&ctx->mutex);
 	INIT_LIST_HEAD(&ctx->active_ctx_list);
-	INIT_LIST_HEAD(&ctx->pinned_groups);
-	INIT_LIST_HEAD(&ctx->flexible_groups);
+	perf_event_groups_init(&ctx->pinned_groups);
+	perf_event_groups_init(&ctx->flexible_groups);
 	INIT_LIST_HEAD(&ctx->event_list);
 	atomic_set(&ctx->refcount, 1);
 }
@@ -9370,6 +9570,7 @@ static void account_event(struct perf_event *event)
 	INIT_LIST_HEAD(&event->group_entry);
 	INIT_LIST_HEAD(&event->event_entry);
 	INIT_LIST_HEAD(&event->sibling_list);
+	init_event_group(event);
 	INIT_LIST_HEAD(&event->rb_entry);
 	INIT_LIST_HEAD(&event->active_entry);
 	INIT_LIST_HEAD(&event->addr_filters.list);
@@ -10880,7 +11081,7 @@ static int perf_event_init_context(struct task_struct *child, int ctxn)
 	 * We dont have to disable NMIs - we are only looking at
 	 * the list, not manipulating it:
 	 */
-	list_for_each_entry(event, &parent_ctx->pinned_groups, group_entry) {
+	perf_event_groups_for_each(event, &parent_ctx->pinned_groups, group_node) {
 		ret = inherit_task_group(event, parent, parent_ctx,
 					 child, ctxn, &inherited_all);
 		if (ret)
@@ -10896,7 +11097,7 @@ static int perf_event_init_context(struct task_struct *child, int ctxn)
 	parent_ctx->rotate_disable = 1;
 	raw_spin_unlock_irqrestore(&parent_ctx->lock, flags);
 
-	list_for_each_entry(event, &parent_ctx->flexible_groups, group_entry) {
+	perf_event_groups_for_each(event, &parent_ctx->flexible_groups, group_node) {
 		ret = inherit_task_group(event, parent, parent_ctx,
 					 child, ctxn, &inherited_all);
 		if (ret)

[toc] | [prev] | [next] | [standalone]

#1726589

On Tue, Sep 05, 2017 at 12:51:35AM -0700, Stephane Eranian wrote:
> >> Esp the cgroup stuff is entirely untested since I simply don't know how
> >> to operate that. I did run Vince's tests on it, and I think it doesn't
> >> regress, but I'm near a migraine so I can't really see straight atm.
> >>
> >> Vince, Stephane, could you guys have a peek?
> >>
> > okay, I will run some tests with cgroups on my systems.
> >
> I ran some cgroups tests, including multiplexing and so far it appears to work
> normally.

Shiny!

> It is easy to create a cgroup and move a shell into it:
> $ mount -t cgroup none /sys/fs/cgroups
> $ cd /sys/fs/cgroups/perf_events
> $ mkdir memtoy
> $ cd memtoy
> $ echo $$ >tasks
> 
> At this point your shell is part of the cgroup.
> Then you can use perf to monitor globally or inside the cgroup:
> 
> $ perf stat -a -e cycles,cycles -G memtoy -I 1000 sleep 1000
> 
> That monitors cycles on all CPUs twice, once only when a member
> of the cgroup memtoy runs, and the other globally.

Right, thanks!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web