Re: [PATCH 1/1] perf/core: fix dangling cgroup pointer in cpuctx

[David Wang]

At 2025-06-03 14:34:59, "Yeoreum Yun" <yeoreum.yun@xxxxxxx> wrote:
>Hi David,
>> > attach_state doesn't related for event->state change.
>> > if one event already cleared PERF_ATTACH_CONTEXT, that event is called
>> > via list_del_event()
>>
>> Maybe this concern could be clarified, what about other subtle impacts.
>> The change should be thorough reviewed, if you want to push it further.
>>
>> It takes me more than a month to figure out a procedure to reproduce the kernel panic bug,
>> It is  just very hard to capture a bug happens in rare situation.
>>
>> And your patch has a global impact, it changes behavior unnecessarily.
>
>TBH, this patch just change of time of "event->state" while doing,
>As my bad miss the disable cgorup perf.
>I think there seems no other side effect for chaning state while in
>removing event.
>But, Let's wait for other people's review.
>
>> >
>> > Also, your patch couldn't solve a problem describe in
>> > commit a3c3c6667("perf/core: Fix child_total_time_enabled accounting bug at task exit")
>> > for INCATIVE event's total_enable_time.
>>
>> I do not think so.
>> Correct me if I am making silly  mistakes,
>> The patch, https://lore.kernel.org/lkml/20250603032651.3988-1-00107082@xxxxxxx/
>> calls perf_event_set_state() based on DETACH_EXIT flag, which cover the INACTIVE state, right?
>> If DETACH_EXIT is not used for this purpose? Then why should it exist at the first place?
>> I think I does not revert the purpose of commit a3c3c6667.....But I could be wrong
>> Would you show a call path where DETACH_EXIT is not set, but the changes in commit a3c3c6667 is still needed?
>
>Sorry for my bad explaination without detail.
>Think about cpu specific event and closed by task.
>If there is specific child cpu event specified in cpu 0.
>  1. cpu 0 -> active
>  2. scheulded to cpu1 -> inactive
>  3. close the cpu event from parent -> inactive close
>
>Can be failed to count total_enable_time.

Is this explaining the purpose of commit a3c3c6667 ?
I am not arguing with it. And I also not suggest reverting it. (it is just that reverting it can fix the kernel panic.)

>
>Thanks.
>--
>Sincerely,
>Yeoreum Yun