Re: [PATCH v4] events/core: fix acoount failure for event's child_total_enable_time at task exit
From: Leo Yan
Date: Tue Mar 25 2025 - 04:43:34 EST
On Tue, Mar 25, 2025 at 07:39:03AM +0000, Yeoreum Yun wrote:
[...]
> > > After this patch, this problem is gone like:
> > >
> > > sudo ./perf stat -vvv -e armv8_pmuv3_0/event=0x08/ -e armv8_pmuv3_1/event=0x08/ -- stress-ng --pthread=2 -t 10s
> > > ...
> > > armv8_pmuv3_0/event=0x08/: 15396770398 32157963940 21898169000
> > > armv8_pmuv3_1/event=0x08/: 22428964974 32157963940 10259794940
> > >
> > > Performance counter stats for 'stress-ng --pthread=2 -t 10s':
> > >
> > > 15,396,770,398 armv8_pmuv3_0/event=0x08/ (68.10%)
> > > 22,428,964,974 armv8_pmuv3_1/event=0x08/ (31.90%)
> > >
> > > Signed-off-by: Yeoreum Yun <yeoreum.yun@xxxxxxx>
> > > Suggsted-by: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> >
> > /Suggsted-by/Suggested-by/
>
> Thanks ;) I'll respin.
Given this patch is a fix, it is good to add a fix tag.
> > > ---
> > > kernel/events/core.c | 18 +++++++++---------
> > > 1 file changed, 9 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > > index 6364319e2f88..058533a50493 100644
> > > --- a/kernel/events/core.c
> > > +++ b/kernel/events/core.c
> > > @@ -2407,6 +2407,7 @@ ctx_time_update_event(struct perf_event_context *ctx, struct perf_event *event)
> > > #define DETACH_GROUP 0x01UL
> > > #define DETACH_CHILD 0x02UL
> > > #define DETACH_DEAD 0x04UL
> > > +#define DETACH_EXIT 0x08UL
> > >
> > > /*
> > > * Cross CPU call to remove a performance event
> > > @@ -2421,6 +2422,7 @@ __perf_remove_from_context(struct perf_event *event,
> > > void *info)
> > > {
> > > struct perf_event_pmu_context *pmu_ctx = event->pmu_ctx;
> > > + enum perf_event_state state = PERF_EVENT_STATE_OFF;
> > > unsigned long flags = (unsigned long)info;
> > >
> > > ctx_time_update(cpuctx, ctx);
> > > @@ -2429,16 +2431,19 @@ __perf_remove_from_context(struct perf_event *event,
> > > * Ensure event_sched_out() switches to OFF, at the very least
> > > * this avoids raising perf_pending_task() at this time.
> > > */
> > > - if (flags & DETACH_DEAD)
> > > + if (flags & DETACH_EXIT)
> > > + state = PERF_EVENT_STATE_EXIT;
> > > + if (flags & DETACH_DEAD) {
> > > event->pending_disable = 1;
> > > + state = PERF_EVENT_STATE_DEAD;
> > > + }
> > > event_sched_out(event, ctx);
> > > + perf_event_set_state(event, min(event->state, state));
> >
> > Nitpick: can we move perf_event_set_state() before event_sched_out()?
> >
> > So the function handles the state machine ahead, then proceed for
> > other operations.
>
> No It couldn't. IIUC, event_sched_out() disable pmu with ACTIVE state
> event only.
> If state is changed first from active state, it wouldn't be sched out by
> event_sched_out.
Indeed ! Please ignore my comment.
> > > if (flags & DETACH_GROUP)
> > > perf_group_detach(event);
> > > if (flags & DETACH_CHILD)
> > > perf_child_detach(event);
> > > list_del_event(event, ctx);
> > > - if (flags & DETACH_DEAD)
> > > - event->state = PERF_EVENT_STATE_DEAD;
> > >
> > > if (!pmu_ctx->nr_events) {
> > > pmu_ctx->rotate_necessary = 0;
> > > @@ -13424,12 +13429,7 @@ perf_event_exit_event(struct perf_event *event, struct perf_event_context *ctx)
> > > mutex_lock(&parent_event->child_mutex);
> > > }
> > >
> > > - perf_remove_from_context(event, detach_flags);
> > > -
> > > - raw_spin_lock_irq(&ctx->lock);
> > > - if (event->state > PERF_EVENT_STATE_EXIT)
> > > - perf_event_set_state(event, PERF_EVENT_STATE_EXIT);
> > > - raw_spin_unlock_irq(&ctx->lock);
> > > + perf_remove_from_context(event, detach_flags | DETACH_EXIT);
It is good to add a description in commit log for why remove the
code chunk for updating state in the function perf_event_exit_event().
As we discussed, it uses a central place __perf_remove_from_context()
to maintain the state when event exits, this can avoid race
condition. To support this, the 'DETACH_EXIT' flag is passed to
__perf_remove_from_context() instead.
Thanks,
Leo
> > >
> > > /*
> > > * Child events can be freed.
> > > --
> > > LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
> > >