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

Liang, Kan
2025-03-14T14:38:32Z

On 2025-03-14 9:50 a.m., Liang, Kan wrote:
>
>
> On 2025-03-14 8:59 a.m., Peter Zijlstra wrote:
>> On Thu, Mar 13, 2025 at 05:02:38PM -0700, kan.liang@linux.intel.com wrote:
>>
>>> @@ -12551,6 +12813,14 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
>>> if (err)
>>> return ERR_PTR(err);
>>>
>>> + if (event->attach_state & PERF_ATTACH_TASK_DATA) {
>>> + err = attach_perf_ctx_data(event);
>>> + if (err) {
>>> + security_perf_event_free(event);
>>> + return ERR_PTR(err);
>>> + }
>>> + }
>>> +
>>> /* symmetric to unaccount_event() in _free_event() */
>>> account_event(event);
>>>
>>
>> This seems weird. We just pushed all the error handling into
>> __free_event(), and now you're adding one special case back.
>>
>> Also, you've placed it after security_perf_event_alloc(), which I
>> _think_ wants to be last.
>
> Right, I will send a new series to fix it.
>

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);
+ }
+
/*
* Disallow uncore-task events. Similarly, disallow uncore-cgroup
* events (they don't make sense as the cgroup will be different

Thanks,
Kan