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

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

Hi Peter,

> > > This doesn't make sense; e1 should never reach event_sched_in() and it
> > > should already be INACTIVE.
> > >
> > > Notable events are created INACTIVE when !attr->disabled.
> >
> > But in perf stat code, it via enable_counter(), so it's set with
> > INACTIVE.
>
> your text above references ctx_sched_in(), what you're now saying is
> __perf_event_enable(); *that* will indeed set INACTIVE, but it will then
> also fail event_filter_match() and never even reschedule.
>
> > > Also, scheduling should not get beyond merge_sched_in()'s
> > > event_filter_match(), which will find the CPU is a mismatch and stop
> > > right there.
> > >
> > > This also means the event (e1) does not get to go on flexible_active
> > > (see below).
> >
> > No, when perf stat command with above, the cpu sets as == -1,
> > So, It doesn't filter out in event_filter_match(). so it enter into
> > merge_sched_in() and get into event_sched_in().
>
> Hurmph, I thought the hybrid stuff used to set CPU.
>
> Let me try and remember how the hybrid stuff works again. Ah
> pmu::filter(), that's called in visit_groups_merge() and should stop
> right there if the PMU doesn't work on that CPU.
>
> Is your hybrid PMu not set up right?

Oh, I miss it for filter. Thanks correct this.
But this is not related to this error.
The error occurs at "task exit" -- failured to account last enable time
for inactive event at exit.

> > > > To address this, update total_enable_time in event_sched_out() when event state
> > > > is PERF_EVENT_STATE_INACTIVE.
> > >
> > > This is a giant jump that I'm not following. Notably ctx_sched_out()
> > > will only iterate pmu_ctx->{pinned,flexible}_active and that list should
> > > only include ACTIVE events.
> > > So how does handling INACTIVE in event_sched_out() even begin to help?
> >
> > the answer is in the perf_event_exit_event()'s
> > perf_remove_from_context(). in here
> > event_sched_out() is called via __perf_remove_from_context()
> > So above case, the enable time is fixed in here.
>
> 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)

Thanks