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

Peter Zijlstra
2025-03-14T21:04:18Z

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.