Re: [PATCH V9 3/7] perf: attach/detach PMU specific data

[Peter Zijlstra]

On Fri, Mar 14, 2025 at 10:38:29AM -0400, Liang, Kan wrote:
> I think the attach_perf_ctx_data() should be moved even earlier, right
> after the perf_init_event(). Because the PERF_ATTACH_TASK_DATA is set in
> the event_init()->hw_config(), rather than after perf attach the data.
> 
> So any errors between perf_init_event() and attach_perf_ctx_data(event)
> would cause the detach_perf_ctx_data() mistakenly invoked.
> 
> @@ -5393,6 +5607,9 @@ static void __free_event(struct perf_event *event)
>  	if (is_cgroup_event(event))
>  		perf_detach_cgroup(event);
> 
> +	if (event->attach_state & PERF_ATTACH_TASK_DATA)
> +		detach_perf_ctx_data(event);
> +
>  	if (event->destroy)
>  		event->destroy(event);
> @@ -12481,6 +12746,18 @@ perf_event_alloc(struct perf_event_attr *attr,
> int cpu,
>  	if (IS_ERR(pmu))
>  		return (void*)pmu;
> 
> +	/*
> +	 * The PERF_ATTACH_TASK_DATA is set in the event_init()->hw_config().
> +	 * The attach should be right after the perf_init_event().
> +	 * Otherwise, the __free_event() would mistakenly detach the non-exist
> +	 * perf_ctx_data because of the other errors between them.
> +	 */
> +	if (event->attach_state & PERF_ATTACH_TASK_DATA) {
> +		err = attach_perf_ctx_data(event);
> + 		if (err)
> +			return ERR_PTR(err);
> +	}
> +

AFAICT it is possible for perF_init_event() to return an error *and* set
ATTACH_TASK_DATA, at which point we're up a creek with __free_event().

I'm afraid we need more state to track this.

(additionally, I'll need to rebase the pmu_unregister patch on top of
this -- that needs more changes still).

Anyway, let me go poke at this a bit, see what I can do with v10.

I'll reply there.