Re: [PATCH v2] events/core: fix acoount failure for event's total_enable_time

[Yeoreum Yun <yeoreum.yun@arm.com>]

Hi Peter.

> On Fri, Dec 20, 2024 at 03:26:42PM +0000, Yeoreum Yun wrote:
>
> > > OK, how's this then?
> > >
> > > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > > index 065f9188b44a..d12b402f9751 100644
> > > --- a/kernel/events/core.c
> > > +++ b/kernel/events/core.c
> > > @@ -2422,6 +2422,7 @@ __perf_remove_from_context(struct perf_event *event,
> > >  {
> > >  	struct perf_event_pmu_context *pmu_ctx = event->pmu_ctx;
> > >  	unsigned long flags = (unsigned long)info;
> > > +	enum perf_event_state state = PERF_EVENT_STATE_OFF;
> > >
> > >  	ctx_time_update(cpuctx, ctx);
> > >
> > > @@ -2438,7 +2439,9 @@ __perf_remove_from_context(struct perf_event *event,
> > >  		perf_child_detach(event);
> > >  	list_del_event(event, ctx);
> > >  	if (flags & DETACH_DEAD)
> > > -		event->state = PERF_EVENT_STATE_DEAD;
> > > +		state = PERF_EVENT_STATE_DEAD;
> > > +
> > > +	perf_event_set_state(event, state);
> > >
> >
> > It works. but what about this?
> >
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 065f9188b44a..71ed8f847b04 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -2432,6 +2432,7 @@ __perf_remove_from_context(struct perf_event *event,
> >         if (flags & DETACH_DEAD)
> >                 event->pending_disable = 1;
> >         event_sched_out(event, ctx);
> > +       perf_event_update_time(event);
> >         if (flags & DETACH_GROUP)
> >                 perf_group_detach(event);
> >         if (flags & DETACH_CHILD)
>

This patch doesn't work when the event is child event.
In case of parent's event, when you see the list_del_event(),
the total_enable_time is updated properly by changing state with
PERF_EVENT_STATE_OFF.

However, child event's total_enable_time is added before list_del_event.
So, the missing total_enable_time isn't added to parents event and the
error print happens.

So, I think it wouldn't be possible to update time with set_state.
instead I think it should update total_enable_time before
child's total_enable_time is added to parents' child_total_enable_time

like

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 065f9188b44a..d27717c44924 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -13337,6 +13337,7 @@ static void sync_child_event(struct perf_event *child_event)
        }

        child_val = perf_event_count(child_event, false);
+       perf_event_update_time(child_event);

        /*
         * Add back the child's count to the parent's count:

Thanks.