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

["Liang, Kan" <kan.liang@linux.intel.com>]

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