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

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

On Fri, Dec 20, 2024 at 04:28:10PM +0100, Peter Zijlstra wrote:
> On Fri, Dec 20, 2024 at 04:14:14PM +0100, Peter Zijlstra wrote:
> > On Fri, Dec 20, 2024 at 02:05:39PM +0000, Yeoreum Yun wrote:
>
> > > 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..96a00034b26f 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -2406,7 +2406,8 @@ 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	0x04UL
> +#define DETACH_DEAD	0x048L
>
>  /*
>   * 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,20 @@ __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);
>  	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;
> +
> +	perf_event_set_state(event, min(event->state, state));
>
>  	if (!pmu_ctx->nr_events) {
>  		pmu_ctx->rotate_necessary = 0;
> @@ -13371,12 +13377,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);
>
>  	/*
>  	 * Child events can be freed.

Yes. I think this looks good to me to solve this error prints :)