Re: [PATCH v2] perf/core: Optimize event reschedule for a PMU

[Peter Zijlstra <peterz@infradead.org>]

On Mon, Aug 05, 2024 at 11:19:48PM -0700, Namhyung Kim wrote:
> On Mon, Aug 5, 2024 at 7:58 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Mon, Aug 05, 2024 at 11:20:58AM +0200, Peter Zijlstra wrote:
> > > On Fri, Aug 02, 2024 at 02:30:19PM -0400, Liang, Kan wrote:
> > > > > @@ -2792,7 +2833,14 @@ static int  __perf_install_in_context(void *info)
> > > > >   if (reprogram) {
> > > > >           ctx_sched_out(ctx, EVENT_TIME);
> > > > >           add_event_to_ctx(event, ctx);
> > > > > -         ctx_resched(cpuctx, task_ctx, get_event_type(event));
> > > > > +         if (ctx->nr_events == 1) {
> > > > > +                 /* The first event needs to set ctx->is_active. */
> > > > > +                 ctx_resched(cpuctx, task_ctx, NULL, get_event_type(event));
> > > > > +         } else {
> > > > > +                 ctx_resched(cpuctx, task_ctx, event->pmu_ctx->pmu,
> > > > > +                             get_event_type(event));
> > > > > +                 ctx_sched_in(ctx, EVENT_TIME);
> > > >
> > > > The changelog doesn't mention the time difference much. As my
> > > > understanding, the time is shared among PMUs in the same ctx.
> > > > When perf does ctx_resched(), the time is deducted.
> > > > There is no problem to stop and restart the global time when perf
> > > > re-schedule all PMUs.
> > > > But if only one PMU is re-scheduled while others are still running, it
> > > > may be a problem to stop and restart the global time. Other PMUs will be
> > > > impacted.
> > >
> > > So afaict, since we hold ctx->lock, nobody can observe EVENT_TIME was
> > > cleared for a little while.
> > >
> > > So the point was to make all the various ctx_sched_out() calls have the
> > > same timestamp. It does this by clearing EVENT_TIME first. Then the
> > > first ctx_sched_in() will set it again, and later ctx_sched_in() won't
> > > touch time.
> > >
> > > That leaves a little hole, because the time between
> > > ctx_sched_out(EVENT_TIME) and the first ctx_sched_in() gets lost.
> > >
> > > This isn't typically a problem, but not very nice. Let me go find an
> > > alternative solution for this. The simple update I did saturday is
> > > broken as per the perf test.
> >
> > OK, took a little longer than I would have liked, nor is it entirely
> > pretty, but it seems to pass 'perf test'.
> >
> > Please look at: queue.git perf/resched
> >
> > I'll try and post it all tomorrow.
> 
> Thanks for doing this.  But some of my tests are still failing.
> I'm seeing some system-wide events are not counted.
> Let me take a deeper look at it.

Does this help? What would be an easy reproducer?

---
diff --git a/kernel/events/core.c b/kernel/events/core.c
index c67fc43fe877..4a04611333d9 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -179,23 +179,27 @@ static void perf_ctx_lock(struct perf_cpu_context *cpuctx,
 	}
 }
 
+static inline void __perf_ctx_unlock(struct perf_event_context *ctx)
+{
+	/*
+	 * If ctx_sched_in() didn't again set any ALL flags, clean up
+	 * after ctx_sched_out() by clearing is_active.
+	 */
+	if (ctx->is_active & EVENT_FROZEN) {
+		if (!(ctx->is_active & EVENT_ALL))
+			ctx->is_active = 0;
+		else
+			ctx->is_active &= ~EVENT_FROZEN;
+	}
+	raw_spin_unlock(&ctx->lock);
+}
+
 static void perf_ctx_unlock(struct perf_cpu_context *cpuctx,
 			    struct perf_event_context *ctx)
 {
-	if (ctx) {
-		/*
-		 * If ctx_sched_in() didn't again set any ALL flags, clean up
-		 * after ctx_sched_out() by clearing is_active.
-		 */
-		if (ctx->is_active & EVENT_FROZEN) {
-			if (!(ctx->is_active & EVENT_ALL))
-				ctx->is_active = 0;
-			else
-				ctx->is_active &= ~EVENT_FROZEN;
-		}
-		raw_spin_unlock(&ctx->lock);
-	}
-	raw_spin_unlock(&cpuctx->ctx.lock);
+	if (ctx)
+		__perf_ctx_unlock(ctx);
+	__perf_ctx_unlock(&cpuctx->ctx.lock);
 }
 
 #define TASK_TOMBSTONE ((void *)-1L)