Re: [PATCH v6 2/3] perf/core: Run sched_task() for PMUs with only CPU-wide events

On Fri, Aug 7, 2026 at 11:08 AM Peter Zijlstra <[email protected]> wrote:
>
> On Thu, Aug 06, 2026 at 06:52:22AM -0700, Puranjay Mohan wrote:
> > perf_pmu_sched_task() returns early when cpuctx->task_ctx is set and
> > leaves the work to perf_ctx_sched_task_cb(). That one only walks
> > ctx->pmu_ctx_list, so a PMU whose events are all CPU-wide is never
> > visited and its sched_task() callback does not run. With
> >
> >   perf record -b -e cycles -a -- ls
> >
> > armv8pmu_sched_task() is skipped on every switch to a task that has a
> > perf event of its own, and BRBE records leak across the task boundary.
> > intel_pmu_lbr_add() calls perf_sched_cb_inc() unconditionally as well,
> > so LBR records leak the same way on x86.
> >
> > Drop the early return and instead skip the individual CPCs that
> > perf_ctx_sched_task_cb() already handles.
> >
> > That requires the two to agree on which CPC belongs to which path, and
> > they do not. perf_ctx_sched_task_cb() gates on cpc->sched_cb_usage,
> > which perf_sched_cb_inc() sets per CPU for every branch stack user,
> > while the new gate uses cpc->task_epc, which __link_epc() sets only on
> > the CPU the task context is scheduled in on. A task with an event for
> > that PMU pinned to another CPU has an epc on ctx->pmu_ctx_list while
> > cpc->task_epc is NULL, so both paths would run and sched_task() would be
> > called twice per context switch. On x86 the second
> > __intel_pmu_lbr_restore() finds lbr_stack_state == LBR_NONE and calls
> > intel_pmu_lbr_reset(), throwing away the callstack the first one
> > restored. So gate perf_ctx_sched_task_cb() on cpc->task_epc too.
> >
> > For the CPCs that perf_pmu_sched_task() now handles, the callback no
> > longer runs inside the perf_ctx_disable() and perf_ctx_enable() pair in
> > perf_event_context_sched_in(). __perf_pmu_sched_task() disables the PMU
> > around the call itself, so the callback still runs with it disabled.
> >
> > Fixes: bd2756811766 ("perf: Rewrite core context handling")
> > Cc: [email protected]
> > Acked-by: Usama Arif <[email protected]>
> > Signed-off-by: Puranjay Mohan <[email protected]>
> > ---
> >  kernel/events/core.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 9815894b67e77..675dd05935f35 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -3757,6 +3757,9 @@ static void perf_ctx_sched_task_cb(struct perf_event_context *ctx,
> >       list_for_each_entry(pmu_ctx, &ctx->pmu_ctx_list, pmu_ctx_entry) {
> >               cpc = this_cpc(pmu_ctx->pmu);
> >
> > +             if (cpc->task_epc != pmu_ctx)
> > +                     continue;
>
> Why isn't this the inverse condition of the below? That is, we should
> call either this or the other, right?

It should be. There's only one epc per PMU on ctx->pmu_ctx_list, so
!= pmu_ctx here is really just "task_epc isn't set", and once the extra
term below goes the two are inverses.

>
> > +
> >               if (cpc->sched_cb_usage && pmu_ctx->pmu->sched_task)
> >                       pmu_ctx->pmu->sched_task(pmu_ctx, task, sched_in);
> >       }
> > @@ -3921,12 +3924,15 @@ static void perf_pmu_sched_task(struct task_struct *prev,
> >       struct perf_cpu_context *cpuctx = this_cpu_ptr(&perf_cpu_context);
> >       struct perf_cpu_pmu_context *cpc;
> >
> > -     /* cpuctx->task_ctx will be handled in perf_event_context_sched_in/out */
> > -     if (prev == next || cpuctx->task_ctx)
> > +     if (prev == next)
> >               return;
> >
> > -     list_for_each_entry(cpc, this_cpu_ptr(&sched_cb_list), sched_cb_entry)
> > +     list_for_each_entry(cpc, this_cpu_ptr(&sched_cb_list), sched_cb_entry) {
> > +             if (cpuctx->task_ctx && cpc->task_epc)
>
> I'm not sure I see the need for the first part of that clause; why
> isn't: 'cpuc->task_epc' sufficient?

It is, task_epc can't be set without task_ctx. ctx_sched_in() sets
task_ctx first, ctx_sched_out() clears both, and the window in between
is under ctx->lock with IRQs off so the scheduler never lands in it.
That includes ctx_resched() scheduling out a single pmu, which leaves it
open the longest.

Will fix both in v7.

Thanks,
Puranjay